Refactor Attestation-Service #216

Xynnn007 · 2023-11-10T07:21:20Z

This huge PR mainly aims to refactor Attestation-Service crate and binaries, including the following:

Changes the API of verifier and makes it as a separate crate
This would help other projects to import our different verifier plugins. Also this commit changes the API of verifier crate from

async fn evaluate(
        &self,
        nonce: String,
        attestation: &Attestation,
    ) -> Result<TeeEvidenceParsedClaim>;

to

    async fn evaluate(
        &self,
        evidence: &[u8],
        expected_report_data: Option<&[u8]>,
        expected_init_data_hash: Option<&[u8]>,
    ) -> Result<TeeEvidenceParsedClaim>;

The reason is that Attestation struct is defined in KBS protocol that also includes the tee-pubkey, but a user of a verifier crate will expect a direct API to verify the evidence without extra information.

But the logic inside attestation-service crate will still do the same thing as before, s.t. the underlying change of verifier crate will not influence the behavior of CoCo-AS. I plan to update the CoCo-AS API in later PRs.

Makes rvps as a separate crate

This is because CoCo-AS has two ways to integrate RVPS now:

with a lib
by gRPC client

A separate crate will decrease the size of the compiled binary when choosing to use gRPC remote RVPS. Also, the RVPS is functionally independent from CoCo-AS. A separate crate would do help to maintaince.

Adds Makefiles for RVPS
Removed as-types crate

This crate is somehow useless and only for the definition of SetPolicyInput. We can embed this definition in CoCo-AS because it is not used by other crates. Please see the commit 9e08408 for more details.

DO NOT MERGE until @fitzthum and @mkulke approve as they are maintainer for SNP and Azure vTPM SNP verifier.

Xynnn007 · 2023-11-11T07:51:14Z

The e2e error is as expected, because after changing the token format of AS, no teepub key field will be inside the CoCoAS Token.

Let me fix this in another PR. KBS will have its own token with tee pub key inside, which is rational because attestation token should not carry semantics of KBS protocol. The reason why KBS has its own "token" is because attestation (done by CoCo-AS) is a lower level semantics than authentication (done by KBS, the authorization is via KBS token).

See #217

Xynnn007 · 2023-11-13T02:20:25Z

Tidied the commits. Now ready to be reviewed.

Xynnn007 · 2023-11-13T02:24:12Z

attestation-service/verifier/src/az_snp_vtpm/mod.rs

-        let evidence = serde_json::from_str::<Evidence>(&attestation.tee_evidence)
+        if report_data.is_none() {
+            bail!("Azure SNP vTPM verifier must provide report data field!");
+        }


@mkulke Hi Magnus, I've changed the API of verifier of AzSnpVtpm. PTL if it looks good to you.

BTW, it will now change the behavior of current CoCoAS.

Xynnn007 · 2023-11-13T02:24:48Z

attestation-service/verifier/src/az_snp_vtpm/mod.rs

+    snp_report: &AttestationReport,
+    vcek: &Vcek,
+    init_data_hash: Option<&[u8]>,
+) -> Result<()> {


Hi @fitzthum Hi Tobin, I've changed the API of verifier of SNP. PTL if it looks good to you.

BTW, it will now change the behavior of current CoCoAS.

Xynnn007 · 2023-11-13T02:25:07Z

attestation-service/verifier/src/cca/mod.rs

-        attestation: &Attestation,
+        evidence: &[u8],
+        report_data: Option<&[u8]>,
+        init_data_hash: Option<&[u8]>,


Hi @chendave Hi Dave, I've changed the API of verifier of CCA. PTL if it looks good to you.

BTW, it will now change the behavior of current CoCoAS.

attestation-service/verifier/src/csv/mod.rs

attestation-service/verifier/src/tdx/mod.rs

BaoshunFang

LGTM.

attestation-service/verifier/src/csv/mod.rs

Xynnn007 · 2023-11-13T05:04:40Z

attestation-service/verifier/src/sgx/mod.rs

@@ -43,22 +43,18 @@ pub struct SgxVerifier {}
 impl Verifier for SgxVerifier {


Hi @mythi I've changed the API of verifier of SGX. PTL if it looks good to you.

BTW, it will now change the behavior of current CoCoAS.

mkulke · 2023-11-13T08:50:57Z

The e2e error is as expected, because after changing the token format of AS, no teepub key field will be inside the CoCoAS Token.

Let me fix this in another PR. KBS will have its own token with tee pub key inside, which is rational because attestation token should not carry semantics of KBS protocol. The reason why KBS has its own "token" is because attestation (done by CoCo-AS) is a lower level semantics than authentication (done by KBS, the authorization is via KBS token).

See #217

If the e2e tests fail and cannot be make pass in the scope of this PR, does this mean that main would be in a non-working state if we merge it? (until we merge the other KBS refactoring PR)? If that's the case, since we have consolidated AS and KBS in a single repo, does it make sense to apply the protocol change in a dedicated atomic PR without the other unrelated fixes?

attestation-service/verifier/src/az_snp_vtpm/mod.rs

attestation-service/verifier/src/sample/mod.rs

mkulke · 2023-11-13T09:22:03Z

attestation-service/attestation-service/README.md

+    "runtime_data": ["YWFhCg==...", "YWFhCg==..."], // base64 encoded materials that used to calculate the
+                                                    // digest inside report_data to check the binding.
+                                                    // If sets [] the comparation will be skipped.
+    "init_data": ["YWFhCg==...", "YWFhCg==..."],    // base64 encoded materials that used to calculate the


Is this an implementation of what's being proposed in RFC 171?

They are related. The API here is provided by CoCo-AS. It can help a caller to check the original data and the binding with an evidence.

The API can not only help the checking step in the RFC, but also other users who use the CoCo-AS as a separate attestation service to check any binding relationship between some data and the evidence.

Xynnn007 · 2023-11-13T09:27:21Z

The e2e error is as expected, because after changing the token format of AS, no teepub key field will be inside the CoCoAS Token.
Let me fix this in another PR. KBS will have its own token with tee pub key inside, which is rational because attestation token should not carry semantics of KBS protocol. The reason why KBS has its own "token" is because attestation (done by CoCo-AS) is a lower level semantics than authentication (done by KBS, the authorization is via KBS token).
See #217

If the e2e tests fail and cannot be make pass in the scope of this PR, does this mean that main would be in a non-working state if we merge it? (until we merge the other KBS refactoring PR)? If that's the case, since we have consolidated AS and KBS in a single repo, does it make sense to apply the protocol change in a dedicated atomic PR without the other unrelated fixes?

Sounds good. Let me just leave the code structure changing in this PR.

Xynnn007 · 2023-11-14T03:51:15Z

Changed the PR intro. Now ready to be reviewed. Other work will be include in separate PRs.

PTAL @mkulke

Xynnn007 · 2023-11-23T12:07:50Z

Fixed as @mkulke suggested.

mkulke · 2023-11-23T15:01:11Z

Fixed as @mkulke suggested.

if we want to stop the noisy propagation of lifetimes (afaiu this is due to problems w/ #[async_trait], hopefully it will soon not be required anymore 🤞) you can switch to references, to make it less noisy. I attached a diff for the sample-verifier that does this:

diff --git a/attestation-service/attestation-service/src/lib.rs b/attestation-service/attestation-service/src/lib.rs
index 3a60109..a94cf2b 100644
--- a/attestation-service/attestation-service/src/lib.rs
+++ b/attestation-service/attestation-service/src/lib.rs
@@ -102,13 +102,13 @@ impl AttestationService {
         let claims_from_tee_evidence = verifier
             .evaluate(
                 attestation.tee_evidence.as_bytes(),
-                report_data,
+                &report_data,
                 // We currently do not need to check the initdata hash in AS when using
                 // `verifier` crate.
                 //
                 // We will refactor the CoCo-AS' API to leverage the parameter.
                 // See https://github.com/confidential-containers/kbs/issues/177
-                InitDataHash::NotProvided,
+                &InitDataHash::NotProvided,
             )
             .await
             .map_err(|e| anyhow!("Verifier evaluate failed: {e:?}"))?;
diff --git a/attestation-service/verifier/src/lib.rs b/attestation-service/verifier/src/lib.rs
index c16f491..e50f873 100644
--- a/attestation-service/verifier/src/lib.rs
+++ b/attestation-service/verifier/src/lib.rs
@@ -123,10 +123,10 @@ pub trait Verifier {
     /// instance is created. It is always provided by untrusted host,
     /// but its integrity will be protected by the tee evidence.
     /// Typical `init_data_hash` is `HOSTDATA` for SNP.
-    async fn evaluate<'a, 'b>(
+    async fn evaluate(
         &self,
         evidence: &[u8],
-        expected_report_data: ReportData<'a>,
-        expected_init_data_hash: InitDataHash<'b>,
+        expected_report_data: &ReportData,
+        expected_init_data_hash: &InitDataHash,
     ) -> Result<TeeEvidenceParsedClaim>;
 }
diff --git a/attestation-service/verifier/src/sample/mod.rs b/attestation-service/verifier/src/sample/mod.rs
index 4292f63..6f6cdcd 100644
--- a/attestation-service/verifier/src/sample/mod.rs
+++ b/attestation-service/verifier/src/sample/mod.rs
@@ -23,11 +23,11 @@ pub struct Sample {}
 
 #[async_trait]
 impl Verifier for Sample {
-    async fn evaluate<'a, 'b>(
+    async fn evaluate(
         &self,
         evidence: &[u8],
-        expected_report_data: ReportData<'a>,
-        expected_init_data_hash: InitDataHash<'b>,
+        expected_report_data: &ReportData,
+        expected_init_data_hash: &InitDataHash,
     ) -> Result<TeeEvidenceParsedClaim> {
         let tee_evidence = serde_json::from_slice::<SampleTeeEvidence>(evidence)
             .context("Deserialize Quote failed.")?;
@@ -43,8 +43,8 @@ impl Verifier for Sample {
 }
 
 async fn verify_tee_evidence(
-    expected_report_data: ReportData<'_>,
-    expected_init_data_hash: InitDataHash<'_>,
+    expected_report_data: &ReportData<'_>,
+    expected_init_data_hash: &InitDataHash<'_>,
     evidence: &SampleTeeEvidence,
 ) -> Result<()> {
     // Verify the TEE Hardware signature. (Null for sample TEE)
@@ -55,7 +55,7 @@ async fn verify_tee_evidence(
         let ev_report_data = base64::engine::general_purpose::STANDARD
             .decode(&evidence.report_data)
             .context("base64 decode report data for sample evidence")?;
-        if expected_report_data != ev_report_data {
+        if *expected_report_data != ev_report_data {
             bail!("REPORT_DATA is different from that in Sample Quote");
         }
     }
@@ -66,7 +66,7 @@ async fn verify_tee_evidence(
         let ev_init_data_hash = base64::engine::general_purpose::STANDARD
             .decode(&evidence.init_data_hash)
             .context("base64 decode init data hash for sample evidence")?;
-        if expected_init_data_hash != ev_init_data_hash {
+        if *expected_init_data_hash != ev_init_data_hash {
             bail!("INIT DATA HASH is different from that in Sample Quote");
         }
     }

Xynnn007 · 2023-11-23T15:04:36Z

@mkulke Cool. The propogation of lifetime really makes me sad : (. Let me try this. Thanks!

Xynnn007 · 2023-11-23T15:16:35Z

@mkulke Have fixed with your suggestion. PTAL

mkulke

lgtm

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

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

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

mythi · 2023-11-24T12:56:31Z

was the PR merged by accident again?

Xynnn007 · 2023-11-24T13:00:19Z

was the PR merged by accident again?

I think we get an agreement?

btw, see https://github.com/confidential-containers/kbs/pull/240/files#diff-a200532515643126f81cfe1d9c2ae91282ae2f2b4ce5cb019ca0719a85a06ba8R124-R132

mythi · 2023-11-24T13:26:49Z

to me, the agreement was to get this to work without any API changes first with the tradeoffs that are only impacting SGX/TDX.

mkulke · 2023-11-24T13:27:58Z

I think we get an agreement?

maybe a misunderstanding on my side, but I also assumed this was blocked by the init-data proposal being resolved? Would this code still be valid independently from how we inject init-data into the guest (embedded into policy or top-level via kata-agent api)?

mythi · 2023-11-24T13:38:33Z

I think we get an agreement?

maybe a misunderstanding on my side, but I also assumed this was blocked by the init-data proposal being resolved? Would this code still be valid independently from how we inject init-data into the guest (embedded into policy or top-level via kata-agent api)?

There's another blocker too that I'm talking about. It moved the code to the opposite direction to what I had in mind with confidential-containers/guest-components@4499301 which is related to my (slow) attempts to get CoCo KBS to work with non-CoCo ASes.

Xynnn007 · 2023-11-24T14:50:25Z

maybe a misunderstanding on my side, but I also assumed this was blocked by the init-data proposal being resolved? Would this code still be valid independently from how we inject init-data into the guest (embedded into policy or top-level via kata-agent api)?

These are independent things I think. As this only provides extra ability to check the initdata hash inside evidence for different verifiers, but we are still doing what we do before this PR -- does not care anything about initdata when doing KBS protocol and RCAR handshake -- as it used to be. So does #240 I think.

If we use policy to inject initdata, we need an expected hostdata (which is called init_data_hash in verifier's API) of the policy.
If we use initdata mechanism to inject initdata, we also need an expected hostdata of the initdata.

Does this make sense? Maybe I have missed something..

mkulke · 2023-11-24T15:01:03Z

Does this make sense? Maybe I have missed something..

yup it does. thanks for clarifying! 👍

Xynnn007 · 2023-11-24T15:10:35Z

@mythi The common aim for us would be to support non-CoCo-AS in KBS protocol, right?

Binding specific hash algorithm to specific TEE platform seems a coupling things. A potentially better and flexible way is to add hash algorithm handshake in KBS protocol. In this way, after the first round of RCAR handshake, the kbs-client will use the hash algorithm specified by the AS. By this verifier and attester can provide independent functionality to verify already-given report_data or init_data.

I have made a PR based on that idea #240 to let CoCo-AS to handle attestation requests with flexible hash algorithms.

I think it might be better for us to go straightforward to the way of updating protocol as the consensus? Starting from making a proposal for that.
I hope that we won't use some code-coupling methods to implement a function if we have already realized that, which might potentially cause a huge burden for adding new functions in the future.

There's another blocker too that I'm talking about. It moved the code to the opposite direction to what I had in mind with confidential-containers/guest-components@4499301 which is related to my (slow) attempts to get CoCo KBS to work with non-CoCo ASes.

I got your point that we should include the digest of the whole content of tee-pubkey inside the evidence here. This is great!

mythi · 2023-11-27T06:25:14Z

@mythi The common aim for us would be to support non-CoCo-AS in KBS protocol, right?

Binding specific hash algorithm to specific TEE platform seems a coupling things. A potentially better and flexible way is to add hash algorithm handshake in KBS protocol.

For what is forth, this PR made the coupling stronger and blocked my work. I asked to avoid it but that was ignored. I find the pattern that code is merged by the PR author even when there is no consensus quite concerning. This is the second time it happened in a short period of time. Can we agree that in these type of case the PR author recuses themselves from merging?

I think it might be better for us to go straightforward to the way of updating protocol as the consensus?

I don't agree that "let's just change the spec" is the best approach to start with. The problem I was trying to solve does not require spec changes to start with. They are nice/good to have but a better approach would have been to fix things first and then see what are all the places that need updating.

I got your point that we should include the digest of the whole content of tee-pubkey inside the evidence here. This is great!

It's not my "my point". It's what the protocol spec says and the implementation does not follow it.

Xynnn007 force-pushed the refactor-as branch 3 times, most recently from af2baf9 to 8347740 Compare November 11, 2023 07:41

Xynnn007 force-pushed the refactor-as branch from 8347740 to 04dad00 Compare November 11, 2023 12:14

Xynnn007 marked this pull request as ready for review November 11, 2023 12:53

Xynnn007 requested a review from sameo as a code owner November 11, 2023 12:53

Xynnn007 marked this pull request as draft November 11, 2023 14:06

Xynnn007 force-pushed the refactor-as branch from 04dad00 to 78ac576 Compare November 11, 2023 14:06

Xynnn007 marked this pull request as ready for review November 11, 2023 14:55

Xynnn007 force-pushed the refactor-as branch 4 times, most recently from 34876a7 to 958bb61 Compare November 13, 2023 02:18

Xynnn007 commented Nov 13, 2023

View reviewed changes

attestation-service/verifier/src/csv/mod.rs Outdated Show resolved Hide resolved

Xynnn007 commented Nov 13, 2023

View reviewed changes

attestation-service/verifier/src/tdx/mod.rs Outdated Show resolved Hide resolved

BaoshunFang approved these changes Nov 13, 2023

View reviewed changes

attestation-service/verifier/src/csv/mod.rs Outdated Show resolved Hide resolved

Xynnn007 commented Nov 13, 2023

View reviewed changes

mkulke reviewed Nov 13, 2023

View reviewed changes

Xynnn007 force-pushed the refactor-as branch 3 times, most recently from 603b135 to b1820b8 Compare November 14, 2023 03:50

Xynnn007 force-pushed the refactor-as branch from d430c73 to 1203fc7 Compare November 23, 2023 12:04

Xynnn007 force-pushed the refactor-as branch from 1203fc7 to 21a5279 Compare November 23, 2023 15:15

mkulke approved these changes Nov 23, 2023

View reviewed changes

Xynnn007 added 11 commits November 24, 2023 08:47

repo: delete as-types repo

0b24d5f

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

AS: bring verifier out as a separate crate

0c239d0

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

AS: move rvps as a separate crate

f825bcd

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

AS/RVPS: fix document

87e3098

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

AS/RVPS: add Makefile for RVPS

eba524b

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

AS: use workspace dependency

4b38c2b

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

KBS: resolve AS dep

f20f1e7

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

ci: fix AS ci

f3eb3dd

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

ci: fix RVPS image building

a226069

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

Docker: add dockerignore

ca329bf

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

AS: fix function name

da32a48

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

Xynnn007 force-pushed the refactor-as branch from 21a5279 to da32a48 Compare November 24, 2023 00:48

Xynnn007 merged commit 725513e into confidential-containers:main Nov 24, 2023
9 checks passed

Xynnn007 deleted the refactor-as branch November 24, 2023 01:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Attestation-Service #216

Refactor Attestation-Service #216

Xynnn007 commented Nov 10, 2023 •

edited

Loading

Xynnn007 commented Nov 11, 2023 •

edited

Loading

Xynnn007 commented Nov 13, 2023

Xynnn007 Nov 13, 2023

Xynnn007 Nov 13, 2023

Xynnn007 Nov 13, 2023

BaoshunFang left a comment

Xynnn007 Nov 13, 2023

mkulke commented Nov 13, 2023

mkulke Nov 13, 2023

Xynnn007 Nov 13, 2023

Xynnn007 commented Nov 13, 2023

Xynnn007 commented Nov 14, 2023

Xynnn007 commented Nov 23, 2023

mkulke commented Nov 23, 2023

Xynnn007 commented Nov 23, 2023

Xynnn007 commented Nov 23, 2023

mkulke left a comment

mythi commented Nov 24, 2023

Xynnn007 commented Nov 24, 2023

mythi commented Nov 24, 2023

mkulke commented Nov 24, 2023

mythi commented Nov 24, 2023

Xynnn007 commented Nov 24, 2023 •

edited

Loading

mkulke commented Nov 24, 2023

Xynnn007 commented Nov 24, 2023 •

edited

Loading

mythi commented Nov 27, 2023

		@@ -43,22 +43,18 @@ pub struct SgxVerifier {}
		impl Verifier for SgxVerifier {

Refactor Attestation-Service #216

Refactor Attestation-Service #216

Conversation

Xynnn007 commented Nov 10, 2023 • edited Loading

Xynnn007 commented Nov 11, 2023 • edited Loading

Xynnn007 commented Nov 13, 2023

Xynnn007 Nov 13, 2023

Choose a reason for hiding this comment

Xynnn007 Nov 13, 2023

Choose a reason for hiding this comment

Xynnn007 Nov 13, 2023

Choose a reason for hiding this comment

BaoshunFang left a comment

Choose a reason for hiding this comment

Xynnn007 Nov 13, 2023

Choose a reason for hiding this comment

mkulke commented Nov 13, 2023

mkulke Nov 13, 2023

Choose a reason for hiding this comment

Xynnn007 Nov 13, 2023

Choose a reason for hiding this comment

Xynnn007 commented Nov 13, 2023

Xynnn007 commented Nov 14, 2023

Xynnn007 commented Nov 23, 2023

mkulke commented Nov 23, 2023

Xynnn007 commented Nov 23, 2023

Xynnn007 commented Nov 23, 2023

mkulke left a comment

Choose a reason for hiding this comment

mythi commented Nov 24, 2023

Xynnn007 commented Nov 24, 2023

mythi commented Nov 24, 2023

mkulke commented Nov 24, 2023

mythi commented Nov 24, 2023

Xynnn007 commented Nov 24, 2023 • edited Loading

mkulke commented Nov 24, 2023

Xynnn007 commented Nov 24, 2023 • edited Loading

mythi commented Nov 27, 2023

Xynnn007 commented Nov 10, 2023 •

edited

Loading

Xynnn007 commented Nov 11, 2023 •

edited

Loading

Xynnn007 commented Nov 24, 2023 •

edited

Loading

Xynnn007 commented Nov 24, 2023 •

edited

Loading