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

contrast: remove default SVNs for TDX #916

Merged
merged 2 commits into from
Oct 7, 2024
Merged

Conversation

Freax13
Copy link
Contributor

@Freax13 Freax13 commented Oct 7, 2024

We don't ship minimum default SVNs for bare metal, so we shouldn't have a default for minimumTeeTcbSvn. Having a default for mrSeam is problematic because it legitimately can change as a customer updates their firmware.

@Freax13 Freax13 added the no changelog PRs not listed in the release notes label Oct 7, 2024
@katexochen
Copy link
Member

I think we need documentation to instruct users how to fill these values if we don't have defaults, right?

We don't ship minimum default SVNs for bare metal, so we shouldn't have
a default for minimumTeeTcbSvn. Having a default for mrSeam is
problematic because it legitimately can change as a customer updates
their firmware.
Copy link

github-actions bot commented Oct 7, 2024

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2024-10-07 14:02 UTC

@Freax13 Freax13 requested a review from burgerdev October 7, 2024 10:58
:::note[Missing TCB values]
On bare metal SEV-SNP, `contrast generate` is unable to fill in the `MinimumTCB` values as they can vary between platforms.
They will have to be filled in manually.
If you don't know the correct values use `{"BootloaderVersion":255,"TEEVersion":255,"SNPVersion":255,"MicrocodeVersion":255}` and observe the correct values in the error messages in the following steps. Note that the values will differ between CPU models.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I feel the word "correct" needs some explanation around trust-on-first-use. Even better would be some instructions for deriving the values directly on the host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nit: I feel the word "correct" needs some explanation around trust-on-first-use.

Will do.

Even better would be some instructions for deriving the values directly on the host.

AFAICT there's no interface to ask the host kernel about the active values and I don't think the TDX module even exposes that. AFAICT the only way to get the values is to launch a virtual machine and create a quote. While this is technically feasible, as of right now, I don't think there's existing tooling for users to do this, though I'd be more than happy to write some.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would sure be great to have something like snphost for TDX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's tdxhost, but it only implements this ok subcommand.

We could implement a tool to launch a minimal VM and request an attestation report for it. This could work on TDX and SNP.

@katexochen what are your thoughts?

Comment on lines +212 to +213
// The generate command doesn't fill in all required fields when
// generating a manifest for baremetal TDX. Do that now.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to inject the reference values both here and in the justfile? I'd prefer if we always configured them externally, because they depend only on the runtime environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code isn't executed when running the default just target and the code in the justfile isn't executed when running the e2e tests. In both cases, this only affects our development workflows and so we don't expect this to change unless we upgrade the firmware on our server.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking along the lines of an e2e test command line argument, but we don't need to do that right now.

@Freax13 Freax13 merged commit 48c1230 into main Oct 7, 2024
10 checks passed
@Freax13 Freax13 deleted the tom/remove-tdx-defaults branch October 7, 2024 14:00
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.

3 participants