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

Bump kbs-types and kbs_protocol with a KBS protocol version change #445

Merged
merged 3 commits into from
Aug 28, 2024
Merged

Bump kbs-types and kbs_protocol with a KBS protocol version change #445

merged 3 commits into from
Aug 28, 2024

Conversation

mythi
Copy link
Contributor

@mythi mythi commented Jul 26, 2024 •
edited
Loading

@mythi mythi force-pushed the deps-kbs-types branch 9 times, most recently from 2b9aad2 to 934cdf1 Compare August 1, 2024 06:08
@mythi mythi marked this pull request as ready for review August 7, 2024 05:07
@mythi mythi requested a review from sameo as a code owner August 7, 2024 05:07
@mythi mythi changed the title chore(deps): Bump kbs-types from 0.6.0 to 0.7.0 Bump kbs-types and kbs_protocol with a KBS protocol version change Aug 7, 2024
@mythi
Copy link
Contributor Author

mythi commented Aug 7, 2024

@Xynnn007 @mkulke @fitzthum this is ready for review. It's needed to get the RCAR test_client tests back in action

Xynnn007
Xynnn007 reviewed Aug 8, 2024
View reviewed changes
Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

Thanks for this. I only have some opens due to compatibility.

The evidence has been tranformed from string into value, which will give opportunity for combined attestation evidences like stated in confidential-containers/confidential-containers#176. Next phase we might need to think about to leverage this, e.g. once GPU attestation joins Trustee Bucket Meal.

cc @imlk0 @zvonkok

kbs/docs/kbs_attestation_protocol.md Show resolved Hide resolved
kbs/src/http/attest.rs Show resolved Hide resolved
@mkulke
Copy link
Contributor

mkulke commented Aug 8, 2024

Thanks for this. I only have some opens due to compatibility.

The evidence has been tranformed from string into value, which will give opportunity for combined attestation evidences like stated in confidential-containers/confidential-containers#176. Next phase we might need to think about to leverage this, e.g. once GPU attestation joins Trustee Bucket Meal.

cc @imlk0 @zvonkok

@Xynnn007 I have opened an issue on kbs-types about this, but maybe we can also discuss it here. I suspect using serde_json on that type can lead to problems, because the internal representation of Value might change between versions of serde_json and then having a different version of serde_json in your tree will break compilation.

Is there any upside of using Value vs just a Vec<u8> for flexible extra_params? For both we need to have a manual parsing step.

huoqifeng
huoqifeng reviewed Aug 8, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@mythi
Copy link
Contributor Author

mythi commented Aug 21, 2024

rebased and force-pushed due to some dependabot PRs being merged to avoid any merge conflicts.

@mythi
Copy link
Contributor Author

mythi commented Aug 26, 2024

Just a reminder that the guest-components side of the same change is already merged and thus keeps some rcar client tests skipped. This also blocks the work with #242. Therefore, it would be good to get this merged. We can get back to virtee/kbs-types#43 once the discussion gets to a conclusion and kbs-types maintainers agree.

fitzthum
fitzthum approved these changes Aug 26, 2024
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. Sorry for the delay. I was trying to find the tab.

kbs/src/http/attest.rs Show resolved Hide resolved
mythi added 3 commits August 27, 2024 20:57
@mythi
kbs: allow only clients with exact protocol version match
8c8c304 
kbs does guarantee backwards compatible functionality so only
clients with exact protocol match are allowed.

Signed-off-by: Mikko Ylinen <[email protected]>
@mythi
chore(deps): Bump kbs-types and kbs_protocol
5f2e03f 
This bumps kbs-types from 0.6.0 to 0.7.0 and kbs_protocol from
guest-components to the latest HEAD. It's done in one commit to
make the KBS protocol changes atomic.

Signed-off-by: Mikko Ylinen <[email protected]>
@mythi
kbs: doc: update protocol spec with the latest functionality
6a1ee5f 
KBS protocol version was bumped up to 0.1.1 so updating the spec
accordingly. In addition, clarify the error handling of "request":
also errors can happen, such as when the "request" version does not
meet all the requirements.

Signed-off-by: Mikko Ylinen <[email protected]>
@mythi
Copy link
Contributor Author

mythi commented Aug 28, 2024

@Xynnn007 this is ready to be merged. It was approved by @fitzthum and also @mkulke earlier

@Xynnn007 Xynnn007 merged commit 5c973bd into confidential-containers:main Aug 28, 2024
16 checks passed
@mythi
Copy link
Contributor Author

mythi commented Aug 28, 2024

Thanks! I have confirmed locally that the guest-components tests are working correctly:

attestation-agent/kbs_protocol$ cargo test client::rcar_client::test::test_client -- --nocapture

stevenhorsman added a commit to stevenhorsman/cloud-api-adaptor that referenced this pull request Sep 24, 2024
@stevenhorsman
initdata: Bump version to 0.1.1
b6b9d31 
Trustee bumped to version 0.1.1 in
confidential-containers/trustee#445 ,
so update our initdata to match

Signed-off-by: stevenhorsman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huoqifeng huoqifeng huoqifeng left review comments

@mkulke mkulke mkulke left review comments

@Xynnn007 Xynnn007 Xynnn007 approved these changes

@fitzthum fitzthum fitzthum approved these changes

@sameo sameo Awaiting requested review from sameo

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

Successfully merging this pull request may close these issues.
5 participants
@mythi @mkulke @fitzthum @huoqifeng @Xynnn007