Skip to content
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

Add pk and kek to the SecureBootState proto message and populate them. #534

Merged
merged 7 commits into from
Feb 6, 2025

Conversation

eytankidron
Copy link
Contributor

No description provided.

@eytankidron
Copy link
Contributor Author

@alexmwu

Copy link
Contributor

@alexmwu alexmwu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add/update tests for the happy path at least.

@@ -149,6 +149,10 @@ message SecureBootState {
// Authority events post-separator. Pre-separator authorities
// are currently not supported.
Database authority = 4;
// Platform Keys
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please keep comments consistent:
"The Secure Boot Platform key, used to sign key exchange keys."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// Platform Keys
Database pk = 5;
// Exchange Keys
Database kek = 6;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The Secure Boot Key Exchange Keys, used to sign db and dbx updates."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@alexmwu alexmwu requested a review from jkl73 February 3, 2025 16:18
@eytankidron
Copy link
Contributor Author

Please add/update tests for the happy path at least.

Done

Copy link
Contributor

@alexmwu alexmwu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are failing (https://github.com/google/go-tpm-tools/actions/runs/13159943001/job/36742118354?pr=534). You need to regenerate the pb files and check the changes in using go generate ./...

for _, cert := range msState.GetSecureBoot().GetKek().GetCerts() {
switch c := cert.GetRepresentation().(type) {
case *attestpb.Certificate_Der:
if !bytes.Equal(c.Der, MicrosoftKEKCA2011Cert) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it would be nice to update

func matchWellKnown(cert x509.Certificate) (pb.WellKnownCertificate, error) {
with the MS KEK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
And as discussed offline, I also added GceDefaultPKCert to this list of well known certificates.

@eytankidron
Copy link
Contributor Author

Tests are failing (https://github.com/google/go-tpm-tools/actions/runs/13159943001/job/36742118354?pr=534). You need to regenerate the pb files and check the changes in using go generate ./...

Done. Regenerated attest.pb.go and tpm.pb.go.

Comment on lines 700 to 707
case *attestpb.Certificate_WellKnown:
if c.WellKnown == attestpb.WellKnownCertificate_UNKNOWN {
t.Error(("found WellKnownCertificate_UNKNOWN in pk"))
}
if c.WellKnown == attestpb.WellKnownCertificate_GCE_DEFAULT_PK {
containsGCEDefaultPK = true
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be simplified:

certs := msState.GetSecureBoot().GetPk().GetCerts()
if len(certs) != 1 {
  t.Error()
}
wkRep, ok := certs[0].(*attestpb.Certificate_WellKnown)
if !ok {
  t.Error()
}
if wkRep.WellKnown != WellKnownCertificate_GCE_DEFAULT_PK{
  t.Error()
}

Same with the KEK check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be simplified:

certs := msState.GetSecureBoot().GetPk().GetCerts()
if len(certs) != 1 {
  t.Error()
}
wkRep, ok := certs[0].(*attestpb.Certificate_WellKnown)
if !ok {
  t.Error()
}
if wkRep.WellKnown != WellKnownCertificate_GCE_DEFAULT_PK{
  t.Error()
}

Same with the KEK check.

Done

@alexmwu alexmwu merged commit d17bba4 into google:main Feb 6, 2025
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants