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

mod_authz_unixgroup: Use getgrouplist #55

Merged
merged 1 commit into from
Nov 23, 2024

Conversation

joakim-tjernlund
Copy link
Contributor

Some services, like sssd, can optimize away grp->gr_mem which makes this module fail group lookup.

Use getgrouplist(3) instead and gid_from_group(3bsd) which uses libbsd, link with -lbsd.
This avoids the problematic getgrgid()/getgrnam() functions.

This replaces #54

@bimimicah
Copy link
Collaborator

bimimicah commented Nov 8, 2024

I took a look at these and I can see why some auth providers (especially ones that might connect to remote LDAP like sssd) would want to avoid making copies of the entire membership of any given group, and thus might feel justified in breaking POSIX standards by leaving gr_mem null.

I am somewhat concerned about compatibility issues with this new solution, though, since the critical getgrouplist function and especially the gid_from_group function are not in POSIX and the latter appears to be mostly BSD-only.
Personally I would prefer the conditionally defined solution you provided from #54. If we merge this, we could add a troubleshooting note in the wiki. [EDIT] What I mean here is to put your solution from this pull request inside conditional define checks the way you did in #54, not to use that code directly [/EDIT]

@pbiering, are you still using mod_authz_unixgroup? Do you have any thoughts on this? I don't use this module personally and don't have a way to effectively test it.

@bimimicah

This comment was marked as outdated.

@bimimicah

This comment was marked as resolved.

@joakim-tjernlund
Copy link
Contributor Author

It looks like #54 doesn't actually solve the problem, I can see why you replaced it with #55 now. If you could, I would prefer a conditionally defined solution like #54, but with the new code from #55.

I could do that but I would prefer to make the new version default, what do you think?

@bimimicah
Copy link
Collaborator

It looks like #54 doesn't actually solve the problem, I can see why you replaced it with #55 now. If you could, I would prefer a conditionally defined solution like #54, but with the new code from #55.

I could do that but I would prefer to make the new version default, what do you think?

I think that should be fine, since the distros that package mod_authz_unixgroup all package libbsd as well, from what I can tell.

Please add the relevant link flag to mod_authz_unixgroup's INSTALL file, e.g. on the apxs -c line.

Once it is ready, I will probably wait a week or so before merging to give @pbiering a chance to weigh in.

@joakim-tjernlund

This comment was marked as outdated.

@joakim-tjernlund
Copy link
Contributor Author

Please make a release too.

@joakim-tjernlund

This comment was marked as resolved.

@bimimicah

This comment was marked as resolved.

@joakim-tjernlund

This comment was marked as resolved.

@bimimicah

This comment was marked as resolved.

@joakim-tjernlund

This comment was marked as resolved.

@bimimicah

This comment was marked as resolved.

@bimimicah
Copy link
Collaborator

I think this PR looks good. If there are no comments by next week, I will merge it and make a new release.

@bimimicah
Copy link
Collaborator

Thanks for your contribution!

@pbiering

This comment was marked as resolved.

@joakim-tjernlund
Copy link
Contributor Author

just a small typo in INSTALL file fixed

@joakim-tjernlund

This comment was marked as resolved.

@bimimicah

This comment was marked as outdated.

@bimimicah bimimicah force-pushed the no_grps branch 19 times, most recently from f323515 to 7b2536c Compare November 23, 2024 03:24
…mpatibility

Some services, like sssd, can optimize away grp->gr_mem which makes this
module fail group lookup.

Use getgrouplist(3) instead and gid_from_group(3bsd) which uses libbsd,
link with -lbsd.
This avoids the problematic getgrgid()/getgrnam() functions.
@bimimicah
Copy link
Collaborator

After a great deal of probably unnecessary pain (do we even really need to support macOS anyway?), we now have this PR building on all platforms! 🎉

Thanks again @joakim-tjernlund for your contribution!

@bimimicah bimimicah merged commit e430131 into phokz:master Nov 23, 2024
5 checks passed
@bimimicah
Copy link
Collaborator

Please make a release too.

Version 1.2.0 of mod_authz_unixgroup is now tagged and released. 🎉

@joakim-tjernlund
Copy link
Contributor Author

Thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants