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

[lopsided98]

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

[lopsided98]

@ofborg test initrd-network-ssh

[CRTified]

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]

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

[Mic92]

@GrahamcOfBorg test initrd-network-ssh

[nixos-discourse]

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]

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

[emilazy]

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!)

[Ma27]

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]

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]

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]

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]

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

[stale]

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