Skip to content
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.

Sign up for GitHub

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

Jump to bottom

nixos/systemd-stage-1: Add basic LUKS support #168554

Merged
merged 2 commits into from
Apr 22, 2022
Merged

nixos/systemd-stage-1: Add basic LUKS support #168554

merged 2 commits into from
Apr 22, 2022

Conversation

dasJ
Copy link
Member

@dasJ dasJ commented Apr 13, 2022 •
edited
Loading

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` and removed 6.topic: systemd labels Apr 13, 2022
@dasJ dasJ marked this pull request as draft April 13, 2022 22:14
@dasJ dasJ marked this pull request as ready for review April 14, 2022 08:53
@dasJ dasJ force-pushed the feat/systemd-stage-1-luks branch from 27a34ef to 97b43ee Compare April 14, 2022 08:54
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Apr 14, 2022
ElvishJerricco
ElvishJerricco reviewed Apr 14, 2022
View reviewed changes
nixos/modules/system/boot/luksroot.nix
"remote-cryptsetup.target"
];
storePaths = [
"${config.boot.initrd.systemd.package}/lib/systemd/systemd-cryptsetup"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is covered by the extraBin, right? Not that it really matters technically.

ElvishJerricco
ElvishJerricco reviewed Apr 14, 2022
View reviewed changes
Copy link
Contributor

@ElvishJerricco ElvishJerricco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!

@dasJ dasJ force-pushed the feat/systemd-stage-1-luks branch from 97b43ee to c481bb1 Compare April 14, 2022 21:08
andersk
andersk reviewed Apr 16, 2022
View reviewed changes
nixos/modules/system/boot/luksroot.nix
Comment on lines +880 to +888
{ assertion = config.boot.initrd.systemd.enable -> all (dev: !dev.fallbackToPassword) (attrValues luks.devices);
message = "boot.initrd.luks.devices.<name>.fallbackToPassword is implied by systemd stage 1.";
}
{ assertion = config.boot.initrd.systemd.enable -> all (dev: dev.preLVM) (attrValues luks.devices);
message = "boot.initrd.luks.devices.<name>.preLVM is not used by systemd stage 1.";
}
{ assertion = config.boot.initrd.systemd.enable -> options.boot.initrd.luks.reusePassphrases.highestPrio == defaultPrio;
message = "boot.initrd.luks.reusePassphrases has no effect with systemd stage 1.";
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these three be warnings?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, why would they be?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine the rationale is that these don't break anything if set when systemd stage 1 is enabled, and therefore they should be warnings as opposed to assertions.

I'm uncertain whether that is correct though, since setting something that should affect boot but doesn't might break your boot, even if it didn't break the config eval :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, having preLVM or reusePassphrases set won't break your expectations, as systemd stage 1 does the correct thing either way so it will do what you wanted without regard for those options. Disabling fallbackToPassword would have unexpected results if we didn't error the eval though.

I don't particularly care one way or the other about warnings vs assertions for preLVM and reusePassphrases though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's exactly the motivation. If you set something that is not processed by the new initrd, it might prevent your system from booting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"need" was too strong a word. I don't think we really need to worry about this concern very much in the first place TBH.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andersk I'm suggesting that if a user writes fallbackToPassword = false; in their nixos configuration, we need that to be an error, not a warning, because it will not behave as expected.

That would be a reasonable check. It is not, however, the check performed by this code. This code throws an error on fallbackToPassword = true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if people did fallbackToPassword = lib.mkIf (!config.boot.initrd.systemd.enable) false?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping this an assertion makes sure there are no issues. Making it a warning might cause issues.

Let's please land this and revisit before making the options public. Assertion failures when switching stage-1 implementations are to be expected, especially at this stage.

We do not need to tackle the distinction between what should be a warning or an assertion right now.

Let's first make it work, then we can worry about making it good, and finally, we can make it easy :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/NixOS/nixpkgs/projects/51#card-81007876

@dasJ dasJ force-pushed the feat/systemd-stage-1-luks branch from c481bb1 to 76f3903 Compare April 16, 2022 19:42
lovesegfault
lovesegfault approved these changes Apr 16, 2022
View reviewed changes
Copy link
Member

@lovesegfault lovesegfault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it!

@arianvp
Copy link
Member

arianvp commented Apr 18, 2022

I tried this on my local system and it's working. LGTM

@PAI5REECHO
Copy link

Does this address #74281 at all?

@dasJ dasJ force-pushed the feat/systemd-stage-1-luks branch from 76f3903 to 28c7721 Compare April 18, 2022 10:43
@dasJ
Copy link
Member Author

dasJ commented Apr 18, 2022

CI failure unrelated. Comes from #124019

@lovesegfault lovesegfault merged commit b23ec41 into NixOS:master Apr 22, 2022
@ajs124 ajs124 deleted the feat/systemd-stage-1-luks branch April 22, 2022 21:50
@vcunat
Copy link
Member

vcunat commented Apr 23, 2022

The tests don't succeed much on Hydra: https://hydra.nixos.org/eval/1757428#tabs-new

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-unlock-some-luks-devices-with-a-keyfile-on-a-first-luks-device/18949/5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lovesegfault lovesegfault lovesegfault approved these changes

@ElvishJerricco ElvishJerricco ElvishJerricco left review comments

@andersk andersk andersk left review comments

@arianvp arianvp arianvp approved these changes

@ajs124 ajs124 Awaiting requested review from ajs124

@bobvanderlinden bobvanderlinden Awaiting requested review from bobvanderlinden

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
systemd in Stage 1
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@dasJ @arianvp @PAI5REECHO @vcunat @nixos-discourse @andersk @ElvishJerricco @lovesegfault