From 494c5a836902b0e7d091781f11e9a3d02f4efcc1 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 17 Jan 2025 14:33:47 -0800 Subject: [PATCH 01/12] test: add more testing for CRL revocation In revocation_test.go, fetch all CRLs, and look for revoked certificates on both CRLs and OCSP. Make s3-test-srv listen on all interfaces, so the CRL URLs in the CA config work. Add IssuerNameIDs to the CRL URLs in ca.json, to match how those CRLs are uploaded to S3. Make TestRevocation parallel. Speedup from ~60s to ~3s. Increase ocsp-responder's allowed parallelism to account for parallel test. --- test/config-next/ca.json | 12 +- test/config-next/ocsp-responder.json | 4 +- test/config/ca.json | 12 +- test/integration/revocation_test.go | 157 ++++++++++++++++++++++++++- test/startservers.py | 2 +- 5 files changed, 167 insertions(+), 20 deletions(-) diff --git a/test/config-next/ca.json b/test/config-next/ca.json index 37f69f6bbd7..3c1accbf7f5 100644 --- a/test/config-next/ca.json +++ b/test/config-next/ca.json @@ -77,7 +77,7 @@ "active": true, "issuerURL": "http://ca.example.org:4502/int-ecdsa-a", "ocspURL": "http://ca.example.org:4002/", - "crlURLBase": "http://ca.example.org:4501/ecdsa-a/", + "crlURLBase": "http://ca.example.org:4501/lets-encrypt-crls/43104258997432926/", "location": { "configFile": "test/certs/webpki/int-ecdsa-a.pkcs11.json", "certFile": "test/certs/webpki/int-ecdsa-a.cert.pem", @@ -88,7 +88,7 @@ "active": true, "issuerURL": "http://ca.example.org:4502/int-ecdsa-b", "ocspURL": "http://ca.example.org:4002/", - "crlURLBase": "http://ca.example.org:4501/ecdsa-b/", + "crlURLBase": "http://ca.example.org:4501/lets-encrypt-crls/17302365692836921/", "location": { "configFile": "test/certs/webpki/int-ecdsa-b.pkcs11.json", "certFile": "test/certs/webpki/int-ecdsa-b.cert.pem", @@ -99,7 +99,7 @@ "active": false, "issuerURL": "http://ca.example.org:4502/int-ecdsa-c", "ocspURL": "http://ca.example.org:4002/", - "crlURLBase": "http://ca.example.org:4501/ecdsa-c/", + "crlURLBase": "http://ca.example.org:4501/lets-encrypt-crls/56560759852043581/", "location": { "configFile": "test/certs/webpki/int-ecdsa-c.pkcs11.json", "certFile": "test/certs/webpki/int-ecdsa-c.cert.pem", @@ -110,7 +110,7 @@ "active": true, "issuerURL": "http://ca.example.org:4502/int-rsa-a", "ocspURL": "http://ca.example.org:4002/", - "crlURLBase": "http://ca.example.org:4501/rsa-a/", + "crlURLBase": "http://ca.example.org:4501/lets-encrypt-crls/29947985078257530/", "location": { "configFile": "test/certs/webpki/int-rsa-a.pkcs11.json", "certFile": "test/certs/webpki/int-rsa-a.cert.pem", @@ -121,7 +121,7 @@ "active": true, "issuerURL": "http://ca.example.org:4502/int-rsa-b", "ocspURL": "http://ca.example.org:4002/", - "crlURLBase": "http://ca.example.org:4501/rsa-b/", + "crlURLBase": "http://ca.example.org:4501/lets-encrypt-crls/6762885421992935/", "location": { "configFile": "test/certs/webpki/int-rsa-b.pkcs11.json", "certFile": "test/certs/webpki/int-rsa-b.cert.pem", @@ -132,7 +132,7 @@ "active": false, "issuerURL": "http://ca.example.org:4502/int-rsa-c", "ocspURL": "http://ca.example.org:4002/", - "crlURLBase": "http://ca.example.org:4501/rsa-c/", + "crlURLBase": "http://ca.example.org:4501/lets-encrypt-crls/56183656833365902/", "location": { "configFile": "test/certs/webpki/int-rsa-c.pkcs11.json", "certFile": "test/certs/webpki/int-rsa-c.cert.pem", diff --git a/test/config-next/ocsp-responder.json b/test/config-next/ocsp-responder.json index bae65304459..03ccc431874 100644 --- a/test/config-next/ocsp-responder.json +++ b/test/config-next/ocsp-responder.json @@ -53,8 +53,8 @@ ], "liveSigningPeriod": "60h", "timeout": "4.9s", - "maxInflightSignings": 2, - "maxSigningWaiters": 1, + "maxInflightSignings": 20, + "maxSigningWaiters": 100, "shutdownStopTimeout": "10s", "requiredSerialPrefixes": [ "7f" diff --git a/test/config/ca.json b/test/config/ca.json index 809d626ac34..32c5214524b 100644 --- a/test/config/ca.json +++ b/test/config/ca.json @@ -62,7 +62,7 @@ "active": true, "issuerURL": "http://ca.example.org:4502/int-ecdsa-a", "ocspURL": "http://ca.example.org:4002/", - "crlURLBase": "http://ca.example.org:4501/ecdsa-a/", + "crlURLBase": "http://ca.example.org:4501/lets-encrypt-crls/43104258997432926/", "location": { "configFile": "test/certs/webpki/int-ecdsa-a.pkcs11.json", "certFile": "test/certs/webpki/int-ecdsa-a.cert.pem", @@ -73,7 +73,7 @@ "active": true, "issuerURL": "http://ca.example.org:4502/int-ecdsa-b", "ocspURL": "http://ca.example.org:4002/", - "crlURLBase": "http://ca.example.org:4501/ecdsa-b/", + "crlURLBase": "http://ca.example.org:4501/lets-encrypt-crls/17302365692836921/", "location": { "configFile": "test/certs/webpki/int-ecdsa-b.pkcs11.json", "certFile": "test/certs/webpki/int-ecdsa-b.cert.pem", @@ -84,7 +84,7 @@ "active": false, "issuerURL": "http://ca.example.org:4502/int-ecdsa-c", "ocspURL": "http://ca.example.org:4002/", - "crlURLBase": "http://ca.example.org:4501/ecdsa-c/", + "crlURLBase": "http://ca.example.org:4501/lets-encrypt-crls/56560759852043581/", "location": { "configFile": "test/certs/webpki/int-ecdsa-c.pkcs11.json", "certFile": "test/certs/webpki/int-ecdsa-c.cert.pem", @@ -95,7 +95,7 @@ "active": true, "issuerURL": "http://ca.example.org:4502/int-rsa-a", "ocspURL": "http://ca.example.org:4002/", - "crlURLBase": "http://ca.example.org:4501/rsa-a/", + "crlURLBase": "http://ca.example.org:4501/lets-encrypt-crls/29947985078257530/", "location": { "configFile": "test/certs/webpki/int-rsa-a.pkcs11.json", "certFile": "test/certs/webpki/int-rsa-a.cert.pem", @@ -106,7 +106,7 @@ "active": true, "issuerURL": "http://ca.example.org:4502/int-rsa-b", "ocspURL": "http://ca.example.org:4002/", - "crlURLBase": "http://ca.example.org:4501/rsa-b/", + "crlURLBase": "http://ca.example.org:4501/lets-encrypt-crls/6762885421992935/", "location": { "configFile": "test/certs/webpki/int-rsa-b.pkcs11.json", "certFile": "test/certs/webpki/int-rsa-b.cert.pem", @@ -117,7 +117,7 @@ "active": false, "issuerURL": "http://ca.example.org:4502/int-rsa-c", "ocspURL": "http://ca.example.org:4002/", - "crlURLBase": "http://ca.example.org:4501/rsa-c/", + "crlURLBase": "http://ca.example.org:4501/lets-encrypt-crls/56183656833365902/", "location": { "configFile": "test/certs/webpki/int-rsa-c.pkcs11.json", "certFile": "test/certs/webpki/int-rsa-c.cert.pem", diff --git a/test/integration/revocation_test.go b/test/integration/revocation_test.go index c6ae66d73e2..e36a209a069 100644 --- a/test/integration/revocation_test.go +++ b/test/integration/revocation_test.go @@ -8,16 +8,23 @@ import ( "crypto/elliptic" "crypto/rand" "crypto/x509" + "encoding/hex" + "encoding/json" + "encoding/pem" "fmt" "io" "net/http" + "os" + "path" "strings" + "sync" "testing" "time" "github.com/eggsampler/acme/v3" "golang.org/x/crypto/ocsp" + "github.com/letsencrypt/boulder/crl/idp" "github.com/letsencrypt/boulder/test" ocsp_helper "github.com/letsencrypt/boulder/test/ocsp/helper" ) @@ -33,6 +40,120 @@ func isPrecert(cert *x509.Certificate) bool { return false } +// getALLCRLs fetches and parses each certificate for each configured CA. +// Returns a map from issuer SKID (hex) to a list of that issuer's CRLs. +func getAllCRLs(t *testing.T) map[string][]*x509.RevocationList { + b, err := os.ReadFile(path.Join(os.Getenv("BOULDER_CONFIG_DIR"), "ca.json")) + if err != nil { + t.Fatalf("reading CA config: %s", err) + } + + var conf struct { + CA struct { + Issuance struct { + Issuers []struct { + CRLURLBase string + CRLShards int + Location struct { + CertFile string + } + } + } + } + } + + err = json.Unmarshal(b, &conf) + if err != nil { + t.Fatalf("unmarshaling CA config: %s", err) + } + + ret := make(map[string][]*x509.RevocationList) + + for _, issuer := range conf.CA.Issuance.Issuers { + issuerPEMBytes, err := os.ReadFile(issuer.Location.CertFile) + if err != nil { + t.Fatalf("reading CRL issuer: %s", err) + } + + block, _ := pem.Decode(issuerPEMBytes) + issuerCert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + t.Fatalf("parsing CRL issuer: %s", err) + } + + issuerSKID := hex.EncodeToString(issuerCert.SubjectKeyId) + + // 10 is the number of shards configured in test/config*/crl-updater.json + for i := range 10 { + crlURL := fmt.Sprintf("%s%d.crl", issuer.CRLURLBase, i+1) + resp, err := http.Get(crlURL) + if err != nil { + t.Fatalf("getting CRL from %s: %s", crlURL, err) + } + // When there's nothing in a shard, I guess it doesn't get created? + if resp.StatusCode == 404 { + continue + } + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("reading CRL from %s: %s", crlURL, err) + } + + list, err := x509.ParseRevocationList(body) + if err != nil { + t.Fatalf("parsing CRL from %s: %s (bytes: %x)", crlURL, err, string(body)) + } + + err = list.CheckSignatureFrom(issuerCert) + if err != nil { + t.Errorf("checking CRL signature on %s from %s: %s", + crlURL, issuerCert.Subject, err) + } + + idpURIs, err := idp.GetIDPURIs(list.Extensions) + if err != nil { + t.Fatalf("getting IDP URIs: %s", err) + } + if len(idpURIs) != 1 { + t.Errorf("CRL at %s: expected 1 IDP URI, got %s", crlURL, idpURIs) + } + if idpURIs[0] != crlURL { + t.Errorf("fetched CRL from %s, got IDP of %s (should be same)", crlURL, idpURIs[0]) + } + + ret[issuerSKID] = append(ret[issuerSKID], list) + } + } + return ret +} + +func checkRevoked(t *testing.T, revocations map[string][]*x509.RevocationList, cert *x509.Certificate, reason int) { + akid := hex.EncodeToString(cert.AuthorityKeyId) + if len(revocations[akid]) == 0 { + t.Errorf("no CRLs found for authorityKeyID %s", akid) + } + var matches []x509.RevocationListEntry + var count int + for _, list := range revocations[akid] { + for _, entry := range list.RevokedCertificateEntries { + count++ + if entry.SerialNumber.Cmp(cert.SerialNumber) == 0 { + matches = append(matches, entry) + } + } + } + if len(matches) == 0 { + t.Errorf("didn't find matching entry on combined CRLs of length %d", count) + + } + for _, match := range matches { + if match.ReasonCode != reason { + t.Errorf("revoked certificate %x: got reason %d, want %d", cert.SerialNumber, match.ReasonCode, reason) + } + return + } +} + // TestRevocation tests that a certificate can be revoked using all of the // RFC 8555 revocation authentication mechanisms. It does so for both certs and // precerts (with no corresponding final cert), and for both the Unspecified and @@ -79,9 +200,21 @@ func TestRevocation(t *testing.T) { } } + type expectedRevocation struct { + tc testCase + cert *x509.Certificate + reason int + } + + var wg sync.WaitGroup + var once sync.Once + var revocations map[string][]*x509.RevocationList + for _, tc := range testCases { name := fmt.Sprintf("%s_%d_%s", tc.kind, tc.reason, tc.method) t.Run(name, func(t *testing.T) { + t.Parallel() + wg.Add(1) issueClient, err := makeClient() test.AssertNotError(t, err, "creating acme client") @@ -176,17 +309,31 @@ func TestRevocation(t *testing.T) { // names, the reason should be overwritten to CessationOfOperation (5), // and if the request was made by key, then the reason should be set to // KeyCompromise (1). - ocspConfig = ocsp_helper.DefaultConfig.WithExpectStatus(ocsp.Revoked) + var expectedReason int switch tc.method { case byAuth: - ocspConfig = ocspConfig.WithExpectReason(ocsp.CessationOfOperation) + expectedReason = ocsp.CessationOfOperation case byKey: - ocspConfig = ocspConfig.WithExpectReason(ocsp.KeyCompromise) + expectedReason = ocsp.KeyCompromise default: - ocspConfig = ocspConfig.WithExpectReason(tc.reason) + expectedReason = tc.reason } - _, err = ocsp_helper.ReqDER(cert.Raw, ocspConfig) + _, err = ocsp_helper.ReqDER(cert.Raw, ocsp_helper. + DefaultConfig. + WithExpectStatus(ocsp.Revoked). + WithExpectReason(expectedReason)) test.AssertNotError(t, err, "requesting OCSP for revoked cert") + + // For CRLs, wait till everyone's done before running the updater and + // fetching all the CRLs. + wg.Done() + wg.Wait() + once.Do(func() { + runUpdater(t, path.Join(os.Getenv("BOULDER_CONFIG_DIR"), "crl-updater.json")) + revocations = getAllCRLs(t) + }) + + checkRevoked(t, revocations, cert, expectedReason) }) } } diff --git a/test/startservers.py b/test/startservers.py index 93d0c25bcee..2d94c53e5df 100644 --- a/test/startservers.py +++ b/test/startservers.py @@ -84,7 +84,7 @@ ('akamai-test-srv',)), Service('s3-test-srv', 4501, None, None, - ('./bin/s3-test-srv', '--listen', 'localhost:4501'), + ('./bin/s3-test-srv', '--listen', ':4501'), None), Service('crl-storer', 9667, None, None, From b64f5deed53b18f36a374c35421ea3459fc59641 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 17 Jan 2025 15:47:43 -0800 Subject: [PATCH 02/12] Fix data race --- test/integration/revocation_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/revocation_test.go b/test/integration/revocation_test.go index e36a209a069..4f8a6ce4a7b 100644 --- a/test/integration/revocation_test.go +++ b/test/integration/revocation_test.go @@ -206,7 +206,7 @@ func TestRevocation(t *testing.T) { reason int } - var wg sync.WaitGroup + wg := new(sync.WaitGroup) var once sync.Once var revocations map[string][]*x509.RevocationList From 7910ce5f15d7bcd0d462a741fab5ad299d88dbc7 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 17 Jan 2025 15:54:04 -0800 Subject: [PATCH 03/12] Fix data race --- test/integration/revocation_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/revocation_test.go b/test/integration/revocation_test.go index 4f8a6ce4a7b..d10fd095138 100644 --- a/test/integration/revocation_test.go +++ b/test/integration/revocation_test.go @@ -206,15 +206,15 @@ func TestRevocation(t *testing.T) { reason int } - wg := new(sync.WaitGroup) + var wg sync.WaitGroup var once sync.Once var revocations map[string][]*x509.RevocationList for _, tc := range testCases { name := fmt.Sprintf("%s_%d_%s", tc.kind, tc.reason, tc.method) + wg.Add(1) t.Run(name, func(t *testing.T) { t.Parallel() - wg.Add(1) issueClient, err := makeClient() test.AssertNotError(t, err, "creating acme client") From c1071b525475f35cd84115cb82f5bb4bf62e6a64 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 17 Jan 2025 23:17:03 -0800 Subject: [PATCH 04/12] Rearrange parallelism --- test/integration/revocation_test.go | 272 ++++++++++++++-------------- 1 file changed, 139 insertions(+), 133 deletions(-) diff --git a/test/integration/revocation_test.go b/test/integration/revocation_test.go index d10fd095138..62341d2b9b5 100644 --- a/test/integration/revocation_test.go +++ b/test/integration/revocation_test.go @@ -180,160 +180,166 @@ func TestRevocation(t *testing.T) { kind certKind } - var testCases []testCase - for _, kind := range []certKind{precert, finalcert} { - for _, reason := range []int{ocsp.Unspecified, ocsp.KeyCompromise} { - for _, method := range []authMethod{byAccount, byAuth, byKey} { - testCases = append(testCases, testCase{ - method: method, - reason: reason, - kind: kind, - // We do not expect any of these revocation requests to error. - // The ones done byAccount will succeed as requested, but will not - // result in the key being blocked for future issuance. - // The ones done byAuth will succeed, but will be overwritten to have - // reason code 5 (cessationOfOperation). - // The ones done byKey will succeed, but will be overwritten to have - // reason code 1 (keyCompromise), and will block the key. - }) - } - } - } + issueAndRevoke := func(tc testCase) *x509.Certificate { + issueClient, err := makeClient() + test.AssertNotError(t, err, "creating acme client") - type expectedRevocation struct { - tc testCase - cert *x509.Certificate - reason int - } + certKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + test.AssertNotError(t, err, "creating random cert key") - var wg sync.WaitGroup - var once sync.Once - var revocations map[string][]*x509.RevocationList + domain := random_domain() - for _, tc := range testCases { - name := fmt.Sprintf("%s_%d_%s", tc.kind, tc.reason, tc.method) - wg.Add(1) - t.Run(name, func(t *testing.T) { - t.Parallel() - issueClient, err := makeClient() - test.AssertNotError(t, err, "creating acme client") + // Try to issue a certificate for the name. + var cert *x509.Certificate + switch tc.kind { + case finalcert: + res, err := authAndIssue(issueClient, certKey, []string{domain}, true) + test.AssertNotError(t, err, "authAndIssue failed") + cert = res.certs[0] + + case precert: + // Make sure the ct-test-srv will reject generating SCTs for the domain, + // so we only get a precert and no final cert. + err := ctAddRejectHost(domain) + test.AssertNotError(t, err, "adding ct-test-srv reject host") + + _, err = authAndIssue(issueClient, certKey, []string{domain}, true) + test.AssertError(t, err, "expected error from authAndIssue, was nil") + if !strings.Contains(err.Error(), "urn:ietf:params:acme:error:serverInternal") || + !strings.Contains(err.Error(), "SCT embedding") { + t.Fatal(err) + } - certKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) - test.AssertNotError(t, err, "creating random cert key") + // Instead recover the precertificate from CT. + cert, err = ctFindRejection([]string{domain}) + if err != nil || cert == nil { + t.Fatalf("couldn't find rejected precert for %q", domain) + } + // And make sure the cert we found is in fact a precert. + if !isPrecert(cert) { + t.Fatal("precert was missing poison extension") + } - domain := random_domain() + default: + t.Fatalf("unrecognized cert kind %q", tc.kind) + } - // Try to issue a certificate for the name. - var cert *x509.Certificate - switch tc.kind { - case finalcert: - res, err := authAndIssue(issueClient, certKey, []string{domain}, true) - test.AssertNotError(t, err, "authAndIssue failed") - cert = res.certs[0] - - case precert: - // Make sure the ct-test-srv will reject generating SCTs for the domain, - // so we only get a precert and no final cert. - err := ctAddRejectHost(domain) - test.AssertNotError(t, err, "adding ct-test-srv reject host") - - _, err = authAndIssue(issueClient, certKey, []string{domain}, true) - test.AssertError(t, err, "expected error from authAndIssue, was nil") - if !strings.Contains(err.Error(), "urn:ietf:params:acme:error:serverInternal") || - !strings.Contains(err.Error(), "SCT embedding") { - t.Fatal(err) - } + // Initially, the cert should have a Good OCSP response. + ocspConfig := ocsp_helper.DefaultConfig.WithExpectStatus(ocsp.Good) + _, err = ocsp_helper.ReqDER(cert.Raw, ocspConfig) + test.AssertNotError(t, err, "requesting OCSP for precert") - // Instead recover the precertificate from CT. - cert, err = ctFindRejection([]string{domain}) - if err != nil || cert == nil { - t.Fatalf("couldn't find rejected precert for %q", domain) - } - // And make sure the cert we found is in fact a precert. - if !isPrecert(cert) { - t.Fatal("precert was missing poison extension") - } + // Set up the account and key that we'll use to revoke the cert. + var revokeClient *client + var revokeKey crypto.Signer + switch tc.method { + case byAccount: + // When revoking by account, use the same client and key as were used + // for the original issuance. + revokeClient = issueClient + revokeKey = revokeClient.PrivateKey + + case byAuth: + // When revoking by auth, create a brand new client, authorize it for + // the same domain, and use that account and key for revocation. Ignore + // errors from authAndIssue because all we need is the auth, not the + // issuance. + revokeClient, err = makeClient() + test.AssertNotError(t, err, "creating second acme client") + _, _ = authAndIssue(revokeClient, certKey, []string{domain}, true) + revokeKey = revokeClient.PrivateKey - default: - t.Fatalf("unrecognized cert kind %q", tc.kind) - } + case byKey: + // When revoking by key, create a brand new client and use it with + // the cert's key for revocation. + revokeClient, err = makeClient() + test.AssertNotError(t, err, "creating second acme client") + revokeKey = certKey + + default: + t.Fatalf("unrecognized revocation method %q", tc.method) + } - // Initially, the cert should have a Good OCSP response. - ocspConfig := ocsp_helper.DefaultConfig.WithExpectStatus(ocsp.Good) - _, err = ocsp_helper.ReqDER(cert.Raw, ocspConfig) - test.AssertNotError(t, err, "requesting OCSP for precert") + // Revoke the cert using the specified key and client. + err = revokeClient.RevokeCertificate( + revokeClient.Account, + cert, + revokeKey, + tc.reason, + ) + test.AssertNotError(t, err, "revocation should have succeeded") - // Set up the account and key that we'll use to revoke the cert. - var revokeClient *client - var revokeKey crypto.Signer - switch tc.method { - case byAccount: - // When revoking by account, use the same client and key as were used - // for the original issuance. - revokeClient = issueClient - revokeKey = revokeClient.PrivateKey + return cert + } - case byAuth: - // When revoking by auth, create a brand new client, authorize it for - // the same domain, and use that account and key for revocation. Ignore - // errors from authAndIssue because all we need is the auth, not the - // issuance. - revokeClient, err = makeClient() - test.AssertNotError(t, err, "creating second acme client") - _, _ = authAndIssue(revokeClient, certKey, []string{domain}, true) - revokeKey = revokeClient.PrivateKey + type expectation struct { + *x509.Certificate + name string + revocationReason int + } + var expectations []expectation + var eMu sync.Mutex + var wg sync.WaitGroup - case byKey: - // When revoking by key, create a brand new client and use it with - // the cert's key for revocation. - revokeClient, err = makeClient() - test.AssertNotError(t, err, "creating second acme client") - revokeKey = certKey + for _, kind := range []certKind{precert, finalcert} { + for _, reason := range []int{ocsp.Unspecified, ocsp.KeyCompromise} { + for _, method := range []authMethod{byAccount, byAuth, byKey} { + wg.Add(1) + go func() { + defer wg.Done() + cert := issueAndRevoke(testCase{ + method: method, + reason: reason, + kind: kind, + // We do not expect any of these revocation requests to error. + // The ones done byAccount will succeed as requested, but will not + // result in the key being blocked for future issuance. + // The ones done byAuth will succeed, but will be overwritten to have + // reason code 5 (cessationOfOperation). + // The ones done byKey will succeed, but will be overwritten to have + // reason code 1 (keyCompromise), and will block the key. + }) + + // If the request was made by demonstrating control over the + // names, the reason should be overwritten to CessationOfOperation (5), + // and if the request was made by key, then the reason should be set to + // KeyCompromise (1). + expectedReason := reason + switch method { + case byAuth: + expectedReason = ocsp.CessationOfOperation + case byKey: + expectedReason = ocsp.KeyCompromise + default: + } - default: - t.Fatalf("unrecognized revocation method %q", tc.method) + eMu.Lock() + expectations = append(expectations, expectation{ + Certificate: cert, + name: fmt.Sprintf("%s_%d_%s", kind, reason, method), + revocationReason: expectedReason, + }) + eMu.Unlock() + }() } + } + } - // Revoke the cert using the specified key and client. - err = revokeClient.RevokeCertificate( - revokeClient.Account, - cert, - revokeKey, - tc.reason, - ) + wg.Wait() - test.AssertNotError(t, err, "revocation should have succeeded") + runUpdater(t, path.Join(os.Getenv("BOULDER_CONFIG_DIR"), "crl-updater.json")) + revocations := getAllCRLs(t) - // Check the OCSP response for the certificate again. It should now be - // revoked. If the request was made by demonstrating control over the - // names, the reason should be overwritten to CessationOfOperation (5), - // and if the request was made by key, then the reason should be set to - // KeyCompromise (1). - var expectedReason int - switch tc.method { - case byAuth: - expectedReason = ocsp.CessationOfOperation - case byKey: - expectedReason = ocsp.KeyCompromise - default: - expectedReason = tc.reason - } - _, err = ocsp_helper.ReqDER(cert.Raw, ocsp_helper. + for _, expectation := range expectations { + t.Run(expectation.name, func(t *testing.T) { + t.Parallel() + _, err := ocsp_helper.ReqDER(expectation.Raw, ocsp_helper. DefaultConfig. WithExpectStatus(ocsp.Revoked). - WithExpectReason(expectedReason)) + WithExpectReason(expectation.revocationReason)) test.AssertNotError(t, err, "requesting OCSP for revoked cert") - // For CRLs, wait till everyone's done before running the updater and - // fetching all the CRLs. - wg.Done() - wg.Wait() - once.Do(func() { - runUpdater(t, path.Join(os.Getenv("BOULDER_CONFIG_DIR"), "crl-updater.json")) - revocations = getAllCRLs(t) - }) - - checkRevoked(t, revocations, cert, expectedReason) + checkRevoked(t, revocations, expectation.Certificate, expectation.revocationReason) }) } } From 9d25fb06e8879ab945db48ca3fdec7b8090433be Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Tue, 21 Jan 2025 10:02:59 -0800 Subject: [PATCH 05/12] Change crl-updater log levels --- test/config-next/crl-updater.json | 2 +- test/config/crl-updater.json | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/config-next/crl-updater.json b/test/config-next/crl-updater.json index 86f7e601d3d..ae2a90ac333 100644 --- a/test/config-next/crl-updater.json +++ b/test/config-next/crl-updater.json @@ -53,7 +53,7 @@ "features": {} }, "syslog": { - "stdoutlevel": 6, + "stdoutlevel": 4, "sysloglevel": -1 }, "openTelemetry": { diff --git a/test/config/crl-updater.json b/test/config/crl-updater.json index 21f3603bb5d..d89ee38461c 100644 --- a/test/config/crl-updater.json +++ b/test/config/crl-updater.json @@ -53,7 +53,7 @@ "features": {} }, "syslog": { - "stdoutlevel": 6, - "sysloglevel": 6 + "stdoutlevel": 4, + "sysloglevel": 4 } } From 06dca5e934764865e301dfe9a109ee719283e09e Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Tue, 21 Jan 2025 10:40:49 -0800 Subject: [PATCH 06/12] Put a mutex around runUpdater --- test/integration/crl_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/integration/crl_test.go b/test/integration/crl_test.go index fc7cc28a01a..c567047d962 100644 --- a/test/integration/crl_test.go +++ b/test/integration/crl_test.go @@ -11,6 +11,7 @@ import ( "path" "path/filepath" "strings" + "sync" "testing" "time" @@ -21,10 +22,16 @@ import ( "github.com/letsencrypt/boulder/test/vars" ) +// crlUpdaterMu controls access to `runUpdater`, because two crl-updaters running +// at once will result in errors trying to lease shards that are already leased. +var crlUpdaterMu sync.Mutex + // runUpdater executes the crl-updater binary with the -runOnce flag, and // returns when it completes. func runUpdater(t *testing.T, configFile string) { t.Helper() + crlUpdaterMu.Lock() + defer crlUpdaterMu.Unlock() binPath, err := filepath.Abs("bin/boulder") test.AssertNotError(t, err, "computing boulder binary path") From fde3fdaf211b69036ed12f32fa1a02b60061d35d Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 22 Jan 2025 14:02:21 -0800 Subject: [PATCH 07/12] Review feedback --- test/integration/revocation_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/revocation_test.go b/test/integration/revocation_test.go index 62341d2b9b5..2ecab3d18c1 100644 --- a/test/integration/revocation_test.go +++ b/test/integration/revocation_test.go @@ -98,6 +98,7 @@ func getAllCRLs(t *testing.T) map[string][]*x509.RevocationList { if err != nil { t.Fatalf("reading CRL from %s: %s", crlURL, err) } + resp.Body.Close() list, err := x509.ParseRevocationList(body) if err != nil { @@ -150,7 +151,6 @@ func checkRevoked(t *testing.T, revocations map[string][]*x509.RevocationList, c if match.ReasonCode != reason { t.Errorf("revoked certificate %x: got reason %d, want %d", cert.SerialNumber, match.ReasonCode, reason) } - return } } From 3f04e44d768470a7210144ff382680096bbd5a0c Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 23 Jan 2025 10:03:10 -0800 Subject: [PATCH 08/12] Review feedback --- test/integration/revocation_test.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/integration/revocation_test.go b/test/integration/revocation_test.go index 2ecab3d18c1..1834f34dd63 100644 --- a/test/integration/revocation_test.go +++ b/test/integration/revocation_test.go @@ -43,6 +43,7 @@ func isPrecert(cert *x509.Certificate) bool { // getALLCRLs fetches and parses each certificate for each configured CA. // Returns a map from issuer SKID (hex) to a list of that issuer's CRLs. func getAllCRLs(t *testing.T) map[string][]*x509.RevocationList { + t.Helper() b, err := os.ReadFile(path.Join(os.Getenv("BOULDER_CONFIG_DIR"), "ca.json")) if err != nil { t.Fatalf("reading CA config: %s", err) @@ -53,7 +54,6 @@ func getAllCRLs(t *testing.T) map[string][]*x509.RevocationList { Issuance struct { Issuers []struct { CRLURLBase string - CRLShards int Location struct { CertFile string } @@ -102,7 +102,7 @@ func getAllCRLs(t *testing.T) map[string][]*x509.RevocationList { list, err := x509.ParseRevocationList(body) if err != nil { - t.Fatalf("parsing CRL from %s: %s (bytes: %x)", crlURL, err, string(body)) + t.Fatalf("parsing CRL from %s: %s (bytes: %x)", crlURL, err, body) } err = list.CheckSignatureFrom(issuerCert) @@ -129,6 +129,7 @@ func getAllCRLs(t *testing.T) map[string][]*x509.RevocationList { } func checkRevoked(t *testing.T, revocations map[string][]*x509.RevocationList, cert *x509.Certificate, reason int) { + t.Helper() akid := hex.EncodeToString(cert.AuthorityKeyId) if len(revocations[akid]) == 0 { t.Errorf("no CRLs found for authorityKeyID %s", akid) @@ -144,8 +145,7 @@ func checkRevoked(t *testing.T, revocations map[string][]*x509.RevocationList, c } } if len(matches) == 0 { - t.Errorf("didn't find matching entry on combined CRLs of length %d", count) - + t.Errorf("searching for %x in CRLs: no entry on combined CRLs of length %d", cert.SerialNumber, count) } for _, match := range matches { if match.ReasonCode != reason { @@ -272,8 +272,11 @@ func TestRevocation(t *testing.T) { return cert } + // expectation represents a certificate that was issued and revoked, along with + // its revocation reason. Once all the issuances and revocations are done, this + // will be used to check the CRL statuses for each certificate. type expectation struct { - *x509.Certificate + revokedCert *x509.Certificate name string revocationReason int } @@ -315,7 +318,7 @@ func TestRevocation(t *testing.T) { eMu.Lock() expectations = append(expectations, expectation{ - Certificate: cert, + revokedCert: cert, name: fmt.Sprintf("%s_%d_%s", kind, reason, method), revocationReason: expectedReason, }) @@ -333,13 +336,10 @@ func TestRevocation(t *testing.T) { for _, expectation := range expectations { t.Run(expectation.name, func(t *testing.T) { t.Parallel() - _, err := ocsp_helper.ReqDER(expectation.Raw, ocsp_helper. - DefaultConfig. - WithExpectStatus(ocsp.Revoked). - WithExpectReason(expectation.revocationReason)) + _, err := ocsp_helper.ReqDER(expectation.revokedCert.Raw, ocsp_helper.DefaultConfig.WithExpectStatus(ocsp.Revoked).WithExpectReason(expectation.revocationReason)) test.AssertNotError(t, err, "requesting OCSP for revoked cert") - checkRevoked(t, revocations, expectation.Certificate, expectation.revocationReason) + checkRevoked(t, revocations, expectation.revokedCert, expectation.revocationReason) }) } } From f85c34c3fc8a8e2084949e331660c811f99e818b Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 23 Jan 2025 10:41:23 -0800 Subject: [PATCH 09/12] Refine comment --- test/integration/revocation_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/integration/revocation_test.go b/test/integration/revocation_test.go index 1834f34dd63..d2ba01fc1aa 100644 --- a/test/integration/revocation_test.go +++ b/test/integration/revocation_test.go @@ -272,9 +272,10 @@ func TestRevocation(t *testing.T) { return cert } - // expectation represents a certificate that was issued and revoked, along with - // its revocation reason. Once all the issuances and revocations are done, this - // will be used to check the CRL statuses for each certificate. + // expectation represents an expectation that a specific certificate will be revoked + // with a specific revocation reason. We need the whole certificate because we depend + // on various fields to validate revocation: the issuer, serial number, and + // AuthorityInformationAccess extension. type expectation struct { revokedCert *x509.Certificate name string From 4b1ba28899672f21f73b90a038faa2413bf4166c Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 23 Jan 2025 10:45:14 -0800 Subject: [PATCH 10/12] Remove workaround for 404 (no longer needed) --- test/integration/revocation_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/integration/revocation_test.go b/test/integration/revocation_test.go index d2ba01fc1aa..ae392c0b6f1 100644 --- a/test/integration/revocation_test.go +++ b/test/integration/revocation_test.go @@ -90,9 +90,8 @@ func getAllCRLs(t *testing.T) map[string][]*x509.RevocationList { if err != nil { t.Fatalf("getting CRL from %s: %s", crlURL, err) } - // When there's nothing in a shard, I guess it doesn't get created? - if resp.StatusCode == 404 { - continue + if resp.StatusCode != http.StatusOK { + t.Fatalf("fetching %s: status code %d", crlURL, resp.StatusCode) } body, err := io.ReadAll(resp.Body) if err != nil { From 8efdcdcf25ed061c0cff1a0f219180c59d3ccad4 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 23 Jan 2025 15:16:51 -0800 Subject: [PATCH 11/12] Switch to closures --- test/integration/revocation_test.go | 47 ++++++++++++----------------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/test/integration/revocation_test.go b/test/integration/revocation_test.go index ae392c0b6f1..093b9ccd8c7 100644 --- a/test/integration/revocation_test.go +++ b/test/integration/revocation_test.go @@ -271,17 +271,13 @@ func TestRevocation(t *testing.T) { return cert } - // expectation represents an expectation that a specific certificate will be revoked - // with a specific revocation reason. We need the whole certificate because we depend - // on various fields to validate revocation: the issuer, serial number, and - // AuthorityInformationAccess extension. - type expectation struct { - revokedCert *x509.Certificate - name string - revocationReason int - } - var expectations []expectation - var eMu sync.Mutex + // revocationCheck represents a deferred that a specific certificate is revoked. + // + // We defer these checks for performance reasons: we want to run crl-updater once, + // after all certificates have been revoked. + type revocationCheck func(t *testing.T, allCRLs map[string][]*x509.RevocationList) + var revocationChecks []revocationCheck + var rcMu sync.Mutex var wg sync.WaitGroup for _, kind := range []certKind{precert, finalcert} { @@ -316,13 +312,16 @@ func TestRevocation(t *testing.T) { default: } - eMu.Lock() - expectations = append(expectations, expectation{ - revokedCert: cert, - name: fmt.Sprintf("%s_%d_%s", kind, reason, method), - revocationReason: expectedReason, - }) - eMu.Unlock() + check := func(t *testing.T, allCRLs map[string][]*x509.RevocationList) { + _, err := ocsp_helper.ReqDER(cert.Raw, ocsp_helper.DefaultConfig.WithExpectStatus(ocsp.Revoked).WithExpectReason(expectedReason)) + test.AssertNotError(t, err, "requesting OCSP for revoked cert") + + checkRevoked(t, allCRLs, cert, expectedReason) + } + + rcMu.Lock() + revocationChecks = append(revocationChecks, check) + rcMu.Unlock() }() } } @@ -331,16 +330,10 @@ func TestRevocation(t *testing.T) { wg.Wait() runUpdater(t, path.Join(os.Getenv("BOULDER_CONFIG_DIR"), "crl-updater.json")) - revocations := getAllCRLs(t) - - for _, expectation := range expectations { - t.Run(expectation.name, func(t *testing.T) { - t.Parallel() - _, err := ocsp_helper.ReqDER(expectation.revokedCert.Raw, ocsp_helper.DefaultConfig.WithExpectStatus(ocsp.Revoked).WithExpectReason(expectation.revocationReason)) - test.AssertNotError(t, err, "requesting OCSP for revoked cert") + allCRLs := getAllCRLs(t) - checkRevoked(t, revocations, expectation.revokedCert, expectation.revocationReason) - }) + for _, check := range revocationChecks { + check(t, allCRLs) } } From 238530d14a6dad47647f69203f978e599c751a41 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 23 Jan 2025 15:49:37 -0800 Subject: [PATCH 12/12] Update ocsp-responder configs --- test/config-next/ocsp-responder.json | 2 +- test/config/ocsp-responder.json | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/test/config-next/ocsp-responder.json b/test/config-next/ocsp-responder.json index 03ccc431874..4e506323328 100644 --- a/test/config-next/ocsp-responder.json +++ b/test/config-next/ocsp-responder.json @@ -53,9 +53,9 @@ ], "liveSigningPeriod": "60h", "timeout": "4.9s", + "shutdownStopTimeout": "10s", "maxInflightSignings": 20, "maxSigningWaiters": 100, - "shutdownStopTimeout": "10s", "requiredSerialPrefixes": [ "7f" ], diff --git a/test/config/ocsp-responder.json b/test/config/ocsp-responder.json index c67aa41f736..2e4ba1461a5 100644 --- a/test/config/ocsp-responder.json +++ b/test/config/ocsp-responder.json @@ -59,6 +59,7 @@ "liveSigningPeriod": "60h", "timeout": "4.9s", "shutdownStopTimeout": "10s", + "maxInflightSignings": 20, "debugAddr": ":8005", "requiredSerialPrefixes": [ "7f"