Extend KBS to provide the resources required to create an encrypted overlay network #451

cclaudio · 2024-07-31T04:34:39Z

This PR requires confidential-containers/guest-components#634 to be merged first.

This PR resolves #396 by adding the following changes:

Add plugin interface
Add nebula-plugin

Steps to test it

enable nebula in the kbs-config.toml: enabled_plugins = ["nebula"]
Generate User authentication key pair as in quickstart guide
docker compose build
docker compose up
build kbs client: cd kbs && ATTESTER=snp-attester make cli && sudo make install-cli
kbs-client --url http://127.0.0.1:8080 config --auth-private-key kbs/config/private.key set-resource-policy --policy-file kbs/sample_policies/allow_all.rego
kbs-client --url http://127.0.0.1:8080 get-resource --path 'plugin/nebula/credential?ip[ip]=10.11.12.13&ip[netbits]=21&name=pod1' | base64 -d

The last command should return the credential requested, e.g.:

{
"node_crt":[45,45,45,45,45,66,69,71,73,78,32,78,69,66,85,76,65,32,67,69,82,84,73,70,73,67,65,84,69,45,45,45,45,45,10,67,109,69,75,66,72,66,118,90,68,69,83,67,89,50,89,114,70,67,65,56,80,47,47,68,121,106,83,117,113,83,49,66,106,67,97,49,113,88,69,66,106,111,103,83,85,66,117,100,74,104,101,84,75,105,49,122,50,86,66,77,102,112,66,10,68,103,105,120,77,56,87,81,90,55,97,81,109,99,114,68,114,53,52,53,47,50,74,75,73,68,116,87,73,57,109,43,70,98,120,80,119,107,52,54,122,71,90,88,116,56,70,103,112,74,52,71,52,120,88,57,76,97,108,56,78,55,98,116,10,120,65,43,68,69,107,65,76,110,114,80,88,87,88,116,51,113,102,77,109,87,48,102,110,68,88,97,111,48,90,104,112,108,54,65,104,49,115,82,47,115,115,120,120,80,56,109,99,108,99,78,87,101,110,47,76,82,84,68,48,112,101,122,68,10,105,101,76,82,75,79,70,109,121,104,111,74,107,118,66,73,86,109,53,82,104,54,43,56,68,89,115,68,10,45,45,45,45,45,69,78,68,32,78,69,66,85,76,65,32,67,69,82,84,73,70,73,67,65,84,69,45,45,45,45,45,10],
"node_key":[45,45,45,45,45,66,69,71,73,78,32,78,69,66,85,76,65,32,88,50,53,53,49,57,32,80,82,73,86,65,84,69,32,75,69,89,45,45,45,45,45,10,79,69,103,121,70,88,106,101,107,97,78,115,50,98,111,122,56,80,101,68,69,112,51,113,82,51,47,114,89,120,67,82,79,79,57,89,70,49,103,109,118,102,69,61,10,45,45,45,45,45,69,78,68,32,78,69,66,85,76,65,32,88,50,53,53,49,57,32,80,82,73,86,65,84,69,32,75,69,89,45,45,45,45,45,10],
"ca_crt":[45,45,45,45,45,66,69,71,73,78,32,78,69,66,85,76,65,32,67,69,82,84,73,70,73,67,65,84,69,45,45,45,45,45,10,67,107,115,75,71,85,53,108,89,110,86,115,89,83,66,68,81,83,66,109,98,51,73,103,86,72,74,49,99,51,82,108,90,83,66,76,81,108,77,111,109,43,43,103,116,81,89,119,109,57,97,108,120,65,89,54,73,76,54,55,105,73,115,79,10,51,83,68,71,76,67,49,84,121,54,78,82,77,121,48,57,56,56,84,110,53,51,77,47,71,88,112,80,77,84,99,57,66,43,85,73,81,65,69,83,81,80,75,67,107,97,99,72,79,89,86,121,66,104,57,69,102,106,74,107,65,110,48,72,10,81,73,72,102,114,106,51,83,76,48,67,118,49,77,81,109,81,98,107,55,99,89,116,81,70,82,114,111,104,57,51,104,121,55,99,81,55,112,51,99,56,105,86,110,67,56,109,80,97,107,113,70,47,101,66,84,48,82,111,67,69,119,111,61,10,45,45,45,45,45,69,78,68,32,78,69,66,85,76,65,32,67,69,82,84,73,70,73,67,65,84,69,45,45,45,45,45,10]
}

cclaudio · 2024-07-31T04:38:02Z

Once confidential-containers/guest-components#634 is merged, I can update this PR and remove its draft label.

fitzthum

Looks good. A few preliminary comments. I haven't gotten into the meat of the nebula.rs yet

kbs/src/resource/plugin/mod.rs

kbs/src/resource/plugin/nebula.rs

Move the kbs-config.toml to its own directory. Other kbs related config files can then be stored under the same directory. Signed-off-by: Claudio Carvalho <[email protected]>

Currently, a resource is requested to the KBS through the resource URI, which takes the resource description in the format '/<repository>/<type>/<tag>'. This patch re-purposes the repository='plugin' to allow getting resources through plugins. A plugin can take additional parameters via a URL query string and can also do some processing before returning the resource. When the repository='plugin', the get-resource() interface will then call the plugin manager to handle the request, where: - the type provided is the name of the plugin to be invoked - the tag provided is the name of the resource requested - additional parameters can be provided through query string The example below shows a plugin resource request for the nebula plugin where the resource name is credential. Additional parameters are provided in the query string. GET /kbs/v0/resource/plugin/nebula/credential?ip[ip]=10.11.12.13&ip[netbits]=21&name=pod1 set-resource() is not supported when repository='plugin'. Plugins are required to implement two traits, both defined by the plugin manager: - PluginBuild: functions required to create/initialize a plugin if it is included in the 'plugin_manager_config.enabled_plugins' in the kbs-config.toml. - Plugin: functions required to invoke the plugin that will handle a get-resource request received. Signed-off-by: Claudio Carvalho <[email protected]>

…orage The current resource storage path /opt/confidential-containers/kbs/repository is misleading because repository is actually part of the resource description (repository/type/tag). Signed-off-by: Claudio Carvalho <[email protected]>

The nebula plugin can be used to deliver credentials for nodes (confidential PODs or VMs) to join a Nebula overlay network. Within the nebula network, the communication between nodes is automatically encrypted by Nebula. A nebula credential can be requested using the kbs-client: kbs-client --url http://127.0.0.1:8080 \ get-resource \ --path 'plugin/nebula/credential?ip[ip]=10.11.12.13&ip[netbits]=21&name=pod1' at least the IPv4 address (in CIDR notation) and the name of the node must be provided in the query string. The other parameters supported can be found in the struct NebulaCredentialParams. After receiving a credential request, the nebula plugin will call the nebula-cert binary to create a key pair and sign a certificate using the Nebula CA. The generated node.crt and node.key, as well as the ca.rt are then returned to the caller. During the nebula-plugin initialization, a self signed Nebula CA can be created if 'ca_generation_policy = 1' in the nebula-config.toml, the file contains all parameters supported. Another option is to pre-install a ca.key and ca.crt, and set 'ca_generation_policy = 2'. The nebula-plugin cargo feature is set by default, however the plugin itself is not initialized by default. In order to initialize it, you need to add 'nebula' to 'manager_plugin_config.enabled_plugins' in the kbs-config.toml. Closes confidential-containers#396 Signed-off-by: Claudio Carvalho <[email protected]>

cclaudio · 2024-08-05T16:49:30Z

Feedback applied, thanks @fitzthum

I also updated the guest-components revision in the Cargo.toml to point to latest, which includes the query string support required by this PR.

fitzthum

A few more comments

fitzthum · 2024-08-05T18:52:10Z

kbs/src/resource/plugin/mod.rs

+#[cfg(feature = "nebula-plugin")]
+use crate::resource::plugin::nebula::NebulaPluginConfig;
+
+trait PluginBuild {


maybe PluginBuilder would be better

fitzthum · 2024-08-05T18:55:54Z

kbs/src/resource/plugin/mod.rs

+                .with_context(|| "Create resource plugin dir".to_string())?;
+        }
+
+        #[allow(unused_mut)]


Do you need this? Seems like you modify the manager below. Btw you might also think about creating the vector first and then making the manager from it at the end.

fitzthum · 2024-08-05T19:01:30Z

kbs/src/resource/plugin/mod.rs

+                    plugin_name,
+                ))?;
+
+            let plugin_dir = format!("{}/{}", self.work_dir, builder.get_plugin_name());


Do you think there is any need to cleanup these directories? I guess when we're running in a container it isn't too significant.

fitzthum · 2024-08-05T19:14:27Z

kbs/src/resource/plugin/nebula.rs

+        }
+
+        match config.ca_generation_policy {
+            x if x == CaGenerationPolicy::GenerateIfNotFound as u32 => {


You don't use x for anything, so I don't see the point of x if x ==. Why not just check directly again the enum?

match config.ca_generation_policy { CaGenerationPolicy::GenerateIfNotFound => {

If you need to do some casting, you can do it in the first line

match config.ca_generation_policy as u32{ CaGenerationPolicy::GenerateIfNotFound => {

#11 132.6 70 | match config.ca_generation_policy { #11 132.6 | --------------------------- this expression has type `u32` #11 132.6 71 | CaGenerationPolicy::GenerateIfNotFound => { #11 132.6 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u32`, found `CaGenerationPolicy`

Rust does not allow casting in the left side of a match case. So we have to either convert config.ca_generation_policy to CaGenerationPolicy or use the expression trick x if x == in the match case.

fitzthum · 2024-08-05T19:46:26Z

kbs/src/resource/plugin/nebula.rs

+
+        match config.ca_generation_policy {
+            x if x == CaGenerationPolicy::GenerateIfNotFound as u32 => {
+                if !ca.crt.exists() || !ca.key.exists() {


This is a really big match statement. It looks kind of weird to me. You might want to try nesting matches rather than using a bunch of if blocks inside the match. For instance you could turn this if into another match.

It might also be cleaner if you created a helper function to generate the cert.

Finally, you might think about switching the order of these conditions. It might be cleaner to first cleanup incomplete state (which you should do in any case). Then check if the cert is there and then if it isn't there, check what the regeneration policy is.

if the key/cert need to be generated and then check the CaGenerationPolicy

Yeah, makes sense. I will create a helper function to generate the cert. Thanks.

fitzthum · 2024-08-05T19:47:23Z

kbs/src/resource/plugin/nebula.rs

+                }
+            }
+            x => {
+                bail!("CaGenerationPolicy {x} not supported");


Something is definitely fishy with this match statement. If you implement a match on an enum and the enum only has two values, you won't need this kind of fallback case.

I can convert config.ca_generation_policy to CaGenerationPolicy outside of the match to avoid this fallback.

fitzthum · 2024-08-05T20:04:17Z

kbs/src/resource/plugin/nebula.rs

+
+    pub fn test_all(&self) -> Result<()> {
+        self.test_nebula_cert_sign()
+    }


Do you call this test anywhere?

It's pretty common to put test code in a separate module rather than implementing it alongside the real methods. Not sure it's a big deal.

It is called when the plugin is initialized. It runs simple tests to check if the provided nebula-cert is working.

fitzthum · 2024-08-05T20:07:43Z

kbs/src/resource/plugin/nebula.rs

+    }
+
+    /// Run "nebula-cert sign" to generate a credential
+    pub fn generate(


maybe call this generate_credential

fitzthum · 2024-08-05T20:08:21Z

kbs/src/resource/plugin/nebula.rs

+                    params.push("-out-key".into());
+                    params.push(ca.key.as_path().into());
+
+                    let status = Command::new(NEBULA_CERT_BIN)


Should we check if this binary actually exists before trying to use it?

Xynnn007

Thanks @cclaudio for this. I made some suggestions from a user perspective.

Xynnn007 · 2024-08-09T01:17:01Z

Cargo.toml

@@ -29,17 +29,18 @@ config = "0.13.3"
 env_logger = "0.10.0"
 hex = "0.4.3"
 jwt-simple = "0.11"
-kbs_protocol = { git = "https://github.com/confidential-containers/guest-components.git", rev="9bd6f06a9704e01808e91abde130dffb20e632a5", default-features = false }
+kbs_protocol = { git = "https://github.com/confidential-containers/guest-components.git", rev="0d08cccf3c72647de273fee90716d6842b9ddcfd", default-features = false }


We should also update the version of kms as they come from the same repo.

Xynnn007 · 2024-08-09T01:36:27Z

kbs/src/resource/local_fs.rs

@@ -7,7 +7,7 @@ use anyhow::{Context, Result};
 use serde::Deserialize;
 use std::path::{Path, PathBuf};

-pub const DEFAULT_REPO_DIR_PATH: &str = "/opt/confidential-containers/kbs/repository";
+pub const DEFAULT_REPO_DIR_PATH: &str = "/opt/confidential-containers/kbs/storage";


This change will change the mount path inside KBS container. I am not sure if it will influence current CI settings.

cc @stevenhorsman @BbolroC @wainersm

Yes, this would force changes in a lot of CI and documentation across repos. I hope we can avoid changing that.

Xynnn007 · 2024-08-09T02:00:14Z

kbs/src/http/resource.rs

+    let resource_byte = if resource_description.repository_name == "plugin" {
+        repository_plugin
+            .read()
+            .await
+            .get_resource(
+                resource_description.resource_type.as_str(),
+                resource_description.resource_tag.as_str(),
+                request.query_string(),
+            )
+            .await
+            .map_err(|e| Error::ReadSecretFailed(e.to_string()))?
+    } else {
+        repository
+            .read()
+            .await
+            .read_secret_resource(resource_description)
+            .await
+            .map_err(|e| Error::ReadSecretFailed(e.to_string()))?
+    };


We also need to update the KBS PROTOCOL document under docs to show that plugin is reserved.

Xynnn007 · 2024-08-09T02:10:58Z

kbs/config/docker-compose/kbs-config.toml

+[plugin_manager_config]
+work_dir = "/opt/confidential-containers/kbs/plugin"
+enabled_plugins = []
+


This would have an overall workdir for all plugins. Maybe we should let every single plugin has its own configs, like the workdir of them own?

Some config format in my mind

insecure_http = true insecure_api = true [attestation_token_config] attestation_token_type = "CoCo" [grpc_config] as_addr = "http://127.0.0.1:50004" pool_size = 200 [repository_config] type = "LocalFs" dir_path = "/opt/confidential-containers/kbs/storage" [plugins.nebula] config_path = "/etc/kbs/plugin/nebula-config.toml" [plugins.another] config_a=xxx config_b=yyy

If only [plugins.nebula] is explicitly specified in the toml, nebula be initialized. In this way every single plugin could have its own config, or specify the config path inside that.

This could be implemented via

#[derive(Clone, Debug, Deserialize)] pub struct KbsConfig { ... pub plugins: Option<PluginManagerConfig>, } #[derive(Clone, Debug, Deserialize)] struct PluginManagerConfig { #[cfg(feature = "nebula-plugin")] nebula: Option<NebulaConfig>, #[cfg(feature = "another-plugin")] another: Option<AnotherPluginConfig>, } #[cfg(feature = "nebula-plugin")] #[derive(Clone, Debug, Deserialize)] struct NebulaConfig { config_path: String, } #[cfg(feature = "another-plugin")] #[derive(Clone, Debug, Deserialize)] struct AnotherPluginConfig { config_a: String, config_b: String, }

Xynnn007 · 2024-08-09T02:14:55Z

kbs/docker/coco-as-grpc/Dockerfile

@@ -2,18 +2,24 @@ FROM rust:latest as builder
 ARG ARCH=x86_64
 ARG HTTPS_CRYPTO=rustls
 ARG ALIYUN=false
+ARG PLUGINS="nebula-plugin"


I suggest that we leave default plugins empty. Also, we can add some guides in docs to show how to enable nebula when building KBS image.

For this we might need to add a new doc about plugins here.

Xynnn007 · 2024-08-09T02:15:56Z

kbs/src/http/resource.rs

+            .await
+            .read_secret_resource(resource_description)
+            .await
+            .map_err(|e| Error::ReadSecretFailed(e.to_string()))?


Suggested change

.map_err(|e| Error::ReadSecretFailed(e.to_string()))?

.map_err(|e| Error::ReadSecretFailed(format!("{e:?}")))?;

Xynnn007 · 2024-08-09T02:16:05Z

kbs/src/http/resource.rs

+                request.query_string(),
+            )
+            .await
+            .map_err(|e| Error::ReadSecretFailed(e.to_string()))?


Suggested change

.map_err(|e| Error::ReadSecretFailed(e.to_string()))?

.map_err(|e| Error::ReadSecretFailed(format!("{e:?}")))?;

mkulke · 2024-08-09T08:26:22Z

kbs/src/resource/local_fs.rs

@@ -7,7 +7,7 @@ use anyhow::{Context, Result};
 use serde::Deserialize;
 use std::path::{Path, PathBuf};

-pub const DEFAULT_REPO_DIR_PATH: &str = "/opt/confidential-containers/kbs/repository";
+pub const DEFAULT_REPO_DIR_PATH: &str = "/opt/confidential-containers/kbs/storage";


Yes, this would force changes in a lot of CI and documentation across repos. I hope we can avoid changing that.

mkulke · 2024-08-09T08:34:36Z

kbs/src/resource/plugin/mod.rs

+
+#[async_trait::async_trait]
+trait Plugin {
+    async fn get_name(&self) -> &str;


for pure accessor method it's more idiomatic to use the field name as fn. also why should it be async?

Suggested change

async fn get_name(&self) -> &str;

fn name(&self) -> &str;

mkulke · 2024-08-09T09:14:07Z

kbs/src/resource/plugin/mod.rs

+    enabled_plugins: Vec<String>,
+}
+
+impl PluginManagerConfig {


Are we sure the PluginManager/PluginManagerConfig/PluginBuilder abstractions are required? I am concerned they introduce accidental complexity without a use case. Afaict we don't really need to "manage" plugins, they are activated and configured at launch, and do not change at runtime.

IMO a plugin could be an implementation of this trait:

trait Plugin { async fn get_resource(&self, resource: &str, query_string: &str) -> Result<Vec<u8>>; }

when parsing KbsConfig a Plugin can be initialized and registered in a lookup map.

// we don't want to have dynamic strings for plugin lookup, they are known at compile-time const NEBULA: &str = "nebula"; ... // this needs to be in the app's state for lookup in the route handlers let mut enabled_plugins: BTreeMap<&'static str, Box<dyn Plugin + Send + Sync>>; ... let nebula_plugin = nebula::new(&kbs_config)?; enabled_plugins.insert(NEBULA, Box::new(nebula_plugin));

portersrc

I made an initial pass over the changes -- great work. A couple comments below:

portersrc · 2024-08-12T20:36:56Z

kbs/src/resource/plugin/mod.rs

+        resource: &str,
+        query_string: &str,
+    ) -> Result<Vec<u8>> {
+        for plugin in self.plugins.iter() {


Would a map work better than iterating over a vector here (or maybe there are only a handful of plugins anyway)?

portersrc · 2024-08-12T21:27:31Z

kbs/src/resource/plugin/nebula.rs

+        };
+
+        log::info!("nebula-cert binary: {}", ca.get_version()?.trim());
+        ca.test_all()?;


This line looks like you want to move it to a test section now

portersrc · 2024-08-12T21:33:57Z

kbs/src/resource/plugin/nebula.rs

+                            .with_context(|| format!("Remove {} file", ca.crt.display()))?;
+                    }
+                    if ca.key.exists() {
+                        fs::remove_file(ca.crt.as_path())


maybe paste mistake: ca.key

cclaudio · 2024-09-24T14:14:50Z

Thanks for all feedback!
I am making changes to this PR to support the plugin design discussed in #502 , thanks @Xynnn007 .

cclaudio requested a review from sameo as a code owner July 31, 2024 04:34

cclaudio marked this pull request as draft July 31, 2024 04:35

fitzthum reviewed Aug 2, 2024

View reviewed changes

cclaudio added 4 commits August 5, 2024 19:07

docker-composer: Move kbs-config.toml to /etc/kbs

341a318

Move the kbs-config.toml to its own directory. Other kbs related config files can then be stored under the same directory. Signed-off-by: Claudio Carvalho <[email protected]>

cclaudio force-pushed the plugin-preview branch from 49865eb to baf0bd6 Compare August 5, 2024 16:13

cclaudio marked this pull request as ready for review August 5, 2024 16:14

fitzthum reviewed Aug 5, 2024

View reviewed changes

Xynnn007 requested changes Aug 9, 2024

View reviewed changes

mkulke reviewed Aug 9, 2024

View reviewed changes

portersrc reviewed Aug 12, 2024

View reviewed changes

portersrc mentioned this pull request Aug 15, 2024

[RFC] Add mesh VPN support to the CDH confidential-containers/guest-components#674

Open

Xynnn007 mentioned this pull request Sep 18, 2024

Extend KBS to provide the resources required to create an encrypted overlay network #396

Open

Xynnn007 mentioned this pull request Sep 27, 2024

KBS | Refactoring the codebase / update config file format / bring in plugin mechanism #514

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend KBS to provide the resources required to create an encrypted overlay network #451

Extend KBS to provide the resources required to create an encrypted overlay network #451

cclaudio commented Jul 31, 2024 •

edited

Loading

cclaudio commented Jul 31, 2024

fitzthum left a comment

cclaudio commented Aug 5, 2024

fitzthum left a comment

fitzthum Aug 5, 2024

fitzthum Aug 5, 2024

fitzthum Aug 5, 2024

fitzthum Aug 5, 2024

cclaudio Sep 24, 2024

fitzthum Aug 5, 2024

cclaudio Sep 24, 2024

fitzthum Aug 5, 2024

cclaudio Sep 24, 2024

fitzthum Aug 5, 2024

cclaudio Sep 24, 2024

fitzthum Aug 5, 2024

fitzthum Aug 5, 2024

Xynnn007 left a comment

Xynnn007 Aug 9, 2024

Xynnn007 Aug 9, 2024

mkulke Aug 9, 2024

Xynnn007 Aug 9, 2024

Xynnn007 Aug 9, 2024 •

edited

Loading

Xynnn007 Aug 9, 2024

Xynnn007 Aug 9, 2024

Xynnn007 Aug 9, 2024

mkulke Aug 9, 2024

mkulke Aug 9, 2024

mkulke Aug 9, 2024

portersrc left a comment

portersrc Aug 12, 2024

portersrc Aug 12, 2024

portersrc Aug 12, 2024

cclaudio commented Sep 24, 2024 •

edited

Loading

	.map_err(\|e\| Error::ReadSecretFailed(e.to_string()))?
	.map_err(\|e\| Error::ReadSecretFailed(format!("{e:?}")))?;

Extend KBS to provide the resources required to create an encrypted overlay network #451

Are you sure you want to change the base?

Extend KBS to provide the resources required to create an encrypted overlay network #451

Conversation

cclaudio commented Jul 31, 2024 • edited Loading

Steps to test it

cclaudio commented Jul 31, 2024

fitzthum left a comment

Choose a reason for hiding this comment

cclaudio commented Aug 5, 2024

fitzthum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xynnn007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xynnn007 Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

portersrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cclaudio commented Sep 24, 2024 • edited Loading

cclaudio commented Jul 31, 2024 •

edited

Loading

Xynnn007 Aug 9, 2024 •

edited

Loading

cclaudio commented Sep 24, 2024 •

edited

Loading