add CRYPTROOT_AUTOUNLOCK option and fix CRYPTROOT for uefi builds #8805
Conversation
Hey @sashasimkin! 👋Thanks for submitting your first pull request to the Armbian project — we're excited to have you contributing! 🧡 If you'd like to stay informed about project updates or collaborate more closely with the team, Also, don’t forget to ⭐ star the repo if you haven’t already — and welcome aboard! 🚀 |
github-actions
bot
added
size/medium
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds an autounlock path for LUKS root encryption: when CRYPTROOT_AUTOUNLOCK=yes a temporary key file is generated (e.g., /etc/rootfs.key) and used to format/open the root LUKS container and referenced in crypttab; the existing passphrase-based path remains. Configuration validation now accepts either CRYPTROOT_PASSPHRASE or CRYPTROOT_AUTOUNLOCK when CRYPTROOT_ENABLE=yes. Initramfs cache hashing includes /etc/crypttab. GRUB changes set a conditional UEFI_MOUNT_POINT, pass --efi-directory to grub-install, ensure UUID probing for root and /boot, and force UUID usage in GRUB config. LUKS resize uses /etc/rootfs.key when present. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
extensions/fs-cryptroot-support.sh(2 hunks)extensions/grub.sh(4 hunks)lib/functions/configuration/main-config.sh(1 hunks)lib/functions/image/initrd.sh(1 hunks)lib/functions/image/partitioning.sh(1 hunks)packages/bsp/common/usr/lib/armbian/armbian-resize-filesystem(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
lib/functions/image/partitioning.sh (1)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)
extensions/fs-cryptroot-support.sh (3)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)extensions/lvm.sh (1)
prepare_root_device__create_volume_group(41-68)lib/functions/rootfs/trap-rootfs.sh (1)
prepare_rootfs_build_params_and_trap(12-51)
extensions/grub.sh (2)
lib/functions/logging/runners.sh (1)
chroot_custom(155-159)extensions/grub-riscv64.sh (3)
extension_prepare_config__prepare_grub-riscv64(3-25)pre_umount_final_image__install_grub(27-100)configure_grub(102-142)
🔇 Additional comments (8)
lib/functions/configuration/main-config.sh (1)
172-173: LGTM: Prerequisite check correctly updated for autounlock support.The logic now properly allows either passphrase-based or autounlock-based encryption, aligning with the new CRYPTROOT_AUTOUNLOCK feature introduced in this PR.
lib/functions/image/initrd.sh (1)
65-65: LGTM: Proper initramfs cache invalidation for crypttab changes.Adding crypttab to the cache hash ensures the initramfs is correctly rebuilt when key file configuration changes, which is essential for the autounlock feature.
extensions/grub.sh (4)
170-170: LGTM: Explicit EFI directory specification.The explicit
--efi-directory=${UEFI_MOUNT_POINT}parameter correctly supports flexible EFI partition locations, which is essential for cryptroot configurations with separate boot partitions.
182-187: LGTM: UUID preparation for GRUB configuration.The UUID symlink preparation is essential for proper GRUB configuration with encrypted root. The comments clearly explain:
- Root UUID is needed so grub-update discovers the unlocked
/dev/mapper/armbian-rootUUID- Boot partition UUID is included to handle cases with separate boot partitions (LVM, cryptroot)
The implementation correctly handles both cases and won't fail if the symlinks already exist.
298-299: LGTM: Explicit UUID enablement in GRUB.Explicitly setting both
GRUB_DISABLE_UUID=falseandGRUB_DISABLE_LINUX_UUID=falseensures GRUB uses UUIDs rather than device paths. This is essential for cryptroot support and aligns with the UUID symlink preparation (lines 182-187).
24-28: No action needed — variable has proper default defined.The review comment's concern is unfounded. UEFI_MOUNT_POINT is initialized with a default value of
/boot/efiinlib/functions/image/partitioning.sh. WhenBOOTPART_REQUIRED != "yes", the variable retains this default; it's only overridden to/bootwhen the condition is true. The unconditional use on line 28 is safe.Likely an incorrect or invalid review comment.
lib/functions/image/partitioning.sh (1)
332-336: Configuration is correct and follows standard Debian/Ubuntu practice.The KEYFILE_PATTERN mechanism in /etc/cryptsetup-initramfs/conf-hook and UMASK=0077 restriction are the standard approach documented by cryptsetup-initramfs for including key files in the initramfs. The implementation shown is correct.
extensions/fs-cryptroot-support.sh (1)
28-42: Now I need to search the codebase to verify the actual behavior. Let me check where the temp key file is moved and if TMPDIR is set:Temp key file cleanup remains an unaddressed concern.
The temporary key file created at line 30 with
mktempis not guaranteed to be cleaned up. By default mktemp will use user's $TMPDIR. If not defined it will use /tmp. Thecleanup_cryptrootfunction only closes the LUKS mapper and does not delete the temporary key file. If an error occurs before the file is moved to its permanent location, the temporary key file could be left behind on the system.Adding explicit cleanup for
$crytproot_autounlock_key_filein thecleanup_cryptrootfunction (line 99) would be a prudent safeguard to ensure the key material is not accidentally exposed on disk.
sashasimkin
force-pushed
the
main
branch
2 times, most recently
from
e8768f1 to
2079ceb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
extensions/fs-cryptroot-support.sh(2 hunks)extensions/grub.sh(4 hunks)lib/functions/configuration/main-config.sh(1 hunks)lib/functions/image/initrd.sh(1 hunks)lib/functions/image/partitioning.sh(1 hunks)packages/bsp/common/usr/lib/armbian/armbian-resize-filesystem(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/functions/image/initrd.sh
- packages/bsp/common/usr/lib/armbian/armbian-resize-filesystem
- lib/functions/configuration/main-config.sh
- lib/functions/image/partitioning.sh
🧰 Additional context used
🧬 Code graph analysis (2)
extensions/grub.sh (2)
lib/functions/logging/runners.sh (1)
chroot_custom(155-159)extensions/grub-riscv64.sh (3)
extension_prepare_config__prepare_grub-riscv64(3-25)pre_umount_final_image__install_grub(27-100)configure_grub(102-142)
extensions/fs-cryptroot-support.sh (3)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)extensions/lvm.sh (1)
prepare_root_device__create_volume_group(41-68)lib/functions/rootfs/trap-rootfs.sh (1)
prepare_rootfs_build_params_and_trap(12-51)
🔇 Additional comments (4)
extensions/fs-cryptroot-support.sh (2)
28-38: Verify temporary key file cleanup and handoff to target image.The autounlock workflow generates a temporary key file (line 31) with
mktemp, but I don't see explicit cleanup incleanup_cryptroot()(line 99). While the global variablecryptroot_autounlock_key_filepersists for use by other stages (e.g., writing to/etc/rootfs.keyin partitioning.sh), ensure that the temporary file is properly cleaned up after it's written to the target image.Please verify:
- That
cleanup_cryptroot()or the image finalization stage removes the temporary key file from the build host.- That the key file is correctly copied to
/etc/rootfs.keyin the target image (expected per AI summary mentioning partitioning.sh changes).- That the variable remains accessible for use by later build stages that need to reference it.
27-40: Autounlock encryption logic looks sound.The conditional branching between autounlock (lines 28–33) and passphrase-based (lines 34–37) encryption is clear and correct. Key generation, LUKS formatting, and mapper opening flow properly for both paths.
extensions/grub.sh (2)
182-187: UUID handling for root and boot partitions is sound.The use of
grub-probe --target=fs_uuidto discover partition UUIDs and create symlinks in/dev/disk/by-uuid/is a robust approach. This ensures GRUB references stable UUIDs rather than device paths, which is essential for encrypted root scenarios where/dev/mapper/...names may differ.The addition of a second symlink for
/boot(lines 185–187) correctly handles cases where a separate boot partition exists (e.g., with LVM or cryptroot), while remaining safe if no separate boot partition is present.
298-299: Explicit UUID settings improve GRUB configuration robustness.Setting
GRUB_DISABLE_UUID=falseandGRUB_DISABLE_LINUX_UUID=falseensures GRUB uses UUIDs in the generated config, which is critical for correctness in encrypted and partitioned scenarios.
|
For the documentation: |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
extensions/fs-cryptroot-support.sh(2 hunks)extensions/grub.sh(4 hunks)lib/functions/configuration/main-config.sh(1 hunks)lib/functions/image/initrd.sh(1 hunks)lib/functions/image/partitioning.sh(1 hunks)packages/bsp/common/usr/lib/armbian/armbian-resize-filesystem(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/bsp/common/usr/lib/armbian/armbian-resize-filesystem
- lib/functions/image/partitioning.sh
- lib/functions/image/initrd.sh
- extensions/grub.sh
🧰 Additional context used
🧬 Code graph analysis (1)
extensions/fs-cryptroot-support.sh (2)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)extensions/lvm.sh (1)
prepare_root_device__create_volume_group(41-68)
🔇 Additional comments (3)
extensions/fs-cryptroot-support.sh (2)
27-27: LGTM on conditional logic and passphrase branch.The conditional structure properly branches between autounlock (lines 28-33) and passphrase (lines 34-37) paths. The passphrase branch uses existing patterns and the autounlock branch correctly generates a random key. Note: the previous typo (
crytproot_autounlock_key_file) has been corrected tocryptroot_autounlock_key_filein line 30.Also applies to: 34-37
28-38: Global variable scope and cleanup handler verified as correct.The verification confirms the code properly handles the concerns raised:
Global variable scope:
declare -g cryptroot_autounlock_key_filecorrectly exposes the variable globally, making it accessible tolib/functions/image/partitioning.shat line 331 where it's moved to the final filesystem.Cleanup timing: The cleanup handler registered at line 39 only executes
cryptsetup luksCloseand does not delete the key file. Since the file is moved (not copied) to the final image at${SDCARD}/etc/rootfs.keyduring partitioning, it no longer exists in/tmpwhen the post-mount cleanup runs.The implementation is correct and requires no changes.
lib/functions/configuration/main-config.sh (1)
172-173: Configuration validation correctly enforces new requirement.The updated conditional properly validates that either
CRYPTROOT_PASSPHRASEorCRYPTROOT_AUTOUNLOCKis set whenCRYPTROOT_ENABLE=yes. The error message clearly communicates both options to the user.
|
@igorpecovnik I think this is R4R now :) |
|
Bot suggestions are not harmful ... but yeah, many segments in the code is not done perfect, so generally we at least try to improve what is being added. |
|
Got it - fixed. I usually try to keep changes to bare minimum to avoid unintended side-effects. |
|
quick update (both target trixie):
|
I hope all those boot issues for rk3588/s are sorted once current is being rolled-over to the next LTS kernel which may be 6.18 which is edge atm. |
|
✅ This PR has been reviewed and approved — all set for merge! |
|
I converted it to draft to figure out and fix why growpart wasn’t executed as expected. |
|
here's the log of armbian-resize-filesystem. This worked with my changes in 25.02 in bookworm, but now doesn't in trixie. And I checked changes to resize script - there's nothing that could've caused the issue. I'll build bookworm and check if that works. This is the issue: here's some debug output: |
|
so, I checked the following combinations in addition to trixie/current:
All combinations have the same output(except uname): But then I went and compared this with the image I built earlier for RPi CM4: Later I'm gonna find a uefi-x86 image that I built earlier and will check the situation there. x86 was built with 6.12.28 kernel and I also used bookworm. I found a fix for this to be the and but also a simple way to confirm it works as expected for non-crypt scenarios: I'm also reading online that the output is not sorted and man pages have this: It's weird why same version returns different order though. But anyways - I'm gonna add One thing I wanted to understand before doing that - @igorpecovnik do you think it's okay solution? Could you explain the reason why there was such validation in the first place? |
We should probably lean toward JSON / machine output of those tools and process that. If that is available here?
Not sure I follow - which validation in specific you are asking. |
yep, it has both --json and --raw (that they call for machine-readable format). Not sure it'll help here though, but may simplify the whole script on the other hand.
Ah, sorry, I already have an idea in my head but didn't explain it out loud :D this is the part that is failing: I'm trying to understand what else there could be besides "part" ? P.s: I already checked that adding |
|
so, I just flushed one of the older uefi-x86 images(bookworm, 6.12.28) to this nvme drive, and... the behavior is the same. However, I realized that all previous times for uefi-x86, and rpi4b the storage was EMMC, and not the nvme drive that I'm using now. And this was the key! When flushing the same dkk image(faster) to EMMC storage the output is the following: Just to confirm it works as expected: So I'm gonna add Will need to check the case with cryptroot disabled, and most likely that's gonna be organepi5 :( @EvilOlaf Regarding orangepi5 - the vendor kernel has booted once, but it seems that it has |
|
I don't know that much about those internals. But if it works for current and edge that be a good starting point. At some point vendor will be abandoned. |
3 participants
Description
This fixes #6280 and enables uefi-x86 builds with CRYPTROOT_ENABLE=yes.
Also, it adds CRYPTROOT_AUTOUNLOCK option, that will generate
/etc/rootfs.keyand use it for unattended unlocking.This is useful for automated scenarios when such key can be further enrolled into TPM afterwards, or just to have illusion of security if user wants so.
Documentation summary for feature / change
armbian/documentation#836
How Has This Been Tested?
I've verified that it builds for uefi-x86 and orangepi5 with CRYPTROOT_ENABLE=yes and CRYPTROOT_AUTOUNLOCK=yes. Later, I'll confirm that it boots.
Checklist:
Please delete options that are not relevant.