Use structured settings for PAM configuration #105098

kwohlfahrt · 2020-11-26T23:53:19Z

Motivation for this change

Originally, this was brought on by the lack of support for the crypt option to pam_ldap, with no easy way of changing it. #90640 then provided motivation for a more thorough rework. So far, this PR keep the external interface, but replaces security.pam.services.<name>.text with .entries, consisting of a list of structured entries.

I'm opening the PR now for discussion, because this is a large change and @rissson was interested in working together.

TODO

Priorities for PAM service entries (see nixos/nsswitch: Make databases more configurable #85998 for reference)
Move service-specific options to their respective services, out of the PAM module
use mkRenamedOptionModule where appropriate
??? any other changes we want to make to the interface

Things done

Coalesce lines where possible, to reduce the number of screens of options

This is a more structured form, that will allow easier editing later.

rissson · 2020-11-27T01:19:26Z

ping @Rudi9719 @flokli who were part of the original discussion

I think this is already a huge step forward, i.e. we now have some structure for entries! Though I'm not too fond of the structure used (a list) to store those entries.

I would have imagined something like this:

security.pam.services.whatever = {
  auth = {
    "pam_unix.so" = {
      control = "required";
      extraArgs = [ "whatever" ];
    };
  };

  account = {
    /* ... */
  };

  password = {
    /* ... */
  };

  session = {
    /* ... */
  };
};

But this stops working when you want to use the same module several times, like pam_succeed_if.so to validate several conditions.
Also, overriding orders in a list is hard. For instance, if the default in nixpkgs is

# Let's put all of those in the session module, for the sake of the example
[
  { path = "pam_unix.so"; control = "required"; }
  { path = "pam_krb5.so"; control = "sufficient"; }
]

But I want to change that to invert the order, for whatever reason, how would I go about it? Overriding order in sets is easy, as we just have to change an order attribute to the required value, without having to rewrite the whole pam logic as we would have to for lists.

A solution to that would be to use sets like this:

  auth = {
    "unix" = {
      path = "pam_unix.so";
      control = "required";
      order = 10;
      extraArgs = [ "whatever" ];
    };
    "krb5" = {
      path = "pam_krb5.so";
      control = "sufficient";
      order = 20;
    };
  };

and then we could do something like security.pam.services.login.auth.krb5.order = 5; to easily change to order in which the modules get rendered to pam's config. I don't feel good neither bad with having an attribute name we just discard and that is only used for identifying the rule you want to change, but it is an idea and maybe a solution.

rissson · 2020-11-27T01:26:51Z

Also, you said that your current changes keep the external interface, but we shouldn't feel blocked by this. It isn't used widely in nixpkgs, and people tweaking it in their config would either be happy with an improvement of the pam situation, or they're already overriding the whole module like I do. Either way, it'll probably end up in the release notes anyway.

aanderse · 2020-11-27T10:51:05Z

@rissson your suggestions are good 👍 The nginx module does the exact same thing, as you are aware, so your proposed changes fit in well with nixpkgs and provide the highest level of flexibility. Especially if combined with a small section in the manual on how to make customizations using this module, with some examples, this would be amazing.

kwohlfahrt · 2020-11-27T23:23:57Z

Though I'm not too fond of the structure used (a list) to store those entries.

Agreed - I think we can do better here, I just wasn't sure enough on how to begin. In fact, the current implementation wouldn't necessarily solve my original issue ;) Note the proposed orderOf in #97392 might be applicable here. But until that is merged, the suggestion above seems reasonable.

My only question is how we should assign orderings to our default values - should we leave a gap (say 100) between each default entry? Or should we group them into blocks with the same priority? Perhaps certain rules should generally move together, and a more tree-like structure is called for (see include and submodule thoughts below)?

