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 defaults option #49506

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@hyperfekt
Copy link
Contributor

commented Oct 31, 2018

Changing all PAM configuration files derived from the default in pam.nix was impossible from outside the module.

Motivation for this change

Looking for a way to achieve the effect of #30333 for my system, it turned out there is no way to enumerate all PAM services without depending on config (and thus causing infinite recursion), making it impossible to append all their configuration files with desired changes.
With this change, additions to the default PAM configuration can be trivially made.
Example (emulating #30333): security.pam.defaults = "session required pam_keyinit.so force revoke"

Things done
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

Show resolved Hide resolved nixos/modules/security/pam.nix Outdated

@hyperfekt hyperfekt force-pushed the hyperfekt:pam_defaults branch Dec 11, 2018

@hyperfekt hyperfekt requested a review from Infinisil as a code owner Dec 11, 2018

@hyperfekt hyperfekt force-pushed the hyperfekt:pam_defaults branch to 5899d93 Dec 11, 2018

@hyperfekt hyperfekt force-pushed the hyperfekt:pam_defaults branch from 5899d93 to 2613905 Feb 9, 2019

@aanderse

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

This seems pretty reasonable to me. How about you @Ma27?

Show resolved Hide resolved nixos/modules/security/pam.nix Outdated
nixos/pam: add defaults option
Changing all PAM configuration files derived from the default in pam.nix was impossible from outside the module.

@hyperfekt hyperfekt force-pushed the hyperfekt:pam_defaults branch from 2613905 to 769f80e May 12, 2019

@Infinisil

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Man, this PAM config sure is a mess..

I'm pretty sure this option here isn't strictly needed actually, the module system itself should allow you to set text = mkDefault "some extra text" which should concatenate with the default specified there. It would probably even make more sense to change the PAM module to not use mkDefault there such that you don't need a mkDefault to add to it, oh well. Note that you might need mkAfter to control the ordering of it, so text = mkDefault (mkAfter "some extra text after the default definition") should work.

@eadwu

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Looking for a way to achieve the effect of #30333 for my system, it turned out there is no way to enumerate all PAM services without depending on config (and thus causing infinite recursion), making it impossible to append all their configuration files with desired changes.

Trying to replicate it in each pam module doesn't look like it be possible without evaluating the config in a separate instance.

@Infinisil

This comment has been minimized.

Copy link
Member

commented May 15, 2019

You can do it with this:

{
  options.security.pam.services = mkOption {
    type = types.loaOf (types.submodule {
      config.text = mkDefault (mkAfter "Some text after everything else");
    });
  };
}

I guess it's hard to figure this out though.

@hyperfekt

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Ah, that makes sense! Thanks a lot, I guess that obviates this PR then.

@hyperfekt hyperfekt closed this May 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.