acme: support serial-only revocation via local cert-version lookup

Closes the #7 acquisition-readiness blocker from the 2026-05-01 issuer coverage audit. Pre-fix, ACME RevokeCertificate at acme.go:L519-L529 returned the literal error "ACME revocation by serial not supported in V1; provide certificate DER". RFC 8555 §7.6 genuinely requires the cert DER bytes (not just the serial), but a CLM platform's job is to abstract over that limitation. Operators routinely have only the serial in hand: lost PEM, rotated key, GUI revoke action driven by a row in the certs list. This commit: - Adds CertificateLookupRepo interface at the ACME connector boundary (connector boundary, NOT a service/repository import — the connector accepts whatever satisfies the shape). Production wiring in cmd/server/main.go injects the postgres CertificateRepository; tests inject a fake. - Adds CertificateRepository.GetVersionBySerial(ctx, issuerID, serial) + interface declaration in repository/interfaces.go, returning the certificate_versions row whose SerialNumber matches, scoped to the issuer via JOIN on managed_certificates. Mirrors the existing GetByIssuerAndSerial shape but returns the version (where PEMChain lives). Per RFC 5280 §5.2.3 the issuer scope is required for determinism. - Adds SetCertificateLookup + SetIssuerID setters on *acme.Connector. Mirror the pattern local.Connector already uses for OCSP responder wiring. Both must be wired before serial-only revoke works; unwired state falls back to a more actionable error pointing at the wiring requirement (the historical "not supported" wording is retired). - Rewrites RevokeCertificate end-to-end: lookup → empty-PEM check → pem.Decode → block.Type == "CERTIFICATE" check → ensureClient → golang.org/x/crypto/acme.Client.RevokeCert(ctx, accountKey, der, reasonCode). RFC 8555 §7.6 case 1 (revocation request signed with account key) — the same account key issued the cert, so authority is intrinsic. The not-found path returns an actionable operator- facing error pointing at the local-store requirement. - Adds mapRevocationReason translating RFC 5280 §5.3.1 reason strings (unspecified, keyCompromise, cACompromise, affiliationChanged, superseded, cessationOfOperation, certificateHold, removeFromCRL, privilegeWithdrawn, aACompromise) into golang.org/x/crypto/acme. CRLReasonCode. Accepts canonical camelCase + underscore_lower + ALL_CAPS_UNDERSCORE. Nil reason → 0 (unspecified). Unknown reason errors rather than silently demoting (operators rely on the reason for compliance reporting). - Wiring update in service/issuer_registry.go: SetACMECertLookup setter on the registry; Rebuild type-asserts *acme.Connector and calls SetCertificateLookup + SetIssuerID, mirroring the existing *local.Connector branch. cmd/server/main.go calls issuerRegistry.SetACMECertLookup(certificateRepo) immediately after SetIssuanceMetrics — the postgres repo satisfies the interface via GetVersionBySerial. - Tests: * acme_revoke_test.go (new): TestRevokeCertificate_NoCertLookupWired, TestRevokeCertificate_NoIssuerIDWired, TestRevokeCertificate_LookupReturnsNotFound (operator-facing "may not have been issued through certctl" hint pinned), TestRevokeCertificate_LookupArbitraryError, TestRevokeCertificate_VersionPEMEmpty (corrupt-row guard), TestRevokeCertificate_PEMMalformed_NoBlock, TestRevokeCertificate_PEMMalformed_WrongType (PRIVATE KEY block rejected as not a CERTIFICATE). * TestMapRevocationReason_TableDriven: full RFC 5280 reason set plus camelCase / underscore / ALL-CAPS variants plus nil-reason and unknown-reason cases. * acme_failure_test.go: renamed TestRevokeCertificate_AlwaysError → TestRevokeCertificate_UnwiredCertLookupFallback; the test still exercises the same backward-compat branch but now asserts the new "CertificateLookup wiring" error wording. - Mock-repo updates (3 sites): mockCertificateRepository in internal/integration/lifecycle_test.go, mockCertRepo in internal/service/testutil_test.go, mockCertRepoWithGetError in internal/service/shortlived_test.go each gain a GetVersionBySerial implementation that mirrors the GetByIssuerAndSerial logic but returns the version row. - docs/connectors.md ACME section: new "Revocation by serial number" subsection covering the workflow, the local-store requirement (cert was issued through certctl, not imported), the reason-code mapping with the three accepted spelling variants, and a pointer to the audit reference. Out of scope (intentional, per spec): - Recovering the DER from outside the local cert store (CT logs, CSR + signature reconstruction). If the cert wasn't issued through certctl, revoke-by-serial via certctl isn't possible. - Revocation via the cert's private key (RFC 8555 §7.6 case 2). The account-key path covers all certctl-issued certs because the same account key issued them. - Pebble-backed integration test for the happy path. Pebble integration is the right home for that — the unit tests in this commit pin all failure-mode branches before the network call, and the wiring branch in Rebuild is exercised by the existing TestIssuerRegistryRebuild paths. Verified locally: - gofmt -l . clean - go vet ./... clean - staticcheck ./... clean - go test -short -count=1 across connector, service, repository, integration, api/middleware, api/handler: green Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md Top-10 fix #7.
2026-06-10 01:18:52 +00:00 · 2026-05-02 13:09:30 +00:00
parent 2a384c690e
commit fefa5a5fd7
11 changed files with 560 additions and 15 deletions
@@ -7,9 +7,11 @@ import (
 	"crypto/rand"
 	"crypto/tls"
 	"crypto/x509"
+	"database/sql"
 	"encoding/base64"
 	"encoding/json"
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"io"
 	"log/slog"
@@ -24,6 +26,7 @@ import (
 	"golang.org/x/crypto/acme"

 	"github.com/shankar0123/certctl/internal/connector/issuer"
+	"github.com/shankar0123/certctl/internal/domain"
 )

 // Config represents the ACME issuer connector configuration.
@@ -84,6 +87,28 @@ type Config struct {
 	Insecure bool `json:"insecure,omitempty"`
 }

+// CertificateLookupRepo lets the ACME connector recover a previously-issued
+// certificate's PEM chain from the local cert store given only the serial.
+// RFC 8555 §7.6 requires the certificate DER bytes (not just the serial) on
+// the revoke wire — this interface bridges the gap so an operator who calls
+// RevokeCertificate with just a serial in hand (lost PEM, rotated key, GUI
+// revoke action) gets the same outcome as one who supplies the full DER.
+//
+// Defined at the connector boundary on purpose: the connector doesn't import
+// the service or repository packages — it accepts whatever satisfies this
+// shape. Production wiring in cmd/server/main.go injects the postgres
+// CertificateRepository (which has GetVersionBySerial); tests inject a fake.
+//
+// Audit fix #7.
+type CertificateLookupRepo interface {
+	// GetVersionBySerial returns the certificate version (the row that
+	// holds the PEM chain) whose SerialNumber matches the supplied
+	// serial, scoped to the issuerID. Returns sql.ErrNoRows when no
+	// match exists. Per RFC 5280 §5.2.3 serials are unique only within
+	// a single issuer, so the scope is required.
+	GetVersionBySerial(ctx context.Context, issuerID, serial string) (*domain.CertificateVersion, error)
+}
+
 // Connector implements the issuer.Connector interface for ACME-compatible CAs
 // (Let's Encrypt, Sectigo, ZeroSSL, etc.).
 //
@@ -104,6 +129,35 @@ type Connector struct {

 	// DNS-01 challenge solver (nil if using HTTP-01)
 	dnsSolver DNSSolver
+
+	// issuerID + certLookup are wired by the registry's Rebuild via
+	// SetIssuerID + SetCertificateLookup. When certLookup is nil, the
+	// serial-only revoke path falls back to the legacy "not supported"
+	// error so old wiring paths keep their behaviour. Audit fix #7.
+	issuerID   string
+	certLookup CertificateLookupRepo
+}
+
+// SetIssuerID records the issuer ID so the serial-only revoke path can
+// scope the cert-version lookup correctly. Per RFC 5280 §5.2.3 serial
+// numbers are only unique within a single issuer, so the scope is
+// required for the lookup to be deterministic. Mirrors the existing
+// SetIssuerID setter on local.Connector.
+//
+// Audit fix #7.
+func (c *Connector) SetIssuerID(id string) {
+	c.issuerID = id
+}
+
+// SetCertificateLookup wires the cert-version lookup so the ACME
+// connector can recover the leaf-cert PEM (and thus the DER bytes
+// needed by RFC 8555 §7.6) from a serial-only revoke request. nil
+// means revoke-by-serial is not supported (the historical V1
+// behaviour, preserved for old wiring paths).
+//
+// Audit fix #7.
+func (c *Connector) SetCertificateLookup(repo CertificateLookupRepo) {
+	c.certLookup = repo
 }

 // New creates a new ACME connector with the given configuration and logger.
@@ -515,20 +569,146 @@ func (c *Connector) RenewCertificate(ctx context.Context, request issuer.Renewal
 	})
 }

