LDAP: Allow autogenerating user-private groups #402

jhrozek · 2017-10-10T20:11:04Z

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_private_groups.rst

fidencio · 2017-10-16T07:43:04Z

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;

fidencio · 2017-10-16T07:45:05Z

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?

fidencio · 2017-10-16T07:46:01Z

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.

pbrezina · 2017-10-16T11:41:27Z

I can chime in as well.

jhrozek · 2017-10-16T18:22:49Z

@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..

jhrozek · 2017-10-16T18:56:06Z

About the downstream tests, no, but thanks for the reminder, I will run some now.

fidencio · 2017-10-17T13:34:05Z

@jhrozek, patch set looks good to me, so ACK!

I'd explicitly wait for @pbrezina's ACK and the result of downstream tests.

jhrozek · 2017-10-17T19:20:09Z

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.

fidencio · 2017-10-17T19:33:08Z

CI:
http://vm-058-233.${abc}/logs/job/79/73/summary.html

pbrezina · 2017-10-19T09:14:44Z

I have just one question -- what happens when gidNumber is present in the user entry and gidNumber == uidNumber and mpg is true?

jhrozek · 2017-10-19T09:23:03Z

Good question, let's add a test!

jhrozek · 2017-10-19T09:23:39Z

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

jhrozek · 2017-10-19T10:57:46Z

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.

jhrozek · 2017-10-19T18:54:50Z

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.

jhrozek · 2017-10-19T18:55:25Z

btw downstream tests were rescheduled with the new code and I'll post the results (job IDs) here later, when the tests finish.

jhrozek · 2017-10-20T15:58:10Z

OK, first downstream tests passed. I'll just post job IDs here:

2104793 - local provider tests
2104801 - Tests-for-Multiple-Domains

The auto_private_groups option is used to configure the domain->mpg flag which was already set automatically for subdomains, but for some time was not settable by the admin via the configuration file. The new option name, instead of the old magic_private_groups, was chosen purely because this name would hopefully be better understood by admins. The option doesn't do anything yet, it is just added to all the places a new option should be added to. Related: https://pagure.io/SSSD/sssd/issue/1872

Since this confdb definition was completely unused across the codebase, this patch just removes the definition.

This commit allows saving the users in the MPG domain in the SDAP layer. The commit contains the following changes: - abstracts the change where if the primary GID exists in the original object, it is saved instead as the SYSDB_PRIMARY_GROUP_GIDNUM attribute, which will allow the original primary GID to be exposed as a secondary group - if the primary GID does not exist, no SYSDB_PRIMARY_GROUP_GIDNUM is added. This will allow to handle LDAP objects that only contain the UID but no GID. Since this is a new use-case, a test is added later - a branch that handles the above is added to sdap_save_user() also for joined domains that set the MPG flag. Previously, only subdomains were handled. - to allow passing GID=0 to the sysdb layer, the range check is relaxed. Related: https://pagure.io/SSSD/sssd/issue/1872

If the primary group GID or the group name is requested before the user is, we need to also search the user space to save the user in the back end which then allows the responder to generate the group from the user entry. Related: https://pagure.io/SSSD/sssd/issue/1872

…r id_provider=local This commit makes the check when adding an object in a MPG domain stricter in the sense that not only same names are allowed in a MPG domain, but also the same groups are not allowed either. This commit is a backwards-incompatible change, but one that is needed, otherwise requesting the duplicate group first and then requesting the user entry would yield two object when searching by GID. In order to keep backwards-compatibility, this uniqueness is NOT enforced with id_provider=local. This constraint can be removed in the future (or the local provider can be dropped altogether)

Related: https://pagure.io/SSSD/sssd/issue/1872

jhrozek · 2017-10-21T21:05:13Z

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

jhrozek · 2017-10-21T21:06:08Z

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.

fidencio · 2017-10-24T12:54:10Z

@jhrozek, although this version looks quite fine by me, I'd still like to have an ack from @pbrezina on this one before adding the "Accepted" label.

