aws: reintroduce SNP-based attestation #2601

derpsteb · 2023-11-14T13:01:58Z

Context

As far as we can tell AWS has now fixed all issues regarding booting SNP machines and generating attestation reports.
Thus we want to re enable SNP-based attestation to Constellation. Since multiple aspects changed (attesationconfigapi, verify cmd, go-sev-guest for validation) since the original implementation of AWS SNP attestation, this PR is a superset of the original implementation in #1916.

This PR refactors the verify command, the attestationconfigapi client & fetcher and the attestationconfigapi CLI in order to not introduce duplicate code.

Proposed change(s)

refactor verify to be more CSP agnostic.
add the actual attestation implementation. The default TCB values are hardcoded and replaced in a later commit.
adjusts the verify command to fetch and handle VLEK certificates. Without this, VLEKs are not displayed correctly (cspid vs hwid) and no certificate chain is fetched.
refactor internal/api/attestationconfigapi to receive a variant argument. Previously the whole code was built for Azure. By introducing a variant argument we can reuse the code for AWS.
Add support to attestationconfigapi CLI for a csp and kind argument. The CSP argument is used to select whether to upload an snp-report for azure or aws. The kind argument will be used to upload launchmeasurement metadata. This was planned for this PR originally, which is why it is included here. However, we decided to leave it out for now as some open points need to be discussed.
Adjust CI to upload values for AWS

Checklist

Update docs
Add labels (e.g., for changelog category)
Is PR title adequate for changelog?
Link to Milestone

netlify · 2023-11-14T13:02:14Z

✅ Deploy Preview for constellation-docs ready!

Name	Link
🔨 Latest commit	`1891fcf`
🔍 Latest deploy log	https://app.netlify.com/sites/constellation-docs/deploys/6560aa5b2f607a00085ca71b
😎 Deploy Preview	https://deploy-preview-2601--constellation-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

daniel-weisse

Didn't look at any of the CLI stuff yet, and didnt do an indepth review of the attestation parts

internal/api/attestationconfigapi/cli/main.go

internal/api/attestationconfigapi/cli/main_test.go

internal/api/attestationconfigapi/cli/delete_test.go

internal/api/attestationconfigapi/cli/delete.go

internal/attestation/aws/snp/validator.go

internal/api/attestationconfigapi/cli/e2e/test.sh.in

internal/attestation/aws/snp/validator.go

internal/attestation/snp/snp_test.go

elchead

I mostly reviewed the attestationconfigapi relevant changes. we should add tests as daniel mentioned, but otherwise lgtm

internal/api/attestationconfigapi/cli/delete.go

internal/api/attestationconfigapi/cli/upload.go

internal/api/attestationconfigapi/snp.go

internal/api/attestationconfigapi/reporter.go

internal/api/attestationconfigapi/fetcher.go

internal/api/attestationconfigapi/fetcher_test.go

internal/api/attestationconfigapi/cli/delete.go

derpsteb · 2023-11-17T08:26:54Z

Regarding the CLI UX: since Daniel said he would be fine with the current proposal if the CLI is only used in the CI, which it is, I would keep things as is for now. If Moritz or Thomas don't raise more concerns.

msanft

Looked at the attestation-specific changes, which lgtm mostly

msanft · 2023-11-16T08:10:37Z

internal/verify/verify_test.go

