Support Confidential Sealed Secrets (as env vars) #9719

fitzthum · 2024-05-25T03:04:00Z

This PR brings sealed secret functionality to main. This functionality was originally implemented by @LindaYu17 and @Lu-Biao.

I have made some improvements and converted the old sealed secrets test from the defunct CCv0 branch of the tests repo into a test that is similar to the existing CoCo attestation test.

There are still refinements to be made (especially in the last PR). I have done some local testing, but let's see what the CI makes of it.

fidencio · 2024-05-25T07:13:34Z

Hey @fitzthum! First of all, thanks for taking over this work and opening the PR, very much appreciated.

I see this touches mainly 2 parts of the code:

runtime, which can be enabled / disabled in each specific configuration file
agent, which can be enabled / disabled only during build time

When we have something enabled / disabled only during build time on the agent side, it basically means that in order to properly test it we'd have to create a new agent build for that specific case, and then add this new specific agent into the rootfs / initrd, which is a pain as it leads to a way too huge amount of things to test.

With this in mind, I´d like to ask you, is there the possibility to have this still as an option, still defaulting to not using it in the agent's Makefile, but enabled by default in the agent build (here:

kata-containers/tools/packaging/kata-deploy/local-build/kata-deploy-binaries.sh

Line 821 in b89c3e3

    
           DESTDIR="${destdir}" AGENT_POLICY="yes" PULL_TYPE=${PULL_TYPE} "${agent_builder}"

)? This would simplify things a lot, if doable. If you consider this not doable, mind to explain what are the issues you foresee with this approach?

fitzthum · 2024-05-28T20:43:30Z

@fidencio Currently the sealed-secret stuff in the agent is behind the sealed-secret feature, which is disabled by default in the Makefile. I add the SEALED_SECRET=yes to install_initrd_confidential and install_image_confidential. I'm not sure if that's the best place to put it, but the idea is to only enable this for CoCo builds, since the feature depends on the CDH.

You are suggesting setting SEALED_SECRET in install_agent. Would this enable it for non-coco builds as well? I could potentially rework the code a bit fail gracefully if the CDH is not there, but the feature does increase the binary size.

fidencio · 2024-05-28T21:16:56Z

You are suggesting setting SEALED_SECRET in install_agent. Would this enable it for non-coco builds as well? I could potentially rework the code a bit fail gracefully if the CDH is not there, but the feature does increase the binary size

I'd prefer that. I understand that it'll increase the binary size, but what we ship here is an example of what people should use, and I sincerely hope people end up building their own content when they decide to get kata-containers into their production environment. With that said, as long as we leave a way for folks to build the agent without sealed secrets support, we're good. That's better than having to do deal with different agent builds.

fitzthum · 2024-05-31T20:16:33Z

