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.

Sign up for GitHub

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

Jump to bottom

KBS-Protocol | Add AAEvidenceProvider implementation #868

Merged
merged 3 commits into from
Jan 17, 2025
Merged

KBS-Protocol | Add AAEvidenceProvider implementation #868

merged 3 commits into from
Jan 17, 2025

Conversation

Xynnn007
Copy link
Member

@Xynnn007 Xynnn007 commented Jan 6, 2025 •
edited
Loading

Let CDH get evidence rather than token from AA when doing get_resource based on RCAR.

Add AAEvidenceProvider implementation to kbs-protocol crate

@Xynnn007
Cargo.toml: format the toml file
c018b51 
Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007
AA/kbs_protocol: rename feature name aa_token to aa_ttrpc
3f168cc 
This feature enables dependencies to leverage ttrpc capabilities to
connect to AA via ttrpc, thus `aa_ttrpc` is a better name for future
extension.

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007 Xynnn007 requested a review from a team as a code owner January 6, 2025 07:55
@Xynnn007 Xynnn007 marked this pull request as draft January 6, 2025 07:57
@Xynnn007 Xynnn007 marked this pull request as ready for review January 7, 2025 01:43
mkulke
mkulke reviewed Jan 7, 2025
View reviewed changes
Copy link
Contributor

@mkulke mkulke left a comment

Choose a reason for hiding this comment

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

After this commit, only CDH will access the KBS.

This change will require the KBS that does RCAR and stores resource be
the same in CoCo, s.t. background check model. which is up to now
nearly all the deployments we have met.

This will not break the passport model because kbs_protocol crate still
provides a way for developers to integrate either passport model or
background check model in their own code.

Will coco deployments that provide individual resource-kbs and token-kbs's still work after this change?

If not, what would a user have to do to enable such a deployment?

@fitzthum
Copy link
Member

fitzthum commented Jan 7, 2025

So the key use case here is running multiple CDHs? Maybe we should have a discussion about supporting multi-KBS deployments more generally. I think we should try to have a standard way to enable multi-kbs workloads once init-data is supported.

@Xynnn007
Copy link
Member Author

Xynnn007 commented Jan 8, 2025

Ok. Currently CoCo deployments does not support the scenario where token-KBS and resource-KBS are different as they both are specified with aa_kbc_params.

The concrete case is for scenarios beyond CoCo:

  1. Running AA as a daemon
  2. Use one-shot CDH to connect to different KBS each time

Let me only leave the commits that add AAEvidenceProvider. And we can go back to the interaction to KBS problem then.

@Xynnn007 Xynnn007 changed the title Update the interaction between AA and CDH KBS-Protocol | Add AAEvidenceProvider implementation Jan 9, 2025
@Xynnn007
Copy link
Member Author

Xynnn007 commented Jan 9, 2025

@fitzthum @mkulke I abandon the commits of changing the behavior and only adds the AAEvidenceProvider plugin to kbs_protocol. This will not change any current behavior and deployments.

fitzthum
fitzthum reviewed Jan 9, 2025
View reviewed changes
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.

A couple nits but generally this seems ok.

I think we should discuss multi-kbs more generally. I don't really think we should use multiple CDHs. instead we can just add support for connecting to multiple KBSes in the CDH, but that's a topic for another time.

attestation-agent/kbs_protocol/src/evidence_provider/aa.rs Outdated Show resolved Hide resolved
attestation-agent/kbs_protocol/src/ttrpc_protos/mod.rs Outdated Show resolved Hide resolved
attestation-agent/kbs_protocol/src/evidence_provider/aa.rs Outdated Show resolved Hide resolved
attestation-agent/kbs_protocol/src/evidence_provider/aa.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this file should be called ttrpc.rs to contrast with native.rs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the "provider" is AA, so maybe aa_ttrpc is clearer?

@Xynnn007
AA/kbs_protocol: add AAEvidenceProvider
b255ba6 
AAEvidenceProvider gets evidence via ttrpc from AA. This patch also does
some refactoring upon the code structure of ttrpc to avoid duplication
of ttrpc files.

Signed-off-by: Xynnn007 <[email protected]>
fitzthum
fitzthum approved these changes Jan 16, 2025
View reviewed changes
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

@Xynnn007 Xynnn007 merged commit ffd22b3 into confidential-containers:main Jan 17, 2025
21 checks passed
@Xynnn007 Xynnn007 deleted the aa-move-to-cdh branch January 17, 2025 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fitzthum fitzthum fitzthum approved these changes

@mkulke mkulke mkulke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Xynnn007 @fitzthum @mkulke