Skip to content

nixos/tests/initrd-network-ssh: fix with real initrd secrets implementation#91744

Closed
lopsided98 wants to merge 2 commits intoNixOS:masterfrom
lopsided98:fix-initrd-ssh-test
Closed

nixos/tests/initrd-network-ssh: fix with real initrd secrets implementation#91744
lopsided98 wants to merge 2 commits intoNixOS:masterfrom
lopsided98:fix-initrd-ssh-test

Conversation

@lopsided98
Copy link
Contributor

@lopsided98 lopsided98 commented Jun 28, 2020

Motivation for this change

Fixes the initrd-network-ssh test, which was broken by the use of a real initrd secrets implementation. This was fixed by enabling the bootloader in the VM, which also exposed a few other bugs along the way.

The -serial pty QEMU option specified in the boot disk image builder prevented errors from being shown, and -nographics is redundant.

For the secret to be accessible to the VM disk builder, it needed to be added to the store, which broke some assumptions in the initrd-ssh module. I removed some hacks there that do not seem to be necessary, though I may be missing some problem with this.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc @emilazy @CRTified

@ofborg ofborg 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/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jun 28, 2020
@lopsided98 lopsided98 changed the title Fix initrd ssh test Fix initrd-ssh test Jun 28, 2020
@lopsided98 lopsided98 changed the title Fix initrd-ssh test Fix initrd-network-ssh test Jun 28, 2020
This was referenced Jun 28, 2020
Merged
Merged
@lopsided98
Copy link
Contributor Author

lopsided98 commented Jun 28, 2020

@ofborg test initrd-network-ssh

@lopsided98 lopsided98 changed the title Fix initrd-network-ssh test nixos/tests/initrd-network-ssh: fix with real initrd secrets implementation Jun 28, 2020
@CRTified
Copy link
Contributor

CRTified commented Jun 28, 2020

Thank you for taking care of this :)
Would it make sense to change the type of hostKeys to types.str then, as boot.initrd.secrets expects strings in the end?

@lopsided98
Copy link
Contributor Author

lopsided98 commented Jun 29, 2020

It looks like virtualisation.useBootLoader is broken on aarch64 and it looks pretty involved to make it work.

@Ma27 Ma27 requested a review from WilliButz June 29, 2020 14:34
@lopsided98 lopsided98 force-pushed the fix-initrd-ssh-test branch from 920f532 to 06fc2a6 Compare July 11, 2020 20:45
@lopsided98 lopsided98 mentioned this pull request Jul 11, 2020
Merged
10 tasks
@Mic92
Copy link
Member

Mic92 commented Jul 14, 2020

@GrahamcOfBorg test initrd-network-ssh

@nixos-discourse
Copy link

nixos-discourse commented Jul 23, 2020

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

https://discourse.nixos.org/t/full-encrypted-nixos-system-on-legacy-boot-with-secrets-and-remote-unlock-for-unstable-20-03/8279/1

@Ma27
Copy link
Member

Ma27 commented Sep 12, 2020

ping @emilazy could you please take a look? :)
Also, we should probably document this behavior.

@emilazy
Copy link
Member

emilazy commented Sep 12, 2020 •
edited
Loading

Unfortunately I no longer remember what the builtins.unsafeDiscardStringContext was needed for; I worry a little about breaking working configs but it's probably fine to remove. I think it would be nice to still make hostKeys = [ ./ssh_host_ed25519_key ] work (although it's arguably a footgun), but if it doesn't then the type of the option should probably be updated. (Sorry for not getting to this sooner!)

@lopsided98 lopsided98 force-pushed the fix-initrd-ssh-test branch 2 times, most recently from ce8dac5 to 9e27deb Compare September 12, 2020 18:22
@Ma27
Copy link
Member

Ma27 commented Sep 12, 2020

I worry a little about breaking working configs

@emilazy now that I think of it, I fully agree that this is quite important to keep in mind! For instance, I'm now using hostKeys = [ /run/keys/sshkey ] rather than ["/run/keys/sshkey"] to make sure that the file from the key directory of nixops gets copied to /etc/ssh/sshkey and AFAICS this patch would leak my hostkey into the nix-store when deploying it with my current config.

@emilazy
Copy link
Member

emilazy commented Sep 12, 2020

If your bootloader has initrd secrets support it's generally preferable to use the quoted paths; non-quoted paths will always be copied to the Nix store, whereas quoted paths are processed by the initrd secrets generator during nixos-rebuild. i haven't used NixOps so unfortunately i'm not sure how it affects this calculus. initrd secrets are flaky in a bunch of ways, e.g. #85000 (unfortunately pretty much amounts to "implement proper initrd secrets support for every bootloader", I think), #85563.

@Ma27
Copy link
Member

Ma27 commented Sep 12, 2020

No they don't, unless you do e.g. "${./.}" or am I misunderstanding you? As long as they get converted back to a string (e.g. with toString or in this case baseNameOf) no copy happens.

@lopsided98
nixos/initrd-ssh: fix handling of store paths
220ef94
@lopsided98
nixos/tests/initrd-network-sshd: fix using a real initrd secrets impl…
05e073d 
…ementation

Previously, the test did not use a bootloader, but was still configured to use
GRUB, which did not have an initrd secrets implementation, so the secrets were
stored in the Nix store. Now that GRUB has a initrd secrets implementation, we
need to use a bootloader in the VM so the secrets get copied correctly.
@lopsided98
Copy link
Contributor Author

lopsided98 commented Sep 12, 2020

Yes, there are a lot of edge cases here. I think I have fixed it now.

In the test, the bootloader is installed in a VM (using runInLinuxVM), therefore the key must be in the store, making it difficult to test some of the more realistic cases. "${./ssh_host_ed25519_key}" results in a string (ie. isString == true) with a context that references the store path and needs to be removed. Therefore, I moved the unsafeDiscardStringContext call to cover both branches of initrdKeyPath. This allows the test to work and shouldn't break anyone's config.

@emilazy emilazy mentioned this pull request Sep 16, 2020
Open
@emilazy emilazy mentioned this pull request Nov 3, 2020
Merged
10 tasks
@emilazy
Copy link
Member

emilazy commented Nov 3, 2020

This PR probably wants to revert or at least adjust #102530.

@stale
Copy link

stale bot commented Jun 4, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 4, 2021
@lopsided98 lopsided98 closed this Jun 4, 2021
@lopsided98 lopsided98 deleted the fix-initrd-ssh-test branch July 24, 2022 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WilliButz WilliButz Awaiting requested review from WilliButz

Assignees

No one assigned

Labels

2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 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: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@lopsided98 @CRTified @Mic92 @nixos-discourse @Ma27 @emilazy