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 serviceDefaults #340197

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xyven1
Copy link
Contributor

@xyven1 xyven1 commented Sep 6, 2024

Description of changes

This change addresses a shortcoming of how the PAM module handles setting up service configurations. As it stands now, if you enable certain modules, then that will affect all the default service configurations for all services, with no way of overriding it. This change adds configuration options to set the default option for all services, taking priority over the defaults given by the system configuration.

Example (fprintd):

When you set services.fprintd.enable = true in your NixOS config, the fprintAuth option is set to true by default for all services, making fingerprint the preferred PAM method for all services by default. If the goal is simply to disable fingerprint for one service, this is easily accomplished by updating the configuration for that particular service (i.e. security.pam.services.sudo.fprintAuth = false for the sudo service). If, on the other hand, one only want's fprintAuth to be set to true for one service, then short of manually setting it to false for all the enabled services (infinite recursion cuts short any attempts at making this automatic), there is no way to change the default settings for fprintAuth to false.

In other words, the only paradigm for setting PAM settings is opt-out, and opt-in is not possible.

With the changes in place, one simply sets security.pam.serviceDefaults.fprintAuth = false;

Motive for how it was implemented

The way the security.pam.serviceDefaults option is implemented should guarantee complete backwards compatibility with old configurations, with the opt-out paradigm still being the default.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Sep 6, 2024
@xyven1 xyven1 force-pushed the pam-add-service-defaults branch from d591f9d to 7b48b53 Compare September 6, 2024 21:30
@xyven1
Copy link
Contributor Author

xyven1 commented Sep 6, 2024

Right now, this module is completely functional when used in my configuration, but is causing the manual build to fail due to some documentation shenanigans.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Sep 6, 2024
@xyven1 xyven1 marked this pull request as draft September 7, 2024 01:24
@xyven1
Copy link
Contributor Author

xyven1 commented Sep 7, 2024

@philiptaron What do think about this change, does it fit the pam module?

I can see there being an argument against it as it is adds some complexity to the module configuration (only if opted into, but still), and there are some definitely some questions about how to best document the behavior (what is the best default text for the service modules?)

@xyven1 xyven1 force-pushed the pam-add-service-defaults branch from dadc695 to 80d1bd1 Compare September 7, 2024 01:30
@xyven1 xyven1 marked this pull request as ready for review September 7, 2024 01:30
@xyven1 xyven1 force-pushed the pam-add-service-defaults branch from 80d1bd1 to 7ae3575 Compare September 7, 2024 01:40
@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Sep 7, 2024
@philiptaron
Copy link
Contributor

I admit, I'll have to do a lot of reading and learning to form a coherent opinion on the matter. I bet there are others in the Nix community that already have opinions and stances -- I'm just not one of them.

Maybe write out something on discourse and encourage people who care about PAM and local machine authentication to take a look at this change?

@xyven1 xyven1 force-pushed the pam-add-service-defaults branch from 7ae3575 to 6026525 Compare September 7, 2024 16:50
@xyven1 xyven1 requested a review from Stunkymonkey September 7, 2024 16:53
@xyven1 xyven1 force-pushed the pam-add-service-defaults branch from 6026525 to 43101d9 Compare September 7, 2024 23:40
@infinisil
Copy link
Member

infinisil commented Sep 8, 2024

I believe this can be achieved without this PR using

{ lib, ... }: {
  security.pam.services = lib.mkOption {
    type = lib.types.attrsOf (lib.types.submodule ({ config, ... }: {

      # Disable unix auth by default if p11Auth is enabled
      config.unixAuth = lib.mkIf config.p11Auth (lib.mkDefault false);
    
    }));
  };
}

Can you confirm that this would work for you? Note that there could still be a reason to have the option from this PR regardless: To make it easier to discover, because the above is a bit obscure. However, if the above works, you can definitely use the same approach in the implementation.

There's also efforts like @rhendric's #314058 to make this pattern easier, or the alternative of improving documentation and discoverability for the above pattern, which I'd weakly prefer to this PR though.

@Stunkymonkey Stunkymonkey removed their request for review September 9, 2024 16:34
@xyven1
Copy link
Contributor Author

xyven1 commented Sep 9, 2024

@infinisil
Honestly, I hadn't yet seen that pattern when writing this PR (a testament to the lack of documentation and visibility), and I agree there is no reason that couldn't work for my situation. Three notes:

  1. mkDefault is not directly equivalent to setting the actual option default, this could cause confusing behavior for less informed individuals if the option you provided becomes the recommended option
  2. I really think the advantage of the implementation I provided is that it does directly affect the option default, providing the most consistent behavior with other options.
  3. Even if the proposed technique had more visibility and documentation, it would never be close to as well/self documented as simply seeing (or even getting autocomplete with LSP) the serviceDefaults option right along side all the other pam options (IMHO)

Overall, though, I think the solution you provided is enough of a solution as to make this PR not necessary, but without some improvements to the pattern it is quite confusing (you are directly manipulating the module system's inner workings just to set a setting, which I feel should simply be an option exposed as with all other options).

Edit:
I think there is reflected here one of the dualities of NixOS I often run across: People who use NixOS...

  • are very much power users and accept that at least some of their time will be dedicated to managing their system
  • want to be able to introduce and recommend NixOS to less technical people, or people who don't want time "wasted" vs just using docker or some other option they consider more efficient

In this specific case, there is maybe not an enormous impact, but making things like directly interacting with the module system a recommendation will have a lasting impact, as more and more modules will adopt that style of configuration, which I believe is antithetical to the second point mentioned above. I appreciate that edge cases are edge cases, but this PR is an example where a relatively small addition completely eradicates the need for that workaround, for anyone else. The direct cost is higher eval time (I presume) and more complex modules.

NixOS will never be a simple or non technical solution, but being able allowing people to flexibly configure their systems with the simple ease of trivial nix expressions, I feel is overall a desirable goal. On the flip side allowing people to get further into the NixOS ecosystem without using anything but trivial nix expressions will likely lead to them hitting a cliff when they inevitably need/want to do something more powerful, and so there is a good argument that modules should remain concerned with automating aspects of the software/service (such as PAM config files), versus trying to cover the automate the configuration itself (which should be done with the nix language). So the indirect cost of such a change as proposed in this PR is not insignificant either.

At the end of the day, I'm really not sure which direction the core NixOS team and the community feel the project should continue in, but there are aspects of both arguments with merit.

@xyven1
Copy link
Contributor Author

xyven1 commented Oct 21, 2024

By the way, there is precedent for this: this PR mimics how the security.acme module works.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants