mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 13:51:36 +00:00
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:
@@ -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)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user