attestation: use go-sev-guest library

[msanft]

Context

We want to use the go-sev-guest library to have an easily maintainable interface to the verification / validation of SEV-SNP attestation reports. (For now on Azure only due to deviations in the attestation processes of the CSPs)

Proposed change(s)

• Provide a 1:1-translation of our current custom attestation strategy to a go-sev-guest policy. However, there are checks which a) we currently perform but do not seem to be expressable in a go-sev-guest policy or b) are automatically performed by go-sev-guest but not currently done by us. Below lists all occurances of those cases. -> See the sections below for how our previous checks are covered by which checks from go-sev-guest
• @reviewers: In addition to carefully reviewing the code, please pay special attention to the checks listed below and if they make sense and are restrictive enough.

Checks we currently do but can't be expressed in go-sev-guest

• CommittedTCB >= TCB specified in Config:

  constellation/internal/attestation/azure/snp/validator.go

  Lines 151 to 153 in 590931f

  	if !report.CommittedTCB.isVersion(v.config.BootloaderVersion.Value, v.config.TEEVersion.Value, v.config.SNPVersion.Value, v.config.MicrocodeVersion.Value) {
  	return &versionError{"COMMITTED_TCB", report.CommittedTCB}
  	}

covered by "LaunchTCB >= Minimum LaunchTCB". This implies that the config now defines the minimum launchTCB. This probably makes more sense. Since this was never documented I think we can silently change this. The new verification is more permissive with SVNs that are newer the the configured, which is good (more robust). ~ @derpsteb

• LaunchTCB != CommittedTCB:

  constellation/internal/attestation/azure/snp/validator.go

  Lines 154 to 156 in 590931f

  	if report.LaunchTCB != report.CommittedTCB {
  	return &versionError{"LAUNCH_TCB", report.LaunchTCB}
  	}

fine to remove. This is unnecessarily restrictive. Can't remember why it's there. ~ @derpsteb

• CurrentTCB >= CommittedTCB:

  constellation/internal/attestation/azure/snp/validator.go

  Lines 157 to 159 in 590931f

  	if !report.CommittedTCB.supersededBy(report.CurrentTCB) {
  	return &versionError{"CURRENT_TCB", report.CurrentTCB}
  	}

we should set PermitProvisionalFirmware to true so this check is kept. I currently don't see a reason why we would want to prevent this. It should improve robustness during firmware upgrades imo. ~ @derpsteb

• MAA checks depending on EnforcementPolicy set in the Constellation config:
  

  constellation/internal/attestation/azure/snp/validator.go

  Lines 190 to 216 in 590931f

  	hasExpectedIDKeyDigest := false
  	for _, digest := range v.config.FirmwareSignerConfig.AcceptedKeyDigests {
  	if bytes.Equal(digest, report.IDKeyDigest[:]) {
  	hasExpectedIDKeyDigest = true
  	break
  	}
  	}
  	if !hasExpectedIDKeyDigest {
  	switch v.config.FirmwareSignerConfig.EnforcementPolicy {
  	case idkeydigest.MAAFallback:
  	v.log.Infof(
  	"configured idkeydigests %x don't contain reported idkeydigest %x, falling back to MAA validation",
  	v.config.FirmwareSignerConfig.AcceptedKeyDigests,
  	report.IDKeyDigest[:],
  	)
  	return v.maa.validateToken(ctx, v.config.FirmwareSignerConfig.MAAURL, maaToken, extraData)
  	case idkeydigest.WarnOnly:
  	v.log.Warnf(
  	"configured idkeydigests %x don't contain reported idkeydigest %x",
  	v.config.FirmwareSignerConfig.AcceptedKeyDigests,
  	report.IDKeyDigest[:],
  	)
  	default:
  	return &idKeyError{report.IDKeyDigest[:], v.config.FirmwareSignerConfig.AcceptedKeyDigests}
  	}
  	}

  
  Might be achievable by ignoring the go-sev-guest IdKey hash check if EnforcementPolicy is MaaFallback or WarnOnly and performing a custom check after. (See current idea of an implementation in the PR)

we need to keep this. ~ @derpsteb
-> @msanft : implement custom check after go-sev-guest validation

• TCB in VCEK == CommittedTCB
  

  constellation/internal/attestation/azure/snp/validator.go

  Lines 221 to 222 in 5272e7c

  	// validateVCEKExtensions checks that the certificate extension values in cert match the values described in report.
  	func validateVCEKExtensions(cert *x509.Certificate, report snpAttestationReport) error {

  
  This might be replaced by the CurrentTCB >= TCB specified in VCEK check mentioned below.

Re 5.: I agree that this should be replaced with "CurrentTCB >= TCB specified in VCEK". The current check is overly restrictive. ~ @derpsteb

Checks we didn't do before but are automatically done by go-sev-guest

• Depending on an allow flag, either CurrentTCB >= CommittedTCB OR CurrentTCB != CommittedTCB: https://github.com/google/go-sev-guest/blob/d8660ce1f6ee9699f62ff463e6787e32c318ae1c/validate/validate.go#L394-L399
• LaunchTCB >= Minimum LaunchTCB (provided by us): https://github.com/google/go-sev-guest/blob/d8660ce1f6ee9699f62ff463e6787e32c318ae1c/validate/validate.go#L401
  (May be easy for us by just giving in the Minimum TCB specified in our Config)
• ReportedTCB != TCB specified in VCEK: https://github.com/google/go-sev-guest/blob/d8660ce1f6ee9699f62ff463e6787e32c318ae1c/validate/validate.go#L408
• CurrentTCB >= TCB specified in VCEK: https://github.com/google/go-sev-guest/blob/d8660ce1f6ee9699f62ff463e6787e32c318ae1c/validate/validate.go#L409
• ReportedTCB >= Minimum TCB (provided by us): https://github.com/google/go-sev-guest/blob/d8660ce1f6ee9699f62ff463e6787e32c318ae1c/validate/validate.go#L410

Checklist

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

[netlify]

✅ Deploy Preview for constellation-docs canceled.

Name	Link
🔨 Latest commit	ed7e78d
🔍 Latest deploy log	https://app.netlify.com/sites/constellation-docs/deploys/6501d85e6954e10008896758

[thomasten]

Ideally, we can drop all of our own SNP report validation (including VCEK verification) except for MAA and maybe IDKey.

Regarding TCB version checks, @derpsteb and @katexochen should know best if the TCB version checks of the library are good to replace ours.

Regarding IDKey, the implementation may be simpler if we never use the library for it and just keep our current check. What do you think @msanft ?

[msanft]

Ideally, we can drop all of our own SNP report validation (including VCEK verification) except for MAA and maybe IDKey.

Yes, I think this is the main point of adopting the library now.

Regarding IDKey, the implementation may be simpler if we never use the library for it and just keep our current check.

I think so too. Double-checking with the library has no obvious advantages to me while making the code harder to read.

[derpsteb]

I didn't check the code yet as the pipeline is still failing and this is still a draft.

Regarding the TCB checks.
From "Checks we currently do but can't be expressed in go-sev-guest":
re 1.: fine to remove. covered by "LaunchTCB >= Minimum LaunchTCB". This implies that the config now defines the minimum launchTCB. This probably makes more sense. Since this was never documented I think we can silently change this. The new verification is more permissive with SVNs that are newer the the configured, which is good (more robust).

re 2.: fine to remove. This is unnecessarily restrictive. Can't remember why it's there.

re 3.: we should set PermitProvisionalFirmware to true so this check is kept. I currently don't see a reason why we would want to prevent this. It should improve robustness during firmware upgrades imo.

re 4.: we need to keep this.

Regarding the checks that we gain: seems like a good addition to me. I always read the AMD docs in a way that suggests that the PSP firmware enforces these conditions and that a attestation verifier wouldn't have to check them. But this isn't stated anywhere. Better to have them. 👍

[derpsteb]

Re 5.: I agree that this should be replaced with "CurrentTCB >= TCB specified in VCEK". The current check is overly restrictive.

[msanft]

latest update: Both unit test TestTrustedKeyFromSNP and manual test with an actual cluster report the following error:

verifying SNP attestation: report signature verification error: x509: ECDSA verification failure

of which I don't know why they report it yet. If anyone has ideas to debug this, please let me know.
To reproduce it, just run the corresponding unit test (TestTrustedKeyFromSNP)

This passage in the library produces the error:
https://github.com/google/go-sev-guest/blob/14ac50e9ffcc05cd1d12247b710c65093beedb58/verify/verify.go#L447-L449

[thomasten]

To me it seems that the report and the vcek just don't match. Please try using the vcek and chain that the issuer is getting from Azure THIM instead of downloading it from AMD.
If this turns out to work, let's discuss pros and cons of both ways.

[msanft]

needs google/go-sev-guest#73

[msanft]

One other bug I need to track down:

A test with an actual cluster returns:

Error: init call: rpc error: code = Unavailable desc = connection error: desc = "transport: authentication handshake failed: validating attestation public key: verifying SNP attestation: report signature verification error: x509: ECDSA verification failure"

If I print the report that is being verified in the above trace it works when checking it "manually" in my test environment with the following snippet:

report, err := abi.ReportToProto(testdata.AttestationReport)
if err != nil {
	panic(err)
}

fmt.Println("Report HWID:", hex.EncodeToString(report.GetChipId()))
fmt.Println("TCB level:", kds.DecomposeTCBVersion(kds.TCBVersion(report.GetCurrentTcb())))

attestation, err := verify.GetAttestationFromReport(report, &verify.Options{})
if err != nil {
	panic(err)
}

err = verify.SnpAttestation(attestation, &verify.Options{})
if err != nil {
	panic(err)
}

I assume that either somewhere in the build process, the pseudo-version of go-sev-guest (including our patch) isn't being respected, or I make some silly mistake at another point

[msanft]

^ Possibly caused by an indirect dependency on v0.6.1 of go-sev-guest in our hack folder

[msanft]

^ Possibly caused by an indirect dependency on v0.6.1 of go-sev-guest in our hack folder

Correcting this fixed it.

[github-actions]

Coverage report

Package	Old	New	Trend
cli/internal/cmd	55.10%	55.10%	↔️
internal/attestation/azure/snp	10.60%	46.90%	↗️

[msanft]

We also probably want to use the AMD root certificate from the config instead of the one embedded in the go-sev-guest library.

[thomasten]

Thanks!

[derpsteb]

LGTM. I didn't do another deep dive as I am unde the impression Thomas and Paul already did that. I think it's great that you extended the unittests.

[3u13r]

LGTM

[daniel-weisse]

Suggested change

	github.com/google/go-sev-guest v0.8.0
	github.com/google/go-sev-guest v0.9.0

v0.9.0 was released 2 days ago

[msanft]

One last E2E run, merging afterwards if successful