Resolve missing URI scheme issue

[jodh-intel]

Writing tests for confidential-containers/attestation-agent#29 uncovered the fact that the KBS URI is not technically a URI since it doesn't include a scheme.

We need to resolve which scheme is appropriate. We're using ttrpc so is it http, https, h2c, h2, other?

A minimal diff once we've decided is something like:

diff --git a/Cargo.toml b/Cargo.toml
index 1da653a..5200469 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -22,6 +22,7 @@ lazy_static = "1.4.0"
 string-error = "0.1.0"
 foreign-types = "0.5.0"
 openssl = { version = "0.10", optional = true, features = ["vendored"]}
+url = "2.2.2"
 
 [build-dependencies]
 tonic-build = "0.5"
diff --git a/src/grpc/mod.rs b/src/grpc/mod.rs
index 51820bf..dec2e94 100644
--- a/src/grpc/mod.rs
+++ b/src/grpc/mod.rs
@@ -28,6 +28,7 @@ const ERR_ANNOTATION_NOT_BASE64: &str = "annotation is not base64 encoded";
 const ERR_DC_EMPTY: &str = "missing Dc value";
 const ERR_KBC_KBS_NOT_BASE64: &str = "KBC/KBS pair not base64 encoded";
 const ERR_KBC_KBS_NOT_FOUND: &str = "KBC/KBS pair not found";
+const ERR_KBS_URI_INVALID: &str = "KBS URI is invalid";
 const ERR_NO_KBC_NAME: &str = "missing KBC name";
 const ERR_NO_KBS_URI: &str = "missing KBS URI";
 const ERR_WRONG_DC_PARAM: &str = "Dc parameter not destined for agent";
@@ -222,6 +223,9 @@ fn str_to_kbc_kbs(value: &str) -> Result<(String, String)> {
             return Err(anyhow!(ERR_NO_KBS_URI));
         }
 
+        let _ =
+            url::Url::parse(kbs_uri).map_err(|e| anyhow!("{}: {:?}", ERR_KBS_URI_INVALID, e))?;
+
         Ok((kbc_name.to_string(), kbs_uri.to_string()))
     } else {
         Err(anyhow!(ERR_KBC_KBS_NOT_FOUND))
@@ -688,6 +692,10 @@ mod tests {
                 value: "::foo",
                 result: Err(anyhow!(ERR_NO_KBC_NAME)),
             },
+            TestData {
+                value: "foo::bar",
+                result: Err(anyhow!(ERR_KBS_URI_INVALID)),
+            },
             TestData {
                 value: "foo::https://foo.bar.com/?silly=yes&colons=bar:::baz:::wibble::",
                 result: Ok((

[jodh-intel]

Related: https://github.com/containerd/ttrpc#differences-from-grpc.

[dubek]

As far as I understand we're currently using grpc but planning to change to ttrpc once all other parties support it.

If I look at the sample_kbs code (which the AA connects to using KBS URI), it is a grpc server implemented by tonic::transport::Server. Its docs say:

This builder exposes easy configuration parameters for providing a fully featured http2 based gRPC server. This should provide a very good out of the box http2 server for use with tonic but is also a reference implementation that should be a good starting point for anyone wanting to create a more complex and/or specific implementation.

So, at least for now, the KBS is an http2 server (exposing an underlying grpc protocol). So I think the correct URI scheme is http:// or https://. The http2 RFC says:

HTTP/2 uses the same "http" and "https" URI schemes used by HTTP/1.1.
HTTP/2 shares the same default port numbers: 80 for "http" URIs and
443 for "https" URIs. As a result, implementations processing
requests for target resource URIs like "http://example.org/foo" or
"https://example.com/bar" are required to first discover whether the
upstream server (the immediate peer to which the client wishes to
establish a connection) supports HTTP/2.

That still leaves an open question of what should be the URI scheme after we modify to ttrpc. Maybe we can see what other ttrpc-based projects did.

[jodh-intel]

afaik https://github.com/containerd/ttrpc-rust is compatible with https://github.com/containerd/ttrpc, and that states:

The protocol stack has been replaced with a lighter protocol that doesn't require http, http2 and tls.

[fitzthum]

The KBS_URI is passed to the KBC, which can do whatever it wants with it. We can validate that the URI is valid in the grpc module, but validating the scheme itself is probably best left to the KBC.

Currently we only have one KBC that actually uses this field. When we add this test, we'll need to fixup the EAA KBC as well. We'll also want to make sure that people know that they need to change their agent config file slightly.

We've been using the convention of passing null as the KBS_URI in the agent config for the offline KBCs. We might need some alternative if that doesn't pass the check.

[ariel-adam]

@jodh-intel is this issue still relevant or can be closed?
If it's still relevant to what release do you think we should map it to (mid-November, end-December, mid-February etc...)?