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

snp: ensure we never use ARK supplied by Issuer #3025

Merged
merged 5 commits into from
Apr 12, 2024

Conversation

daniel-weisse
Copy link
Member

@daniel-weisse daniel-weisse commented Apr 12, 2024

Context

Our SNP certificate parsing code potentially sets the ARK to one returned by the Issuer.
While the calling code never actually used this ARK, it still requires a lot of attention by the developer to not accidentally use this potentially unsafe ARK to verify an SNP attestation.

Proposed change(s)

  • Ensure the ARK used by the Validator is never passed through from the Issuer
    • We now completely ignore any ARKs supplied by the Issuer and try to fetch them in the following order:
      • From the user's config / JoinService's cache
      • From the AMD KDS
  • Pass the pre-fetched ASK on GCP to the Validator
  • Use the correct reportData when validating SNP reports on GCP

Additional info

  • Tested this manually on GCP with SEV-SNP to make sure attestation passes and the ASK is actually present in the report.
    • The code is built error safe in case an ASK should not be there for some reason

Checklist

@daniel-weisse daniel-weisse mentioned this pull request Apr 12, 2024
5 tasks
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
@daniel-weisse daniel-weisse marked this pull request as ready for review April 12, 2024 10:17
internal/attestation/snp/snp.go Show resolved Hide resolved
daniel-weisse and others added 3 commits April 12, 2024 14:34
Co-authored-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
Copy link
Contributor

Coverage report

Package Old New Trend
cli/internal/cloudcmd 18.70% 19.00% ↗️
cli/internal/cmd 42.70% 41.10% ↘️
cli/internal/terraform 57.30% 56.80% ↘️
internal/attestation/aws/snp 8.30% 8.20% ↘️
internal/attestation/azure/snp 8.40% 8.20% ↘️
internal/attestation/choose 8.50% 8.30% ↘️
internal/attestation/gcp 12.10% 0.00% ↘️
internal/attestation/gcp/es 0.00% 12.30% 🆕
internal/attestation/gcp/snp 0.00% 0.00% 🆕
internal/attestation/measurements 41.10% 40.70% ↘️
internal/attestation/measurements/measurement-generator 0.60% 0.60% 🚧
internal/attestation/snp 20.80% 20.30% ↘️
internal/attestation/variant 0.00% 0.00% 🚧
internal/attestation/vtpm 5.30% 5.20% ↘️
internal/config 68.60% 68.50% ↘️
internal/constellation/state 74.30% 73.30% ↘️
measurement-reader/cmd 0.00% 0.00% 🚧
terraform-provider-constellation/internal/provider 4.00% 3.60% ↘️

@daniel-weisse daniel-weisse merged commit 5768288 into feat/gcp-sev-snp Apr 12, 2024
5 checks passed
@daniel-weisse daniel-weisse deleted the ref/attestation/snp branch April 12, 2024 13:59
msanft added a commit that referenced this pull request Apr 16, 2024
* Make sure SNP ARK is always loaded from config, or fetched from AMD KDS
* GCP: Set validator `reportData` correctly

---------

Signed-off-by: Daniel Weiße <dw@edgeless.systems>
Co-authored-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
msanft added a commit that referenced this pull request Apr 16, 2024
* terraform: enable creation of SEV-SNP VMs on GCP

* variant: add SEV-SNP attestation variant

* config: add SEV-SNP config options for GCP

* measurements: add GCP SEV-SNP measurements

* gcp: separate package for SEV-ES

* attestation: add GCP SEV-SNP attestation logic

* gcp: factor out common logic

* choose: add GCP SEV-SNP

* cli: add TF variable passthrough for GCP SEV-SNP variables

* cli: support GCP SEV-SNP for `constellation verify`

* Adjust usage of GCP SEV-SNP throughout codebase

* ci: add GCP SEV-SNP

* terraform-provider: support GCP SEV-SNP

* docs: add GCP SEV-SNP reference

* linter fixes

* gcp: only run test with TPM simulator

* gcp: remove nonsense test

* Update cli/internal/cmd/verify.go

Co-authored-by: Daniel Weiße <66256922+daniel-weisse@users.noreply.github.com>

* Update docs/docs/overview/clouds.md

Co-authored-by: Daniel Weiße <66256922+daniel-weisse@users.noreply.github.com>

* Update terraform-provider-constellation/internal/provider/attestation_data_source_test.go

Co-authored-by: Adrian Stobbe <stobbe.adrian@gmail.com>

* linter fixes

* terraform_provider: correctly pass down CC technology

* config: mark attestationconfigapi as unimplemented

* gcp: fix comments and typos

* snp: use nonce and PK hash in SNP report

* snp: ensure we never use ARK supplied by Issuer (#3025)

* Make sure SNP ARK is always loaded from config, or fetched from AMD KDS
* GCP: Set validator `reportData` correctly

---------

Signed-off-by: Daniel Weiße <dw@edgeless.systems>
Co-authored-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* attestationconfigapi: add GCP to uploading

* snp: use correct cert

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* terraform-provider: enable fetching of attestation config values for GCP SEV-SNP

* linter fixes

---------

Signed-off-by: Daniel Weiße <dw@edgeless.systems>
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Co-authored-by: Daniel Weiße <66256922+daniel-weisse@users.noreply.github.com>
Co-authored-by: Adrian Stobbe <stobbe.adrian@gmail.com>
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.

2 participants