Skip to content

Add password as fallback to LUKS #1135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add password as fallback to LUKS #1135

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions lib/types/luks.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,26 @@
...
}:
let
passwordFd =
if config.askPassword then
''<(set +x; echo -n "$password"; set -x)''
else if config.passwordFile != null then
''<(set +x; echo -n "$(cat ${config.passwordFile})"; set -x)''
else
null;
keyFile =
if config.settings ? "keyFile" then
config.settings.keyFile
else if config.askPassword then
''<(set +x; echo -n "$password"; set -x)''
else if
config.passwordFile != null
# do not print the password to the console
then
''<(set +x; echo -n "$(cat ${config.passwordFile})"; set -x)''
else if config.keyFile != null then
lib.warn (
"The option `keyFile` is deprecated."
+ "Use passwordFile instead if you want to use interactive login or settings.keyFile if you want to use key file login"
) config.keyFile
else
null;
addPasswordAsFallback = keyFile != null && passwordFd != null;
keyFileArgs = ''
${lib.optionalString (keyFile != null) "--key-file ${keyFile}"} \
--key-file ${if (keyFile != null) then keyFile else passwordFd} \
Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I guess it would've technically worked before, where with this change it doesn't. Will add code to handle this case.

${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}"} \
'';
Expand Down Expand Up @@ -165,6 +166,10 @@ in
if ! cryptsetup status "${config.name}" >/dev/null; then
${cryptsetupOpen} --persistent
fi

${lib.optionalString addPasswordAsFallback ''
cryptsetup luksAddKey "${config.device}" ${passwordFd} ${keyFileArgs}
''}
Comment on lines +170 to +172
Copy link

@coderabbitai coderabbitai bot Sep 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Copy link
Author

@Sandarr95 Sandarr95 Sep 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

🧩 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:

  1. addPasswordAsFallback: cryptsetup luksAddKey "${config.device}" ${passwordFd} ${keyFileArgs}
  2. additionalKeyFiles: cryptsetup luksAddKey "${config.device}" ${keyFile} ${keyFileArgs} (where keyFileArgs contains passwordFd)

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, keyFileArgs uses keyFile as the unlock key
  • passwordFd is the NEW key being added
  • Issue exists: If device already exists and askPassword = true, $password (referenced in passwordFd) would be unset

For additionalKeyFiles block:

  • Command: cryptsetup luksAddKey "${config.device}" ${keyFile} ${keyFileArgs} (where keyFile is from the forEach)
  • The unlock key in keyFileArgs depends on whether the main keyFile is null or not
  • If main keyFile != null: keyFileArgs uses main keyFile as unlock key → No issue
  • If main keyFile == null: keyFileArgs uses passwordFd as 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.

Copy link
Author

@Sandarr95 Sandarr95 Sep 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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 = { /* ... */ };
};

Copy link

Choose a reason for hiding this comment

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

@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 passwordFd

So 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.

${toString (
lib.forEach config.additionalKeyFiles (keyFile: ''
cryptsetup luksAddKey "${config.device}" ${keyFile} ${keyFileArgs}
Expand Down