fix: move pam configuration to sudo_local

[Mic92]

Supersedes #1020, #591, #787 and #662

Closes #784 and #985

[dwt]

I was just bitten again by the latest os update reverting this option - so is there anything I can do to move this forward?

[Enzime]

I'm in the process of fixing this PR :) Aiming to merge it today 😄

[Enzime]

@Mic92 can you test this PR again?

[Enzime]

@dwt @pitkling @takoverflow if you could also test this PR that'd be great

[mirkolenz]

Great to see this moving forward! Would it be possible to add a setting like extraConfig to pass additional pam modules? For example, I added pam_watchid.so to my config like so:

{
  environment.etc."pam.d/sudo_local" = {
    enable = true;
    text = ''
      auth       optional       ${lib.getLib pkgs.pam-reattach}/lib/pam/pam_reattach.so
      auth       sufficient     ${lib.getLib pkgs.pam-watchid}/lib/pam/pam_watchid.so
      auth       sufficient     pam_tid.so
    '';
}

(note that pkgs.pam-watchid is a added via a custom overlay)

[emilazy]

The latter two commits should have Co-authored-by: Andrew Lubawy <[email protected]> trailers to preserve attribution (possibly @Mic92 too? I didn’t look at the previous state of this PR). Otherwise LGTM in principle with some comments.

[emilazy]

nit: I’d prefer to trim the unnecessary alignment whitespace here, but it doesn’t matter.

[emilazy]

As I said in the previous PR I think we shouldn’t turn this on by default. It brings a third‐party PAM module into the sudo chain which I don’t think we should be doing implicitly, and I think most users probably don’t need it. IIRC it relies on undocumented private APIs too.

[emilazy]

Similarly, this could be introduced as security.pam.services.sudo_local.reattach.

[emilazy]

Per the previous comment, since we’re adding more options and moving stuff around, this could be renamed to security.pam.services.sudo_local.touchIdAuth to match the NixOS structure.

[emilazy]

I take it this is to accommodate users that want a custom sudo_local file?

I’m wondering if this should be security.pam.services.sudo_local.enable. That’s not an option that exists in NixOS, but it brings us closer to NixOS’s structure for security.pam.*. We’re just managing the sudo_local configuration rather than PAM as a whole here.

I might prefer something like @mirkolenz’s solution, which could also increase NixOS compatibility: we could have security.pam.services.sudo_local.text which is lib.types.lines and populate it with our settings by default; users who want extra configuration could use lib.mkAfter; users who want to replace the entire configuration could override it completely. If someone really wants a totally unmanaged sudo_local file, we could make environment.etc."pam.d/sudo_local".enable just a lib.mkDefault so that they can disable it directly. (Or we could keep the non‐standard .enable, but I think in the presence of .text it would be niche enough that it’s fine to expect someone to toggle the file like that.)

(if not for .text, we could make it security.pam.services.sudo.* since we’re effectively extending the sudo configuration. But if we add .text – which seems nicer than .enable here – then we should make sure it’s sudo_local, as security.pam.services.sudo_local.text will not be able to overwrite the entire PAM sudo service configuration, as a NixOS user or configuration might expect.)

[pitkling]

@dwt @pitkling @takoverflow if you could also test this PR that'd be great

@Enzime: Works great for me (tested on a current Sequoia installation), thanks for continuing this.

That said, I agree with @emilazy's review comments. Especially that enableSudoPamReattach should be disabled by default. And using an interface based around security.pam.services.sudo_local.text like described in this review comment seems like a nice and flexible way. I would slightly prefer to keep the (seemingly non-standard) security.pam.services.sudo_local.enable in favor of having the user to disable the management directly via environment.etc."pam.d/sudo_local".enable, though. Just to keep the module configuration under the same hood.

[ivankovnatsky]

Also tested the current implementation, no issues.