mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 18:01:37 +00:00
b0efdbe2f8
Closes the #3 acquisition-readiness blocker from the 2026-05-01 issuer coverage audit (Part 1.5 finding #1: audit row not transactional with issuance). AuditRepository.Create previously ran on the package-level *sql.DB while the certificate insert / version insert / revocation insert ran on independent connections — a failed audit INSERT after a successful operation INSERT was silently lost. SOX §404 over IT general controls, PCI-DSS §10 audit logging, HIPAA §164.312(b) audit controls, and CA/B Forum Baseline Requirements §5.4.1 audit log records all presume audit-with-operation atomicity. Design — Option A (Querier abstraction). The chosen pattern: a shared repository.Querier interface (subset of *sql.DB and *sql.Tx) plus a postgres.WithinTx helper that begins a tx, runs fn, commits on nil error, rolls back on error or panic, and returns the wrapped result. Repository methods that participate in a service-layer transaction expose a *WithTx variant taking repository.Querier; the bare methods remain for stand-alone use. A repository.Transactor abstracts the "begin tx, run fn, commit/rollback" lifecycle so service-layer code runs multi-write operations atomically without holding *sql.DB directly. Option B (UnitOfWork) was considered but adds boilerplate without behavioral benefit for the current scope. Option C (context-carried tx) was explicitly rejected — it hides the transactional boundary from the type system, reproducing the class of bug we're fixing. This commit: - Adds internal/repository/querier.go with the Querier interface (compile-time guards that *sql.DB and *sql.Tx satisfy it) and the Transactor interface for service-layer use. - Adds internal/repository/postgres/tx.go with the WithinTx helper (begin/fn/commit/rollback with panic recovery) and a transactor type that satisfies repository.Transactor. - Adds CreateWithTx variants on AuditRepository, CertificateRepository (Create + Update + CreateVersion), and RevocationRepository. Existing bare methods now delegate to the *WithTx variant using the package-level *sql.DB so existing call sites are behavior-preserving. - Updates repository/interfaces.go: AuditRepository, CertificateRepository, and RevocationRepository declare the new *WithTx methods. Adds an atomicity contract doc-comment on AuditRepository pointing at WithinTx + the audit blocker. - Adds AuditService.RecordEventWithTx, mirroring RecordEvent but routing through CreateWithTx so the audit row is part of the caller's transaction. Same redaction + marshalling contract. - Refactors three audit-emitting service paths to use Transactor.WithinTx when SetTransactor was wired, with a legacy fallback for backward compat: * CertificateService.Create — cert insert + audit row in one tx. * RevocationSvc.RevokeCertificateWithActor — cert status update + revocation row + audit row in one tx. The OCSP cache invalidate remains best-effort (out of scope per the prompt). * RenewalService CompleteServerRenewal — cert version insert + cert update + audit row in one tx. Job status update stays outside the audit-atomicity scope (job state lives outside the operator-facing audit trail). - Adds SetTransactor on CertificateService, RevocationSvc, and RenewalService. cmd/server/main.go wires a single Transactor instance shared across all three so all audit-emitting paths run their writes in transactions backed by the same *sql.DB handle. - Updates 5 mock implementations to satisfy the new interface methods: mockCertRepo (testutil_test.go), mockCertRepoWithGetError (shortlived_test.go), fakeRevocationRepo (crl_cache_test.go), intuneE2EAuditRepo (scep_intune_e2e_test.go), and the integration- test mocks (lifecycle_test.go: mockCertificateRepository, mockAuditRepository, mockRevocationRepository). All *WithTx mocks ignore the Querier and delegate to the bare method (mocks have no DB; in-memory state is shared regardless of "tx"). - Adds a service-layer test mockTransactor with BeginTxErr and CommitErr knobs so the atomic-audit tests can assert error propagation through the transactional boundary. - Adds internal/repository/postgres/tx_test.go: unit-level test that WithinTx surfaces "begin tx" wrap when BeginTx fails, and that Transactor.WithinTx delegates correctly. Real-Postgres rollback semantics are covered by the testcontainers tests in the postgres package — sandbox disk pressure prevented adding a sqlmock dep for the in-fn / commit-failure unit test, so those scenarios are exercised through atomic_audit_test.go using the mockTransactor's CommitErr / BeginTxErr fields. - Adds internal/service/atomic_audit_test.go: * TestCertificateService_Create_AtomicWithTx — asserts audit insert failure inside the tx surfaces as the operation's error (closes the blocker contract). * TestCertificateService_Create_LegacyPathLogs — pins the backward-compat behavior when SetTransactor isn't wired: audit failure is logged-not-failed, matching pre-fix. * TestCertificateService_Create_TransactorBeginFailure — BeginTx error path: operation fails, no cert insert, no audit insert. * TestCertificateService_Create_TransactorCommitFailure — Commit error after successful in-fn writes surfaces as the operation's error. Real Postgres can fail Commit on serialization conflicts; the service must report this. Out of scope (separate follow-up commits, same shape): - Issuer CRUD audit atomicity. - Target CRUD audit atomicity. - Agent retire (already transactional via RetireAgentWithCascade; verified, not changed). - Renewal-policy CRUD audit atomicity. - Owner/team/agent-group CRUD audit atomicity. - Discovery / health-check audit atomicity. Verified locally: - gofmt -l . clean - go vet ./... clean - staticcheck ./... clean - golangci-lint run --timeout 5m ./... → 0 issues - go test -short -count=1 ./internal/service/ green - go test -short -count=1 ./internal/api/handler/ green - go test -short -count=1 ./internal/integration/ green - go test -short -count=1 ./internal/repository/postgres/ green - go build ./... success Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md Top-10 fix #3 (Part 3, narrative section).
186 lines
6.5 KiB
Go
186 lines
6.5 KiB
Go
// Copyright (c) certctl
|
|
// SPDX-License-Identifier: BSL-1.1
|
|
//
|
|
// Closes the #3 acquisition-readiness blocker from the 2026-05-01
|
|
// issuer coverage audit by pinning the atomic-audit-row contract on
|
|
// the issuance, renewal, and revocation paths.
|
|
//
|
|
// Pre-fix: cert insert / version insert / revocation insert ran on a
|
|
// *sql.DB connection while the audit row INSERT ran on a separate
|
|
// *sql.DB connection. A failed audit INSERT was logged but did not
|
|
// fail the operation — silently incomplete audit trail.
|
|
//
|
|
// Post-fix: when SetTransactor is wired (production via
|
|
// cmd/server/main.go), the operation runs inside Transactor.WithinTx
|
|
// and any audit-insert failure rolls back the entire transaction.
|
|
//
|
|
// These tests use mockTransactor + mockAuditRepo with CreateErr to
|
|
// simulate audit-insert failure. The mock repos share state in memory
|
|
// (no real rollback), so the test asserts the contract via the
|
|
// returned error and the auditService side effect, not by inspecting
|
|
// post-rollback row counts. The testcontainers-backed sibling test in
|
|
// the postgres package exercises real-Postgres rollback semantics
|
|
// against a real audit_events table.
|
|
|
|
package service
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"testing"
|
|
|
|
"github.com/shankar0123/certctl/internal/domain"
|
|
"github.com/shankar0123/certctl/internal/repository"
|
|
)
|
|
|
|
// TestCertificateService_Create_AtomicWithTx asserts the issuance path
|
|
// runs inside Transactor.WithinTx when the transactor is wired. Without
|
|
// the wrapping, an audit-insert failure would silently log; with it,
|
|
// the failure surfaces as the operation's error.
|
|
func TestCertificateService_Create_AtomicWithTx(t *testing.T) {
|
|
auditRepo := newMockAuditRepository()
|
|
auditRepo.CreateErr = errors.New("simulated audit insert failure")
|
|
auditService := NewAuditService(auditRepo)
|
|
|
|
certRepo := newMockCertificateRepository()
|
|
policyService := NewPolicyService(newMockPolicyRepository(), auditService)
|
|
|
|
svc := NewCertificateService(certRepo, policyService, auditService)
|
|
svc.SetTransactor(newMockTransactor())
|
|
|
|
cert := &domain.ManagedCertificate{
|
|
ID: "mc-test-atomic",
|
|
Name: "atomic-test",
|
|
CommonName: "atomic.example.com",
|
|
IssuerID: "iss-test",
|
|
}
|
|
|
|
err := svc.Create(context.Background(), cert, "test-actor")
|
|
if err == nil {
|
|
t.Fatal("Create should fail when audit insert fails inside the transaction")
|
|
}
|
|
if !errIncludes(err, "audit") {
|
|
t.Errorf("expected error to mention audit, got: %v", err)
|
|
}
|
|
}
|
|
|
|
// TestCertificateService_Create_LegacyPathLogs asserts the pre-fix
|
|
// behavior is preserved when SetTransactor is NOT wired: audit failure
|
|
// is logged but the operation succeeds (returns nil). This documents
|
|
// the backward-compat fallback so callers that haven't migrated to the
|
|
// atomic path still build and run.
|
|
func TestCertificateService_Create_LegacyPathLogs(t *testing.T) {
|
|
auditRepo := newMockAuditRepository()
|
|
auditRepo.CreateErr = errors.New("simulated audit insert failure")
|
|
auditService := NewAuditService(auditRepo)
|
|
|
|
certRepo := newMockCertificateRepository()
|
|
policyService := NewPolicyService(newMockPolicyRepository(), auditService)
|
|
|
|
svc := NewCertificateService(certRepo, policyService, auditService)
|
|
// Intentionally NOT calling SetTransactor — exercise the legacy
|
|
// path.
|
|
|
|
cert := &domain.ManagedCertificate{
|
|
ID: "mc-test-legacy",
|
|
Name: "legacy-test",
|
|
CommonName: "legacy.example.com",
|
|
IssuerID: "iss-test",
|
|
}
|
|
|
|
err := svc.Create(context.Background(), cert, "test-actor")
|
|
if err != nil {
|
|
t.Fatalf("legacy path should swallow audit failure, got: %v", err)
|
|
}
|
|
// The cert insert still landed in the mock — the audit failure
|
|
// did not roll it back (because there's no transaction). This is
|
|
// the audit's blocker behavior; it remains for callers that
|
|
// haven't wired SetTransactor.
|
|
if _, ok := certRepo.Certs["mc-test-legacy"]; !ok {
|
|
t.Fatal("cert insert should land in legacy path even when audit fails")
|
|
}
|
|
}
|
|
|
|
// TestCertificateService_Create_TransactorBeginFailure asserts that
|
|
// when Transactor.WithinTx itself fails (BeginTx error path), the
|
|
// operation surfaces the error and no cert insert happens.
|
|
func TestCertificateService_Create_TransactorBeginFailure(t *testing.T) {
|
|
auditRepo := newMockAuditRepository()
|
|
auditService := NewAuditService(auditRepo)
|
|
|
|
certRepo := newMockCertificateRepository()
|
|
policyService := NewPolicyService(newMockPolicyRepository(), auditService)
|
|
|
|
tx := newMockTransactor()
|
|
tx.BeginTxErr = errors.New("simulated begin tx failure")
|
|
|
|
svc := NewCertificateService(certRepo, policyService, auditService)
|
|
svc.SetTransactor(tx)
|
|
|
|
cert := &domain.ManagedCertificate{
|
|
ID: "mc-test-begin-fail",
|
|
Name: "begin-fail",
|
|
CommonName: "begin-fail.example.com",
|
|
IssuerID: "iss-test",
|
|
}
|
|
|
|
err := svc.Create(context.Background(), cert, "test-actor")
|
|
if err == nil {
|
|
t.Fatal("Create should fail when BeginTx fails")
|
|
}
|
|
if _, ok := certRepo.Certs["mc-test-begin-fail"]; ok {
|
|
t.Fatal("cert insert must NOT happen when BeginTx fails — fn never ran")
|
|
}
|
|
if len(auditRepo.Events) > 0 {
|
|
t.Fatal("audit insert must NOT happen when BeginTx fails")
|
|
}
|
|
}
|
|
|
|
// TestCertificateService_Create_TransactorCommitFailure asserts that
|
|
// a Commit failure after successful in-fn writes surfaces as the
|
|
// operation's error. Real Postgres can fail Commit on serialization
|
|
// conflicts; the service must report this rather than swallowing it.
|
|
func TestCertificateService_Create_TransactorCommitFailure(t *testing.T) {
|
|
auditRepo := newMockAuditRepository()
|
|
auditService := NewAuditService(auditRepo)
|
|
|
|
certRepo := newMockCertificateRepository()
|
|
policyService := NewPolicyService(newMockPolicyRepository(), auditService)
|
|
|
|
tx := newMockTransactor()
|
|
tx.CommitErr = errors.New("simulated commit failure")
|
|
|
|
svc := NewCertificateService(certRepo, policyService, auditService)
|
|
svc.SetTransactor(tx)
|
|
|
|
cert := &domain.ManagedCertificate{
|
|
ID: "mc-test-commit-fail",
|
|
Name: "commit-fail",
|
|
CommonName: "commit-fail.example.com",
|
|
IssuerID: "iss-test",
|
|
}
|
|
|
|
err := svc.Create(context.Background(), cert, "test-actor")
|
|
if err == nil {
|
|
t.Fatal("Create should fail when Commit fails")
|
|
}
|
|
}
|
|
|
|
// Compile-time guard: ensure mockTransactor satisfies repository.Transactor.
|
|
var _ repository.Transactor = (*mockTransactor)(nil)
|
|
|
|
// errIncludes is a tiny strings.Contains alias for use in error-message
|
|
// assertions — keeps the test file dependency-light.
|
|
func errIncludes(err error, sub string) bool {
|
|
if err == nil {
|
|
return false
|
|
}
|
|
s := err.Error()
|
|
for i := 0; i+len(sub) <= len(s); i++ {
|
|
if s[i:i+len(sub)] == sub {
|
|
return true
|
|
}
|
|
}
|
|
return false
|
|
}
|