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.
| 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.
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.
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.
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.
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.
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.
msanft
force-pushed
the
msanft/dm-verity
branch
from
7021e68
to
43563f0
Compare
msanft
force-pushed
the
msanft/dm-verity
branch
from
43563f0
to
750a64d
Compare
There was a problem hiding this comment.
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)!
msanft
force-pushed
the
msanft/dm-verity
branch
from
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.
| # 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 |
msanft
force-pushed
the
msanft/dm-verity
branch
from
178dc03
to
d0213a7
Compare
There was a problem hiding this comment.
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.
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.
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.
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-verityin 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-generatorand downstream ones likenix-store-veritysetup-generatoralike)Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.