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

attestation: add TDX validator #768

Merged
merged 3 commits into from
Aug 20, 2024
Merged

attestation: add TDX validator #768

merged 3 commits into from
Aug 20, 2024

Conversation

msanft
Copy link
Contributor

@msanft msanft commented Jul 29, 2024

This adds a preliminary TDX validator. It is not yet used throughout the code, nor does it implement certificate extensions like the SNP one does. But to not have the PR size explode, and since this is an independent component, I opted to put it into a separate PR with this.

Sent a review request to @daniel-weisse for the go-tdx-guest code.

@msanft msanft added the no changelog PRs not listed in the release notes label Jul 29, 2024
@msanft msanft requested a review from katexochen as a code owner July 29, 2024 18:36
@msanft msanft force-pushed the msanft/tdx-validator branch from 275274c to 7aa2786 Compare July 29, 2024 18:40
@msanft msanft requested a review from Freax13 July 29, 2024 19:18
Copy link
Contributor

@burgerdev burgerdev left a comment

Choose a reason for hiding this comment

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

I understand that there are validations to be added in subsequent PRs. Could you please leave a TODO for them to document that something is missing and what this is?

internal/attestation/tdx/validator.go Show resolved Hide resolved
internal/attestation/tdx/validator.go Outdated Show resolved Hide resolved
@msanft
Copy link
Contributor Author

msanft commented Jul 30, 2024

I understand that there are validations to be added in subsequent PRs. Could you please leave a TODO for them to document that something is missing and what this is?

There should not be (at least not intentionally) any validations missing. The only thing that's missing related to validation is the options generator which takes a set of reference values and transforms them into something that can be understood by the TDX library. As this is required to use the validator implemented in this PR by design (i.e. since no other type implements the required interface, as of now), I don't think leaving a TODO for that makes much sense.

The certificate extension mechanism I talk about in the PR description refers to the embedding of SNP attestation data into the certificate issued by the mesh authority, which, to my understanding, is not integral to an MVP for TDX attestation. Since it'll require quite a lot of code, and I want to keep my PRs easy to review, I simply opted not to do that in this PR.

@msanft msanft force-pushed the msanft/tdx-validator branch from 7aa2786 to 871fd1a Compare July 30, 2024 06:49
@msanft msanft requested review from burgerdev and Freax13 July 30, 2024 06:49
Comment on lines +105 to +106
quote := &tdx.QuoteV4{}
if err := proto.Unmarshal(attDocRaw, quote); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Why does the Issuer return a marshalled proto quote, instead of just the raw quote?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure actually. I think the raw report should even be shorter, as it's non-self-describing too

@msanft msanft force-pushed the msanft/tdx-validator branch 2 times, most recently from f10c6a7 to f8c3549 Compare July 30, 2024 07:03
@msanft msanft force-pushed the msanft/tdx-trusted-measurement-embedding branch 4 times, most recently from 1b3d3c0 to fda28a4 Compare July 30, 2024 13:19
Base automatically changed from msanft/tdx-trusted-measurement-embedding to main July 30, 2024 13:37
@msanft msanft force-pushed the msanft/tdx-validator branch 2 times, most recently from 6840d6a to 40d9d6d Compare July 31, 2024 06:44
@Freax13
Copy link
Contributor

Freax13 commented Aug 12, 2024

What's the status of this PR?

@msanft
Copy link
Contributor Author

msanft commented Aug 12, 2024

What's the status of this PR?

Theoretically we could rebase and merge this already, without actually using the validator.

However, since #783 looks ok to be reviewed and merged too (which I expected to not be the case this fast), i'd advocate to merge that one first.

@msanft msanft force-pushed the msanft/tdx-validator branch from 40d9d6d to 757133e Compare August 19, 2024 15:23
@Freax13
Copy link
Contributor

Freax13 commented Aug 20, 2024

CI is not happy yet.

msanft added 3 commits August 20, 2024 10:05
The TDX validator will also require a mechanism to store certificates, and as we have such code already present for SNP, just use it for both.
This adds a basic implementation of a TDX-capable validator. It's not yet used throughout the code, and it doesn't implement certificate extensions yet.
This aligns the code structure and error messages of the SNP validator to those of the TDX validator.
@Freax13 Freax13 force-pushed the msanft/tdx-validator branch from 757133e to 1d2dc39 Compare August 20, 2024 08:17
@Freax13
Copy link
Contributor

Freax13 commented Aug 20, 2024

Rebased onto main.

@Freax13 Freax13 merged commit 2aafba5 into main Aug 20, 2024
11 checks passed
@Freax13 Freax13 deleted the msanft/tdx-validator branch August 20, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants