Confidential DataHub Part 4: CDH binary & Attestation API for AA #322

Xynnn007 · 2023-08-14T09:38:17Z

This PR is the last PR in v0.8.0 cycle for CDH & sealed secret. This PR

Added CDH binary
Added auth module for CDH binary. The auth module is to prepare necessary resources or environments for CDH
Added GetToken/GetEvidence API for AA
Hided the "kbc" abstraction and move it to a KMS plugin in CDH

This PR only added new features to current AA. It didn't delete any existed code.

Some TODOs after v0.8.0:

Attestation-Agent's code should be refactored to delete useless module and tidy the code structure. Only leave codes about get_token and get_evidence
Impl image decryption logic in CDH

KBCs are now impled as KMS drivers, also known as `get_resource_provider` in CDH. To follow the forward compatibility, `sev` is the same as previous `online_sev_kbc` and `kbs` is the same as previous `cc_kbc`. Signed-off-by: Xynnn007 <[email protected]>

Signed-off-by: Xynnn007 <[email protected]>

confidential-data-hub/hub/src/bin/confidential-data-hub/main.rs

fitzthum

Nice. Some comments.

fitzthum · 2023-08-18T16:38:29Z

confidential-data-hub/README.md

+| sev                 | For SEV based on efi secret pre-attestation                        |
+
+Note: `offline-fs` is built-in, we do not need to manually enable. If no `RESOURCE_PROVIDER`
+is given, all features will be enabled.


Are we deprecating the sample option?

I'm not sure whether we are still using sample in current CoCo (except enclave-cc). As all the tests can be covered by offline-fs-kbc, sample seem somehow useless. If it is still used I can add that.

fitzthum · 2023-08-18T16:39:37Z

confidential-data-hub/hub/protos/api.proto

+
+package api;
+
+message UnSealSecretInput {


nit: UnSeal -> Unseal

Nice catch, we should sync with https://github.com/kata-containers/kata-containers/pull/7545/files#diff-e0bf528729d06d2a2c080a9689807bc7b8795745c6edb1a6803dbe67b3be2bddR5. Let me fix this here.

@Lu-Biao could you help to update the PR of yours ?

confidential-data-hub/hub/src/auth/kbs.rs

fitzthum · 2023-08-18T16:55:16Z

confidential-data-hub/hub/src/auth/kbs.rs

+//!
+//! for example
+//! ```
+//! cdh.kbs_resources=kbs:///default/key/1::/run/temp1,kbs:///default/key/2::/run/temp2


I am skeptical of this format and of reading this config from the kernel command line.

To specify the resources (like credential for KMS) are needed. This is only a workaround up to now as we do not have a generic mechanism to inject these data. See kata-containers/kata-containers#7669 (comment) and maybe we can improve this in future

Hmmm. I am not sure about the future proposal or the workaround tbh.Are you sure that this config actually needs to be measured? I will comment on the issue as well.

Also, it seems a bit odd to store all this on the FS. What is the advantage of this over holding credentials in the memory of the CDH process?

I guess one key question is who actually sets up all these configs? Does the user have to do that? Will the shim do it? I had pictured that a user who wrapped a key with the Aliyun KMS, the data in the wrapped secret would be enough to complete the entire process. I guess this allows us to decouple the credentials for that KBS from the information about the secret itself?

Hmmm. I am not sure about the future proposal or the workaround tbh.Are you sure that this config actually needs to be measured? I will comment on the issue as well.

Yes I think so. When the pod's attestation report can pass KBS's RCAR handshake, which resource will be fetched should be known and ensured to avoid abuse, or unexpected overwriting. The config specifies the resource names.

Also, it seems a bit odd to store all this on the FS. What is the advantage of this over holding credentials in the memory of the CDH process?

The FS is still inside the guest's encrypted memory as it is in /run so it can not be seen by the host. If we put them in the ram FS, the high-level code of hub do not need to care about how to organize the credential files and cosume them, it just get_resource and put to where it should be. All later work can be done by KMS plugin code.

I guess one key question is who actually sets up all these configs? Does the user have to do that? Will the shim do it? I had pictured that a user who wrapped a key with the Aliyun KMS, the data in the wrapped secret would be enough to complete the entire process. I guess this allows us to decouple the credentials for that KBS from the information about the secret itself?

The user should do that. Currently an user of aliyun kms does not have an open api to receive all the KMS requests. Instead we have different KMS "instances" like different servers which need different credential to connect to. So the information inside the sealed secret must include the kms instance id and the credential id. (kms instance and credential are not one-to-one)

So the whole story will be: an user makes a sealed secret with aliyun kms, then store the credential into kbs and set the launch parameter of cdh to get the credential from KBS. When launching cdh, it will gets enough credential from the kbs and put them into encrypted guest memory. When unsealling secret, it detects which credential will be used and get from the encrypted guest memory.

The workaround give a straightforward way. We can talk about the init data injection mechanism for kata/coco. If we have that, a more easy way for the user to specify the configs via k8s annotations can be done.

Moreover, for diffferent CSP, the steps can be automatic via outer web page or user interface.

Hm well I guess what we're getting into here are integration questions.

My main concern is that we're making things too complicated for the end user. It's true that the CSP could setup some of this stuff automatically, but keep in mind that today people can use CoCo simply by adding a runtime class to their yaml file. I'm a bit worried that the new features we're adding will be too hard to consume.

With the workaround the issues are mainly that the kernel params are not very robust. This will make validating the launch measurement much more difficult. For the short term, I guess that could be okay.

Anyway, we can probably go ahead here, but I think there are some unanswered integration questions still.

confidential-data-hub/hub/src/auth/sev_es.rs

fitzthum · 2023-08-18T18:15:11Z

confidential-data-hub/hub/src/bin/confidential-data-hub/server.rs

+        input: UnSealSecretInput,
+    ) -> ::ttrpc::Result<UnSealSecretOutput> {
+        debug!("get new UnsealSecret request");
+        let reader = HUB.read().await;


I would just call this hub.

btw does this mean that the hub methods can't modify the state of the hub?

Yes. UnsealSecret function does not change any state of the hub. It only

read KMS credential and create a client

use the kms plugin to unseal the secret

No modifications would be applied to local fs or the status of hub.

confidential-data-hub/kms/src/plugins/aliyun/client.rs

confidential-data-hub/kms/src/plugins/kbs/offline_fs.rs

confidential-data-hub/kms/src/plugins/mod.rs

fitzthum · 2023-08-18T19:14:12Z

confidential-data-hub/hub/src/hub.rs

@@ -93,19 +55,13 @@ impl DataHub for Hub {

    async fn get_resource(&self, uri: String) -> Result<Vec<u8>> {
        // to initialize a get_resource_provider client we do not need the ProviderSettings.
-        let mut client = kms::new_getter(&self.get_resource_provider, ProviderSettings::default())
+        let mut client = kms::new_getter("kbs", ProviderSettings::default())


Any sev option here?

Also, why create a new getter for each request? That means that you have to use the fake client. Instead could we just create the getter once at the beginning?

the concrete kbc under kbs plugin is determined by aa_kbc_params. So we do not need to care if it uses online-sev-kbc of cc-kbc.

Also, why create a new getter for each request? That means that you have to use the fake client. Instead could we just create the getter once at the beginning?

Your idea is ok, too. But we will store an extra fake client object in CDH, while the fake client only provides the function pointer to the real client. Also, when an unseal secret request with kbs provider comes, it will still create a fake client. The consumption of creation a fake client is very low (check the lock), thus I think the way makes sense.

Ok, the first part is a bit confusing because in the past the KbcClient was just for the KBS protocol, but here it can do both that and the SEV protocol. I guess that is ok tho.

Signed-off-by: Xynnn007 <[email protected]>

The kbs auth plugin helps to get the resouces that CDH will needs during the runtime. Such as the credential files that KMS plugin need, etc. Signed-off-by: Xynnn007 <[email protected]>

Signed-off-by: Xynnn007 <[email protected]>

attestation-agent/lib/src/token.rs

confidential-data-hub/secret/src/secret/layout/vault.rs

confidential-data-hub/kms/src/plugins/mod.rs

arronwy

Thanks @Xynnn007 LGTM! verified with api-server-rest

attestation-agent/app/src/grpc.rs

attestation-agent/lib/src/token.rs

Signed-off-by: Xynnn007 <[email protected]>

As everytime the guest launches, only one KBC will be specified by aa_kbc_params in kernel commandline. Thus the KMS plugin only needs to expose one API to hide the underlying kbc type. Also, this commit adds offline-fs-kbc as the by default supported one. If kernel does not specify the offline-fs-kbc, no files will be read, so we do not need to worry about the guest image. Signed-off-by: Xynnn007 <[email protected]>

when aliyun feature is not enabled, the lint error will be raised. Signed-off-by: Xynnn007 <[email protected]>

Ttrpc api receives Arc<Box<A>> as a parameter to init the server, while Arc<Box<A+B>> cannot downcast to Arc<Box<A>>. So a static variable is used to be the actual server object. Signed-off-by: Xynnn007 <[email protected]>

Signed-off-by: Xynnn007 <[email protected]>

Because we combine cc-kbc/offline-fs-kbc/online-sev-kbc into one KMS plugin named kbs. Thus the high level caller hub does not need to care about the concrete kbc type underneath. Signed-off-by: Xynnn007 <[email protected]>

Signed-off-by: Xynnn007 <[email protected]>

fitzthum

LGTM. One or two things I might still tweak, but fine to get the code in.

Xynnn007 added 2 commits August 14, 2023 17:32

cdh/kms: reexport ResourceUri struct from kbs plugin

d275dbc

Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 requested review from jialez0 and sameo as code owners August 14, 2023 09:38

Xynnn007 marked this pull request as draft August 15, 2023 06:48

Xynnn007 force-pushed the feat-cdh-binary branch 2 times, most recently from 5c5ac9e to 51de772 Compare August 15, 2023 09:33

Xynnn007 marked this pull request as ready for review August 16, 2023 01:45

Xynnn007 changed the title ~~Confidential DataHub Part 4: CDH binary & GetToken API for AA~~ Confidential DataHub Part 4: CDH binary & Attestation API for AA Aug 16, 2023

Xynnn007 force-pushed the feat-cdh-binary branch from a8dd0b8 to fce83dc Compare August 17, 2023 05:28

arronwy mentioned this pull request Aug 17, 2023

Initial implementation rest api server for CoCo #325

Merged

Lu-Biao reviewed Aug 18, 2023

View reviewed changes

confidential-data-hub/hub/src/bin/confidential-data-hub/main.rs Outdated Show resolved Hide resolved

Xynnn007 force-pushed the feat-cdh-binary branch from 8b36a70 to 5bfdd90 Compare August 18, 2023 11:07

fitzthum reviewed Aug 18, 2023

View reviewed changes

Xynnn007 added 4 commits August 20, 2023 09:14

cdh/hub: refactor auth module

65a38df

Signed-off-by: Xynnn007 <[email protected]>

cdh/hub: add kbs auth plugin

f55eb11

The kbs auth plugin helps to get the resouces that CDH will needs during the runtime. Such as the credential files that KMS plugin need, etc. Signed-off-by: Xynnn007 <[email protected]>

cdh/hub: add confidential data hub binary

2b483f3

Signed-off-by: Xynnn007 <[email protected]>

aa: move attestation-agent.proto to outer dir

ba0c9a6

Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 force-pushed the feat-cdh-binary branch 2 times, most recently from 7601425 to 5dafdd2 Compare August 20, 2023 10:18

arronwy reviewed Aug 21, 2023

View reviewed changes

attestation-agent/lib/src/token.rs Outdated Show resolved Hide resolved

confidential-data-hub/secret/src/secret/layout/vault.rs Outdated Show resolved Hide resolved

confidential-data-hub/kms/src/plugins/mod.rs Show resolved Hide resolved

Xynnn007 force-pushed the feat-cdh-binary branch from 5dafdd2 to dbc2db2 Compare August 22, 2023 01:12

arronwy approved these changes Aug 22, 2023

View reviewed changes

attestation-agent/app/src/grpc.rs Outdated Show resolved Hide resolved

attestation-agent/lib/src/token.rs Show resolved Hide resolved

Xynnn007 added 7 commits August 23, 2023 10:06

aa: add get_token api

1b50149

Signed-off-by: Xynnn007 <[email protected]>

aa: add get_evidence API

9cf6659

Signed-off-by: Xynnn007 <[email protected]>

aa/app: fix lint error

a269176

Signed-off-by: Xynnn007 <[email protected]>

cdh/kms: fix lint error

789f747

when aliyun feature is not enabled, the lint error will be raised. Signed-off-by: Xynnn007 <[email protected]>

cdh/bin: add get_resource API

f93f020

Ttrpc api receives Arc<Box<A>> as a parameter to init the server, while Arc<Box<A+B>> cannot downcast to Arc<Box<A>>. So a static variable is used to be the actual server object. Signed-off-by: Xynnn007 <[email protected]>

cdh: add README.md and Makefile

e56a26d

Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 added 3 commits August 23, 2023 10:06

cdh/hub: delete logic of resource provider

5d45e32

Because we combine cc-kbc/offline-fs-kbc/online-sev-kbc into one KMS plugin named kbs. Thus the high level caller hub does not need to care about the concrete kbc type underneath. Signed-off-by: Xynnn007 <[email protected]>

cdh/secret: fix cli tool build error

2be4ace

Signed-off-by: Xynnn007 <[email protected]>

cdh/kms: add enum to replace provider type

1dbd378

Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 force-pushed the feat-cdh-binary branch from dbc2db2 to 1dbd378 Compare August 23, 2023 02:07

arronwy mentioned this pull request Aug 23, 2023

CC | protocols: add support sealed_secret kata-containers/kata-containers#7545

Closed

fitzthum approved these changes Aug 23, 2023

View reviewed changes

arronwy merged commit 3d8192f into confidential-containers:main Aug 23, 2023

Xynnn007 deleted the feat-cdh-binary branch August 24, 2023 01:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Confidential DataHub Part 4: CDH binary & Attestation API for AA #322

Confidential DataHub Part 4: CDH binary & Attestation API for AA #322

Xynnn007 commented Aug 14, 2023 •

edited

Loading

fitzthum left a comment

fitzthum Aug 18, 2023

Xynnn007 Aug 20, 2023

fitzthum Aug 18, 2023

Xynnn007 Aug 20, 2023 •

edited

Loading

fitzthum Aug 18, 2023

Xynnn007 Aug 20, 2023

fitzthum Aug 21, 2023 •

edited

Loading

Xynnn007 Aug 22, 2023

fitzthum Aug 22, 2023

fitzthum Aug 18, 2023

Xynnn007 Aug 20, 2023

fitzthum Aug 18, 2023

Xynnn007 Aug 20, 2023

fitzthum Aug 23, 2023

arronwy left a comment

fitzthum left a comment


		package api;

		message UnSealSecretInput {

Confidential DataHub Part 4: CDH binary & Attestation API for AA #322

Confidential DataHub Part 4: CDH binary & Attestation API for AA #322

Conversation

Xynnn007 commented Aug 14, 2023 • 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

Xynnn007 Aug 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzthum Aug 21, 2023 • 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

Choose a reason for hiding this comment

arronwy left a comment

Choose a reason for hiding this comment

fitzthum left a comment

Choose a reason for hiding this comment

Xynnn007 commented Aug 14, 2023 •

edited

Loading

Xynnn007 Aug 20, 2023 •

edited

Loading

fitzthum Aug 21, 2023 •

edited

Loading