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
Mutable users vs authorizedKeysFiles #42526
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes behaviour and might break existing setups, this definitively needs a changelog entry. And maybe we should keep the current behaviour for stateVersion < 18.09
to avoid potentially loosing access after upgrading.
nixos/modules/security/pam.nix
Outdated
@@ -287,7 +287,7 @@ let | |||
${optionalString cfg.logFailures | |||
"auth required pam_tally.so"} | |||
${optionalString (config.security.pam.enableSSHAgentAuth && cfg.sshAgentAuth) | |||
"auth sufficient ${pkgs.pam_ssh_agent_auth}/libexec/pam_ssh_agent_auth.so file=~/.ssh/authorized_keys:~/.ssh/authorized_keys2:/etc/ssh/authorized_keys.d/%u"} | |||
"auth sufficient ${pkgs.pam_ssh_agent_auth}/libexec/pam_ssh_agent_auth.so file=${concatStringsSet ":" config.services.openssh.authorizedKeysFiles}"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concatStringsSet
-> concatStringsSep
There are security concerns: someone could upload a new SSH key, and we will never know. This still can be extended or overridden. The %h placeholder is added to fit both sshd_config and pam_ssh_agent_auth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be surprising, so it can use a bit of documentation under mutableUsers. Otherwise it's a good idea.
This PR is related with the security vulnerability #31611. I don't think this should be merged as is until the issue is resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 on the change in principle.
I wonder if a special option like users.mutableUserSshKeys
that defaults on the value of users.mutableUsers
might make sense for people depending on the old behaviour with users.mutableUsers
turned on. Or at least mention changing services.openssh.authorizedKeysFiles
in the release notes, which we btw also need since this is a breaking change.
@@ -287,7 +287,7 @@ let | |||
${optionalString cfg.logFailures | |||
"auth required pam_tally.so"} | |||
${optionalString (config.security.pam.enableSSHAgentAuth && cfg.sshAgentAuth) | |||
"auth sufficient ${pkgs.pam_ssh_agent_auth}/libexec/pam_ssh_agent_auth.so file=~/.ssh/authorized_keys:~/.ssh/authorized_keys2:/etc/ssh/authorized_keys.d/%u"} | |||
"auth sufficient ${pkgs.pam_ssh_agent_auth}/libexec/pam_ssh_agent_auth.so file=${concatStringsSep ":" config.services.openssh.authorizedKeysFiles}"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change will be obsolete by #62317.
Thank you for your contributions.
|
@ip1981 please resolve the merge conflict. |
I marked this as stale due to inactivity. → More info |
No mutable SSH keys by default for immutable users
There are security concerns: someone could upload a new SSH key,
and we will never know. This still can be extended or overridden.
The %h placeholder is added to fit both sshd_config and pam_ssh_agent_auth.
Also synchronize keys with openssh.