Fix and improve GELI disk encryption support#25
Merged
Conversation
- Fix GELI initialization parameters: Use explicit AES-XTS cipher, 256-bit key length, and 4096-byte sector size (matching FreeBSD/bsdinstall defaults) instead of relying on implicit defaults - Fix encrypted swap handling: Write .eli suffix in fstab for encrypted swap partitions so the kernel creates a one-time GELI provider with a random key at boot, matching standard FreeBSD behavior - Fix ZFS ashift sysctl: Correct vfs.zfs.min_auto_ashift to vfs.zfs.vdev.min_auto_ashift - Fix GELI cleanup: Clear stale GELI metadata before geli init to prevent failures on reinstall, and use rc_nohalt for detach/clear to avoid aborting on already-detached providers - Fix loader.conf entries: Only write geom_eli_load and aesni_load when encryption is actually in use; remove redundant crypto_load entry - Load aesni module during GELI setup for hardware-accelerated encryption - Increase key file size from 64 bytes to 4096 bytes for stronger key-file-only encryption - Fix livezfs extraction: Wrap cpdup call with rc_halt_echo for proper error handling - Remove -g (geliboot) flag from key-file mode where it is unnecessary — only passphrase mode needs geliboot for the loader prompt - Update examples and documentation to reflect ZFS+GELI layout with encrypted swap
Reviewer's GuideRefines GELI-based disk encryption handling across installation scripts by aligning GELI init parameters with FreeBSD defaults, correctly configuring encrypted swap and loader.conf behavior, fixing ZFS ashift sysctl usage, making GELI cleanup more robust, and tightening live ZFS extraction error handling and documentation. Sequence diagram for GELI-encrypted ZFS passphrase boot flowsequenceDiagram
actor User
participant Loader as FreeBSD_loader
participant Kernel as FreeBSD_kernel
participant GEOMELI as geom_eli
participant ZFS as ZFS_vdev
User->>Loader: Power on system
Loader->>Loader: Read boot_loader_conf
Loader->>Kernel: Load module geom_eli (geom_eli_load=YES)
Loader->>Kernel: Load module aesni (aesni_load=YES)
Loader->>User: Prompt for GELI passphrase (-bg on geli init)
User->>Loader: Enter passphrase
Loader->>GEOMELI: Attach GELI provider for data partition
GEOMELI-->>Loader: Provider created partition_eli
Loader->>ZFS: Import ZFS pool on partition_eli
ZFS-->>Loader: Pool imported
Loader->>Kernel: Hand off boot to kernel
Kernel->>GEOMELI: Use attached providers for I O
Kernel->>ZFS: Mount root filesystem
ZFS-->>Kernel: Root mounted
Kernel-->>User: Encrypted system booted
Flow diagram for GELI setup in setup_filesystemsflowchart TD
A[Start setup_filesystems for partition] --> B{PARTENC = ON?}
B -- No --> C[Set EXT empty string]
C --> Z[Continue with filesystem setup]
B -- Yes --> D{PARTFS = SWAP?}
D -- Yes --> E[Do not run geli init]
E --> F[Set EXT empty string so glabel uses raw device]
F --> Z
D -- No --> G[Load geom_eli module if needed]
G --> H[Load aesni module]
H --> I[Clear stale GELI metadata geli clear PARTDEV]
I --> J{Passphrase file exists?}
J -- Yes --> K[geli init -bg -e AES-XTS -l 256 -s 4096 -J encpass PARTDEV]
K --> L[geli attach -j encpass PARTDEV]
L --> M[Set EXT .eli]
M --> N{Mirrored ZFS disks configured?}
N -- Yes --> O[Loop over GELI_CLONE_ZFS_DISKS]
O --> P[geli init -b -e AES-XTS -l 256 -s 4096 -J encpass mirror_disk]
P --> Q[geli attach -j encpass mirror_disk]
Q --> Z
N -- No --> Z
J -- No --> R[dd if=/dev/random of=GELIKEYDIR PART key bs=4096 count=1]
R --> S[geli init -b -e AES-XTS -l 256 -s 4096 -P -K key PARTDEV]
S --> T[geli attach -p -k key PARTDEV]
T --> M
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- When setting up mirrored ZFS GELI members in
setup_filesystems(), you now initialise only the primary device withgeli clearand aesni loaded, but the mirror members are initialised directly; for consistency and to avoid leftover metadata, consider applying the samegeli clearand module-loading logic to each mirror disk beforegeli init.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When setting up mirrored ZFS GELI members in `setup_filesystems()`, you now initialise only the primary device with `geli clear` and aesni loaded, but the mirror members are initialised directly; for consistency and to avoid leftover metadata, consider applying the same `geli clear` and module-loading logic to each mirror disk before `geli init`.
## Individual Comments
### Comment 1
<location path="backend/functions-disk.sh" line_range="419-423" />
<code_context>
+ done
+
+ # Clear GELI metadata from all partitions on this disk
+ for i in `ls ${_geld}*`
+ do
+ echo $i | grep -q '.eli' 2>/dev/null
+ if [ $? -ne 0 ] ; then
+ rc_nohalt "geli clear ${i}"
fi
done
</code_context>
<issue_to_address>
**issue:** Using command substitution with `ls ${_geld}*` can break when the glob does not match and may feed error text into the loop.
If `${_geld}*` matches nothing, `ls` prints `No such file or directory` to stdout and exits non‑zero, and that error text is then treated as a path in the loop. A safer approach is to iterate over the glob directly and skip non‑existent entries:
```sh
for i in ${_geld}*; do
[ -e "$i" ] || continue
echo "$i" | grep -q '.eli' 2>/dev/null || rc_nohalt "geli clear ${i}"
done
```
This removes the `ls` command substitution and correctly handles the no‑match case.
</issue_to_address>
### Comment 2
<location path="backend/functions-newfs.sh" line_range="165-174" />
<code_context>
+ rc_halt "geli init -bg -e AES-XTS -l 256 -s 4096 -J ${PARTDIR}-enc/${PART}-encpass ${PARTDEV}"
</code_context>
<issue_to_address>
**question (bug_risk):** Mirror members are initialised without `-g`, which may diverge from the boot behaviour of the primary device.
Mirrored devices created by this code path are now initialised without `-g`, whereas they previously were and the main device still is. If these mirrors are ever used in a boot pool or need geliboot metadata for early‑boot attachment, this mismatch could cause them to behave differently or fail to boot. If mirrors are intended to remain bootable, please also include `-g` when initialising the cloned devices.
</issue_to_address>
### Comment 3
<location path="backend/functions-newfs.sh" line_range="177-182" />
<code_context>
fi
EXT=".eli"
+ elif [ "${PARTENC}" = "ON" -a "${PARTFS}" = "SWAP" ]
+ then
+ # Swap encryption uses one-time random keys managed by the kernel
+ # at boot via the .eli suffix in fstab. No geli init is needed;
+ # just skip the EXT so glabel operates on the raw device.
+ EXT=""
else
# No Encryption
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `-a` in `[ ... ]` can be brittle; `&&` in a `test`/`[` context is generally safer.
`[ "${PARTENC}" = "ON" -a "${PARTFS}" = "SWAP" ]` uses `-a`, which has precedence quirks and can misbehave with empty or space-containing values. Prefer chaining tests with `&&`:
```sh
elif [ "${PARTENC}" = "ON" ] && [ "${PARTFS}" = "SWAP" ]; then
...
fi
```
This is more robust and easier to read in `/bin/sh`.
Suggested implementation:
```
EXT=".eli"
elif [ "${PARTENC}" = "ON" ] && [ "${PARTFS}" = "SWAP" ]
then
# Swap encryption uses one-time random keys managed by the kernel
# at boot via the .eli suffix in fstab. No geli init is needed;
# just skip the EXT so glabel operates on the raw device.
EXT=""
else
```
```
# If we are doing mirrored ZFS disks, initialise GELI on each mirror member
# using the same passphrase so all members can be unlocked at boot.
if [ -n "$GELI_CLONE_ZFS_DISKS" ] && [ "$GELI_CLONE_ZFS_DEV" = "$PARTDEV" ] ; then
for gC in $GELI_CLONE_ZFS_DISKS
do
echo_log "Setting up GELI on mirrored disks: ${gC}"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Replace `-a` with `&&` in conditional statements for better readability and consistency. - Add `-g` (geliboot) flag during GELI initialization in mirrored ZFS disk setup. - Simplify GELI metadata clearing loop by streamlining checks to handle missing files gracefully.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Improve and harden GELI disk encryption handling across installation, cleanup, and boot, aligning behavior with standard FreeBSD defaults.
Bug Fixes:
Enhancements: