URL: https://github.com/SSSD/sssd/pull/5784 Author: ikerexxe Title: #5784: proxy: allow removing group members Action: opened
PR body: """ The proxy provider doesn't allow to remove group members once they have been added. This patch allows to do it by looping the member list from the cache and comparing it with the actual membership list. If a member is missing then it's removed from the cache.
Resolves: https://github.com/SSSD/sssd/issues/5783 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5784/head:pr5784 git checkout pr5784
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: +Bugzilla
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/5784 Author: ikerexxe Title: #5784: proxy: allow removing group members Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5784/head:pr5784 git checkout pr5784
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
alexey-tikhonov commented: """ Hi @ikerexxe ,
I've left few comment inline. In general patch should work.
But the first question to answer:
You search for groups user is currently member of (`SYSDB_MEMBEROF`) => got list of DNs => convert every DN to RDN (group name) => and then for every RDN you iterate over initgroups GID list to get a group name of every GID => convert it to FQDN and then do a string comparison (RDN with FQDN), right?
This feels extremely inefficient: you repeat **the same set** of lookups and conversions over initgroups GIDs for every RDN (and it will be also repeated later in `get_initgr_groups_process()`::`NSS_STATUS_SUCCESS`)...
Wouldn't it make sense to instead lookup GID of SYSDB_MEMBEROF DN and compare GIDs (numerical values)? (If this is not feasible then at the very least you should compose a list of RDNs first and then do initgroups GIDs iteration once)
@sumit-bose , can it happen (is it supported) two groups in different domains have the same GID? """
See the full comment at https://github.com/SSSD/sssd/pull/5784#issuecomment-933460178
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
sumit-bose commented: """
@sumit-bose , can it happen (is it supported) two groups in different domains have the same GID?
Hi,
if the two domains are managed separately it can of course happen that two groups have the same GID. For lookups by GID the group from the first domain listed in the `domains` option will win. A lookup by name, especially with a fully-qualified name, will most probably resolve both groups.
Given that the `id` command will do lookups by GID an `id` lookup for the user from the second domain which is a member of the group with the duplicated GID will return the wrong group name. In this sense I would say we do not support this kind of configuration.
Additionally, the filesystem is doing access control based with respect to groups with the help of the GID using the same GID in different domains might give users access to files of users from the other domain, which is typically not expected.
HTH
bye, Sumit
"""
See the full comment at https://github.com/SSSD/sssd/pull/5784#issuecomment-934479813
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
alexey-tikhonov commented: """
Given that the `id` command will do lookups by GID an `id` lookup for the user from the second domain which is a member of the group with the duplicated GID will return the wrong group name. In this sense I would say we do not support this kind of configuration.
Additionally, the filesystem is doing access control based with respect to groups with the help of the GID using the same GID in different domains might give users access to files of users from the other domain, which is typically not expected.
I read this as GIDs collisions aren't supported (i.e. GIDs are unique) and hence we can compare GIDs. """
See the full comment at https://github.com/SSSD/sssd/pull/5784#issuecomment-934497065
URL: https://github.com/SSSD/sssd/pull/5784 Author: ikerexxe Title: #5784: proxy: allow removing group members Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5784/head:pr5784 git checkout pr5784
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
ikerexxe commented: """
Wouldn't it make sense to instead lookup GID of SYSDB_MEMBEROF DN and compare GIDs (numerical values)?
Yes, so I've updated the PR accordingly. Nowadays, a list of GIDs is composed from the cache and this is compared with the actual GIDs list. This should optimize the execution flow compared with the previous proposed patch.
"""
See the full comment at https://github.com/SSSD/sssd/pull/5784#issuecomment-936256484
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/5784 Author: ikerexxe Title: #5784: proxy: allow removing group members Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5784/head:pr5784 git checkout pr5784
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
ikerexxe commented: """ I've updated the patch taking into account your suggestions. """
See the full comment at https://github.com/SSSD/sssd/pull/5784#issuecomment-945777100
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/5784 Author: ikerexxe Title: #5784: proxy: allow removing group members Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5784/head:pr5784 git checkout pr5784
URL: https://github.com/SSSD/sssd/pull/5784 Author: ikerexxe Title: #5784: proxy: allow removing group members Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5784/head:pr5784 git checkout pr5784
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
ikerexxe commented: """ Ready for a new rounds of reviews. """
See the full comment at https://github.com/SSSD/sssd/pull/5784#issuecomment-948683748
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
alexey-tikhonov commented: """ Thank you for changes. ACK from me.
Since I somewhat influenced final solution, would be good to if somebody else took a look.
"""
See the full comment at https://github.com/SSSD/sssd/pull/5784#issuecomment-950870013
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
alexey-tikhonov commented: """
Thank you for changes. ACK from me.
Since I somewhat influenced final solution, would be good to if somebody else took a look.
@sumit-bose , @pbrezina , @thalman , @justin-stephenson , could anyone of you please take a look? """
See the full comment at https://github.com/SSSD/sssd/pull/5784#issuecomment-953189103
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
pbrezina commented: """ I did not test it, but codewise ack. """
See the full comment at https://github.com/SSSD/sssd/pull/5784#issuecomment-954680724
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: +Ready to push
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
pbrezina commented: """ Pushed PR: https://github.com/SSSD/sssd/pull/5784
* `master` * 301659a662a7a7aac11096fd0409f83b45cb41d1 - proxy: allow removing group members
"""
See the full comment at https://github.com/SSSD/sssd/pull/5784#issuecomment-956103712
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/5784 Author: ikerexxe Title: #5784: proxy: allow removing group members Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5784/head:pr5784 git checkout pr5784
URL: https://github.com/SSSD/sssd/pull/5784 Title: #5784: proxy: allow removing group members
Label: -Ready to push
sssd-devel@lists.fedorahosted.org
-
alexey-tikhonov -
ikerexxe
-
pbrezina
-
sumit-bose