+)
+
+func TestParseCerts(t *testing.T) {
+	validCert := `-----BEGIN CERTIFICATE-----


nit: Not a huge fan of having these inline. We could possibly move them to a testdata package too. But only a personal preference.

I moved all the values you commented on to a testdata package. But instead of embedding them I put the values into constants to prevent accidental overwrites. Let's talk in case you want to stick to our current approach.

internal/api/attestationconfigapi/cli/main.go

internal/attestation/aws/snp/errors.go

internal/attestation/aws/snp/issuer.go

internal/attestation/aws/snp/validator.go

internal/attestation/aws/snp/validator_test.go

internal/attestation/snp/snp_test.go

msanft · 2023-11-17T09:20:21Z

internal/config/azure.go

@@ -17,6 +17,9 @@ import (
 	"github.com/edgelesssys/constellation/v2/internal/cloud/cloudprovider"
 )

+// ARKPEM is the PEM encoded AMD root key. Received from the AMD Key Distribution System API (KDS).
+const ARKPEM = `-----BEGIN CERTIFICATE-----\nMIIGYzCCBBKgAwIBAgIDAQAAMEYGCSqGSIb3DQEBCjA5oA8wDQYJYIZIAWUDBAIC\nBQChHDAaBgkqhkiG9w0BAQgwDQYJYIZIAWUDBAICBQCiAwIBMKMDAgEBMHsxFDAS\nBgNVBAsMC0VuZ2luZWVyaW5nMQswCQYDVQQGEwJVUzEUMBIGA1UEBwwLU2FudGEg\nQ2xhcmExCzAJBgNVBAgMAkNBMR8wHQYDVQQKDBZBZHZhbmNlZCBNaWNybyBEZXZp\nY2VzMRIwEAYDVQQDDAlBUkstTWlsYW4wHhcNMjAxMDIyMTcyMzA1WhcNNDUxMDIy\nMTcyMzA1WjB7MRQwEgYDVQQLDAtFbmdpbmVlcmluZzELMAkGA1UEBhMCVVMxFDAS\nBgNVBAcMC1NhbnRhIENsYXJhMQswCQYDVQQIDAJDQTEfMB0GA1UECgwWQWR2YW5j\nZWQgTWljcm8gRGV2aWNlczESMBAGA1UEAwwJQVJLLU1pbGFuMIICIjANBgkqhkiG\n9w0BAQEFAAOCAg8AMIICCgKCAgEA0Ld52RJOdeiJlqK2JdsVmD7FktuotWwX1fNg\nW41XY9Xz1HEhSUmhLz9Cu9DHRlvgJSNxbeYYsnJfvyjx1MfU0V5tkKiU1EesNFta\n1kTA0szNisdYc9isqk7mXT5+KfGRbfc4V/9zRIcE8jlHN61S1ju8X93+6dxDUrG2\nSzxqJ4BhqyYmUDruPXJSX4vUc01P7j98MpqOS95rORdGHeI52Naz5m2B+O+vjsC0\n60d37jY9LFeuOP4Meri8qgfi2S5kKqg/aF6aPtuAZQVR7u3KFYXP59XmJgtcog05\ngmI0T/OitLhuzVvpZcLph0odh/1IPXqx3+MnjD97A7fXpqGd/y8KxX7jksTEzAOg\nbKAeam3lm+3yKIcTYMlsRMXPcjNbIvmsBykD//xSniusuHBkgnlENEWx1UcbQQrs\n+gVDkuVPhsnzIRNgYvM48Y+7LGiJYnrmE8xcrexekBxrva2V9TJQqnN3Q53kt5vi\nQi3+gCfmkwC0F0tirIZbLkXPrPwzZ0M9eNxhIySb2npJfgnqz55I0u33wh4r0ZNQ\neTGfw03MBUtyuzGesGkcw+loqMaq1qR4tjGbPYxCvpCq7+OgpCCoMNit2uLo9M18\nfHz10lOMT8nWAUvRZFzteXCm+7PHdYPlmQwUw3LvenJ/ILXoQPHfbkH0CyPfhl1j\nWhJFZasCAwEAAaN+MHwwDgYDVR0PAQH/BAQDAgEGMB0GA1UdDgQWBBSFrBrRQ/fI\nrFXUxR1BSKvVeErUUzAPBgNVHRMBAf8EBTADAQH/MDoGA1UdHwQzMDEwL6AtoCuG\nKWh0dHBzOi8va2RzaW50Zi5hbWQuY29tL3ZjZWsvdjEvTWlsYW4vY3JsMEYGCSqG\nSIb3DQEBCjA5oA8wDQYJYIZIAWUDBAICBQChHDAaBgkqhkiG9w0BAQgwDQYJYIZI\nAWUDBAICBQCiAwIBMKMDAgEBA4ICAQC6m0kDp6zv4Ojfgy+zleehsx6ol0ocgVel\nETobpx+EuCsqVFRPK1jZ1sp/lyd9+0fQ0r66n7kagRk4Ca39g66WGTJMeJdqYriw\nSTjjDCKVPSesWXYPVAyDhmP5n2v+BYipZWhpvqpaiO+EGK5IBP+578QeW/sSokrK\ndHaLAxG2LhZxj9aF73fqC7OAJZ5aPonw4RE299FVarh1Tx2eT3wSgkDgutCTB1Yq\nzT5DuwvAe+co2CIVIzMDamYuSFjPN0BCgojl7V+bTou7dMsqIu/TW/rPCX9/EUcp\nKGKqPQ3P+N9r1hjEFY1plBg93t53OOo49GNI+V1zvXPLI6xIFVsh+mto2RtgEX/e\npmMKTNN6psW88qg7c1hTWtN6MbRuQ0vm+O+/2tKBF2h8THb94OvvHHoFDpbCELlq\nHnIYhxy0YKXGyaW1NjfULxrrmxVW4wcn5E8GddmvNa6yYm8scJagEi13mhGu4Jqh\n3QU3sf8iUSUr09xQDwHtOQUVIqx4maBZPBtSMf+qUDtjXSSq8lfWcd8bLr9mdsUn\nJZJ0+tuPMKmBnSH860llKk+VpVQsgqbzDIvOLvD6W1Umq25boxCYJ+TuBoa4s+HH\nCViAvgT9kf/rBq1d+ivj6skkHxuzcxbk1xv6ZGxrteJxVH7KlX7YRdZ6eARKwLe4\nAFZEAwoKCQ==\n-----END CERTIFICATE-----\n`


We could move this to a testdata package too, or make it readable within the file.

I would like this to stay a constant as it is used during aTLS verification in the CLI. Overwriting this value on accident would be less than ideal.

True, valid point.

elchead

could you run verify on AWS and include the upload step on your PR branch to test that everything works as expected? otherwise lgtm.

derpsteb · 2023-11-20T10:45:00Z

Verify run with patched condition: 🟢

derpsteb · 2023-11-20T11:30:53Z

had to rebase to use latest image in pipeline. didn't want to use outdated image.

derpsteb · 2023-11-21T07:27:09Z

Testrun ~~was successful and API is serving SVNs that mitigate CacheWarp~~is pending. I will wait for a comment from @thomasten and then revert the CI change (allowing non-main TCB value uploads), rebase, squash fixups and merge.

.github/actions/e2e_verify/action.yml

thomasten

just some nits

docs/docs/architecture/attestation.md

internal/attestation/aws/snp/issuer.go

internal/attestation/aws/snp/validator.go

internal/attestation/snp/snp.go

internal/config/azure.go

The package holds code shared between SNP-based attestation implementations on AWS and Azure .

This ensure that issuer and verify (as consumer) use the same types for marshalling/unmarshalling.

With the introduction of SNP-based attestation on AWS some of the information in the report (MAAToken) is not applicable to all attestation reports anymore. Thus, make verify cmd CSP-agnostic and move CSP-specific logic to internal/verify. Also make internal/attestation/snp CSP aware.

The user can choose to supply an intermediate certificate through the config, like they can for the root key. If none is supplied, the KDS is queried for a valid ASK.

The cli now takes CSP and object kind as argument. Also made upload an explicit command and the report path/version an argument. Previously the report was a flag. The CSP was hardcoded. There was only one object kind (snp-report).

There is now one SEVSNPVersions type that has a variant property. That property is used to build the correct JSON path. The surrounding methods handling the version objects are also updated to receive a variant argument and work for multiple variants. This simplifies adding AWS support.

The attestationconfig api CLI now uploads SNP TCB versions for AWS.

If no TCB value is set to `latest`, the fetcher is now no longer called.

derpsteb · 2023-11-24T13:52:09Z

Final testrun with all suggestions and no temporary ci modifications: https://github.com/edgelesssys/constellation/actions/runs/6981570312

github-actions · 2023-11-24T14:00:23Z

Coverage report

Package	Old	New	Trend
cli/internal/cmd	54.60%	58.70%	↗️
internal/api/attestationconfigapi	31.00%	32.60%	↗️
internal/api/attestationconfigapi/cli	10.90%	[no test files]	↘️
internal/attestation/aws/snp	43.90%	32.60%	↘️
internal/attestation/azure/snp	49.00%	24.40%	↘️
internal/attestation/snp	0.00%	85.30%	🆕
internal/config	80.20%	79.20%	↘️
internal/verify	[no test files]	17.10%	↗️
joinservice/internal/certcache	73.60%	70.70%	↘️
joinservice/internal/watcher	78.90%	73.70%	↘️

derpsteb added the feature This introduces new functionality label Nov 14, 2023

derpsteb requested a review from daniel-weisse as a code owner November 14, 2023 13:01

derpsteb marked this pull request as draft November 14, 2023 13:14

derpsteb force-pushed the feat/attestation/aws-snp branch from 3054ecb to 2def25d Compare November 14, 2023 15:01

daniel-weisse reviewed Nov 14, 2023

View reviewed changes

derpsteb force-pushed the feat/attestation/aws-snp branch 2 times, most recently from ba17461 to 4654cf5 Compare November 15, 2023 10:03

elchead requested review from daniel-weisse and elchead November 15, 2023 10:21

derpsteb force-pushed the feat/attestation/aws-snp branch from 4654cf5 to 27f20a1 Compare November 15, 2023 10:23

derpsteb marked this pull request as ready for review November 15, 2023 10:55

derpsteb requested a review from msanft November 15, 2023 10:56

derpsteb requested a review from thomasten as a code owner November 15, 2023 11:12

daniel-weisse reviewed Nov 15, 2023

View reviewed changes

elchead reviewed Nov 16, 2023

View reviewed changes

msanft reviewed Nov 17, 2023

View reviewed changes

elchead approved these changes Nov 20, 2023

View reviewed changes

derpsteb force-pushed the feat/attestation/aws-snp branch from 45e86a3 to f64052b Compare November 20, 2023 11:29

daniel-weisse approved these changes Nov 20, 2023

View reviewed changes

derpsteb force-pushed the feat/attestation/aws-snp branch 2 times, most recently from 258179d to 6970baa Compare November 20, 2023 15:59

elchead reviewed Nov 21, 2023

View reviewed changes

.github/actions/e2e_verify/action.yml Outdated Show resolved Hide resolved

thomasten approved these changes Nov 21, 2023

View reviewed changes

elchead mentioned this pull request Nov 24, 2023

terraform-provider: add attestation data source #2640

Merged

4 tasks

derpsteb added this to the v2.14.0 milestone Nov 24, 2023

derpsteb added 12 commits November 24, 2023 14:50

attestation: add snp package

6e95150

The package holds code shared between SNP-based attestation implementations on AWS and Azure .

cli: use new instance info struct in verify

91f7708

This ensure that issuer and verify (as consumer) use the same types for marshalling/unmarshalling.

attestation: use SNP-based attestation for AWS SNP

79b5cc3

verify: query vlek ASK from KDS if not set

8738bc5

The user can choose to supply an intermediate certificate through the config, like they can for the root key. If none is supplied, the KDS is queried for a valid ASK.

api: refactor attestationcfgapi cli

18eef9a

The cli now takes CSP and object kind as argument. Also made upload an explicit command and the report path/version an argument. Previously the report was a flag. The CSP was hardcoded. There was only one object kind (snp-report).

config: fetch latest AWS TCB values

242c8dd

api: add support to upload AWS TCB values

2d8c447

The attestationconfig api CLI now uploads SNP TCB versions for AWS.

config: only fetch TCB values from api if wanted

d7ab9bd

If no TCB value is set to `latest`, the fetcher is now no longer called.

ci: call TCB upload step for AWS

ece82f3

docs: explain config options for AWS SNP

1891fcf

derpsteb force-pushed the feat/attestation/aws-snp branch from d5dce7b to 1891fcf Compare November 24, 2023 13:51

derpsteb merged commit 2b199fd into main Nov 24, 2023
15 checks passed

derpsteb deleted the feat/attestation/aws-snp branch November 24, 2023 14:49

daniel-weisse mentioned this pull request Nov 28, 2023

ci: fix service accounts introduced by merge #2652

Merged

thomasten mentioned this pull request Nov 28, 2023

config: remove AWS SNP warning #2654

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aws: reintroduce SNP-based attestation #2601

aws: reintroduce SNP-based attestation #2601

derpsteb commented Nov 14, 2023 •

edited

Loading

netlify bot commented Nov 14, 2023 •

edited

Loading

daniel-weisse left a comment

elchead left a comment

derpsteb commented Nov 17, 2023

msanft left a comment

msanft Nov 16, 2023

derpsteb Nov 20, 2023

msanft Nov 17, 2023

derpsteb Nov 20, 2023

msanft Nov 20, 2023

elchead left a comment

derpsteb commented Nov 20, 2023 •

edited

Loading

derpsteb commented Nov 20, 2023

derpsteb commented Nov 21, 2023 •

edited

Loading

thomasten left a comment

derpsteb commented Nov 24, 2023

github-actions bot commented Nov 24, 2023

aws: reintroduce SNP-based attestation #2601

aws: reintroduce SNP-based attestation #2601

Conversation

derpsteb commented Nov 14, 2023 • edited Loading

Context

Proposed change(s)

Checklist

netlify bot commented Nov 14, 2023 • edited Loading

✅ Deploy Preview for constellation-docs ready!

daniel-weisse left a comment

Choose a reason for hiding this comment

elchead left a comment

Choose a reason for hiding this comment

derpsteb commented Nov 17, 2023

msanft left a comment

Choose a reason for hiding this comment

msanft Nov 16, 2023

Choose a reason for hiding this comment

derpsteb Nov 20, 2023

Choose a reason for hiding this comment

msanft Nov 17, 2023

Choose a reason for hiding this comment

derpsteb Nov 20, 2023

Choose a reason for hiding this comment

msanft Nov 20, 2023

Choose a reason for hiding this comment

elchead left a comment

Choose a reason for hiding this comment

derpsteb commented Nov 20, 2023 • edited Loading

derpsteb commented Nov 20, 2023

derpsteb commented Nov 21, 2023 • edited Loading

thomasten left a comment

Choose a reason for hiding this comment

derpsteb commented Nov 24, 2023

github-actions bot commented Nov 24, 2023

Coverage report

derpsteb commented Nov 14, 2023 •

edited

Loading

netlify bot commented Nov 14, 2023 •

edited

Loading

derpsteb commented Nov 20, 2023 •

edited

Loading

derpsteb commented Nov 21, 2023 •

edited

Loading