Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kbs: simplify tee-pubkey reading from the attestation token #414

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

mythi
Copy link
Contributor

@mythi mythi commented Jun 13, 2024

get-resource API searches tee-pubkey claim from the attestation token. Instead of doing the search by ourselves, simply pass the desired path and leverage pointer() method to do the search. This simplifies the code when the search path needs to be made configurable.

One such case as of today is when attestation service provided tokens carry tee-pubkey under a different claims structure. We piggyback the existing cargo features to let each AS implementation define their own search paths compile time.

@mythi mythi force-pushed the ita-4 branch 3 times, most recently from 7e2657e to d9ee995 Compare June 14, 2024 08:32
@mythi mythi marked this pull request as ready for review June 14, 2024 08:54
@fitzthum
Copy link
Member

Can you add PR description and some commit messages?

@mythi
Copy link
Contributor Author

mythi commented Jun 17, 2024

Can you add PR description and some commit messages?

Done

Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment.

I wonder if we should incorporate the variable pubkey path into the Attest trait at some point. This seems fine for now.

@@ -14,6 +14,11 @@ use intel_trust_authority::*;
use kbs_types::{Challenge, Tee};
use rand::{thread_rng, Rng};

#[cfg(any(feature = "coco-as-builtin", feature = "coco-as-grpc"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be #[cfg(not(feature = "intel-trust-authority-as"))]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that works also. the proposed cfg was a copy-paste from the other coco-as* configs in this file.

Comment on lines +22 to +25
#[cfg(feature = "as")]
const TOKEN_TEE_PUBKEY_PATH: &str = AS_TOKEN_TEE_PUBKEY_PATH;
#[cfg(not(feature = "as"))]
const TOKEN_TEE_PUBKEY_PATH: &str = "/customized_claims/runtime_data/tee-pubkey";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two places defining TOKEN_TEE_PUBKEY_PATH, like before. Is it possible to define only on one place and use a reference on the other side?

Copy link
Contributor Author

@mythi mythi Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted it to be like that but I found the problem that TOKEN_TEE_PUBKEY_PATH needs to be defined even completely without attestation (i.e., as not enabled) so some "default" must be kept here.

mythi added 2 commits June 19, 2024 08:46
get-resource API searches tee-pubkey claim from the attestation token.
Instead of doing the search by ourselves, simply pass the desired path
and leverage pointer() method to do the search. This simplifies the
code when the search path needs to be made configurable.

Signed-off-by: Mikko Ylinen <[email protected]>
Attestation service provided tokens carry tee-pubkey under a different
claims structure. Piggyback the existing cargo features to let each AS
implementation define their own search paths compile time.

Signed-off-by: Mikko Ylinen <[email protected]>
Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fitzthum fitzthum merged commit 5c403b2 into confidential-containers:main Jun 19, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants