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

modules/security/pam.nix: add enableSSHAgentAuthNoHome #102187

Closed
wants to merge 1 commit into from
Closed

modules/security/pam.nix: add enableSSHAgentAuthNoHome #102187

wants to merge 1 commit into from

Conversation

@jkarlson
Copy link

@jkarlson jkarlson commented Oct 30, 2020

This variant of the old sshAgentAuth is useful, if sudo is
being used by authenticating as the user and not
as root as is the default as user could normally just
edit the ~/.ssh/authorized_keys to get access as root.

Motivation for this change

Make pam_ssh_agent_auth useful for sudo default settings

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • [ x] 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.
@lourkeur
Copy link
Contributor

@lourkeur lourkeur commented Oct 31, 2020

Tested on sudo on Nightingale. It can prevent sudoing based on just the user-editable files, but you have to be super explicit about it:

{ lib, ... }:
{
  security.pam.services.sudo = {
    sshAgentAuth = lib.mkForce false;
    sshAgentAuthNoHome = true;
  };
}

otherwise you get 2 auth_sufficient lines in /etc/pam.d/sudo. That's because nixos/modules/security/sudo.nix enables it for convenience. That is a usability problem in my opinion. This raises the question of what the config generator should do if both sshAgentAuth and sshAgentAuthNoHome are set. I would say it should either inhibit sshAgentAuth and use the stricter setting, or perhaps fail an assert.

@jkarlson
Copy link
Author

@jkarlson jkarlson commented Oct 31, 2020

Tested on sudo on Nightingale. It can prevent sudoing based on just the user-editable files, but you have to be super explicit about it:

{ lib, ... }:
{
  security.pam.services.sudo = {
    sshAgentAuth = lib.mkForce false;
    sshAgentAuthNoHome = true;
  };
}

otherwise you get 2 auth_sufficient lines in /etc/pam.d/sudo. That's because nixos/modules/security/sudo.nix enables it for convenience. That is a usability problem in my opinion. This raises the question of what the config generator should do if both sshAgentAuth and sshAgentAuthNoHome are set. I would say it should either inhibit sshAgentAuth and use the stricter setting, or perhaps fail an assert.

note optionalString cfg.sshAgentAuthNoHome not (config.security.pam.enableSSHAgentAuth && cfg.sshAgentAuth) so you don't need to have config.security.pam.enableSSHAgentAuth enabled and you don't get the double entry.

I absolutely dislike the current solution and my solution, I would only have cfg.sshAgentAuth boolean and cfg.sshAgentAuthPath listOf str, but that would break backwards compat, so I proposed this more conservative solution that bypasses the current config.

@jkarlson
Copy link
Author

@jkarlson jkarlson commented Oct 31, 2020

I guess there is backwards compatible solution of making

     ${optionalString cfg.sshAgentAuth
          "auth sufficient ${pkgs.pam_ssh_agent_auth}/libexec/pam_ssh_agent_auth.so file=${lib.concatStringsSep ":" cfg.sshAgentAuthPaths}"}

where default paths would be the old paths and for new style config you would just unset config.security.pam.enableSSHAgentAuth and set paths and boolean however you needed

this would only break configs that had noop config of cfg.sshAgentAuth for some security.pam.services.name, but not config.security.pam.enableSSHAgentAuth, which would be inert

This defining sshAgentAuthPaths is useful, if sudo is
being used by authenticating as the user and not
as root as is the default as user could normally just
edit the ~/.ssh/authorized_keys to get access as root.

Allow specifying sshAgentAuth without
security.pam.enableSSHAgentAuth as there is no reason
not to allow it.
@jkarlson
Copy link
Author

@jkarlson jkarlson commented Oct 31, 2020

This new revision would be my preferred solution (apart from completely dropping config.security.pam.enableSSHAgentAuth completely and the entry on sudo module).

It's to my understanding perfectly backwards compatible apart from people who have only enableSSHAgentAuth = true specified and expect it to have no effect, which is just silly.

Copy link
Member

@Infinisil Infinisil left a comment

Other than my suggestion and the fact that it needs to be rebased, this looks good to me!

However I'd like @risson to take a look at this one as well, because they've just started reworking the pam module entirely in #105319

@@ -223,7 +223,7 @@ in

environment.systemPackages = [ sudo ];

security.pam.services.sudo = { sshAgentAuth = true; };
security.pam.services.sudo = lib.optionalAttrs config.security.pam.enableSSHAgentAuth { sshAgentAuth = true; };
Copy link
Member

@Infinisil Infinisil Nov 30, 2020

I think it probably makes more sense to move this definition to the pam.nix module instead, then option definition and use is in the same file.

@jkarlson
Copy link
Author

@jkarlson jkarlson commented Nov 30, 2020

I see this commit has been sufficiently fixed in ba1fa0c for me.

@jkarlson jkarlson closed this Nov 30, 2020
@jkarlson
Copy link
Author

@jkarlson jkarlson commented Nov 30, 2020

already fixed by another commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants