Wait for the CRNG before minting the vault volume passphrase - #104
Conversation
WalkthroughThe change switches LUKS bootstrap entropy to blocking ChangesRNG hygiene enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/check-rng-hygiene.sh (2)
77-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIterate the file list with
read, not unquoted expansion.
for f in $SOURCESsplits on IFS and applies globbing. A tracked path with a space or a glob character produces wrong file names, and awk then fails, which turns into a scanner error. Awhile readloop removes that failure mode.♻️ Proposed loop change
preprocess() { local f rc out rc=0 - for f in $SOURCES; do + while IFS= read -r f; do + [ -n "$f" ] || continue out=$(awk -v fname="$f" -v optout="$OPT_OUT" ' @@ ' "$f") || rc=2 [ -n "$out" ] && printf '%s\n' "$out" - done + done <<EOF +$SOURCES +EOF return "$rc" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-rng-hygiene.sh` around lines 77 - 99, Update preprocess() to iterate over SOURCES using a read-based loop rather than unquoted word expansion, preserving each source path exactly—including spaces and glob characters. Keep the existing awk processing, output handling, and return-code behavior unchanged while ensuring the loop correctly handles all listed filenames.
143-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePin the length comparison, not only the error text.
Rule 3 passes if
refusing to formatstays but the comparison changes, for example to-ge 1. Match the assertion as well to make the invariant hard to weaken.♻️ Proposed check
if [ -f nixos/frost-gate.nix ]; then - if ! grep -q 'refusing to format' nixos/frost-gate.nix; then + if ! grep -q 'refusing to format' nixos/frost-gate.nix \ + || ! grep -qE '\-eq 44' nixos/frost-gate.nix; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-rng-hygiene.sh` around lines 143 - 156, Update rule 3 in the script to grep for the specific passphrase length comparison as well as the existing “refusing to format” text. Require the expected equality check against the full 32-byte length, so changing the comparison to a weaker condition causes the hygiene check to fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 58-61: Update the actions/checkout step in the CI workflow to
disable credential persistence, while keeping the existing rng-hygiene step
unchanged. Also configure branch protection so the rng-hygiene status check is
required for merges.
In `@scripts/check-rng-hygiene.sh`:
- Around line 110-120: Move scanner failure handling out of scanner_died and
into both scan command-substitution call sites, including the invocations around
the existing status checks near lines 131 and 138. Ensure a failed awk/scan
substitution is detected by the parent shell and exits nonzero before reporting
“RNG hygiene: OK”; preserve normal finding collection and successful scans.
---
Nitpick comments:
In `@scripts/check-rng-hygiene.sh`:
- Around line 77-99: Update preprocess() to iterate over SOURCES using a
read-based loop rather than unquoted word expansion, preserving each source path
exactly—including spaces and glob characters. Keep the existing awk processing,
output handling, and return-code behavior unchanged while ensuring the loop
correctly handles all listed filenames.
- Around line 143-156: Update rule 3 in the script to grep for the specific
passphrase length comparison as well as the existing “refusing to format” text.
Require the expected equality check against the full 32-byte length, so changing
the comparison to a weaker condition causes the hygiene check to fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d69a7703-8175-4121-8459-b6c44a59c384
📒 Files selected for processing (4)
.github/workflows/ci.ymldocs/SECURITY.mdnixos/frost-gate.nixscripts/check-rng-hygiene.sh
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - name: Check for silent RNG fallbacks | ||
| run: ./scripts/check-rng-hygiene.sh |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable credential persistence for this checkout.
The job only reads tracked files. It never pushes. actions/checkout writes the job token into .git/config by default, which leaves it readable by any later step or tool in the job.
Also confirm that rng-hygiene is a required status check in branch protection. Otherwise a failing guard does not block a merge.
🔒 Proposed fix
steps:
- - uses: actions/checkout@v6
+ - uses: actions/checkout@v6
+ with:
+ persist-credentials: false
- name: Check for silent RNG fallbacks
run: ./scripts/check-rng-hygiene.sh📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| steps: | |
| - uses: actions/checkout@v6 | |
| - name: Check for silent RNG fallbacks | |
| run: ./scripts/check-rng-hygiene.sh | |
| steps: | |
| - uses: actions/checkout@v6 | |
| with: | |
| persist-credentials: false | |
| - name: Check for silent RNG fallbacks | |
| run: ./scripts/check-rng-hygiene.sh |
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 59-59: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 58 - 61, Update the actions/checkout
step in the CI workflow to disable credential persistence, while keeping the
existing rng-hygiene step unchanged. Also configure branch protection so the
rng-hygiene status check is required for merges.
Source: Linters/SAST tools
50e8887 to
5c4b7c5
Compare
5c4b7c5 to
e3447d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check-rng-hygiene.sh`:
- Around line 90-134: Update split_line() so Nix’s ``''`` sequence is consumed
as literal/token data rather than toggling q twice, ensuring a # within
``''${...}`` remains code and reaches scan(). Preserve the existing backslash
escape handling while recognizing this sequence, including escaped # and quote
characters outside quoted strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4572f75d-2d1c-4710-af5e-e41bef03a61c
📒 Files selected for processing (3)
docs/SECURITY.mdnixos/frost-gate.nixscripts/check-rng-hygiene.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- nixos/frost-gate.nix
| # Split a line into code and comment, honouring quoting. A naive | ||
| # index($0, "#") splits `''${#pass}` and, worse, treats a `#` inside a | ||
| # string as a comment start -- which let `printf "#x"; pass=/dev/urandom` | ||
| # hide the read, and let an opt-out marker inside a string literal exempt | ||
| # a line with no comment a reviewer could see. | ||
| function split_line(s, i, c, n, q, out) { | ||
| out = ""; cmt = ""; q = "" | ||
| n = length(s) | ||
| for (i = 1; i <= n; i++) { | ||
| c = substr(s, i, 1) | ||
| if (q != "") { | ||
| if (c == "\\") { out = out c substr(s, i + 1, 1); i++; continue } | ||
| if (c == q) q = "" | ||
| out = out c | ||
| continue | ||
| } | ||
| if (c == "\"" || c == "'"'"'") { q = c; out = out c; continue } | ||
| if (c == "#") { cmt = substr(s, i); return out } | ||
| out = out c | ||
| } | ||
| return out | ||
| } | ||
|
|
||
| BEGIN { blockopt = 0; prose = 0 } | ||
| { | ||
| raw = $0 | ||
| code = trim(split_line(raw)) | ||
|
|
||
| # Nix option documentation is prose, not code. Without this, a | ||
| # description mentioning /dev/urandom is a finding, and the only way to | ||
| # silence it is a marker that renders as literal text in the rendered | ||
| # option docs. | ||
| if (prose) { | ||
| if (raw ~ /(\x27\x27;|\x27\x27[ \t]*$)/) prose = 0 | ||
| next | ||
| } | ||
| if (code ~ /(description|example|longDescription)[ \t]*=[ \t]*\x27\x27/ && code !~ /\x27\x27.*\x27\x27[ \t]*;/) { | ||
| prose = 1 | ||
| next | ||
| } | ||
|
|
||
| # A blank line ends the comment block, so a marker cannot carry across | ||
| # unrelated lines to exempt something far below it. | ||
| if (code == "" && cmt == "") { blockopt = 0; next } | ||
| if (code == "") { blockopt = (index(cmt, optout) ? 1 : blockopt); next } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Reproduce the ''${ misparse in isolation with the file's own split_line logic.
set -euo pipefail
awk '
function split_line(s, i, c, n, q, out) {
out = ""; cmt = ""; q = ""
n = length(s)
for (i = 1; i <= n; i++) {
c = substr(s, i, 1)
if (q != "") {
if (c == "\\") { out = out c substr(s, i + 1, 1); i++; continue }
if (c == q) q = ""
out = out c
continue
}
if (c == "\"" || c == "'"'"'") { q = c; out = out c; continue }
if (c == "#") { cmt = substr(s, i); return out }
out = out c
}
return out
}
{ print "code=[" split_line($0) "] cmt=[" cmt "]" }
' <<< $'n=\x27\x27${`#pass`}; leaked=/dev/urandom'Repository: privkeyio/keep-node
Length of output: 205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File context =="
wc -l scripts/check-rng-hygiene.sh
sed -n '1,240p' scripts/check-rng-hygiene.sh | cat -n
echo "== References to files =="
rg -n "nixos/frost-gate\\.nix|/dev/(u)?random|\\$RANDOM|split_line|BLOCKOPT|SELF|list_sources" scripts/check-rng-hygiene.sh
echo "== Focused Nix path if present =="
if [ -f nixos/frost-gate.nix ]; then
rg -n -C 4 "''\\$\\{`#pass`\\}|/dev/(u)?random|base64|LUKS" nixos/frost-gate.nix
fi
echo "== POSIX awk availability =="
command -v awk nawk gawk mawk 2>/dev/null || trueRepository: privkeyio/keep-node
Length of output: 11372
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== search relevant paths =="
rg -E 'nixos/frost-gate\.nix|/dev/(u)?random|split_line|SELF|list_sources|RANDOM' scripts/check-rng-hygiene.sh
echo "== nixos/frost-gate.nix excerpt if present =="
if [ -f nixos/frost-gate.nix ]; then
wc -l nixos/frost-gate.nix
rg -n -C 6 "''\\$\\{`#pass`\\}|head -c 32 /dev/random|-eq 44|base64" nixos/frost-gate.nix || true
else
echo "nixos/frost-gate.nix not present"
fi
echo "== awk output for escaped-outer-quote ''${ and escaped outer comment =="
awk '
function split_line(s, i, c, n, q, out) {
out = ""; cmt = ""; q = ""
n = length(s)
for (i = 1; i <= n; i++) {
c = substr(s, i, 1)
if (q != "") {
if (c == "\\") { out = out c substr(s, i + 1, 1)";i++"; continue }
if (c == q) q = ""
out = out c
continue
}
if (c == "\"" || c == "'"'"'") { q = c; out = out c; continue }
if (c == "#") { cmt = substr(s, i); return out }
out = out c
}
return out
}
{ print "raw=[" NR "] code=[" split_line($0) "] cmt=[" cmt "]" }
' <<'EOF'
n=''${`#pass`}; leaked=/dev/urandom
printf "`#not-a-comment`"; read /dev/random
EOFRepository: privkeyio/keep-node
Length of output: 323
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== search relevant paths =="
grep -nE 'nixos/frost-gate\.nix|/dev/(u)?random|split_line|SELF|list_sources|RANDOM' scripts/check-rng-hygiene.sh
echo "== nixos/frost-gate.nix excerpt if present =="
if [ -f nixos/frost-gate.nix ]; then
wc -l nixos/frost-gate.nix
grep -n -C 6 "''\${`#pass`}\|head -c 32 /dev/random|-eq 44|base64" nixos/frost-gate.nix || true
else
echo "nixos/frost-gate.nix not present"
fi
echo "== awk output for escaped-outer-quote ''${ and escaped outer comment =="
awk '
function split_line(s, i, c, n, q, out) {
out = ""; cmt = ""; q = ""
n = length(s)
for (i = 1; i <= n; i++) {
c = substr(s, i, 1)
if (q != "") {
if (c == "\\") { out = out c substr(s, i + 1, 1)";i++"; continue }
if (c == q) q = ""
out = out c
continue
}
if (c == "\"" || c == "'"'"'") { q = c; out = out c; continue }
if (c == "#") { cmt = substr(s, i); return out }
out = out c
}
return out
}
{ print "raw=[" NR "] code=[" split_line($0) "] cmt=[" cmt "]" }
' <<'EOF'
n=''${`#pass`}; leaked=/dev/urandom
printf "`#not-a-comment`"; read /dev/random
EOFRepository: privkeyio/keep-node
Length of output: 3366
Don’t treat Nix ''${ as two independent quote operators.
split_line() toggles a quote state on each ', so an unwrapped Nix escape like ''${#pass} closes the quote before the #. Content after that # is classified as a comment and is not sent to scan(), so a banned pattern on the same line is bypassed. Keep '' as literal/token data in the Nix preprocessor and preserve the outer-scope backslash skip for escaped #/quotes too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-rng-hygiene.sh` around lines 90 - 134, Update split_line() so
Nix’s ``''`` sequence is consumed as literal/token data rather than toggling q
twice, ensuring a # within ``''${...}`` remains code and reaches scan().
Preserve the existing backslash escape handling while recognizing this sequence,
including escaped # and quote characters outside quoted strings.
Summary
nixos/frost-gate.nix's first-bootprovision(). It was drawn from/dev/urandom./dev/urandomnever blocks. Read before the kernel CRNG is initialised it returns output anyway, with nothing but a kernel log line to say so. This runs during the first boot of a freshly imaged disk, before any seed file exists, which is the one window where that distinction has teeth./dev/randomon Linux 5.6+ blocks only until the CRNG is up and never afterwards, so it is strictly better here at no runtime cost.set -o pipefailalready catches a failing read, but a short one would not raise:head -c 32 /dev/random | base64exits 0 with a shorter string, and provisioning wouldluksFormatthe vault volume under a truncated passphrase and carry on. 32 bytes base64 is exactly 44 characters, so the assertion is exact.How much this is and is not
Stated plainly, because overclaiming is the failure mode this whole sweep is about: this is not a demonstrated break. By the time a systemd unit runs on a modern kernel the CRNG has almost certainly been seeded from RDRAND and jitter entropy, and I have no evidence any deployed node produced a weak passphrase. What I am fixing is that the code asked for the non-blocking source when the blocking one was free, on the single boot where the difference exists, for the key that protects the vault at rest. "Almost certainly" is not the standard that key should be held to, and the change is one character plus an assertion.
Everything else swept came back clean: no bash
$RANDOMin key material (the one use is retry jitter in the OPRF unlock loop, now carrying an explicit marker), no build-time key generation outside the clearly-labelled test fixtures inflake.nix, SSH host keys generated at first boot by sshd rather than baked into the image, and no/var/lib/systemd/random-seedshipped in the image.The guard
scripts/check-rng-hygiene.shplus a lightweightrng-hygieneCI job pin four things: no/dev/urandomin provisioning code, no bash$RANDOMwithout an explicit marker, and — the repo-specific pair — that the bootstrap passphrase is still drawn from/dev/randomand still length-checked. The positive source check exists because a rule that only bans the wrong source leaves swapping the whole draw for a derived value green; see the review round.The job is deliberately separate from the flake checks: it is pure shell and finishes in seconds, while the existing matrix is sixteen QEMU VM tests. An entropy defect gets reported immediately and stays reportable even when the flake is broken for unrelated reasons.
Like its siblings, the guard fails closed. A scanner error, a run outside a git work tree, or an empty file list is reported as a failure rather than passing quietly, because a guard that prints "OK" when it scanned nothing is worse than no guard once people start trusting it.
Decision log
Hardest decision: whether this was worth changing at all, given it is not a demonstrated break. The argument for is that the cost is nil and the window is real and unrepeatable: get the vault volume's passphrase wrong on first boot and no later reprovision fixes the disk that already exists. The argument against is churn in a boot path that is hard to test. The tiebreaker was that the same edit lets the invariant be stated and pinned, which is worth more than the one-character change on its own.
Alternatives rejected:
systemd-creds/getrandom(2)via a helper. Cleaner in principle, but it adds a binary dependency to an early-boot script for a guarantee/dev/randomalready gives on every kernel this image ships./dev/urandomrepo-wide includingtests/. Test fixtures read it deliberately and are not provisioning a real box; a rule that flags them trains people to add markers reflexively.Least confident about: rule 3 pins the draw and the assertion by grepping the module, not by parsing the shell out of the Nix string literal. That is coarse, and a sufficiently creative rewrite of the same lines could satisfy it while changing behaviour. It is the shape of check that stops working quietly, which is why the review round's first pass at it (grepping the error message) was replaced with one that pins the structure.
What is not verified
No hardware run, and no local VM run:
nix flake checkfails identically onmainand on this branch (a pre-existing missingfrost-group-fixturederivation in this workspace), so CI is the first place thefrost-gateVM tests execute — which is why this PR carries thefull-cilabel rather than taking the fast lane. Nothing here has been observed provisioning a real volume on real hardware, and the length assertion has not been seen firing.Test plan
nix-instantiate --parse nixos/frost-gate.nix: parses (the''${#pass}escape is correct inside the Nix string)nix flake check --no-buildoutput byte-identical onmainand this branch, confirming the pre-existing failure is not caused by this changescripts/check-rng-hygiene.shexits 0 on the treeReview round
A security review ran over the branch with the job of falsifying the "exactly one secret" claim and of checking that the blocking read cannot brick a fleet. It confirmed the parts that mattered and found real problems around them.
Confirmed: the
''${#pass}escaping produces the intended shell (evaluated in isolation), 44 really is base64-of-32-bytes and GNUbase64does not wrap below 76 columns, no golden-image key material ships in the closure (host keys andmachine-idare per-device, the root fs ismkfs'd at install, and theflake.nixkey generators are consumed only bytests/), and provisioning cannot half-complete:provision()is only reached after the wipe, so an interrupted run leaves a blank device the next boot re-provisions.The blocking read had no explicit timeout, and CI could not have seen it.
TimeoutStartSecwas set only inside themode == "oprf"branch. The new read lives intpmGateScript— the one mode with no explicit start timeout, inheriting a 90s default that had to cover the CRNG wait plusluksFormat --iter-time 1000,systemd-cryptenroll's argon2id benchmark, andmkfs.ext4. The nixpkgs VM tests run withDefaultTimeoutStartSec = 300, so the test suite provisions with a 3.3x larger budget than a real appliance and could never observe the production one. On x86_64 with RDRAND the CRNG is ready in milliseconds, so this is unlikely rather than impossible — but "unlikely" plus "a failed gate takes the vault down and needs a manual restart" is not a budget to leave unstated. The tpm-mode gate now setsTimeoutStartSec = 600explicitly, generous on purpose: a slow first boot is a much better outcome than a false timeout.The same path also lacked the
TERM/INTtrap the oprf path has. A stop-timeout SIGTERM does not fire theERRtrap, so a timeout mid-provision would leave a labelled, TPM2-enrolled device with no filesystem. Now trapped and rolled back.Six working bypasses in the guard, each now closed and verified by reintroduction:
#inside a string literal was treated as a comment start, soprintf "#x"; pass="head -c 32 /dev/urandom"hid the read entirely. The preprocessor now tracks quoting.refusing to format, so deleting the check while leaving those words in a comment passed. It now pins the structural assertion./dev/urandom; it never required/dev/random, so replacing the whole draw withdate +%s | sha256sumwas green. There is now a positive check on the source.scan()failed open:scanner_died'sexit 1ran inside a command substitution and killed only the subshell, so an awk failure produced empty findings and anOK. Exactly the shape this guard exists to reject, and the second time this session that a guard I wrote had it.Also fixed: a Nix option
descriptionmentioning/dev/urandomwas a false positive whose only escape was a marker that renders as literal text in the generated option docs. Prose blocks are now skipped.Three secret-bearing options had a documented invariant with nothing enforcing it (separate commit, flagged rather than folded in silently).
vaultReplication.rsaKeyFile— the cluster-wide Vaultwarden JWT signing key — plusingress.tlsCertFile/tlsKeyFileandfrostGate.keepPasswordEnvFileall say "not a Nix-path literal" in their descriptions, whilekeepWebandmeshback the same wording with ahasPrefix builtins.storeDirassertion. Without it,rsaKeyFile = "${./secrets/rsa_key.pem}"type-checks and copies the key into/nix/storeat 0444 — and CI pushes store paths to a public Cachix cache. All four now assert.Corrected in the docs: the "only key material this repo generates" sentence was too broad. It is now scoped to the appliance runtime, naming the debug-profile TLS keypair and the
flake.nixtest fixtures as the deliberate exclusions. The docs also no longer say the script "enforces" anything:mainis not a protected branch, so the job is advisory.Test plan (updated)
nix fmtclean; every touched module parses (nix-instantiate --parse)full-cilabel rather than the fast lane, because this PR touches first-boot provisioning and the fast lane skipsfrost-gateandfrost-gate-assertionsentirely#-in-string hiding the read, opt-out inside a string literal, a marker carried across blanks, swapping the draw for a derived value, weakening the length check)descriptionmentioning/dev/urandom, and marked$RANDOMjitterawkexiting 2 and a run outside a git work treedescriptionblock is still scannedSummary by CodeRabbit
Security
Reliability
Documentation
Chores