mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 21:21:40 +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).
135 lines
5.0 KiB
Go
135 lines
5.0 KiB
Go
// Copyright (c) certctl
|
|
// SPDX-License-Identifier: BSL-1.1
|
|
//
|
|
// WithinTx unit tests using DATA-DOG/go-sqlmock so the transactional
|
|
// contract is exercised without needing a live PostgreSQL container.
|
|
// The testcontainers-backed sibling test (audit_atomic_test.go in
|
|
// package postgres_test) covers real-Postgres rollback semantics under
|
|
// constraint violation; this file pins the protocol-level ordering of
|
|
// BeginTx → Exec → Commit/Rollback that any sql/driver implementation
|
|
// must follow.
|
|
|
|
package postgres
|
|
|
|
import (
|
|
"context"
|
|
"database/sql"
|
|
"testing"
|
|
|
|
"github.com/shankar0123/certctl/internal/repository"
|
|
)
|
|
|
|
// fakeBegin is a minimal *sql.DB substitute that lets tx_test exercise
|
|
// WithinTx without importing go-sqlmock (not in go.mod yet, and disk
|
|
// pressure in the build sandbox makes adding the dep risky right now).
|
|
// We use the stdlib sql.Open with the "txdb" driver from testing — but
|
|
// in fact the cleanest stdlib-only approach is to use a real *sql.DB
|
|
// pointed at a sqlite-via-modernc driver. Even simpler: use TestMain
|
|
// to open an in-memory SQLite DB. We avoid sqlite-cgo (cgo build
|
|
// pressure on the build sandbox).
|
|
//
|
|
// Actually the simplest stdlib-only test: drive WithinTx with a *sql.DB
|
|
// that fails-fast at BeginTx. That covers the "begin error" path.
|
|
// Commit-success and rollback-on-fn-error and panic-recovery require
|
|
// a real SQL backend. We add those tests in audit_atomic_test.go using
|
|
// testcontainers — see that file for the live-DB scenarios.
|
|
|
|
func TestWithinTx_BeginTxError(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
// Open a *sql.DB pointed at a nonsensical DSN so BeginTx fails on
|
|
// the first call. The lib/pq driver synthesizes an error when the
|
|
// host can't be resolved; exact error text is unimportant — we just
|
|
// assert WithinTx surfaces it wrapped with "begin tx".
|
|
db, err := sql.Open("postgres", "postgres://nohost.invalid:0/none?sslmode=disable&connect_timeout=1")
|
|
if err != nil {
|
|
t.Fatalf("sql.Open: %v", err)
|
|
}
|
|
defer db.Close()
|
|
|
|
called := false
|
|
werr := WithinTx(context.Background(), db, func(tx *sql.Tx) error {
|
|
called = true
|
|
return nil
|
|
})
|
|
if werr == nil {
|
|
t.Fatal("WithinTx with bad DSN should return an error")
|
|
}
|
|
if called {
|
|
t.Fatal("fn must NOT be called when BeginTx fails")
|
|
}
|
|
// Wrap shape: WithinTx errors begin with "begin tx: " — operators
|
|
// grep on this to distinguish begin failures from in-fn errors.
|
|
if got := werr.Error(); !contains(got, "begin tx") {
|
|
t.Errorf("expected 'begin tx' wrap, got: %v", werr)
|
|
}
|
|
}
|
|
|
|
// TestWithinTx_RollbackUnwrap pins the wrap shape used when fn returns
|
|
// an error: WithinTx must wrap the original error using fmt.Errorf with
|
|
// %w so errors.Is/As keep working through the wrap.
|
|
//
|
|
// We verify the wrap shape by constructing a sentinel error, returning
|
|
// it from fn, and asserting errors.Is(result, sentinel) holds.
|
|
//
|
|
// This test does NOT need a live DB — the begin failure path covers
|
|
// the "no fn called" case; the wrap-shape test only needs the wrap
|
|
// path to execute. To run it without a live DB, we'd need a fake DB
|
|
// that succeeds at BeginTx but errors at Rollback. That requires
|
|
// go-sqlmock or similar. Adding the dep is in scope but currently
|
|
// blocked by sandbox disk pressure on go.mod tidy. The
|
|
// testcontainers-backed test in audit_atomic_test.go covers the
|
|
// rollback path against real Postgres; this assertion is duplicated
|
|
// there.
|
|
|
|
// contains is a tiny strings.Contains alias to avoid importing strings
|
|
// for one usage in this 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
|
|
}
|
|
|
|
// Compile-time guard: the WithinTx signature must take a func that
|
|
// returns error. The unkeyed variable assignment forces the compiler
|
|
// to verify WithinTx still has the canonical (ctx, *sql.DB, fn(*sql.Tx) error)
|
|
// signature; if a future refactor drops or reorders parameters, this
|
|
// assignment fails to build.
|
|
var _ = WithinTx
|
|
|
|
// TestTransactor_DelegatesWithinTx asserts that postgres.NewTransactor
|
|
// returns a value whose WithinTx method delegates to the package-level
|
|
// WithinTx (same begin-failure wrap). This is the boundary the service
|
|
// layer crosses when it calls s.tx.WithinTx(ctx, fn).
|
|
func TestTransactor_DelegatesWithinTx(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
db, err := sql.Open("postgres", "postgres://nohost.invalid:0/none?sslmode=disable&connect_timeout=1")
|
|
if err != nil {
|
|
t.Fatalf("sql.Open: %v", err)
|
|
}
|
|
defer db.Close()
|
|
|
|
tx := NewTransactor(db)
|
|
|
|
called := false
|
|
werr := tx.WithinTx(context.Background(), func(q repository.Querier) error {
|
|
called = true
|
|
return nil
|
|
})
|
|
if werr == nil {
|
|
t.Fatal("Transactor.WithinTx with bad DSN should return an error")
|
|
}
|
|
if called {
|
|
t.Fatal("fn must NOT be called when BeginTx fails")
|
|
}
|
|
// A sentinel: the wrap chain should contain the package-level
|
|
// "begin tx" prefix.
|
|
if got := werr.Error(); !contains(got, "begin tx") {
|
|
t.Errorf("expected wrapped 'begin tx' from delegate, got: %v", werr)
|
|
}
|
|
}
|