-// RevokeCertificate revokes a certificate at the ACME CA.
+// RevokeCertificate revokes a certificate at the ACME CA. RFC 8555 §7.6
+// requires the certificate DER bytes (not just the serial) on the revoke
+// wire — but a CLM platform's job is to abstract over that limitation.
+// Operators routinely have only the serial in hand: lost PEM, rotated
+// key, GUI revoke action driven by a row in the certs list.
+//
+// This method recovers the leaf-cert DER by looking the serial up in
+// the local cert-version store (CertificateLookupRepo, wired by the
+// registry's Rebuild), decoding the PEM chain into DER, and calling
+// golang.org/x/crypto/acme.Client.RevokeCert with (accountKey, der,
+// reasonCode). The reason is mapped from the RFC 5280 string in the
+// request via mapRevocationReason; nil reason maps to 0 (unspecified).
+//
+// Audit fix #7. Pre-fix this returned the literal error
+// "ACME revocation by serial not supported in V1; provide certificate
+// DER" which made GUI-driven revoke unusable for ACME-issued certs.
 func (c *Connector) RevokeCertificate(ctx context.Context, request issuer.RevocationRequest) error {
 	c.logger.Info("processing ACME revocation request", "serial", request.Serial)

-	if err := c.ensureClient(ctx); err != nil {
-		return fmt.Errorf("ACME client init: %w", err)
+	if c.certLookup == nil {
+		// Backward-compat fallback. Only fires in test paths or old
+		// wiring where SetCertificateLookup was not called. The audit
+		// mandates the lookup wire as the production path; this is
+		// retained for the test cases that build the connector
+		// directly without the registry.
+		return fmt.Errorf("ACME revocation by serial requires CertificateLookup wiring; call SetCertificateLookup")
 	}

-	// ACME revocation requires the certificate DER, not just the serial.
-	// For now, log a warning. Full revocation requires storing the cert DER
-	// or re-fetching it from the order.
-	c.logger.Warn("ACME revocation requires certificate DER bytes; serial-only revocation not supported in V1",
-		"serial", request.Serial)
-	return fmt.Errorf("ACME revocation by serial not supported in V1; provide certificate DER")
+	if c.issuerID == "" {
+		// Same backward-compat reasoning. The registry calls
+		// SetIssuerID alongside SetCertificateLookup; both are
+		// required for the lookup to be deterministic per RFC 5280
+		// §5.2.3.
+		return fmt.Errorf("ACME revocation by serial requires issuer ID wiring; call SetIssuerID")
+	}
+
+	version, err := c.certLookup.GetVersionBySerial(ctx, c.issuerID, request.Serial)
+	if err != nil {
+		if errors.Is(err, sql.ErrNoRows) {
+			return fmt.Errorf("ACME revoke: no local cert with serial %s for issuer %s (cert may not have been issued through certctl)", request.Serial, c.issuerID)
+		}
+		return fmt.Errorf("ACME revoke: cert version lookup: %w", err)
+	}
+
+	if version == nil || version.PEMChain == "" {
+		return fmt.Errorf("ACME revoke: local cert version row has empty PEM chain (corrupt row?) — serial=%s", request.Serial)
+	}
+
+	// PEMChain is "leaf cert PEM\nchain PEMs..."; we only need the
+	// leaf for the ACME revoke wire. pem.Decode returns the FIRST
+	// block, which is exactly the leaf, then leaves the rest in the
+	// trailing slice (which we discard).
+	block, _ := pem.Decode([]byte(version.PEMChain))
+	if block == nil {
+		return fmt.Errorf("ACME revoke: cert version PEM malformed — no PEM block found in chain (serial=%s)", request.Serial)
+	}
+	if block.Type != "CERTIFICATE" {
+		return fmt.Errorf("ACME revoke: cert version PEM has unexpected block type %q (expected CERTIFICATE, serial=%s)", block.Type, request.Serial)
+	}
+	der := block.Bytes
+
+	if err := c.ensureClient(ctx); err != nil {
+		return fmt.Errorf("ACME revoke: client init: %w", err)
+	}
+
+	reasonCode, err := mapRevocationReason(request.Reason)
+	if err != nil {
+		return fmt.Errorf("ACME revoke: %w", err)
+	}
+
+	// golang.org/x/crypto/acme.Client.RevokeCert authenticates the
+	// revoke with the supplied account key (RFC 8555 §7.6 case 1,
+	// "revocation request signed with account key"). The same account
+	// key issued the cert, so this path covers all certctl-issued
+	// ACME certs. Revocation via the cert's private key is the
+	// alternative auth path (RFC 8555 §7.6 case 2) and is out of
+	// scope here.
+	c.logger.Info("ACME revoke: issuing RevokeCert", "serial", request.Serial, "reason_code", reasonCode)
+	if err := c.client.RevokeCert(ctx, c.accountKey, der, reasonCode); err != nil {
+		return fmt.Errorf("ACME RevokeCert: %w", err)
+	}
+
+	c.logger.Info("ACME certificate revoked", "serial", request.Serial)
+	return nil
+}
+
+// mapRevocationReason translates an RFC 5280 §5.3.1 reason string (as
+// it appears in a RevocationRequest from the certctl service layer)
+// into the integer reason code that
+// golang.org/x/crypto/acme.CRLReasonCode expects. Codes match RFC 5280 §5.3.1: 0 unspecified,
+// 1 keyCompromise, 2 cACompromise, 3 affiliationChanged, 4 superseded,
+// 5 cessationOfOperation, 6 certificateHold, 8 removeFromCRL,
+// 9 privilegeWithdrawn, 10 aACompromise. (7 is reserved.)
+//
+// A nil reason maps to 0 (unspecified) per RFC 5280 §5.3.1's "if the
+// reason code extension is absent the reason is unspecified". An
+// unknown reason string returns an error rather than silently mapping
+// to unspecified — operators rely on the reason for compliance
+// reporting (PCI-DSS / HIPAA) and a silent demotion would obscure a
+// real bug.
+//
+// Accepted forms: the canonical RFC 5280 camelCase ("keyCompromise"),
+// underscore_lower ("key_compromise"), and ALL_CAPS_UNDERSCORE
+// ("KEY_COMPROMISE"). The certctl revocation service emits the
+// camelCase form today, but the more relaxed parsing makes it
+// trivially safe for operators typing reasons via the API.
+//
+// Audit fix #7.
+func mapRevocationReason(reason *string) (acme.CRLReasonCode, error) {
+	if reason == nil || *reason == "" {
+		return acme.CRLReasonUnspecified, nil
+	}
+	// Normalise: lowercase, strip underscores. "keyCompromise",
+	// "key_compromise", "KEY_COMPROMISE" all collapse to
+	// "keycompromise" and match.
+	normalized := strings.ToLower(strings.ReplaceAll(*reason, "_", ""))
+	switch normalized {
+	case "unspecified":
+		return acme.CRLReasonUnspecified, nil
+	case "keycompromise":
+		return acme.CRLReasonKeyCompromise, nil
+	case "cacompromise":
+		return acme.CRLReasonCACompromise, nil
+	case "affiliationchanged":
+		return acme.CRLReasonAffiliationChanged, nil
+	case "superseded":
+		return acme.CRLReasonSuperseded, nil
+	case "cessationofoperation":
+		return acme.CRLReasonCessationOfOperation, nil
+	case "certificatehold":
+		return acme.CRLReasonCertificateHold, nil
+	case "removefromcrl":
+		return acme.CRLReasonRemoveFromCRL, nil
+	case "privilegewithdrawn":
+		return acme.CRLReasonPrivilegeWithdrawn, nil
+	case "aacompromise":
+		return acme.CRLReasonAACompromise, nil
+	default:
+		return 0, fmt.Errorf("unknown revocation reason %q (expected one of: unspecified, keyCompromise, cACompromise, affiliationChanged, superseded, cessationOfOperation, certificateHold, removeFromCRL, privilegeWithdrawn, aACompromise)", *reason)
+	}
 }

 // GetOrderStatus retrieves the current status of an ACME order.
@@ -563,10 +563,17 @@ func TestFetchNonce_HappyPath(t *testing.T) {

 // ---------------------------------------------------------------------------
 // RevokeCertificate / GetCACertPEM / GenerateCRL / SignOCSPResponse —
-// always-error paths
+// fallback / always-error paths
 // ---------------------------------------------------------------------------

-func TestRevokeCertificate_AlwaysError(t *testing.T) {
+// TestRevokeCertificate_UnwiredCertLookupFallback exercises the
+// backward-compat branch in RevokeCertificate that fires when
+// SetCertificateLookup was never called. Audit fix #7 replaced the
+// historical "ACME revocation by serial not supported in V1" error
+// with a more actionable one pointing at the wiring requirement; the
+// production path always wires the lookup, so this branch only fires
+// in tests / old wiring paths.
+func TestRevokeCertificate_UnwiredCertLookupFallback(t *testing.T) {
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Content-Type", "application/json")
 		_, _ = io.WriteString(w, `{"newOrder":"","newAccount":"","newNonce":""}`)
@@ -578,17 +585,19 @@ func TestRevokeCertificate_AlwaysError(t *testing.T) {
 		Email:         "test@example.com",
 		ChallengeType: "http-01",
 	})
+	// Intentionally do NOT call SetCertificateLookup — that's the
+	// behaviour under test.

-	reason := "key compromise"
+	reason := "keyCompromise"
 	err := c.RevokeCertificate(context.Background(), issuer.RevocationRequest{
 		Serial: "ABC123",
 		Reason: &reason,
 	})
 	if err == nil {
-		t.Fatal("expected error from V1 ACME revocation")
+		t.Fatal("expected error when CertificateLookup is unwired")
 	}
-	if !strings.Contains(err.Error(), "not supported") {
-		t.Errorf("error %q should mention 'not supported'", err)
+	if !strings.Contains(err.Error(), "CertificateLookup") {
+		t.Errorf("error %q should reference CertificateLookup wiring", err)
 	}
 }

@@ -0,0 +1,217 @@
+package acme
+
+// Audit fix #7 — serial-only ACME revocation tests.
+//
+// The happy path (issue → revoke-by-serial against a real ACME server)
+// is covered by the pebble integration test in pebble_mock_test.go's
+// follow-up; this file pins the failure-mode branches and the pure
+// mapRevocationReason translation.
+
+import (
+	"context"
+	"database/sql"
+	"errors"
+	"testing"
+
+	"golang.org/x/crypto/acme"
+
+	"github.com/shankar0123/certctl/internal/connector/issuer"
+	"github.com/shankar0123/certctl/internal/domain"
+)
+
+// fakeCertLookup implements CertificateLookupRepo for tests. The two
+// fields control the GetVersionBySerial behavior; tests set them per
+// scenario.
+type fakeCertLookup struct {
+	version *domain.CertificateVersion
+	err     error
+}
+
+func (f *fakeCertLookup) GetVersionBySerial(ctx context.Context, issuerID, serial string) (*domain.CertificateVersion, error) {
+	return f.version, f.err
+}
+
+// newConnectorForRevoke builds an ACME connector pre-wired for a
+// revoke test. The cert-lookup is set to the supplied fake; the
+// issuer ID is "iss-test" unless cleared by the caller.
+func newConnectorForRevoke(t *testing.T, lookup CertificateLookupRepo) *Connector {
+	t.Helper()
+	c := New(&Config{
+		DirectoryURL: "https://acme.example.test/dir",
+		Email:        "ops@example.com",
+	}, testLogger())
+	c.SetIssuerID("iss-test")
+	c.SetCertificateLookup(lookup)
+	return c
+}
+
+func TestRevokeCertificate_NoCertLookupWired(t *testing.T) {
+	c := New(&Config{DirectoryURL: "https://x.test/dir", Email: "a@b"}, testLogger())
+	// Intentionally NOT calling SetCertificateLookup — exercises the
+	// backward-compat fallback for tests/old wiring paths.
+	err := c.RevokeCertificate(context.Background(), issuer.RevocationRequest{Serial: "AB:CD"})
+	if err == nil {
+		t.Fatal("expected error when CertificateLookup is unwired")
+	}
+	if !contains(err.Error(), "CertificateLookup") {
+		t.Errorf("expected wiring-error message, got: %v", err)
+	}
+}
+
+func TestRevokeCertificate_NoIssuerIDWired(t *testing.T) {
+	c := New(&Config{DirectoryURL: "https://x.test/dir", Email: "a@b"}, testLogger())
+	c.SetCertificateLookup(&fakeCertLookup{})
+	// Skip SetIssuerID — exercises the second backward-compat guard.
+	err := c.RevokeCertificate(context.Background(), issuer.RevocationRequest{Serial: "AB:CD"})
+	if err == nil {
+		t.Fatal("expected error when issuer ID is unwired")
+	}
+	if !contains(err.Error(), "issuer ID") {
+		t.Errorf("expected issuer-ID-error message, got: %v", err)
+	}
+}
+
+func TestRevokeCertificate_LookupReturnsNotFound(t *testing.T) {
+	c := newConnectorForRevoke(t, &fakeCertLookup{err: sql.ErrNoRows})
+	err := c.RevokeCertificate(context.Background(), issuer.RevocationRequest{Serial: "DEAD:BEEF"})
+	if err == nil {
+		t.Fatal("expected error when lookup returns ErrNoRows")
+	}
+	// Operator-facing error must mention serial + suggest the cert
+	// wasn't issued through certctl.
+	if !contains(err.Error(), "DEAD:BEEF") {
+		t.Errorf("expected error to include serial, got: %v", err)
+	}
+	if !contains(err.Error(), "may not have been issued through certctl") {
+		t.Errorf("expected operator-facing hint about cert not in local store, got: %v", err)
+	}
+}
+
+func TestRevokeCertificate_LookupArbitraryError(t *testing.T) {
+	c := newConnectorForRevoke(t, &fakeCertLookup{err: errors.New("connection refused")})
+	err := c.RevokeCertificate(context.Background(), issuer.RevocationRequest{Serial: "AB:CD"})
+	if err == nil {
+		t.Fatal("expected error to propagate")
+	}
+	if !contains(err.Error(), "connection refused") {
+		t.Errorf("expected wrapped repo error, got: %v", err)
+	}
+	if !contains(err.Error(), "lookup") {
+		t.Errorf("expected 'lookup' framing in error, got: %v", err)
+	}
+}
+
+func TestRevokeCertificate_VersionPEMEmpty(t *testing.T) {
+	c := newConnectorForRevoke(t, &fakeCertLookup{
+		version: &domain.CertificateVersion{
+			SerialNumber: "AB:CD",
+			PEMChain:     "",
+		},
+	})
+	err := c.RevokeCertificate(context.Background(), issuer.RevocationRequest{Serial: "AB:CD"})
+	if err == nil {
+		t.Fatal("expected error when version row has empty PEMChain")
+	}
+	if !contains(err.Error(), "empty PEM chain") {
+		t.Errorf("expected empty-PEM error, got: %v", err)
+	}
+}
+
+func TestRevokeCertificate_PEMMalformed_NoBlock(t *testing.T) {
+	c := newConnectorForRevoke(t, &fakeCertLookup{
+		version: &domain.CertificateVersion{
+			SerialNumber: "AB:CD",
+			PEMChain:     "this is not a PEM block at all",
+		},
+	})
+	err := c.RevokeCertificate(context.Background(), issuer.RevocationRequest{Serial: "AB:CD"})
+	if err == nil {
+		t.Fatal("expected error when PEM chain has no decodable block")
+	}
+	if !contains(err.Error(), "no PEM block") {
+		t.Errorf("expected no-PEM-block error, got: %v", err)
+	}
+}
+
+func TestRevokeCertificate_PEMMalformed_WrongType(t *testing.T) {
+	// A valid PEM block, but type is PRIVATE KEY — must be rejected
+	// as "expected CERTIFICATE".
+	pemPrivKey := "-----BEGIN PRIVATE KEY-----\nMIIBVgIBADANBgkqhkiG9w0BAQE=\n-----END PRIVATE KEY-----\n"
+	c := newConnectorForRevoke(t, &fakeCertLookup{
+		version: &domain.CertificateVersion{
+			SerialNumber: "AB:CD",
+			PEMChain:     pemPrivKey,
+		},
+	})
+	err := c.RevokeCertificate(context.Background(), issuer.RevocationRequest{Serial: "AB:CD"})
+	if err == nil {
+		t.Fatal("expected error when PEM block type is not CERTIFICATE")
+	}
+	if !contains(err.Error(), "PRIVATE KEY") {
+		t.Errorf("expected error to mention the actual block type, got: %v", err)
+	}
+}
+
+// TestMapRevocationReason_TableDriven covers the full RFC 5280 §5.3.1
+// reason set plus the canonical / underscore / ALL-CAPS spelling
+// variants and the unknown-reason and nil-reason behaviors.
+func TestMapRevocationReason_TableDriven(t *testing.T) {
+	str := func(s string) *string { return &s }
+	cases := []struct {
+		name    string
+		reason  *string
+		want    acme.CRLReasonCode
+		wantErr bool
+	}{
+		// Nil → unspecified. RFC 5280 §5.3.1: "if the reason code
+		// extension is absent the reason is unspecified".
+		{"nil_reason_unspecified", nil, acme.CRLReasonUnspecified, false},
+		{"empty_string_unspecified", str(""), acme.CRLReasonUnspecified, false},
+
+		// Canonical RFC 5280 camelCase.
+		{"camel_unspecified", str("unspecified"), acme.CRLReasonUnspecified, false},
+		{"camel_keyCompromise", str("keyCompromise"), acme.CRLReasonKeyCompromise, false},
+		{"camel_cACompromise", str("cACompromise"), acme.CRLReasonCACompromise, false},
+		{"camel_affiliationChanged", str("affiliationChanged"), acme.CRLReasonAffiliationChanged, false},
+		{"camel_superseded", str("superseded"), acme.CRLReasonSuperseded, false},
+		{"camel_cessationOfOperation", str("cessationOfOperation"), acme.CRLReasonCessationOfOperation, false},
+		{"camel_certificateHold", str("certificateHold"), acme.CRLReasonCertificateHold, false},
+		{"camel_removeFromCRL", str("removeFromCRL"), acme.CRLReasonRemoveFromCRL, false},
+		{"camel_privilegeWithdrawn", str("privilegeWithdrawn"), acme.CRLReasonPrivilegeWithdrawn, false},
+		{"camel_aACompromise", str("aACompromise"), acme.CRLReasonAACompromise, false},
+
+		// underscore_lower.
+		{"underscore_key_compromise", str("key_compromise"), acme.CRLReasonKeyCompromise, false},
+		{"underscore_ca_compromise", str("ca_compromise"), acme.CRLReasonCACompromise, false},
+
+		// ALL_CAPS_UNDERSCORE.
+		{"caps_KEY_COMPROMISE", str("KEY_COMPROMISE"), acme.CRLReasonKeyCompromise, false},
+		{"caps_REMOVE_FROM_CRL", str("REMOVE_FROM_CRL"), acme.CRLReasonRemoveFromCRL, false},
+
+		// Unknown — must error rather than silently demote.
+		{"unknown_reason_errors", str("totallyMadeUp"), 0, true},
+		{"reserved_code_7_unhandled", str("reserved"), 0, true}, // Reserved per RFC 5280, no canonical name.
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := mapRevocationReason(tc.reason)
+			if (err != nil) != tc.wantErr {
+				t.Fatalf("err=%v wantErr=%v", err, tc.wantErr)
+			}
+			if !tc.wantErr && got != tc.want {
+				t.Errorf("got code %d, want %d", got, tc.want)
+			}
+		})
+	}
+}
+
+// contains is a tiny helper to avoid pulling strings into every test.
+func contains(haystack, needle string) bool {
+	for i := 0; i+len(needle) <= len(haystack); i++ {
+		if haystack[i:i+len(needle)] == needle {
+			return true
+		}
+	}
+	return false
+}