diff --git a/cmd/server/main.go b/cmd/server/main.go index 08c6abe..dade20e 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -223,6 +223,14 @@ func main() { issuanceMetrics := service.NewIssuanceMetrics(service.DefaultIssuanceBucketBoundaries) issuerRegistry.SetIssuanceMetrics(issuanceMetrics) + // Audit fix #7: wire the cert-version lookup so ACME connectors + // built by Rebuild can recover the leaf-cert DER from a serial- + // only revoke request. The postgres CertificateRepository + // satisfies acme.CertificateLookupRepo via its GetVersionBySerial + // method. Without this, ACME RevokeCertificate falls back to the + // legacy V1 "not supported" error. + issuerRegistry.SetACMECertLookup(certificateRepo) + // Initialize revocation repository revocationRepo := postgres.NewRevocationRepository(db) diff --git a/docs/connectors.md b/docs/connectors.md index 4c713a7..d0d39da 100644 --- a/docs/connectors.md +++ b/docs/connectors.md @@ -261,6 +261,14 @@ The connector is registered in the issuer registry under `iss-acme-staging` and **Note:** ACME-issued certificates rely on the Local CA for CRL/OCSP endpoints if they are stored in certctl's inventory. For issuers with their own public CRL/OCSP infrastructure (e.g., Let's Encrypt), clients should validate against the issuer's endpoints instead. +**Revocation by serial number.** 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: the original PEM was lost, the private key was rotated, the operator clicked "revoke" in the GUI based on a row in the certs list. certctl's ACME `RevokeCertificate(ctx, RevocationRequest{Serial: ...})` looks the serial up in the local cert store (`certificate_versions.pem_chain`), decodes the leaf-cert PEM into DER, and calls the ACME revoke endpoint with `(accountKey, der, reasonCode)` — RFC 8555 §7.6 case 1, "revocation request signed with account key". This works because the same account key issued the cert, so authority is intrinsic. + +The cert version must exist in the local store: this means the cert was issued through certctl, not imported. If `GetVersionBySerial` returns `sql.ErrNoRows`, the connector returns an actionable error pointing at the local-store requirement. Revoke-by-serial is therefore only available for ACME certs that certctl issued. + +Reason codes follow RFC 5280 §5.3.1: nil reason maps to `unspecified` (0), and the connector accepts the canonical camelCase form (`keyCompromise`, `cACompromise`, `affiliationChanged`, `superseded`, `cessationOfOperation`, `certificateHold`, `removeFromCRL`, `privilegeWithdrawn`, `aACompromise`) plus underscore_lower and ALL_CAPS_UNDERSCORE variants. An unknown reason returns an error rather than silently demoting to `unspecified` — operators rely on the reason for compliance reporting (PCI-DSS §3.6, HIPAA §164.312). + +Audit reference: `cowork/issuer-coverage-audit-2026-05-01/RESULTS.md` Top-10 fix #7. + Location: `internal/connector/issuer/acme/acme.go`, `internal/connector/issuer/acme/dns.go` ### Built-in: step-ca (Smallstep Private CA) diff --git a/internal/connector/issuer/acme/acme.go b/internal/connector/issuer/acme/acme.go index 4568cb4..8ce548a 100644 --- a/internal/connector/issuer/acme/acme.go +++ b/internal/connector/issuer/acme/acme.go @@ -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. diff --git a/internal/connector/issuer/acme/acme_failure_test.go b/internal/connector/issuer/acme/acme_failure_test.go index 8bba818..250ae4d 100644 --- a/internal/connector/issuer/acme/acme_failure_test.go +++ b/internal/connector/issuer/acme/acme_failure_test.go @@ -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) } } diff --git a/internal/connector/issuer/acme/acme_revoke_test.go b/internal/connector/issuer/acme/acme_revoke_test.go new file mode 100644 index 0000000..14bcb0f --- /dev/null +++ b/internal/connector/issuer/acme/acme_revoke_test.go @@ -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 +} diff --git a/internal/integration/lifecycle_test.go b/internal/integration/lifecycle_test.go index 9f59b6f..7377d01 100644 --- a/internal/integration/lifecycle_test.go +++ b/internal/integration/lifecycle_test.go @@ -619,6 +619,22 @@ func (m *mockCertificateRepository) GetByIssuerAndSerial(ctx context.Context, is return nil, sql.ErrNoRows } +// GetVersionBySerial mirrors GetByIssuerAndSerial but returns the version +// row (where the PEM lives) — used by the ACME serial-only revoke path. +func (m *mockCertificateRepository) GetVersionBySerial(ctx context.Context, issuerID, serial string) (*domain.CertificateVersion, error) { + for _, cert := range m.certs { + if cert.IssuerID != issuerID { + continue + } + for _, v := range m.versions[cert.ID] { + if v.SerialNumber == serial { + return v, nil + } + } + } + return nil, sql.ErrNoRows +} + type mockJobRepository struct { jobs map[string]*domain.Job } diff --git a/internal/repository/interfaces.go b/internal/repository/interfaces.go index 137b316..96a97d5 100644 --- a/internal/repository/interfaces.go +++ b/internal/repository/interfaces.go @@ -71,6 +71,13 @@ type CertificateRepository interface { // issuer. Returns sql.ErrNoRows when no match exists so callers can // distinguish "unknown cert" from a real repository error. GetByIssuerAndSerial(ctx context.Context, issuerID, serial string) (*domain.ManagedCertificate, error) + // GetVersionBySerial retrieves the certificate_versions row whose + // serial_number matches, scoped to the issuer via a JOIN on + // managed_certificates. Returns the version (PEMChain, NotBefore, + // NotAfter, etc.) — used by the ACME serial-only revoke path to + // recover the leaf-cert DER when the operator no longer has the + // PEM in hand. Returns sql.ErrNoRows when no match exists. + GetVersionBySerial(ctx context.Context, issuerID, serial string) (*domain.CertificateVersion, error) } // RevocationRepository defines operations for managing certificate revocations. diff --git a/internal/repository/postgres/certificate.go b/internal/repository/postgres/certificate.go index 9ed6a88..40477d5 100644 --- a/internal/repository/postgres/certificate.go +++ b/internal/repository/postgres/certificate.go @@ -608,6 +608,47 @@ func (r *CertificateRepository) GetExpiringCertificates(ctx context.Context, bef return certs, nil } +// GetVersionBySerial returns the certificate_versions row whose serial_number +// matches the supplied serial, scoped to the issuer via a JOIN on +// managed_certificates. Returns sql.ErrNoRows when no match exists so callers +// can distinguish "unknown cert" from a real repository error. +// +// Callers needing this method are protocol endpoints that carry the issuer +// in the request path (ACME revoke-by-serial, OCSP, CRL generation); per RFC +// 5280 §5.2.3 serial numbers are unique only within a single issuer, so a +// serial-only lookup would be ambiguous across issuers. +// +// Audit fix #7: ACME revoke-by-serial uses this to recover the leaf cert PEM +// when the operator only has a serial in hand. +func (r *CertificateRepository) GetVersionBySerial(ctx context.Context, issuerID, serial string) (*domain.CertificateVersion, error) { + var v domain.CertificateVersion + var csrPEM sql.NullString + var keyAlgo sql.NullString + var keySize sql.NullInt64 + err := r.db.QueryRowContext(ctx, ` + SELECT cv.id, cv.certificate_id, cv.serial_number, cv.not_before, cv.not_after, + cv.fingerprint_sha256, cv.pem_chain, cv.csr_pem, cv.key_algorithm, cv.key_size, cv.created_at + FROM certificate_versions cv + JOIN managed_certificates mc ON mc.id = cv.certificate_id + WHERE mc.issuer_id = $1 AND cv.serial_number = $2 + ORDER BY cv.created_at DESC + LIMIT 1 + `, issuerID, serial).Scan(&v.ID, &v.CertificateID, &v.SerialNumber, &v.NotBefore, &v.NotAfter, + &v.FingerprintSHA256, &v.PEMChain, &csrPEM, &keyAlgo, &keySize, &v.CreatedAt) + v.CSRPEM = csrPEM.String + v.KeyAlgorithm = keyAlgo.String + v.KeySize = int(keySize.Int64) + + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, sql.ErrNoRows + } + return nil, fmt.Errorf("failed to get certificate version by issuer+serial: %w", err) + } + + return &v, nil +} + // GetLatestVersion returns the most recent certificate version for a certificate. func (r *CertificateRepository) GetLatestVersion(ctx context.Context, certID string) (*domain.CertificateVersion, error) { var v domain.CertificateVersion diff --git a/internal/service/issuer_registry.go b/internal/service/issuer_registry.go index fb1e49f..6d2af76 100644 --- a/internal/service/issuer_registry.go +++ b/internal/service/issuer_registry.go @@ -8,6 +8,7 @@ import ( "sync" "time" + "github.com/shankar0123/certctl/internal/connector/issuer/acme" "github.com/shankar0123/certctl/internal/connector/issuer/local" "github.com/shankar0123/certctl/internal/connector/issuerfactory" "github.com/shankar0123/certctl/internal/crypto" @@ -38,6 +39,14 @@ type IssuerRegistry struct { // the per-issuer-type counter + histogram + failure tables. // Closes the #4 audit-readiness blocker (per-issuer-type metrics). metrics *IssuanceMetrics + + // acmeCertLookup — when set, every freshly-constructed + // *acme.Connector is wired with SetCertificateLookup + SetIssuerID + // so its serial-only revoke path can recover the leaf-cert DER + // from the local cert store. Closes the #7 audit-readiness blocker. + // Nil leaves the legacy "ACME revocation by serial requires + // CertificateLookup wiring" error in place for old wiring paths. + acmeCertLookup acme.CertificateLookupRepo } // LocalIssuerDeps groups the optional dependencies that the local @@ -83,6 +92,24 @@ func (r *IssuerRegistry) SetIssuanceMetrics(m *IssuanceMetrics) { r.metrics = m } +// SetACMECertLookup wires the cert-version lookup repo for every +// *acme.Connector constructed by Rebuild. The lookup is used by the +// serial-only revoke path (RevokeCertificate) to recover the leaf- +// cert DER bytes from the local cert store; without it, ACME +// RevokeCertificate falls back to the legacy V1 "not supported" +// error. Closes the #7 audit-readiness blocker. +// +// Wire on the registry, not per-call: the registry already owns the +// post-factory wiring step (mirrors SetLocalIssuerDeps), and the same +// repo serves every ACME connector regardless of issuer ID — the +// connector scopes its own lookups via the issuer ID injected by +// SetIssuerID inside Rebuild. +func (r *IssuerRegistry) SetACMECertLookup(repo acme.CertificateLookupRepo) { + r.mu.Lock() + defer r.mu.Unlock() + r.acmeCertLookup = repo +} + // Get returns the issuer connector for the given ID and whether it exists. func (r *IssuerRegistry) Get(id string) (IssuerConnector, bool) { r.mu.RLock() @@ -189,6 +216,18 @@ func (r *IssuerRegistry) Rebuild(ctx context.Context, configs []*domain.Issuer, "key_dir", r.localDeps.KeyDir) } + // Audit fix #7: when the cert-version lookup is configured on + // the registry, inject it into every freshly-constructed + // *acme.Connector so its serial-only revoke path can recover + // the leaf-cert DER bytes. SetIssuerID is paired so the lookup + // can scope by issuer per RFC 5280 §5.2.3. + if acmeConn, ok := connector.(*acme.Connector); ok && r.acmeCertLookup != nil { + acmeConn.SetIssuerID(cfg.ID) + acmeConn.SetCertificateLookup(r.acmeCertLookup) + r.logger.Info("ACME issuer wired with cert-version lookup for serial-only revoke", + "id", cfg.ID) + } + adapter := NewIssuerConnectorAdapter(connector) // Wire per-issuer-type metrics (audit fix #4) when SetIssuanceMetrics // was called. The adapter is the IssuerConnector interface; type- diff --git a/internal/service/shortlived_test.go b/internal/service/shortlived_test.go index 7707cfa..93d3678 100644 --- a/internal/service/shortlived_test.go +++ b/internal/service/shortlived_test.go @@ -214,6 +214,10 @@ func (m *mockCertRepoWithGetError) GetByIssuerAndSerial(ctx context.Context, iss return nil, nil } +func (m *mockCertRepoWithGetError) GetVersionBySerial(ctx context.Context, issuerID, serial string) (*domain.CertificateVersion, error) { + return nil, nil +} + func (m *mockCertRepoWithGetError) GetExpiringCertificates(ctx context.Context, before time.Time) ([]*domain.ManagedCertificate, error) { return nil, m.GetExpiringCertificatesErr } diff --git a/internal/service/testutil_test.go b/internal/service/testutil_test.go index d38b6c2..86af28d 100644 --- a/internal/service/testutil_test.go +++ b/internal/service/testutil_test.go @@ -168,6 +168,22 @@ func (m *mockCertRepo) GetByIssuerAndSerial(ctx context.Context, issuerID, seria return nil, sql.ErrNoRows } +// GetVersionBySerial mirrors GetByIssuerAndSerial but returns the version +// row — exists to support the ACME serial-only revoke path tests. +func (m *mockCertRepo) GetVersionBySerial(ctx context.Context, issuerID, serial string) (*domain.CertificateVersion, error) { + for _, cert := range m.Certs { + if cert.IssuerID != issuerID { + continue + } + for _, v := range m.Versions[cert.ID] { + if v.SerialNumber == serial { + return v, nil + } + } + } + return nil, sql.ErrNoRows +} + func (m *mockCertRepo) AddCert(cert *domain.ManagedCertificate) { m.Certs[cert.ID] = cert }