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

Jump to bottom

nixos/pam: safe defaults for pam_ssh_agent_auth #62317

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

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.

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
Draft
@fpletz fpletz added this to the 19.09 milestone Jun 2, 2019
@fpletz fpletz mentioned this pull request Jun 2, 2019
Closed
@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
@bricewge
pam_ssh_agent_auth: add license and fix manual
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

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
@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

@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 fpletz approved these changes

@c0bw3b c0bw3b c0bw3b left review comments

@infinisil infinisil infinisil requested changes

@wmertens wmertens wmertens approved these changes

@zimbatm zimbatm Awaiting requested review from zimbatm

@edolstra edolstra Awaiting requested review from edolstra

Assignees
No one assigned
Labels
1.severity: security 2.status: merge conflict 6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 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