Move AA abilities to CDH #427

Xynnn007 · 2024-01-03T09:23:28Z

This PR aims to move signature verification, image decryption functionalities from AA to CDH.

We still need to update the integration test and enclave-cc side to make sure the whole path works.

Close #412

Xynnn007 · 2024-01-04T15:06:07Z

Let's see whether enclave-cc could pass with this PR confidential-containers/enclave-cc#319

Xynnn007 · 2024-01-04T15:34:59Z

Nice. confidential-containers/enclave-cc#319 passes. It means that the native part of image-rs/resource/kbs works.

Let me make the PR ready for review.

fitzthum

Nice! This will conflict with #425 and #426 a little bit.

This reminds me that we still have some cleanup to do with some of the tests, especially, but that is for later.

fitzthum · 2024-01-05T18:35:50Z

image-rs/src/resource/kbs/ttrpc.rs

@@ -14,7 +14,7 @@ use super::Client;
 use super::ttrpc_proto::getresource::GetResourceRequest;
 use super::ttrpc_proto::getresource_ttrpc::GetResourceServiceClient;

-const SOCKET_ADDR: &str = "unix:///run/confidential-containers/attestation-agent/getresource.sock";
+const SOCKET_ADDR: &str = "unix:///run/confidential-containers/cdh.sock";


Is there only one socket for the CDH? What about connections to the Kata Agent? Maybe this should have a more specific name.

Yes. All APIs of CDH will be provided by this socket. I think we do not need to create multiple sockets for CDH for different APIs, as one socket would neither block other api calls nor decrease the performance. wdyt?

What do you mean by "connections to Kata Agent"?

Ok, I was worried about the name, but if there is only one socket than it is fine.

What do you mean by "connections to Kata Agent"?

For things like sealed secrets the Kata Agent will connect to the CDH. Will that be the same socket as well?

Yes. It is the same socket. For ASR (Api Server Rest), it also connects to the same socket file and converts RESTful requests to ttrpc callings.

fitzthum · 2024-01-05T18:36:55Z

ocicrypt-rs/src/keywrap/keyprovider/native.rs

@@ -0,0 +1,73 @@
+// Copyright The ocicrypt Authors.


Is this the right copyright? Ocicrypt vs ocicrypt-rs?

Oh. I just copy the one from original ocicrypt-rs https://github.com/confidential-containers/guest-components/blob/main/ocicrypt-rs/src/keywrap/mod.rs#L1-L2

Seems that the original is wrong

Might as well update this one but I wouldn't worry about it too much otherwise.

Ok. Let me temporarily mark this copy right to alibaba. But I think we should then fix all other copyrights in ocicrypt-rs?

A naive thought is that we could select a copyright mark for all coco repos? Or we could have our own like sigstore does. I am not familiar with the legality behind this, but it seems really cool to me.

Yeah I don't really know how the legal stuff works.

image-rs/src/resource/kbs/grpc.rs

image-rs/src/resource/kbs/mod.rs

image-rs/src/resource/kbs/native.rs

ocicrypt-rs/src/keywrap/keyprovider/native.rs

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

confidential-data-hub/hub/Cargo.toml

Makefile

image-rs/scripts/install_offline_fs_kbc_files.sh

Xynnn007 · 2024-01-08T05:21:38Z

@fitzthum Thanks for the review. Fixed some parts of suggestions. There are still some discussions left. PTAL

As stated in confidential-containers#412, AA will never be used as a component that provides abilities more than attestation. This commit changes the ttrpc socket path from AA to CDH for image-rs to GetResource API. Also, for enclave-cc, the Native resource client will instead use the kbs_protocol crate to do the RCAR handshake and do GetResource. For gRPC, we still use the legacy address, but the API was changed as we do not assume that the API is provided by AA but CDH. Signed-off-by: Xynnn007 <[email protected]>

As stated in confidential-containers#412, AA will never be used as a component that provides abilities more than attestation. This commit changes the AA lib calling to decrypt image. This will influence enclave-cc behavior. Signed-off-by: Xynnn007 <[email protected]>

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

add logs for every request. Also deletes previous ttrpc socket file every time the CDH launches. Also, create the parent directory tree when given a unix socket path. Signed-off-by: Xynnn007 <[email protected]>

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

Before this commit, if we do not specify the RESOURCE_PROVIDER field when make, kbs and sev features will be enabled. This will prevent offline-fs-kbc from being activated. This patch requires programmers that manually provide the RESOURCE_PROVIDER parameter when executing make command. Signed-off-by: Xynnn007 <[email protected]>

In protobuf, the `package` matters when a client calls to a server. In ocicrypt-rs, the proto of KeyProvider follows ocicrypt standard, where the package is `keyprovider`. We once use a common name `api` for all apis of CDH, but this does not follow the ocicrypt standard. This patch splits the ocicrypt parts into a separate proto file, whose package is `keyprovider`. Signed-off-by: Xynnn007 <[email protected]>

1. Fix the place of AnnotationPacket. The old code points to a wrong place that was never test so we never found that. 2. Fix the provider comparation logic. The scheme of KBS should be `kbs` rather than `Kbs`. Signed-off-by: Xynnn007 <[email protected]>

We used to request AA for image decryption keys and public keys, etc. Now we are using CDH for these non-attestation APIs. This patch brings a workaround that make the test environment look like it is a "peer pod" environment, then the CDH will read aa_kbc_params from a file rather than kernel cmdline. In future, we will define a launch configuration file for CDH. After that, this workaround can be depreciated. Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 · 2024-01-10T05:48:29Z

@fitzthum Finished round two fixes. Some discussions still to be continued.

fitzthum

LGTM

jialez0

LGTM.

Xynnn007 force-pushed the move-image-decryption-signature-verification-to-cdh branch 3 times, most recently from a3017b4 to ce62a36 Compare January 4, 2024 06:14

jialez0 mentioned this pull request Jan 4, 2024

Clean out-of-date code and documents of AA. #428

Closed

Xynnn007 force-pushed the move-image-decryption-signature-verification-to-cdh branch 5 times, most recently from a742dc4 to 737ac15 Compare January 4, 2024 14:48

Xynnn007 force-pushed the move-image-decryption-signature-verification-to-cdh branch 2 times, most recently from 6f8ff7c to 33b6f38 Compare January 4, 2024 15:25

Xynnn007 marked this pull request as ready for review January 4, 2024 15:35

Xynnn007 requested review from sameo, jiangliu, arronwy, lumjjb and jialez0 as code owners January 4, 2024 15:35

Xynnn007 mentioned this pull request Jan 4, 2024

bump: image-rs to new version confidential-containers/enclave-cc#319

Merged

fitzthum reviewed Jan 5, 2024

View reviewed changes

Xynnn007 force-pushed the move-image-decryption-signature-verification-to-cdh branch 4 times, most recently from 10bdbf9 to e76c49e Compare January 8, 2024 04:58

Xynnn007 force-pushed the move-image-decryption-signature-verification-to-cdh branch 2 times, most recently from 17c7578 to d970bed Compare January 9, 2024 03:22

Xynnn007 added 2 commits January 9, 2024 11:31

Xynnn007 added 8 commits January 9, 2024 11:31

CDH: fix feature dependencies

9ec57f1

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

ci: delete duplicated ocicrypt-rs test cases

3abdeba

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

CDH: add log for launch and requests

a628283

add logs for every request. Also deletes previous ttrpc socket file every time the CDH launches. Also, create the parent directory tree when given a unix socket path. Signed-off-by: Xynnn007 <[email protected]>

CDH: update ttrpc generated files

2be47c3

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

CDH/image: fix unwrap key logic

9c01cd0

1. Fix the place of AnnotationPacket. The old code points to a wrong place that was never test so we never found that. 2. Fix the provider comparation logic. The scheme of KBS should be `kbs` rather than `Kbs`. Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 force-pushed the move-image-decryption-signature-verification-to-cdh branch from d970bed to 68523c5 Compare January 9, 2024 03:31

Xynnn007 requested a review from fitzthum January 10, 2024 05:47

fitzthum approved these changes Jan 11, 2024

View reviewed changes

jialez0 approved these changes Jan 12, 2024

View reviewed changes

jialez0 merged commit ee6306c into confidential-containers:main Jan 12, 2024
12 checks passed

Xynnn007 mentioned this pull request Jan 24, 2024

port attestation agent from CCv0 branch to main branch kata-containers/kata-containers#8870

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move AA abilities to CDH #427

Move AA abilities to CDH #427

Xynnn007 commented Jan 3, 2024

Xynnn007 commented Jan 4, 2024

Xynnn007 commented Jan 4, 2024

fitzthum left a comment

fitzthum Jan 5, 2024

Xynnn007 Jan 7, 2024

fitzthum Jan 8, 2024

Xynnn007 Jan 9, 2024

fitzthum Jan 5, 2024

Xynnn007 Jan 7, 2024

fitzthum Jan 8, 2024

Xynnn007 Jan 9, 2024

fitzthum Jan 11, 2024

Xynnn007 commented Jan 8, 2024

Xynnn007 commented Jan 10, 2024

fitzthum left a comment

jialez0 left a comment

Move AA abilities to CDH #427

Move AA abilities to CDH #427

Conversation

Xynnn007 commented Jan 3, 2024

Xynnn007 commented Jan 4, 2024

Xynnn007 commented Jan 4, 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

Xynnn007 commented Jan 8, 2024

Xynnn007 commented Jan 10, 2024

fitzthum left a comment

Choose a reason for hiding this comment

jialez0 left a comment

Choose a reason for hiding this comment