From 32c430d670d3cd12ddfd88c7997387fb4c9a247f Mon Sep 17 00:00:00 2001 From: Manish Kumar Date: Thu, 13 Aug 2026 20:11:59 -0500 Subject: [PATCH] fix(secrets): generate LIMSX_FIELD_ENCRYPTION_KEY on install Fresh Studio installations could not start LIMS. The Electron app never provisioned LIMSX_FIELD_ENCRYPTION_KEY, so compose interpolated it to the empty string (it is wired as a bare ${VAR}, with no `:?` guard, so compose only warns and exits 0), and omnibioai-lims' settings.py then raised "FIELD_ENCRYPTION_KEY must be set in non-debug environments" at import -- crash-looping the container. Three compounding defects, all in this repo: 1. Never generated. 951ad25 wired the variable into compose without a `:?` guard; fad98ff built SECRET_DEFAULTS by enumerating the `${VAR:?...}` required vars, so this key was structurally invisible to that audit. 2. Actively erased. writeEnvFile() rewrites .env wholesale from a fixed list of lines that omitted the key, so even a hand-added value was destroyed on the next settings save. 3. Misdocumented. DEPLOYMENT.md told operators to set FIELD_ENCRYPTION_KEY, but every compose file reads LIMSX_FIELD_ENCRYPTION_KEY -- following the runbook verbatim had no effect. Generating it as plain hex like every other secret would NOT have worked: LIMS builds a `cryptography` Fernet from this value, which requires exactly 32 url-safe-base64-encoded bytes (44 chars). A 64-char hex key satisfies LIMS's own startup guard (which only checks non-emptiness) and then throws ValueError on the first encrypted-field write -- and core/fields.py's read path swallows that into a silent None, so encrypted data would read back blank. Hence SECRET_GENERATORS: a per-key format override, defaulting to the existing hex behavior for every other secret. The key's SECRET_DEFAULTS entry is `null` -- unlike the other credentials it never had a weak literal to rotate away from, so only a genuinely unset value is generated and an operator-supplied key is preserved verbatim. LIMS's fail-fast guard is deliberately unchanged: it was always correct, nothing ever satisfied it. Verified end to end -- generateSecrets() -> compose -> real LIMS production settings import -> real Fernet encrypt/decrypt roundtrip. Tests assert on key shape/properties only and never print a generated value. Co-Authored-By: Claude Sonnet 5 --- .env.example | 9 +++ DEPLOYMENT.md | 9 ++- electron/main.js | 5 ++ electron/secrets.js | 50 +++++++++++- tests/test_secret_generation.js | 132 ++++++++++++++++++++++++++++++++ 5 files changed, 199 insertions(+), 6 deletions(-) diff --git a/.env.example b/.env.example index 63b2733..166092c 100644 --- a/.env.example +++ b/.env.example @@ -32,6 +32,15 @@ LICENSE_SECRET=change-me-in-production # AUTO-GENERATED on first launch — do not share # Signs LIMS's own session cookies (distinct from AUTH_SECRET_KEY) LIMSX_DJANGO_SECRET_KEY=change-me-in-production +# AUTO-GENERATED on first launch — do not share +# Fernet key encrypting LIMS field-level data at rest (EncryptedCharField). +# Unlike the secrets above this is NOT an opaque token: it must be exactly +# 32 url-safe base64-encoded bytes, or LIMS raises +# "Fernet key must be 32 url-safe base64-encoded bytes" on first use. +# Generate with: +# python3 -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())" +# Changing this after data exists makes already-encrypted values unreadable. +LIMSX_FIELD_ENCRYPTION_KEY= # ── LIMS ─────────────────────────────────────────────── LIMS_USERNAME=admin diff --git a/DEPLOYMENT.md b/DEPLOYMENT.md index 50fca1e..3aac4b6 100644 --- a/DEPLOYMENT.md +++ b/DEPLOYMENT.md @@ -71,8 +71,11 @@ LIMSX_DJANGO_SECRET_KEY= +LIMSX_FIELD_ENCRYPTION_KEY= # ── RAG ─────────────────────────────────────────────────────────────────────── # generate: python3 -c "import secrets; print(secrets.token_urlsafe(32))" @@ -124,7 +127,7 @@ DB_INIT_DIR=/home//Desktop/machine/db-init # Django / Auth secret key python3 -c "import secrets; print(secrets.token_urlsafe(50))" -# Fernet encryption key (FIELD_ENCRYPTION_KEY) +# Fernet encryption key (LIMSX_FIELD_ENCRYPTION_KEY) python3 -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())" # API token (RAGBIO_API_KEY) @@ -136,7 +139,7 @@ python3 -c "import secrets; print(secrets.token_urlsafe(32))" ## Pre-Deployment Checklist - [ ] All environment variables set in `.env` -- [ ] `FIELD_ENCRYPTION_KEY` generated and stored in secrets manager (Vault / AWS Secrets Manager) +- [ ] `LIMSX_FIELD_ENCRYPTION_KEY` generated and stored in secrets manager (Vault / AWS Secrets Manager) - [ ] `RAGBIO_API_KEY` generated and stored in secrets manager - [ ] `LIMSX_DJANGO_SECRET_KEY` generated and stored in secrets manager - [ ] `AUTH_SECRET_KEY` generated and stored in secrets manager diff --git a/electron/main.js b/electron/main.js index 5347d14..ef391e7 100644 --- a/electron/main.js +++ b/electron/main.js @@ -194,6 +194,11 @@ function writeEnvFile(config) { `MYSQL_ROOT_PASSWORD=${existing.MYSQL_ROOT_PASSWORD || ''}`, `MYSQL_DEFAULT_DB=omnibioai`, `LIMSX_DJANGO_SECRET_KEY=${existing.LIMSX_DJANGO_SECRET_KEY || ''}`, + // Fernet key for LIMS's EncryptedCharField. Must be carried through + // here like every other generated secret -- this function rewrites + // .env wholesale from this list, so omitting it would erase the + // generated key and crash-loop the LIMS container on next start. + `LIMSX_FIELD_ENCRYPTION_KEY=${existing.LIMSX_FIELD_ENCRYPTION_KEY || ''}`, `AUTH_SECRET_KEY=${existing.AUTH_SECRET_KEY || ''}`, `GF_ADMIN_PASSWORD=${existing.GF_ADMIN_PASSWORD || ''}`, `GF_STUDIO_TOKEN=${existing.GF_STUDIO_TOKEN || ''}`, diff --git a/electron/secrets.js b/electron/secrets.js index 385c25b..2bfb1d0 100644 --- a/electron/secrets.js +++ b/electron/secrets.js @@ -23,6 +23,17 @@ const crypto = require("crypto"); // session-cookie signing key, and the Jupyter/RStudio/VSCode terminal // credentials), since nothing ever rotated them. Added here to close that // gap -- see docker-compose.release.yml's matching ${VAR:?...} guards. +// LIMSX_FIELD_ENCRYPTION_KEY was likewise missing for every release prior +// to this fix, but for a different reason and with a different failure +// mode. It is wired into all three compose files as a bare +// ${LIMSX_FIELD_ENCRYPTION_KEY} (no `:?` guard), so the audit that built +// the list above -- which enumerated the `${VAR:?...}` required vars -- +// never saw it. An unset value therefore interpolates to the empty string +// and compose starts happily, but omnibioai-lims' settings.py raises +// `FIELD_ENCRYPTION_KEY must be set in non-debug environments` at import +// and the LIMS container crash-loops on a fresh install. It has no weak +// literal to rotate away from (it never had a default at all), so its +// entry below is `null`. const SECRET_DEFAULTS = { AUTH_SECRET_KEY: "change-me", MYSQL_ROOT_PASSWORD: "omnibioai", @@ -33,8 +44,33 @@ const SECRET_DEFAULTS = { RSTUDIO_PASSWORD: "omnibioai", VSCODE_PASSWORD: "omnibioai", ADMIN_KEY: "admin-secret", + LIMSX_FIELD_ENCRYPTION_KEY: null, }; +// Secrets whose *format* is constrained by their consumer, rather than +// being an opaque random token. Anything not listed here gets the default +// 32-byte hex treatment. +// +// LIMSX_FIELD_ENCRYPTION_KEY feeds omnibioai-lims' EncryptedCharField +// (core/fields.py), which constructs a `cryptography` Fernet from it. +// Fernet requires exactly 32 url-safe-base64-encoded bytes -- a 44-char +// string ending in `=`. The default hex encoding used for every other +// secret here produces 64 chars and is *rejected* by Fernet, and does so +// in a way that would slip past LIMS's own startup guard (which only +// checks the value is non-empty): Django would boot fine and then throw +// `ValueError: Fernet key must be 32 url-safe base64-encoded bytes` on the +// first encrypted-field write. Generating the right shape here is what +// makes the value actually usable, not merely present. +const SECRET_GENERATORS = { + LIMSX_FIELD_ENCRYPTION_KEY: () => + crypto.randomBytes(32).toString("base64url").padEnd(44, "="), +}; + +function generateSecretValue(key) { + const generator = SECRET_GENERATORS[key]; + return generator ? generator() : crypto.randomBytes(32).toString("hex"); +} + function parseEnvFile(envPath) { const env = {}; if (fs.existsSync(envPath)) { @@ -57,8 +93,10 @@ function generateSecrets(envPath) { let changed = false; for (const [key, defaultVal] of Object.entries(SECRET_DEFAULTS)) { - if (!env[key] || env[key] === defaultVal) { - env[key] = crypto.randomBytes(32).toString("hex"); + // `defaultVal === null` means the secret never had a weak literal to + // rotate away from -- only a genuinely unset value needs generating. + if (!env[key] || (defaultVal !== null && env[key] === defaultVal)) { + env[key] = generateSecretValue(key); changed = true; } } @@ -74,4 +112,10 @@ function generateSecrets(envPath) { return changed; } -module.exports = { SECRET_DEFAULTS, parseEnvFile, generateSecrets }; +module.exports = { + SECRET_DEFAULTS, + SECRET_GENERATORS, + generateSecretValue, + parseEnvFile, + generateSecrets, +}; diff --git a/tests/test_secret_generation.js b/tests/test_secret_generation.js index 59c8220..d9d013c 100644 --- a/tests/test_secret_generation.js +++ b/tests/test_secret_generation.js @@ -20,6 +20,7 @@ const path = require("node:path"); const { SECRET_DEFAULTS, + SECRET_GENERATORS, parseEnvFile, generateSecrets, } = require("../electron/secrets.js"); @@ -40,8 +41,20 @@ const COMPOSE_REQUIRED = [ "JUPYTER_TOKEN", "RSTUDIO_PASSWORD", "VSCODE_PASSWORD", + // Not `:?`-guarded in compose (so a missing value interpolates to "" and + // compose still starts), but omnibioai-lims' settings.py raises at import + // when it is empty -- the LIMS container crash-loops instead. Just as + // install-blocking as the guarded vars above, which is exactly why it was + // missed: the audit that built this list enumerated the `:?` vars. + "LIMSX_FIELD_ENCRYPTION_KEY", ]; +// Secrets generated as plain 32-byte hex. Anything in SECRET_GENERATORS has +// a consumer-imposed format instead and is asserted separately below. +const HEX_SECRETS = Object.keys(SECRET_DEFAULTS).filter( + (k) => !(k in SECRET_GENERATORS) +); + test("provisions every credential the release compose file requires", () => { for (const key of COMPOSE_REQUIRED) { assert.ok( @@ -62,6 +75,8 @@ test("generates secrets into an empty/missing .env", () => { const env = parseEnvFile(envPath); for (const key of Object.keys(SECRET_DEFAULTS)) { assert.ok(env[key], `${key} should have been generated`); + } + for (const key of HEX_SECRETS) { assert.strictEqual( env[key].length, 64, @@ -78,6 +93,7 @@ test("rotates values still set to the known-weak literals", () => { fs.writeFileSync( envPath, Object.entries(SECRET_DEFAULTS) + .filter(([, v]) => v !== null) .map(([k, v]) => `${k}=${v}`) .join("\n") + "\n" ); @@ -87,11 +103,14 @@ test("rotates values still set to the known-weak literals", () => { const env = parseEnvFile(envPath); for (const [key, weak] of Object.entries(SECRET_DEFAULTS)) { + if (weak === null) continue; assert.notStrictEqual( env[key], weak, `${key} must not still hold its known-weak default` ); + } + for (const key of HEX_SECRETS) { assert.match(env[key], /^[0-9a-f]{64}$/); } }); @@ -171,3 +190,116 @@ test("parseEnvFile handles values containing '='", () => { test("parseEnvFile returns empty object for a missing file", () => { assert.deepStrictEqual(parseEnvFile(tmpEnvPath()), {}); }); + +// ── LIMS field-encryption key (Fernet format) ──────────────────────────── +// +// Generating this one as plain hex like every other secret would satisfy +// LIMS's own startup guard (which only checks non-emptiness) and then fail +// at the first encrypted-field write with "Fernet key must be 32 url-safe +// base64-encoded bytes". These assert the shape `cryptography`'s Fernet +// actually accepts, without ever printing a generated key. + +test("generates LIMSX_FIELD_ENCRYPTION_KEY in valid Fernet format", () => { + const envPath = tmpEnvPath(); + generateSecrets(envPath); + const key = parseEnvFile(envPath).LIMSX_FIELD_ENCRYPTION_KEY; + + assert.ok(key, "LIMSX_FIELD_ENCRYPTION_KEY must be generated"); + assert.strictEqual( + key.length, + 44, + "a Fernet key is exactly 44 characters (32 bytes url-safe base64 + padding)" + ); + assert.match( + key, + /^[A-Za-z0-9_-]{43}=$/, + "must be url-safe base64 (-_ alphabet, no +/) with '=' padding -- " + + "hex or standard base64 would be rejected by Fernet" + ); + // Decodes back to exactly 32 bytes, which is the actual Fernet contract. + assert.strictEqual( + Buffer.from(key, "base64").length, + 32, + "must decode to exactly 32 bytes" + ); +}); + +test("LIMSX_FIELD_ENCRYPTION_KEY is not hex-formatted", () => { + // Direct regression guard on the specific wrong fix: adding the key to + // SECRET_DEFAULTS without a format-aware generator. + const envPath = tmpEnvPath(); + generateSecrets(envPath); + const key = parseEnvFile(envPath).LIMSX_FIELD_ENCRYPTION_KEY; + + assert.doesNotMatch( + key, + /^[0-9a-f]{64}$/, + "a 64-char hex key passes LIMS's non-empty startup guard but throws " + + "ValueError on the first encrypted-field write -- it must be Fernet-shaped" + ); +}); + +test("LIMSX_FIELD_ENCRYPTION_KEY survives relaunch and differs per install", () => { + const envA = tmpEnvPath(); + const envB = tmpEnvPath(); + generateSecrets(envA); + const firstA = parseEnvFile(envA).LIMSX_FIELD_ENCRYPTION_KEY; + + // Rotating this value on relaunch would orphan every already-encrypted + // field -- the data would silently read back as null. + generateSecrets(envA); + assert.strictEqual( + parseEnvFile(envA).LIMSX_FIELD_ENCRYPTION_KEY, + firstA, + "must not rotate on a subsequent launch -- existing ciphertext would " + + "become permanently unreadable" + ); + + generateSecrets(envB); + assert.notStrictEqual( + parseEnvFile(envB).LIMSX_FIELD_ENCRYPTION_KEY, + firstA, + "each installation must get its own encryption key" + ); +}); + +test("main.js writeEnvFile carries every generated secret through", () => { + // writeEnvFile() rewrites .env wholesale from a fixed list of lines. Any + // generated secret missing from that list is erased on the next config + // save -- which is how LIMSX_FIELD_ENCRYPTION_KEY would have been wiped + // even after being generated correctly. Static source check: main.js + // pulls in Electron APIs at require() time and cannot be imported here. + const mainSrc = fs.readFileSync( + path.join(__dirname, "..", "electron", "main.js"), + "utf8" + ); + + for (const key of Object.keys(SECRET_DEFAULTS)) { + if (key === "ADMIN_KEY") continue; // dead var, retained only for rotation + assert.ok( + mainSrc.includes(`${key}=\${existing.${key}`), + `electron/main.js's writeEnvFile() must preserve ${key} from the ` + + `existing .env -- omitting it silently erases the generated value ` + + `the next time settings are saved` + ); + } +}); + +test("an operator-supplied Fernet key is never overwritten", () => { + const envPath = tmpEnvPath(); + // A key an operator generated themselves per DEPLOYMENT.md. Throwaway, + // structurally valid, not a real deployment value. + const supplied = require("node:crypto") + .randomBytes(32) + .toString("base64url") + .padEnd(44, "="); + fs.writeFileSync(envPath, `LIMSX_FIELD_ENCRYPTION_KEY=${supplied}\n`); + + generateSecrets(envPath); + + assert.strictEqual( + parseEnvFile(envPath).LIMSX_FIELD_ENCRYPTION_KEY, + supplied, + "an operator-provided encryption key must be preserved verbatim" + ); +});