From d3df5af563d5f3146d4d7b66bc34adae7b867ad9 Mon Sep 17 00:00:00 2001 From: Andrew Gabbitas Date: Sat, 1 Apr 2023 15:40:38 -0600 Subject: [PATCH 1/3] Use 90 day test certs in temporal shard window Change the behavior of the test certificate generation to always create a 90 day certificate, and make sure it falls in the temporal shard window if one is defined. If issuing a certificate for a current temporal shard (where the system time + 90d `NotAfter` lands in the temporal window), then generate a certifcate based on the current time. If generating a certificate for a past or future temporal shard, then generate a 90d certificate that falls in that window. Fixes #127 --- pki/certs.go | 57 ++++++++++++---------- pki/certs_test.go | 120 ++++++++++++++++++++++++++++++++-------------- 2 files changed, 115 insertions(+), 62 deletions(-) diff --git a/pki/certs.go b/pki/certs.go index 165339ca..b8eb578e 100644 --- a/pki/certs.go +++ b/pki/certs.go @@ -101,23 +101,29 @@ type CertificatePair struct { Cert *x509.Certificate } -// IssueTestCertificate uses the monitor's certIssuer and certIssuerKey to generate -// a precertificate and a matching final leaf-certificate that can be submitted -// to a log. The certificate's subject common name will be a random subdomain -// based on the certificate serial under the provided baseDomain (or -// defaultTestCertDomain domain if baseDomain is empty). +// IssueTestCertificate uses the monitor's certIssuer and certIssuerKey to +// generate a precertificate and a matching final leaf-certificate that +// can be submitted to a log. The certificate's subject common name will +// be a random subdomain based on the certificate serial under the +// provided baseDomain (or defaultTestCertDomain domain if baseDomain is +// empty). // -// If windowStart is nil the certificate NotBefore will be set to the current -// time based on the provided clock. If windowStart is not nil then the -// certificate NotBefore will be set to the windowStart plus one day. - -// If windowEnd is nil the certificate NotAfter will be set to 90 days after the -// current time based on the provided clock. If windowEnd is not nil then the -// certificate NotAfter will be set to the windowEnd minus one day. +// If windowStart is nil the certificate NotBefore will be set to the +// current time based on the provided clock. +// +// If windowEnd is nil the certificate NotAfter will be set to 90 days +// after the current time based on the provided clock. +// +// If windowStart and windowEnd are not nil then issue a 90 day +// certificate that falls in the window. // -// This function creates certificates that will be submitted to public logs and -// so while they are not issued by a trusted root we try to avoid cablint -// errors to avoid requiring log monitors special-case our submissions. +// with a NotAfter of `windowEnd - 1 hour` and a NotBefore +// 90 days earlier. +// +// This function creates certificates that will be submitted to public +// logs and so while they are not issued by a trusted root we try to +// avoid cablint errors to avoid requiring log monitors special-case our +// submissions. func IssueTestCertificate( baseDomain string, issuerKey *ecdsa.PrivateKey, @@ -141,16 +147,17 @@ func IssueTestCertificate( return CertificatePair{}, err } - earliest := clk.Now() - latest := earliest.AddDate(0, 0, 90) + notBefore := clk.Now() + notAfter := notBefore.AddDate(0, 0, 90) - if windowStart != nil { - earliest = *windowStart - earliest.AddDate(0, 0, 1) - } + // If `notAfter` generated from the system clock doesn't fall within the + // temporal shard window then we need to adjust the certificate validity + // to fall within the window. if windowEnd != nil { - latest = *windowEnd - latest.AddDate(0, 0, -1) + if notAfter.Before(*windowStart) || notAfter.After(*windowEnd) { + notAfter = windowEnd.Add(-1 * time.Hour) + notBefore = notAfter.AddDate(0, 0, -90) + } } domain := hex.EncodeToString(serial.Bytes()[:5]) + baseDomain @@ -162,8 +169,8 @@ func IssueTestCertificate( }, DNSNames: []string{domain}, SerialNumber: serial, - NotBefore: earliest, - NotAfter: latest.AddDate(0, 0, -1), + NotBefore: notBefore, + NotAfter: notAfter, KeyUsage: x509.KeyUsageDigitalSignature, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}, BasicConstraintsValid: true, diff --git a/pki/certs_test.go b/pki/certs_test.go index 3610a05d..ddcecefd 100644 --- a/pki/certs_test.go +++ b/pki/certs_test.go @@ -165,7 +165,7 @@ func TestIssueTestCertificate(t *testing.T) { } } -func TestIssueTestCertificateWindow(t *testing.T) { +func TestIssueTestCertificateWindowNil(t *testing.T) { issuerKey, _ := RandKey() issuerCert := &x509.Certificate{} clk := clock.New() @@ -190,7 +190,7 @@ func TestIssueTestCertificateWindow(t *testing.T) { // Check that the precert notbefore/notafter match defaults now := shortFormat(clk.Now()) - defaultNotAfter := shortFormat(clk.Now().AddDate(0, 0, 89)) + defaultNotAfter := shortFormat(clk.Now().AddDate(0, 0, 90)) notBefore := shortFormat(certPair.PreCert.NotBefore) notAfter := shortFormat(certPair.PreCert.NotAfter) if notBefore != now { @@ -213,48 +213,94 @@ func TestIssueTestCertificateWindow(t *testing.T) { t.Errorf("cert notAfter was %q, expected %q", notAfter, defaultNotAfter) } +} - windowStart, _ := time.Parse(time.RFC3339, "2000-01-01T00:00:00Z") - windowEnd, _ := time.Parse(time.RFC3339, "2001-01-01T00:00:00Z") - - // Issue a cert pair with specific WindowStart and WindowEnd - certPair, err = IssueTestCertificate("", issuerKey, issuerCert, clk, &windowStart, &windowEnd) - if err != nil { - t.Fatalf("unexpected error from IssueTestCertificate: %s", err.Error()) - } +// TestIssueTestCertificateWindowNotNil tests cases where the certificate +// is being generated for temporal shards and ensures the certificates are +// generated within the temporal window. +func TestIssueTestCertificateWindowNotNil(t *testing.T) { + issuerKey, _ := RandKey() + issuerCert := &x509.Certificate{} + clk := clock.New() - if certPair.PreCert == nil { - t.Fatalf("unexpected nil PreCert in CertPair returned from IssueTestCertificate") - } + // set window for a shard that is in its active temporal window + currentWindowStart := clk.Now().AddDate(0, 0, -30) + currentWindowEnd := clk.Now().AddDate(0, 0, 180) - if certPair.Cert == nil { - t.Fatalf("unexpected nil Cert in CertPair returned from IssueTestCertificate") - } + // set window for a shard that is in the past + pastWindowStart, _ := time.Parse(time.RFC3339, "2000-01-01T00:00:00Z") + pastWindowEnd, _ := time.Parse(time.RFC3339, "2001-01-01T00:00:00Z") - expectedStartDate := shortFormat(windowStart) - expectedEndDate := shortFormat(windowEnd.AddDate(0, 0, -1)) + // set window for a shard that is in the future + futureWindowStart, _ := time.Parse(time.RFC3339, "2100-01-01T00:00:00Z") + futureWindowEnd, _ := time.Parse(time.RFC3339, "2101-01-01T00:00:00Z") - // Check the precert notbefore/notafter match expected - notBefore = shortFormat(certPair.PreCert.NotBefore) - notAfter = shortFormat(certPair.PreCert.NotAfter) - if notBefore != expectedStartDate { - t.Errorf("preCert notBefore was %q, expected %q", - notBefore, expectedStartDate) - } - if notAfter != expectedEndDate { - t.Errorf("preCert notAfter was %q, expected %q", - notAfter, expectedEndDate) + type testCase struct { + name string + windowStart time.Time + windowEnd time.Time } - // Check that the cert notbefore/notafter match expected - notBefore = shortFormat(certPair.Cert.NotBefore) - notAfter = shortFormat(certPair.Cert.NotAfter) - if notBefore != expectedStartDate { - t.Errorf("cert notBefore was %q, expected %q", - notBefore, expectedStartDate) + testCases := []testCase{ + { + name: "current temporal shard", + windowStart: currentWindowStart, + windowEnd: currentWindowEnd, + }, + { + name: "past temporal shard", + windowStart: pastWindowStart, + windowEnd: pastWindowEnd, + }, + { + name: "future temporal shard", + windowStart: futureWindowStart, + windowEnd: futureWindowEnd, + }, } - if notAfter != expectedEndDate { - t.Errorf("cert notAfter was %q, expected %q", - notAfter, expectedEndDate) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Issue a cert pair with specific WindowStart and WindowEnd + certPair, err := IssueTestCertificate("", issuerKey, issuerCert, clk, &tc.windowStart, &tc.windowEnd) + if err != nil { + t.Fatalf("unexpected error from IssueTestCertificate: %s", err.Error()) + } + + if certPair.PreCert == nil { + t.Fatalf("unexpected nil PreCert in CertPair returned from IssueTestCertificate") + } + + if certPair.Cert == nil { + t.Fatalf("unexpected nil Cert in CertPair returned from IssueTestCertificate") + } + + shortFormat := func(t time.Time) string { + return t.Format("2006-01-02") + } + + // Check the precert notbefore/notafter match expected + notBefore := certPair.PreCert.NotBefore + notAfter := certPair.PreCert.NotAfter + if certPair.PreCert.NotAfter != certPair.PreCert.NotBefore.AddDate(0, 0, 90) { + t.Errorf("preCert notAfter was %q, expected %q", + notAfter, notBefore.AddDate(0, 0, 90)) + } + if certPair.PreCert.NotAfter.Before(tc.windowStart) || certPair.PreCert.NotAfter.After(tc.windowEnd) { + t.Errorf("preCert notAfter was %q, expected to be between %q and %q", + notAfter, tc.windowStart, tc.windowEnd) + } + + // Check that the cert notbefore/notafter match expected + notBefore = certPair.Cert.NotBefore + notAfter = certPair.Cert.NotAfter + if certPair.Cert.NotAfter != certPair.Cert.NotBefore.AddDate(0, 0, 90) { + t.Errorf("preCert notAfter was %q, expected %q", + shortFormat(notAfter), shortFormat(notBefore.AddDate(0, 0, 90))) + } + if certPair.Cert.NotAfter.Before(tc.windowStart) || certPair.Cert.NotAfter.After(tc.windowEnd) { + t.Errorf("cert notAfter was %q, expected to be between %q and %q", + notAfter, tc.windowStart, tc.windowEnd) + } + }) } } From b19f7cc5c1310756e3616c2a8778885a7ce2f75f Mon Sep 17 00:00:00 2001 From: Andrew Gabbitas Date: Sat, 1 Apr 2023 20:47:23 -0600 Subject: [PATCH 2/3] Fix failing CI tests * Update golangci-lint * Add the ability to run tests and lint manually in GitHub Actions * Add lint exclusions * Fix misspelling * Remove duplicate tls import --- .github/workflows/golangci-lint.yml | 10 ++++++---- .github/workflows/test.yml | 5 +++-- .golangci.yaml | 9 +++++++++ monitor/sth_fetcher.go | 25 +++++++++++++++---------- test/cttestsrv/log.go | 9 ++++----- 5 files changed, 37 insertions(+), 21 deletions(-) diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml index 305976b6..00555eea 100644 --- a/.github/workflows/golangci-lint.yml +++ b/.github/workflows/golangci-lint.yml @@ -8,6 +8,8 @@ on: pull_request: branches: - '*' + # Allows you to run this workflow manually from the Actions tab + workflow_dispatch: permissions: contents: read pull-requests: read @@ -16,11 +18,11 @@ jobs: name: lint runs-on: ubuntu-latest steps: - - uses: actions/setup-go@v2 - - uses: actions/checkout@v2 + - uses: actions/setup-go@v4 + - uses: actions/checkout@v3 - name: golangci-lint - uses: golangci/golangci-lint-action@v2 + uses: golangci/golangci-lint-action@v3 with: - version: v1.45.2 + version: latest args: --timeout 9m only-new-issues: true diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8e3a9ba7..0a39158d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -5,7 +5,8 @@ on: branches: [ main ] pull_request: branches: [ main ] - + # Allows you to run this workflow manually from the Actions tab + workflow_dispatch: jobs: test: runs-on: ubuntu-latest @@ -15,7 +16,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v3 with: - go-version: 1.17 + go-version: 1.20.2 - name: Install cover run: go install golang.org/x/tools/cmd/cover@latest diff --git a/.golangci.yaml b/.golangci.yaml index 9662998f..5846d5a0 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -12,3 +12,12 @@ linters: - stylecheck - unconvert - unused +linters-settings: + stylecheck: + # ST1005: Incorrectly formatted error string + checks: ["all", "-ST1005"] + gosec: + excludes: + # TODO: Identify, fix, and remove violations of most of these rules + - G112 # Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec) + - G404 # Use of weak random number generator (math/rand instead of crypto/rand) diff --git a/monitor/sth_fetcher.go b/monitor/sth_fetcher.go index de28e801..341787db 100644 --- a/monitor/sth_fetcher.go +++ b/monitor/sth_fetcher.go @@ -203,16 +203,21 @@ func (f *sthFetcher) observeSTH() { // firstSTH and secondSTH. If the two STHs don't verify then the // `sth_inconsistencies` prometheus counter is incremented with a label // indicating the category of inconsistency and an error is logged with -// `logErrorf`. Presently there are three possible categories of STH consistency -// failure: -// 1. "equal-treesize-inequal-hash" - the two STHs are the same tree size but -// have different root hashes. -// 2. "failed-to-get-proof" - the monitor encountered an error getting -// a consistency proof between the two STHs from the log. -// 3. "failed-to-verify-proof" - the monitor returned a proof of consistency -// between the two STHs that did not verify. -// When the monitor fetches a consistency proof from the log it publishes the -// latency of the operation to the `sth_proof_latency` prometheus histogram. +// `logErrorf`. Presently there are three possible categories of STH +// consistency failure: +// +// 1. "equal-treesize-inequal-hash" - the two STHs are the same tree size +// but have different root hashes. +// +// 2. "failed-to-get-proof" - the monitor encountered an error getting a +// consistency proof between the two STHs from the log. +// +// 3. "failed-to-verify-proof" - the monitor returned a proof of +// consistency between the two STHs that did not verify. +// +// When the monitor fetches a consistency proof from the log it publishes +// the latency of the operation to the `sth_proof_latency` prometheus +// histogram. func (f *sthFetcher) verifySTHConsistency(firstSTH, secondSTH *ct.SignedTreeHead) { if firstSTH == nil || secondSTH == nil { f.logErrorf("firstSTH or secondSTH was nil") diff --git a/test/cttestsrv/log.go b/test/cttestsrv/log.go index 8e15bdbd..3869aa6e 100644 --- a/test/cttestsrv/log.go +++ b/test/cttestsrv/log.go @@ -11,7 +11,6 @@ import ( "time" ct "github.com/google/certificate-transparency-go" - "github.com/google/certificate-transparency-go/tls" cttls "github.com/google/certificate-transparency-go/tls" ctfe "github.com/google/certificate-transparency-go/trillian/ctfe" "github.com/google/certificate-transparency-go/trillian/util" @@ -99,7 +98,7 @@ func makeTree(name string, _ *ecdsa.PrivateKey) (*testTree, error) { // initialize the tree with an empty STH if err := initSTH(tt); err != nil { - return nil, fmt.Errorf("initalizing STH: %w", err) + return nil, fmt.Errorf("initializing STH: %w", err) } return tt, nil @@ -111,14 +110,14 @@ func initSTH(tt *testTree) error { // init the new tree by signing a STH for the empty root slr := types.LogRoot{ - Version: tls.Enum(trillian.LogRootFormat_LOG_ROOT_FORMAT_V1), + Version: cttls.Enum(trillian.LogRootFormat_LOG_ROOT_FORMAT_V1), V1: &types.LogRootV1{ TreeSize: 0, RootHash: emptyRootHash[:], TimestampNanos: uint64(timeSource.Now().UnixNano()), }, } - slrBytes, err := tls.Marshal(slr) + slrBytes, err := cttls.Marshal(slr) if err != nil { return err } @@ -239,7 +238,7 @@ func (tl *testLog) getSTH() (*ct.SignedTreeHead, error) { } var slr ct.SignedTreeHead - _, err = tls.Unmarshal(signedLogRoot.LogRoot, &slr) + _, err = cttls.Unmarshal(signedLogRoot.LogRoot, &slr) if err != nil { return nil, err } From ac899be5b3b2465e6529cea7dbfda739e4d7b225 Mon Sep 17 00:00:00 2001 From: Andrew Gabbitas Date: Mon, 3 Apr 2023 12:37:09 -0600 Subject: [PATCH 3/3] Address review comments --- .golangci.yaml | 2 +- pki/certs.go | 11 ++++++----- pki/certs_test.go | 28 +++++++++++++++------------- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 5846d5a0..5a6ba153 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -18,6 +18,6 @@ linters-settings: checks: ["all", "-ST1005"] gosec: excludes: - # TODO: Identify, fix, and remove violations of most of these rules + # TODO(#133): Identify, fix, and remove violations of most of these rules - G112 # Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec) - G404 # Use of weak random number generator (math/rand instead of crypto/rand) diff --git a/pki/certs.go b/pki/certs.go index b8eb578e..26414fcb 100644 --- a/pki/certs.go +++ b/pki/certs.go @@ -117,9 +117,6 @@ type CertificatePair struct { // If windowStart and windowEnd are not nil then issue a 90 day // certificate that falls in the window. // -// with a NotAfter of `windowEnd - 1 hour` and a NotBefore -// 90 days earlier. -// // This function creates certificates that will be submitted to public // logs and so while they are not issued by a trusted root we try to // avoid cablint errors to avoid requiring log monitors special-case our @@ -147,8 +144,12 @@ func IssueTestCertificate( return CertificatePair{}, err } + // validityPeriod is 90 days minus 1 second, because RFC 5280 counts + // the final second as inclusive and golang counts it as exclusive. + validityPeriod := 90*24*time.Hour - time.Second + notBefore := clk.Now() - notAfter := notBefore.AddDate(0, 0, 90) + notAfter := notBefore.Add(validityPeriod) // If `notAfter` generated from the system clock doesn't fall within the // temporal shard window then we need to adjust the certificate validity @@ -156,7 +157,7 @@ func IssueTestCertificate( if windowEnd != nil { if notAfter.Before(*windowStart) || notAfter.After(*windowEnd) { notAfter = windowEnd.Add(-1 * time.Hour) - notBefore = notAfter.AddDate(0, 0, -90) + notBefore = notAfter.Add(-validityPeriod) } } diff --git a/pki/certs_test.go b/pki/certs_test.go index ddcecefd..de71007b 100644 --- a/pki/certs_test.go +++ b/pki/certs_test.go @@ -185,12 +185,13 @@ func TestIssueTestCertificateWindowNil(t *testing.T) { } shortFormat := func(t time.Time) string { - return t.Format("2006-01-02") + return t.Format("2006-01-02 15:04") } // Check that the precert notbefore/notafter match defaults - now := shortFormat(clk.Now()) - defaultNotAfter := shortFormat(clk.Now().AddDate(0, 0, 90)) + now := shortFormat(clk.Now().UTC()) + validityPeriod := 90*24*time.Hour - time.Second + defaultNotAfter := shortFormat(clk.Now().UTC().Add(validityPeriod)) notBefore := shortFormat(certPair.PreCert.NotBefore) notAfter := shortFormat(certPair.PreCert.NotAfter) if notBefore != now { @@ -222,6 +223,7 @@ func TestIssueTestCertificateWindowNotNil(t *testing.T) { issuerKey, _ := RandKey() issuerCert := &x509.Certificate{} clk := clock.New() + validityPeriod := 90*24*time.Hour - time.Second // set window for a shard that is in its active temporal window currentWindowStart := clk.Now().AddDate(0, 0, -30) @@ -274,15 +276,15 @@ func TestIssueTestCertificateWindowNotNil(t *testing.T) { } shortFormat := func(t time.Time) string { - return t.Format("2006-01-02") + return t.Format("2006-01-02 15:04:05") } // Check the precert notbefore/notafter match expected - notBefore := certPair.PreCert.NotBefore - notAfter := certPair.PreCert.NotAfter - if certPair.PreCert.NotAfter != certPair.PreCert.NotBefore.AddDate(0, 0, 90) { + notAfter := shortFormat(certPair.PreCert.NotAfter) + expectedNotAfter := shortFormat(certPair.PreCert.NotBefore.Add(validityPeriod)) + if notAfter != expectedNotAfter { t.Errorf("preCert notAfter was %q, expected %q", - notAfter, notBefore.AddDate(0, 0, 90)) + notAfter, expectedNotAfter) } if certPair.PreCert.NotAfter.Before(tc.windowStart) || certPair.PreCert.NotAfter.After(tc.windowEnd) { t.Errorf("preCert notAfter was %q, expected to be between %q and %q", @@ -290,11 +292,11 @@ func TestIssueTestCertificateWindowNotNil(t *testing.T) { } // Check that the cert notbefore/notafter match expected - notBefore = certPair.Cert.NotBefore - notAfter = certPair.Cert.NotAfter - if certPair.Cert.NotAfter != certPair.Cert.NotBefore.AddDate(0, 0, 90) { - t.Errorf("preCert notAfter was %q, expected %q", - shortFormat(notAfter), shortFormat(notBefore.AddDate(0, 0, 90))) + notAfter = shortFormat(certPair.Cert.NotAfter) + expectedNotAfter = shortFormat(certPair.Cert.NotBefore.Add(validityPeriod)) + if notAfter != expectedNotAfter { + t.Errorf("Cert notAfter was %q, expected %q", + notAfter, expectedNotAfter) } if certPair.Cert.NotAfter.Before(tc.windowStart) || certPair.Cert.NotAfter.After(tc.windowEnd) { t.Errorf("cert notAfter was %q, expected to be between %q and %q",