mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-11 23:58:57 +00:00
repo,service: introduce WithinTx and atomic audit rows for issue/renew/revoke
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).
This commit is contained in:
@@ -0,0 +1,185 @@
|
||||
// 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
|
||||
}
|
||||
@@ -58,6 +58,41 @@ func (s *AuditService) RecordEvent(ctx context.Context, actor string, actorType
|
||||
return nil
|
||||
}
|
||||
|
||||
// RecordEventWithTx records an audit event using the supplied repository.Querier.
|
||||
//
|
||||
// Pass *sql.Tx (typically obtained from postgres.WithinTx) to participate in
|
||||
// a caller's transaction so the audit row is atomic with the operation that
|
||||
// triggered it. Closes the #3 acquisition-readiness blocker from the
|
||||
// 2026-05-01 issuer coverage audit (audit row not transactional with the
|
||||
// operation it audits).
|
||||
//
|
||||
// Same redaction + marshalling contract as RecordEvent; only the database
|
||||
// handle changes.
|
||||
func (s *AuditService) RecordEventWithTx(ctx context.Context, q repository.Querier, actor string, actorType domain.ActorType, action string, resourceType string, resourceID string, details map[string]interface{}) error {
|
||||
redacted := RedactDetailsForAudit(details)
|
||||
detailsJSON, err := json.Marshal(redacted)
|
||||
if err != nil {
|
||||
detailsJSON = []byte("{}")
|
||||
}
|
||||
|
||||
event := &domain.AuditEvent{
|
||||
ID: generateID("audit"),
|
||||
Timestamp: time.Now(),
|
||||
Actor: actor,
|
||||
ActorType: actorType,
|
||||
Action: action,
|
||||
ResourceType: resourceType,
|
||||
ResourceID: resourceID,
|
||||
Details: json.RawMessage(detailsJSON),
|
||||
}
|
||||
|
||||
if err := s.auditRepo.CreateWithTx(ctx, q, event); err != nil {
|
||||
return fmt.Errorf("failed to record audit event: %w", err)
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// List returns audit events matching filter criteria.
|
||||
func (s *AuditService) List(ctx context.Context, filter *repository.AuditFilter) ([]*domain.AuditEvent, error) {
|
||||
events, err := s.auditRepo.List(ctx, filter)
|
||||
|
||||
@@ -19,6 +19,13 @@ type CertificateService struct {
|
||||
auditService *AuditService
|
||||
revSvc *RevocationSvc
|
||||
caSvc *CAOperationsSvc
|
||||
// tx, when set, wraps the issuance write (cert insert + audit row)
|
||||
// in a single transaction so the audit row cannot be silently lost
|
||||
// after a successful cert insert. Closes the #3 audit-readiness
|
||||
// blocker (atomic audit rows). Optional via SetTransactor — when
|
||||
// nil, Create falls back to the legacy non-transactional path
|
||||
// (cert.Create + best-effort RecordEvent) for backward compatibility.
|
||||
tx repository.Transactor
|
||||
// crlCacheSvc, when set, makes GenerateDERCRL serve from the
|
||||
// pre-generated cache instead of regenerating per request. Bundle
|
||||
// CRL/OCSP-Responder Phase 4. Optional; when nil GenerateDERCRL
|
||||
@@ -40,6 +47,16 @@ func NewCertificateService(
|
||||
}
|
||||
}
|
||||
|
||||
// SetTransactor wires a Transactor for atomic issuance (cert insert +
|
||||
// audit row) and atomic revocation (cert update + revocation row + audit
|
||||
// row). Closes the #3 acquisition-readiness blocker from the 2026-05-01
|
||||
// issuer coverage audit. Optional — when nil, Create falls back to the
|
||||
// legacy non-transactional path for backward compat with callers that
|
||||
// haven't been updated.
|
||||
func (s *CertificateService) SetTransactor(tx repository.Transactor) {
|
||||
s.tx = tx
|
||||
}
|
||||
|
||||
// SetRevocationSvc sets the revocation service.
|
||||
func (s *CertificateService) SetRevocationSvc(svc *RevocationSvc) {
|
||||
s.revSvc = svc
|
||||
@@ -133,19 +150,37 @@ func (s *CertificateService) Create(ctx context.Context, cert *domain.ManagedCer
|
||||
}
|
||||
}
|
||||
|
||||
// Store certificate
|
||||
auditDetails := map[string]interface{}{"common_name": cert.CommonName}
|
||||
|
||||
// Atomic path (production): cert insert + audit row in a single
|
||||
// transaction. Closes the #3 audit-readiness blocker — if the audit
|
||||
// insert fails after the cert insert, the cert insert rolls back so
|
||||
// the operator sees the failure and the audit trail is never silently
|
||||
// incomplete.
|
||||
if s.tx != nil {
|
||||
return s.tx.WithinTx(ctx, func(q repository.Querier) error {
|
||||
if err := s.certRepo.CreateWithTx(ctx, q, cert); err != nil {
|
||||
return fmt.Errorf("failed to create certificate: %w", err)
|
||||
}
|
||||
if err := s.auditService.RecordEventWithTx(ctx, q, actor, domain.ActorTypeUser,
|
||||
"certificate_created", "certificate", cert.ID, auditDetails); err != nil {
|
||||
return fmt.Errorf("failed to record audit event: %w", err)
|
||||
}
|
||||
return nil
|
||||
})
|
||||
}
|
||||
|
||||
// Legacy non-transactional path — kept for callers that haven't
|
||||
// wired SetTransactor yet. Fails open on audit-insert failure (logs
|
||||
// and returns success), which is the pre-fix behavior; do not
|
||||
// rely on this path for compliance-relevant audit trails.
|
||||
if err := s.certRepo.Create(ctx, cert); err != nil {
|
||||
return fmt.Errorf("failed to create certificate: %w", err)
|
||||
}
|
||||
|
||||
// Record audit event
|
||||
if err := s.auditService.RecordEvent(ctx, actor, domain.ActorTypeUser,
|
||||
"certificate_created", "certificate", cert.ID,
|
||||
map[string]interface{}{"common_name": cert.CommonName}); err != nil {
|
||||
// Log but don't fail the operation
|
||||
"certificate_created", "certificate", cert.ID, auditDetails); err != nil {
|
||||
slog.Error("failed to record audit event", "error", err)
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
|
||||
@@ -12,6 +12,7 @@ import (
|
||||
"github.com/shankar0123/certctl/internal/connector/issuer"
|
||||
localissuer "github.com/shankar0123/certctl/internal/connector/issuer/local"
|
||||
"github.com/shankar0123/certctl/internal/domain"
|
||||
"github.com/shankar0123/certctl/internal/repository"
|
||||
"github.com/shankar0123/certctl/internal/service"
|
||||
)
|
||||
|
||||
@@ -88,6 +89,9 @@ type fakeRevocationRepo struct{}
|
||||
func (fakeRevocationRepo) Create(context.Context, *domain.CertificateRevocation) error {
|
||||
return nil
|
||||
}
|
||||
func (fakeRevocationRepo) CreateWithTx(context.Context, repository.Querier, *domain.CertificateRevocation) error {
|
||||
return nil
|
||||
}
|
||||
func (fakeRevocationRepo) GetByIssuerAndSerial(context.Context, string, string) (*domain.CertificateRevocation, error) {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
+56
-21
@@ -31,6 +31,10 @@ type RenewalService struct {
|
||||
notificationSvc *NotificationService
|
||||
issuerRegistry *IssuerRegistry
|
||||
keygenMode string // "agent" (default) or "server" (demo only)
|
||||
// tx — when set, wraps the cert version insert + cert update + audit
|
||||
// row in a single transaction. Closes the #3 audit-readiness blocker
|
||||
// for the renewal path. Optional via SetTransactor.
|
||||
tx repository.Transactor
|
||||
}
|
||||
|
||||
// SetTargetRepo sets the target repository for resolving agent_id on deployment jobs.
|
||||
@@ -38,6 +42,14 @@ func (s *RenewalService) SetTargetRepo(repo repository.TargetRepository) {
|
||||
s.targetRepo = repo
|
||||
}
|
||||
|
||||
// SetTransactor wires a Transactor for atomic renewal completion (cert
|
||||
// version insert + cert update + audit row in a single transaction).
|
||||
// Closes the #3 audit-readiness blocker for the renewal path. Optional
|
||||
// — nil reverts to legacy non-transactional behavior.
|
||||
func (s *RenewalService) SetTransactor(tx repository.Transactor) {
|
||||
s.tx = tx
|
||||
}
|
||||
|
||||
// IssuerConnector defines the service-layer interface for interacting with certificate issuers.
|
||||
// This is distinct from the connector-layer issuer.Connector interface to maintain dependency
|
||||
// inversion. Use IssuerConnectorAdapter to bridge between the two.
|
||||
@@ -508,23 +520,58 @@ func (s *RenewalService) processRenewalServerKeygen(ctx context.Context, job *do
|
||||
CreatedAt: time.Now(),
|
||||
}
|
||||
|
||||
if err := s.certRepo.CreateVersion(ctx, version); err != nil {
|
||||
s.failJob(ctx, job, fmt.Sprintf("version creation failed: %v", err))
|
||||
return fmt.Errorf("failed to create certificate version: %w", err)
|
||||
}
|
||||
|
||||
// Update certificate status and expiry
|
||||
cert.Status = domain.CertificateStatusActive
|
||||
cert.ExpiresAt = result.NotAfter
|
||||
now := time.Now()
|
||||
cert.LastRenewalAt = &now
|
||||
cert.UpdatedAt = now
|
||||
if err := s.certRepo.Update(ctx, cert); err != nil {
|
||||
s.failJob(ctx, job, fmt.Sprintf("cert update failed: %v", err))
|
||||
return fmt.Errorf("failed to update certificate: %w", err)
|
||||
|
||||
auditDetails := map[string]interface{}{
|
||||
"job_id": job.ID,
|
||||
"serial": result.Serial,
|
||||
"not_after": result.NotAfter,
|
||||
"keygen_mode": "server",
|
||||
}
|
||||
|
||||
// Mark renewal job as completed
|
||||
// Atomic three-write path (when SetTransactor was wired): version
|
||||
// insert + cert update + audit row in a single transaction. Closes
|
||||
// the #3 audit-readiness blocker for the renewal path.
|
||||
if s.tx != nil {
|
||||
if err := s.tx.WithinTx(ctx, func(q repository.Querier) error {
|
||||
if err := s.certRepo.CreateVersionWithTx(ctx, q, version); err != nil {
|
||||
return fmt.Errorf("failed to create certificate version: %w", err)
|
||||
}
|
||||
if err := s.certRepo.UpdateWithTx(ctx, q, cert); err != nil {
|
||||
return fmt.Errorf("failed to update certificate: %w", err)
|
||||
}
|
||||
if err := s.auditService.RecordEventWithTx(ctx, q, "system", domain.ActorTypeSystem,
|
||||
"renewal_job_completed", "certificate", job.CertificateID, auditDetails); err != nil {
|
||||
return fmt.Errorf("failed to record audit event: %w", err)
|
||||
}
|
||||
return nil
|
||||
}); err != nil {
|
||||
s.failJob(ctx, job, err.Error())
|
||||
return err
|
||||
}
|
||||
} else {
|
||||
// Legacy non-transactional path — pre-fix behavior.
|
||||
if err := s.certRepo.CreateVersion(ctx, version); err != nil {
|
||||
s.failJob(ctx, job, fmt.Sprintf("version creation failed: %v", err))
|
||||
return fmt.Errorf("failed to create certificate version: %w", err)
|
||||
}
|
||||
if err := s.certRepo.Update(ctx, cert); err != nil {
|
||||
s.failJob(ctx, job, fmt.Sprintf("cert update failed: %v", err))
|
||||
return fmt.Errorf("failed to update certificate: %w", err)
|
||||
}
|
||||
if auditErr := s.auditService.RecordEvent(ctx, "system", domain.ActorTypeSystem,
|
||||
"renewal_job_completed", "certificate", job.CertificateID, auditDetails); auditErr != nil {
|
||||
slog.Error("failed to record audit event", "error", auditErr)
|
||||
}
|
||||
}
|
||||
|
||||
// Mark renewal job as completed (independent of the cert/audit
|
||||
// transaction — job state lives outside the audit-atomicity scope).
|
||||
if err := s.jobRepo.UpdateStatus(ctx, job.ID, domain.JobStatusCompleted, ""); err != nil {
|
||||
return fmt.Errorf("failed to update job status: %w", err)
|
||||
}
|
||||
@@ -537,18 +584,6 @@ func (s *RenewalService) processRenewalServerKeygen(ctx context.Context, job *do
|
||||
slog.Error("failed to send renewal notification", "error", err)
|
||||
}
|
||||
|
||||
// Record audit event
|
||||
if auditErr := s.auditService.RecordEvent(ctx, "system", domain.ActorTypeSystem,
|
||||
"renewal_job_completed", "certificate", job.CertificateID,
|
||||
map[string]interface{}{
|
||||
"job_id": job.ID,
|
||||
"serial": result.Serial,
|
||||
"not_after": result.NotAfter,
|
||||
"keygen_mode": "server",
|
||||
}); auditErr != nil {
|
||||
slog.Error("failed to record audit event", "error", auditErr)
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
|
||||
@@ -18,6 +18,13 @@ type RevocationSvc struct {
|
||||
auditService *AuditService
|
||||
notificationSvc *NotificationService
|
||||
issuerRegistry *IssuerRegistry
|
||||
// tx — when set, wraps the cert status update + revocation row
|
||||
// insert + audit row in a single transaction. Closes the #3 audit-
|
||||
// readiness blocker for the revocation path. Optional via
|
||||
// SetTransactor; nil means legacy non-transactional behavior
|
||||
// (cert.Update committed independently from revocation row +
|
||||
// audit, with revocation insert + audit logged-but-not-failed).
|
||||
tx repository.Transactor
|
||||
// ocspCacheInvalidator — production hardening II Phase 2 load-
|
||||
// bearing security wire. After a successful revocation, the
|
||||
// service MUST invalidate the OCSP response cache for this
|
||||
@@ -26,6 +33,14 @@ type RevocationSvc struct {
|
||||
ocspCacheInvalidator OCSPCacheInvalidator
|
||||
}
|
||||
|
||||
// SetTransactor wires a Transactor for atomic revocation (cert update
|
||||
// + revocation row + audit row in a single transaction). Closes the
|
||||
// #3 audit-readiness blocker for the revocation path. Optional —
|
||||
// nil reverts to the legacy non-transactional behavior.
|
||||
func (s *RevocationSvc) SetTransactor(tx repository.Transactor) {
|
||||
s.tx = tx
|
||||
}
|
||||
|
||||
// OCSPCacheInvalidator is the minimum surface RevocationSvc needs
|
||||
// from the OCSP cache. The cache service implements this interface;
|
||||
// the indirection keeps RevocationSvc from depending on the cache
|
||||
@@ -100,31 +115,73 @@ func (s *RevocationSvc) RevokeCertificateWithActor(ctx context.Context, certID s
|
||||
return fmt.Errorf("failed to get certificate version: %w", err)
|
||||
}
|
||||
|
||||
// 3. Update certificate status to Revoked
|
||||
// 3. + 4. + audit: cert status update + revocation row + audit row.
|
||||
// Atomic path (when SetTransactor was wired) keeps these three
|
||||
// writes consistent: a failure in any one rolls back the others.
|
||||
// Closes the #3 audit-readiness blocker for the revocation path.
|
||||
now := time.Now()
|
||||
cert.Status = domain.CertificateStatusRevoked
|
||||
cert.RevokedAt = &now
|
||||
cert.RevocationReason = reason
|
||||
cert.UpdatedAt = now
|
||||
if err := s.certRepo.Update(ctx, cert); err != nil {
|
||||
return fmt.Errorf("failed to update certificate status: %w", err)
|
||||
|
||||
auditDetails := map[string]interface{}{
|
||||
"common_name": cert.CommonName,
|
||||
"serial": version.SerialNumber,
|
||||
"reason": reason,
|
||||
}
|
||||
|
||||
// 4. Record revocation in certificate_revocations table (for CRL generation)
|
||||
if s.revocationRepo != nil {
|
||||
revocation := &domain.CertificateRevocation{
|
||||
ID: generateID("rev"),
|
||||
CertificateID: certID,
|
||||
SerialNumber: version.SerialNumber,
|
||||
Reason: reason,
|
||||
RevokedBy: actor,
|
||||
RevokedAt: now,
|
||||
IssuerID: cert.IssuerID,
|
||||
CreatedAt: now,
|
||||
if s.tx != nil {
|
||||
// Atomic three-write path.
|
||||
if err := s.tx.WithinTx(ctx, func(q repository.Querier) error {
|
||||
if err := s.certRepo.UpdateWithTx(ctx, q, cert); err != nil {
|
||||
return fmt.Errorf("failed to update certificate status: %w", err)
|
||||
}
|
||||
if s.revocationRepo != nil {
|
||||
revocation := &domain.CertificateRevocation{
|
||||
ID: generateID("rev"),
|
||||
CertificateID: certID,
|
||||
SerialNumber: version.SerialNumber,
|
||||
Reason: reason,
|
||||
RevokedBy: actor,
|
||||
RevokedAt: now,
|
||||
IssuerID: cert.IssuerID,
|
||||
CreatedAt: now,
|
||||
}
|
||||
if err := s.revocationRepo.CreateWithTx(ctx, q, revocation); err != nil {
|
||||
return fmt.Errorf("failed to record revocation: %w", err)
|
||||
}
|
||||
}
|
||||
if err := s.auditService.RecordEventWithTx(ctx, q, actor, domain.ActorTypeUser,
|
||||
"certificate_revoked", "certificate", certID, auditDetails); err != nil {
|
||||
return fmt.Errorf("failed to record audit event: %w", err)
|
||||
}
|
||||
return nil
|
||||
}); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := s.revocationRepo.Create(ctx, revocation); err != nil {
|
||||
slog.Error("failed to record revocation for CRL", "error", err, "certificate_id", certID)
|
||||
// Don't fail the overall revocation — the cert status is already updated
|
||||
} else {
|
||||
// Legacy non-transactional path. Pre-fix behavior preserved
|
||||
// for backward compat with callers that haven't wired
|
||||
// SetTransactor.
|
||||
if err := s.certRepo.Update(ctx, cert); err != nil {
|
||||
return fmt.Errorf("failed to update certificate status: %w", err)
|
||||
}
|
||||
if s.revocationRepo != nil {
|
||||
revocation := &domain.CertificateRevocation{
|
||||
ID: generateID("rev"),
|
||||
CertificateID: certID,
|
||||
SerialNumber: version.SerialNumber,
|
||||
Reason: reason,
|
||||
RevokedBy: actor,
|
||||
RevokedAt: now,
|
||||
IssuerID: cert.IssuerID,
|
||||
CreatedAt: now,
|
||||
}
|
||||
if err := s.revocationRepo.Create(ctx, revocation); err != nil {
|
||||
slog.Error("failed to record revocation for CRL", "error", err, "certificate_id", certID)
|
||||
// Don't fail the overall revocation — the cert status is already updated
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -171,15 +228,13 @@ func (s *RevocationSvc) RevokeCertificateWithActor(ctx context.Context, certID s
|
||||
}
|
||||
}
|
||||
|
||||
// 6. Record audit event
|
||||
if err := s.auditService.RecordEvent(ctx, actor, domain.ActorTypeUser,
|
||||
"certificate_revoked", "certificate", certID,
|
||||
map[string]interface{}{
|
||||
"common_name": cert.CommonName,
|
||||
"serial": version.SerialNumber,
|
||||
"reason": reason,
|
||||
}); err != nil {
|
||||
slog.Error("failed to record audit event", "error", err)
|
||||
// 6. Record audit event (legacy non-transactional path only — the
|
||||
// atomic path already recorded the audit inside the tx above).
|
||||
if s.tx == nil {
|
||||
if err := s.auditService.RecordEvent(ctx, actor, domain.ActorTypeUser,
|
||||
"certificate_revoked", "certificate", certID, auditDetails); err != nil {
|
||||
slog.Error("failed to record audit event", "error", err)
|
||||
}
|
||||
}
|
||||
|
||||
// 7. Send revocation notification
|
||||
|
||||
@@ -178,10 +178,18 @@ func (m *mockCertRepoWithGetError) Create(ctx context.Context, cert *domain.Mana
|
||||
return nil
|
||||
}
|
||||
|
||||
func (m *mockCertRepoWithGetError) CreateWithTx(ctx context.Context, q repository.Querier, cert *domain.ManagedCertificate) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
func (m *mockCertRepoWithGetError) Update(ctx context.Context, cert *domain.ManagedCertificate) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
func (m *mockCertRepoWithGetError) UpdateWithTx(ctx context.Context, q repository.Querier, cert *domain.ManagedCertificate) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
func (m *mockCertRepoWithGetError) Archive(ctx context.Context, id string) error {
|
||||
return nil
|
||||
}
|
||||
@@ -194,6 +202,10 @@ func (m *mockCertRepoWithGetError) CreateVersion(ctx context.Context, version *d
|
||||
return nil
|
||||
}
|
||||
|
||||
func (m *mockCertRepoWithGetError) CreateVersionWithTx(ctx context.Context, q repository.Querier, version *domain.CertificateVersion) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
func (m *mockCertRepoWithGetError) GetLatestVersion(ctx context.Context, certID string) (*domain.CertificateVersion, error) {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
@@ -70,6 +70,13 @@ func (m *mockCertRepo) Create(ctx context.Context, cert *domain.ManagedCertifica
|
||||
return nil
|
||||
}
|
||||
|
||||
// CreateWithTx mirrors Create — mocks have no DB, so the Querier
|
||||
// argument is ignored. Production behavior comes from postgres.WithTx
|
||||
// path; mocks just exercise the in-memory state.
|
||||
func (m *mockCertRepo) CreateWithTx(ctx context.Context, q repository.Querier, cert *domain.ManagedCertificate) error {
|
||||
return m.Create(ctx, cert)
|
||||
}
|
||||
|
||||
func (m *mockCertRepo) Update(ctx context.Context, cert *domain.ManagedCertificate) error {
|
||||
if m.UpdateErr != nil {
|
||||
return m.UpdateErr
|
||||
@@ -79,6 +86,11 @@ func (m *mockCertRepo) Update(ctx context.Context, cert *domain.ManagedCertifica
|
||||
return nil
|
||||
}
|
||||
|
||||
// UpdateWithTx mirrors Update — see CreateWithTx note.
|
||||
func (m *mockCertRepo) UpdateWithTx(ctx context.Context, q repository.Querier, cert *domain.ManagedCertificate) error {
|
||||
return m.Update(ctx, cert)
|
||||
}
|
||||
|
||||
func (m *mockCertRepo) Archive(ctx context.Context, id string) error {
|
||||
if m.ArchiveErr != nil {
|
||||
return m.ArchiveErr
|
||||
@@ -109,6 +121,11 @@ func (m *mockCertRepo) CreateVersion(ctx context.Context, version *domain.Certif
|
||||
return nil
|
||||
}
|
||||
|
||||
// CreateVersionWithTx mirrors CreateVersion.
|
||||
func (m *mockCertRepo) CreateVersionWithTx(ctx context.Context, q repository.Querier, version *domain.CertificateVersion) error {
|
||||
return m.CreateVersion(ctx, version)
|
||||
}
|
||||
|
||||
func (m *mockCertRepo) GetExpiringCertificates(ctx context.Context, before time.Time) ([]*domain.ManagedCertificate, error) {
|
||||
// Return MockGetExpiring if set, for test control
|
||||
if m.MockGetExpiring != nil {
|
||||
@@ -664,6 +681,11 @@ func (m *mockAuditRepo) Create(ctx context.Context, event *domain.AuditEvent) er
|
||||
return nil
|
||||
}
|
||||
|
||||
// CreateWithTx mirrors Create — mocks have no DB; the Querier is ignored.
|
||||
func (m *mockAuditRepo) CreateWithTx(ctx context.Context, q repository.Querier, event *domain.AuditEvent) error {
|
||||
return m.Create(ctx, event)
|
||||
}
|
||||
|
||||
func (m *mockAuditRepo) List(ctx context.Context, filter *repository.AuditFilter) ([]*domain.AuditEvent, error) {
|
||||
m.mu.Lock()
|
||||
defer m.mu.Unlock()
|
||||
@@ -1380,6 +1402,31 @@ func newMockRenewalPolicyRepository() *mockRenewalPolicyRepo {
|
||||
}
|
||||
}
|
||||
|
||||
// mockTransactor is a no-op repository.Transactor for tests. It runs fn
|
||||
// synchronously without any DB; the Querier passed to fn is nil because
|
||||
// the mock repo *WithTx methods ignore it. If fn returns an error, the
|
||||
// "transaction" is not committed — but since mocks share state, in-memory
|
||||
// rollback isn't simulated. Tests that need rollback semantics use
|
||||
// mockTransactor with WantRollbackOnErr=true to assert fn's error
|
||||
// propagated correctly.
|
||||
type mockTransactor struct {
|
||||
WantRollbackOnErr bool
|
||||
BeginTxErr error
|
||||
CommitErr error
|
||||
}
|
||||
|
||||
func (m *mockTransactor) WithinTx(ctx context.Context, fn func(q repository.Querier) error) error {
|
||||
if m.BeginTxErr != nil {
|
||||
return m.BeginTxErr
|
||||
}
|
||||
if err := fn(nil); err != nil {
|
||||
return err
|
||||
}
|
||||
return m.CommitErr
|
||||
}
|
||||
|
||||
func newMockTransactor() *mockTransactor { return &mockTransactor{} }
|
||||
|
||||
func newMockAgentRepository() *mockAgentRepo {
|
||||
return &mockAgentRepo{
|
||||
Agents: make(map[string]*domain.Agent),
|
||||
@@ -1491,6 +1538,11 @@ type mockRevocationRepo struct {
|
||||
LastListIssuerID string
|
||||
}
|
||||
|
||||
// CreateWithTx mirrors Create — mocks have no DB; the Querier is ignored.
|
||||
func (m *mockRevocationRepo) CreateWithTx(ctx context.Context, q repository.Querier, revocation *domain.CertificateRevocation) error {
|
||||
return m.Create(ctx, revocation)
|
||||
}
|
||||
|
||||
func (m *mockRevocationRepo) Create(ctx context.Context, revocation *domain.CertificateRevocation) error {
|
||||
if m.CreateErr != nil {
|
||||
return m.CreateErr
|
||||
|
||||
Reference in New Issue
Block a user