Add SEV-SNP policy for signed UEFI measurements #446
Conversation
deeglaze
force-pushed
the
gcetcb
branch
3 times, most recently
from
7029313
to
afa3be8
Compare
deeglaze
force-pushed
the
gcetcb
branch
2 times, most recently
from
6a6511b
to
799e2d8
Compare
|
/gcbrun |
|
@jkl73 not sure what to make of the CS presubmit failure. |
just ignore the error now.. I'm still trying to fix the the build |
|
/gcbrun |
proto/attest.proto
Outdated
| // digest, or otherwise presented as cached collateral with the attestation | ||
| // itself. The method of delivery is vendor-specific. | ||
| message RIMPolicy { | ||
| // If true, the signed measurement must be available and the target |
There was a problem hiding this comment.
available where? IIUC, this is the attestation certificate table, but we state it.
There was a problem hiding this comment.
It can be in the certificate table, a UEFI variable, a local file, downloadable given a URI, or any other means that could be specified by a vendor. I've updated the wording a little.
proto/attest.proto
Outdated
| // measurement must be among the listed signed measurements. | ||
| // If false, then only error if there is a problem verifying the signed | ||
| // measurements when they are available. | ||
| bool require = 1; |
There was a problem hiding this comment.
How about cert_entry_required`?
server/policy.go
Outdated
| Validate: gceverify.SNPValidateFunc(&gceverify.Options{ | ||
| RootsOfTrust: uefirot, | ||
| Now: time.Now(), | ||
| // TODO: No getter? Network fallback can be helpful. |
There was a problem hiding this comment.
What specifically do we need to do here? Is it to introduce a Getter/an option for a Getter?
There was a problem hiding this comment.
How do you feel about EvaluatePolicyOpt with a new *PolicyOptions argument? The EvaluatePolicy can use default options like Getter: trust.DefaultHTTPSGetter() and Now: time.Now() (Now moved to options to make evaluateSevSnpPolicy more deterministic and testable).
server/policy_test.go
Outdated
| dents2, err := os.ReadDir(outdir2) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
I might be missing something, but I don't see where we use outdir2
| @@ -18,10 +19,17 @@ require ( | |||
| github.com/google/go-configfs-tsm v0.2.2 // indirect | |||
| github.com/google/go-tspi v0.3.0 // indirect | |||
| github.com/google/uuid v1.6.0 // indirect | |||
| github.com/inconshreveable/mousetrap v1.1.0 // indirect | |||
There was a problem hiding this comment.
this is a really bizarre dependency to see
| @@ -13,7 +19,69 @@ import ( | |||
| // will describe in what way the state failed. See the Policy documentation for | |||
| // more information about the specifics of different policies. | |||
| func EvaluatePolicy(state *pb.MachineState, policy *pb.Policy) error { | |||
There was a problem hiding this comment.
Me want rego policy
Not related to your change, but would you be open to this being completely moved to a rego policy in the near future?
There was a problem hiding this comment.
I would certainly. The issue is primarily populating the Rego context to know that the signed values are even available and their verification status.
server/policy.go
Outdated
|
|
||
| // the SEV-SNP attestation signature is already verified by this point. | ||
| func evaluateSevSnpPolicy(state *spb.Attestation, policy *pb.SevSnpPolicy) error { | ||
| if policy == nil { |
There was a problem hiding this comment.
This feels like a scary way to represent any "everything" policy to me. I feel like this is really easily done accidentally.
I don't know enough about protocol buffers to have a suggestion here, so perhaps when we have a different sort of policy language this concern will no longer be relevant.
WDYT?
There was a problem hiding this comment.
policy == nil should be understood as &pb.SevSnpPolicy{}. I've removed this check. The real concern is the policy.GetUefi() == nil check, since if I treat nil as &policy.RIMPolicy{}, then the behavior is different.
I can change the behavior to be like the evaluatePlatformPolicy, where at least one root has to be present to care about any signers. No signers means any value is okay. That would make the nil check and empty message the same.
I wouldn't want to start failing all policies because there are no signed measurements deployed yet. Do you want to depend on signed reference values right now? I suppose you could but then have failing tests internally until the signed firmware package finally rolls out.
server/policy_test.go
Outdated
| outdir := t.TempDir() | ||
| outdir2 := t.TempDir() |
There was a problem hiding this comment.
nit: can you define these closer to where they are used?
server/policy_test.go
Outdated
| }, | ||
| }, | ||
| } | ||
| if err := EvaluatePolicy(ms, pol); err != nil { |
There was a problem hiding this comment.
Consider adding tests cases for nil policy + nil UEFI policy
There was a problem hiding this comment.
Thanks, found a bug.
| // add for policy reasons. | ||
| message SevSnpPolicy { | ||
| // The policy for checking the signed reference values for the UEFI at launch. | ||
| RIMPolicy uefi = 1; |
There was a problem hiding this comment.
Is it possible to make this policy not just specific to snp?
I wonder later if tdx can have a similar firmware policy check
There was a problem hiding this comment.
The technologies will have signatures deployed at different dates. We can have them separate for now and then deprecate for an overarching one, later?
deeglaze
force-pushed
the
gcetcb
branch
2 times, most recently
from
c60a680
to
c7614bc
Compare
|
/gcbrun |
|
/gcbrun |
Depends on PR#445
This adds an extra validation check beyond well-formedness that the verification step checks. If the reference values are available within the SEV-SNP attestation certificate chain, then verify the signature and check the report measurement against the golden values.