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

boot.initrd.secrets fixes #210812

Merged
merged 4 commits into from
Jan 16, 2023
Merged

boot.initrd.secrets fixes #210812

merged 4 commits into from
Jan 16, 2023

Conversation

rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Jan 15, 2023

Description of changes

A couple of fixes for making the user experience of boot.initrd.secrets less miserable.
This completely fixes #73404.

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 with nixosTests.installer.fullDiskEncryption
  • 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
  • 23.05 Release Notes (or backporting 22.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.

@rnhmjoj rnhmjoj requested a review from ncfavier January 15, 2023 02:54
@rnhmjoj rnhmjoj requested a review from dasJ as a code owner January 15, 2023 02:54
rnhmjoj
rnhmjoj commented Jan 15, 2023
View reviewed changes
nixos/tests/installer.nix Outdated Show resolved Hide resolved
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jan 15, 2023

@GrahamcOfBorg test grub installer.fullDiskEncryption

ncfavier
ncfavier reviewed Jan 15, 2023
View reviewed changes
Copy link
Member

@ncfavier ncfavier left a comment

Choose a reason for hiding this comment

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

Not super happy about the bind mount thing but I don't see a better way.

Could you rebase on master and change

nixpkgs/nixos/tests/installer.nix

Line 52 in 2d798b3

boot.initrd.secrets."/etc/secret" = /etc/nixos/secret;

to a relative path?

nixos/tests/installer.nix Show resolved Hide resolved
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jan 15, 2023

Not super happy about the bind mount thing but I don't see a better way.

Yeah, as I wrote in the commit message the proper solution is to build the system within chroot, but that has several problems too and it's a lot of work. It could be something for the future, but for now it's important to just get rid of this bug.

Could you rebase on master and change

Ok!

@rnhmjoj rnhmjoj force-pushed the pr-initrd-secrets branch 2 times, most recently from 5acebe6 to dc81448 Compare January 15, 2023 16:06
@ncfavier
Copy link
Member

There are conflicts now

@rnhmjoj
install-grub.pl: improve initrd-secrets error messages
eccc1e5 
The build of initrd-secrets can routinely fail for old boot entries
if the secrets have been removed or renamed in a later generation.
This always happens for generation 1, because it's built from the
NixOS installer and the paths differs by the mount point (i.e. /mnt).

The error is very confusing because it fails to mention it's about
an older generation and that it's somewhat harmless.

This commit turns the error into a warning for all generations but the
current, adds the name of the failed entry to the message and a note
explaining why it can happen.
@rnhmjoj
nixos-install: fix missing initrd.secrets paths
9fc47e6 
When installing NixOS in the target filesystem /mnt, paths relative to
configuration.nix in `initrd.secrets` are turned by Nix into absolute
paths that reference /mnt. While building the system derivation works,
installing the bootloader fails because the latter process takes place
inside the chroot environment where /mnt does not exist.

Ideally, we would also build the system within chroot, but this greatly
complicates the matter as it requires  manually copying over Nix, its
runtime dependencies and all channels. Possibly, this would also break
several assumptions users have about how nixos-install works.

A simpler and safer (but less neat) solution is to temporarily bind
mount all mount points in /mnt under /mnt/mnt to keep the paths
functional while the bootloader is being installed.
This is essentially the workaround described in issue NixOS#73404.
@rnhmjoj
nixos/tests/installer: add full disk encryption test
928181b 
This tests a common full disk encryption setup:
https://gist.github.com/ladinu/bfebdd90a5afd45dec811296016b2a3f
@rnhmjoj
nixos/tests/installer: test relative paths in initrd secrets
9bb888c
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jan 16, 2023

Fixed the conflicts and re-run the tests.

@rnhmjoj rnhmjoj merged commit 594b94b into NixOS:master Jan 16, 2023
@vcunat
Copy link
Member

vcunat commented Jan 17, 2023 •
edited
Loading

This (commit 9fc47e6) broke our amazonImage build and thus is blocking both unstable NixOS channels. https://hydra.nixos.org/build/205943384

EDIT: local testing e.g. by

nix build -f nixos/release-combined.nix nixos.amazonImage.x86_64-linux

@ncfavier ncfavier mentioned this pull request Jan 17, 2023
Merged
@ncfavier
Copy link
Member

#211218

pwaller
pwaller reviewed Jan 18, 2023
View reviewed changes
nixos/modules/system/boot/loader/grub/install-grub.pl
say STDERR "warning: failed to create initrd secrets for \"$name\", an older generation";
say STDERR "note: this is normal after having removed or renamed a file in `boot.initrd.secrets`";
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You may also be interested in this fix, please take a look:
#209156

@bergkvist bergkvist mentioned this pull request Jan 18, 2023
Closed
@vcunat
Copy link
Member

vcunat commented Jan 19, 2023

Aaand another installer test regression from this PR, also blocking the nixos-unstable channel: https://hydra.nixos.org/build/206057315

$ nix build -f. nixosTests.installer.simpleUefiSystemdBoot
[...]
boot-after-install # cp: cannot stat '/mnt/etc/nixos/secret': No such file or directory
[...]
(finished: cleanup, in 0.00 seconds)
# and now hanging very long; after that occasionally some additional log line

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jan 19, 2023

That's because we have exposed the fact relative paths have never worked before. I have to make the failure a warning in systemd-boot as well.

vcunat added a commit that referenced this pull request Jan 19, 2023
@vcunat
Revert "nixos/tests/installer: test relative paths in initrd secrets"
8d45d82 
This reverts commit 9bb888c from PR #210812.
We first need to fix nixosTests.installer.simpleUefiSystemdBoot
@ncfavier
Copy link
Member

ncfavier commented Jan 19, 2023 •
edited
Loading

I'm confused by this failure. Why is append-initrd-secrets seeing a path that starts with /mnt in the booted system?

EDIT: I think I see. When constructing the boot menu we have to process the old generation in addition to the new one, and appending its secrets using its append-initrd-secrets fails because it refers to the old path.

It's unclear to me why we need to re-invoke append-initrd-secrets for old generations though.

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jan 19, 2023

It's unclear to me why we need to re-invoke append-initrd-secrets for old generations though.

Fair point, in this way it's neither fully stateless nor stateful.

@pwaller
Copy link
Contributor

pwaller commented Jan 21, 2023

When constructing the boot menu we have to process the old generation in addition to the new one, and appending its secrets using its append-initrd-secrets fails because it refers to the old path.

This sounds tangentially related to #209156

It's unclear to me why we need to re-invoke append-initrd-secrets for old generations though.

Theorizing: as with the kernel and initrd: it has the effect of reinstating the available kernels under /boot if they have been removed or modified. It does seem unfortunate though that the secrets were not by-system but instead by-initrd which doesn't make sense when you consider that you might have two system versions with incompatible secrets, and possibly want to boot into either. I tried to fix this in #209156 by naming the append-initrd-secrets after the system instead.

@rnhmjoj rnhmjoj deleted the pr-initrd-secrets branch July 10, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pwaller pwaller pwaller left review comments

@vcunat vcunat vcunat left review comments

@ncfavier ncfavier ncfavier approved these changes

@dasJ dasJ Awaiting requested review from dasJ

@lheckemann lheckemann Awaiting requested review from lheckemann

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
4 participants
@rnhmjoj @ncfavier @vcunat @pwaller