src/runtime/virtcontainers/kata_agent.go

 	ociMounts := spec.Mounts

 	for index, m := range ociMounts {
 		if guestMount, ok := guestMounts[m.Destination]; ok {
 			k.Logger().Debugf("Replacing OCI mount (%s) source %s with %s", m.Destination, m.Source, guestMount.Source)
+			if sealedSecretEnabled {
+				if strings.Contains(m.Source, "kubernetes.io~secret") {
+					ociMounts[index].Destination = "/sealed" + m.Destination


I'm not sure this approach will allow sealed secrets and non-sealed secrets to coexist. Any ideas how to improve? cc: @Xynnn007 maybe

Ideally I'd like to remove the shim support entirely.

I am not expert upon this. But at the firt glance, this logic will add all secrets (sealed/unsealed) with "/sealed" prefix. Togetherly, at kata-agent side all prefixs will be striped and put all files under /run/secrets/. Whether to unseal the file is determined by the content format of the secret file. Thus I am afraid we might not need both add-prefix on runtime side and delete-prefix part on agent side.

I am not sure, we might need to do some test for this. With both sealed and non-sealed secret binding to a container and check if it works.

Yeah i suspect there are a couple ways to get rid of the runtime stuff.

fitzthum · 2024-05-31T20:49:50Z

Fixed up static checks.

I think I will wait for #9749 before reshuffling how the feature is enabled.

Xynnn007

There are some comments. Most are about the code architecture.

Xynnn007 · 2024-06-04T03:44:34Z

src/agent/src/cdh.rs

+        let sealed_secret = sealed_secret
+            .strip_prefix("sealed.")
+            .ok_or(anyhow!("secret prefix not found"))?;


In current impl of CDH we delayed the strip in CDH. Thus we do not need to .strip_prefix("sealed.") here. Just put it into CDH's API.

I removed this and bumped guest-components. Hopefully this won't cause any other issues.

Xynnn007 · 2024-06-04T03:50:53Z

src/agent/src/cdh.rs

+    pub async fn unseal_env(&self, env: &str) -> Result<String> {
+        let (key, value) = env.split_once('=').unwrap();
+        if value.starts_with("sealed.") {
+            let unsealed_value = self.unseal_secret_async(value).await?;
+            let unsealed_env = format!("{}={}", key, std::str::from_utf8(&unsealed_value).unwrap());
+
+            return Ok(unsealed_env);
+        }
+        Ok((*env.to_owned()).to_string())
+    }
+}


Maybe we do not need the unseal_env() func. Just do some condition judgement in outer rpc.rs and if sealed secret are detected (with sealed. prefix), unseal_secret_async() will then be called.

btw, let's try to avoid unwrap()s

I am a bit split on this. I would prefer to keep the handling of the env outside of cdh.rs, but I would also like to hide as much complexity as possible from rpc.rs. Maybe other reviewers have a viewpoint on this?

Brainstorming: I read this comment after I began to review the code. I confess that I was wondering if those unseal methods wouldn't fit better somewhere else. Sealed secrets seems to not belong to the "cdh" concept, but it seems rather a simple user of cdh (to fetch the secret for unsealing the secret).

Yeah cdh.rs currently contains the direct cdh code (which is very minimal) and also helper functions for some of the operations that use the cdh. I think this is reasonable but I am open to either renaming the file to be more clear or moving some of the helper code somewhere else if anyone has any idea where would be good.

Xynnn007 · 2024-06-04T03:52:20Z

src/agent/src/rpc.rs

+    #[cfg(feature = "sealed-secret")]
+    cdh_client: Option<CDHClient>,


We could have a OnceCell<CDHClient> inside cdh.rs and only be used in that mod file. And only expose unseal_secret_async() to higher level. This would help higher callers to ignore the underlying details, thus more modularization.

This is a good suggestion, but I have refactored the setup of the client to work a bit differently. Now it is instantiated based on what is specified in the new gc_procs setting. Thus, I am continuing to expose the entire cdh_client to rpc.rs. I think this will also be helpful when we start to introduce other capabilities that depend on cdh.rs.

Xynnn007 · 2024-06-04T03:53:24Z

src/agent/src/rpc.rs

+    #[cfg(feature = "sealed-secret")]
+    cdh_client: Option<CDHClient>,


We could have a OnceCell<CDHClient> inside cdh.rs and only be used in that mod file. And only expose unseal_secret_async() to higher level. This would help higher callers to ignore the underlying details, thus more modularization.

src/agent/src/cdh.rs

docs/how-to/how-to-set-sandbox-config-kata.md

Xynnn007 · 2024-06-04T05:05:02Z

src/runtime/virtcontainers/kata_agent.go

 	ociMounts := spec.Mounts

 	for index, m := range ociMounts {
 		if guestMount, ok := guestMounts[m.Destination]; ok {
 			k.Logger().Debugf("Replacing OCI mount (%s) source %s with %s", m.Destination, m.Source, guestMount.Source)
+			if sealedSecretEnabled {
+				if strings.Contains(m.Source, "kubernetes.io~secret") {
+					ociMounts[index].Destination = "/sealed" + m.Destination


I am not expert upon this. But at the firt glance, this logic will add all secrets (sealed/unsealed) with "/sealed" prefix. Togetherly, at kata-agent side all prefixs will be striped and put all files under /run/secrets/. Whether to unseal the file is determined by the content format of the secret file. Thus I am afraid we might not need both add-prefix on runtime side and delete-prefix part on agent side.

I am not sure, we might need to do some test for this. With both sealed and non-sealed secret binding to a container and check if it works.

fitzthum · 2024-06-05T19:47:40Z

I have some significant changes planned here so I will make this a draft.

fitzthum · 2024-06-06T21:17:24Z

I have found a few issues with the implementation of sealed secrets as files. I have a plan for fixing this, but I think the smart thing to do is to break this PR apart and start only with secrets in environment variables, which are simpler. To that end, I have dropped a couple commits from this PR. I will re-introduce them in a follow-up. Notice that we don't actually need any shim support for sealed secrets to work with environment variables. I am hoping for the same thing when I do secrets in files.

Tomorrow I will finally rework the way the feature is loaded and address other comments.

src/agent/src/rpc.rs

fitzthum · 2024-06-07T22:12:05Z

I have reshuffled how this feature is enabled. There is no longer any feature required for sealed-secret support (for the agent or the shim). Instead, it can be enabled at runtime simply be setting guest-components-procs to either confidential-data-hub or api-server-rest. In other words, if the CDH has been started, you can use sealed secrets for free. Otherwise, skip the code paths added here.

src/agent/src/rpc.rs

src/agent/src/cdh.rs

wainersm · 2024-06-18T23:43:30Z

src/agent/src/cdh.rs

+
+    pub async fn unseal_env(&self, env: &str) -> Result<String> {
+        let (key, value) = env.split_once('=').unwrap();
+        if value.starts_with("sealed.") {


We could make this "sealed." as a constant (eg. SEALED_PREFIX) and document the expected format of a sealed secret. We also could have a simple parser.

I'm also wondering if we shouldn't sanitize the input to avoid kind of injection attack (really don't know if a attack here is doable).

I can't think of any issues with injection, but I'll think about it a bit more.

There is a tool for generating sealed secrets in the guest-components repo.

wainersm · 2024-06-18T23:55:11Z

tests/integration/kubernetes/k8s-sealed-secret.bats

+	kubectl create secret generic sealed-secret --from-literal='password=sealed.fakejwsheader.ewogICAgInZlcnNpb24iOiAiMC4xLjAiLAogICAgInR5cGUiOiAidmF1bHQiLAogICAgIm5hbWUiOiAia2JzOi8vL2RlZmF1bHQvc2VhbGVkLXNlY3JldC90ZXN0IiwKICAgICJwcm92aWRlciI6ICJrYnMiLAogICAgInByb3ZpZGVyX3NldHRpbmdzIjoge30sCiAgICAiYW5ub3RhdGlvbnMiOiB7fQp9Cg==.fakesignature'
+}
+
+@test "Unseal Secrets with CDH" {


Idea of new test: mix sealed and non-sealed secrects in a single deployment to check it can handle well (if possible make a unit test instead).

Do you think we should add a new test for this or just extend the existing one? Can this be a follow-up PR?

I like the idea of checking this mixed scenario. I think a new test might be clearer

I ended up adding a not-sealed secret to the existing tests to avoid making a second yaml file.

fitzthum · 2024-07-02T02:58:44Z

Non-tee tests passing (including with the sealed-secret test). Other tests looking ok too. Good time for some more review.

Xynnn007

Cool. The pr looks good. Only a nit

Xynnn007 · 2024-07-02T08:24:07Z

src/agent/src/main.rs

+        sandbox.clone(),
+        config.server_addr.as_str(),
+        init_mode,
+        gc_enabled,


Maybe we could directly pass Some(CDHClient) as parameter here? which would help to avoid splited condition judgement on different places.

Good idea. I moved the initialization of the CDHClient to init_attestation_components so we can create the CDHClient right after starting the CDIH.

stevenhorsman

A few comments - generally I think it looks great. Maybe updating the PR name to be clearer that it's just sealed secrets via envs, not files would be helpful?

tests/integration/kubernetes/k8s-sealed-secret.bats

stevenhorsman · 2024-07-03T10:34:57Z

tests/integration/kubernetes/k8s-sealed-secret.bats

+	kubectl create secret generic sealed-secret --from-literal='password=sealed.fakejwsheader.ewogICAgInZlcnNpb24iOiAiMC4xLjAiLAogICAgInR5cGUiOiAidmF1bHQiLAogICAgIm5hbWUiOiAia2JzOi8vL2RlZmF1bHQvc2VhbGVkLXNlY3JldC90ZXN0IiwKICAgICJwcm92aWRlciI6ICJrYnMiLAogICAgInByb3ZpZGVyX3NldHRpbmdzIjoge30sCiAgICAiYW5ub3RhdGlvbnMiOiB7fQp9Cg==.fakesignature'
+}
+
+@test "Unseal Secrets with CDH" {


I like the idea of checking this mixed scenario. I think a new test might be clearer

tests/integration/kubernetes/k8s-sealed-secret.bats

stevenhorsman · 2024-07-03T10:39:19Z

tests/integration/kubernetes/k8s-sealed-secret.bats

+	# Setup k8s secret
+	kubectl delete secret sealed-secret --ignore-not-found
+
+	# Sealed secret format is defined at: https://github.com/confidential-containers/guest-components/blob/main/confidential-data-hub/docs/SEALED_SECRET.md#vault


I think it might be nice for test clarity and customisability (or a real word that means that), to have the secret generated in the test code rather than hard coded, but I'm fine for that to be part of a later PR.

stevenhorsman · 2024-07-03T10:42:31Z

tests/integration/kubernetes/runtimeclass_workloads/pod-sealed-secret.yaml

+      if [ "$SECRET_PASSWORD" == "unsealed_secret" ]; then
+        echo "unsealed environment as expected"
+      fi


Idea: I'm wondering whether having something like echo "Unsealed env secret: $SECRET_PASSWORD" and then including the secret content in the grep check in the test might be a bit more customisable?

Good idea. Ideally we could generate a random key each time and then package it into a secret. I haven't gone that far, but I've move the check from the yaml file to the bash script.

To unseal a secret, the Kata agent will contact the CDH using ttRPC. Add the proto that describes the sealed secret service and messages that will be used. Signed-off-by: Tobin Feldman-Fitzthum <tobin@ibm.com> Signed-off-by: Biao Lu <biao.lu@intel.com>

When sealed-secret is enabled, the Kata Agent intercepts environment variables containing sealed secrets and uses the CDH to unseal the value. Signed-off-by: Tobin Feldman-Fitzthum <tobin@ibm.com> Signed-off-by: Linda Yu <linda.yu@intel.com>

To test unsealing secrets stored in environment variables, we create a simple test server that takes the place of the CDH. We start this server and then use it to unseal a test secret. Signed-off-by: Tobin Feldman-Fitzthum <tobin@ibm.com> Signed-off-by: Linda Yu <linda.yu@intel.com>

The sealed secret test depends on the KBS to provide the unsealed value of a vault secret. This secret is provisioned to an environment variable. Signed-off-by: Tobin Feldman-Fitzthum <tobin@ibm.com>

stevenhorsman

I think all my suggestions are either for future PRs, or have been address here, so this looks good to me. Thanks!

fitzthum requested a review from a team as a code owner May 25, 2024 03:04

katacontainersbot added the size/huge Largest and most complex task (probably needs breaking into small pieces) label May 25, 2024

fitzthum force-pushed the sealed-secret branch from aecc63e to 9952c6f Compare May 31, 2024 20:12

fitzthum commented May 31, 2024

View reviewed changes

fitzthum force-pushed the sealed-secret branch from 9952c6f to 728db58 Compare May 31, 2024 20:29

Xynnn007 reviewed Jun 4, 2024

View reviewed changes

fitzthum marked this pull request as draft June 5, 2024 19:47

fitzthum force-pushed the sealed-secret branch from 728db58 to 5449a37 Compare June 6, 2024 21:11

katacontainersbot added size/large Task of significant size and removed size/huge Largest and most complex task (probably needs breaking into small pieces) labels Jun 6, 2024

fitzthum force-pushed the sealed-secret branch 2 times, most recently from e5e99f9 to d5b6851 Compare June 7, 2024 22:06

fitzthum commented Jun 7, 2024

View reviewed changes

src/agent/src/rpc.rs Outdated Show resolved Hide resolved

fitzthum force-pushed the sealed-secret branch from d5b6851 to 35365da Compare June 7, 2024 22:14

stevenhorsman added the ok-to-test label Jun 12, 2024

fitzthum commented Jun 12, 2024

View reviewed changes

src/agent/src/rpc.rs Outdated Show resolved Hide resolved

fitzthum force-pushed the sealed-secret branch 3 times, most recently from 4468ed5 to 562b08f Compare June 13, 2024 21:14

wainersm reviewed Jun 18, 2024

View reviewed changes

src/agent/src/cdh.rs Outdated Show resolved Hide resolved

wainersm reviewed Jun 18, 2024

View reviewed changes

src/agent/src/cdh.rs Outdated Show resolved Hide resolved

wainersm reviewed Jun 18, 2024

View reviewed changes

fitzthum force-pushed the sealed-secret branch 3 times, most recently from 5c73dc2 to a966d51 Compare June 28, 2024 18:44

fitzthum force-pushed the sealed-secret branch 4 times, most recently from 40a5322 to 71af0e2 Compare July 1, 2024 22:09

fitzthum marked this pull request as ready for review July 2, 2024 02:55

Xynnn007 approved these changes Jul 2, 2024

View reviewed changes

fitzthum force-pushed the sealed-secret branch from 71af0e2 to f281952 Compare July 2, 2024 18:29

stevenhorsman reviewed Jul 3, 2024

View reviewed changes

fitzthum force-pushed the sealed-secret branch from f281952 to 9bc909e Compare July 3, 2024 15:15

fitzthum changed the title ~~Support Confidential Sealed Secrets~~ Support Confidential Sealed Secrets (as env vars) Jul 3, 2024

fitzthum force-pushed the sealed-secret branch 2 times, most recently from 6e2bd28 to 754e725 Compare July 3, 2024 17:16

portersrc approved these changes Jul 3, 2024

View reviewed changes

fitzthum force-pushed the sealed-secret branch from 754e725 to 87ddce6 Compare July 3, 2024 19:19

Lu-Biao and others added 4 commits July 4, 2024 01:03

agent: support sealed secret as env in kata

b6844c0

When sealed-secret is enabled, the Kata Agent intercepts environment variables containing sealed secrets and uses the CDH to unseal the value. Signed-off-by: Tobin Feldman-Fitzthum <tobin@ibm.com> Signed-off-by: Linda Yu <linda.yu@intel.com>

tests: add test for sealed env secrets

2c7ddcf

The sealed secret test depends on the KBS to provide the unsealed value of a vault secret. This secret is provisioned to an environment variable. Signed-off-by: Tobin Feldman-Fitzthum <tobin@ibm.com>

fitzthum force-pushed the sealed-secret branch from 87ddce6 to 2c7ddcf Compare July 4, 2024 06:03

stevenhorsman approved these changes Jul 5, 2024

View reviewed changes

		#[cfg(feature = "sealed-secret")]
		cdh_client: Option<CDHClient>,

Support Confidential Sealed Secrets (as env vars) #9719

Are you sure you want to change the base?

Support Confidential Sealed Secrets (as env vars) #9719

Conversation

fitzthum commented May 25, 2024

fidencio commented May 25, 2024

fitzthum commented May 28, 2024

fidencio commented May 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzthum commented May 31, 2024

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

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

fitzthum commented Jun 5, 2024

fitzthum commented Jun 6, 2024

fitzthum commented Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

fitzthum Jun 21, 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

fitzthum commented Jul 2, 2024

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

stevenhorsman 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

stevenhorsman left a comment

Choose a reason for hiding this comment

fitzthum commented Jun 7, 2024 •

edited

Loading

fitzthum Jun 21, 2024 •

edited

Loading