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: ability to calls an external command #90490

Closed
wants to merge 1 commit into from
Closed

nixos/pam: ability to calls an external command #90490

wants to merge 1 commit into from

Conversation

datafoo
Copy link
Contributor

@datafoo datafoo commented Jun 15, 2020

As indicated in #90488, I would appreciate feedback on this.

Motivation for this change

See #90488

Things done
  • Tested using NixOps
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 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.

Mic92
Mic92 reviewed Jun 15, 2020
View reviewed changes
nixos/modules/security/pam.nix
@@ -463,6 +505,9 @@ let
"session optional ${pkgs.gnome3.gnome-keyring}/lib/security/pam_gnome_keyring.so auto_start"}
${optionalString (config.virtualisation.lxc.lxcfs.enable)
"session optional ${pkgs.lxc}/lib/security/pam_cgfs.so -c all"}

${optionalString (cfg.externalCommand != null)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should move this to a separate file. However there might be a reason we have a monolith here because the order matters?

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 do not think the order matters.

Copy link
Contributor

Choose a reason for hiding this comment

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

We really shouldn't add more to this monolith IMHO. Please see #90640.

Copy link
Member

Choose a reason for hiding this comment

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

@datafoo yes, the order does matter. I'm with @flokli on this one, the PAM module needs some real reworking, let's not make it worse.

Copy link

Choose a reason for hiding this comment

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

In PAM, order very much matters. Especially when using enterprise solutions such as AD/SSSD etc. @flokli pointed me here from my issue #104346 where having pam_unix.so as required, and before pam_sss.so causes some major porblems. pam_unix.so will always fail a non-local account, so by requiring pam_unix.so all network/SSSD accounts will fail.

@flokli flokli mentioned this pull request Nov 20, 2020
Closed
10 tasks
@Mic92
Copy link
Member

Mic92 commented Dec 13, 2020

I hope that #105319 gets ready soon. Than this module becomes easier to integrate.

@rissson
Copy link
Member

rissson commented Dec 13, 2020

I hope that #105319 gets ready soon. Than this module becomes easier to integrate.

Haha yes! It still needs a lot of review though.

@stale
Copy link

stale bot commented Jul 21, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021
@winterqt
Copy link
Member

winterqt commented Mar 4, 2022

Considering this has been stale for over a year, and the pam rework PR was closed: is there any interest in a version of this being merged, at least while the pam rework stuff is being sorted out?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 4, 2022
@datafoo
Copy link
Contributor Author

datafoo commented Mar 4, 2022

Considering this has been stale for over a year, and the pam rework PR was closed: is there any interest in a version of this being merged, at least while the pam rework stuff is being sorted out?

No, thank you

@datafoo datafoo closed this Mar 4, 2022
@datafoo datafoo deleted the proposal-90488 branch December 4, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@flokli flokli flokli left review comments

@Rudi9719 Rudi9719 Rudi9719 left review comments

@rissson rissson rissson left review comments

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: nixos 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.

None yet
7 participants
@datafoo @Mic92 @rissson @winterqt @flokli @Rudi9719 @SuperSandro2000