3 Commits

Author	SHA1	Message	Date
shankar0123	6460e43888	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-05-02 13:09:30 +00:00
cowork	388caf9a47	Bundle J follow-up: fix CI staticcheck QF1002 in acme_failure_test.go ... CI on the Bundle J merge (0eacde5) failed at golangci-lint: internal/connector/issuer/acme/acme_failure_test.go:244:3: QF1002: could use tagged switch on r.URL.Path (staticcheck) TestGetRenewalInfo_ARI5xx had a switch{} with case r.URL.Path == ... which staticcheck QF1002 flags as a quick-fix candidate (use tagged switch instead). The function also accumulated dead ts/ts2/ts3 setup from earlier iteration — only ts3 was actually used by the assertion. This commit: - Collapses the 3-server scaffold into a single ts using if/return instead of switch (sidesteps QF1002 entirely + removes ~25 LoC of dead code) - Verifies via 'staticcheck -checks all' (which includes QF*) that the package is clean except for pre-existing ST1000 hits in acme.go/ari.go/dns.go/profile.go (out of scope for this fix) Verification: staticcheck -checks all internal/connector/issuer/acme/... clean (excluding 4 pre-existing ST1000 'missing package comment') go vet ./internal/connector/issuer/acme/... clean go test -short ./internal/connector/issuer/acme/... PASS Coverage unchanged at 55.6% (the test logic was already correct; this commit only removes lint friction).	2026-04-27 16:31:37 +00:00
cowork	a85778d32d	Bundle J (Coverage Audit Closure): ACME failure-mode test batch — C-001 partial-closed ... internal/connector/issuer/acme line coverage 41.8% -> 55.6% (+13.8pp) via internal/connector/issuer/acme/acme_failure_test.go (~700 LoC, 23 tests). Failure modes pinned (all hermetic via httptest.Server, no live ACME): EAB auto-fetch: network-error, malformed-JSON, 5xx, 401, success=false ARI: dir-unreachable, 5xx, 404 (nil/nil), malformed-JSON, empty-suggestedWindow, dir-malformed-falls-to-fallback, invalid-PEM, happy-path with explanationURL Profile-order: directory-discovery-failure on JWS-POST branch empty-profile fast-path delegation fetchNonce: no-URL, no-Replay-Nonce, network-error, happy-path Always-error V1: RevokeCertificate, GenerateCRL, SignOCSPResponse, GetCACertPEM ensureClient propagation: IssueCertificate / RenewCertificate / GetOrderStatus surface 'ACME client init' wrap Challenge handler (HTTP-01): known-token serves, unknown-token 404 presentPersistRecord: no-solver + DNSSolver-fallback Defense-in-depth: error messages do not leak HMAC key bytes Per-function deltas: GetRenewalInfo 11.4% -> 91.4% getARIEndpoint 0.0% -> 82.4% computeARICertID 50.0% -> 100.0% RenewCertificate 0.0% -> 100.0% RevokeCertificate 0.0% -> 80.0% presentPersistRecord 0.0% -> 80.0% fetchNonce 78.6% -> 92.9% ensureClient 79.3% -> 86.2% fetchZeroSSLEAB 80.8% -> 88.5% Engineering: preWiredConnector fixture pre-sets c.client + c.accountKey so ensureClient short-circuits, letting tests exercise post-init paths (ARI/profile/revoke/getOrderStatus) without a full registration mock. Why partial-closed: residual ~30pp gap to >=85% target lives in IssueCertificate (~115 LoC) + solveAuthorizations[HTTP01|DNS01|DNSPersist01] (~280 LoC) + authorizeOrderWithProfile JWS-POST branch — all require a Pebble-style ACME mock (~300-500 LoC infra + ~500 LoC tests). Tracked as follow-on 'Bundle J-extended'. C-001 status open -> partial_closed. Verification: go vet ./internal/connector/issuer/acme/... clean staticcheck ./internal/connector/issuer/acme/... clean go test -short ./internal/connector/issuer/acme/ PASS, 55.6% coverage go test -race ./internal/connector/issuer/acme/ PASS, 0 races Audit deliverables: findings.yaml: C-001 status open -> partial_closed with closure_note gap-backlog.md: closure log + C-001 row updated coverage-matrix.md: ACME 41.8 -> 55.6 closure-plan.md: Bundle J [~] partial-closed CHANGELOG.md: [unreleased] Bundle J entry with per-function table	2026-04-27 16:26:24 +00:00