-
-
Notifications
You must be signed in to change notification settings - Fork 268
feat: add FIDO2 & recovery support for LUKS #1143
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.
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
base: master
Are you sure you want to change the base?
Conversation
WalkthroughAdds FIDO2 enrollment and optional recovery support to LUKS handling, exposes a test flag to simulate a Canokey FIDO2 device, adds an example disk configuration using FIDO2-enrolled LUKS root, and introduces a test that runs with a simulated FIDO2 device. Changes
Sequence Diagram(s)sequenceDiagram
participant Operator
participant Disko
participant cryptsetup
participant systemdCryptenroll as systemd-cryptenroll
participant FIDO as Canokey (FIDO2)
rect rgb(240,248,255)
Note over Disko: Format & enroll flow (initial)
Operator->>Disko: apply config with enrollFido2=true
Disko->>Disko: optionally generate temp password
Disko->>cryptsetup: luksFormat (use temp password or keyfile)
Disko->>systemdCryptenroll: enroll FIDO2 (fido2-device=auto + extra args)
systemdCryptenroll->>FIDO: request touch/PIN
FIDO-->>systemdCryptenroll: token response
systemdCryptenroll->>cryptsetup: add FIDO2 credential
end
rect rgb(255,245,240)
Note over Disko: Optional recovery enrollment
Disko->>systemdCryptenroll: enroll recovery (generate/store QR if enabled)
systemdCryptenroll->>cryptsetup: add recovery key
end
rect rgb(240,255,240)
Note over Disko: Subsequent unlock
Disko->>cryptsetup: open (token-based open command)
cryptsetup->>FIDO: challenge token
FIDO-->>cryptsetup: response
cryptsetup->>Disko: unlocked
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
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)
47-57: Do not pass--token-onlyduring opens that may require passphrase/key-file; it disables fallback.Using
--token-onlywhenenrollFido2is true prevents passphrase/key-file fallback and breaks the immediate post-format open (no token enrolled yet). The comment “try FIRST the token then passphrase” is inaccurate for--token-only.Apply this guarded fix so fallback remains available unless explicitly disabled:
- createOpenCommand = keyFileArgs: '' + createOpenCommand = keyFileArgs: '' cryptsetup open "${config.device}" "${config.name}" \ - ${lib.optionalString (config.enrollFido2 or false) "--token-only"} \ + ${lib.optionalString ((config.enrollFido2 or false) && !(config.settings.fallbackToPassword or false)) "--token-only"} \ ${lib.optionalString (config.settings.allowDiscards or false) "--allow-discards"} \ ${ lib.optionalString (config.settings.bypassWorkqueues or false ) "--perf-no_read_workqueue --perf-no_write_workqueue" } \ ${toString config.extraOpenArgs} \ ${keyFileArgs} \ '';Optionally, drop
--token-onlyentirely and rely on automatic token probing for maximum compatibility.
🧹 Nitpick comments (6)
tests/luks-fido2.nix (1)
11-13: Strengthen assertions: verify FIDO2 token enrollment, not just LUKS header.Currently we only check
isLuks. Please also assert that a FIDO2 token is enrolled in the header to catch regressions in the enrollment flow.Apply this diff to extend the test while keeping the existing check:
extraTestScript = '' machine.succeed("cryptsetup isLuks /dev/vda2"); + # Verify a FIDO2 token is present (looks for Token sections mentioning fido2). + machine.succeed("cryptsetup luksDump /dev/vda2 | grep -i 'Token' | grep -i 'fido2'") '';lib/tests.nix (1)
283-290: Avoid duplicating Canokey QEMU args; define them once to prevent drift.The device is injected via both
virtualisation.qemu.options(Lines 285–288) andstart_command(Lines 330–333). Prefer a single source (start_command) since that’s what actually launches QEMU here.Apply this diff to drop the
virtualisation.qemu.optionsbranch:virtualisation = { emptyDiskImages = builtins.genList (_: 4096) num-disks; - qemu.options = lib.mkIf enableCanokey [ - "-device pci-ohci,id=usb-bus" - "-device canokey,bus=usb-bus.0,file=/tmp/canokey-file" - ]; };Also applies to: 329-333
example/luks-fido2.nix (1)
20-29: Explicitly set a fallback to passphrase to aid recovery scenarios.Consider documenting intent by enabling password fallback. This helps when the token is unavailable.
Apply this minimal change:
settings.allowDiscards = true; + # Allow passphrase/keyfile fallback if token is not available. + settings.fallbackToPassword = true; enrollFido2 = true;lib/types/luks.nix (3)
236-259: Token wait loop is coarse; consider a bounded wait and clearer detection.Polling
/dev/hidraw*can loop forever. Add a timeout and a clearer message to fail fast in CI.Example:
wait_for_token() { echo "Waiting for FIDO2 token insertion..." - # Check if any FIDO2 device is available via /dev/hidraw* - while true; do + # Check if any FIDO2 device is available via /dev/hidraw* + end=$(( $(date +%s) + 60 )) + while [ $(date +%s) -lt $end ]; do if ls /dev/hidraw* &>/dev/null; then echo "FIDO2 device detected." break else echo "FIDO2 device not detected, waiting..." sleep 2 fi done + if ! ls /dev/hidraw* &>/dev/null; then + echo "FIDO2 device not detected within timeout." >&2 + exit 1 + fi }
332-345: Use CRYPTSETUP_TOKEN_PATH for token plugins; LD_LIBRARY_PATH is not the right knob.To make cryptsetup find systemd’s token modules, set
CRYPTSETUP_TOKEN_PATHto${pkgs.systemd}/lib/cryptsetup. Keep LD_LIBRARY_PATH if you need it, but it doesn’t control plugin discovery.Apply:
(pkgs.runCommandNoCC pkgs.cryptsetup.name { nativeBuildInputs = [ pkgs.makeWrapper ]; } '' mkdir -p $out/bin/ makeWrapper ${pkgs.cryptsetup.bin}/bin/cryptsetup $out/bin/cryptsetup \ - --prefix LD_LIBRARY_PATH : ${pkgs.systemd}/lib/cryptsetup + --prefix LD_LIBRARY_PATH : ${pkgs.systemd}/lib \ + --set CRYPTSETUP_TOKEN_PATH ${pkgs.systemd}/lib/cryptsetup '')
95-97: Default for askPassword reads well; small clarity tweak.To keep the intent explicit with FIDO2, reference
enrollFido2directly indefaultText.- defaultText = "true if neither keyFile nor passwordFile nor enrollFido2 are set"; + defaultText = "true if neither keyFile nor passwordFile are set and FIDO2 enrollment is disabled";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
example/luks-fido2.nix(1 hunks)lib/tests.nix(3 hunks)lib/types/luks.nix(7 hunks)tests/luks-fido2.nix(1 hunks)
| ${lib.optionalString enableCanokey '' | ||
| start_command += ["-device", "pci-ohci,id=usb-bus", | ||
| "-device", "canokey,bus=usb-bus.0,file=/tmp/canokey-file" | ||
| ] | ||
| ''} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure host-side Canokey backing file exists before QEMU starts.
file=/tmp/canokey-file is consumed by the host QEMU process, not the guest. If it’s missing, start-up may fail.
Add a lightweight pre-step in the Python testScript to create it:
${lib.optionalString enableCanokey ''
+ import pathlib
+ pathlib.Path("/tmp/canokey-file").touch()
start_command += ["-device", "pci-ohci,id=usb-bus",
"-device", "canokey,bus=usb-bus.0,file=/tmp/canokey-file"
]
''}🤖 Prompt for AI Agents
In lib/tests.nix around lines 329 to 333, the QEMU device uses a host backing
file (/tmp/canokey-file) which must exist before QEMU starts; modify the Python
testScript to create the file as a lightweight pre-step (e.g., open(path,
'a').close()) and set safe permissions (e.g., 0600) before launching QEMU, and
optionally remove the file in teardown to avoid leftover artifacts.
| ${lib.optionalString config.enrollRecovery '' | ||
| systemd-cryptenroll \ | ||
| --recovery-key \ | ||
| --unlock-key-file=${formatKeyFile} \ | ||
| "${config.device}" | ||
| read -p "Press Enter when you scanned the QR code offscreen or that the recovery key is stored securely." | ||
| ''} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid interactive blocking on recovery enrollment in non-interactive contexts.
read -p ... blocks automated runs. Gate it behind the same test env guard you use elsewhere.
Apply:
systemd-cryptenroll \
--recovery-key \
--unlock-key-file=${formatKeyFile} \
"${config.device}"
- read -p "Press Enter when you scanned the QR code offscreen or that the recovery key is stored securely."
+ if [ -z ''${IN_DISKO_TEST+x} ]; then
+ read -p "Press Enter when you scanned the QR code offscreen or that the recovery key is stored securely."
+ fife34bd1 to
47903ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (3)
lib/types/luks.nix (2)
227-231: FixluksAddKeyargument order and variable shadowing.Two issues:
- The
cryptsetup luksAddKeyoptions must precede the device argument. Current order places options last.- The loop variable
keyFileshadows the top-levelkeyFilebinding (lines 14-30), reducing clarity.Apply this diff:
${toString ( - lib.forEach config.additionalKeyFiles (keyFile: '' - cryptsetup luksAddKey "${config.device}" ${keyFile} ${formatKeyFileArgs} + lib.forEach config.additionalKeyFiles (additionalKeyFile: '' + cryptsetup luksAddKey ${formatKeyFileArgs} "${config.device}" ${additionalKeyFile} '') )}
233-240: Gate interactive prompt in non-interactive contexts.The
read -pat line 239 will block automated/CI runs. Wrap it in the sameIN_DISKO_TESTguard used elsewhere.Apply this diff:
systemd-cryptenroll \ --recovery-key \ --unlock-key-file=${formatKeyFile} \ "${config.device}" - read -p "Press Enter when you scanned the QR code offscreen or that the recovery key is stored securely." + if [ -z ''${IN_DISKO_TEST+x} ]; then + read -p "Press Enter when you scanned the QR code offscreen or that the recovery key is stored securely." + filib/tests.nix (1)
329-333: Create Canokey backing file before QEMU starts.The
/tmp/canokey-fileis consumed by the host QEMU process but is never created. This will cause startup failures.Add file creation in the testScript before machine startup:
${lib.optionalString enableCanokey '' + import pathlib + pathlib.Path("/tmp/canokey-file").touch(mode=0o600) start_command += ["-device", "pci-ohci,id=usb-bus", "-device", "canokey,bus=usb-bus.0,file=/tmp/canokey-file" ] ''}
🧹 Nitpick comments (2)
lib/types/luks.nix (2)
241-264: Consider adding timeout to FIDO2 device detection.The
wait_for_tokenloop at lines 246-254 could wait indefinitely if no FIDO2 device is inserted. Consider adding a timeout with a clear error message.Example enhancement:
${lib.optionalString config.enrollFido2 '' wait_for_token() { echo "Waiting for FIDO2 token insertion..." + MAX_WAIT=300 # 5 minutes + elapsed=0 # Check if any FIDO2 device is available via /dev/hidraw* while true; do if ls /dev/hidraw* &>/dev/null; then echo "FIDO2 device detected." break + elif [ $elapsed -ge $MAX_WAIT ]; then + echo "Timeout waiting for FIDO2 device after $MAX_WAIT seconds." + exit 1 else echo "FIDO2 device not detected, waiting..." sleep 2 + elapsed=$((elapsed + 2)) fi done }
320-325: Consider making systemd requirement an assertion.The comment at lines 323-324 raises a good point. Since FIDO2 tokens require systemd stage 1, this could be enforced with an assertion rather than silently enabling it.
Optional enhancement:
# Instead of or in addition to: boot.initrd.systemd.enable = config.enrollFido2; # Add an assertion: assertions = [ { assertion = !config.enrollFido2 || config.boot.initrd.systemd.enable; message = "FIDO2 enrollment requires boot.initrd.systemd.enable = true"; } ];However, the current approach (auto-enabling) provides better UX and is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
example/luks-fido2.nix(1 hunks)lib/tests.nix(3 hunks)lib/types/luks.nix(7 hunks)tests/luks-fido2.nix(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- example/luks-fido2.nix
🔇 Additional comments (11)
lib/types/luks.nix (8)
11-12: Good approach for temporary password handling.The
autogeneratedPasswordflag cleanly centralizes the decision to generate a temporary password that will be removed after FIDO2 enrollment.
32-44: Well-structured key file argument handling.The separation of
keyFileArgs(standard operations) andformatKeyFileArgs(2-stage formatting) clearly delineates the two different use cases.
46-68: Excellent abstraction of open command logic.The
createOpenCommandfunction with optionaltokenTypecleanly handles both post-enrollment (with token) and pre-enrollment (format-time) scenarios. The distinction betweencryptsetupOpenandformatCryptsetupOpenprevents token-only mode issues during formatting.
100-101: Correct update to askPassword default.Adding the
!autogeneratedPasswordcondition prevents interactive password prompts when FIDO2 enrollment auto-generates credentials.
104-122: Well-designed public API options.The three new options are clearly documented with sensible defaults. Linking
enrollRecoverytoenrollFido2by default is a good UX choice for hardware token scenarios.
207-218: Secure temporary password generation.Using
openssl rand -hex 32provides sufficient entropy, and theSLOT_ZERO_TO_DELETEmarker ensures the temporary password is properly cleaned up after FIDO2 enrollment.
219-224: Format and open sequence looks correct.The use of
formatKeyFileArgsfor bothluksFormatandformatCryptsetupOpenensures consistency when handling the temporary password in FIDO2 scenarios.
339-349: Pragmatic solution for token library loading.The
LD_LIBRARY_PATHwrapper ensures cryptsetup can load systemd's token libraries. While not the cleanest approach, it's a practical solution given the constraints.tests/luks-fido2.nix (1)
1-14: Clean and focused test definition.The test correctly enables Canokey simulation and verifies LUKS formatting. The device path
/dev/vda2aligns with the booted machine's device layout (after the VM device shift).lib/tests.nix (2)
82-82: Good addition of Canokey simulation support.The
enableCanokeyparameter provides a clean way to opt into FIDO2 device simulation for tests.
283-289: Canokey device configuration looks correct.The QEMU options properly configure the USB bus and Canokey device. However, note that
/tmp/canokey-filemust exist before QEMU starts (see comment on lines 329-333).
This introduces what has been described in https://0pointer.net/blog/unlocking-luks2-volumes-with-tpm2-fido2-pkcs11-security-hardware-on-systemd-248.html. That is: support for FIDO2 and recovery passphrases and their boot mechanism. Testing is hard right now because of canokey-qemu being broken and U2F is not a valid replacement for FIDO2… I tried to keep as much as possible the previous behavior and make it possible to mix FIDO2 and normal passphrases or key files without any problem. PIV support is out of scope for this change but can easily be added. Signed-off-by: Raito Bezarius <masterancpp@gmail.com>
47903ad to
d63769e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (3)
lib/types/luks.nix (2)
227-231: VerifyluksAddKeyargument order.The
cryptsetup luksAddKeysyntax is[options] <device> [new-key-file]. The current order places${keyFile}before${formatKeyFileArgs}, which may be incorrect ifformatKeyFileArgscontains the unlock key option (--key-file).Check the
cryptsetupman page to confirm the correct order. IfformatKeyFileArgscontains--key-file, apply:- cryptsetup luksAddKey "${config.device}" ${keyFile} ${formatKeyFileArgs} + cryptsetup luksAddKey ${formatKeyFileArgs} "${config.device}" ${keyFile}Based on past review comments.
233-240: Gate the interactive prompt to avoid blocking automated runs.The
read -pon line 239 will block in non-interactive contexts such as CI/CD pipelines or automated testing.Apply this fix:
systemd-cryptenroll \ --recovery-key \ --unlock-key-file=${formatKeyFile} \ "${config.device}" - set +x; read -p "Press Enter when you scanned the QR code offscreen or that the recovery key is stored securely."; set -x + if [ -z ''${IN_DISKO_TEST+x} ]; then + set +x; read -p "Press Enter when you scanned the QR code offscreen or that the recovery key is stored securely."; set -x + fiBased on past review comments.
lib/tests.nix (1)
329-333: Create/tmp/canokey-filebefore QEMU starts to prevent startup failures.The QEMU device references a host backing file that must exist before the VM launches. Without pre-creation, QEMU may fail to start.
Add this snippet before line 334 to create the file:
${lib.optionalString enableCanokey '' + import pathlib + pathlib.Path("/tmp/canokey-file").touch(mode=0o600) start_command += ["-device", "pci-ohci,id=usb-bus", "-device", "canokey,bus=usb-bus.0,file=/tmp/canokey-file" ] ''}Based on past review comments.
🧹 Nitpick comments (2)
lib/types/luks.nix (2)
262-262: RenameSLOT_ZERO_TO_DELETEto avoid environment variable collisions.The variable name is generic and could conflict with user or system environment variables. Use a disko-specific prefix.
- ''${SLOT_ZERO_TO_DELETE:+--wipe-slot=0} \ + ''${_DISKO_SLOT_ZERO_TO_DELETE:+--wipe-slot=0} \And update line 217:
- export SLOT_ZERO_TO_DELETE=true + export _DISKO_SLOT_ZERO_TO_DELETE=true
320-328: Consider using an assertion instead of forcingsystemd.enable.Silently overriding the user's initrd configuration could mask incompatibilities. An assertion would make the requirement explicit and prevent surprising behavior.
Replace line 327 with:
assertions = lib.mkIf config.enrollFido2 [{ assertion = config.boot.initrd.systemd.enable or false; message = "FIDO2 enrollment requires boot.initrd.systemd.enable = true"; }];Or if forcing is intended, document why systemd stage 1 is required in a comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
example/luks-fido2.nix(1 hunks)lib/tests.nix(3 hunks)lib/types/luks.nix(7 hunks)tests/luks-fido2.nix(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- example/luks-fido2.nix
🔇 Additional comments (9)
tests/luks-fido2.nix (1)
1-14: LGTM! Clean test structure.The test correctly configures FIDO2 simulation and validates LUKS setup.
lib/tests.nix (1)
82-82: Good addition of the Canokey simulation flag.The parameter is appropriately optional and enables FIDO2 testing scenarios.
lib/types/luks.nix (7)
11-12: Clear autogeneration flag logic.The flag correctly identifies when a temporary password should be auto-generated for FIDO2 enrollment.
32-44: Well-structured key file handling for two-stage enrollment.The separation between
formatKeyFile(used during initial format) andkeyFile(used during normal mount) is essential for FIDO2 flows where the initial password is temporary.
46-68: Good abstraction for open commands.The helper function correctly parameterizes token type and key file arguments, enabling both standard and FIDO2 unlock paths.
104-122: FIDO2 and recovery options are well-designed.The options provide clear control over enrollment behavior, and the
enrollRecoverydefault matchingenrollFido2makes sense for security.
207-218: Temporary password generation is secure.Using
openssl rand -hex 32provides sufficient entropy, and marking slot 0 for deletion ensures cleanup.
241-266: FIDO2 enrollment flow is well-implemented.The wait-for-token logic and conditional slot wiping work correctly together.
341-351: Cryptsetup wrapper with LD_LIBRARY_PATH is pragmatic.While
LD_LIBRARY_PATHis a workaround, it's an acceptable solution for linking against systemd's token libraries without patching cryptsetup binaries.
This introduces what has been described in
https://0pointer.net/blog/unlocking-luks2-volumes-with-tpm2-fido2-pkcs11-security-hardware-on-systemd-248.html.
That is: support for FIDO2 and recovery passphrases and their boot mechanism.
Testing is hard right now because of canokey-qemu being broken and U2F is not a valid replacement for FIDO2…
I tried to keep as much as possible the previous behavior and make it possible to mix FIDO2 and normal passphrases or key files without any problem.
PIV support is out of scope for this change but can easily be added.
Summary by CodeRabbit
New Features
Examples
Tests