@pbrezina, can you review this as well? (if you're too overloaded with another tasks, let me know and I'll add the "Accepted" label without your review)

pbrezina · 2017-10-25T11:25:35Z

Ack.

jhrozek · 2017-10-26T08:10:21Z

Thanks both for review. I also ran job 2110250 which tested these patches with the AD provider and it PASS-ed.

jhrozek · 2017-10-26T08:17:55Z

fidencio self-assigned this Oct 16, 2017

fidencio added the Changes requested label Oct 16, 2017

jhrozek force-pushed the mpg branch from c632815 to e66765a Compare October 16, 2017 18:22

jhrozek removed the Changes requested label Oct 16, 2017

jhrozek assigned pbrezina Oct 16, 2017

jhrozek added the Changes requested label Oct 19, 2017

jhrozek force-pushed the mpg branch from e66765a to 822932e Compare October 19, 2017 18:52

jhrozek added 6 commits October 21, 2017 20:42

CONFDB: Remove the obsolete option magic_private_groups

75465a2

Since this confdb definition was completely unused across the codebase, this patch just removes the definition.

TESTS: Add integration tests for the auto_private_groups option

a89e339

Related: https://pagure.io/SSSD/sssd/issue/1872

jhrozek force-pushed the mpg branch from 822932e to a89e339 Compare October 21, 2017 21:02

jhrozek removed the Changes requested label Oct 21, 2017

fidencio added the Accepted label Oct 25, 2017

jhrozek closed this Oct 26, 2017

jhrozek added the Pushed label Oct 26, 2017

pbrezina mentioned this pull request Apr 28, 2020

[RFE] Support User Private Groups for main domains, too pbrezina/sssd-test#1876

Closed

sssd-bot mentioned this pull request May 2, 2020

[RFE] Support User Private Groups for main domains, too #2914

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LDAP: Allow autogenerating user-private groups #402

LDAP: Allow autogenerating user-private groups #402

jhrozek commented Oct 10, 2017

fidencio commented Oct 16, 2017

fidencio commented Oct 16, 2017

fidencio commented Oct 16, 2017

pbrezina commented Oct 16, 2017

jhrozek commented Oct 16, 2017

jhrozek commented Oct 16, 2017

fidencio commented Oct 17, 2017

jhrozek commented Oct 17, 2017

fidencio commented Oct 17, 2017

pbrezina commented Oct 19, 2017

jhrozek commented Oct 19, 2017

jhrozek commented Oct 19, 2017

jhrozek commented Oct 19, 2017

jhrozek commented Oct 19, 2017

jhrozek commented Oct 19, 2017

jhrozek commented Oct 20, 2017

jhrozek commented Oct 21, 2017

jhrozek commented Oct 21, 2017

fidencio commented Oct 24, 2017

pbrezina commented Oct 25, 2017

jhrozek commented Oct 26, 2017

jhrozek commented Oct 26, 2017

LDAP: Allow autogenerating user-private groups #402

LDAP: Allow autogenerating user-private groups #402

Conversation

jhrozek commented Oct 10, 2017

fidencio commented Oct 16, 2017

fidencio commented Oct 16, 2017

fidencio commented Oct 16, 2017

pbrezina commented Oct 16, 2017

jhrozek commented Oct 16, 2017

jhrozek commented Oct 16, 2017

fidencio commented Oct 17, 2017

jhrozek commented Oct 17, 2017

fidencio commented Oct 17, 2017

pbrezina commented Oct 19, 2017

jhrozek commented Oct 19, 2017

jhrozek commented Oct 19, 2017

jhrozek commented Oct 19, 2017

jhrozek commented Oct 19, 2017

jhrozek commented Oct 19, 2017

jhrozek commented Oct 20, 2017

jhrozek commented Oct 21, 2017

jhrozek commented Oct 21, 2017

fidencio commented Oct 24, 2017

pbrezina commented Oct 25, 2017

jhrozek commented Oct 26, 2017

jhrozek commented Oct 26, 2017