-
Notifications
You must be signed in to change notification settings - Fork 74
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
Export function to extract and validate AK from server #492
Conversation
caf16c7
to
3d3ca12
Compare
server/verify.go
Outdated
@@ -183,6 +152,45 @@ func VerifyAttestation(attestation *pb.Attestation, opts VerifyOpts) (*pb.Machin | |||
return nil, fmt.Errorf("attestation does not contain a supported quote") | |||
} | |||
|
|||
// ParseAndValidateAK parses and validate AK cert in the attestation, and populate GCE instance info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we have composable funcs where we can verify the attestation key against the Attestation proto and VerifyOpts and then extract info from it. So I suggest a bit more refactoring:
- call this
ValidateAK
- Have
ValidateAK
just callvalidateAKPub
andvalidateAKCert
- refactor
validateAKPub
andvalidateAKCert
to only return error - refactor
validateAKCert
to not callgetInstanceInfoFromExtensions
- in
VerifyAttestation
callValidateAK
and thengetInstanceInfoFromExtensions
(orGetGCEInstanceInfo
if cert != nil)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
{ | ||
name: "failed with missing intermediates", | ||
opts: VerifyOpts{TrustedRootCerts: GceEKRoots}, | ||
wantPass: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test case setting TrustedAKs in VerifyOpts but not Trusted/Intermediate certs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
3d3ca12
to
5eb876b
Compare
02dce70
to
65204d1
Compare
65204d1
to
e2f86ab
Compare
/gcbrun |
Add CAS-based EK/AK root CAs. Add warning about using GetGCEInstanceInfo.
/gcbrun |
.github/workflows/ci.yml
Outdated
@@ -77,11 +77,12 @@ jobs: | |||
run: | | |||
GO_EXECUTABLE_PATH=$(which go) | |||
sudo $GO_EXECUTABLE_PATH test -v -run "TestFetchImageSignaturesDockerPublic" ./launcher | |||
sudo $GO_EXECUTABLE_PATH test -v -run "TestHwAttestationPass" ./cmd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this failing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See b/368161073, it returns couldn't create report entry in configfs, no such device
error
server/verify.go
Outdated
if akCert != nil { | ||
instanceInfo, err := GetGCEInstanceInfo(akCert) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to validate AK certificate: %w", err) | ||
return nil, fmt.Errorf("failed to extract GCE instance info from AK cert: %w", err) | ||
} | ||
akPubKey = akCert.PublicKey.(crypto.PublicKey) | ||
// Populate GCE instance info. | ||
machineState.Platform = &pb.PlatformState{InstanceInfo: instanceInfo} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops I realize a security issue: we can't blindly just parse GCE instance info, since we might have validated with the TrustedAKs instead
Which is why we had the extract in validateAK in the first place :).
I'll add something to your PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please give my changes a look @yawangwang and merge it in if you feel it looks okay.
LGTM, now merging this PR |
* Export ValidateAKCert and add CAS EK root CA + skip TestHwAttestationPass --------- Co-authored-by: Alex Wu <wuale@google.com>
We need to export a function from server to extract/validate AK, and populate GCE instance info into machine state so that GAV can consume.