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.

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

nixos/pam: Add assertion for SSH-agent auth #276499

Merged
merged 1 commit into from
Jan 7, 2024
Merged

nixos/pam: Add assertion for SSH-agent auth #276499

merged 1 commit into from
Jan 7, 2024

Conversation

nbraud
Copy link
Contributor

@nbraud nbraud commented Dec 24, 2023 •
edited
Loading

Description of changes

Assertion in the pam NixOS module, ensures that services.openssh.authorizedKeysFiles is a non-empty list when security.pam.enableSSHAgentAuth.
Otherwise, the PAM module fails with an unhelpful error message, and users cannot authenticate (with it)

Split out from #266332

Things done

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 24, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 24, 2023
@nbraud nbraud requested a review from Majiir December 29, 2023 11:16
@nbraud
Copy link
Contributor Author

nbraud commented Dec 29, 2023

@Majiir, same as #266332 : feel free to remove the request for a review

Majiir
Majiir approved these changes Dec 29, 2023
View reviewed changes
Copy link
Contributor

@Majiir Majiir left a comment

Choose a reason for hiding this comment

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

LGTM.

You may also be interested in #254121. It could be split into two parts, one to add the new option and another to fix the insecure default.

@nbraud nbraud added needs_merger (old Marvin label, do not use) 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Dec 29, 2023
@nbraud
Copy link
Contributor Author

nbraud commented Dec 29, 2023

You may also be interested in #254121. It could be split into two parts, one to add the new option and another to fix the insecure default.

Thanks! I had also noticed the security issue, but hadn't found the GitHub issue & PR.

@nbraud nbraud force-pushed the nixos/pam/ssh-agent-auth branch from c39668f to 607679c Compare December 30, 2023 22:19
@nbraud
Copy link
Contributor Author

nbraud commented Dec 30, 2023

Rebased to address merge conflict

@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 31, 2023
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 31, 2023
@mkg20001 mkg20001 merged commit c931d73 into NixOS:master Jan 7, 2024
7 checks passed
This was referenced Jan 8, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Majiir Majiir Majiir approved these changes

Assignees
No one assigned
Labels
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 12.approvals: 1 This PR was reviewed and approved by one reputable person needs_merger (old Marvin label, do not use)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nbraud @Majiir @delroth @mkg20001