From 4a4701c32942e1e9cc1fd82667dcd492749c75a1 Mon Sep 17 00:00:00 2001 From: Dionna Glaze Date: Fri, 16 Jun 2023 16:45:58 +0000 Subject: [PATCH] Remove KDS clock skew workaround AMD has changed KDS to backdate their NotBefore timestamp to 24 hours prior to the request, as a new test confirms. Closes Issue#45. Signed-off-by: Dionna Glaze --- verify/verify.go | 48 +----------------- verify/verify_test.go | 114 ++++++++---------------------------------- 2 files changed, 24 insertions(+), 138 deletions(-) diff --git a/verify/verify.go b/verify/verify.go index e5caaf1..3f9cf3b 100644 --- a/verify/verify.go +++ b/verify/verify.go @@ -473,10 +473,6 @@ type Options struct { // Getter takes a URL and returns the body of its contents. By default uses http.Get and returns // the body. Getter trust.HTTPSGetter - // KDSClockSkewThreshold is the length of time permitted to wait for a certificate from KDS to - // become valid. The host and KDS servers' clocks may be skewed such that a VCEK certificate - // may have been "certified in the future". - KDSClockSkewThreshold time.Duration // Now is the time at which to verify the validity of certificates. If unset, uses time.Now(). Now time.Time // TrustedRoots specifies the ARK and ASK certificates to trust when checking the VCEK. If nil, @@ -488,9 +484,8 @@ type Options struct { // DefaultOptions returns a useful default verification option setting func DefaultOptions() *Options { return &Options{ - Getter: trust.DefaultHTTPSGetter(), - KDSClockSkewThreshold: 5 * time.Minute, - Now: time.Now(), + Getter: trust.DefaultHTTPSGetter(), + Now: time.Now(), } } @@ -552,44 +547,6 @@ func SnpAttestation(attestation *spb.Attestation, options *Options) error { return SnpProtoReportSignature(attestation.GetReport(), vcek) } -// waitForClockSkew allows a fresh certificate to be NotBefore a future time if that time is within -// a threshold of acceptable clock skew between the host and KDS. -func waitForClockSkew(certRaw []byte, opts *Options) error { - cert, err := x509.ParseCertificate(certRaw) - if err != nil { - return err - } - now := opts.Now - // KDS hasn't returned a certificate from the future. No wait needed. - if now.After(cert.NotBefore) { - return nil - } - // Follow the x509 convention that zero means to use system time. - realNow := now - if realNow.IsZero() { - realNow = time.Now() - } - - // The certificate is from the future. If within the accepted threshold, then - // either wait for the system clock to catch up or flub the Now value, depending - // on what the user specified for Now. - skew := cert.NotBefore.Sub(realNow) - // Wait for the host to catch up if within the threshold, otherwise verification - // will fail when comparing time.Now() against cert.NotBefore. - if skew <= opts.KDSClockSkewThreshold { - if now.IsZero() { - // The system time is used for verification, so wait until the future - // time of NotBefore before continuing. - time.Sleep(skew) - } else { - // The Now value won't be interpreted as time.Now() since it's not zero, but - // the threshold is acceptable to bump up the Now option for verification. - opts.Now = cert.NotBefore - } - } - return nil -} - // fillInAttestation uses AMD's KDS to populate any empty certificate field in the attestation's // certificate chain. func fillInAttestation(attestation *spb.Attestation, options *Options) error { @@ -627,7 +584,6 @@ func fillInAttestation(attestation *spb.Attestation, options *Options) error { } } chain.VcekCert = vcek - return waitForClockSkew(vcek, options) } return nil } diff --git a/verify/verify_test.go b/verify/verify_test.go index 2a3f73f..b8028ef 100644 --- a/verify/verify_test.go +++ b/verify/verify_test.go @@ -20,7 +20,6 @@ import ( "crypto/x509/pkix" _ "embed" "encoding/asn1" - "fmt" "math/big" "math/rand" "os" @@ -395,97 +394,6 @@ func TestCRLRootValidity(t *testing.T) { } } -func TestClockSkew(t *testing.T) { - if !sg.UseDefaultSevGuest() { - t.Skip("Skipping certificate skew test for hardware device testing") - } - // Tests that the CRL is signed by the ARK. - trust.ClearProductCertCache() - staticNow := time.Date(2022, time.June, 14, 12, 0, 0, 0, time.UTC) - skewSigner := func(t *testing.T, now time.Time, skew time.Duration) *test.AmdSigner { - future := now.Add(skew) - sb := &test.AmdSignerBuilder{ - Product: "Milan", - ArkCreationTime: future, - AskCreationTime: future, - VcekCreationTime: future, - } - signer2, err := sb.CertChain() - if err != nil { - t.Fatal(err) - } - return signer2 - } - var nonce [64]byte - - tcs := []struct { - name string - now time.Time - skew time.Duration - threshold time.Duration - wantErr string - }{ - { - name: "happy path", - now: staticNow, - skew: 50 * time.Millisecond, - threshold: time.Second, - }, - - { - name: "Too new", - skew: 5 * time.Second, - now: staticNow, - threshold: time.Second, - wantErr: fmt.Sprintf("%s Last error: %s: current time %v is before %v", - "VCEK could not be verified by any trusted roots.", - "error verifying VCEK certificate: x509: certificate has expired or is not yet valid", - staticNow.Format(time.RFC3339), staticNow.Add(5*time.Second).Format(time.RFC3339)), - }, - { - name: "happy path (system)", - // At the cost of a longer test, allow for computation to take much longer - // than one should expect it to take to avoid test flakes. - skew: 10 * time.Second, - threshold: 15 * time.Second, - }, - { - name: "Too new (system)", - skew: 10 * time.Second, - threshold: time.Second, - wantErr: "x509: certificate has expired or is not yet valid", - }, - } - testData := test.TestCases() - for _, tc := range tcs { - t.Run(tc.name, func(t *testing.T) { - realNow := tc.now - if realNow.IsZero() { - realNow = time.Now() - } - signer2 := skewSigner(t, realNow, tc.skew) - dopts := &test.DeviceOptions{Now: realNow, Signer: signer2} - d, goodRoots, _, getter := testclient.GetSevGuest(testData, dopts, t) - // Sign the zero report for verification. - report, err := sg.GetReport(d, nonce) - if err != nil { - t.Fatal(err) - } - - // Verify the report with a skewed certificate. - err = SnpReport(report, &Options{ - Getter: getter, - KDSClockSkewThreshold: tc.threshold, - TrustedRoots: goodRoots, - Now: tc.now, - }) - if !test.Match(err, tc.wantErr) { - t.Fatalf("SnpReport(report, {KDSClockSkewThreshold: %v}) = %v. Want err: %v", tc.threshold, err, tc.wantErr) - } - }) - } -} - func TestOpenGetExtendedReportVerifyClose(t *testing.T) { trust.ClearProductCertCache() tests := test.TestCases() @@ -551,3 +459,25 @@ func TestRealAttestationVerification(t *testing.T) { t.Error(err) } } + +func TestKDSCertBackdated(t *testing.T) { + if !test.TestUseKDS() { + t.Skip() + } + getter := test.GetKDS(t) + // Throttle requests to KDS. + time.Sleep(10 * time.Second) + bytes, err := getter.Get("https://kdsintf.amd.com/vcek/v1/Milan/3ac3fe21e13fb0990eb28a802e3fb6a29483a6b0753590c951bdd3b8e53786184ca39e359669a2b76a1936776b564ea464cdce40c05f63c9b610c5068b006b5d?blSPL=2&teeSPL=0&snpSPL=5&ucodeSPL=68") + if err != nil { + t.Skipf("Live KDS query failed: %v", err) + } + cert, err := x509.ParseCertificate(bytes) + if err != nil { + t.Fatalf("Could not parse live VCEK certificate: %v", err) + } + now := time.Now() + if !cert.NotBefore.Before(now.Add(-23 * time.Hour)) { + t.Fatalf("KDS has not backdated its certificates. NotBefore: %s, now: %s", + cert.NotBefore.Format(time.RFC3339), now.Format(time.RFC3339)) + } +}