URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: opened
PR body: """ Those two patches fix https://bugzilla.redhat.com/show_bug.cgi?id=1401241
The sssd.conf used in order to reproduce this issue looks like:
``` [sssd] config_file_version = 2 services = nss, pam domains = ad.fidencio.lan
[nss]
[pam]
[domain/ad.fidencio.lan] ad_domain = ad.fidencio.lan krb5_realm = AD.FIDENCIO.LAN realmd_tags = manages-system joined-with-adcli cache_credentials = True krb5_store_password_if_offline = True default_shell = /bin/bash ldap_id_mapping = True use_fully_qualified_names = True fallback_homedir = /home/%u@%d ldap_referrals = false enumerate = false id_provider = ldap #id_provider = ad auth_provider = krb5 chpass_provider = krb5 access_provider = ldap ldap_sasl_mech = GSSAPI ldap_schema = ad ldap_user_object_class = user ldap_user_home_directory = unixHomeDirectory ldap_user_principal = userPrincipalName ldap_group_object_class = group ldap_access_order = expire ldap_account_expire_policy = ad ldap_force_upper_case_realm = true
```
The reproducer can be found in the bug report. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ CI: http://sssd-ci.duckdns.org/logs/job/60/61/summary.html
Debian testing failure is unrelated. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-273534076
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ CI: http://sssd-ci.duckdns.org/logs/job/60/61/summary.html
Debian testing failure is unrelated. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-273534076
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ I haven't read the patches, I just realized we might want to have a ticket and not just a downstream bugzilla. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-274185826
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ https://fedorahosted.org/sssd/ticket/3282 """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-274185860
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ Updating the patch set with a test for this fix ... """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-274545739
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ The code looks good and manual tests went fine so far. Also the CI run passed: http://sssd-ci.duckdns.org/logs/job/62/55/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-279370447
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ The tests didn't find any regressions, ACK """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-279653099
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
lslebodn commented: """ On (14/02/17 01:25), Jakub Hrozek wrote:
The tests didn't find any regressions, ACK
I have a tiny question:
What would happen if there ate two groups in directlry server with the same GID?
I know it is a little bit corner case but we hit it few times.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-279658103
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ @lslebodn: Firstly, my answer may be incomplete due to the lack of knowledge, but let's try ... 1) As far as I understand SSSD does not deal properly with multiple groups having the same GID and I'm saying that based on both AD's and LDAP's code, where the search is done by the GID and we expect only one result; 2) We already have at least one bug opened for this situation (https://fedorahosted.org/sssd/ticket/2982) and in case we decide to deal properly with this my feeling is that it will have to be done in all different parts of the code.
I understand why you're worried and I see we can hit this situation. But we can hit this situation even without my fix. So I'd like to propose to fix this situation when someone has time to work on this and in a better way than just "don't deal with group renaming".
Does this make sense for you? """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-279661448
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
lslebodn commented: """ On (14/02/17 01:57), fidencio wrote:
@lslebodn: Firstly, my answer may be incomplete due to the lack of knowledge, but let's try ...
- As far as I understand SSSD does not deal properly with multiple groups having the same GID and I'm saying that based on both AD's and LDAP's code, where the search is done by the GID and we expect only one result;
Yes, we expect but reality is different and we got bug reports about incomplete groups. And result of bug investigation was colliding GIDs.
Current version detects that there is a collision of GIDs and will not return any result for problematic groups.
- We already have at least one bug opened for this situation (https://fedorahosted.org/sssd/ticket/2982) and in case we decide to deal properly with this my feeling is that it will have to be done in all different parts of the code.
I understand why you're worried and I see we can hit this situation. But we can hit this situation even without my fix. So I'd like to propose to fix this situation when someone has time to work on this and in a better way than just "don't deal with group renaming".
Yes we can hit this situation without your fix but I am curious what will be a difference between current behaviour and with this PR.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-279702381
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ On Tue, Feb 14, 2017 at 2:07 PM, lslebodn notifications@github.com wrote:
On (14/02/17 01:57), fidencio wrote:
@lslebodn: Firstly, my answer may be incomplete due to the lack of knowledge, but
let's try ...
- As far as I understand SSSD does not deal properly with multiple
groups having the same GID and I'm saying that based on both AD's and LDAP's code, where the search is done by the GID and we expect only one result;
Yes, we expect but reality is different and we got bug reports about incomplete groups. And result of bug investigation was colliding GIDs.
Current version detects that there is a collision of GIDs and will not return any result for problematic groups.
- We already have at least one bug opened for this situation (
https://fedorahosted.org/sssd/ticket/2982) and in case we decide to deal properly with this my feeling is that it will have to be done in all different parts of the code.
I understand why you're worried and I see we can hit this situation. But
we can hit this situation even without my fix. So I'd like to propose to fix this situation when someone has time to work on this and in a better way than just "don't deal with group renaming".
Yes we can hit this situation without your fix but I am curious what will be a difference between current behaviour and with this PR.
With this patch we will end up removing one of first group cached with the gid and update with the new one. Yes, as you mentioned, it's a corner case. And yes, as you said, it may bite us really hard in the future.
So, I'd like to ask for suggestions (@sbose ?, @jhrozek ?) on how to deal with this.
In case we get bitten by one of those two bugs, which one would hurt less?
Also, would be nice to see some bug reports about this (in case you have those handy, @lslebodn).
Last but not least, @lslebodn suggested (in face to face conversation in the office) that maybe we could add an option which would be used for fixing the group renaming for whoever reported this bug (and this option wouldn't be enabled by default). Opinions on Lukáš' idea?
Best Regards,
"""
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-279709619
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ On Tue, Feb 14, 2017 at 05:41:34AM -0800, fidencio wrote:
On Tue, Feb 14, 2017 at 2:07 PM, lslebodn notifications@github.com wrote:
On (14/02/17 01:57), fidencio wrote:
@lslebodn: Firstly, my answer may be incomplete due to the lack of knowledge, but
let's try ...
- As far as I understand SSSD does not deal properly with multiple
groups having the same GID and I'm saying that based on both AD's and LDAP's code, where the search is done by the GID and we expect only one result;
Yes, we expect but reality is different and we got bug reports about incomplete groups. And result of bug investigation was colliding GIDs.
Current version detects that there is a collision of GIDs and will not return any result for problematic groups.
- We already have at least one bug opened for this situation (
https://fedorahosted.org/sssd/ticket/2982) and in case we decide to deal properly with this my feeling is that it will have to be done in all different parts of the code.
I understand why you're worried and I see we can hit this situation. But
we can hit this situation even without my fix. So I'd like to propose to fix this situation when someone has time to work on this and in a better way than just "don't deal with group renaming".
Yes we can hit this situation without your fix but I am curious what will be a difference between current behaviour and with this PR.
With this patch we will end up removing one of first group cached with the gid and update with the new one. Yes, as you mentioned, it's a corner case. And yes, as you said, it may bite us really hard in the future.
So, I'd like to ask for suggestions (@sbose ?, @jhrozek ?) on how to deal with this.
I wonder if a low-tech solution would help here. In case we hit this codepath, issue a really loud debug message informing that a group was renamed from X to Y and if the group was renamed on the server, it's expected, otherwise it's an error.
btw we should (unless we already do) check that requests by ID return only one result.
In case we get bitten by one of those two bugs, which one would hurt less?
Also, would be nice to see some bug reports about this (in case you have those handy, @lslebodn).
I don't remember those off-hand, but I know there were some and that's the reason we added debug messages to the NSS responder informing about ID duplicates.
Last but not least, @lslebodn suggested (in face to face conversation in the office) that maybe we could add an option which would be used for fixing the group renaming for whoever reported this bug (and this option wouldn't be enabled by default). Opinions on Lukáš' idea?
I'm not sure..it does steer towards the safe side, but on the other hand, renaming a group is a legally fine operation and I'm not sure I like an option that the admin must enable in order to proceed with an OK operation..
"""
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-279754463
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
lslebodn commented: """ On (14/02/17 08:17), Jakub Hrozek wrote:
Last but not least, @lslebodn suggested (in face to face conversation in the office) that maybe we could add an option which would be used for fixing the group renaming for whoever reported this bug (and this option wouldn't be enabled by default). Opinions on Lukáš' idea?
I'm not sure..it does steer towards the safe side, but on the other hand, renaming a group is a legally fine operation and I'm not sure I like an option that the admin must enable in order to proceed with an OK operation..
Renaming is fine. But coliding UIDs/GIDs is not rare situation. especialy if they use old clients (nss-ldap) which do not cache entries and do not care about colliding IDs.
ATM we are quite safe in case of colliding IDs The main problem is what whether this change might results in more bug reports related to issues with colliding IDs (renamed group very often). It might be difficult to identify it.
BTW IIRC this use case (colliding IDs is quite common in /etc/passwd,group)
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-279779305
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ So, as far as I remember, the conclusion about this patch is that we should also have a really loud debug message saying that the group has been renamed.
Is everyone here in agreement about this? @lslebodn, @jhrozek, @sumit-bose """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-282295811
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ Just updated the patches adding the loud debug message saying the group has been renamed. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-282480921
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ So I was thinking about this PR a bit more and I'm no longer sure we can rename groups at will. I think we can only support that if we support multiple objects with the same ID (which we currently don't and which would be a big task) or if we implement some heuristics to see that it's indeed the same, just renamed group and not a conflict.
So, I suggest that we, before renaming the group check that at least one of these conditions applies: - both objects have the same SID - both objects have the same UUID - both objects have the same originalDN
if these don't apply, then we can't know if the object is the same or different and we thrown an error. What do you think? """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-321608219
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ @jhrozek, your suggestion is good.
I've updated the patchset and I'm waiting for CI's results. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-321655235
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ CI passed: http://vm-058-233.$%7Babc%7D/logs/job/73/09/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-321674948
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ @lslebodn: patchset has been updated. Thanks for your suggestions! """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-324004045
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ So, I've just made a simple test after talking to @jhrozek on #sssd.
Here's the sssd.conf of my IPA client: ``` [root@clnt x86_64]# cat /etc/sssd/sssd.conf [domain/freeipa.ff]
cache_credentials = True krb5_store_password_if_offline = True ipa_domain = freeipa.ff id_provider = ipa auth_provider = ipa access_provider = ipa ipa_hostname = clnt.freeipa.ff chpass_provider = ipa ipa_server = _srv_, mstr.freeipa.ff ldap_tls_cacert = /etc/ipa/ca.crt debug_level = 10
[sssd] services = nss, sudo, pam, ssh domains = freeipa.ff services_startup_timeout = 90 debug_level = 10
[nss] homedir_substring = /home debug_level = 10
[pam] debug_level = 10
[sudo] debug_level = 10
[autofs]
[ssh] debug_level = 10
[pac]
[ifp]
[secrets] ```
Then ... - Before my patches: - Client: ``` [root@clnt x86_64]# id tuser00 uid=289200001(tuser00) gid=289200001(tuser00) groups=289200001(tuser00),289200003(oldname) ``` - Server: ``` [root@mstr ~]# ipa group-mod --rename newname oldname ------------------------ Modified group "oldname" ------------------------ Group name: newname GID: 289200003 Member users: tuser00 ``` - Client: ``` [root@clnt x86_64]# sss_cache -E [root@clnt x86_64]# id tuser00 uid=289200001(tuser00) gid=289200001(tuser00) groups=289200001(tuser00),289200003 ``` - `systemctl stop sssd; rm -rf /var/lib/sss/db/*; systemctl start sssd` - After my patches: - Client: ``` [root@clnt x86_64]# id tuser00 uid=289200001(tuser00) gid=289200001(tuser00) groups=289200001(tuser00),289200003(newname) ``` - Server: ``` [root@mstr ~]# ipa group-mod --rename oldname newname ------------------------ Modified group "newname" ------------------------ Group name: oldname GID: 289200003 Member users: tuser00 ``` - Client: ``` [root@clnt x86_64]# sss_cache -E [root@clnt x86_64]# id tuser00 uid=289200001(tuser00) gid=289200001(tuser00) groups=289200001(tuser00),289200003(oldname) ``` """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-325825877
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ And I'm updating the patches as I've just realized that the link to the pagure issue is wrong. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-325825958
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ Squashed your patch into mines and updated the PR. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-329428257
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ So hopefully we can finally close this PR after almost a year because there is also an integration test here: https://github.com/jhrozek/sssd/tree/group_rename
I submitted two CI runs, one with your patch in the tree (that run should succeed) and one without your patches (to make sure the tests fail). I will report back when the tests finish, but since I'm about to finish my today's "shift", this might happen in the evening. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-332160281
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ tests added with https://github.com/SSSD/sssd/pull/394 """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-332300491
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
lslebodn commented: """ BTW I am not sure whether fix is sufficient. base on comment https://github.com/SSSD/sssd/pull/128#issuecomment-325825877 "sss_cache" was called which invalidates entries + memory cache
But there were some bugs where people noticed "corrupted" memory cache after renaming groups. And I doubt they called `sss_cache` for invalidating entries.
It would be good to check that use-case as well. I will check myself on Friday if nobody else has a time. Removing accepting label before such investigation.
"""
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-333634431
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ hmm, perhaps we could also forcibly drop the contents of memory cache like we do with the back end to nss back channel when the groups are renamed? Although with the renaming we should not end up with duplicate entries anymore... """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-333644305
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ Just for the record, the issues mentioned by @lslebodn were reported recently and I really would prefer to treat them as not part of this PR (if necessary).
To be completely fair, this PR is so old and each interaction takes so long that I have to re-test, re-prepare environments, re-do everything related to it every few months as I don't even remember the context of this. :-/ """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-333658737
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
lslebodn commented: """
Just for the record, the issues mentioned by @lslebodn were reported recently and I really would prefer to treat them as not part of this PR (if necessary).
That is wrong preference because after renaming groups there might be two different entries in memory cache with the same UID/GID but with different name. And that is a problem. And must be fixed as part of https://fedorahosted.org/sssd/ticket/3282.
hmm, perhaps we could also forcibly drop the contents of memory cache like we do with the back end to nss back channel when the groups are renamed?
Did you mean with the back end to nss back channel when authentication change initgroups?
Although with the renaming we should not end up with duplicate entries anymore...
Are you sure? Because sysdb will not contain duplicate entries but memory cache might have. Unless you will be lucky and 1st entry before renaming is already expired in memory cache.
"""
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-334148621
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ On Wed, Oct 4, 2017 at 3:05 PM, lslebodn notifications@github.com wrote:
Just for the record, the issues mentioned by @lslebodn https://github.com/lslebodn were reported recently and I really would prefer to treat them as not part of this PR (if necessary).
That is wrong preference because after renaming groups there might be two different entries in memory cache with the same UID/GID but with different name. And that is a problem. And must be fixed as part of https://fedorahosted.org/sssd/ticket/3282.
Okay, then. But differently from the other times I'll wait a few weeks before touching this PR again so if something else comes to someone mind I'll do everything at once (and not split in a several requests over a year).
hmm, perhaps we could also forcibly drop the contents of memory cache like we do with the back end to nss back channel when the groups are renamed?
Did you mean with the back end to nss back channel when authentication change initgroups?
Although with the renaming we should not end up with duplicate entries anymore...
Are you sure? Because sysdb will not contain duplicate entries but memory cache might have. Unless you will be lucky and 1st entry before renaming is already expired in memory cache.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/128#issuecomment-334148621, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4epAy6y99FvuEtpWbdhLTP6lOJZAZks5so4K2gaJpZM4LnEr8 .
"""
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-334151306
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ @lslebodn, what's the result of your check done last Friday? """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-335043746
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ ping? """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-336364642
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ I'm removing the "Changes Requested" label from this PR as it's waiting from an action from the reviewers. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-338021773
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
mc-meta commented: """ Hello,
we are heavily hit by this bug. Is there any forecast on when the change will be released?
Thanks,
-m """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-362623867
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ There was a recent confusion about this PR. I thought that this should actually have been marked as changes requested to solve @lslebodn 's comment from Oct 2 that mentioned expiring the entries from the memory cache. Other than that, this PR looks OK to me and there is even a test in a different PR.
"""
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-364146093
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ Okay, as far as I remember we didn't even have an agreement that we should expire the en tries from the memory cache and since then I've been waiting for the tests on this area (thus I've sent the several pings about this).
Okay. so I'm assuming then that we do have to expire the entries from the memory cache and I'm adding the "Changes Requested" label so I can work on this bug as the time allows. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-364153819
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ Hmm, which tests in particular would you like to see? Perhaps I could help sometimes during next week to write a test that would help you verify something?
(I'm sorry if this was already said earlier in this PR, but there is so many comments in the PR already it's easy to lose track) """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-364223813
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ So, a comment from Oct 2nd says: "Although with the renaming we should not end up with duplicate entries anymore..." and then I've been waiting to know whether I should stay with the current version of the patches or I should go and do some changes related to the comment.
The PR has been opened for so long that I don't even remember what I tried to solved neither how ... But from the comments I'm guessing that just testing what I've mentioned above would be good to decide whether some changes are needed or not (and if they are, have some tests to also cover those issues).
I'll be off for the next days and I'll work on this PR at some point next week (most likely by the end of the next week) ... if you have the time to check this, I'll pay you a beer! :-) """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-364235359
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ Okay, I've been playing with the suggestion but I'm kind stuck with one problem here: how to invalidate the negative cache from the sysdb_ops.c?
@pbrezina has suggested to just fire a d-bus call to nss responder and do it from there, which is a quite good option (and I have it partially implemented) but in order to communicate with the client, well, I need access to the struct dp_client ... which doesn't seem something easy to have from the sysdb_ops.
@sumit-bose suggested to add a new return error (something like SUCCESS_BUT_RENAME_DETECED) and treat it way up in the stack and then update the memory cache.
Both those ideas are quite intrusive and the second, somehow, seems more error-prone than the other.
There's also the possibility to start passing down the id_ctx (which has a pointer to be_ctx, which has a pointer to data_provider, from where I can get the struct dp_client) ... so we can just fire the d-bus call in the right place ... but that's also something that is quite ugly to do.
So, I'm really open for suggestions here ... """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-366279844
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ @jhrozek: ^ """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-366279919
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
sumit-bose commented: """ @fidencio, you cannot do this on the sysdb level, that's the wrong layer. sysdb_add_incomplete_group() is called in only two places. So I would suggest instead to deleting the old entry in sysdb_add_incomplete_group() to return a specific error code so that the caller knows about the rename, remove the old group on its own, call sysdb_add_incomplete_group() again to add the new entry.
The two callers call sysdb_add_incomplete_group() in a loop. So just returning an error code here to notify the next layer would not work. But since it is already in the general LDAP code if might be easier to pass down the needed contexts to make the dbus calls. As an alternative the callers can return a list of objects which have to be deleted from the memory cache so that some upper layers can handle them. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-366324379
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ I agree with @sumit-bose that sysdb should not be doing this. The return code is much cleaner option. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-366500056
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ Okay, I'll work on Sumit's suggestion.
Please, just mind that while I (theoretically) do have access* to the struct data_provider from the sdap_ad_save_group_membership_with_idmapping() (which is one of the callers of sysdb_add_incomplete_group()), I don't from sdap_add_incomplete_group() (which is the other caller) and passing struct data_provider down there seems quite intrusive ... on the other hand, there's nothing else we can do differently here.
*: I do have access from idmap_ctx->id_ctx->be_ctx->provider ... which is a quite ugly and indirect way to access it.
Not related to this series, but we should start adding some new methods to give a clear idea about what we should or should not do from specific layers ... IOW ... "Does it have a method to get this structure? go for it!" ... "Do I have to be accessing stuff in a really indirect way? ... Then you most likely are doing something wrong" ...
I'll re-updated the patches soon.
@sumit-bose, @jhrozek, thanks for the input! """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-366616192
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ Patch set has been updated. It already includes the tests provided on #394. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-366779085
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ I also would like to ask for help in this one as I didn't find an easy way to test it without having to call "sss_cache" to invalidate the entries. The reason for that is because we add entries to the LDAP server in a way that it doesn't trigger the update on SSSD code (and here I'm stuck on how to trigger the update on SSSD code as it's now done from sdap_handle_id_collision_for_incomplete_groups()) """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-373748289
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ @jhrozek, @pbrezina, @sumit-bose, @mzidek-rh ... Please, could someone take a look on https://github.com/SSSD/sssd/pull/128#issuecomment-373748289 ? """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-378175540
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ So I don't like passing the provider from everywhere. But because this PR was going on for so long, I actually prepared alternative version where the data provider is part of sdap_options and passed this way. Can you check my branch called "group_rename" ? This was we could get rid of all the "SDAP: Pass struct data_provider to XYZ" patches. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-379712704
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ Your version seems okay, please, force-push it to my branch.
Just a note: Although I do appreciate you took some time to come up with a better solution, for the next time, if it's possible, I'd strongly prefer if you could give me your suggestion and then I'd rework the patch ... that would help get a better understanding of the parts that I'm still lacking knowledge in the project. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-379716865
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ OK, I'll prettify the patches and force push.
About just nacking the patch, I was both not sure if'd you'd appreciate 10th time someone tells you to rework the PR and at the same time, I wasn't sure if the approach is viable.. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-379721083
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ ...viable until I actually sat down and coded the approach myself. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-379721155
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ Hmm, I'm getting permission rejected from pushing to your repository, any chance you can just fetch my branch and push into yours? """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-379741663
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ I've squashed your patches into mines and updated the PR.
My ask for help still stand: https://github.com/SSSD/sssd/pull/128#issuecomment-373748289 and on https://github.com/SSSD/sssd/pull/128#issuecomment-378175540 """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-379761968
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ Have you considered changing the ldb expiration timestamps with pyldb or shelling out to ldbmodify? """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-379779211
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """
Have you considered changing the ldb expiration timestamps with pyldb or shelling out to ldbmodify?
I don't understand how it would help, Jakub. How doing changing the ldb expiration timestamps with pyldb would trigger sdap_handle_id_collision_for_incomplete_groups()? """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-379782555
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ Maybe I don't understand the problem. What is it you're trying to test, the memcache invalidation? """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-379789250
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ I really failed to understand how to write a test without calling `sss_cache` to invalidate the cache as done in https://github.com/SSSD/sssd/pull/128/files#diff-cb96335bb2dbee3face9f5ba04f... ... That was @lslebodn's suggestion.
When I reworked the patches I didn't find an easy wai to have sdap_handle_id_collision_for_incomplete_groups() triggered (sorry, I can't give you more details from the top of my head) and the test was not passing when removing the `sss_cache` commands.
So, seems that there are a lot of things to be understood in this part ... but that I'd really need someone's help to do so. If you have the time and the willing to do this, please, let me know. We can do this over IRC ... just gimme an "one day head's up" so I could try to remember the last state of the patch set and then we can talk. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-379791988
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ I would suggest sometimes this week because currently I do have the patch in my head :) """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-379794535
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ Here's the CI results for the current version of this patch set: http://vm-031.$%7Babc%7D/logs/job/87/73/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-379805537
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ I pushed the patch that detects the group rename vs. GID duplicate to https://github.com/jhrozek/sssd/tree/group_renaming
The rdn_same test now fails when I revert your patches. Thank you for spotting the test was not hitting the codepath at all.
I haven't ran any downstream tests yet, just the integration tests and unit tests. But I would appreciate code review. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-381725671
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ @jhrozek, I've updated the branch. Your tests are solid now. I've added "Reviewed-by: Fabiano Fidêncio ..." in your patches.
Now we're just waiting for the results of downstream tests to be sure that the latest patch in this series is not going to break something else. """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-381908618
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
jhrozek commented: """ I'm sorry about the delay. I had to spend some time to triage the downstream tests, because they were failing and at the same time, our test engine internally at red hat had some scheduling issues.
Nonetheless, the tests seem to be passing -- the most relevant tests are LDAP provider (job ID 2432961) and the AD provider test (job ID 2441955), in both tests there were either some known failures or intermittent failures that were gone during a manual re-run.
ACK """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-383898362
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/128 Title: #128: Fix group renaming issue when "id_provider = ldap" is set
fidencio commented: """ master: 851d312 709c42f ccd349f d2633d9 a537df2 a2e743c 514b2be 35d6fb7 ba2d5f7 """
See the full comment at https://github.com/SSSD/sssd/pull/128#issuecomment-384166569
URL: https://github.com/SSSD/sssd/pull/128 Author: fidencio Title: #128: Fix group renaming issue when "id_provider = ldap" is set Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/128/head:pr128 git checkout pr128
sssd-devel@lists.fedorahosted.org
-
fidencio -
jhrozek
-
lslebodn
-
mc-meta
-
sumit-bose