Regarding structuring the attributes as { auth = ...; session = ...; ... } with one key per module type - this seems reasonable, as long as the different module types are independent (i.e. auth rules can't interfere with password rules and vice versa) - I think they are, but this is not spelled out anywhere in the PAM guides.

I also plan to improve support for include and submodule rules. It might be interesting to explore a more tree-like organization, but I'm not enough of an expert with PAM to see if this would have any advantages at a glance?

rissson · 2020-11-28T19:15:49Z

Just so you know @kwohlfahrt, I started building something with orderOf on top of your PR. Will probably take another day before I have something to show.

rissson · 2020-11-28T21:06:27Z

nixos/modules/security/pam.nix

-
-{
+  makePAMService = name: service: let
+    argumentToString = value: if hasInfix " " value then "[${escape [ "]" ] value}]" else value;


Why do we need the escape here?

Nevermind, I RTFMed

Note, if you wish to include spaces in an argument, you should surround that argument with square brackets.

When using this convention, you can include [' characters inside the string, and if you wish to include a ]' character inside the string that will survive the argument parsing, you should use `]'. In other words: [..[..]..] --> ..[..]..

Maybe it should be up to the user to escape any ] in a single argument that contains spaces. I'm guessing we will miss something here and it might break stuff for people. Let's not do backend stuff like this, users getting to this level of configuration already know about this or will find out from the manpage like I did.

I soooo wish I could think of a way to make this more Nixy than a list of strings. For example, if I want to override the umask in the mkHomeDir module, I'd have to override the whole list, and I wouldn't be able to do something like mkHomeDir.args.umask = "0027"; (random value, use at your own risk). Anyone else has a take on this?

Let's not do backend stuff like this, users getting to this level of configuration already know about this or will find out from the manpage like I did.

I disagree pretty strongly with this take, because accidentally adding an argument with spaces doesn't result in an error, it results in a silently different configuration (with that argument treated as multiple separate arguments). You clearly have been configuring PAM modules, and weren't aware of the need to escape spaces. If you had added an argument that contained spaces, it would have silently been accepted but then treated as multiple separate arguments, which could have resulted in an insecure configuration.

I'm also unable to think of a situation in which case us doing the escaping would cause trouble, apart from perhaps double-escaping. But I think people are more likely to read the nix option docs, where we can note that we do this.

I soooo wish I could think of a way to make this more Nixy than a list of strings.

I agree here, but there are lots of options that are just a single string... 🤷 It'd be nice if they had to be key-value pairs.

Ah, I have a thought. We can use types.attrsOf (types.either types.str types.bool). Then, if the value is bool, we add the key if it is true, omit the key if it is false. If the value is str, we use key=value syntax. Could this work?

I can't see how that would work. Could you give an example?
attrsOf (nullOr str) might be better. It would look like this, assuming we have a render function outputting the result from a single argument (name, value pair):

>>> render { name = "debug"; value = null; } "debug" >>> render { name = "umask"; value = "0027"; } "umask=0027"

Usage:

args = { debug = null; umask = "0027"; };

I missed this comment, I was thinking as follows:

args = { debug = true; umask = "0027"; }

which would render as "debug umask=0027". The render function would check whether the attr value is a string, in which case i renders as "key=value" or a boolean, in which case it renders "key" if the value is true, otherwise "". The advantage of this is it allows removing a key, by doing { args.debug = mkForce false; }, I'm not sure theres a nice way of unsetting a key from an attrset?

rissson · 2020-11-28T21:07:02Z

nixos/modules/security/pam.nix

-{
+  makePAMService = name: service: let
+    argumentToString = value: if hasInfix " " value then "[${escape [ "]" ] value}]" else value;
+    controlToString = value:


It would be nice to use the apply arg to mkOption to achieve this

Something like this:

control = mkOption { type = controlType; apply = value: if isString value then value else concatStringsSep " " (mapAttrsToList (n: v: "${n}=${toString v}") value); }; args = mkOption { default = []; apply = value: concatStringsSep " " (map toString value); };

EDIT: it didn't work before

See
https://github.com/NixOS/nixpkgs/pull/105319/files#diff-b08ed0eadd5d6b3a3da3a911d502ff1de42cc47bc4bd0c296576a247fd4ac96fR25-R27
and
https://github.com/NixOS/nixpkgs/pull/105319/files#diff-b08ed0eadd5d6b3a3da3a911d502ff1de42cc47bc4bd0c296576a247fd4ac96fR38

nixos/modules/security/pam.nix

rissson · 2020-11-28T21:08:32Z

nixos/modules/security/pam.nix

+                 { control = "required"; path = "pam_deny.so"; }];
+      in listToAttrs (map (t: nameValuePair t rules) [ "auth" "account" "password" "session" ]);
+
+      # Most of these should be moved to specific modules.


I wonder where those would end up...

I think the original intent was that the kerberos rules would live in their respective packages (e.g. kerberos, LDAP, etc).

nixos/modules/security/pam.nix

kwohlfahrt · 2020-11-28T23:31:13Z

Just so you know @kwohlfahrt, I started building something with orderOf on top of your PR. Will probably take another day before I have something to show.

Oops, sorry for the merge conflicts :P

I'm interested to see what you come up with, the downside to using orderOf is that it makes it difficult to refer to existing rules (so I can't add an argument to an existing rule added by say the LDAP module). Having named rule-sets with priorities would allow this, so perhaps that's a better approach? Like this:

services.pam.foo.entries = {
  # This attr-name is semantically meaningful, it sets the module type
  auth = {
    # This attr-name is ignored
    ldapRules = {
      entry = { ...; }; # Can use include/substack for multiple rules in a block
      priority = 100;
    };
  };
};

I think combined with the include/substack support I just added, this could be promising?

rissson · 2020-11-28T23:42:25Z

I still have to figure out how to integrate the include/substack functionality of PAM in my implementation. I wonder if it is relevant to have it in NixOS. Maybe let's table that discussion for later? We won't lose any functionality anyway...

rissson · 2020-11-29T16:34:40Z

I'm interested to see what you come up with, the downside to using orderOf is that it makes it difficult to refer to existing rules (so I can't add an argument to an existing rule added by say the LDAP module). Having named rule-sets with priorities would allow this, so perhaps that's a better approach? Like this:
services.pam.foo.entries = {
  # This attr-name is semantically meaningful, it sets the module type
  auth = {
    # This attr-name is ignored
    ldapRules = {
      entry = { ...; }; # Can use include/substack for multiple rules in a block
      priority = 100;
    };
  };
};

I just included the priority (or order as I named it) inside the entry. So in my implementation, your example would look like this:

security.pam.services.foo = {
  auth = {
    ldap = {
      # control, path, ...
      order = 100;
    };
  };
};

I think combined with the include/substack support I just added, this could be promising?

I think include and substack should just be another control type, without enforcing anything. What if someone wants to do:

# This could come from any number of sources and doesn't have to be defined in NixOS configuration
environment.etc."pam.d/example".text = "wathever";

security.pam.services.myService = {
  auth = {
    myEntry = {
      control = "include";
      path = "example";
      order = 42;
    };
  };
};

kwohlfahrt · 2020-11-29T17:09:18Z

I think include and substack should just be another control type, without enforcing anything. What if someone wants to do:

Passing a string path is supported, it just also supports passing more entries, in which case they will be written to a file and that will be included. They use the same attribute, so you can entries = "/foo/bar" or entries = { auth = ...; }

rissson · 2020-11-29T17:22:00Z

Passing a string path is supported, it just also supports passing more entries, in which case they will be written to a file and that will be included. They use the same attribute, so you can entries = "/foo/bar" or entries = { auth = ...; }

That's nice. But that means there would be two ways of using include/substack, and I don't think we want that. Also entries = "/foo/bar", is that include or substack, it isn't explicit.

kwohlfahrt · 2020-11-30T18:44:12Z

My example wasn't clear, the directive is still explicitly required:

services.pam.foo.entries = {
  # I agree these shouldn't be a list, but named attrsets (TODO)
  auth = [{
    control = "include";
    entries = "/foo/bar";
  } {
    control = "include";
    # These entries could be implicitly all auth to reduce boilerplate
    entries = { auth = [ ... ]; };
  }];

This way I think it looks quite similar to what you were proposing (except path is renamed to entries, and supports a string path, or an attrset which will be automatically written to a file and then the store path is included).

I think supporting nested rules via nix expressions is useful, because substack has semantic meaning that can't be emulated otherwise. Similarly, supporting nested rules via direct paths is also useful, because people might have pre-written rules they just want to import. In the ideal world everything would be added via nix expressions, but I think this is still useful until all tutorials embrace Nix 😄

rissson · 2020-11-30T20:00:20Z

I think supporting nested rules via nix expressions is useful, because substack has semantic meaning that can't be emulated otherwise.

What about creating another PAM service that you then use in your substack?

Similarly, supporting nested rules via direct paths is also useful, because people might have pre-written rules they just want to import.

Yes, that has to be supported, of course.

kwohlfahrt · 2020-12-01T22:08:41Z

I think supporting nested rules via nix expressions is useful, because substack has semantic meaning that can't be emulated otherwise.

What about creating another PAM service that you then use in your substack?

I'm not sure I follow here. The substack entries could refer to another services rules like this, but I'm not sure that's what you mean...

security.pam.services.foo.entries = {
  auth = [{
    control = "substack";
    entries = security.pam.services.bar.entries.auth;
  }]
}

The actual file containing the rules will be a store path, not a service path in /etc/pam.d.

rissson · 2020-12-02T09:36:13Z

I'm not sure I follow here. The substack entries could refer to another services rules like this, but I'm not sure that's what you mean...
security.pam.services.foo.entries = {
  auth = [{
    control = "substack";
    entries = security.pam.services.bar.entries.auth;
  }]
}

What I meant is that your bar service would end up in /etc/pam.d/bar anyway, so for your foo service, you could do:

security.pam.services.foo.entries = {
  auth = [{
    control = "substack";
    path = "bar";
  }];
};

kwohlfahrt · 2020-12-02T19:17:13Z

You could, but that would be relying on a NixOS implementation detail. Nothing is stopping us from migrating to having all service configs together in /etc/pam.conf instead.

Not suggesting we do that, but that seems like a fragile configuration, that just so happens to work because of the details of the current implementation (and should not be encouraged, but I don't see any way to prevent it).

stale · 2021-11-16T12:21:11Z

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

flokli · 2021-11-16T12:54:14Z

I'd still love to see this getting in :-)

aanderse · 2021-11-16T12:57:59Z

No better time than in a few weeks, once 21.11 is released.

PolarizedIons · 2023-09-04T10:27:55Z

Sorry to bump, but is there anything that can be done to help?

nixos-discourse · 2023-09-21T23:25:47Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/what-do-you-want-from-pam-security-pam-in-nixos/33265/4

kwohlfahrt added 4 commits November 25, 2020 22:23

nixos/pam: Formatting

b184e91

Coalesce lines where possible, to reduce the number of screens of options

nixos/pam: Convert PAM text to entries

734ed6d

This is a more structured form, that will allow easier editing later.

nixos/pam: use control type for existing options

92e08e4

Merge remote-tracking branch 'upstream/master' into pam

b13475d

ofborg bot added 6.topic: nixos 8.has: module (update) labels Nov 26, 2020

nixos/pam: Add descriptions

fb11ab3

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Nov 27, 2020

nixos/pam: Group rules by functionality

29c8c6b

rissson suggested changes Nov 28, 2020

View reviewed changes

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

kwohlfahrt added 2 commits November 28, 2020 23:12

nixos/pam: Add support for nested PAM scopes rule

3ebc44e

nixos/pam: Fix PR nits

633b522

rissson mentioned this pull request Nov 29, 2020

nixos/pam: rework of the entire module #105319

Closed

33 tasks

kwohlfahrt added 2 commits November 29, 2020 12:09

Merge remote-tracking branch 'upstream/master' into pam

31afb4d

nixos/pam: Fix description for entry type

4ad3596

SuperSandro2000 added the 2.status: merge conflict label Jan 18, 2021

Merge branch 'master' into pam

fb71185

ofborg bot removed the 2.status: merge conflict label May 15, 2021

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 16, 2021

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 16, 2021

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 19, 2022

bjornfor mentioned this pull request Dec 1, 2022

No mechanism to insert arbitrary lines to /etc/pam.d/<file> #202337

Open

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 4, 2023

Majiir mentioned this pull request Sep 16, 2023

nixos/pam: assemble rules from modular configuration #255547

Merged

10 tasks

wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024

Use structured settings for PAM configuration #105098

Are you sure you want to change the base?

Use structured settings for PAM configuration #105098

Conversation

kwohlfahrt commented Nov 26, 2020

Motivation for this change

TODO

Things done

rissson commented Nov 27, 2020

rissson commented Nov 27, 2020

aanderse commented Nov 27, 2020

kwohlfahrt commented Nov 27, 2020

rissson commented Nov 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rissson Nov 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rissson Nov 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwohlfahrt commented Nov 28, 2020

rissson commented Nov 28, 2020

rissson commented Nov 29, 2020 • edited

kwohlfahrt commented Nov 29, 2020

rissson commented Nov 29, 2020

kwohlfahrt commented Nov 30, 2020 • edited

rissson commented Nov 30, 2020

kwohlfahrt commented Dec 1, 2020

rissson commented Dec 2, 2020 • edited

kwohlfahrt commented Dec 2, 2020 • edited

stale bot commented Nov 16, 2021

flokli commented Nov 16, 2021

aanderse commented Nov 16, 2021

PolarizedIons commented Sep 4, 2023

nixos-discourse commented Sep 21, 2023

rissson Nov 28, 2020 •

edited

rissson Nov 28, 2020 •

edited

rissson commented Nov 29, 2020 •

edited

kwohlfahrt commented Nov 30, 2020 •

edited

rissson commented Dec 2, 2020 •

edited

kwohlfahrt commented Dec 2, 2020 •

edited