Skip to content

refactor(config): reject unusable APP_ENCRYPTION_KEY at startup - #1201

Open
nbmaiti wants to merge 2 commits into
mainfrom
fix/validate-encryption-key
Open

refactor(config): reject unusable APP_ENCRYPTION_KEY at startup#1201
nbmaiti wants to merge 2 commits into
mainfrom
fix/validate-encryption-key

Conversation

@nbmaiti

@nbmaiti nbmaiti commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Issue

A hand-supplied APP_ENCRYPTION_KEY was never checked. A wrong-sized key only
failed at first use — an opaque HTTP 500 (crypto/aes: invalid key size 15)
that the web UI never surfaced to the user — and a zero-entropy key like
aaaaaaaaaaaaaaaa was accepted outright, silently negating the encryption of
device credentials.

Changes

  • config.NewConfig now validates a non-empty key and refuses to start, naming
    the offending setting and suggesting openssl rand -base64 24.
  • Rules: 16, 24 or 32 characters (AES-128/192/256), ≥8 distinct characters,
    ≥3 bits of Shannon entropy per character, no whole-key repetition, no run of
    6 sequential code points. Thresholds clear the key Console generates for
    itself (24 random bytes → 32 base64 chars) and any random 16-char key with
    margin.
  • Keys loaded from the secret store / OS keyring may predate this check, so they
    are validated where they are loaded: wrong size is fatal (it can never
    encrypt), weak-but-usable only warns — exiting would lock the operator out of
    credentials already encrypted with that key.
  • .env.example documents the requirement.

Testing

go test -race -count=1 ./... green; go vet clean. Validator tests cover the
three aaaa… examples from the report and assert 100 generated keys pass.

Fixes: #NNNN

@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.92771% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.28%. Comparing base (ec14602) to head (67621f7).

Files with missing lines Patch % Lines
cmd/app/main.go 45.00% 10 Missing and 1 partial ⚠️
config/encryption_key.go 92.72% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1201      +/-   ##
==========================================
+ Coverage   50.09%   50.28%   +0.19%     
==========================================
  Files         146      147       +1     
  Lines       13552    13635      +83     
==========================================
+ Hits         6789     6857      +68     
- Misses       6171     6183      +12     
- Partials      592      595       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Tightens startup validation around APP_ENCRYPTION_KEY so invalid or dangerously weak keys are caught early (with clearer operator guidance), and adds validation when loading keys from the secret store/keyring to avoid runtime encryption failures.

Changes:

  • Introduces ValidateEncryptionKey with length + strength checks and unit tests for common weak patterns and generated keys.
  • Validates hand-supplied keys during config.NewConfig() and surfaces actionable errors (including an openssl rand -base64 24 suggestion).
  • Validates stored keys on load in cmd/app, treating wrong-sized keys as fatal and weak keys as a warning; documents requirements in .env.example.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
config/encryption_key.go Adds encryption-key validation logic (length + strength heuristics).
config/encryption_key_test.go Adds unit tests for the validator and NewConfig() behavior with env-supplied keys.
config/config.go Enforces validation of non-empty APP_ENCRYPTION_KEY at config load time with a clearer error message.
cmd/app/main.go Validates keys loaded from secret store/keyring; fatal on unusable size, warn on weak-but-usable.
cmd/app/main_test.go Adds coverage for checkStoredEncryptionKey warning/success paths.
.env.example Documents encryption key expectations and generation guidance.
Suppressed comments (1)

config/encryption_key.go:92

  • for i := range len(key) does not compile in Go. This loop should iterate over the string indices.
	counts := make(map[byte]int, len(key))
	for i := range len(key) {
		counts[key[i]]++
	}

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread config/encryption_key.go
Comment thread config/encryption_key_test.go
@nbmaiti
nbmaiti force-pushed the fix/validate-encryption-key branch from ee74bb6 to 0a09e19 Compare August 13, 2026 15:50
nbmaiti and others added 2 commits August 17, 2026 10:45
A hand-supplied APP_ENCRYPTION_KEY was accepted without any check, so a
key of the wrong size only failed at first use, as an opaque HTTP 500
("crypto/aes: invalid key size 15") that the web UI never surfaced. A
key with no entropy ("aaaaaaaaaaaaaaaa") was accepted outright and
silently negated the encryption of device credentials.

Validate the key in config.NewConfig instead, so Console refuses to
start and names the offending setting. The key must be 16, 24 or 32
characters (AES-128/192/256) and must not be repetitive: at least 8
distinct characters, 3 bits of Shannon entropy per character, no
whole-key repetition and no run of 6 sequential code points. The
thresholds clear the generated key (24 random bytes, base64-encoded)
and any random 16-character key with margin.

Keys read back from the secret store or the OS keyring may predate this
check, so they are validated where they are loaded: a wrong-sized key is
fatal because it can never encrypt anything, while a weak but usable key
only warns - exiting would leave the operator unable to start Console
and read credentials already encrypted with it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
Iterate key bytes explicitly in distinctChars and entropyBitsPerChar,
rebind the loop variable in table-driven subtests per repo convention,
and correct the paralleltest suppressions that golangci-lint reported
as unused.

Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
@nbmaiti
nbmaiti force-pushed the fix/validate-encryption-key branch from 0762884 to 67621f7 Compare August 17, 2026 05:15
@nbmaiti
nbmaiti requested a lite review from Copilot August 17, 2026 05:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

config/encryption_key.go:48

  • The doc comment says callers should skip an empty key, but ValidateEncryptionKey currently treats an empty string as an invalid length (and tests assert that). Please update the comment to match the actual behavior to avoid confusion for future callers.
//
// Callers are responsible for skipping the empty key, which means "no key
// supplied" rather than "invalid key" — see handleEncryptionKey in cmd/app.

@nbmaiti
nbmaiti marked this pull request as ready for review August 17, 2026 06:26
@nbmaiti
nbmaiti requested a review from a team as a code owner August 17, 2026 06:27
@nbmaiti
nbmaiti requested a review from sudhir-intc August 17, 2026 06:27
@sudhir-intc sudhir-intc changed the title fix(config): reject unusable APP_ENCRYPTION_KEY at startup refactor(config): reject unusable APP_ENCRYPTION_KEY at startup Aug 17, 2026
Comment thread config/encryption_key.go
//
// Callers are responsible for skipping the empty key, which means "no key
// supplied" rather than "invalid key" — see handleEncryptionKey in cmd/app.
func ValidateEncryptionKey(key string) error {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See if you could use any of the go pacakge for this validation. One usage could be around using the aes.NewCipher

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants