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

nixos/pam: add pam_rssh support #336609

Merged
merged 1 commit into from
Oct 10, 2024
Merged

nixos/pam: add pam_rssh support #336609

merged 1 commit into from
Oct 10, 2024

Conversation

illdefined
Copy link
Contributor

@illdefined illdefined commented Aug 22, 2024

Description of changes

This adds support for pam_rssh. This PAM module works in a manner similar to pam_ssh_agent_auth, but supports a broader range of SSH key types, including keys backed by security keys (FIDO2), and is implemented in a memory‐safe language (Rust).

pam_rssh unfortunately cannot be used as a drop‐in replacement for pam_ssh_agent_auth as it does not support pattern expansion in path names.

This module depends on pam_rssh Version ≥ 1.2.0-rc2 (#339035).

This solves #195942.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

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: module (update) This PR changes an existing module in `nixos/` labels Aug 22, 2024
@Majiir
Copy link
Contributor

Majiir commented Aug 25, 2024

pam_rssh unfortunately cannot be used as a drop‐in replacement for pam_ssh_agent_auth as it does not support pattern expansion in path names.

FYI, pam_rssh supports PAM item expansion in v1.2.0-rc1. But I think it's a good idea to use a new set of options rather than using rssh as a drop-in.

@illdefined
Copy link
Contributor Author

pam_rssh unfortunately cannot be used as a drop‐in replacement for pam_ssh_agent_auth as it does not support pattern expansion in path names.

FYI, pam_rssh supports PAM item expansion in v1.2.0-rc1. But I think it's a good idea to use a new set of options rather than using rssh as a drop-in.

Thanks! I think I’ll update the package to the release candidate and amend the module accordingly.

@illdefined
Copy link
Contributor Author

I updated all dependencies of the pam_rssh package to allow building with Rust 1.80 and opened a pull request in the upstream repository: z4yx/pam_rssh#21

The package source can be reverted back to the upstream repository after it has been merged.

@illdefined
Copy link
Contributor Author

Result of nixpkgs-review pr 336609 run on x86_64-linux 1

1 package blacklisted:
  • nixos-install-tools
2 packages built:
  • disko
  • pam_rssh

@illdefined illdefined requested a review from Majiir August 27, 2024 21:36
@illdefined illdefined marked this pull request as ready for review August 27, 2024 21:36
@Majiir
Copy link
Contributor

Majiir commented Aug 28, 2024

I think we should handle the pam_rssh update in a separate PR. It is not necessary for the NixOS module addition. The option descriptions will need to be edited accordingly, of course. For reference, there is already an upstream PR for updating dependencies (z4yx/pam_rssh#20) and it looks like the update is non-trivial.

@illdefined illdefined force-pushed the pam-rssh branch 2 times, most recently from b9f5e2e to 868cb7a Compare September 2, 2024 18:39
@illdefined
Copy link
Contributor Author

Now that #339035 has been merged, I believe that this PR is ready.

I added a warning if both authorizedKeysFile and authorizedKeysCommand are set, and an assertion that at least one of them is set if the module is enabled.

@illdefined
Copy link
Contributor Author

Is there anything I should improve, so that this can be merged?

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.

Is there anything I should improve, so that this can be merged?

Something that could be improved is adopting RFC0042 freeform options. For example, see #276106 for how security.pam.u2f was converted. It's not a blocker for merging since we can always rename the options later, but it will reduce confusion for users if the options start out with the right names. (For example, instead of security.pam.rssh.authorizedKeysFile, we would have security.pam.rssh.settings.auth_key_file.)

nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
@illdefined
Copy link
Contributor Author

Is there anything I should improve, so that this can be merged?

Something that could be improved is adopting RFC0042 freeform options. For example, see #276106 for how security.pam.u2f was converted. It's not a blocker for merging since we can always rename the options later, but it will reduce confusion for users if the options start out with the right names. (For example, instead of security.pam.rssh.authorizedKeysFile, we would have security.pam.rssh.settings.auth_key_file.)

I adapted the module accordingly and removed the loglevel and authorizedKeysCommand options, assuming that they are of little interest for most users and therefore do not require documentation in the NixOS manual beyond the general reference to the upstream documentation. I can of course re‐add them to settings, if that assumption is incorrect.

Thank you for catching the typos.

@vikanezrimaya
Copy link
Member

In my opinion, this might need a NixOS test. I'm going to try my hand at writing one later.

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.

Tested on my config, and it works as expected. Added just some nitpicks.

Thank you for doing this!

nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
nixos/modules/security/pam.nix Show resolved Hide resolved
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 13, 2024
@illdefined
Copy link
Contributor Author

I took care of the mkEnableOption formatting issue and the typo, but I would prefer to expand $ruser without a default value for now, unless there is an important reason for it. Perhaps we have to revisit this once a few users have started using it in their configurations.

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 14, 2024
@fpletz fpletz merged commit 262f0e3 into NixOS:master Oct 10, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants