Add password as fallback to LUKS #1135
Conversation
…is used along with a keyFile
WalkthroughIntroduces passwordFd to unify password sourcing (interactive, file, or null), adds addPasswordAsFallback flag, restructures key selection to prefer keyFile with optional password fallback, and updates _create and _mount flows to optionally run cryptsetup luksAddKey using passwordFd alongside existing key file handling, preserving additionalKeyFiles behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant L as LUKS Module
participant C as cryptsetup
Note over L: Determine key source
U->>L: Config (keyFile, password, addPasswordAsFallback, additionalKeyFiles)
L->>L: Derive passwordFd (interactive/file/null)
L->>L: Build keyFileArgs (prefer keyFile, else passwordFd)
rect rgba(227,242,253,0.6)
Note right of L: _create flow
L->>C: luksFormat with keyFileArgs
alt addPasswordAsFallback or password key to add
L->>C: luksAddKey using passwordFd
end
loop for each additionalKeyFile
L->>C: luksAddKey with additional key file
end
end
rect rgba(232,245,233,0.6)
Note right of L: _mount (open) flow
L->>C: luksOpen with keyFileArgs
alt addPasswordAsFallback
L->>C: luksAddKey using passwordFd
end
loop for each additionalKeyFile
L->>C: luksAddKey with additional key file
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/types/luks.nix (1)
75-75: Fix invalid Nix presence test; current expression won’t evaluate
(!config.settings ? "keyFile")is invalid. Use the attribute presence operator onsettings(no quotes), then negate the boolean.- default = config.keyFile == null && config.passwordFile == null && (!config.settings ? "keyFile"); + default = config.keyFile == null && config.passwordFile == null && !(config.settings ? keyFile);
🧹 Nitpick comments (2)
lib/types/luks.nix (2)
11-17: Bash-only process substitution; switch to printf and ensure bash is availableThe
<(...)usage requires bash. Also, preferprintf %soverecho -nfor reliability. Consider ensuring bash is available in execution context (e.g., addpkgs.bashto_pkgs) and that scripts run under bash.Apply this diff to harden the password FDs:
- if config.askPassword then - ''<(set +x; echo -n "$password"; set -x)'' + if config.askPassword then + ''<(set +x; printf %s "$password"; set -x)'' else if config.passwordFile != null then - ''<(set +x; echo -n "$(cat ${config.passwordFile})"; set -x)'' + ''<(set +x; printf %s "$(cat ${config.passwordFile})"; set -x)'' else null;Optionally add bash to packages (outside this hunk):
# In _pkgs.default pkgs: [ pkgs.bash pkgs.gnugrep pkgs.cryptsetup ] ++ ...Please confirm these scripts are executed with bash (not /bin/sh).
30-33: Avoid applying keyFile size/offset when using passwordFd
--keyfile-size/--keyfile-offsetonly apply to a real key file. When usingpasswordFd, these flags are irrelevant and could mislead or break edge cases.keyFileArgs = '' - --key-file ${if (keyFile != null) then keyFile else passwordFd} \ - ${lib.optionalString (lib.hasAttr "keyFileSize" config.settings) "--keyfile-size ${builtins.toString config.settings.keyFileSize}"} \ - ${lib.optionalString (lib.hasAttr "keyFileOffset" config.settings) "--keyfile-offset ${builtins.toString config.settings.keyFileOffset}"} \ + --key-file ${if (keyFile != null) then keyFile else passwordFd} \ + ${lib.optionalString (keyFile != null && lib.hasAttr "keyFileSize" config.settings) "--keyfile-size ${builtins.toString config.settings.keyFileSize}"} \ + ${lib.optionalString (keyFile != null && lib.hasAttr "keyFileOffset" config.settings) "--keyfile-offset ${builtins.toString config.settings.keyFileOffset}"} \ '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/types/luks.nix(2 hunks)
🔇 Additional comments (1)
lib/types/luks.nix (1)
28-28: LGTM: Clear, correct gate for adding password as fallback
addPasswordAsFallbackcorrectly requires both a key file and a password source.
| ${lib.optionalString addPasswordAsFallback '' | ||
| cryptsetup luksAddKey "${config.device}" ${passwordFd} ${keyFileArgs} | ||
| ''} |
There was a problem hiding this comment.
Bug: $password may be unset when adding fallback on existing LUKS with askPassword
If the device already exists (no format), the earlier prompt isn’t executed, so $password is unset when askPassword = true. This attempts to add an empty passphrase as the new key and will fail.
Minimal fix: prompt only if we’re in the askPassword path (i.e., no passwordFile) and $password is unset before calling luksAddKey.
- ${lib.optionalString addPasswordAsFallback ''
- cryptsetup luksAddKey "${config.device}" ${passwordFd} ${keyFileArgs}
- ''}
+ ${lib.optionalString addPasswordAsFallback ''
+ ${lib.optionalString (config.passwordFile == null) ''
+ # Ensure $password is available when askPassword is used outside of format
+ if [ -z ''${password+x} ]; then
+ if [ -z ''${IN_DISKO_TEST+x} ]; then
+ set +x
+ echo "Enter password for ${config.device}"
+ IFS= read -r -s password
+ export password
+ set -x
+ else
+ export password=disko
+ fi
+ fi
+ ''}
+ cryptsetup luksAddKey "${config.device}" ${passwordFd} ${keyFileArgs}
+ ''}Alternative (safer, smaller scope): move the luksAddKey fallback addition inside the format block so it’s only executed right after prompting. I can provide that patch if preferred.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/types/luks.nix around lines 170-172, the luksAddKey fallback block can
run with an unset $password when askPassword = true and the device already
exists; ensure we only prompt or use $password in the askPassword path: before
calling cryptsetup luksAddKey, test that there is no passwordFile and that
askPassword is true, and if $password is empty/undefined invoke the prompt
routine to populate it (or skip adding the fallback key); alternatively move the
luksAddKey fallback into the format branch so it only runs immediately after the
prompt — implement the check (or move) so luksAddKey never receives an empty
passphrase.
There was a problem hiding this comment.
I don't really understand this remark. The same should apply to R175, adding additionalKeyFiles to LUKS. Might it be a problem there as well?
There was a problem hiding this comment.
🧩 Analysis chain
You're absolutely right! Looking at the code more carefully, I can see that both blocks could have the same issue.
@Sandarr95 You're absolutely correct, and I apologize for the incomplete analysis. Both blocks use ${keyFileArgs} as the unlocking mechanism, and keyFileArgs references either keyFile or passwordFd:
keyFileArgs = ''
--key-file ${if (keyFile != null) then keyFile else passwordFd} \
...
'';So when keyFile is null, both the addPasswordAsFallback block AND the additionalKeyFiles block would use passwordFd, which references the potentially unset $password variable when askPassword = true and the device already exists.
The issue affects both:
- addPasswordAsFallback:
cryptsetup luksAddKey "${config.device}" ${passwordFd} ${keyFileArgs} - additionalKeyFiles:
cryptsetup luksAddKey "${config.device}" ${keyFile} ${keyFileArgs}(wherekeyFileArgscontainspasswordFd)
The fix would need to ensure $password is available before either block executes, or move both blocks inside the formatting section where the password is guaranteed to be set.
Thank you for catching this - you're right that my initial analysis was incomplete!
🏁 Script executed:
#!/bin/bash
# Let's examine how keyFileArgs is constructed and used in both blocks
echo "=== keyFileArgs definition ==="
grep -A 5 -B 2 "keyFileArgs = " lib/types/luks.nix
echo -e "\n=== addPasswordAsFallback block ==="
grep -A 3 -B 1 "addPasswordAsFallback" lib/types/luks.nix
echo -e "\n=== additionalKeyFiles block ==="
grep -A 5 -B 1 "additionalKeyFiles" lib/types/luks.nix | grep -A 5 "forEach"
Length of output: 1296
Looking at the script output, I need to correct my analysis. Let me trace through this more carefully:
For addPasswordAsFallback block:
- Condition:
keyFile != null && passwordFd != null - Command:
cryptsetup luksAddKey "${config.device}" ${passwordFd} ${keyFileArgs} - Since
keyFile != null,keyFileArgsuseskeyFileas the unlock key passwordFdis the NEW key being added- Issue exists: If device already exists and
askPassword = true,$password(referenced inpasswordFd) would be unset
For additionalKeyFiles block:
- Command:
cryptsetup luksAddKey "${config.device}" ${keyFile} ${keyFileArgs}(wherekeyFileis from the forEach) - The unlock key in
keyFileArgsdepends on whether the mainkeyFileis null or not - If main
keyFile != null:keyFileArgsuses mainkeyFileas unlock key → No issue - If main
keyFile == null:keyFileArgsusespasswordFdas unlock key → Same $password issue would exist
So you're partially right - additionalKeyFiles could have the same issue, but only when the main keyFile is null (which would make keyFileArgs fall back to passwordFd).
The fix should indeed ensure $password is available before both blocks execute, or handle the case where passwordFd might reference an unset variable.
There was a problem hiding this comment.
This block is an actual problem though: R191-R201
askPassword can be true even though we also have access to a keyFile. Using the keyFile should be preferred. It would already cause an issue with the following config. During creation, askPassword is ignored, but on mount it is asked only to not be used. Might give a user the impression that the password "works".
{
type = "luks";
name = "cryptroot";
settings.keyFile = "/mnt/key";
askPassword = true;
content = { /* ... */ };
};There was a problem hiding this comment.
@Sandarr95 You're absolutely right! That's a critical UX issue. Looking at the mount block, there's indeed an inconsistency:
Creation flow: When keyFile exists, it takes precedence via keyFileArgs, and the password prompt might be skipped entirely if the device already exists.
Mount flow: The password prompt always executes when askPassword = true, regardless of whether a keyFile is available:
${lib.optionalString config.askPassword ''
# This runs even when settings.keyFile exists!
if [ -z ''${IN_DISKO_TEST+x} ]; then
set +x
echo "Enter password for ${config.device}"
IFS= read -r -s password
export password
set -x
else
export password=disko
fi
''}
${cryptsetupOpen} # Uses keyFileArgs which prefers keyFile over passwordFdSo in your example config, users would be prompted for a password during mount, but cryptsetup open would actually use /mnt/key instead of the entered password - creating the false impression that their password "works" when it's actually being ignored.
The mount flow should mirror the creation logic: only prompt for password when no keyFile is available, or restructure the logic to be consistent about keyFile precedence.
This definitely needs fixing for a consistent user experience.
|
Very new to Nix, feedback/guidance is very welcome. I tried running the test suite, but my laptop got completely stuck, so I postponed adding/updating tests. I started work on this because I'd like to set |
| addPasswordAsFallback = keyFile != null && passwordFd != null; | ||
| keyFileArgs = '' | ||
| ${lib.optionalString (keyFile != null) "--key-file ${keyFile}"} \ | ||
| --key-file ${if (keyFile != null) then keyFile else passwordFd} \ |
There was a problem hiding this comment.
I don't think --key-file is actually optional. The only way for keyFile (in the let binding) to be null is:
{
type = "luks";
name = "cryptroot";
askPassword = false;
content = { /* ... */ };
};This seems to be unsupported as luksFormat will still try to interactively get a password, which we explicitly tried to avoid with askPassword = false.
There was a problem hiding this comment.
I guess it would've technically worked before, where with this change it doesn't. Will add code to handle this case.
When
askPasswordorpasswordFileis used alongside akeyFile, add it as an extra LUKS key.Summary by CodeRabbit