mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-09 21:08:52 +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).
423 lines
14 KiB
Go
423 lines
14 KiB
Go
package service
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"log/slog"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/shankar0123/certctl/internal/domain"
|
|
"github.com/shankar0123/certctl/internal/repository"
|
|
)
|
|
|
|
// setupShortLivedTestService creates a RenewalService with mock dependencies for short-lived cert tests
|
|
func setupShortLivedTestService(
|
|
certRepo *mockCertRepo,
|
|
profileRepo *mockProfileRepo,
|
|
auditRepo *mockAuditRepo,
|
|
) *RenewalService {
|
|
auditSvc := NewAuditService(auditRepo)
|
|
|
|
issuerRegistry := NewIssuerRegistry(slog.Default())
|
|
issuerRegistry.Set("iss-test", &mockIssuerConnector{})
|
|
|
|
svc := NewRenewalService(
|
|
certRepo,
|
|
newMockJobRepository(),
|
|
newMockRenewalPolicyRepository(),
|
|
profileRepo,
|
|
auditSvc,
|
|
NewNotificationService(newMockNotificationRepository(), map[string]Notifier{}),
|
|
issuerRegistry,
|
|
"agent",
|
|
)
|
|
|
|
return svc
|
|
}
|
|
|
|
// TestExpireShortLivedCertificates_Success verifies that active certificates with
|
|
// expired short-lived profiles are transitioned to Expired status
|
|
func TestExpireShortLivedCertificates_Success(t *testing.T) {
|
|
ctx := context.Background()
|
|
now := time.Now()
|
|
|
|
certRepo := newMockCertificateRepository()
|
|
profileRepo := newMockProfileRepository()
|
|
auditRepo := newMockAuditRepository()
|
|
|
|
// Create a short-lived profile (TTL < 1 hour = 3600 seconds)
|
|
shortLivedProfile := &domain.CertificateProfile{
|
|
ID: "prof-short",
|
|
Name: "Short-Lived",
|
|
MaxTTLSeconds: 300, // 5 minutes
|
|
AllowShortLived: true,
|
|
Enabled: true,
|
|
AllowedKeyAlgorithms: domain.DefaultKeyAlgorithms(),
|
|
AllowedEKUs: domain.DefaultEKUs(),
|
|
CreatedAt: now,
|
|
UpdatedAt: now,
|
|
}
|
|
profileRepo.AddProfile(shortLivedProfile)
|
|
|
|
// Create an active certificate that has already expired
|
|
expiredCert := &domain.ManagedCertificate{
|
|
ID: "mc-expired-short",
|
|
Name: "Expired Short-Lived Cert",
|
|
CommonName: "short.example.com",
|
|
SANs: []string{},
|
|
IssuerID: "iss-test",
|
|
CertificateProfileID: "prof-short",
|
|
Status: domain.CertificateStatusActive,
|
|
ExpiresAt: now.Add(-5 * time.Minute), // Already expired
|
|
CreatedAt: now.Add(-15 * time.Minute),
|
|
UpdatedAt: now.Add(-5 * time.Minute),
|
|
Tags: make(map[string]string),
|
|
}
|
|
certRepo.AddCert(expiredCert)
|
|
|
|
svc := setupShortLivedTestService(certRepo, profileRepo, auditRepo)
|
|
|
|
// Run the expiry check
|
|
err := svc.ExpireShortLivedCertificates(ctx)
|
|
if err != nil {
|
|
t.Fatalf("ExpireShortLivedCertificates failed: %v", err)
|
|
}
|
|
|
|
// Verify the cert status was updated to Expired
|
|
updated, err := certRepo.Get(ctx, "mc-expired-short")
|
|
if err != nil {
|
|
t.Fatalf("failed to get updated cert: %v", err)
|
|
}
|
|
if updated.Status != domain.CertificateStatusExpired {
|
|
t.Errorf("expected cert status to be Expired, got %s", updated.Status)
|
|
}
|
|
|
|
// Verify an audit event was recorded
|
|
if len(auditRepo.Events) == 0 {
|
|
t.Errorf("expected audit event to be recorded, got none")
|
|
}
|
|
}
|
|
|
|
// TestExpireShortLivedCertificates_NoCertsToExpire verifies the function handles
|
|
// empty certificate lists gracefully
|
|
func TestExpireShortLivedCertificates_NoCertsToExpire(t *testing.T) {
|
|
ctx := context.Background()
|
|
|
|
certRepo := newMockCertificateRepository()
|
|
profileRepo := newMockProfileRepository()
|
|
auditRepo := newMockAuditRepository()
|
|
|
|
svc := setupShortLivedTestService(certRepo, profileRepo, auditRepo)
|
|
|
|
// Run the expiry check on empty certificate list
|
|
err := svc.ExpireShortLivedCertificates(ctx)
|
|
if err != nil {
|
|
t.Fatalf("ExpireShortLivedCertificates failed: %v", err)
|
|
}
|
|
|
|
// Verify no audit events were recorded
|
|
if len(auditRepo.Events) != 0 {
|
|
t.Errorf("expected no audit events, got %d", len(auditRepo.Events))
|
|
}
|
|
}
|
|
|
|
// TestExpireShortLivedCertificates_ListError verifies that repository errors
|
|
// are properly propagated
|
|
func TestExpireShortLivedCertificates_ListError(t *testing.T) {
|
|
ctx := context.Background()
|
|
|
|
// Create a custom mock that returns an error from GetExpiringCertificates
|
|
customCertRepo := &mockCertRepoWithGetError{
|
|
GetExpiringCertificatesErr: errors.New("database connection failed"),
|
|
}
|
|
|
|
profileRepo := newMockProfileRepository()
|
|
auditRepo := newMockAuditRepository()
|
|
|
|
// Create the service manually to use our custom cert repo
|
|
auditSvc := NewAuditService(auditRepo)
|
|
issuerRegistry := NewIssuerRegistry(slog.Default())
|
|
issuerRegistry.Set("iss-test", &mockIssuerConnector{})
|
|
|
|
svc := NewRenewalService(
|
|
customCertRepo,
|
|
newMockJobRepository(),
|
|
newMockRenewalPolicyRepository(),
|
|
profileRepo,
|
|
auditSvc,
|
|
NewNotificationService(newMockNotificationRepository(), map[string]Notifier{}),
|
|
issuerRegistry,
|
|
"agent",
|
|
)
|
|
|
|
// Run the expiry check, expecting an error
|
|
err := svc.ExpireShortLivedCertificates(ctx)
|
|
if err == nil {
|
|
t.Fatalf("expected ExpireShortLivedCertificates to return an error, got nil")
|
|
}
|
|
if !errors.Is(err, customCertRepo.GetExpiringCertificatesErr) {
|
|
t.Errorf("expected error containing 'database connection failed', got %v", err)
|
|
}
|
|
}
|
|
|
|
// mockCertRepoWithGetError is a minimal custom mock for testing GetExpiringCertificates error handling
|
|
type mockCertRepoWithGetError struct {
|
|
GetExpiringCertificatesErr error
|
|
}
|
|
|
|
func (m *mockCertRepoWithGetError) List(ctx context.Context, filter *repository.CertificateFilter) ([]*domain.ManagedCertificate, int, error) {
|
|
return nil, 0, nil
|
|
}
|
|
|
|
func (m *mockCertRepoWithGetError) Get(ctx context.Context, id string) (*domain.ManagedCertificate, error) {
|
|
return nil, nil
|
|
}
|
|
|
|
func (m *mockCertRepoWithGetError) Create(ctx context.Context, cert *domain.ManagedCertificate) error {
|
|
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
|
|
}
|
|
|
|
func (m *mockCertRepoWithGetError) ListVersions(ctx context.Context, certID string) ([]*domain.CertificateVersion, error) {
|
|
return nil, nil
|
|
}
|
|
|
|
func (m *mockCertRepoWithGetError) CreateVersion(ctx context.Context, version *domain.CertificateVersion) error {
|
|
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
|
|
}
|
|
|
|
func (m *mockCertRepoWithGetError) GetByIssuerAndSerial(ctx context.Context, issuerID, serial string) (*domain.ManagedCertificate, error) {
|
|
return nil, nil
|
|
}
|
|
|
|
func (m *mockCertRepoWithGetError) GetExpiringCertificates(ctx context.Context, before time.Time) ([]*domain.ManagedCertificate, error) {
|
|
return nil, m.GetExpiringCertificatesErr
|
|
}
|
|
|
|
// TestExpireShortLivedCertificates_PartialUpdateError verifies that update errors
|
|
// on individual certs are logged but don't fail the entire operation
|
|
func TestExpireShortLivedCertificates_PartialUpdateError(t *testing.T) {
|
|
ctx := context.Background()
|
|
now := time.Now()
|
|
|
|
certRepo := newMockCertificateRepository()
|
|
profileRepo := newMockProfileRepository()
|
|
auditRepo := newMockAuditRepository()
|
|
|
|
// Create a short-lived profile
|
|
shortLivedProfile := &domain.CertificateProfile{
|
|
ID: "prof-short",
|
|
Name: "Short-Lived",
|
|
MaxTTLSeconds: 300,
|
|
AllowShortLived: true,
|
|
Enabled: true,
|
|
AllowedKeyAlgorithms: domain.DefaultKeyAlgorithms(),
|
|
AllowedEKUs: domain.DefaultEKUs(),
|
|
CreatedAt: now,
|
|
UpdatedAt: now,
|
|
}
|
|
profileRepo.AddProfile(shortLivedProfile)
|
|
|
|
// Create a certificate with a failing update
|
|
expiredCert := &domain.ManagedCertificate{
|
|
ID: "mc-expired-fail",
|
|
Name: "Expired Cert That Will Fail",
|
|
CommonName: "fail.example.com",
|
|
SANs: []string{},
|
|
IssuerID: "iss-test",
|
|
CertificateProfileID: "prof-short",
|
|
Status: domain.CertificateStatusActive,
|
|
ExpiresAt: now.Add(-5 * time.Minute),
|
|
CreatedAt: now.Add(-15 * time.Minute),
|
|
UpdatedAt: now.Add(-5 * time.Minute),
|
|
Tags: make(map[string]string),
|
|
}
|
|
certRepo.AddCert(expiredCert)
|
|
|
|
// Set up the repo to fail on update
|
|
certRepo.UpdateErr = errors.New("update failed")
|
|
|
|
svc := setupShortLivedTestService(certRepo, profileRepo, auditRepo)
|
|
|
|
// Run the expiry check - should not return an error even though update failed
|
|
err := svc.ExpireShortLivedCertificates(ctx)
|
|
if err != nil {
|
|
t.Fatalf("ExpireShortLivedCertificates should not fail on partial update errors, got %v", err)
|
|
}
|
|
|
|
// Verify no audit events were recorded (update failure skips audit recording)
|
|
if len(auditRepo.Events) != 0 {
|
|
t.Errorf("expected no audit events on update failure, got %d", len(auditRepo.Events))
|
|
}
|
|
}
|
|
|
|
// TestExpireShortLivedCertificates_AlreadyExpired verifies that certificates
|
|
// already in Expired status are not re-processed
|
|
func TestExpireShortLivedCertificates_AlreadyExpired(t *testing.T) {
|
|
ctx := context.Background()
|
|
now := time.Now()
|
|
|
|
certRepo := newMockCertificateRepository()
|
|
profileRepo := newMockProfileRepository()
|
|
auditRepo := newMockAuditRepository()
|
|
|
|
// Create a short-lived profile
|
|
shortLivedProfile := &domain.CertificateProfile{
|
|
ID: "prof-short",
|
|
Name: "Short-Lived",
|
|
MaxTTLSeconds: 300,
|
|
AllowShortLived: true,
|
|
Enabled: true,
|
|
AllowedKeyAlgorithms: domain.DefaultKeyAlgorithms(),
|
|
AllowedEKUs: domain.DefaultEKUs(),
|
|
CreatedAt: now,
|
|
UpdatedAt: now,
|
|
}
|
|
profileRepo.AddProfile(shortLivedProfile)
|
|
|
|
// Create a certificate that's already in Expired status
|
|
alreadyExpiredCert := &domain.ManagedCertificate{
|
|
ID: "mc-already-expired",
|
|
Name: "Already Expired Cert",
|
|
CommonName: "already-expired.example.com",
|
|
SANs: []string{},
|
|
IssuerID: "iss-test",
|
|
CertificateProfileID: "prof-short",
|
|
Status: domain.CertificateStatusExpired, // Already expired
|
|
ExpiresAt: now.Add(-30 * time.Minute),
|
|
CreatedAt: now.Add(-45 * time.Minute),
|
|
UpdatedAt: now.Add(-10 * time.Minute),
|
|
Tags: make(map[string]string),
|
|
}
|
|
certRepo.AddCert(alreadyExpiredCert)
|
|
|
|
svc := setupShortLivedTestService(certRepo, profileRepo, auditRepo)
|
|
|
|
// Run the expiry check
|
|
err := svc.ExpireShortLivedCertificates(ctx)
|
|
if err != nil {
|
|
t.Fatalf("ExpireShortLivedCertificates failed: %v", err)
|
|
}
|
|
|
|
// Verify no new audit events were recorded (cert was skipped)
|
|
if len(auditRepo.Events) != 0 {
|
|
t.Errorf("expected no audit events for already-expired cert, got %d", len(auditRepo.Events))
|
|
}
|
|
}
|
|
|
|
// TestExpireShortLivedCertificates_ProfileNotShortLived verifies that certificates
|
|
// with non-short-lived profiles are not expired by this function
|
|
func TestExpireShortLivedCertificates_ProfileNotShortLived(t *testing.T) {
|
|
ctx := context.Background()
|
|
now := time.Now()
|
|
|
|
certRepo := newMockCertificateRepository()
|
|
profileRepo := newMockProfileRepository()
|
|
auditRepo := newMockAuditRepository()
|
|
|
|
// Create a regular (not short-lived) profile with TTL > 1 hour
|
|
regularProfile := &domain.CertificateProfile{
|
|
ID: "prof-regular",
|
|
Name: "Regular",
|
|
MaxTTLSeconds: 86400, // 24 hours
|
|
AllowShortLived: false,
|
|
Enabled: true,
|
|
AllowedKeyAlgorithms: domain.DefaultKeyAlgorithms(),
|
|
AllowedEKUs: domain.DefaultEKUs(),
|
|
CreatedAt: now,
|
|
UpdatedAt: now,
|
|
}
|
|
profileRepo.AddProfile(regularProfile)
|
|
|
|
// Create an expired certificate with the regular profile
|
|
expiredCert := &domain.ManagedCertificate{
|
|
ID: "mc-expired-regular",
|
|
Name: "Expired Regular Cert",
|
|
CommonName: "regular.example.com",
|
|
SANs: []string{},
|
|
IssuerID: "iss-test",
|
|
CertificateProfileID: "prof-regular",
|
|
Status: domain.CertificateStatusActive,
|
|
ExpiresAt: now.Add(-1 * time.Hour),
|
|
CreatedAt: now.Add(-25 * time.Hour),
|
|
UpdatedAt: now.Add(-1 * time.Hour),
|
|
Tags: make(map[string]string),
|
|
}
|
|
certRepo.AddCert(expiredCert)
|
|
|
|
svc := setupShortLivedTestService(certRepo, profileRepo, auditRepo)
|
|
|
|
// Run the expiry check
|
|
err := svc.ExpireShortLivedCertificates(ctx)
|
|
if err != nil {
|
|
t.Fatalf("ExpireShortLivedCertificates failed: %v", err)
|
|
}
|
|
|
|
// Verify the cert status was NOT changed (because profile is not short-lived)
|
|
cert, _ := certRepo.Get(ctx, "mc-expired-regular")
|
|
if cert.Status != domain.CertificateStatusActive {
|
|
t.Errorf("cert should not have been expired (profile not short-lived), got status %s", cert.Status)
|
|
}
|
|
|
|
// Verify no audit events were recorded
|
|
if len(auditRepo.Events) != 0 {
|
|
t.Errorf("expected no audit events for non-short-lived profile, got %d", len(auditRepo.Events))
|
|
}
|
|
}
|
|
|
|
// TestExpireShortLivedCertificates_NoProfileRepository verifies the function
|
|
// handles nil profileRepo gracefully
|
|
func TestExpireShortLivedCertificates_NoProfileRepository(t *testing.T) {
|
|
ctx := context.Background()
|
|
|
|
certRepo := newMockCertificateRepository()
|
|
auditRepo := &mockAuditRepo{
|
|
Events: make([]*domain.AuditEvent, 0),
|
|
}
|
|
|
|
auditSvc := NewAuditService(auditRepo)
|
|
issuerRegistry := NewIssuerRegistry(slog.Default())
|
|
issuerRegistry.Set("iss-test", &mockIssuerConnector{})
|
|
|
|
svc := NewRenewalService(
|
|
certRepo,
|
|
newMockJobRepository(),
|
|
newMockRenewalPolicyRepository(),
|
|
nil, // nil profileRepo
|
|
auditSvc,
|
|
NewNotificationService(newMockNotificationRepository(), map[string]Notifier{}),
|
|
issuerRegistry,
|
|
"agent",
|
|
)
|
|
|
|
// Run the expiry check with nil profileRepo
|
|
err := svc.ExpireShortLivedCertificates(ctx)
|
|
if err != nil {
|
|
t.Fatalf("ExpireShortLivedCertificates should handle nil profileRepo gracefully, got error: %v", err)
|
|
}
|
|
}
|