-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
Conversation
8575c42
to
92e8050
Compare
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.
a2de264
to
57b5f1e
Compare
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! 🙇♂️ |
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 |
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 {
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 |
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. |
I also think that a DAG would be better.
@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). |
Thanks for linking the home-manager reference. I think the DAG option is interesting! Both DAG and With I don't know how the DAG option handles config overrides. Is it practical for users to reorder rules? If there is a graph I'm not worried about exhausting the rule space with
The values don't wrap around. If there are many rules, the value will just get arbitrarily large. The
Yes, I think it's a (mild) problem with both. It can be solved with 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 Overall, I'm aiming to make a series of PRs to overhaul Questions for all:
|
There was a problem hiding this 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.
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.
57b5f1e
to
08ad8fe
Compare
@infinisil Thank you for the review! This is ready for another pass. |
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.
08ad8fe
to
9d6e6e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
For the record: I'm still not entirely happy about the 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. |
This is already a massive improvement as you now don't have to override the whole generated file to get what you need.
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 I'm very happy to see this making some progress, and am excited to see the next steps! |
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! ❤️ |
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! |
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:
control
settings cannot be changed.See this Discourse thread for more discussion.
Design
A rule set is defined for each
service
andtype
(account
,auth
,password
orsession
). A rule set is now expressed as an attribute set of named rules: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 anorder
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
This example configures the PAM stack in ways not currently possible:
pam_rssh
module is enabled and integrated into the existing service rules.pam_unix
rule is changed fromrequires
torequisite
.pam_rssh
rule is ordered after thepam_unix
rule.pam_rssh
module is passed an additionalauth_key_file
argument.Authentication section of the resulting
/etc/pam.d/sudo
file:Review notes
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
pam-file-contents
pam-oath-login
pam-u2f
pam-ussh
pam-zfs-key