Skip to content

nixos/pam: safe defaults for pam_ssh_agent_auth #62317

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

nixos/pam: safe defaults for pam_ssh_agent_auth #62317

wants to merge 2 commits into from
+58 −6

Conversation

bricewge
Copy link
Contributor

Motivation for this change

Fixes #31611, replace #32178 and advance #43716.

Things done

BREAKING CHANGES

  1. Replaces option security.pam.enableSSHAgentAuth with security.pam.sshAgentAuth.{enable,authorizedKeys} defaulting to safe defaults.
  2. Add license to pam_ssh_agent_auth
  3. sudo don't preserve SSH_AUTH_SOCK anymore
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels May 31, 2019
fpletz
fpletz approved these changes Jun 2, 2019
View reviewed changes
Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up!

nixos/modules/security/pam.nix
This option has been deprecated due to a security vunerability. Read
the 19.09 release notes for more informations and the replacing options.
''; }
];
Copy link
Member

Choose a reason for hiding this comment

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

You could remove options.security.pam.enableSSHAgentAuth and use mkRemovedOptionModule with the text of this assertion instead. Less code. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wish I could but I need assertion because it throw, mkRemovedOptionModule only add a warnings which doesn't:

nixpkgs/lib/modules.nix

Lines 592 to 602 in f712061

mkRemovedOptionModule = optionName: replacementInstructions:
{ options, ... }:
{ options = setAttrByPath optionName (mkOption {
visible = false;
});
config.warnings =
let opt = getAttrFromPath optionName options; in
optional opt.isDefined ''
The option definition `${showOption optionName}' in ${showFiles opt.files} no longer has any effect; please remove it.
${replacementInstructions}'';
};

I don't want someone to be locked out of their systems because of the breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Since #69419 mkRemovedOptionModule throws, so it should be usable for this now

@fpletz fpletz mentioned this pull request Jun 2, 2019
Closed
@fpletz fpletz added this to the 19.09 milestone Jun 2, 2019
@fpletz fpletz added the 1.severity: security Issues which raise a security issue, or PRs that fix one label Jun 2, 2019
@fpletz fpletz mentioned this pull request Jun 2, 2019
Closed
@fpletz fpletz added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 2, 2019
@fpletz fpletz requested review from zimbatm and wmertens June 2, 2019 17:58
@fpletz
Copy link
Member

fpletz commented Jun 2, 2019

cc @sometimes-i-send-pull-requests

@bricewge bricewge force-pushed the pam-ssh-agent-vulnerability branch 2 times, most recently from 7778996 to 0a27261 Compare June 3, 2019 12:13
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 3, 2019
bricewge added 2 commits June 3, 2019 22:17
@bricewge
pam_ssh_agent_auth: add license and fix manual

Verified

This commit was signed with the committer’s verified signature.
vcunat Vladimír Čunát
GPG key ID: E747DF1F9575A3AA
Verified
Learn about vigilant mode
0cf6742 
Help with NixOS#43716
@bricewge
nixos/pam: safe defaults for pam_ssh_agent_auth
fdecc4d 
Replaces option security.pam.enableSSHAgentAuth with
security.pam.sshAgentAuth.{enable,authorizedKeys} defaulting to
safe defaults.
@bricewge bricewge force-pushed the pam-ssh-agent-vulnerability branch from 0a27261 to fdecc4d Compare June 3, 2019 20:23
@bricewge
Copy link
Contributor Author

bricewge commented Jun 3, 2019 •
edited
Loading

I didn't managed to removed the related entry in/etc/sduoers as it's hinted in the manpage of pam_ssh_agent_auth, so I removed that commit. I go more in depth about it here: jbeverly/pam_ssh_agent_auth#16.

EDIT: BTW I think that security.pam.services.<name?>.sshAgentAuth may be broken (apart from sudo) since I don't see how pam_ssh_agent_auth can access the user's SSH_AUTH_SOCK variable.

@ofborg ofborg bot requested a review from edolstra June 3, 2019 20:42
@ofborg ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jun 3, 2019
@wmertens
Copy link
Contributor

This is great, but didn't we have a more visible way to document breaking changes in a release?

@aanderse
Copy link
Member

aanderse commented Sep 1, 2019

ping (triage)

wmertens
wmertens approved these changes Oct 24, 2019
View reviewed changes
Copy link
Contributor

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

LGTM

infinisil
infinisil requested changes Oct 29, 2019
View reviewed changes
nixos/modules/security/pam.nix
This option has been deprecated due to a security vunerability. Read
the 19.09 release notes for more informations and the replacing options.
''; }
];
Copy link
Member

Choose a reason for hiding this comment

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

Since #69419 mkRemovedOptionModule throws, so it should be usable for this now

c0bw3b
c0bw3b reviewed Oct 29, 2019
View reviewed changes
nixos/doc/manual/release-notes/rl-1909.xml
The <option>security.pam.enableSSHAgentAuth</option> module has been
removed because it allowed a privilege escalation from any program running as the user.
The options replacing it don't have the same default and may lock you out of your
system, be extremly careful when migrating!
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
system, be extremly careful when migrating!
system, be extremely careful when migrating!

(or just very careful)

c0bw3b
c0bw3b reviewed Oct 29, 2019
View reviewed changes
nixos/modules/security/pam.nix
@@ -544,13 +544,40 @@ in
default = false;
description =
''
Deprecated option, see release note for 19.09.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Deprecated option, see release note for 19.09.
Deprecated option, see release notes for 19.09.

c0bw3b
c0bw3b reviewed Oct 29, 2019
View reviewed changes
nixos/modules/security/pam.nix
assertions = [
{ assertion = !config.security.pam.enableSSHAgentAuth;
message = ''
This option has been deprecated due to a security vunerability. Read
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This option has been deprecated due to a security vunerability. Read
This option has been deprecated due to a security vulnerability. Read

@ryantm ryantm added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 3, 2020
@veprbl veprbl removed this from the 19.09 milestone May 31, 2021
@bricewge
Copy link
Contributor Author

I don't use NixOs anymore. Please take in hand this PR if you want it to be merged.

@bricewge bricewge closed this Feb 20, 2022
@Majiir Majiir mentioned this pull request Sep 8, 2023
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpletz fpletz

@c0bw3b c0bw3b

@infinisil infinisil

@wmertens wmertens

@zimbatm zimbatm

@edolstra edolstra

Assignees
No one assigned
Labels
1.severity: security Issues which raise a security issue, or PRs that fix one 2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pam_ssh_agent_auth allowing users to define own ssh pubkey
9 participants
@bricewge @fpletz @wmertens @aanderse @edolstra @infinisil @c0bw3b @ryantm @veprbl