Add support for validating VLEK certificates #67
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
deeglaze
force-pushed
the
vlek
branch
2 times, most recently
from
656bbe4
to
454b644
Compare
|
Hey @deeglaze , In any case, this is what I found:
Not sure how these discrepancies should be handled. I guess we need to change go-sev-guest's assumptions? Best, |
|
Thanks Otto. This helps, since I don't have a real example of a VLEK to work from. Do you have a binary that you can share that I could use for testing purposes? |
deeglaze
force-pushed
the
vlek
branch
4 times, most recently
from
3745e45
to
f2ed12c
Compare
|
I've added the VLEK bundle from the KDS and fixed up the issues you mentioned. Please give the updated patch another try. |
derpsteb
reviewed
Aug 7, 2023
verify/trust/trust.go
Outdated
Comment on lines
242
to
256
| switch key { | ||
| case abi.VcekReportSigner: | ||
| intermediates.AddCert(r.Ask) | ||
| case abi.VlekReportSigner: | ||
| intermediates.AddCert(r.Asvk) | ||
| } |
There was a problem hiding this comment.
Move the r.Ask == nil case from the if in L236 to the switch.
Suggested change
| switch key { | |
| case abi.VcekReportSigner: | |
| intermediates.AddCert(r.Ask) | |
| case abi.VlekReportSigner: | |
| intermediates.AddCert(r.Asvk) | |
| } | |
| switch key { | |
| case abi.VcekReportSigner: | |
| if r.Ask == nil { | |
| return nil | |
| } | |
| intermediates.AddCert(r.Ask) | |
| case abi.VlekReportSigner: | |
| if r.Asvk == nil { | |
| return nil | |
| } | |
| intermediates.AddCert(r.Asvk) | |
| } |
That was the only problem left during my testing 🎉
There was a problem hiding this comment.
Nice, thanks so much for your help testing. It's tough without a spec :P
derpsteb
reviewed
Aug 7, 2023
verify/verify.go
Outdated
| if ica == nil { | ||
| return fmt.Errorf("root of trust missing intermediate certificate authority certificate for key %v", key) | ||
| } | ||
| if _, err := cert.Verify(*r.X509Options(opts.Now, key)); err != nil { |
There was a problem hiding this comment.
Suggested change
| if _, err := cert.Verify(*r.X509Options(opts.Now, key)); err != nil { | |
| tmp := r.X509Options(opts.Now, key) | |
| if tmp == nil { | |
| return fmt.Errorf("internal error: could not get X509 options for %v", key) | |
| } | |
| if _, err := cert.Verify(*tmp); err != nil { |
Not sure how to call that variable ^^
There was a problem hiding this comment.
Ah yes, uninitialized intermediate certs will lead to a nil dereference. Good catch.
|
Nicee. Works now :P. LGTM, once the two changes are included 👍 That's the raw cert table from the ExtendedReport I used for testing. Hope that is what you were looking for. |
kongoshuu
reviewed
Sep 8, 2023
| @@ -293,6 +295,13 @@ func mbz(data []uint8, lo, hi int) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func mbz64(data uint64, base string, hi, lo int) error { | |||
| if (data>>lo)&((1<<(hi-lo+1))-1) != 0 { | |||
There was a problem hiding this comment.
Can you add some comments here? It's not quite clear what being checked here.
testing/fake_certs.go
Outdated
| cert.NotAfter = b.VcekCreationTime.Add(vcekExpirationYears * 365 * 24 * time.Hour) | ||
| var hwid [64]byte | ||
| cert.ExtraExtensions = CustomVcekExtensions(kds.TCBParts{}, hwid) | ||
| func (b *AmdSignerBuilder) vekPrecert(creationTime time.Time, hwid []byte, serialNumber *big.Int, key abi.ReportSigner) *x509.Certificate { |
There was a problem hiding this comment.
"vek" stands for "vcek/vlek"? This feels a bit confusing. Can we just call it AmdKeyPrecert or something like that?
There was a problem hiding this comment.
vek is versioned endorsement key. I just expanded to endorsementKey and dropped the versioned.
testing/fake_certs.go
Outdated
| b.Vlek = signed | ||
| return err | ||
| } | ||
|
|
||
| // CertChain creates a test-only certificate chain from the keys and configurables in b. | ||
| func (b *AmdSignerBuilder) CertChain() (*AmdSigner, error) { |
There was a problem hiding this comment.
Can we call this TestOnlyCertChain() or so to make it clear this is test-only, since it's not in a test file?
testing/fakekds.go
Outdated
| RootBundles: map[string]string{"Milan": string(milanCerts)}, | ||
| Certs: &kpb.Certificates{}, | ||
| RootBundles: map[string]RootBundle{"Milan": { | ||
| VcekBundle: string(testdata.MilanBytes), |
There was a problem hiding this comment.
Can we change this also to "MilanVcekBytes"?
verify/testdata/testdata.go
Outdated
| @@ -25,11 +25,16 @@ import _ "embed" | |||
| //go:embed vcek.testcer | |||
| var VcekBytes []byte | |||
|
|
|||
| // MilanBytes is the Milan product cert_chain as issued by the AMD KDS. | |||
| // MilanBytes is the Milan product vcek cert_chain as issued by the AMD KDS. | |||
| // | |||
| //go:embed milan.testcer | |||
| var MilanBytes []byte | |||
| @@ -165,7 +166,11 @@ func TestSnpReportSignature(t *testing.T) { | |||
| if !bytes.Equal(got, want) { | |||
| t.Errorf("%s: GetRawReport(%v) = %v, want %v", tc.Name, tc.Input, got, want) | |||
| } | |||
| if err := SnpReportSignature(raw, d.Signer.Vcek); err != nil { | |||
| key := d.Signer.Vcek | |||
There was a problem hiding this comment.
Can we add an enum or so here for vcek/vlek instead of a bool tc.Vlek? That may be more readable and in case more type of certs are added in the future.
This changes the AuthorKeyEn report field to a better-named SignerInfo. The interpretation of this field selects which key the fake signer will use when signing reports. The VLEK certificate extensions are now checked against AMD's VLEK specification. The difference is VLEK certs are like VCEK certs, except the HWID extension is swapped with a CSP_ID extension. Tests have been updated to reflect these changes, including new VLEK-specific test cases. Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
kongoshuu
approved these changes
Sep 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes the AuthorKeyEn report field to a better-named SignerInfo. The interpretation of this field selects which key the fake signer will use when signing reports. The VLEK certificate extensions are now checked against AMD's VLEK specification. The difference is VLEK certs are like VCEK certs, except the HWID extension is swapped with a CSP_ID extension.
Tests have been updated to reflect these changes, including new VLEK-specific test cases.