nixos/pam: enable lastlog2 import service if any pam service uses lastlog#432567
nixos/pam: enable lastlog2 import service if any pam service uses lastlog#432567K900 merged 3 commits intoNixOS:staging-nextfrom
Conversation
nixpkgs-ci
bot
added
10.rebuild-linux: 1-10
|
While writing a test for the This is fixed in a drop-in now, i will draft a PR to the next staging to fix it in Now the lastlog importer gets enabled whenever anything uses lastlog, and it actually works. |
LordGrimmauld
force-pushed
the
lastlog-improvements
branch
from
1ee1a7b to
122c5aa
Compare
nixpkgs-ci
bot
added
10.rebuild-darwin: 1-10
| }; | ||
| systemd = | ||
| lib.optionalAttrs | ||
| (lib.any (service: service.updateWtmp) (lib.attrValues config.security.pam.services)) |
There was a problem hiding this comment.
Instead of config.security.pam.services, this should be enabledServices.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks! Agreed, it's more of a nitpick.
See TODO in NixOS#432567 for context Co-authored-by: Grimmauld <[email protected]>
See TODO in NixOS#432567 for context Co-authored-by: Grimmauld <[email protected]>
3 participants
As described in util-linux/util-linux#2508 (comment),
lastlog2-import.serviceis used for compatibility to import oldlastlogdatabases. It is possible someone usedupdateWtmpon some PAM service while disabling it onlogin, which would currently make the import of the old database fail.Fixes #429203 (comment)
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.