-
-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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: support dm-verity #336983
nixos: support dm-verity #336983
Conversation
config = lib.mkIf initrdCfg.enable { | ||
assertions = [ | ||
{ | ||
assertion = initrdCfg.enable -> config.boot.initrd.systemd.enable; | ||
message = '' | ||
'boot.initrd.systemd.dmVerity.enable' requires 'boot.initrd.systemd.enable' to be enabled. | ||
''; | ||
} | ||
]; |
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.
config = lib.mkIf initrdCfg.enable { | |
assertions = [ | |
{ | |
assertion = initrdCfg.enable -> config.boot.initrd.systemd.enable; | |
message = '' | |
'boot.initrd.systemd.dmVerity.enable' requires 'boot.initrd.systemd.enable' to be enabled. | |
''; | |
} | |
]; | |
config = lib.mkIf (config.boot.initrd.systemd.enable && cfg.enable) { |
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 it's better to have the assertion here rather than silently not do anything, or what's your point for dropping it?
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.
Agree with @msanft here.
postInstall = '' | ||
# Replace the placeholder with the real root hash. | ||
realRoothash=$(${pkgs.jq}/bin/jq -r "[.[] | select(.roothash != null)] | .[0].roothash" $out/repart-output.json) | ||
sed -i "0,/${roothashPlaceholder}/ s/${roothashPlaceholder}/$realRoothash/" $out/${oldAttrs.pname}_${oldAttrs.version}.raw |
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.
How I've done this in the past is call repart twice. Once with --defer-partition
.
And then have the uki
derivation depend on the repartPre
derivation. But it's a bit tricky to pull off. As to make sure the first derivation also has no references to the ESP in the nix code to avoid infinite recursion. But I think it's doable with some nix-foo
# repartPre.drv
systemd-repart --json --defer-partition esp $out/image.raw | tee $out/repart-output.json
# uki.drv (depends on repartPre.drv)
roothash=$(jq -r '.[]|select(.type=="root-${linuxArch}") | .roothash' ${repartPre}/repart-output.json)
ukify --cmdline roothash=$roothash $out/Image.efi
# repartFinall.drv (depends on uki)
systemd-repart --json $out/image.raw
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 we'd have to extend the repart module for that first. But yeah, it's kind of a pain-point right now.
It's definitely "cleaner" to do it with a deferred partition though.
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'd have to extend the repart module for that first
This work has already been done by @WilliButz, you can already override the derivation, calling it once with ``--defer-partition` and once without.
7021e68
to
43563f0
Compare
43563f0
to
750a64d
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.
I'm requesting changes only for the doc header (minor) and the option "custom" (bikeshedding, future-proofing).
Everything else, especially the test, looks great. The "worked example" of how to set up something read-only that still allows activation to work is going in my idea pail (🪣) for how to set up my actual dev system.
Thank you for this work Moritz (@msanft)!
750a64d
to
178dc03
Compare
nixos/tests/dm-verity.nix
Outdated
# Minimize = "guess" seems to not work very vell for vfat | ||
# partitons. It's better to set a sensible default instead. The |
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.
# Minimize = "guess" seems to not work very vell for vfat | |
# partitons. It's better to set a sensible default instead. The | |
# Minimize = "guess" seems to not work very well for vfat | |
# partitions. It's better to set a sensible default instead. The |
178dc03
to
d0213a7
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.
Welp merged before I could review it. Anyways:
My problem with this PR is largely the same as with your previous PR. Your test is too complex to be an effective and maintanable test of generic dm-verity functionality:
- You should make clear that you test a dm-verity root, i.e. rename the test from dm-verity.nix -> dm-verity-root.nix
- The test needs to be much simpler. Irrespective of your use-case that you indicated in nixos: support
systemd-veritysetup-generator
#305999 (comment), the test should only test the test subject and not a lot of complex setup that you're interested in but which is not relevant for the generic functionality. This is also a question of maintainability, a good test should allow maintainers to maintain the functionality (dm-verity) without having to care too much about bespoke setup around it. To be more concrete: mount a simple writable overlay over/
instead of mounting etc,var,... separately. That's more than enough to show that a dm-verity root works.
# through the systemd tooling. | ||
systemd = { | ||
additionalUpstreamUnits = [ | ||
# https://github.com/systemd/systemd/blob/main/units/veritysetup-pre.target |
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.
These comments linking to docs add almost no relevant information, quite to the contrary they introduce noise that is distracting.
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 that's my doing, or at least my accidental prompting. When I review, I leave comments where I try to find out what the various constituent parts that are referenced land. Being able to show the research done -- as it were, notes on the margin of the page -- is a critical part of how I review that something is fit for purpose.
- What does this service/tool do?
- Where are its docs?
- What other things can it do, other than what this particular PR lights up?
By commenting on the PR and linking the material, I hope to bring it into the conversation, so that others reviewing it might also bring new answers and new questions prompted by the source material.
I don't mean by commenting in the view that these URLs and reference material need to land in the source. Having them be clickable in the PR review is enough of a breadcrumb to follow, in my view.
Though, unlike the view you express above, I don't find them particularly irksome.
postInstall = '' | ||
# Replace the placeholder with the real root hash. | ||
realRoothash=$(${pkgs.jq}/bin/jq -r "[.[] | select(.roothash != null)] | .[0].roothash" $out/repart-output.json) | ||
sed -i "0,/${roothashPlaceholder}/ s/${roothashPlaceholder}/$realRoothash/" $out/${oldAttrs.pname}_${oldAttrs.version}.raw |
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'd have to extend the repart module for that first
This work has already been done by @WilliButz, you can already override the derivation, calling it once with ``--defer-partition` and once without.
Next time, I'll use "intent to merge in a day" @nikstur, rather than merging immediately. I do think that the test as currently written has value to me, even if it's not minimal. It's a worked example of how I could configure my own system to use Beyond the value of a minimal test, I think that holding a PR up because of test minimality is the wrong approach. Tests have no backwards compatibility concerns and can be iterated on easily in future PRs. I see that @msanft has already started on that. The feedback cycle is much more effective with real-world experience in addition to a canned test. As a result, I'd rather get code velocity and some forward movement, especially if the module's options are sane and minimal themselves. |
Description of changes
Supersedes #305999
This is based mostly on the awesome work done by @nikstur in https://github.com/nikstur/nix-store-veritysetup-generator/
Based on the feedback received in the aforementioned PR, this adds a more flexible way of supporting
dm-verity
in the initrd throughsystemd-veritysetup(-generator)
. This allows to mount filesystems arbitrary verity-protected block devices in the initrd, and enables usage of generators, as shown in the NixOS test added. (Upstream generators likesystemd-veritysetup-generator
and downstream ones likenix-store-veritysetup-generator
alike)Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.