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: assemble rules from modular configuration #255547

Merged
merged 15 commits into from
Oct 16, 2023

Conversation

Majiir
Copy link
Contributor

@Majiir Majiir commented Sep 16, 2023

Description

Refactors the security.pam module to assemble /etc/pam.d files from modular rule configuration. Rules can now be modified, added, toggled, and reordered.

Problem statement

The existing module is inflexible and does not allow a user to reconfigure the PAM stack:

  • Module arguments cannot be changed if there is not already a corresponding option.
  • Rule control settings cannot be changed.
  • Rules cannot be reordered.
  • New rules can only be added at the beginning or end of the stack.

See this Discourse thread for more discussion.

Design

A rule set is defined for each service and type (account, auth, password or session). A rule set is now expressed as an attribute set of named rules:

{
  security.pam.services.su.rules.auth = {
    wheel = {
      enable = true;
      control = "required";
      modulePath = "pam_wheel.so";
      order = config.security.pam.services.su.rules.auth.unix.order - 10;
    };
  };
}

The rule name (attribute key) can be used to merge multiple configurations of the same rule (e.g. to override a built-in rule).

Rules have an order option which determines the order the rules are written to the PAM config files. Built-in rules are automatically assigned an order value based on their hard-coded position. Some kind of ordering mechanism is necessary because the rules are now configured in an attribute set.

The new config options are hidden and marked experimental so that maintainers may freely modify the option design or the ordering of rules.

Example

{
  security.pam.services = let
    serviceCfg = service: {
      rules.auth = {
        unix = {
          control = lib.mkForce "requisite";
        };
        rssh = {
          order = config.security.pam.services.${service}.rules.auth.unix.order + 10;
          control = "sufficient";
          modulePath = "${pkgs.pam_rssh}/lib/libpam_rssh.so";
          settings.auth_key_file = "/etc/rssh/authorized_keys";
        };
      };
    };
  in lib.flip lib.genAttrs serviceCfg [
    "sudo"
    "su"
  ];
}

This example configures the PAM stack in ways not currently possible:

  • The pam_rssh module is enabled and integrated into the existing service rules.
  • The pam_unix rule is changed from requires to requisite.
  • The pam_rssh rule is ordered after the pam_unix rule.
  • The pam_rssh module is passed an additional auth_key_file argument.

Authentication section of the resulting /etc/pam.d/sudo file:

# Authentication management.
auth requisite pam_unix.so likeauth try_first_pass # 11600
auth sufficient /nix/store/cn5lccdcx38k81wjz42v63k8c82y2kkq-pam_rssh-1.1.0/lib/libpam_rssh.so auth_key_file=/etc/rssh/authorized_keys # 11610
auth required pam_deny.so # 12400

Review notes

  • Changes are best reviewed commit-by-commit.
  • Many of the commits can be reproduced with regex substitution.
  • None of the commits change the behavior of the resulting PAM rules.

Related work

Since previous attempts lost steam and were abandoned, I'm hoping we can merge this smaller rework and iterate on the module design in subsequent PRs.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • armv7l-linux (cross)
  • Tested, as applicable:
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

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/1

Makes the rules more uniform in structure and style. This makes it
easier to automate subsequent commits. No behavior changes.
@Majiir Majiir force-pushed the pam-modular-rules branch 2 times, most recently from a2de264 to 57b5f1e Compare September 25, 2023 02:02
@Majiir
Copy link
Contributor Author

Majiir commented Sep 25, 2023

  • Changed all new options to hidden.
  • Renamed a couple of the new options.
  • Improved commit readability.
  • Fixed trailing whitespace issues.

This is ready for review! I've been running this on my daily driver without issues. The commits are easy to get through, I promise. Take a look and leave your comments! 🙇‍♂️

@nixos-discourse
Copy link

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/8

@liff
Copy link
Contributor

liff commented Sep 25, 2023

A quick thought (I haven’t spent time considering all the implications), but instead of a numerical ordering, would it make sense to use a DAG like home-manager does?

With something home-manager’s machinery the su example would become:

{
  security.pam.services.su.types.auth.rules = {
    unix = {
      control = lib.mkForce "requisite";
    };
    rssh = dag.entryAfter [ "unix" ] {
      control = "sufficient";
      modulePath = "${pkgs.pam_rssh}/lib/libpam_rssh.so";
      settings.auth_key_file = "/etc/rssh/authorized_keys";
    };
  };
}

This might make using entries with [value=N] controls somewhat unreliable, but I suspect that’s the case even with numbered ordering.

@rissson
Copy link
Member

rissson commented Sep 26, 2023

A quick thought (I haven’t spent time considering all the implications), but instead of a numerical ordering, would it make sense to use a DAG like home-manager does?

This might make using entries with [value=N] controls somewhat unreliable, but I suspect that’s the case even with numbered ordering.

During my original rework I decided against doing something like this especially because it may end with an inconsistent ordering of options. I quite like the way this PR deals with it by recommending referencing existing entries' order.

@lorenzleutgeb
Copy link
Member

lorenzleutgeb commented Sep 27, 2023

I also think that a DAG would be better.

  1. The fact that one can specify a constant for order is a footgun.
  2. order defaulting to 1000 + i * 10 raises eyebrows:
    a. What if I refer to more than 10 rules to be in order after each other (using the ... + 1 mechanism suggested)? Looks like I would get two rules with the same order value.
    b. What if I create more than 1000 / 10 rules? Again, it looks like I would get two rules with the same order value.
  3. How are rules with the same order value placed? If there's no tie-breaking, I fear that this will be done non-deterministically, or at least the user loses control of whichever deterministic process does the ordering.

@rissson could you please explain or give an example for an "inconsistent ordering"? Generally, if the order implied by the DAG is not total, then yes, there are multiple ways of manifesting the rules in a sequence. But in that case, one can add an edge. And some deterministic tie-breaking should be considered. However, you get similar (the same?) issues with just numbering (see (2.) and (3.) above).

@Majiir
Copy link
Contributor Author

Majiir commented Sep 28, 2023

Thanks for linking the home-manager reference. I think the DAG option is interesting!

Both DAG and order face ambiguous ordering scenarios, and I think both can be made to tie-break on the rule name. In the case of order, I think this happens already because sort is a stable sort.

With order, we can define well-known values, for example 1000 could mean "first pass", 2000 could mean "2FA", etc. With DAG, we can do something similar by ordering rules before/after some well-defined labels (almost like systemd targets).

I don't know how the DAG option handles config overrides. Is it practical for users to reorder rules? If there is a graph a -> b -> c, then setting c -> b would create a cycle, so the user would have to somehow unset b -> c in order to get a -> c -> b. Reordering like this is simpler with order at first glance.

I'm not worried about exhausting the rule space with order. We can make the spacing 100 or 1000 and make it arbitrarily unlikely that someone will declare that many PAM rules.

What if I create more than 1000 / 10 rules? Again, it looks like I would get two rules with the same order value.

The values don't wrap around. If there are many rules, the value will just get arbitrarily large. The order for a rule defined in the hardcoded list in pam.nix is 1000 + i * 10, but the default for the submodule is simply 1000.

This might make using entries with [value=N] controls somewhat unreliable, but I suspect that’s the case even with numbered ordering.

Yes, I think it's a (mild) problem with both. It can be solved with substack rules so that [value=N] controls are never needed.


My thinking is that for now, it doesn't matter much which one we choose. The new options are all hidden from the manual, so we can freely change the option interface in a follow-up PR. It should be easy to find and update usages inside nixpkgs.

I lean toward sticking with order because (a) it's more clear how rules may be reordered, and (b) it looks like the DAG option involves pulling some code from home-manager into nixpkgs lib. I would also like to experiment with the user experience of the DAG option to make sure it supports the main use cases.

Overall, I'm aiming to make a series of PRs to overhaul pam.nix, so it's not important that this one gets everything perfect.

Questions for all:

  1. Do you agree that it's okay to stick with order for at least this first round of changes?

  2. Do you think the new options being hidden is enough to let us safely break the interface in the future?

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Very nice PR! I love the individual commits, makes this very reviewable! The overall idea is solid, though I made some suggestions below.

nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
@infinisil infinisil self-assigned this Oct 9, 2023
Unblocks converting the rules from one big string to a rich data
structure.
Eliminates a redundancy between the 'rules' suboptions and the type
specified in each rule.

We eventually want to give each rule a name so that we can merge config
overrides. The PAM name is a natural choice for rule name, but a PAM is
often used in multiple rule types. Organizing rules by type and rule
name avoids name collisions.
Allows us to decompose rules into multiple fields that we later format
as textual rules. Eventually allows users to override individual fields.
These names are internal identifiers. They will be used as keys so that
users can reconfigure rules by merging a rule config with the same name.
The name is arbitrary. The built-in rules are named after the PAM where
practical.
Module arguments have common escaping rules for all PAMs.
@Majiir
Copy link
Contributor Author

Majiir commented Oct 10, 2023

@infinisil Thank you for the review! This is ready for another pass.

nixos/modules/security/pam.nix Show resolved Hide resolved
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
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
Adds easily overrideable settings for the most common PAM argument
styles. These are:

- Flag (e.g. "use_first_pass"): rendered for true boolean values. false
  values are ignored.

- Key-value (e.g. "action=validate"): rendered for non-null, non-boolean
  values.

Most PAM arguments can be configured this way. Others can still be
configured with the 'args' option.
Makes it possible to override properties of a rule by name. Introduces
an 'order' field that can be overridden to change the sequence of rules.

For now, the order value for each built-in rule is derived from its
place in the hardcoded list of rules.
Removes redundant config from the module. Fixes a bug where some modules
(e.g. ussh) were added to apparmor even though they had no rules enabled.
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Looking good!

@infinisil
Copy link
Member

For the record: I'm still not entirely happy about the order option, but I don't think it matters very much, these options will be marked as experimental, and it looks like you intend to continue with this effort after this is merged @Majiir.

I think it's okay to merge this as is, but let's give @rissson and @kwohlfahrt some more time to give their approval or suggestions too.

@rissson
Copy link
Member

rissson commented Oct 11, 2023

This is already a massive improvement as you now don't have to override the whole generated file to get what you need.

For the record: I'm still not entirely happy about the order option, but I don't think it matters very much, these options will be marked as experimental

The main issue is being sure that the order is always the same no matter what. As a user, saying that rule A needs to be after rule B is not sufficient enough. Perhaps the order being described as an int is not enough. We could also imagine asking the user for an ordered list that contains the order they want their rules in.

I'm very happy to see this making some progress, and am excited to see the next steps!

@infinisil infinisil merged commit e0b3b07 into NixOS:master Oct 16, 2023
20 checks passed
@SuperSandro2000
Copy link
Member

I am here because I just hit a merge conflict on rebasing my fork where I changed the pam module rather than using options because the old module just didn't allow it. I just wanted to say a huge thank you to everyone involved that helped closing this giant gap! ❤️

@mjm
Copy link
Contributor

mjm commented Oct 21, 2023

I've been eagerly awaiting this PR landing in unstable so I could reorder my swaylock auth lines and make both fprintd and password work, thank you for the work that went into this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants