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

AA/kbs_protocol: fix RCAR handshake protocol #406

Merged
merged 2 commits into from
Dec 22, 2023

Conversation

Xynnn007
Copy link
Member

Before this commit, the tee-pubkey is not fully integrity-protected by binding the digest into the evidence. The update of this commit is aligned with the KBS side.

Fixes #366

cc @mythi

@Xynnn007
Copy link
Member Author

The test failure is as expected because the runtime data changes.

Comment on lines +137 to +140
let runtime_data = json!({
"tee-pubkey": tee_pubkey,
"nonce": challenge.nonce,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason why to combine them? My expectation was to have it the same way it's implemented in #369

Copy link
Member Author

Choose a reason for hiding this comment

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

no matter Nonce or tee pub key, they all are things whose integrity should be protected by binding the evidence. There must be more scenarios except for the two fields.

We use a json map here to have a standard way to organize them

Copy link
Contributor

Choose a reason for hiding this comment

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

this contributes to the hashing problem we've talked about. it's not only what hash algorithm is used but also what data is hashed. this won't work well with Intel Trust Authority which makes me think that perhaps we are going to need AS specific cfg flags to decide how the reportdata needs to be generated by the attesters

Copy link
Member Author

Choose a reason for hiding this comment

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

see the runtime data spec confidential-containers/trustee#259

This would be a good start to make runtime data usage more standard

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks! I saw it earlier but forgot to take a closer look. I'll do it today.

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. This seems closely tied to confidential-containers/trustee#259 but I guess we can implement that separately.

Before this commit, the tee-pubkey is not fully integrity-protected by
binding the digest into the evidence. The update of this commit is
aligned with the KBS side.

Fixes confidential-containers#366

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007 Xynnn007 requested a review from mythi December 17, 2023 02:37
The newest KBS uses a configuration file to launch. The new KBS image
was built after merging commit
confidential-containers/trustee@f141a78#diff-16118c763a8e72d853ec0c7c5d6e8355496c41d1b3da27bf0e4c432a2ea158a8

That commit updates the KBS side RCAR runtime data definition.

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007
Copy link
Member Author

cc @mythi Is this PR now good to you?

Xynnn007 added a commit to Xynnn007/kbs that referenced this pull request Dec 22, 2023
This is also a workaround. Once
confidential-containers/guest-components#406
is merged, we will change the dep rev to upstream repo.

Signed-off-by: Xynnn007 <[email protected]>
Xynnn007 added a commit to Xynnn007/kbs that referenced this pull request Dec 22, 2023
This is also a workaround. Once
confidential-containers/guest-components#406
is merged, we will change the dep rev to upstream repo.

Signed-off-by: Xynnn007 <[email protected]>
Xynnn007 added a commit to Xynnn007/kbs that referenced this pull request Dec 22, 2023
This is also a workaround. Once
confidential-containers/guest-components#406
is merged, we will change the dep rev to upstream repo.

Signed-off-by: Xynnn007 <[email protected]>
Xynnn007 added a commit to Xynnn007/kbs that referenced this pull request Dec 22, 2023
This is also a workaround. Once
confidential-containers/guest-components#406
is merged, we will change the dep rev to upstream repo.

Signed-off-by: Xynnn007 <[email protected]>
Xynnn007 added a commit to confidential-containers/trustee that referenced this pull request Dec 22, 2023
This is also a workaround. Once
confidential-containers/guest-components#406
is merged, we will change the dep rev to upstream repo.

Signed-off-by: Xynnn007 <[email protected]>
Copy link
Contributor

@mythi mythi left a comment

Choose a reason for hiding this comment

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

+1

@Xynnn007 Xynnn007 merged commit 1e76429 into confidential-containers:main Dec 22, 2023
9 checks passed
@Xynnn007 Xynnn007 deleted the fix-kbs-protocol branch December 22, 2023 05:39
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.

kbs_protocol does not follow the KBS spec
3 participants