nixos/pam: avoid extra lines in pam files

[l0b0]

Motivation for this change

Closes #145286

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable (see below)
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested basic functionality of all binary files (usually in ./result/bin/) (N/A)
• 21.11 Release Notes (or backporting 21.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.

Test procedure (on staging this time)

$ cat > config.nix <<'EOF'
> {
>   config = {
>     krb5.enable = true;
>     security = {
>       apparmor.enable = true;
>       pam = {
>         enableEcryptfs = true;
>         enableSSHAgentAuth = true;
>         makeHomeDir.skelDirectory = true;
>       };
>     };
>     services.sssd.enable = true;
>     users.motd = "";
>     virtualisation.lxc.lxcfs.enable = true;
>   };
> }
> EOF
$ git checkout upstream/staging
[…]
$ git show --oneline
22a4fcfd4e8 (HEAD, upstream/staging) Merge pull request #145239 from Stunkymonkey/development-pname-version-1
$ cat "$(nix-build nixos --include nixos-config=config.nix --attr 'config.environment.etc."pam.d/xlock".source')" > ~/xlock.pam.orig
[…]
$ git checkout refactor/avoid-extra-empty-lines-in-etc-pamd-files
$ git rev-list --count HEAD ^upstream/staging
1
$ cat "$(nix-build nixos --include nixos-config=config.nix --attr 'config.environment.etc."pam.d/xlock".source')" > ~/xlock.pam.new
[…]
$ diff --ignore-blank-lines ~/xlock.pam.{orig,new} && echo 'No difference'
No difference
$ cat ~/xlock.pam.new
# Account management.
account required pam_unix.so
account sufficient /nix/store/xk2wjp29yhbwm3s2zs7bay518pmjz1pc-sssd-2.6.0/lib/security/pam_sss.so
account sufficient /nix/store/jpd67ssw170d4cw3gwg3crx4a50srvq5-pam-krb5-4.10/lib/security/pam_krb5.so

# Authentication management.
auth required pam_unix.so   likeauth
auth optional /nix/store/1gb81pi3dv937vqgd5vh2rppbi356cmi-ecryptfs-111/lib/security/pam_ecryptfs.so unwrap
auth sufficient pam_unix.so   likeauth try_first_pass
auth sufficient /nix/store/xk2wjp29yhbwm3s2zs7bay518pmjz1pc-sssd-2.6.0/lib/security/pam_sss.so use_first_pass
auth [default=ignore success=1 service_err=reset] /nix/store/jpd67ssw170d4cw3gwg3crx4a50srvq5-pam-krb5-4.10/lib/security/pam_krb5.so use_first_pass
auth [default=die success=done] /nix/store/gdj3886xf3wrmbpjq3xb0qa4nnvib7fp-pam_ccreds-10/lib/security/pam_ccreds.so action=validate use_first_pass
auth sufficient /nix/store/gdj3886xf3wrmbpjq3xb0qa4nnvib7fp-pam_ccreds-10/lib/security/pam_ccreds.so action=store use_first_pass
auth required pam_deny.so

# Password management.
password sufficient pam_unix.so nullok sha512
password optional /nix/store/1gb81pi3dv937vqgd5vh2rppbi356cmi-ecryptfs-111/lib/security/pam_ecryptfs.so
password sufficient /nix/store/xk2wjp29yhbwm3s2zs7bay518pmjz1pc-sssd-2.6.0/lib/security/pam_sss.so use_authtok
password sufficient /nix/store/jpd67ssw170d4cw3gwg3crx4a50srvq5-pam-krb5-4.10/lib/security/pam_krb5.so use_first_pass

# Session management.
session required pam_env.so conffile=/etc/pam/environment readenv=0
session required pam_unix.so
session optional /nix/store/1gb81pi3dv937vqgd5vh2rppbi356cmi-ecryptfs-111/lib/security/pam_ecryptfs.so
session optional /nix/store/xk2wjp29yhbwm3s2zs7bay518pmjz1pc-sssd-2.6.0/lib/security/pam_sss.so
session optional /nix/store/jpd67ssw170d4cw3gwg3crx4a50srvq5-pam-krb5-4.10/lib/security/pam_krb5.so
session optional /nix/store/v9cwqh3r09400pyilq722320rkx0w94x-lxc-4.0.11/lib/security/pam_cgfs.so -c all

Caveat: The tests above are not exhaustive - some options are mutually exclusive and some options should be passed to pam.nix rather than in config.nix. Hopefully the above will make it easy for anyone to test their favourite PAM section :)

In summary, this gets rid of a bunch of (all?) the unnecessary empty lines in /etc/pam.d files, while keeping the empty lines between sections of the file and seemingly keeping the structure of each of the entries.

@ttuegel, @bjornfor.

[nixos-discourse]

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

https://discourse.nixos.org/t/how-to-build-an-arbitrary-nixpkgs-file/16013/13

[ck3d]

I would argue that the Nix expression is read more often than the output of the expression. From my point of view, the old code look more appealing to me.

[Artturin]

commit name should be
nixos/pam: avoid extra lines in pam files

[l0b0]

I would argue that the Nix expression is read more often than the output of the expression. From my point of view, the old code look more appealing to me.

Since there are many more users than developers, I would expect the opposite. But we should definitely work on getting the most appealing code before merging.

That said, this is closer to the approach taken further down in the same file. Consistency would be nice too.

[Artturin]

this doesn't need to target staging as there are not many rebuilds

[l0b0]

this doesn't need to target staging as there are not many rebuilds

Oh, that's how it works? Rebased again, and thanks for the tip.

[Artturin]

are you familiar with pam? / do you have experience with it

[l0b0]

are you familiar with pam? / do you have experience with it

Only a couple basic changes to /etc/pam.d files. I think one time I swapped two entries after logging in took a long time, and another time I believe I added a keyword for some LDAP-related reason. I tried to make properly sure only empty lines were removed though, and my (limited) tests bear that out. I don't have flake support enabled, but if you have time to check using your configuration that would be useful to make sure we don't deviate semantically.

Update: If you want to know why I did this, I watched this video and thought, "Hey, I've looked at PAM files before, I wonder what they're like in NixOS."

[ck3d]

Your current version looks better than the original one, great!

[Artturin]

making a diff script..

[Artturin]

{
  inputs = {
    nixpkgs.url = "github:NixOS/nixpkgs/master";
  };

  outputs = inputs@{ self, nixpkgs }: {

    nixosConfigurations.vm = nixpkgs.lib.nixosSystem {
      system = "x86_64-linux";
      specialArgs = { inherit inputs; };
      modules = [
        ({ pkgs, lib, modulesPath, ... }: {
          krb5.enable = true;
          users.mutableUsers = false;
          users.users.root = {
            password = "root";
          };
          users.users.user = {
            password = "user";
            isNormalUser = true;
            extraGroups = [ "wheel" ];
          };

          imports = [
            (modulesPath + "/profiles/minimal.nix")
          ];
        })
      ];
    };
    # So that we can just run 'nix run' instead of
    # 'nix build ".#nixosConfigurations.vm.config.system.build.vm" && ./result/bin/run-nixos-vm'
    defaultPackage.x86_64-linux = self.nixosConfigurations.vm.config.system.build.vm;
    defaultApp.x86_64-linux = {
      type = "app";
      program = "${self.defaultPackage.x86_64-linux}/bin/run-nixos-vm";
    };
  };
}

#!/usr/bin/env bash

nix build -o resultNoFix
nix build --override-input nixpkgs "github:l0b0/nixpkgs/refactor/avoid-extra-empty-lines-in-etc-pamd-files" -o resultFix

files=$(ls resultNoFix/system/etc/pam.d/)

for f in $files; do 
    diff --ignore-blank-lines $(chase resultNoFix/system/etc/pam.d/$f) $(chase resultFix/system/etc/pam.d/$f)
done

no output so there are no differences other than blank line changes

[l0b0]

no output so there are no differences other than blank line changes

Excellent! I don't really want to delay this any longer, but is there some way this could be made into a test? PAM is super important to get right, so it would be great to have some actual proof that it generates what we expect.

[Artturin]

no output so there are no differences other than blank line changes

Excellent! I don't really want to delay this any longer, but is there some way this could be made into a test? PAM is super important to get right, so it would be great to have some actual proof that it generates what we expect.

maybe creating a vm test which runs pamtester commands http://pamtester.sourceforge.net
and various other command

[Artturin]

rebuilt my system with this change https://nixos.wiki/wiki/Nixpkgs/Reviewing_changes#Modules

  disabledModules = [ "security/pam.nix" ];
  
  imports = let
    pkgsReview = builtins.fetchTarball {
      url = "https://github.com/l0b0/nixpkgs/archive/refactor/avoid-extra-empty-lines-in-etc-pamd-files.tar.gz";
    };
  in [
    (import "${pkgsReview}/nixos/modules/security/pam.nix")
  ];
  
  security.pam.p11.enable = true;

and everything seems ok

[l0b0]

maybe creating a vm test which runs pamtester commands http://pamtester.sourceforge.net and various other command

I like the overall idea, I just wouldn't want to use a 15 year old tool hosted on an HTTP web site with a provider known for injecting stuff into the packages they host.