URL: https://github.com/SSSD/sssd/pull/402 Author: jhrozek Title: #402: LDAP: Allow autogenerating user-private groups Action: opened
PR body: """ This PR exposes the already existing feature that generates private groups for user objects for the generic LDAP provider. Most of the work in the PR is tests and different corner cases, but there is also one commit that is technically backwards incompatible: SYSDB: Prevent users and groups ID collision in MPG domains
Without checks for the collision, if an LDAP domain contains both a user entry and a group with the same gidNumber and the group is cached first, then the user is requested and cached, at that point the responder search by ID would return two objects. Therefore we need to guard against the duplicate IDs.
Instead of the backwards incompatible change, we could also enable the check if the domain is neither subdomain nor a id_provider=local domain or we could prefer the user object if a group-by-GID search returns two results.
But both alternatives seem like quite a hack to me and at the same time I wasn't able to find a way where the change matters except for the local domain -- and in that case I think the local domain doesn't matter as a whole.
I've also sent a design page to the sssd-devel mailing list, my WIP version is at https://pagure.io/fork/jhrozek/SSSD/docs/blob/mpg/f/design_pages/auto_privat... """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/402/head:pr402 git checkout pr402
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
fidencio commented: """ A few nitpicks: - **CONFIG: Add a new option auto_private_groups**: ``` diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c index c20cb53ca..a02822481 100644 --- a/src/confdb/confdb.c +++ b/src/confdb/confdb.c @@ -938,7 +938,7 @@ static int confdb_get_domain_internal(struct confdb_ctx *cdb,
ret = get_entry_as_bool(res->msgs[0], &domain->mpg, CONFDB_DOMAIN_AUTO_UPG, 0); - if(ret != EOK) { + if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, "Invalid value for %s\n", CONFDB_DOMAIN_AUTO_UPG); goto done;
```
- **SDAP: Allow the mpg flag for the main domain**: ``` diff --git a/src/providers/ldap/sdap_async_users.c b/src/providers/ldap/sdap_async_users.c index 34c0eabb0..7338b4a15 100644 --- a/src/providers/ldap/sdap_async_users.c +++ b/src/providers/ldap/sdap_async_users.c @@ -424,7 +424,7 @@ int sdap_save_user(TALLOC_CTX *memctx, "Missing GID, won't save the %s attribute\n", SYSDB_PRIMARY_GROUP_GIDNUM);
- /* Store a the UID as GID (since we're in a MPG domain so that it doesn't + /* Store the UID as GID (since we're in a MPG domain so that it doesn't * get treated as a missing attribute and removed */ ret = sdap_replace_id(attrs, SYSDB_GIDNUM, uid);
``` - **LDAP: Turn by-GID request into by-UID request for MPG domains if needed**: ``` diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c index bd988f0dd..9f0c762e9 100644 --- a/src/providers/ldap/ldap_id.c +++ b/src/providers/ldap/ldap_id.c @@ -1165,7 +1165,6 @@ static errno_t groups_get_handle_no_group(struct tevent_req *req) return ret; } break; - case BE_FILTER_IDNUM: gid = (gid_t) strtouint32(state->filter_value, &endptr, 10); if (errno || *endptr || (state->filter_value == endptr)) { @@ -1181,14 +1180,12 @@ static errno_t groups_get_handle_no_group(struct tevent_req *req) return ret; } break; - case BE_FILTER_SECID: case BE_FILTER_UUID: /* Since it is not clear if the SID/UUID belongs to a user or a * group we have nothing to do here. */ ret = EOK; break; - case BE_FILTER_WILDCARD: /* We can't know if all groups are up-to-date, especially in * a large environment. Do not delete any records, let the @@ -1196,7 +1193,6 @@ static errno_t groups_get_handle_no_group(struct tevent_req *req) */ ret = EOK; break; - default: ret = EINVAL; break; ```
"""
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-336805797
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
fidencio commented: """ So, @jhrozek, I didn't clearly/easily understand the changes done in the tests currently avaiable. Would you mind adding some details in the commit messages why those changes were needed?
Also, did you run any downstream test with your patches in order to track down whether we may introduce a regression as we're breaking the backward-compat? """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-336806354
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
fidencio commented: """ And, last but not least, I really would feel more comfortable if we can have another reviewer working on this patch set instead of just me. """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-336806564
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
pbrezina commented: """ I can chime in as well. """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-336861685
URL: https://github.com/SSSD/sssd/pull/402 Author: jhrozek Title: #402: LDAP: Allow autogenerating user-private groups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/402/head:pr402 git checkout pr402
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
jhrozek commented: """ @fidencio thanks, I fixed the nitpicks and added some more content to the commit message about tests. Let me know if more details need clarifying.. """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-336982611
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
jhrozek commented: """ About the downstream tests, no, but thanks for the reminder, I will run some now. """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-336995356
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
fidencio commented: """ @jhrozek, patch set looks good to me, so ACK!
I'd explicitly wait for @pbrezina's ACK and the result of downstream tests. """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-337232931
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
jhrozek commented: """ btw some downstream tests failed and some didn't pick up the correct sssd version and I'm not sure why. I will investigate further. But please don't add the accepted label or push the patches before the test issues are solved. """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-337340994
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
fidencio commented: """ CI: http://vm-058-233.$%7Babc%7D/logs/job/79/73/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-337344947
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
pbrezina commented: """ I have just one question -- what happens when gidNumber is present in the user entry and gidNumber == uidNumber and mpg is true? """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-337848867
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
jhrozek commented: """ Good question, let's add a test! """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-337851092
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
jhrozek commented: """ btw I restarted all the downstram tests because a) in some tests I added the repo to the server, not the client and b) there was a mismatch between ldb used for build and for runtime, which was causing crashes """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-337851249
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
jhrozek commented: """ The case @pbrezina mentioned works fine, but I found another issue resolving the automatic groups in case the user wasn't resolved first. Additionally, some of the downstream tests are failing, so I'm setting Changes Requested until I can figure out both issues. """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-337873402
URL: https://github.com/SSSD/sssd/pull/402 Author: jhrozek Title: #402: LDAP: Allow autogenerating user-private groups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/402/head:pr402 git checkout pr402
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
jhrozek commented: """ New patches are pushed: * the UID and GID uniqueness is only enforces unless the domain had the local provider. This would prevent having to do a backwards-incompatible change. I would like to file a ticket to actually remove that check and break the compatibility, but pagure is giving me 500 server error now.. * I added a test for @pbrezina's suggestion. It turns out we've been handling this case well in the NSS initgroups code already * The request that turns by-group search into by-user search for MPG domains was called for by-GID searches only, which doesn't make sense. We should do that for all search keys. There is also a test added. """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-338003510
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
jhrozek commented: """ btw downstream tests were rescheduled with the new code and I'll post the results (job IDs) here later, when the tests finish. """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-338003698
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
jhrozek commented: """ OK, first downstream tests passed. I'll just post job IDs here: * 2104793 - local provider tests * 2104801 - Tests-for-Multiple-Domains """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-338248214
URL: https://github.com/SSSD/sssd/pull/402 Author: jhrozek Title: #402: LDAP: Allow autogenerating user-private groups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/402/head:pr402 git checkout pr402
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
jhrozek commented: """ OK, so one of the test failures was a bug in my patches, actually. This version passed all the tests I ran so far: * 2106158 - local provider * 2106157 - tests against openldap * 2106156 - multidomain tests * 2106155 - ldap id/auth tests """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-338432323
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
jhrozek commented: """ I will be running some more tests, but these are the ones that should cover the majority of what I've changed. Please review the patches again, thank you. """
See the full comment at https://github.com/SSSD/sssd/pull/402#issuecomment-338432384
URL: https://github.com/SSSD/sssd/pull/402 Title: #402: LDAP: Allow autogenerating user-private groups
Label: -Changes requested
sssd-devel@lists.fedorahosted.org
-
fidencio -
jhrozek
-
pbrezina