[Attestation Service] Change the API of CoCo-AS #240

Xynnn007 · 2023-11-24T10:01:47Z

This PR will refactor the CoCo-AS furthermore, which will help to let CoCo-AS run as a seperate attestation-service without any coupling with KBS.

The API of CoCo AS will be definitely not related to KBS protocol. In this PR, CoCo-AS gRPC service will have the following API

message AttestationRequest {
    // TEE enum. Specify the evidence type
    Tee tee = 1;

    // Base64 encoded evidence. The alphabet is URL_SAFE_NO_PAD.
    // defined in https://datatracker.ietf.org/doc/html/rfc4648#section-5
    string evidence = 2;

    // Runtime Data used to check the binding relationship with report data in
    // Evidence
    oneof runtime_data {
        // Base64 encoded runtime data slice. The whole string will be base64
        // decoded. The result one will then be accumulated into a digest which
        // is used as the expected runtime data to check against the one inside
        // evidence.
        //
        // The alphabet is URL_SAFE_NO_PAD.
        // defined in https://datatracker.ietf.org/doc/html/rfc4648#section-5
        string raw_runtime_data = 3;

        // Runtime data in a JSON map. CoCoAS will rearrange each layer of the
        // data JSON object in dictionary order by key, then serialize and output
        // it into a compact string, and perform hash calculation on the whole
        // to check against the one inside evidence.
        //
        // After the verification, the structured runtime data field will be included
        // inside the token claims.
        string structured_runtime_data = 4;
    }

    // Init Data used to check the binding relationship with init data in
    // Evidence
    oneof init_data {
        // Base64 encoded init data slice. The whole string will be base64
        // decoded. The result one will then be accumulated into a digest which
        // is used as the expected init data to check against the one inside
        // evidence.
        //
        // The alphabet is URL_SAFE_NO_PAD.
        // defined in https://datatracker.ietf.org/doc/html/rfc4648#section-5
        string raw_init_data = 5;

        // Init data in a JSON map. CoCoAS will rearrange each layer of the
        // data JSON object in dictionary order by key, then serialize and output
        // it into a compact string, and perform hash calculation on the whole
        // to check against the one inside evidence.
        // 
        // After the verification, the structured init data field will be included
        // inside the token claims.
        string structured_init_data = 6;
    }

    // Hash algorithm used to calculate runtime data. Currently can be "sha256",
    // "sha384" or "sha512". If not specified, "sha384" will be selected.
    string runtime_data_hash_algorithm = 7;

    // Hash algorithm used to calculate init data. Currently can be "sha256",
    // "sha384" or "sha512". If not specified, "sha384" will be selected.
    string init_data_hash_algorithm = 8;

    // List of IDs of the policy used to check evidence. If not provided,
    // a "default" one will be used.
    repeated string policy_ids = 9;
}

Close #177
Related to #217

cc @jialez0

Xynnn007 · 2023-11-27T06:14:31Z

kbs/src/api/src/attestation/coco/grpc.rs

+            tee: to_grpc_tee(tee).into(),
+            evidence: STANDARD.encode(attestation.tee_evidence),
+            runtime_data,
+            init_data: vec![],


cc @mkulke You might be interested in this -- This is the KBS side code that does not leverage the ability of the initdata verafication in this PR.

jialez0

LGTM. Thanks! @Xynnn007

mythi · 2023-12-01T07:04:45Z

message AttestationRequest {

we may also need nonce?

Xynnn007 · 2023-12-01T07:19:59Z

message AttestationRequest {

we may also need nonce?

Nonce is something that will be bound to the report data. This API now gives a general parameter runtime_data to check the binding between any report data and the evidence.

mythi · 2023-12-01T08:08:53Z

This API now gives a general parameter runtime_data to check the binding between any report data and the evidence.

AFAIUI, the AS cannot generate "expected report data" based on runtime_data (Tee pubkey) only if it does not know the nonce the attester used to generate the quote/report data.

Xynnn007 · 2023-12-01T08:45:38Z

This API now gives a general parameter runtime_data to check the binding between any report data and the evidence.\n\nAFAIUI, the AS cannot generate "expected report data" based on runtime_data (Tee pubkey) only if it does not know the nonce the attester used to generate the quote/report data.

You are right.

This api is now for user who knows the materials to calculate the report data.

We can add an extra API, which will give a nonce to the caller to get evidence. Then the user can call this attestation API.

jepio · 2023-12-01T09:21:24Z

I'm still reviewing, but the attestation requester should not be passing policy_id to evaluate against, that is insecure. The policy in RATS is supplied by the verifier owner, which is different then the attester.

jepio

I think this is a step in the right direction but needs some more thought.

It's not secure to mix claims attested through runtime data (like tee-pubkey, nonce) with "custom claims" which are unattested and user controlled. So we really need to impose some structure on runtime data (like a json dictionary) that can be included in the attested token separately from any user supplied data.

Custom claims should instead be a "nonce" supplied by the user to be included in the attestation token.

attestation-service/attestation-service/src/lib.rs

jepio · 2023-12-01T09:37:15Z

attestation-service/bin/grpc-as/src/server.rs

+        let runtime_data = request
+            .runtime_data
+            .iter()
+            .map(|data_item| {
+                STANDARD
+                    .decode(data_item)
+                    .map_err(|e| Status::aborted(format!("base64 decode runtime data: {e}")))
+            })
+            .collect::<std::result::Result<Vec<Vec<u8>>, Status>>()?;
+
+        let init_data: Vec<Vec<u8>> = request
+            .init_data
+            .iter()
+            .map(|data_item| {
+                STANDARD
+                    .decode(data_item)
+                    .map_err(|e| Status::aborted(format!("base64 decode init data: {e}")))
+            })
+            .collect::<std::result::Result<Vec<Vec<u8>>, Status>>()?;


Why array of base64 strings? I think these should just be base64 strings. We also need to document the fact that parameters are base64 encoded and document which base64 encoding is used in README.md. In remote APIs it is most commonly URL_SAFE_NO_PAD, not STANDARD.

It is embarassing. If we does not use array, it will be difficult to distinguish an "empty runtime data" and "no runtime data" by only one parameter. It would be better to keep as-is and let me add more document about this.

are there optional parameters in grpc? i care more about how this works for a JSON http API, where we can have can have:

"runtime_data": {}

or

"runtime_data": { "data": "" }

or skip passing runtime_data.

In gRPC, optional key word is replaced by repeated. In on-going rest CoCo AS we will:

If no "runtime_data" field is in the request body. We do not check

If "runtime_data": {} field is given. we do the check, by hash(b"")

if we stick to an array it would be more like:

no "runtime_data" or "runtime_data": [] - we don't check

"runtime_data": [""] we check hash(b"").

attestation-service/attestation-service/src/lib.rs

Xynnn007 · 2023-12-01T10:31:08Z

@jepio policy_id in the api would help the caller to apply different use cases.

The key point is that the returned attestation token will have information about the policy ids that are used to check against the evidence. Current AS only has a fixed policy and cannot choose a specific one from API.

Doea this make sense?

jepio · 2023-12-01T11:35:52Z

@jepio policy_id in the api would help the caller to apply different use cases.

The key point is that the returned attestation token will have information about the policy ids that are used to check against the evidence. Current AS only has a fixed policy and cannot choose a specific one from API.

Doea this make sense?

I see. I now see that ITA also allows the user the option to pass the policy in a similar way. Why don't we do it same way as well:

policy id parameter is optional and then default policy is applied
policy id is hash/uuid (i think hashes are a better option as they are unique and unmodifiable), not name

Xynnn007 · 2023-12-04T04:17:29Z

@jepio policy_id in the api would help the caller to apply different use cases.
The key point is that the returned attestation token will have information about the policy ids that are used to check against the evidence. Current AS only has a fixed policy and cannot choose a specific one from API.
Doea this make sense?

I see. I now see that ITA also allows the user the option to pass the policy in a similar way. Why don't we do it same way as well:

policy id parameter is optional and then default policy is applied

policy id is hash/uuid (i think hashes are a better option as they are unique and unmodifiable), not name

This is a great point. I think this is related to how policies are indexed. Currently we only have OPA as policy engine and the regos are stored in filesystem so we directly retrieve them with the file name.

In this PR, I added the policy digest into the result JWT as you suggested to ensure the integrity of policy recorded in the JWT. However how to specify which policy should be used might be another topic, as it is related to how it is stored. We can promote this in another PR. wdyt?

Xynnn007 · 2023-12-04T06:04:18Z

It's not secure to mix claims attested through runtime data (like tee-pubkey, nonce) with "custom claims" which are unattested and user controlled. So we really need to impose some structure on runtime data (like a json dictionary) that can be included in the attested token separately from any user supplied data.

It is a good idea that we should have a runtime data format/spec like undergoing initdata. This could help us to leverage report_data field in a more standard way. A json map would help both protect the integrity and embed into extra claims.

But for custormized claims, my idea is that this would give ability to the user to embed some extra context information without integrity protection. Whether to use such claims is determined by the consumer of the JWT. In CoCo-AS/KBS model, the attestation requester and JWT consumer both are KBS, so it is secure for KBS itself to leverage the pub-key fields inside the claims.

I'm not sure whether both should be supported (with/without integrity protection), but seems that we should.

Custom claims should instead be a "nonce" supplied by the user to be included in the attestation token.

From the perspective of opposing server replay attack, this is worth doing.

Xynnn007 · 2023-12-04T09:39:11Z

Update the PR due to @jepio 's suggestions. This is still under reviewing. I added some more documents and the examples for CoCoAS gRPC.

attestation-service/protos/attestation.proto

jepio · 2023-12-06T08:46:38Z

attestation-service/bin/grpc-as/example.token.json

+    },
+    "evaluation-reports": [
+        {
+            "evaluation-result": "{\"allow\":true}",


does the evaluation result have to be a string? can't it be:

"evaluation-result": { "allow": true }

This is the raw string output by OPA when using the default_policy.rego. I am afraid that we cannot assume what the result will look like. It is decided by the rego.

we can specify what it needs to look like to be valid. and besides: the code already parses the OPA output and checks that it matches this structure.

Let me parse the output directly as a JSON object. I think the document of policies can be added in another PR that also can promote the policy storage.

jepio · 2023-12-06T09:00:51Z

@jepio policy_id in the api would help the caller to apply different use cases.
The key point is that the returned attestation token will have information about the policy ids that are used to check against the evidence. Current AS only has a fixed policy and cannot choose a specific one from API.
Doea this make sense?

I see. I now see that ITA also allows the user the option to pass the policy in a similar way. Why don't we do it same way as well:

policy id parameter is optional and then default policy is applied

policy id is hash/uuid (i think hashes are a better option as they are unique and unmodifiable), not name

This is a great point. I think this is related to how policies are indexed. Currently we only have OPA as policy engine and the regos are stored in filesystem so we directly retrieve them with the file name.

In this PR, I added the policy digest into the result JWT as you suggested to ensure the integrity of policy recorded in the JWT. However how to specify which policy should be used might be another topic, as it is related to how it is stored. We can promote this in another PR. wdyt?

I think we need to address this before. We can't change the token format every week because without stability we won't see adoption. We should not be leaking the fact that policies are stored in filesystem into our token API - in a cloud deployment of CoCo AS the backend storage will be blob storage. So I think we need policy management APIs like ITA (https://docs.trustauthority.intel.com/main/restapi/restapi-policy-management.html). When a user registers a policy they need to either pass a UUID or the API returns a UUID. And then this UUID would be passed with the attestation request. WDYT?

jepio · 2023-12-06T09:05:07Z

It's not secure to mix claims attested through runtime data (like tee-pubkey, nonce) with "custom claims" which are unattested and user controlled. So we really need to impose some structure on runtime data (like a json dictionary) that can be included in the attested token separately from any user supplied data.

It is a good idea that we should have a runtime data format/spec like undergoing initdata. This could help us to leverage report_data field in a more standard way. A json map would help both protect the integrity and embed into extra claims.

But for custormized claims, my idea is that this would give ability to the user to embed some extra context information without integrity protection. Whether to use such claims is determined by the consumer of the JWT. In CoCo-AS/KBS model, the attestation requester and JWT consumer both are KBS, so it is secure for KBS itself to leverage the pub-key fields inside the claims.

No, it's not secure. Anyone can request a CoCo-AS token passing an arbitrary public key in "custom claims", also one not stored in a TEE and then pass that token to CoCo KBS. So "runtime-claims" (attested through a REPORTDATA mechanism) need to be separated from arbitrary "custom claims" in the token. Anything else can't be trusted by consumers.

attestation-service/attestation-service/src/policy_engine/opa/mod.rs

Xynnn007 · 2023-12-08T06:03:33Z

@jepio Updated the PR. Added a separate commit where I use oneof key word to support both raw init/runtime data match and structured init/runtime data match. PTAL if it is good. If it is good, I will update the KBS part to align with the CoCoAS part.

kbs/src/api/src/attestation/coco/grpc.rs

fitzthum

This looks good.

A few questions:

Do you think it would be useful if we had some kind of default policy? This could be set per-AS and evaluated on every request. It might help simplify the process of provisioning policies for simple use cases.

How does reference data fit into this? It looks like reference values aren't set via the attestation request or reported in the attestation results. The same policy could have very different results with different reference values.

attestation-service/attestation-service/src/lib.rs

fitzthum · 2023-12-08T21:19:58Z

attestation-service/attestation-service/src/policy_engine/opa/mod.rs

            if !allow {
-                bail!("Untrusted TEE evidence")
+                bail!("TEE evidence does not pass policy {policy_id}, reason: {policy_res}");


I'm not sure this is the best way to handle this. It might be better to return a token that says that a policy has not passed evaluation. It's a bit hard to know. Maybe that would increase the complexity of the KBS.

If we stick with this approach we should make sure to document it well for clients who are trying to write policies.

In the initial implementation I embed the policy validation results into the result token. Suggested by @jepio #240 (comment) I think it is right that if any policy fails to check, the verifiecation should fail.

@Xynnn007 I think the meaning of specifying multiple policy IDs is to introduce more flexibility in the attestation result report. Therefore, it may not be a good idea to force all policies to pass on the AS side.

The requirement logic for proving a request may be varied. For example, some users may write different policies to verify different field contents, so that they want all specified policies to pass. However, other users may write different policies to correspond to different TEE software versions, so they hope that at least one policy can be passed.

The requirement logic like the above should be decided by the user himself, and the attestation result report should truthfully reflect the verification content of each policy, rather than forcing each specified policy to pass.

So there are two options for now

Provision token only if all policies pass. less flexible for policy but easy to use token.

Provision token regardless of any policy enforcement result. More flexible for policy but needs extra logic to use token.

right?

Now I am ok with both.

wdyt? @fitzthum @jepio

Yeah I think either one is fine. The first option sort of implies that the AS policies are going to return binary values, which isn't necessarily the case. The second option will likely increase the complexity of the KBS policies. I guess we can pick one, merge it, and see how we feel about it down the road.

Xynnn007 · 2023-12-11T03:49:43Z

Do you think it would be useful if we had some kind of default policy? This could be set per-AS and evaluated on every request. It might help simplify the process of provisioning policies for simple use cases.

Yes. I have already added this here. That means if no policy id is set, gRPC CoCo-AS will use the "default" one. This was not implementation inside the library as I think it is a specific behavior of the service rather than generic lib.

On the KBS side, we can add an optional parameter inside the launch configuration to specify the policy. If not specified, a hardcode default one will be used.

How does reference data fit into this? It looks like reference values aren't set via the attestation request or reported in the attestation results. The same policy could have very different results with different reference values.

Reference data seems a design challlenge. Reasons

We currently tightly depend on the OPA policy to check the reference data. That is to say, if the policy does not do the check, the reference data actually do not work.
@thomas-fossati once raised a good point that compound reference values are needed but still not supported by the corrent RVPS.

But overall, I agree with you that attestation token should have the reference data information inside. So let just temporarily ignore how 1 & 2 are handled, and just try to fix the token format, s.t. another field containing reference data information that was used when doing verification. wdyt?

Due to confidential-containers#240 (comment) any claims inside the attestation token should be ensured integrity by binding the digest into the HW evidence. This commit supports both structured and raw init/runtime data to check against the binding of that in evidence. Now two kinds of binding check are supported: 1. Raw data. CoCo-AS can help to check if the runtime/init data field is the same as the given raw data. 2. Structured data. CoCo-AS can help to check if the digest of the given JSON object (with keys in every layer reordered in alphabet) is the same as the runtime/init data. If matched, the result attestation token will contain the JSON object as customized claims. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Xynnn007 · 2023-12-12T03:44:13Z

Rebased and update the PR with the new API.

@jepio @fitzthum @jialez0 a decision left: whether the token can be provisioned without passing all OPA policies

@mythi Added the KBS side check for confidential-containers/guest-components#366 here ~~I will put a PR on guest-components side then.~~ PR: confidential-containers/guest-components#406

Xynnn007 · 2023-12-12T06:21:30Z

~~Container build CI is not related to this PR. Will be resolved in a separate PR.~~
Fixed

Before this commit, the API of Attestation-Service lib is coupled with KBS protocol. Also, not all functionalities of verifier driver are used, including checking the binding of initdata hash and runtime data hash. This commit will help to only have attestation related parameters in this API. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

After update the API of attestation-service lib, gRPC service also needs to be updated. Before this commit, the API of gRPC AS is coupled with KBS protocol. It is needed for a user to just use CoCo-AS to do some verification work without coperating with KBS. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

New CoCo-AS API will decouple KBS protocol semantics. Also, this commit changes the hashed materials when connecting to CoCo-AS. Before this commit, only k_mod and k_exp of tee-pubkey field are protected integrity when doing remote attestation. The `kty` and `alg` field of the tee-pubkey are ignored. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Fixed the wrong guides to build a gRPC CoCo-AS. Also update the new format of attestation-token. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

mythi

LGTM

jepio

lgtm. I'm ok with either of the approaches (always release token or only release token when all policies pass) until we find some problem.

Xynnn007 force-pushed the refactor-as-api branch 3 times, most recently from ff25252 to adadb18 Compare November 24, 2023 11:58

Xynnn007 marked this pull request as ready for review November 24, 2023 12:20

Xynnn007 requested a review from sameo as a code owner November 24, 2023 12:20

Xynnn007 mentioned this pull request Nov 24, 2023

Refactor Attestation-Service #216

Merged

Xynnn007 commented Nov 27, 2023

View reviewed changes

jialez0 approved these changes Dec 1, 2023

View reviewed changes

Xynnn007 force-pushed the refactor-as-api branch from adadb18 to d4dbd55 Compare December 1, 2023 02:47

jepio suggested changes Dec 1, 2023

View reviewed changes

jepio mentioned this pull request Dec 1, 2023

New KBS endpoint: /register #175

Open

Xynnn007 force-pushed the refactor-as-api branch 2 times, most recently from 8f48dd8 to a9ac1f5 Compare December 4, 2023 06:53

Xynnn007 mentioned this pull request Dec 4, 2023

AS: add parsed claims for TDX/SGX and documents #248

Merged

Xynnn007 requested a review from jepio December 4, 2023 07:18

mythi reviewed Dec 4, 2023

View reviewed changes

attestation-service/protos/attestation.proto Outdated Show resolved Hide resolved

jepio reviewed Dec 6, 2023

View reviewed changes

attestation-service/attestation-service/src/policy_engine/opa/mod.rs Outdated Show resolved Hide resolved

Xynnn007 force-pushed the refactor-as-api branch from a9ac1f5 to 115e6cc Compare December 8, 2023 06:00

Xynnn007 requested a review from jepio December 8, 2023 06:14

mythi reviewed Dec 8, 2023

View reviewed changes

kbs/src/api/src/attestation/coco/grpc.rs Outdated Show resolved Hide resolved

fitzthum reviewed Dec 8, 2023

View reviewed changes

Xynnn007 force-pushed the refactor-as-api branch from 115e6cc to f41223e Compare December 11, 2023 06:26

jialez0 mentioned this pull request Dec 12, 2023

[RFC] Runtime data spec #259

Open

Xynnn007 force-pushed the refactor-as-api branch from f41223e to 157ecc4 Compare December 12, 2023 03:32

Xynnn007 force-pushed the refactor-as-api branch from 157ecc4 to 29ffabd Compare December 12, 2023 03:57

Xynnn007 requested review from fitzthum, mythi and jialez0 December 12, 2023 06:20

Xynnn007 added 6 commits December 12, 2023 17:48

AS/proto: add CCA enum to TEE

1de7f69

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

AS: fix documents

7f0c4e7

Fixed the wrong guides to build a gRPC CoCo-AS. Also update the new format of attestation-token. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

tools/client: use the fixed version of kbs-protocol

0cd92cf

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Xynnn007 force-pushed the refactor-as-api branch from d130c1b to 0cd92cf Compare December 12, 2023 09:49

fitzthum approved these changes Dec 12, 2023

View reviewed changes

jialez0 approved these changes Dec 14, 2023

View reviewed changes

mythi approved these changes Dec 15, 2023

View reviewed changes

jepio approved these changes Dec 15, 2023

View reviewed changes

Xynnn007 merged commit 0825413 into confidential-containers:main Dec 15, 2023
9 checks passed

Xynnn007 deleted the refactor-as-api branch December 15, 2023 08:04

Xynnn007 mentioned this pull request Jun 4, 2024

Verifier: Add IBM Secure Execution driver framework #345

Merged

1 task

[Attestation Service] Change the API of CoCo-AS #240

[Attestation Service] Change the API of CoCo-AS #240

Conversation

Xynnn007 commented Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

jialez0 left a comment

Choose a reason for hiding this comment

mythi commented Dec 1, 2023

Xynnn007 commented Dec 1, 2023

mythi commented Dec 1, 2023

Xynnn007 commented Dec 1, 2023

jepio commented Dec 1, 2023

jepio 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

Xynnn007 commented Dec 1, 2023

jepio commented Dec 1, 2023

Xynnn007 commented Dec 4, 2023 • edited Loading

Xynnn007 commented Dec 4, 2023 • edited Loading

Xynnn007 commented Dec 4, 2023

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

jepio commented Dec 6, 2023

jepio commented Dec 6, 2023

Xynnn007 commented Dec 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xynnn007 commented Dec 11, 2023 • edited Loading

Xynnn007 commented Dec 12, 2023 • edited Loading

Xynnn007 commented Dec 12, 2023 • edited Loading

mythi left a comment

Choose a reason for hiding this comment

jepio left a comment

Choose a reason for hiding this comment

Xynnn007 commented Nov 24, 2023 •

edited

Loading

Xynnn007 commented Dec 4, 2023 •

edited

Loading

Xynnn007 commented Dec 4, 2023 •

edited

Loading

Xynnn007 commented Dec 8, 2023 •

edited

Loading

Xynnn007 commented Dec 11, 2023 •

edited

Loading

Xynnn007 commented Dec 12, 2023 •

edited

Loading

Xynnn007 commented Dec 12, 2023 •

edited

Loading