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

ssh: use symlinks for authorizedKeys options #976

Merged
merged 2 commits into from
Jul 10, 2024

Conversation

emilazy
Copy link
Collaborator

@emilazy emilazy commented Jun 15, 2024

This is a security fix over a year in the making; I wish I had been able to get it out sooner. Anyone using AuthorizedKeysCommand for something else will need to ensure they set it in a later file than 101-authorized-keys.conf, but hopefully anyone with that customized an SSH setup knows what they’re doing, and I’m not sure how we could better signal that; I doubt anyone reads the changelog but not merged PRs.

This does mean that the Nix store failing to mount could lead to an SSH lock‐out; I’m not sure how we could handle that elegantly and it seems difficult to recover from without direct access anyway (what if your shell is in the Nix store anyway?). It’s possible we could do an ad‐hoc recreation of copy here that operates unconditionally on the entire directory, I guess.

As explained in the changelog and activation check, the previous
implementation had a nasty security bug that made removing a user’s
authorized keys effectively a no‐op.
This is a huge anti‐declarative footgun; `copy` files cannot
distinguish if a previous version is managed by nix-darwin, so they
can’t check the hash, so they’re prone to destroying data, and
copied files are not deleted when they’re removed from the system
configuration, which led to a security bug. Nothing else in‐tree
was using this functionality, so let’s make sure it doesn’t
cause any more bugs.
@emilazy emilazy requested a review from Enzime June 15, 2024 11:16
@emilazy
Copy link
Collaborator Author

emilazy commented Jun 15, 2024

Incidentally it seems like our /etc code doesn’t clean up empty directories that consisted entirely of Nix store symlinks. That seems unfortunate, but I’m not sure how we could fix it. In this case it seems like it would be best for the entire /etc/ssh/nix_authorized_keys.d directory to be a symlink; it would be easy to handle deleting them in that case. Not sure why it doesn’t work like that presently.

Comment on lines +99 to +101
imports = [
(mkRemovedOptionModule [ "services" "openssh" "authorizedKeysFiles" ] "No `nix-darwin` equivalent to this NixOS option.")
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I added this because SSH keys specified in ~/.ssh/authorized_keys stopped working (see #677), ideally it would be nice if this PR could maintain that, potentially by adding it to the AuthorizedKeysCommand

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that should work out of the box now, since we don’t override AuthorizedKeysFile:

          The  program should produce on standard output zero or more lines
          of authorized_keys output (see AUTHORIZED_KEYS in sshd(8)) .  Au‐
          thorizedKeysCommand is tried after the  usual  AuthorizedKeysFile
          files  and will not be executed if a matching key is found there.
          By default, no AuthorizedKeysCommand is run.

@Enzime
Copy link
Collaborator

Enzime commented Jul 9, 2024

Everything else in the PR LGTM

Thanks for fixing this issue, I've known about it for a while but I never got around to fixing it as I was planning to add functionality that tracked which files were created by nix-darwin and then wipe them if they're no longer necessary (similar to the NixOS boot partition manager script)

@emilazy
Copy link
Collaborator Author

emilazy commented Jul 9, 2024

Yeah, I was talking with @Samasaur1 on Matrix and it seems like we might be able to implement a more robust copy mode by diffing against the currently‐active generation, but I don’t feel 100% confident in our ability to make it reliably clean up without ever trashing user data at present.

@Enzime
Copy link
Collaborator

Enzime commented Jul 9, 2024

My preference would be to write proper state management logic and write a program that writes to a manifest file (JSON probably) that tracks all the generated files by nix-darwin with their original hashes so we know what we can safely override. Alternatively, we could go down the home-manager path which is any time we want to override a file, we can error out and tell the user to deal with it.

Copy link
Collaborator

@Enzime Enzime left a comment

Choose a reason for hiding this comment

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

LGTM

Happy to merge this PR first and if any future consumers want to use .copy they can readd it in their PR with better state management logic

@emilazy emilazy merged commit cf297a8 into LnL7:master Jul 10, 2024
6 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.

2 participants