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.

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

copy over ssh host keys #229

Merged
merged 7 commits into from
Oct 4, 2023
Merged

copy over ssh host keys #229

merged 7 commits into from
Oct 4, 2023

Conversation

tie
Copy link
Contributor

@tie tie commented Sep 29, 2023

@@ -29,6 +29,8 @@ Options:
use another kexec tarball to bootstrap NixOS
* --post-kexec-ssh-port <ssh_port>
after kexec is executed, use a custom ssh port to connect. Defaults to 22
* --no-copy-host-keys
Copy link
Member

@Mic92 Mic92 Sep 30, 2023

Choose a reason for hiding this comment

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

I would like to make this not a default for the following reasons:

  • Users might want to copy their own keys in with --extra-files and this option would override it which is surprising (i.e. when you use agenix and the host key is used to decrypt secrets)
  • I wouldn't trust the original OS to have good ssh keys. Some cloud providers are not great and may have the same ssh keys for every Recovery OS by accident. Same issue would be if someone generates an installer iso with a pregenerated ssh key.

So can we invert the logic here and make it opt-in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some cloud providers are not great and may have the same ssh keys for every Recovery OS by accident.

Good point.

Users might want to copy their own keys in with --extra-files and this option would override it which is surprising (i.e. when you use agenix and the host key is used to decrypt secrets)

To be fair, current behavior is also surprising because we lose the host key that is used to decrypt the secrets after installation. We shouldn’t be overwriting existing files in /mnt though.

So can we invert the logic here and make it opt-in?

Agreed, I’ll invert the logic and avoid copying files if the destination already exists.

Now that I think about it, we can have a more generic --preserve-files that can be specified multiple times to preserve files from before kexec, and have --copy-host-keys be a convenience shortcut to keep SSH host keys. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, though that would potentially be non-trivial to implement while keeping all the permissions and directory structure, and I’d like to limit the scope of this PR to copying SSH host keys.

@Mic92
Copy link
Member

Mic92 commented Oct 3, 2023

Have you tested this?

I am currently getting this error (see the test I added):

installer # + ssh -T -i /tmp/tmp.kBX9n8Yng2/nixos-anywhere -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no root@installed 'chmod 755 /mnt'
installer # Warning: Permanently added 'installed' (ED25519) to the list of known hosts.
installer # + step Installing NixOS
installer # + echo '### Installing NixOS ###'
installer # ### Installing NixOS ###
installer # + ssh_ bash
installer # + ssh -T -i /tmp/tmp.kBX9n8Yng2/nixos-anywhere -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no root@installed bash
installer # Warning: Permanently added 'installed' (ED25519) to the list of known hosts.
installer # + export PATH=/run/wrappers/bin:/root/.nix-profile/bin:/etc/profiles/per-user/root/bin:/nix/var/nix/profiles/default/bin:/run/current-system/sw/bin:/run/current-system/sw/bin
installer # + PATH=/run/wrappers/bin:/root/.nix-profile/bin:/etc/profiles/per-user/root/bin:/nix/var/nix/profiles/default/bin:/run/current-system/sw/bin:/run/current-system/sw/bin
installer # + mkdir -p /mnt/tmp
installer # + chmod 777 /mnt/tmp
installer # bash: line 18: syntax error near unexpected token `done'
installer # + rm -rf /tmp/tmp.kBX9n8Yng2

@tie
Copy link
Contributor Author

tie commented Oct 3, 2023

Yes, I did test and deploy the initial PR, but didn’t check changes made after review. There is a minor typo in the shell script, end instead of fi 🫠

Thank you for adding test case, I’ll update it to verify that ssh-keyscan returns the same list of keys.

@Lassulus
Copy link
Contributor

Lassulus commented Oct 4, 2023

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Oct 4, 2023

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at ffcbf8c

@mergify mergify bot merged commit ffcbf8c into nix-community:main Oct 4, 2023
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants