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

add: snp updates and mods to support VLEK #385

Merged
merged 9 commits into from
May 17, 2024
Merged

add: snp updates and mods to support VLEK #385

merged 9 commits into from
May 17, 2024

Conversation

wobito
Copy link
Contributor

@wobito wobito commented May 13, 2024 •
edited
Loading

This PR will add support for V{C|L}EK from amd snp-sev, this will also bump the sev crate from 1.2.0 to 3.1.1 with hope to close #286

Currently for the trustee sources to compile the azure-cvm-tooling needed to have the sev crate bump as well, that is why there is a change in the cargo for those packages, this would need kinvolk/azure-cvm-tooling#52 to land before we can change the package back to it upstream provider.
This was completed with the merge of kinvolk/azure-cvm-tooling#52

This pr also references a crate bump on sylabs/guest-components until a patch can be contributed upstream there as well.
This was completed with the merge of confidential-containers/guest-components#555

@wobito wobito requested a review from sameo as a code owner May 13, 2024 21:04
@wobito wobito mentioned this pull request May 13, 2024
Merged
@wobito
add: snp updates and mods
6356070 
Signed-off-by: Adrian Wobito <[email protected]>
fitzthum
fitzthum reviewed May 13, 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.

Looks great. I made some comments. Please add a commit message.

Also, is this PR really AWS specific or does it just add VLEK support?

attestation-service/verifier/src/snp/milan_ask_ark.pem Outdated Show resolved Hide resolved
kbs/tools/client/Cargo.toml Outdated Show resolved Hide resolved
attestation-service/verifier/src/snp/mod.rs Outdated Show resolved Hide resolved
attestation-service/verifier/src/snp/mod.rs Outdated Show resolved Hide resolved
attestation-service/verifier/Cargo.toml Outdated Show resolved Hide resolved
attestation-service/verifier/src/snp/mod.rs Outdated Show resolved Hide resolved
attestation-service/verifier/src/snp/mod.rs Outdated Show resolved Hide resolved
attestation-service/verifier/src/snp/mod.rs Outdated Show resolved Hide resolved
attestation-service/verifier/src/snp/mod.rs Show resolved Hide resolved
attestation-service/verifier/src/snp/mod.rs Outdated Show resolved Hide resolved
@wobito
Copy link
Contributor Author

wobito commented May 13, 2024

Thanks again for the comments. Will work on this tomorrow and resolve the issues

Greatly appreciate the review so quickly!

Xynnn007
Xynnn007 reviewed May 14, 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.

Welcome to the community @wobito ! Only some code style things and I also recommend that upstream first and then we can get this PR perfect and merge.

attestation-service/verifier/src/snp/mod.rs Outdated Show resolved Hide resolved
attestation-service/verifier/src/snp/mod.rs Outdated Show resolved Hide resolved
@wobito wobito changed the title add: snp updates and mods to support AWS add: snp updates and mods to support VLEK May 14, 2024
wobito added 3 commits May 14, 2024 12:29
@wobito
update: refactor based on review
780d115 
Signed-off-by: Adrian Wobito <[email protected]>
@wobito
update: check_milan_certificates
01e5008 
Signed-off-by: Adrian Wobito <[email protected]>
@wobito
add: test for VLEK
40e3123 
Signed-off-by: Adrian Wobito <[email protected]>
@wobito wobito requested review from fitzthum and Xynnn007 May 14, 2024 17:09
@wobito
bump: az-cvm-tooling version
13f1a32 
Signed-off-by: Adrian Wobito <[email protected]>
mkulke
mkulke reviewed May 15, 2024
View reviewed changes
attestation-service/verifier/src/snp/mod.rs Outdated
ark: &X509,
asvk: &X509,
) -> Result<X509> {
let raw_vek = cert_chain
Copy link
Contributor

Choose a reason for hiding this comment

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

we might want to avoid introducing new abbreviations, since people will search for it and get confusing results. it's fine as a variable name, but in strings we can spell it out ("VCEK or VLEK not found") maybe.

attestation-service/verifier/src/snp/mod.rs Outdated Show resolved Hide resolved
attestation-service/verifier/src/snp/mod.rs Outdated Show resolved Hide resolved
kbs/tools/client/Cargo.toml Outdated Show resolved Hide resolved
@wobito
Copy link
Contributor Author

wobito commented May 15, 2024

I made the requested changes. I appreciate the quick feedback! :D

still finding the sweet spot with rust ;P

@wobito wobito requested review from mkulke and Xynnn007 May 15, 2024 13:44
mkulke
mkulke approved these changes May 15, 2024
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.

small nit, otherwise lgtm

kbs/tools/client/Cargo.toml Outdated Show resolved Hide resolved
fitzthum
fitzthum reviewed May 15, 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.

Ok, looks really close to me. A couple small nits.

attestation-service/verifier/src/snp/mod.rs Outdated Show resolved Hide resolved
attestation-service/verifier/src/snp/mod.rs Outdated Show resolved Hide resolved
attestation-service/verifier/src/snp/mod.rs Outdated Show resolved Hide resolved
fitzthum
fitzthum approved these changes May 15, 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. Thanks @wobito

@Xynnn007 feel free to merge if your comments are addressed

Xynnn007
Xynnn007 approved these changes May 16, 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.

LGTM. Only one last thing

attestation-service/verifier/src/snp/mod.rs Show resolved Hide resolved
@wobito
update: get common name method
b5e4ad4 
Signed-off-by: Adrian Wobito <[email protected]>
@wobito wobito mentioned this pull request May 16, 2024
Closed
@fitzthum
Copy link
Member

Wait, I think we need to update the evidence struct here still.

@wobito
Copy link
Contributor Author

wobito commented May 17, 2024

Forgive me, but can you provide some more information so I can help in anyway

@mkulke
Copy link
Contributor

mkulke commented May 17, 2024

Forgive me, but can you provide some more information so I can help in anyway

afaiu in the corresponding guest-components change the evidence struct was modified, we need to do the same here, otherwise the evidence will not be parsed by the verifier. there is currently no CI job that would catch this.

@wobito
Copy link
Contributor Author

wobito commented May 17, 2024

Thank you 🙏

I believe I understand that now
I'll take a pass at it

@wobito
update: evidence struct
f900c0e 
Signed-off-by: Adrian Wobito <[email protected]>
@fitzthum
Copy link
Member

Ok, I think we're good now. We do have a baremetal SNP runner that we could use for an e2e test in the future.

@mkulke
Copy link
Contributor

mkulke commented May 17, 2024

Ok, I think we're good now. We do have a baremetal SNP runner that we could use for an e2e test in the future.

that would be useful. the e2e tests themselves should not very invasive, but they do install a bunch of dependencies on the runner that might taint the machine. Maybe we can scrutinize this and run the tests in containers (privileged, w/ tee hw-devices mounted).

@fitzthum fitzthum merged commit 0005ddb into confidential-containers:main May 17, 2024
17 checks passed
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

@Xynnn007 Xynnn007 Xynnn007 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.

Option to use Versioned Loaded Endorsement Key (VLEK) for verifying SNP attestation report in AWS
4 participants
@wobito @fitzthum @mkulke @Xynnn007