Skip to content
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.

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

userdb: Fix use of memberships defined in .user/.group dropins and userdbctl outputs #32871

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

chewi
Copy link

@chewi chewi commented May 16, 2024

The membership iterator was broken because checking the .user and .group dropins was never actually implemented. Only .membership dropins were checked.

This enhances the membership iterator to step through each user/group in these dropins before moving on to the next dropin.

It does not deduplicate the users or groups as it iterates. Some users of the iterator do deduplication, but userdbctl's friendly output and the NSS module do not. Perhaps this is not an issue in practise.

The JSON and classic outputs of userdbctl were also not consistent with the friendly output because they did not use the membership iterator. This is now fixed.

Comprehensive tests are included and passing.

Fixes #24381.

Copy link

Important

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

@chewi
Copy link
Author

chewi commented May 16, 2024

I don't think these failures are related. It seems like they fail a lot.

chewi added 4 commits May 22, 2024 08:49
Failing because the checking of .user and .group dropins by the
membership iterator was never implemented and some of userdbctl's output
formats don't use the iterator anyway.
This was broken because it was never actually implemented. Only
.membership dropins were checked.

This enhances the membership iterator to step through each user/group in
these dropins before moving on to the next dropin.

It does not deduplicate the users or groups as it iterates. Some users
of the iterator do deduplication, but userdbctl's friendly output and
the NSS module do not. Perhaps this is not an issue in practise.

Fixes systemd#24381.
The friendly renderer was using a membership iterator while the JSON
renderer was not.

Fixes systemd#24381.
The friendly renderer was using a membership iterator while the classic
renderer was not.

Fixes systemd#24381.
@poettering
Copy link
Member

Hmm, the intention was actually that programs that drop in .user/.group files would also drop in .membership files. But I guess we never properly documented that.

i.e. for efficiency reasons it kinda sucks having to parse through so many files, hence the idea was to just have that separately, and expect that the tool populating these dirs would generate the right files.

@chewi
Copy link
Author

chewi commented May 22, 2024

That's not really what I wanted to hear after such an effort. 😞 I'm wondering if there's a middle ground here, but I can't think of one. I must admit that I was surprised it reparses files as often as it does, but I figured I'd just follow suit.

@chewi
Copy link
Author

chewi commented May 22, 2024

Would caching the users/groups against the dropin path and checking the mtime be an option? If not, I guess I can at least salvage the classic/JSON output fixes, along with the tests.

@poettering
Copy link
Member

Well, the stuff is in /run/ anyway, it's not really a slow disk, but a fast tmpfs, given how slow the rest of the passwd database access is it seems a bit unnecessary to cache things here.

Maintaining a cache in NSS modules is a bit nasty too, because it will show up in people's memory profilers, and they tend to complain about that.

@chewi
Copy link
Author

chewi commented May 23, 2024

I was thinking more in terms of time spent parsing than disk access.

I thought the cached data would be held by the systemd-userwork processes rather than the module, but I've had another idea in any case. What if the .membership dropins served as the cache? It could read the .user/.group dropins and create .membership files under /run/userdb on the fly. These could be created as symlinks, both to differentiate them from manually-created .membership dropins, and to show which dropin led to their creation. That makes it work more like how it does now, but also ensures consistency between the different files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please-review PR is ready for (re-)review by a maintainer tests userdb util-lib
Development

Successfully merging this pull request may close these issues.

group memberships not shown for drop in user records
2 participants