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.
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
nixos/clevis: init & support for zfs, bcachefs and luks #257525
nixos/clevis: init & support for zfs, bcachefs and luks #257525
Changes from all commits
27493b4
3aa4ed0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit: too many linebreaks here.
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.
No, this should say also that the device may not have the supported FS: zfs or bcachefs.
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.
nit: that's exactly what
lib.optional
give you.nit:
cfg.useTag -> !(cfg.boot.initrd.network.enable || cfg.boot.initrd.systemd.network.enable)
is more readable to me but I may be wrong in the conditional.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.
nit: indent the let binding on the next line otherwise this is unreadable.
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.
One way you could address @RaitoBezarius's comment would be to make a service per device, and wed it to the systemd.device showing up. I think that's pretty clean, and it's a 1:1 map to the code in nixos/modules/system/boot/luksroot.nix
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.
Thank you, I think I'll address these remarks in a follow-up PR.
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.
nit: the name could be factored out in a let-binding as you use it on the next line.
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.
systemd-udev-settle is broken by design and should not be used, ever. See #73095 (comment).
I don't know what you're trying to depend on, but this is most likely not the correct way.
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.
We use that to wait for TPM2 to be available. Do you know of another synchronization point for that ?
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.
If you know the exact device path (say under /dev/ or /sys) in advance you can add a dependency on a device unit (see
systemd.device(5)
). Otherwise you could listen for the specific udev events usingudevadm monitor -s <subsystem>/<devicetype>
in you script. There's an example here.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.
It seems TPM2 devices get linked to /dev/tpmrm, so you should probably add a
Wants=dev-tpmrm0.device
dependency.I don't know if systemd handles TPMs devices by default, though. If not you'll also have to add a
TAG+=systemd
rule on those devices.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.
Ah, look what was just commited to systemd: systemd/systemd@4e1f003
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.
Yea, in the future, we should definitely change this to use
tpm2.target
. At the moment, I don't know if we can add dependencies ondev-tpmrm0.device
directly, because that would cause boot to delay waiting for that device to timeout in the event that one doesn't exist.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.
The tpm2 target does exactly this: it adds a dependecy on
dev-tpmrm0.device
, so it will time out without a TPM.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.
@rnhmjoj But
tpm2.target
is not in the initial transaction by default. It only gets added bysystemd-tpm2-generator
if it detects that the TPM2 exists in/dev
or according to the firmware (via efi variables). Units aren't supposed to haveWants=tpm2.target
, onlyAfter=tpm2.target
so that they're ordered properly when a TPM exists.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.
Ah, I see. Anyway, it seems that this service does require a TPM, so should it be if not a timeout?
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.
@rnhmjoj I don't think a TPM is required necessarily? I think the
.jwe
determines if a TPM is required.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.
nit/future: what is the design for multi-device unlocks via Clevis in case of degraded devices? e.g. one of the disk is missing but the other is here, how does that behave?
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.
I think this is mishandling fileSystems with
device = null
(the default).For example, I'm mounting
/boot
by label withThere 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.
Yes, I think you are right
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.
If the entire zfs pool is encrypted it will ask the password before this check and clevis won't be able to decrypt the pool.I have this config:
The filesystems config is roughly like this:
With this config it tries to execute
zfs load-key rpool/safe/root
which fails withKey load error: Keys must be loaded for encryption root of 'rpool/safe/root'
, but it works if I executezfs load-key rpool
:If I use
boot.initrd.clevis.devices."rpool".secretFile = "/etc/initrd/clevis/rpool.jwe";
it fails to evaluate:This looks like a module deficiency to me.
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.
nit: you better factor out the string logic for a single item in a
mkClevisDecryptCommand
so that we can distinguish things better.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.
Wouldn't this add calls to
clevis
even whenboot.initrd.clevis.enable
is false?I think that's why I started getting this evaluation error yesterday: https://gist.github.com/aij/bb5c49f0afbde2b3efdfc8f30c1e7b6e
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.
Do you have clevis devices set but clevis disabled ? Or do you have nothing clevis related in your config but still evaluation errors ?
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.
Thank you for reporting that, I am proposing a fix here: #272061
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.
Sorry about the delay. FWIW, I had nothing clevis related in my configs.
Fix looks good. Thanks!