Populate the SNP/TDX Machine State field with the verified SNP/TDX attestation data + use a stable COS image version #463
Conversation
yawangwang
force-pushed
the
populate-ms-with-snp
branch
3 times, most recently
from
71bfc80 to
482a46c
Compare
yawangwang
changed the title
yawangwang
force-pushed
the
populate-ms-with-snp
branch
from
482a46c to
1b95570
Compare
server/verify.go
Outdated
| // parseTCGEventLog is a wrapper function around `parsePCClientEventLog` method to: | ||
| // 1. parse partial machine state from TPM TCG event logs. | ||
| // 2. verify GceTechnology since the GCE Technology event is directly related to the TPM. | ||
| func parseTCGEventLog(attestation *pb.Attestation, pcrs *tpmpb.PCRs, opts VerifyOpts) (*pb.MachineState, error) { |
There was a problem hiding this comment.
I liked your suggestion of parseMachineStateFromTPM better than parseTCGEventLog. Since it is also doing event log replay and event parsing. The term "parse" is a bit overloaded unfortunately.
server/verify.go
Outdated
| @@ -176,6 +179,7 @@ func VerifyAttestation(attestation *pb.Attestation, opts VerifyOpts) (*pb.Machin | |||
|
|
|||
| proto.Merge(machineState, celState) | |||
| proto.Merge(machineState, state) | |||
| proto.Merge(machineState, teeState) | |||
There was a problem hiding this comment.
We should avoid proto.Merge where possible as it can override existing values.
| return ms, nil | ||
| } | ||
|
|
||
| // parseTEEAttestation parses a machineState from TeeAttestation. |
There was a problem hiding this comment.
The point of the wrapper func for parsePCClientEventlog is to avoid proto.Merge. proto.Merge is less readable since it implicitly copies every field from src to dst. This requires our implementation to be mutually exclusive, or we risk overwriting. We originally used proto.Merge to merge the state from two separate event logs, but we could just populate the TeeAttestation field directly in the wrapper.
There was a problem hiding this comment.
IOW, this can be inline in parseTCGEventLog.
There was a problem hiding this comment.
My take from "inline" is call parseTEEAttestation from parseMachineStateFromTPM after verifyGceTechnolgy is done. LMK if this is not what you want.
| @@ -318,10 +322,10 @@ func makePool(certs []*x509.Certificate) *x509.CertPool { | |||
| return pool | |||
| } | |||
|
|
|||
| // VerifyGceTechnology checks the GCE-specific GceNonHost event's Trusted Execution Technology (TEE) | |||
There was a problem hiding this comment.
If we unexport this, it's a backwards incompatible change. Please add a breaking comment to the PR description.
server/verify_test.go
Outdated
| @@ -1068,10 +1068,14 @@ func TestVerifyAttestationWithSevSnp(t *testing.T) { | |||
| } | |||
There was a problem hiding this comment.
Please add tests for the failure cases
There was a problem hiding this comment.
Added a new test TestParseTEEAttestation to cover both successful and failure cases.
yawangwang
force-pushed
the
populate-ms-with-snp
branch
from
1b95570 to
5e327b4
Compare
|
/gcbrun |
yawangwang
force-pushed
the
populate-ms-with-snp
branch
from
5e327b4 to
afce07b
Compare
yawangwang
changed the title
|
/gcbrun |
| @@ -1,7 +1,7 @@ | |||
| substitutions: | |||
| # using this base image for now, because there is an issue causing the newest COS dev | |||
| # image not booting with cs. | |||
| '_BASE_IMAGE': '' # left empty means using the latest image in the family | |||
| '_BASE_IMAGE': 'cos-dev-117-18374-0-0' # Use a stable COS version to avoid CS image build issues | |||
There was a problem hiding this comment.
Please do not submit until you get approval from @jkl73 for this change.
There was a problem hiding this comment.
lgtm, this is the same base image on the last dev build.
| // In long term, it should parse a full machineState from TeeAttestation. | ||
| func parseTEEAttestation(attestation *pb.Attestation, tech pb.GCEConfidentialTechnology) (*pb.MachineState, error) { | ||
| switch tech { | ||
| case pb.GCEConfidentialTechnology_AMD_SEV_SNP: |
There was a problem hiding this comment.
I may be missing something: don't we already do these checks in verifyGceTechnology?? Can we have that return pb.MachineState or just remove the call to parseTEEAttestation and do
ms.TeeAttestation = attestation.TeeAttestation?
There was a problem hiding this comment.
ms.TeeAttestation = attestation.TeeAttestation will not work because they're two different proto types.
parseTEEAttestation doesn't do any checks on attestation. This function, in short term, just populates the TeeAttestation field using a deep copy of the verified attestation data, but in long run, we'll want to get a full machineState from TDX/SNP attestation. The intent of having this function is for future-proof purpose.
This PR includes changes to populate the machinState proto with the verified SNP/TDX attestation data. The attestation verifier service will leverage the verified machineState to make claims around.
Breaking changes:
verifyGceTechnologybecause GCETechnology event log is closely related to TPM, this function is directly called after the event log is parsed.cos-dev-117-18374-0-0rather than the latest COS image version to avoid CS image build issues.