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

Improve KBS protocol version handling and bump the version to v0.1.1 due to kbs-types changes #628

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

mythi
Copy link
Contributor

@mythi mythi commented Jul 25, 2024

@fitzthum
Copy link
Member

whoops, didn't mean to make this ready for review

@fitzthum fitzthum marked this pull request as draft July 29, 2024 18:11
@mythi
Copy link
Contributor Author

mythi commented Jul 31, 2024

This PR now depends on getting confidential-containers/trustee#449 merged and released with :latest kbs image.

@mythi mythi marked this pull request as ready for review July 31, 2024 10:41
@mythi
Copy link
Contributor Author

mythi commented Jul 31, 2024

This PR now depends on getting confidential-containers/trustee#449 merged and released with :latest kbs image.

:latest tag does not seem to get updated due to pending s390x jobs.

@mythi mythi changed the title chore(deps): Bump kbs-types from 0.6.0 to 0.7.0 Improve KBS protocol version handling and bump the version to v0.1.1 due to kbs-types changes Jul 31, 2024
@mythi mythi force-pushed the deps-kbs-types branch 2 times, most recently from 10a95c5 to 783b154 Compare August 1, 2024 04:52
@mythi
Copy link
Contributor Author

mythi commented Aug 1, 2024

rebased to include the latest Cargo.toml updates that were merged

@mkulke @fitzthum this PR now breaks the cyclic dependency in a way that we can continue to use test_client test.

@mythi
Copy link
Contributor Author

mythi commented Aug 6, 2024

@Xynnn007 PTAL also

Copy link
Member

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks, @mythi.

lgtm!

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.

lgtm

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.

Sorry for the delay in reviewing. Seems good. One suggestion

attestation-agent/kbs_protocol/src/client/rcar_client.rs Outdated Show resolved Hide resolved
The RCAR client code currently ignores any errors for "request". Such errors
can still happen, e.g., when 'version' field is rejected by KBS.

Without catching errors we try to decode the Challenge json body but it actually
contains the error information in it which results in decode errors instead.

KBS added a new ProtocolVersion error which is now catched by the RCAR
client code. The error is reported to the user if the client and server
use incompatible versions.

Signed-off-by: Mikko Ylinen <[email protected]>
The current setup with trustee repo for KBS server and guest-components
repo for kbs_protocol client code has a cyclic dependency problem.

test_client test uses "KBS latest" container image which won't work if
any RCAR client code changes that change the protocol semantics are made.

The suggested approach to break the cyclic dependency is to get the RCAR
client changes merged first with a KBS protocol version bump. The
RCAR client test is modified to skip the checks in case the "KBS
latest" server returns ProtocolVersion error.

Following this, the corresponding KBS server changes are made in trustee
repo with an update to both kbs_protocol for kbs-client builds and the
server supported protocol version.

Signed-off-by: Mikko Ylinen <[email protected]>
kbs-types introduces a break in JSON semantics so we bump the
kbs protocol version from 0.1.0 to 0.1.1.

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 cd16b44 into confidential-containers:main Aug 6, 2024
20 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.

5 participants