pam: Verify /etc/pam.d/* file contents

[l0b0]

Motivation for this change

PAM file contents are critical for system security, and verifying their contents with a reasonable configuration can go a long way towards avoiding syntax or semantic errors.

Based on the work mostly by @Artturin in #145574, and inspired by @Xe's The Surreal Horror of PAM.

Current approach

(Thanks for the tips, @Artturin and @thiagokokada!):

1. Read a file into a Python set.
2. Verify that all the expected functional lines are there. Ensures that changes to any of the existing lines require changing the test, making any breakage much more visible.
3. Verify that all the rest of the lines are non-functional, that is, either a comment line or empty. Ensures that additions to the file isn't overlooked.
4. (TBD): Concatenate tests for each of the relevant files, and include the filename in the error message.

This approach makes TDD easy (change the expected lines, see the test fail, then change the production code), and should make any breakage really easy to spot.

Questions

• How do you like the current approach?
• Is this sufficiently granular? That is, would you rather have separate tests for each of the conditionals in nixos/modules/security/pam.nix instead of one per /etc/pam.d/* file? This would make it harder to verify that there are no unexpected functional lines in the files. A hybrid approach which verifies the default contents and checks how the output differs based on various settings could work, but could also end up being fairly complex.
• Should I verify that the lines occur in a specific sequence? That is, does PAM care about the sequence?
• Is this sufficiently complete? I might need more than one configuration to test all the possible PAM file contents.

@mkg20001 @ju1m You've both done some non-trivial changes to pam.nix relatively recently. What do you think of the above/the work so far?

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) N/A
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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 N/A
• 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.

[nixos-discourse]

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

https://discourse.nixos.org/t/how-to-read-a-file-in-a-nixpkgs-test/16130/1

[Artturin]

Theres a existing test that uses grep to check the file contents https://github.com/NixOS/nixpkgs/blob/master/nixos/tests/pam-u2f.nix

might be useful https://github.com/pbrezina/pam-test

[nixos-discourse]

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

https://discourse.nixos.org/t/how-to-best-verify-etc-pam-d-file-contents/16217/1

[mkg20001]

If you want to you could add more modules and also test more configuration files to get a good coverage

such as pam.d/login, pam.d/sudo, etc and some modules from https://search.nixos.org/options?channel=21.05&from=0&size=50&sort=relevance&type=packages&query=security.pam

[l0b0]

That's the plan, as soon as I get enough feedback on this that I'm fairly certain the result can be merged without major rework.

[l0b0]

Would it be possible to merge this as-is? Then I can create a separate PR with more tests.

[Artturin]

Please move the other pam tests in to the pam subdirectory
and add a comment to the pam module above where the rules are declared to remind people to update the test|s after changing them

then i will merge this because we can always rework it since its a test and not user facing

[Artturin]

created a tracking issue for pam #147565

[Artturin]

nix build ".#nixosTests.pam-oath-login"
error: getting status of '/nix/store/76yr5ig54sbjfp66xl0vpqhd9qldqjd4-source/nixos/tests/pam/make-test-python.nix': No such file or directory

i fixed it for you

waiting for ofborg and then merging

[Ma27]

Isn't a full-featured VM test a little heavy to test a bunch of generated files?

[l0b0]

Isn't a full-featured VM test a little heavy to test a bunch of generated files?

I just copied the pattern from other tests. If you know of a more light-weight way to do it (with the same kind of guarantees about testing the actually resulting files rather than an intermediate result) I'd be interested.

[Ma27]

I just copied the pattern from other tests

Well, other tests check for functionality in a VM, we only check build products after we have booted into a VM.

You could either

• check inside the testScript the build products (in our case, PAM files) without booting the VM
• use something like pkgs-tests and move the formatting logic of PAM into its own function.

EDIT:L I'm open to other, better ideas though :)