Default MasterKey enforcement to Strict so an empty key refuses startup#444
Merged
Conversation
The MasterKey enforcement default was Warn: an empty / too-short / all-zero key logged a warning and proceeded with a recoverable 0-byte-derived key, so at-rest encryption was "theater by default" on a misconfigured deploy. Flip the default to Strict — a missing or weak key now refuses startup rather than silently encrypting under a recoverable key. Non-breaking for real deployments: they always configure a real master key, so the Strict default never fires for them; it only catches a genuine misconfiguration (key never set). Operators who deliberately run without at-rest protection opt out with SQUID_MASTER_KEY_ENFORCEMENT=warn|off. Scoped to MasterKey only — passes Strict explicitly from ReadEnforcementMode rather than changing the shared EnforcementModeReader.Read default, so the other hardening consumers (cert validation, sensitive-variable decrypt) keep their Warn default. The committed appsettings MasterKey stays intentionally empty so a real deploy must supply its own; the E2E test config's previously all-zero key is replaced with a real random key so the E2E container still boots under Strict. Tests: invert the constructor backward-compat unit test (empty key now throws) + add valid-key-boots and warn-opt-out companions (hermetic, env-var controlled in the serialised collection); add an integration test that the real DI container boots under the Strict default with the configured key and round-trips a secret. Full unit suite (6037) + at-rest integration green.
…rd, hermetic env, docs Adversarial review surfaced five fix-optional items, all addressed: - The integration test proved only "a configured key works" — a valid key passes ValidateMasterKey in every mode, so it would not catch the default regressing to Warn. Add a container-tier NEGATIVE test: override the container's SecuritySetting with an empty MasterKey and assert IVariableEncryptionService refuses to construct under the (unset) Strict default. Soften the positive test's name/message to claim only what it proves. - The constructor tests mutate the process-global SQUID_MASTER_KEY_ENFORCEMENT env var. Two other classes that also construct the service (VariableEncryptionServiceB10Tests, DeploymentAccountCredentialsEncryptionTests) were outside the serialised collection — benign today (they use real keys) but a future flake trap. Move them into the serialised collection. - "Refuses startup" was inaccurate: the encryption service is scoped, so the throw surfaces on first construction (first deploy / secret operation), not at process boot. Reword the code comments + appsettings to say so. - Update the docs/HARDENING_TRACKING.md B.1 row, which still documented the old Warn default and claimed "NOT A BREAKING CHANGE". - Reword the stale "Warn mode (default)" comments in the unit test that contradicted the now-Strict default. Full unit suite (6037) + master-key integration (positive + negative) green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Warn: an empty / too-short / all-zero key logged a warning and proceeded with a recoverable 0-byte-derived key, so at-rest encryption was theater by default on a misconfigured deploy. Flip the default toStrict— a missing or weak key now refuses startup instead of silently encrypting under a recoverable key.SQUID_MASTER_KEY_ENFORCEMENT=warn(allow + log) or=off(silent).ReadEnforcementMode()passesStrictexplicitly rather than changing the sharedEnforcementModeReader.Readdefault, so the other hardening consumers (cert validation, sensitive-variable decrypt) keep theirWarndefault.appsettings.jsonMasterKey stays intentionally empty so a real deploy must supply its own. The E2E test config's previously all-zero key is replaced with a real random key so the E2E container still boots under Strict.This completes the deferred dependency that made the P2 at-rest encryption cluster (#437/#438/#439) genuinely protective rather than cosmetic-by-default.
Behaviour change & opt-out
Opt-out preserved:
SQUID_MASTER_KEY_ENFORCEMENT=warnor=offrestores the prior lenient behaviour. The other hardening env vars are unaffected.Test plan
Constructor_ValidMasterKey_DoesNotThrow(happy path boots) andConstructor_EmptyMasterKey_WarnOptOut_DoesNotThrow(opt-out). Hermetic: env var saved/cleared/restored, class moved into the serialised collection so the process-wide mutation can't race.ValidateMasterKey(..., Strict)matrix already pins empty/short/all-zero → throw.IntegrationMasterKeyStrictDefault: the container boots under the Strict default with the configured real key and round-trips a secret through the V2 envelope.