URL: https://github.com/SSSD/sssd/pull/650 Author: jhrozek Title: #650: Implement a hybrid mode of generating private groups Action: opened
PR body: """ Related: https://pagure.io/SSSD/sssd/issue/3822
Design page PR: https://pagure.io/SSSD/docs/pull-request/72
Commit mesages follow, hopefully they are enough to explain what is going on.
SYSDB: Special case getgrnam and getgrgid searches in hybrid MPG mode
In hybrid MPG mode, we want to return the MPG group only in case the user entry has no original GID set. To achieve this, we first search with the non-MPG filter to find 'real' groups. If that fails, we try the MPG filter, but throw away entries that has any real GID set.
Related: https://pagure.io/SSSD/sssd/issue/3822
SYSDB: Refactor the mpg and non-mpg searches out of sysdb_getgrnam() and sysdb_getgrgid() to make them more reusable
The getgrnam and getgrgid searches already special-case lookups with overrides where in some cases the search falls back no a non-MPG search. The upcoming special case for the hybrid mode would do something similar, just in the opposite direction, so it makes sense to split out the functions for just the MPG step and just the non-MPG step into reusable functions.
Related: https://pagure.io/SSSD/sssd/issue/3822
CONFDB/NSS: Add the hybrid MPG mode
Permits a new option value 'hybrid' for the auto_private_groups option. The option was even previously marked as a string option in both the configAPI and the man pages, so we don't have to change the type now.
If the hybrid mode is selected and the user's original GID number is available, then during initgroups and getpwnam, it is used as their primary GID instead of the MPG group. The original group is also not added as a secondary group during initgroups in this case.
Related: https://pagure.io/SSSD/sssd/issue/3822
CONFDB: Read auto_private_groups as string, not bool
In preparation to adding the third value of auto_private_groups, this patch reads the confdb value as string and checks for the option values on its own.
Related: https://pagure.io/SSSD/sssd/issue/3822
UTIL: Convert bool mpg to an enum mpg_mode
Instead of bool mpg inside struct sss_domain_info, let's introduce enum mpg_mode that currently maps pretty much 1:1 to the boolean. In future patches, a third value will be added.
Also adds a getter for the mpg_mode value because we want to discourage getting or setting the value directly. Instead, the sss_domain_info structure should be opaque in the future.
Related: https://pagure.io/SSSD/sssd/issue/3822
UTIL: Add a is_domain_mpg shorthand
Instead of looking into the domain structure directly, add a sss_domain_is_mpg() function. This will make sense when we add a third state instead of the boolean that will also be mpg-like.
Related: https://pagure.io/SSSD/sssd/issue/3822 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/650/head:pr650 git checkout pr650
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ CI failed because older libcheck versions don't support one check macro: ``` /var/lib/jenkins/workspace/ci/label/rhel7/src/tests/sysdb-tests.c: In function 'test_user_group_by_name_hybrid': /var/lib/jenkins/workspace/ci/label/rhel7/src/tests/sysdb-tests.c:1141:5: error: implicit declaration of function 'ck_assert_ptr_nonnull' [-Werror=implicit-function-declaration] ck_assert_ptr_nonnull(attrs); ^ ``` """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-420159459
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/650 Author: jhrozek Title: #650: Implement a hybrid mode of generating private groups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/650/head:pr650 git checkout pr650
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ I tried to point out places where I'm unsure about something so that the reviewer has hopefully a little easier job. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-420176530
URL: https://github.com/SSSD/sssd/pull/650 Author: jhrozek Title: #650: Implement a hybrid mode of generating private groups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/650/head:pr650 git checkout pr650
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ The internal CI now passed. Previously there was an issue with gid_t being used in place of an unsigned long long in a formatting string, which was causing strange results on some OSs. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-422313031
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
mzidek-rh commented: """ Is anybody reviewing this? If not I can start reviewing this (but I will probably get to this properly next Tuesday first). Tentatively assigning to myself, but please feel free to reassign to yourself if you already started the review or if you plan to start review sooner next week. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-422449036
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
mzidek-rh commented: """ Is anybody reviewing this? If not I can start reviewing this (but I will probably get to this properly next Tuesday first). Tentatively assigning to myself, but please feel free to reassign to yourself if you already started the review or if you plan to start review sooner than next week. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-422449036
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ @mzidek-rh please also review https://pagure.io/SSSD/docs/pull-request/72 along with this PR. Thank you! """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-425415384
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
mzidek-rh commented: """ It looks like these patches work fine I have just few nitpicks, see the following two patches that should be squashed into your patches (to second and to third patch) The first patch just fixes line > 80 chars and the second ads a comment.
Unfortunately the whole patchset needs to be rebased on top of current master due to recent changes (that is my fault, sorry for the delay :/ )
``` From 04fcd2bab86318117b6186225a7df9c9838ca4c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Tue, 2 Oct 2018 12:03:57 +0200 Subject: [PATCH 3/8] Fixup with: UTIL: Convert bool mpg to an enum mpg_mode
--- src/db/sysdb.h | 3 ++- src/db/sysdb_subdomains.c | 3 ++- src/providers/ad/ad_subdomains.c | 3 ++- src/providers/ipa/ipa_subdomains.c | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index a1d415d..9ab43c0 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -526,7 +526,8 @@ sysdb_set_site(struct sss_domain_info *dom, errno_t sysdb_subdomain_store(struct sysdb_ctx *sysdb, const char *name, const char *realm, const char *flat_name, const char *domain_id, - enum sss_domain_mpg_mode mpg_mode, bool enumerate, const char *forest, + enum sss_domain_mpg_mode mpg_mode, + bool enumerate, const char *forest, uint32_t trust_direction, struct ldb_message_element *upn_suffixes);
diff --git a/src/db/sysdb_subdomains.c b/src/db/sysdb_subdomains.c index b0f6540..dced9a9 100644 --- a/src/db/sysdb_subdomains.c +++ b/src/db/sysdb_subdomains.c @@ -900,7 +900,8 @@ done: errno_t sysdb_subdomain_store(struct sysdb_ctx *sysdb, const char *name, const char *realm, const char *flat_name, const char *domain_id, - enum sss_domain_mpg_mode mpg_mode, bool enumerate, const char *forest, + enum sss_domain_mpg_mode mpg_mode, + bool enumerate, const char *forest, uint32_t trust_direction, struct ldb_message_element *upn_suffixes) { diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c index bbd358f..5b04677 100644 --- a/src/providers/ad/ad_subdomains.c +++ b/src/providers/ad/ad_subdomains.c @@ -501,7 +501,8 @@ ad_subdom_store(struct sdap_idmap_ctx *idmap_ctx, goto done; }
- use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(idmap_ctx, name, sid_str); + use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(idmap_ctx, + name, sid_str); if (use_id_mapping == true) { mpg_mode = MPG_ENABLED; } else { diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c index 2d96572..d86ca4c 100644 --- a/src/providers/ipa/ipa_subdomains.c +++ b/src/providers/ipa/ipa_subdomains.c @@ -612,7 +612,8 @@ static errno_t ipa_subdom_store(struct sss_domain_info *parent, goto done; }
- use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(sdap_idmap_ctx, name, id); + use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(sdap_idmap_ctx, + name, id); if (use_id_mapping == true) { mpg_mode = MPG_ENABLED; } else {
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/650 Author: jhrozek Title: #650: Implement a hybrid mode of generating private groups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/650/head:pr650 git checkout pr650
URL: https://github.com/SSSD/sssd/pull/650 Author: jhrozek Title: #650: Implement a hybrid mode of generating private groups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/650/head:pr650 git checkout pr650
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ Thank you, the fixups were squashed """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-426568647
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
mzidek-rh commented: """ I pushed the rebased patches to CI.
"""
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-426594956
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
mzidek-rh commented: """ The CI takes incredibly long to finish on fedora rawhide but other platforms already passed.
ACK. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-426924653
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ Thank you for the review. I would like to wait a bit before pushing the patches to make sure the patches get tested by the customer who actually requested this feature. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-426974529
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: +Blocked
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ I'm adding blocked just to remind me that this shouldn't be pushed until we get a confirmation from the customer. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-427944473
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: -Blocked
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ Welp, it's a good thing we haven't pushed the patches yet. They don't do what the customer wants. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-428313117
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/650 Author: jhrozek Title: #650: Implement a hybrid mode of generating private groups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/650/head:pr650 git checkout pr650
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ I pushed the reworked patches. The difference is that the private group is generated if and only if: - the uid and gid values are the same - the gid number does not resolve to a real group
I also extended the tests and rewrote the man page to make it clear what the patches are about. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-431079137
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ btw @mzidek-rh I'll leave whether you want to do the code review or not up to you. I'll work on a backport next and try to verify with the customer that this iteration of the patches does what they want. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-431079475
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ oh and I need to fix the design page, too """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-431079717
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
mzidek-rh commented: """ Ok, I will postpone the full review until we have an "ack" from the cu that this actually is what their RFE was about. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-431271176
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
mzidek-rh commented: """ Hi added back the changes requested label. It is just so that it is easier for me to filter the PRs that I do not need to immediately look at. Please remove the label once you have confirmation from the cu that the change does what they want. Thanks. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-431272100
URL: https://github.com/SSSD/sssd/pull/650 Author: jhrozek Title: #650: Implement a hybrid mode of generating private groups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/650/head:pr650 git checkout pr650
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ I pushed new patches mostly to have a backup of my work. But there is yet another requirement that needs to be fullfilled, so nothing to review at the moment.. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-440619262
URL: https://github.com/SSSD/sssd/pull/650 Author: jhrozek Title: #650: Implement a hybrid mode of generating private groups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/650/head:pr650 git checkout pr650
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
mzidek-rh commented: """ Sorry, I lost track a bit with these patches. This is still not the final patch set right? """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-458914521
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ No, I hope to have the final set of the patches very soon now """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-460183171
URL: https://github.com/SSSD/sssd/pull/650 Author: jhrozek Title: #650: Implement a hybrid mode of generating private groups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/650/head:pr650 git checkout pr650
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ I pushed yet another new version of the patchset which should hopefully address another requirement in this feature which was that 'a real group must not be shadowed by an autogenerated group even if the real group comes from a different domain'.
To this end, I used Sumit's idea and just moved all the logic into the NSS responder because at that point we really need the cache_req requests to iterate over all the domains.
@mzidek-rh please review """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-471525384
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
mzidek-rh commented: """ LGTM I have just two comments.
First, please add the link to the pagure issue in all commit messages (the last two commits do not seem to follow the usual format).
Second, I think man page should be updated to specify what happens in the hybrid mode when the GID and UID differ, but no corresponding group exists. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-472325639
URL: https://github.com/SSSD/sssd/pull/650 Author: jhrozek Title: #650: Implement a hybrid mode of generating private groups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/650/head:pr650 git checkout pr650
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ Thank you for the review, I amended the man page.
As far as I could see the ticket links were always there, if you see something that is wrong, can you point it out specifically?
btw I'm waiting for a confirmation from the customer who requested this feature as well. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-472367990
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ retest this please """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-472379326
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ I don't know why CI keeps having issues with this PR, because the internal jenkins works fine: http://vm-031.$ABC/logs/job/97/58/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-472379830
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ finally I amended the design page with https://pagure.io/SSSD/docs/pull-request/79 """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-472384416
URL: https://github.com/SSSD/sssd/pull/650 Author: jhrozek Title: #650: Implement a hybrid mode of generating private groups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/650/head:pr650 git checkout pr650
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
mzidek-rh commented: """ The commit messages in the patches I looked at were different, but the code is the same. Not sure if I had older version checked out. Anyway it looks ok now.
ACK. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-472398356
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
mzidek-rh commented: """ ACK. But, maybe it will be better to wait for the response from the people that requested this feature, so that we know this really is what they want. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-472400138
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ The user who requested the feature confirmed that it works for them """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-474821509
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
mzidek-rh commented: """ ACK. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-474825043
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ retest this please """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-474827140
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """ I want at least centos CI to be green before pushing.. """
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-474827476
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
jhrozek commented: """
master; 93007c4 2ea3809 2efc41c db03a19 fae57db 7c83450 3754780 sssd-1-16: 64b855d c083df0 271544b e09dffe e0c34a6 e01473a 15f0177
"""
See the full comment at https://github.com/SSSD/sssd/pull/650#issuecomment-474896579
URL: https://github.com/SSSD/sssd/pull/650 Author: jhrozek Title: #650: Implement a hybrid mode of generating private groups Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/650/head:pr650 git checkout pr650
URL: https://github.com/SSSD/sssd/pull/650 Title: #650: Implement a hybrid mode of generating private groups
Label: +Pushed
sssd-devel@lists.fedorahosted.org
-
jhrozek -
mzidek-rh