Skip to content

nixos/pam: enable lastlog2 import service if any pam service uses lastlog #432567

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.

Sign up for GitHub

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

Merged
K900 merged 3 commits into from
Aug 11, 2025
Merged

nixos/pam: enable lastlog2 import service if any pam service uses lastlog #432567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f3cd67c
nixos/pam: enable lastlog2 import service if any pam service uses las…
LordGrimmauld Aug 10, 2025
a1d47a4
nixos/pam: fix lastlog2 import service
LordGrimmauld Aug 11, 2025
122c5aa
nixos/tests/pam-lastlog: test legacy lastlog importer
LordGrimmauld Aug 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions nixos/modules/security/pam.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2311,11 +2311,28 @@ in

environment.etc = lib.mapAttrs' makePAMService enabledServices;

systemd = lib.optionalAttrs config.security.pam.services.login.updateWtmp {
tmpfiles.packages = [ pkgs.util-linux.lastlog ]; # /lib/tmpfiles.d/lastlog2-tmpfiles.conf
services.lastlog2-import.enable = true;
packages = [ pkgs.util-linux.lastlog ]; # lib/systemd/system/lastlog2-import.service
};
systemd =
lib.optionalAttrs
(lib.any (service: service.updateWtmp) (lib.attrValues config.security.pam.services))
Copy link
Contributor

@Majiir Majiir Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of config.security.pam.services, this should be enabledServices.

Copy link
Contributor Author

@LordGrimmauld LordGrimmauld Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure i agree. the lastlog importer service is gated behind the old lastlog db existing via a systemd condition. I did even consider just enabling the service unconditionally. Arguably, if someone has an old lastlog db but currently no pam service that updates lastlog, they might still want to be able to read that old db using the new tools, which requires the import. And having the impoorter around doesn't really hurt anything, because if there is nothing to import it just won't do anything.

That said, maybe this scenario is a bit niche, so i am willing to switch to enabledServices. I assume your point is to make the installed services as minimal as possible, in which case you do have a point.

Sorry for the mess, i now realize #429203 should have been split into two PRs: the util-linux output split and the PAM changes, with the pam changes waiting for a proper thorough review. If i had to do this again i'd do it differently.

Copy link
Contributor

@Majiir Majiir Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I leave it to you to figure out whether the lastlog import should be conditional or unconditional - I have no idea!

But if you choose to make it conditional on whether any PAM service has updateWtmp, the correct way to do it is with enabledServices. Otherwise, we could be in a situation where an explicitly disabled PAM service (with no PAM rules file) is somehow influencing the system configuration.

I don't like that doing the obvious thing (reading config.security.pam.services) is the wrong thing, so I'm considering this a pam.nix rough edge that needs fixing. Using enabledServices in the meantime helps keep usage consistent.

Copy link
Contributor Author

@LordGrimmauld LordGrimmauld Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough. I'll draft a change to enabledServices, your reasoning is valid. But i don't think the current state is catastrophically broken, so this time we can wait for you to thoroughly review and discuss the changes.

Copy link
Contributor

@Majiir Majiir Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Agreed, it's more of a nitpick.

{
tmpfiles.packages = [ pkgs.util-linux.lastlog ]; # /lib/tmpfiles.d/lastlog2-tmpfiles.conf
services.lastlog2-import = {
enable = true;
wantedBy = [ "default.target" ];
after = [
"local-fs.target"
"systemd-tmpfiles-setup.service"
];
# TODO: ${pkgs.util-linux.lastlog}/lib/systemd/system/lastlog2-import.service
# uses unpatched /usr/bin/mv, needs to be fixed on staging
# in the meantime, use a service drop-in here
serviceConfig.ExecStartPost = [
""
"${lib.getExe' pkgs.coreutils "mv"} /var/log/lastlog /var/log/lastlog.migrated"
];
};
packages = [ pkgs.util-linux.lastlog ]; # lib/systemd/system/lastlog2-import.service
};

security.pam.services = {
other.text = ''
Expand Down
17 changes: 13 additions & 4 deletions nixos/tests/pam/pam-lastlog.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,18 @@
};

testScript = ''
machine.wait_for_unit("multi-user.target")
machine.succeed("run0 --pty true") # perform full login
print(machine.succeed("lastlog2 --active --user root"))
machine.succeed("stat /var/lib/lastlog/lastlog2.db")
with subtest("Test legacy lastlog import"):
# create old lastlog file to test import
# empty = nothing will actually be imported, but the service will run
machine.succeed("touch /var/log/lastlog")
machine.wait_for_unit("lastlog2-import.service")
machine.succeed("journalctl -b --grep 'Starting Import lastlog data into lastlog2 database'")
machine.succeed("stat /var/log/lastlog.migrated")

with subtest("Test lastlog entries are created by logins"):
machine.wait_for_unit("multi-user.target")
machine.succeed("run0 --pty true") # perform full login
print(machine.succeed("lastlog2 --active --user root"))
machine.succeed("stat /var/lib/lastlog/lastlog2.db")
'';
}
Loading