Add splitapi resource plugin #585

salmanyam · 2024-11-19T19:44:21Z

This plugin generates credentials (keys and certificates) for both the API proxy server (required for
kata-containers/kata-containers#9159 and
kata-containers/kata-containers#9752) and the workload owner. This plugin also delivers the credentials to a sandbox (i.e., confidential PODs or VMs), specifically to the kata agent to initiate the SplitAPI proxy server so that a workload owner can communicate with the proxy server using a secure tunnel.

The IPv4 address, name, and the ID of the sandbox (i.e., pod) must be provided in the query string to obtain the credentials from the KBS.

After receiving the credential request, the splitapi plugin will create a key pair for the server and client and sign them using the self-signed CA. The generated credentials are stored in a hashmap with a unique key for each sandbox based on its name, ID, and IP address. SplitAPI plugin responds to a request from the sandbox by sending the server specific credentials (key, cert) along with the CA certificate. A request from workload owner gets the client specific credentials (key, cert) and CA's certificate.

The splitapi plugin itself is not built or initialized by default. To build or initialize it, following steps need to be followed.

How to build

Enable splitapi-plugin feature while building kbs.
Update the kbs/config/kbs-config.toml to enable the splitapi plugin.

$ make background-check-kbs POLICY_ENGINE=opa SPLITAPI_PLUGIN=true
$ cat >> kbs/config/kbs-config.toml << EOF
[[plugins]]
name = "splitapi"
type = "CertManager"
plugin_dir = "/opt/confidential-containers/kbs/plugin/splitapi"

credential_blob_filename = "certificates.json"

[plugins.certificate_details]
country = "AA"
state = "Default State"
locality = "Default City"
organization = "Default Organization"
org_unit = "Default Unit"

[plugins.certificate_details.ca]
common_name = "grpc-tls CA"
validity_days = 3650

[plugins.certificate_details.server]
common_name = "server"
validity_days = 180

[plugins.certificate_details.client]
common_name = "client"
validity_days = 180

Note: Only plugin_dir is mandatory for the plugin configuration. The rest fields will get default values in case those are not configured in the kbs-config.toml.

How to test

This plugin can be tested similarly to the nebula-ca plugin (#539). We need the kbs-client patch available in the branch https://github.com/cclaudio/trustee/tree/nebula-ca-plugin-test to build the kbs-client and test the plugin.

Once the kbs-client is built, use the kbs-client to make a request to KBS for generating and providing the server specific credentials.

$ cd kbs && make cli ATTESTER=snp-attester
$ sudo make install-cli
$ kbs-client --url http://127.0.0.1:8080 get-resource --plugin-name "splitapi" --resource-path "credential?id=3367353&ip=60.11.12.48&name=pod33" | base64 -d

KBS should return the server specific credentials such as server key, server certificates, and the certificate of the CA.

bpradipt · 2024-11-20T07:28:39Z

This plugin generates credentials (keys and certificates) for both the API proxy server (required for kata-containers/kata-containers#9159 and kata-containers/kata-containers#9752) and the workload owner. This plugin also delivers the credentials to a sandbox (i.e., confidential PODs or VMs), specifically to the kata agent to initiate the SplitAPI proxy server so that a workload owner can communicate with the proxy server using a secure tunnel.

The IPv4 address, name, and the ID of the sandbox must be provided in the query string to obtain the credential resources from the kbs.

After receiving the credential request, the splitapi plugin will create a key pair for the server and client and sign them using the self-signed CA. The generated ca.crt, server.crt, and server.key are stored in a directory specific to the sandbox (the caller) and returned to the caller. In addition, ca.key, client.key, and client.crt are also generated and stored to that particular directory specific to the sandbox (i.e., caller).

During the credential generation, a sandbox directory mapper creates a unique directory specific to the sandbox (i.e., the caller). The mapper creates the unique directory using the sandbox parameters passed in the query string. A mapping file is also maintained to store the mapping between the sandbox name and the unique directory created for the sandbox.

The splitapi plugin itself is not initialized by default. To initialize it, you need to add 'splitapi' in the kbs-config.toml.

A generic question. Would it make sense to have a generic plugin to create key-pair for a sandbox and not club it with splitapi?
There are other use cases for per sandbox (pod or VM) keys especially in the peer-pods case.
cc @yoheiueda @davidhadas

fitzthum · 2024-11-20T15:10:15Z

A generic question. Would it make sense to have a generic plugin to create key-pair for a sandbox and not club it with splitapi?

Some kind of PKI plugin would be very useful.

salmanyam · 2024-11-20T15:18:36Z

A generic question. Would it make sense to have a generic plugin to create key-pair for a sandbox and not club it with splitapi?
There are other use cases for per sandbox (pod or VM) keys especially in the peer-pods case.

We are good. I think it would be a good idea to have the key generation as a basic service needed by other plugins. We can definitely consider refactoring and making it as a basic service. In that case, the splitapi will utilize this basic service.

fitzthum · 2024-11-20T17:33:45Z

@salmanyam a potential advantage of a generic PKI approach is that it wouldn't depend on any external patches in kata or wherever to be usable. I haven't look v closely at this PR yet but that's something to consider.

yoheiueda · 2024-11-21T08:23:14Z

A generic PKI approach will be very helpful for our peer pod use cases.

I I understand correctly, the current proposal is to run a key generation service in a trustee, but a PKI approach sounds to me a signing service in a trustee.

Trustee has a private key for CA, and accepts key signing requests from clients. A client in a sandbox generates a key pair, and send a key signing request with the generated public key. The CA in Trustee issues a certificate by singing the received public key using the CA private key, and distribute the issued certificate. I think this is how the current PKI for TLS certificates works, and the sandbox does not need to reveal its private key to Trustee.

I think this will work If the purpose of the certificate generation is TLS secure tunneling.

This is just out of my curiosity, and a key generation service will still very helpful for our peer pod use case.

Another point is that the current proposal generates RSA keys. To support other encryption mechanisms such as WireGuard and SSH, it is helpful to support different types of keys such as Ed25529. WireGuard uses Curve25529 keys, and Ed25529 is also preferable for SSH these days.

fitzthum

Made a few comments. Please run cargo fmt and cargo clippy to fix the CI.

fitzthum · 2024-11-21T15:54:23Z

kbs/src/plugins/implementations/splitapi/generator.rs

+
+        match self.generate_private_key(&self.ca_key, self.key_size) {
+            Ok(_) => println!("CA key generation succeeded and saved to {}.", self.ca_key.display()),
+            Err(e) => eprintln!("CA key generation failed: {}", e),


Please use info! and error! instead of printing.

I don't really like using a match here either. Are these errors fatal? Currently you continue with the function even if it fails. It might make more sense to call each generate function and pass the error up with a question mark. If you need to log something in the success case, you can just add that afterwards.

fitzthum · 2024-11-21T15:55:24Z

kbs/src/plugins/implementations/splitapi/generator.rs

+        let state = "Default State";
+        let locality = "Default City";
+        let organization = "Default Organization";
+        let org_unit = "Default Unit";


These should be updated presumably with something that says that it's coming from a Trustee plugin or you could have this be configurable.

fitzthum · 2024-11-21T15:59:07Z

kbs/src/plugins/implementations/splitapi/manager.rs

+impl SplitAPIBackend for CertManager {
+    async fn get_server_credential(&self, params: &SandboxParams) -> Result<Vec<u8>> {
+        // Try locking the sandbox directory mapper
+        let mut mapper_guard = self.mapper.lock().map_err(|e| {


Having a separate variable for the mapper_guard is a bit c-like. I think you can just use the mapper itself to hold the lock.

fitzthum · 2024-11-21T16:00:03Z

kbs/src/plugins/implementations/splitapi/manager.rs

+                sandbox_dir_info = existing_dir.clone();
+
+                //TODO: check if the credentails are already in there
+                // send the existing credentials if they are not expired


When you create the certs, just store the expiration time alongside them so you can easily see if they are expired.

fitzthum · 2024-11-21T16:08:47Z

kbs/src/plugins/implementations/splitapi/mapper.rs

I think it's a mistake to store all this stuff on the filesystem.

Here's what I would do. Create a struct that contains all the state that represents a keypair. OpenSSL has structs for all the things you need. Store this struct in a dictionary keyed by the connection id. Wrap the struct in a RwLock. If you need some kind of persistence you can serialize/desrialize the dictionary as needed.

I don't see any reason to be writing things to the filesystem and I think this entire mapper concept can be replaced by something from std, which decreases your complexity significantly.

fitzthum · 2024-11-21T16:12:39Z

kbs/src/plugins/implementations/splitapi/manager.rs

+        if let Some(mapper) = mapper_guard.as_mut() {
+            let sandbox_dir_info: SandboxDirectoryInfo;
+
+            if let Some(existing_dir) = mapper.get_directory(&params.name) {


These params are sent in other the network in plaintext. Would it be a problem if someone tampered with the name value in flight?

More fundamentally, why does this need to be stateful at all?

Xynnn007 · 2024-11-22T01:42:40Z

kbs/src/plugins/implementations/splitapi/backend.rs

+pub enum SplitAPIConfig {
+    CertManager(manager::SplitAPIRepoDesc),
+}


If we use enum here, we are hinting that potentially we could have other types of SplitAPIConfig besides CertManager. Do we? If not, I suggest to change SplitAPIConfig into struct and move members of SplitAPIRepoDesc directly into SplitAPIConfig

Xynnn007 · 2024-11-22T01:50:17Z

kbs/src/plugins/implementations/splitapi/manager.rs

+lazy_static! {
+    static ref SANDBOX_DIRECTORY_MAPPER: Arc<Mutex<Option<SandboxDirectoryMapper>>> = Arc::new(Mutex::new(None));
+}


Could we move this static variable directly into CertManager as a member? s.t.

pub struct CertManager { pub plugin_dir: String, pub mapping_filename: String, mapper: Arc<Mutex<SandboxDirectoryMapper>>, }

Also, could we use tokio::sync::Mutex rather than std::sync::Mutex? The async version would promote CPU efficiency.

Xynnn007 · 2024-11-22T01:51:40Z

kbs/src/plugins/implementations/splitapi/manager.rs

+        // Try locking the sandbox directory mapper
+        let mut mapper_guard = self.mapper.lock().map_err(|e| {
+            anyhow!("Failed to lock sandbox directory mapper: {}", e)
+        })?;


anyhow provides an easier way for map_err(|e| anyhow!(..., e)) like things

xxx.context("Failed to lock sandbox directory mapper")?;

This plugin generates credentials (keys and certificates) for both the API proxy server (required for kata-containers/kata-containers#9159 and kata-containers/kata-containers#9752) and the workload owner. This plugin also delivers the credentials to a sandbox (i.e., confidential PODs or VMs), specifically to the kata agent to initiate the SplitAPI proxy server so that a workload owner can communicate with the proxy server using a secure tunnel. The IPv4 address, name, and the ID of the sandbox must be provided in the query string to obtain the credential resources from the kbs. After receiving the credential request, the splitapi plugin will create a key pair for the server and client and sign them using the self-signed CA. The generated ca.crt, server.crt, and server.key are stored in a directory specific to the sandbox (the caller) and returned to the caller. In addition, ca.key, client.key, and client.crt are also generated and stored to that particular directory specific to the sandbox (i.e., caller). During the credential generation, a sandbox directory mapper creates a unique directory specific to the sandbox (i.e., the caller). The mapper creates the unique directory using the sandbox parameters passed in the query string. A mapping file is also maintained to store the mapping between the sandbox name and the unique directory created for the sandbox. The splitapi plugin itself is not initialized by default. To initialize it, you need to add 'splitapi' in the kbs-config.toml. Signed-off-by: Salman Ahmed <sahmed@ibm.com>

Updates include the following changes: - removing the storage of credentials on the filesystem, - serializing/desrializing the credentials dictionary as needed. - configurable credentials/certificate details - other coding related issues Signed-off-by: Salman Ahmed <sahmed@ibm.com>

salmanyam · 2025-01-21T17:28:43Z

Hi @fitzthum and @Xynnn007, I have addressed your comments in the latest commit. Could you please take a look? Thank you!

fitzthum

Thanks for the updates. I think this is a big improvement. At some point you should squash your commits together. I made some comments.

fitzthum · 2025-01-22T21:38:15Z

kbs/src/plugins/implementations/splitapi/backend.rs

+pub struct SandboxParams {
+    /// Required: ID of a sandbox or pod
+    pub id: String,
+    // Required: IP of a sandbox or pod


nit: probably want triple quotes here and on the next field.

fitzthum · 2025-01-22T21:39:41Z

kbs/src/plugins/implementations/splitapi/backend.rs

+}
+
+impl TryFrom<SplitAPIConfig> for SplitAPI {
+    type Error = anyhow::Error;


Do you need this line?

fitzthum · 2025-01-22T21:40:17Z

kbs/src/plugins/implementations/splitapi/backend.rs

+impl TryFrom<SplitAPIConfig> for SplitAPI {
+    type Error = anyhow::Error;
+
+    fn try_from(config: SplitAPIConfig) -> anyhow::Result<Self> {


you are importing anyhow::Result so you can just use Result here

fitzthum · 2025-01-22T21:41:20Z

kbs/src/plugins/implementations/splitapi/backend.rs

+
+pub struct SplitAPI {
+    pub backend: Arc<dyn SplitAPIBackend>,
+}


Do we really need this struct or could we just deal with the SplitAPIBackend directly? I guess this is supposed to be pluggable, but you only have one backend. Are you planning to add more?

fitzthum · 2025-01-22T21:49:59Z

kbs/src/plugins/implementations/splitapi/manager.rs

+        // Return the server credential if the credential presents in the hashmap
+        let key = format!("{}_{}_{}", &params.name, &params.ip, &params.id);
+        if let Some(credentials) = self.load_credentials(&key).await {
+            log::info!("Returning already existed credentials!");


nit: "Returning existing credentials"

fitzthum · 2025-01-22T22:01:57Z

kbs/src/plugins/implementations/splitapi/backend.rs

+use super::generator::CertificateDetails;
+use super::manager;
+
+pub const CREDENTIALS_BLOB_FILE: &str = "certificates.json";


You're using the term credential throughout the PR, but also the term certificates. It might be clearer to just use one especially on lines like this one.

fitzthum · 2025-01-22T22:02:34Z

kbs/src/plugins/implementations/splitapi/manager.rs

+
+        log::info!("Credentials are generated!");
+
+        // Aquire the write lock and write the credential into the hashmap


It's a little odd to be writing state in a method called get...

fitzthum · 2025-01-22T22:04:22Z

kbs/src/plugins/implementations/splitapi/manager.rs

+
+#[async_trait::async_trait]
+impl SplitAPIBackend for CertManager {
+    async fn get_server_credential(&self, params: &SandboxParams) -> Result<Vec<u8>> {


What is the server here? These credentials are for the client, right? Maybe you're referring to some other server, like the SplitAPI server in the guest, but this is pretty confusing.

fitzthum · 2025-01-22T22:06:35Z

kbs/src/plugins/implementations/splitapi/manager.rs

+        })
+    }
+
+    async fn load_credentials(&self, key: &str) -> Option<Credentials> {


nit: you are alternating between credentials (plural) and credential singular in your method names. Probably best to pick one.

Also, this function should probably return Result<Option<Credentials>> given that you could have a failure to load the credentials (i.e. if the file doesn't exist or something) or you could legitimately return no credentials.

fitzthum · 2025-01-22T22:09:53Z

kbs/src/plugins/implementations/splitapi/manager.rs

+
+    async fn load_credentials(&self, key: &str) -> Option<Credentials> {
+        // Check if the credential is not loaded. If not, load them
+        if !self.credential_loaded_from_file.load(Ordering::SeqCst) {


Can we just do this at initialization time and avoid keeping track of whether they've been loaded with every request?

@fitzthum, Thank you very much for providing feedback.

The reason for doing the loading this way is because the plugin initialization code (in plugin_manager.rs), that initializes the plugins, is synchronous, but the load_credentials function is async. Since the async function cannot be used inside a sync function, I cannot directly call the load_credentials function when the splitapi plugin is initialized. There might be two ways to solve it:

changing the load_credentials function to be synchronous, or

making the plugin initialization code to be asynchronous

I am not sure if there's a better way to call an async function inside a sync function.

Either one seems fine. Probably easier to make this sync. You're just reading from a file so that should be doable. I think you can make a new tokio::sync::arc without async.

salmanyam requested a review from a team as a code owner November 19, 2024 19:44

fitzthum reviewed Nov 21, 2024

View reviewed changes

Xynnn007 reviewed Nov 22, 2024

View reviewed changes

salmanyam force-pushed the splitapi-plugin branch from 20e17e9 to 3951960 Compare December 17, 2024 19:34

salmanyam added 2 commits January 18, 2025 06:07

salmanyam force-pushed the splitapi-plugin branch from 3951960 to 560b9e3 Compare January 18, 2025 06:12

fitzthum reviewed Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add splitapi resource plugin #585

Add splitapi resource plugin #585

salmanyam commented Nov 19, 2024 •

edited

Loading

bpradipt commented Nov 20, 2024

fitzthum commented Nov 20, 2024 •

edited

Loading

salmanyam commented Nov 20, 2024 •

edited

Loading

fitzthum commented Nov 20, 2024

yoheiueda commented Nov 21, 2024 •

edited

Loading

fitzthum left a comment

fitzthum Nov 21, 2024

fitzthum Nov 21, 2024

fitzthum Nov 21, 2024

fitzthum Nov 21, 2024

fitzthum Nov 21, 2024

fitzthum Nov 21, 2024

Xynnn007 Nov 22, 2024

Xynnn007 Nov 22, 2024

Xynnn007 Nov 22, 2024

salmanyam commented Jan 21, 2025

fitzthum left a comment

fitzthum Jan 22, 2025

fitzthum Jan 22, 2025

fitzthum Jan 22, 2025

fitzthum Jan 22, 2025

fitzthum Jan 22, 2025

fitzthum Jan 22, 2025

fitzthum Jan 22, 2025

fitzthum Jan 22, 2025

fitzthum Jan 22, 2025

fitzthum Jan 22, 2025

salmanyam Jan 27, 2025

fitzthum Jan 27, 2025


		log::info!("Credentials are generated!");

		// Aquire the write lock and write the credential into the hashmap

Add splitapi resource plugin #585

Are you sure you want to change the base?

Add splitapi resource plugin #585

Conversation

salmanyam commented Nov 19, 2024 • edited Loading

How to build

How to test

bpradipt commented Nov 20, 2024

fitzthum commented Nov 20, 2024 • edited Loading

salmanyam commented Nov 20, 2024 • edited Loading

fitzthum commented Nov 20, 2024

yoheiueda commented Nov 21, 2024 • edited Loading

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

salmanyam commented Jan 21, 2025

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

salmanyam commented Nov 19, 2024 •

edited

Loading

fitzthum commented Nov 20, 2024 •

edited

Loading

salmanyam commented Nov 20, 2024 •

edited

Loading

yoheiueda commented Nov 21, 2024 •

edited

Loading