search

google / go-sev-guest

go-sev-guest offers a library to wrap the /dev/sev-guest device in Linux, as well as a library for attestation verification of fundamental components of an attestation report.
Apache License 2.0
52 stars 19 forks source link

Update attestation_report 'flags' bitmask #47

Closed derpsteb closed 1 year ago

derpsteb commented 1 year ago

I was fetching a report with the lib and was faced with this error message: mbz bits at offset 0x48 not zero: 0x00000004.

Looking at page 44 of the ABI spec here this seems like a valid value. After updating the bitmask with this patch the report was generated.

I am happy to remove the gofumpt formatting if you prefer that. It looked like you also use it, which is why I left it in.

Cheers.

google-cla[bot] commented 1 year ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

derpsteb commented 1 year ago

I just realized that this probably has changed since you built the library since there is now the possibility to have the attestation report signed with a VLEK, instead of a VCEK.

This fix is not correct. Will update later.

deeglaze commented 1 year ago

Thanks for bringing this to my attention, I wasn't aware of the VLEK before this PR. We'll still make sure this generic library stays up to date with the spec if you decide not to pursue a full support PR, just maybe not immediately. Thanks!

Edit: removed part of response with an incorrect understanding of the effect on CSP in TCB.

deeglaze commented 1 year ago

I think for VLEK to be added to go-sev-guest, we need to have a better sense of the whole story so we can have e2e tests with hardware. I know this library doesn't show how to install certificates into /dev/sev, so assuming a loaded VLEK could be okay, but I don't know what the difference will be for VLEK certificate shapes, since our kds library validates expected data about certificates.

I think for now we can extend the ABI representation, but not yet validate VLEK-based attestation reports.

derpsteb commented 1 year ago

Hey! Thanks for the quick response.

re KDS support: the only VLEK documentation I found so far is the ABI spec. It mentioned that the KDS would offer an endpoint to query for the CSP-specific VLEK. However, I couldn't find any information how that API looks like. Playing around with the API I was able to pull a valid certchain from https://kdsintf.amd.com/vlek/v1/Milan/cert_chain. But some official docs would be nice to build out the KDS lib.

re this PR and VLEK support: I am not sure if I am familiar with the code yet to implement full VLEK support. I updated the commit msg to reflect the change more accurately. I am not sure if merging this would introduce incorrect behavior in some places, but it allows me to do what I need. And I don't think it would break anything.

My PoC based on this commit:

Based on this, would you be willing to accept the PR?

deeglaze commented 1 year ago

I think the protocol buffers need to be updated as well. The certificatechain needs the vlek_cert.

I think also the author_key_en field of the attestation report message should be renamed and give new handling in the validation code akin to how the guest policy can be decomposed into its component parts. Decomposition can fail if there are set mbz ranges–I think that interpretation failure would be better than the difficult-to-understand bitmask in use both at tip and in this PR.

Validation options should change to accommodate the new richness of this uint32 field, but I think I can take this responsibility.

When you say you're able to verify a report with a VLEK cert, you're working from a certificate that you've already negotiated a CSP shared secret with AMD using an unpublished KDS API update?

derpsteb commented 1 year ago

The VLEK certificate is part of the extended report. go-sev-guest can retrieve that correctly. From what I understand in the docs, I can treat that like a VCEK and don't have to do anything special.

deeglaze commented 1 year ago

I haven't found the time to fully implement VLEK support with unit tests and fake support, but I'll merge this to unblock. I've opened Issue#53 to track everything that I hope to get to, but alas I can't prioritize that work just yet.