Fix re-revocation representations on CRL (#8096)

jsha · web-flow · commit 27e08f484698 · 2025-04-03T15:03:57.000-07:00
For explicitly sharded certificates, CRL status is read from the
`revokedCertificates` table. This table gets written at revocation time.
At re-revocation time (for key compromise), it only gets written by the
SA if the caller passes a nonzero ShardIdx to UpdateRevokedCertificate.
The RA was never passing a nonzero ShardIdx to UpdateRevokedCertificate.
diff --git a/ra/ra.go b/ra/ra.go
@@ -1754,12 +1754,27 @@ func (ra *RegistrationAuthorityImpl) updateRevocationForKeyCompromise(ctx contex
 		return berrors.AlreadyRevokedError("unable to re-revoke serial %q which is already revoked for keyCompromise", serialString)
 	}
 
+	cert, err := ra.SA.GetCertificate(ctx, &sapb.Serial{Serial: serialString})
+	if err != nil {
+		return berrors.NotFoundError("unable to confirm that serial %q was ever issued: %s", serialString, err)
+	}
+	x509Cert, err := x509.ParseCertificate(cert.Der)
+	if err != nil {
+		return err
+	}
+
+	shardIdx, err := crlShard(x509Cert)
+	if err != nil {
+		return err
+	}
+
 	_, err = ra.SA.UpdateRevokedCertificate(ctx, &sapb.RevokeCertificateRequest{
 		Serial:   serialString,
 		Reason:   int64(ocsp.KeyCompromise),
 		Date:     timestamppb.New(ra.clk.Now()),
 		Backdate: status.RevokedDate,
 		IssuerID: int64(issuerID),
+		ShardIdx: shardIdx,
 	})
 	if err != nil {
 		return err
diff --git a/ra/ra_test.go b/ra/ra_test.go
@@ -3616,6 +3616,35 @@ func (msar *mockSARevocation) GetCertificateStatus(_ context.Context, req *sapb.
 	return nil, berrors.UnknownSerialError()
 }
 
+func (msar *mockSARevocation) GetCertificate(_ context.Context, req *sapb.Serial, _ ...grpc.CallOption) (*corepb.Certificate, error) {
+	var serialBytes [16]byte
+	_, _ = rand.Read(serialBytes[:])
+	serial := big.NewInt(0).SetBytes(serialBytes[:])
+
+	key, err := ecdsa.GenerateKey(elliptic.P224(), rand.Reader)
+	if err != nil {
+		return nil, err
+	}
+
+	template := &x509.Certificate{
+		SerialNumber:          serial,
+		DNSNames:              []string{"revokememaybe.example.com"},
+		NotBefore:             time.Now(),
+		NotAfter:              time.Now().Add(6 * 24 * time.Hour),
+		IssuingCertificateURL: []string{"http://localhost:4001/acme/issuer-cert/1234"},
+		CRLDistributionPoints: []string{"http://example.com/123.crl"},
+	}
+
+	testCertDER, err := x509.CreateCertificate(rand.Reader, template, template, key.Public(), key)
+	if err != nil {
+		return nil, err
+	}
+
+	return &corepb.Certificate{
+		Der: testCertDER,
+	}, nil
+}
+
 func (msar *mockSARevocation) RevokeCertificate(_ context.Context, req *sapb.RevokeCertificateRequest, _ ...grpc.CallOption) (*emptypb.Empty, error) {
 	if _, present := msar.revoked[req.Serial]; present {
 		return nil, berrors.AlreadyRevokedError("already revoked")
diff --git a/test/integration/revocation_test.go b/test/integration/revocation_test.go
@@ -89,45 +89,69 @@ func getAllCRLs(t *testing.T) map[string][]*x509.RevocationList {
 		// 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)
-			}
-			if resp.StatusCode != http.StatusOK {
-				t.Fatalf("fetching %s: status code %d", crlURL, resp.StatusCode)
-			}
-			body, err := io.ReadAll(resp.Body)
-			if err != nil {
-				t.Fatalf("reading CRL from %s: %s", crlURL, err)
-			}
-			resp.Body.Close()
+			list := getCRL(t, crlURL, issuerCert)
 
-			list, err := x509.ParseRevocationList(body)
-			if err != nil {
-				t.Fatalf("parsing CRL from %s: %s (bytes: %x)", crlURL, err, body)
-			}
+			ret[issuerSKID] = append(ret[issuerSKID], list)
+		}
+	}
+	return ret
+}
 
-			err = list.CheckSignatureFrom(issuerCert)
-			if err != nil {
-				t.Errorf("checking CRL signature on %s from %s: %s",
-					crlURL, issuerCert.Subject, err)
-			}
+// getCRL fetches a CRL, parses it, and checks the signature.
+func getCRL(t *testing.T, crlURL string, issuerCert *x509.Certificate) *x509.RevocationList {
+	resp, err := http.Get(crlURL)
+	if err != nil {
+		t.Fatalf("getting CRL from %s: %s", crlURL, err)
+	}
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("fetching %s: status code %d", crlURL, resp.StatusCode)
+	}
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		t.Fatalf("reading CRL from %s: %s", crlURL, err)
+	}
+	resp.Body.Close()
 
-			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])
-			}
+	list, err := x509.ParseRevocationList(body)
+	if err != nil {
+		t.Fatalf("parsing CRL from %s: %s (bytes: %x)", crlURL, err, body)
+	}
 
-			ret[issuerSKID] = append(ret[issuerSKID], list)
+	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])
+	}
+	return list
+}
+
+func fetchAndCheckRevoked(t *testing.T, cert, issuer *x509.Certificate, expectedReason int) {
+	t.Helper()
+	if len(cert.CRLDistributionPoints) != 1 {
+		t.Errorf("expected certificate to have one CRLDistributionPoints field")
+	}
+	crlURL := cert.CRLDistributionPoints[0]
+	list := getCRL(t, crlURL, issuer)
+	for _, entry := range list.RevokedCertificateEntries {
+		if entry.SerialNumber.Cmp(cert.SerialNumber) == 0 {
+			if entry.ReasonCode != expectedReason {
+				t.Errorf("serial %x found on CRL %s with reason %d, want %d", entry.SerialNumber, crlURL, entry.ReasonCode, expectedReason)
+			}
+			return
 		}
 	}
-	return ret
+	t.Errorf("serial %x not found on CRL %s, expected it to be revoked with reason %d", cert.SerialNumber, crlURL, expectedReason)
 }
 
 func checkRevoked(t *testing.T, revocations map[string][]*x509.RevocationList, cert *x509.Certificate, expectedReason int) {
@@ -421,6 +445,7 @@ func TestReRevocation(t *testing.T) {
 			res, err := authAndIssue(issueClient, certKey, []acme.Identifier{{Type: "dns", Value: random_domain()}}, true, "")
 			test.AssertNotError(t, err, "authAndIssue failed")
 			cert := res.certs[0]
+			issuer := res.certs[1]
 
 			// Initially, the cert should have a Good OCSP response.
 			ocspConfig := ocsp_helper.DefaultConfig.WithExpectStatus(ocsp.Good)
@@ -457,11 +482,10 @@ func TestReRevocation(t *testing.T) {
 			)
 			test.AssertNotError(t, err, "initial revocation should have succeeded")
 
-			// Check the OCSP response for the certificate again. It should now be
+			// Check the CRL for the certificate again. It should now be
 			// revoked.
-			ocspConfig = ocsp_helper.DefaultConfig.WithExpectStatus(ocsp.Revoked).WithExpectReason(tc.reason1)
-			_, err = ocsp_helper.ReqDER(cert.Raw, ocspConfig)
-			test.AssertNotError(t, err, "requesting OCSP for revoked cert")
+			runUpdater(t, path.Join(os.Getenv("BOULDER_CONFIG_DIR"), "crl-updater.json"))
+			fetchAndCheckRevoked(t, cert, issuer, tc.reason1)
 
 			// Set up the account and key that we'll use to *re*-revoke the cert.
 			switch tc.method2 {
@@ -498,7 +522,10 @@ func TestReRevocation(t *testing.T) {
 				// revoked, with the same reason.
 				ocspConfig = ocsp_helper.DefaultConfig.WithExpectStatus(ocsp.Revoked).WithExpectReason(tc.reason1)
 				_, err = ocsp_helper.ReqDER(cert.Raw, ocspConfig)
-				test.AssertNotError(t, err, "requesting OCSP for revoked cert")
+				// Check the CRL for the certificate again. It should still be
+				// revoked, with the same reason.
+				runUpdater(t, path.Join(os.Getenv("BOULDER_CONFIG_DIR"), "crl-updater.json"))
+				fetchAndCheckRevoked(t, cert, issuer, tc.reason1)
 
 			case false:
 				test.AssertNotError(t, err, "second revocation should have succeeded")
@@ -507,7 +534,10 @@ func TestReRevocation(t *testing.T) {
 				// revoked with reason keyCompromise.
 				ocspConfig = ocsp_helper.DefaultConfig.WithExpectStatus(ocsp.Revoked).WithExpectReason(tc.reason2)
 				_, err = ocsp_helper.ReqDER(cert.Raw, ocspConfig)
-				test.AssertNotError(t, err, "requesting OCSP for revoked cert")
+				// Check the CRL for the certificate again. It should now be
+				// revoked with reason keyCompromise.
+				runUpdater(t, path.Join(os.Getenv("BOULDER_CONFIG_DIR"), "crl-updater.json"))
+				fetchAndCheckRevoked(t, cert, issuer, tc.reason2)
 			}
 		})
 	}
