Skip to content

Commit

Permalink
Merge pull request #57 from deeglaze/skewbgone
Browse files Browse the repository at this point in the history 
Remove KDS clock skew workaround
  • Loading branch information
@deeglaze
deeglaze authored Jun 20, 2023
2 parents a3e7158 + 4a4701c commit 5b44841
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 138 deletions.
48 changes: 2 additions & 46 deletions verify/verify.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(),
}
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -627,7 +584,6 @@ func fillInAttestation(attestation *spb.Attestation, options *Options) error {
}
}
chain.VcekCert = vcek
return waitForClockSkew(vcek, options)
}
return nil
}
Expand Down
114 changes: 22 additions & 92 deletions verify/verify_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"crypto/x509/pkix"
_ "embed"
"encoding/asn1"
"fmt"
"math/big"
"math/rand"
"os"
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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))
}
}

0 comments on commit 5b44841

Please sign in to comment.