security: fail closed when CERTCTL_CONFIG_ENCRYPTION_KEY is unset (fixes C-2)

EncryptIfKeySet/DecryptIfKeySet in internal/crypto/encryption.go previously
returned plaintext + wasEncrypted=false when the operator had not configured
CERTCTL_CONFIG_ENCRYPTION_KEY. That produced a data-at-rest confidentiality
bypass (CWE-311): sensitive fields on dynamically-configured issuer and
target rows (source='database') were persisted to PostgreSQL without any
encryption, and no caller could distinguish the encrypted from the plaintext
branch at runtime. The only visible signal was a single warning log line
emitted once at startup.

Fail closed instead:

- EncryptIfKeySet / DecryptIfKeySet now return crypto.ErrEncryptionKeyRequired
  (a new exported sentinel, errors.Is-unwrappable) when the key is empty or
  nil, rather than silently emitting plaintext. The (result, wasEncrypted,
  err) tuple signature is preserved for source compatibility; only the
  semantics of the no-key branch changed.

- cmd/server/main.go grows a startup pre-flight check: if no encryption key
  is configured the server lists issuers and targets, counts rows with
  source='database', and refuses to start (os.Exit(1)) if any exist. Operators
  must either configure CERTCTL_CONFIG_ENCRYPTION_KEY or remove the exposed
  rows before the control plane can boot. The warning-only path is retained
  for the clean-slate case (no database rows).

- internal/service/issuer.go's SeedFromEnvVars now guards the encryption call
  with len(s.encryptionKey) > 0 so env-seeded rows (source='env', which are
  reconstructable on every boot from process env) continue to persist as
  plaintext in the 'config' column when no key is configured. Registry load
  already falls through to cfg.Config when EncryptedConfig is nil. GUI/API
  write paths (source='database') remain fail-closed via propagation of
  ErrEncryptionKeyRequired.

- Integration tests that exercise CreateIssuer via the handler layer now
  supply a real 32-byte AES-256 test key so the encrypt path runs instead of
  returning ErrEncryptionKeyRequired. Same pattern in internal/service/
  testutil_test.go for consolidated service-layer tests.

- internal/crypto/encryption_test.go grows regression guards:
  TestEncryptIfKeySet_EmptyKeyFailsClosed (nil_key + empty_key subtests),
  TestDecryptIfKeySet_EmptyKeyFailsClosed (nil_key + empty_key subtests),
  TestEncryptDecryptIfKeySet_RoundTripProducesDifferentCiphertext,
  TestDecryptIfKeySet_RejectsTamperedCiphertext, and
  TestEncryptIfKeySet_PreservesErrEncryptionKeyRequiredSentinel (verifies
  the sentinel unwraps through fmt.Errorf(%w)-style wrapping).

Wire format is unchanged: AES-256-GCM Encrypt/Decrypt/DeriveKey, the
12-byte nonce prefix, the GCM auth tag, the PBKDF2 salt
('certctl-config-encryption-v1'), and the 100,000 iteration count are all
byte-identical. Ciphertexts produced before this change remain decryptable.

Verified:
- go build ./... : clean
- go vet ./...   : clean
- go test -race ./internal/crypto/... ./internal/service/... \
    ./internal/integration/... ./cmd/server/... : pass
- golangci-lint run ./... : 0 issues
- govulncheck ./... : 0 reachable vulnerabilities
- rg 'return plaintext, false, nil' internal/ : no matches
- Coverage: crypto 85.0% (unchanged), service 67.8% (was 67.9%, noise),
  cmd/server 0.0% (unchanged baseline). All above CI thresholds.

See certctl-audit-report.md for the full finding record and resolution log.
This commit is contained in:
Shankar Reddy
2026-04-16 21:10:40 +00:00
parent 0e8e6926eb
commit fb4ce1a243
9 changed files with 238 additions and 31 deletions
+39 -1
View File
@@ -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)
+35 -5
View File
@@ -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)
}
+124 -14
View File
@@ -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) {
+6 -1
View File
@@ -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)
+6 -1
View File
@@ -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)
+14 -2
View File
@@ -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)
+6 -6
View File
@@ -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)
+1 -1
View File
@@ -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) {
+7
View File
@@ -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