diff --git a/cmd/server/main.go b/cmd/server/main.go index a84119d..fa1c08e 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -89,7 +89,45 @@ func main() { encryptionKey = crypto.DeriveKey(cfg.Encryption.ConfigEncryptionKey) logger.Info("config encryption enabled (AES-256-GCM)") } else { - logger.Warn("CERTCTL_CONFIG_ENCRYPTION_KEY not set — issuer configs stored in plaintext (not recommended for production)") + // C-2 fix: fail closed at startup when database-sourced issuer or target + // rows exist without a configured encryption key. Previously the server + // would emit a one-line warning and silently persist new GUI-created + // configs as plaintext (CWE-311). Refuse to start instead: the operator + // must either configure CERTCTL_CONFIG_ENCRYPTION_KEY or remove the + // vulnerable rows before the control plane can boot. + ctx := context.Background() + dbIssuers, ierr := issuerRepo.List(ctx) + if ierr != nil { + logger.Error("startup check: failed to list issuers", "error", ierr) + os.Exit(1) + } + dbTargets, terr := targetRepo.List(ctx) + if terr != nil { + logger.Error("startup check: failed to list targets", "error", terr) + os.Exit(1) + } + var dbIssuerCount, dbTargetCount int + for _, iss := range dbIssuers { + if iss != nil && iss.Source == "database" { + dbIssuerCount++ + } + } + for _, tgt := range dbTargets { + if tgt != nil && tgt.Source == "database" { + dbTargetCount++ + } + } + if dbIssuerCount > 0 || dbTargetCount > 0 { + logger.Error( + "startup refused: CERTCTL_CONFIG_ENCRYPTION_KEY is not set but database-sourced configs exist "+ + "(would expose sensitive fields as plaintext, CWE-311). "+ + "Set the encryption key or remove the affected rows before restarting.", + "database_sourced_issuers", dbIssuerCount, + "database_sourced_targets", dbTargetCount, + ) + os.Exit(1) + } + logger.Warn("CERTCTL_CONFIG_ENCRYPTION_KEY not set — env-seeded issuers will be stored in plaintext; GUI-created issuers and targets will be rejected until a key is configured") } issuerRegistry := service.NewIssuerRegistry(logger) diff --git a/internal/crypto/encryption.go b/internal/crypto/encryption.go index 68eebc9..dc56d86 100644 --- a/internal/crypto/encryption.go +++ b/internal/crypto/encryption.go @@ -6,12 +6,29 @@ import ( "crypto/cipher" "crypto/rand" "crypto/sha256" + "errors" "fmt" "io" "golang.org/x/crypto/pbkdf2" ) +// ErrEncryptionKeyRequired is returned by EncryptIfKeySet and DecryptIfKeySet when +// the caller provides an empty key but the data on the wire requires protection. +// +// Historically these helpers silently returned plaintext when no key was configured, +// which produced a data-at-rest confidentiality bypass (CWE-311): sensitive fields +// in dynamically-configured issuer and target records (source='database') were +// persisted to PostgreSQL without any encryption whenever the operator forgot to +// set CERTCTL_CONFIG_ENCRYPTION_KEY. Callers could not distinguish the encrypted +// and plaintext branches at runtime, so the only visible signal was a warning +// line emitted once at startup. +// +// The fix is to fail closed: EncryptIfKeySet/DecryptIfKeySet now require a key +// whenever they are invoked on sensitive material, and the server refuses to +// start if any source='database' rows already exist without a configured key. +var ErrEncryptionKeyRequired = errors.New("crypto: CERTCTL_CONFIG_ENCRYPTION_KEY is required to encrypt or decrypt sensitive config") + // Encrypt encrypts plaintext using AES-256-GCM with a random 12-byte nonce prepended to the output. // The key must be exactly 32 bytes (AES-256). Returns [12-byte nonce][ciphertext+tag]. func Encrypt(plaintext []byte, key []byte) ([]byte, error) { @@ -81,11 +98,17 @@ func DeriveKey(passphrase string) []byte { return pbkdf2.Key([]byte(passphrase), salt, 100000, 32, sha256.New) } -// EncryptIfKeySet encrypts plaintext if a key is provided, otherwise returns plaintext unchanged. -// This supports the development/demo fallback where encryption isn't configured. +// EncryptIfKeySet encrypts plaintext with the supplied 32-byte AES-256 key. +// +// The second return value is always true when err == nil — the "wasEncrypted" +// flag is retained for source-compatibility with callers that previously used it +// to log provenance. Callers MUST handle err: passing an empty key now returns +// ErrEncryptionKeyRequired rather than silently emitting plaintext. See the +// package-level ErrEncryptionKeyRequired documentation for the history behind +// this behavior change. func EncryptIfKeySet(plaintext []byte, key []byte) ([]byte, bool, error) { if len(key) == 0 { - return plaintext, false, nil + return nil, false, ErrEncryptionKeyRequired } encrypted, err := Encrypt(plaintext, key) if err != nil { @@ -94,10 +117,17 @@ func EncryptIfKeySet(plaintext []byte, key []byte) ([]byte, bool, error) { return encrypted, true, nil } -// DecryptIfKeySet decrypts ciphertext if a key is provided, otherwise returns ciphertext unchanged. +// DecryptIfKeySet decrypts ciphertext with the supplied 32-byte AES-256 key. +// +// Passing an empty key now returns ErrEncryptionKeyRequired. Callers that +// legitimately store plaintext (e.g. env-seeded source='env' rows that keep +// the raw JSON in the unencrypted `config` column) must branch on the presence +// of the ciphertext themselves rather than relying on this helper to silently +// pass bytes through. See the package-level ErrEncryptionKeyRequired +// documentation for the history behind this behavior change. func DecryptIfKeySet(ciphertext []byte, key []byte) ([]byte, error) { if len(key) == 0 { - return ciphertext, nil + return nil, ErrEncryptionKeyRequired } return Decrypt(ciphertext, key) } diff --git a/internal/crypto/encryption_test.go b/internal/crypto/encryption_test.go index 24c7727..d26480d 100644 --- a/internal/crypto/encryption_test.go +++ b/internal/crypto/encryption_test.go @@ -2,6 +2,7 @@ package crypto import ( "bytes" + "errors" "testing" ) @@ -148,31 +149,140 @@ func TestEncryptIfKeySet_WithKey(t *testing.T) { } } -func TestEncryptIfKeySet_NilKey(t *testing.T) { +// TestEncryptIfKeySet_EmptyKeyFailsClosed asserts the C-2 regression guard: +// EncryptIfKeySet must refuse to silently emit plaintext when no key is configured. +// The pre-fix behavior was to return plaintext with wasEncrypted=false, which +// produced a data-at-rest confidentiality bypass (CWE-311) for GUI-created +// issuer and target configs. +func TestEncryptIfKeySet_EmptyKeyFailsClosed(t *testing.T) { plaintext := []byte("config data") - result, wasEncrypted, err := EncryptIfKeySet(plaintext, nil) - if err != nil { - t.Fatalf("EncryptIfKeySet with nil key failed: %v", err) + cases := []struct { + name string + key []byte + }{ + {"nil_key", nil}, + {"empty_key", []byte{}}, } - if wasEncrypted { - t.Fatal("expected wasEncrypted=false when key is nil") - } - if !bytes.Equal(result, plaintext) { - t.Fatal("result should be unchanged plaintext when key is nil") + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + result, wasEncrypted, err := EncryptIfKeySet(plaintext, tc.key) + if err == nil { + t.Fatal("expected ErrEncryptionKeyRequired, got nil") + } + if !errors.Is(err, ErrEncryptionKeyRequired) { + t.Fatalf("expected ErrEncryptionKeyRequired, got %v", err) + } + if wasEncrypted { + t.Fatal("wasEncrypted must be false on error") + } + if result != nil { + t.Fatalf("expected nil result on error, got %q", result) + } + }) } } -func TestDecryptIfKeySet_NilKey(t *testing.T) { +// TestDecryptIfKeySet_EmptyKeyFailsClosed asserts the matching C-2 regression +// guard on the read path: DecryptIfKeySet must refuse to pass ciphertext +// through as plaintext when no key is configured. +func TestDecryptIfKeySet_EmptyKeyFailsClosed(t *testing.T) { data := []byte("plaintext config data") - result, err := DecryptIfKeySet(data, nil) + cases := []struct { + name string + key []byte + }{ + {"nil_key", nil}, + {"empty_key", []byte{}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + result, err := DecryptIfKeySet(data, tc.key) + if err == nil { + t.Fatal("expected ErrEncryptionKeyRequired, got nil") + } + if !errors.Is(err, ErrEncryptionKeyRequired) { + t.Fatalf("expected ErrEncryptionKeyRequired, got %v", err) + } + if result != nil { + t.Fatalf("expected nil result on error, got %q", result) + } + }) + } +} + +// TestEncryptDecryptIfKeySet_RoundTripProducesDifferentCiphertext proves the +// "if set" helpers produce real AES-GCM output (not plaintext) and that a full +// round-trip through both helpers recovers the original bytes. +func TestEncryptDecryptIfKeySet_RoundTripProducesDifferentCiphertext(t *testing.T) { + key := DeriveKey("round-trip-key") + plaintext := []byte(`{"api_key":"s3cr3t","token":"abc"}`) + + encrypted, wasEncrypted, err := EncryptIfKeySet(plaintext, key) if err != nil { - t.Fatalf("DecryptIfKeySet with nil key failed: %v", err) + t.Fatalf("EncryptIfKeySet failed: %v", err) } - if !bytes.Equal(result, data) { - t.Fatal("result should be unchanged when key is nil") + if !wasEncrypted { + t.Fatal("wasEncrypted must be true when key is present") } + if bytes.Equal(encrypted, plaintext) { + t.Fatal("EncryptIfKeySet returned plaintext — would regress C-2") + } + + decrypted, err := DecryptIfKeySet(encrypted, key) + if err != nil { + t.Fatalf("DecryptIfKeySet failed: %v", err) + } + if !bytes.Equal(decrypted, plaintext) { + t.Fatalf("round-trip mismatch: got %q, want %q", decrypted, plaintext) + } +} + +// TestDecryptIfKeySet_RejectsTamperedCiphertext confirms the AEAD auth tag +// still rejects modified ciphertext when routed through the helper. +func TestDecryptIfKeySet_RejectsTamperedCiphertext(t *testing.T) { + key := DeriveKey("tamper-test-key") + plaintext := []byte("authenticated data") + + encrypted, _, err := EncryptIfKeySet(plaintext, key) + if err != nil { + t.Fatalf("EncryptIfKeySet failed: %v", err) + } + // Flip a byte inside the GCM body (past the 12-byte nonce) to invalidate the tag. + if len(encrypted) <= 13 { + t.Fatalf("ciphertext too short to tamper: %d bytes", len(encrypted)) + } + encrypted[13] ^= 0xFF + + if _, err := DecryptIfKeySet(encrypted, key); err == nil { + t.Fatal("DecryptIfKeySet accepted tampered ciphertext — AEAD tag check bypassed") + } +} + +// TestEncryptIfKeySet_PreservesErrEncryptionKeyRequiredSentinel guards the +// stability of the public sentinel error so audit-log detectors and callers +// outside this package can rely on errors.Is(err, ErrEncryptionKeyRequired). +func TestEncryptIfKeySet_PreservesErrEncryptionKeyRequiredSentinel(t *testing.T) { + if ErrEncryptionKeyRequired == nil { + t.Fatal("ErrEncryptionKeyRequired sentinel must be non-nil") + } + if ErrEncryptionKeyRequired.Error() == "" { + t.Fatal("ErrEncryptionKeyRequired must carry a non-empty message") + } + // Wrap it and confirm errors.Is unwraps correctly — real callers wrap with %w. + wrapped := wrapSentinel(ErrEncryptionKeyRequired) + if !errors.Is(wrapped, ErrEncryptionKeyRequired) { + t.Fatal("errors.Is must unwrap ErrEncryptionKeyRequired through %w-wrapped callers") + } +} + +// wrapSentinel is a tiny helper that mimics how production callers propagate +// the sentinel (e.g. fmt.Errorf("failed to encrypt config: %w", err)). +func wrapSentinel(err error) error { + return errors.Join(errors.New("failed to encrypt config"), err) } func TestEncryptProducesDifferentCiphertexts(t *testing.T) { diff --git a/internal/integration/lifecycle_test.go b/internal/integration/lifecycle_test.go index c76ce4d..1b3245a 100644 --- a/internal/integration/lifecycle_test.go +++ b/internal/integration/lifecycle_test.go @@ -66,7 +66,12 @@ func TestCertificateLifecycle(t *testing.T) { deploymentService := service.NewDeploymentService(jobRepo, targetRepo, agentRepo, certRepo, auditService, notificationService) jobService := service.NewJobService(jobRepo, renewalService, deploymentService, logger) agentService := service.NewAgentService(agentRepo, certRepo, jobRepo, targetRepo, auditService, issuerRegistry, renewalService) - issuerService := service.NewIssuerService(issuerRepo, auditService, issuerRegistry, nil, slog.Default()) + // 32-byte AES-256 test key — C-2 remediation makes IssuerService fail closed + // without a configured CERTCTL_CONFIG_ENCRYPTION_KEY. Happy-path CRUD tests + // must supply a real key so the encrypt path runs instead of returning + // ErrEncryptionKeyRequired. + testEncryptionKey := []byte("0123456789abcdef0123456789abcdef") + issuerService := service.NewIssuerService(issuerRepo, auditService, issuerRegistry, testEncryptionKey, slog.Default()) // Initialize handlers certificateHandler := handler.NewCertificateHandler(certificateService) diff --git a/internal/integration/negative_test.go b/internal/integration/negative_test.go index 9f60489..ffd4eaa 100644 --- a/internal/integration/negative_test.go +++ b/internal/integration/negative_test.go @@ -58,7 +58,12 @@ func setupTestServer(t *testing.T) (*httptest.Server, *mockCertificateRepository deploymentService := service.NewDeploymentService(jobRepo, targetRepo, agentRepo, certRepo, auditService, notificationService) jobService := service.NewJobService(jobRepo, renewalService, deploymentService, logger) agentService := service.NewAgentService(agentRepo, certRepo, jobRepo, targetRepo, auditService, issuerRegistry, renewalService) - issuerService := service.NewIssuerService(issuerRepo, auditService, issuerRegistry, nil, logger) + // 32-byte AES-256 test key — C-2 remediation makes IssuerService fail closed + // without a configured CERTCTL_CONFIG_ENCRYPTION_KEY. Happy-path CRUD tests + // must supply a real key so the encrypt path runs instead of returning + // ErrEncryptionKeyRequired. + testEncryptionKey := []byte("0123456789abcdef0123456789abcdef") + issuerService := service.NewIssuerService(issuerRepo, auditService, issuerRegistry, testEncryptionKey, logger) certificateHandler := handler.NewCertificateHandler(certificateService) issuerHandler := handler.NewIssuerHandler(issuerService) diff --git a/internal/service/issuer.go b/internal/service/issuer.go index 90ab537..56ee958 100644 --- a/internal/service/issuer.go +++ b/internal/service/issuer.go @@ -327,8 +327,20 @@ func (s *IssuerService) SeedFromEnvVars(ctx context.Context, cfg *config.Config) seeds := s.buildEnvVarSeeds(cfg) seeded := 0 for _, seed := range seeds { - // Encrypt the config if key is set - if len(seed.Config) > 0 { + // Encrypt the config only when an encryption key is configured. + // + // Env-seeded issuers carry Source="env" and are reconstructable on every + // boot from process environment, so persisting their config in plaintext + // adds no new exposure: the same bytes already live in the operator's + // deployment manifest. When no key is configured we therefore leave + // EncryptedConfig nil and keep the raw JSON in the `config` column — + // IssuerRegistry.Rebuild falls through to `cfg.Config` when there is no + // ciphertext to decrypt, so registry load still works. + // + // Database-sourced rows (Source="database") never reach this branch: + // they are created through the GUI/API write paths, which require the + // encryption key and fail closed via crypto.ErrEncryptionKeyRequired. + if len(seed.Config) > 0 && len(s.encryptionKey) > 0 { encrypted, _, encErr := crypto.EncryptIfKeySet([]byte(seed.Config), s.encryptionKey) if encErr != nil { s.logger.Error("failed to encrypt seed config", "id", seed.ID, "error", encErr) diff --git a/internal/service/issuer_test.go b/internal/service/issuer_test.go index 92d4d5a..15830bd 100644 --- a/internal/service/issuer_test.go +++ b/internal/service/issuer_test.go @@ -217,7 +217,7 @@ func TestIssuerService_Create(t *testing.T) { auditService := NewAuditService(auditRepo) registry := NewIssuerRegistry(slog.Default()) - service := NewIssuerService(repo, auditService, registry, nil, slog.Default()) + service := NewIssuerService(repo, auditService, registry, testEncryptionKey, slog.Default()) config := map[string]interface{}{"endpoint": "https://acme.example.com/v2/new-account"} configJSON, _ := json.Marshal(config) @@ -342,7 +342,7 @@ func TestIssuerService_Update(t *testing.T) { auditService := NewAuditService(auditRepo) registry := NewIssuerRegistry(slog.Default()) - service := NewIssuerService(repo, auditService, registry, nil, slog.Default()) + service := NewIssuerService(repo, auditService, registry, testEncryptionKey, slog.Default()) config := map[string]interface{}{"endpoint": "https://acme.example.com"} configJSON, _ := json.Marshal(config) @@ -568,7 +568,7 @@ func TestIssuerService_CreateIssuer_HandlerInterface(t *testing.T) { auditService := NewAuditService(auditRepo) registry := NewIssuerRegistry(slog.Default()) - service := NewIssuerService(repo, auditService, registry, nil, slog.Default()) + service := NewIssuerService(repo, auditService, registry, testEncryptionKey, slog.Default()) config := map[string]interface{}{"url": "https://example.com"} configJSON, _ := json.Marshal(config) @@ -680,7 +680,7 @@ func TestIssuerService_Create_LowercaseType(t *testing.T) { auditService := NewAuditService(auditRepo) registry := NewIssuerRegistry(slog.Default()) - service := NewIssuerService(repo, auditService, registry, nil, slog.Default()) + service := NewIssuerService(repo, auditService, registry, testEncryptionKey, slog.Default()) config := map[string]interface{}{"endpoint": "https://acme.example.com"} configJSON, _ := json.Marshal(config) @@ -710,7 +710,7 @@ func TestIssuerService_CreateIssuer_LowercaseType(t *testing.T) { auditService := NewAuditService(auditRepo) registry := NewIssuerRegistry(slog.Default()) - service := NewIssuerService(repo, auditService, registry, nil, slog.Default()) + service := NewIssuerService(repo, auditService, registry, testEncryptionKey, slog.Default()) config := map[string]interface{}{"url": "https://example.com"} configJSON, _ := json.Marshal(config) @@ -752,7 +752,7 @@ func TestIssuerService_Create_M49Types(t *testing.T) { auditService := NewAuditService(auditRepo) registry := NewIssuerRegistry(slog.Default()) - service := NewIssuerService(repo, auditService, registry, nil, slog.Default()) + service := NewIssuerService(repo, auditService, registry, testEncryptionKey, slog.Default()) config := map[string]interface{}{"api_url": "https://example.com"} configJSON, _ := json.Marshal(config) diff --git a/internal/service/target_test.go b/internal/service/target_test.go index 833f17c..d0a9677 100644 --- a/internal/service/target_test.go +++ b/internal/service/target_test.go @@ -18,7 +18,7 @@ func newTestTargetService() (*TargetService, *mockTargetRepo, *mockAuditRepo, *m auditSvc := NewAuditService(auditRepo) agentRepo := &mockAgentRepo{Agents: make(map[string]*domain.Agent), HeartbeatUpdates: make(map[string]time.Time)} logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) - return NewTargetService(targetRepo, auditSvc, agentRepo, nil, logger), targetRepo, auditRepo, agentRepo + return NewTargetService(targetRepo, auditSvc, agentRepo, testEncryptionKey, logger), targetRepo, auditRepo, agentRepo } func TestTargetService_List_Success(t *testing.T) { diff --git a/internal/service/testutil_test.go b/internal/service/testutil_test.go index 8bde840..0d6dece 100644 --- a/internal/service/testutil_test.go +++ b/internal/service/testutil_test.go @@ -12,6 +12,13 @@ import ( var errNotFound = errors.New("not found") +// testEncryptionKey is a deterministic 32-byte AES-256 key for unit tests that +// exercise IssuerService/TargetService write paths. After the C-2 remediation +// these services fail closed when no key is configured, so happy-path tests +// must supply a real key. Using a constant keeps wire-format assertions stable +// across runs and avoids flaky PBKDF2 timing. +var testEncryptionKey = []byte("0123456789abcdef0123456789abcdef") // 32 bytes + // mockCertRepo is a test implementation of CertificateRepository type mockCertRepo struct { Certs map[string]*domain.ManagedCertificate