Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a hybrid mode of generating private groups #650

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
2 participants
@jhrozek
Copy link
Contributor

commented Sep 10, 2018

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

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

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);
     ^

@jhrozek jhrozek force-pushed the jhrozek:hybrid_master branch from f5b1f85 to 4ae173d Sep 11, 2018

@@ -576,6 +576,8 @@ bool sss_domain_info_get_output_fqnames(struct sss_domain_info *domain);

bool sss_domain_is_mpg(struct sss_domain_info *domain);

enum sss_domain_mpg_mode get_domain_mpg_mode(struct sss_domain_info *domain);

This comment has been minimized.

Copy link
@jhrozek

jhrozek Sep 11, 2018

Author Contributor

There is a getter but no setter. Since the mpg mode is only set in confdb or low-level sysdb_domain code, I didn't think we need one, but if the reviewer feels like we should also have a setter to have the interface consistent and support moving to an opaque struct sss_domain_info, I'm fine with that.

mpg_mode = MPG_ENABLED;
} else {
DEBUG(SSSDBG_MINOR_FAILURE,
"Invalid value for %s\n, assuming disabled",
SYSDB_SUBDOMAIN_MPG);

This comment has been minimized.

Copy link
@jhrozek

jhrozek Sep 11, 2018

Author Contributor

There is a bit of inconsistency between reading the mpg_mode here and earlier in src/tests/sysdb-tests.c:952. Here we assume no MPG on error, earlier we fail with unknown MPG value. So far this is in line with how the code behaved earlier, but also if the reviewer feels like this should be harmonized, I'm OK with that.

ret = EINVAL;
goto done;
}
domain->mpg_mode = str_to_domain_mpg_mode(tmp);

This comment has been minimized.

Copy link
@jhrozek

jhrozek Sep 11, 2018

Author Contributor

Here we change the behaviour of the code in the sense that an uknown value gets converted to MPG_DISABLED. I felt like I should point this out in the review.

@@ -975,7 +977,64 @@ int sysdb_getgrnam(TALLOC_CTX *mem_ctx,
goto done;
}

if (sss_domain_is_mpg(domain)) {
switch (mpg_mode) {

This comment has been minimized.

Copy link
@jhrozek

jhrozek Sep 11, 2018

Author Contributor

This whole logic has another alternative - only search for groups or users with no original GID in a single LDB search. But because the original GID attribute in sysdb is not indexed, I wasn't sure if it's worth the sysdb upgrade. Another talking point for the review..

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

I tried to point out places where I'm unsure about something so that the reviewer has hopefully a little easier job.

@jhrozek jhrozek force-pushed the jhrozek:hybrid_master branch from 4ae173d to 2080222 Sep 17, 2018

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

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.

@mzidek-rh

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2018

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.

@mzidek-rh mzidek-rh self-assigned this Sep 18, 2018

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

@mzidek-rh please also review https://pagure.io/SSSD/docs/pull-request/72 along with this PR. Thank you!

@mzidek-rh

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2018

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 {
-- 
2.9.5


From e3bc16f7443c4eecd1873f3c5ae8a09db6b8b947 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek@redhat.com>
Date: Tue, 2 Oct 2018 12:53:02 +0200
Subject: [PATCH 5/8] Fixup with: CONFDB: Read auto_private_groups as string,
 not bool

---
 src/db/sysdb_subdomains.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/db/sysdb_subdomains.c b/src/db/sysdb_subdomains.c
index cb9be3f..6083e08 100644
--- a/src/db/sysdb_subdomains.c
+++ b/src/db/sysdb_subdomains.c
@@ -1003,6 +1003,7 @@ errno_t sysdb_subdomain_store(struct sysdb_ctx *sysdb,
         tmp_str = ldb_msg_find_attr_as_string(res->msgs[0],
                                               SYSDB_SUBDOMAIN_MPG,
                                               "false");
+        /* If mpg_mode changed we need to replace the old  value in sysdb */
         switch (mpg_mode) {
         case MPG_ENABLED:
             if (strcasecmp(tmp_str, "true") != 0) {
-- 
2.9.5


@jhrozek jhrozek force-pushed the jhrozek:hybrid_master branch 2 times, most recently from 65360fe to 4fceb7f Oct 3, 2018

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2018

Thank you, the fixups were squashed

@mzidek-rh

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2018

I pushed the rebased patches to CI.

@mzidek-rh

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

The CI takes incredibly long to finish on fedora rawhide but other platforms already passed.

ACK.

@mzidek-rh mzidek-rh added the Accepted label Oct 4, 2018

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

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.

@jhrozek jhrozek added the Blocked label Oct 8, 2018

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2018

I'm adding blocked just to remind me that this shouldn't be pushed until we get a confirmation from the customer.

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

Welp, it's a good thing we haven't pushed the patches yet. They don't do what the customer wants.

@jhrozek jhrozek force-pushed the jhrozek:hybrid_master branch from 7bb8a44 to 027feb3 Jan 20, 2019

@mzidek-rh

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

Sorry, I lost track a bit with these patches. This is still not the final patch set right?

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

No, I hope to have the final set of the patches very soon now

jhrozek added some commits Aug 21, 2018

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

@jhrozek jhrozek force-pushed the jhrozek:hybrid_master branch from 027feb3 to 3173da0 Mar 10, 2019

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

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

@mzidek-rh

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

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.

@jhrozek jhrozek force-pushed the jhrozek:hybrid_master branch from 3173da0 to a00ca25 Mar 13, 2019

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

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.

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

retest this please

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

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

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

finally I amended the design page with https://pagure.io/SSSD/docs/pull-request/79

jhrozek added some commits Mar 9, 2019

CONFDB/SYSDB: 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
CACHE_REQ: Add cache_req_data_get_type()
Adds a utility function which returns the lookup type stored in struct
cache_req_data. This will be used later to switch between different
lookups as appropriate.

Related:
https://pagure.io/SSSD/sssd/issue/3822
NSS: Add the hybrid-MPG mode
Implements the functionality of the hybrid private group mapping.
Uncharacteristically, all the functionality is implemented in the
responder only.

This is because this hybrid mode must not shadow real groups with
autogenerated ones, not even if the real group comes from another
domain. Therefore, the user or group resolution must really call the full
cache_req requests.

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

@jhrozek jhrozek force-pushed the jhrozek:hybrid_master branch from a00ca25 to dab8e42 Mar 13, 2019

@mzidek-rh

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

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.

@mzidek-rh

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

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.

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

The user who requested the feature confirmed that it works for them

@mzidek-rh

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

ACK.

@mzidek-rh mzidek-rh added the Accepted label Mar 20, 2019

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

retest this please

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

I want at least centos CI to be green before pushing..

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

master;
93007c4
2ea3809
2efc41c
db03a19
fae57db
7c83450
3754780
sssd-1-16:
64b855d
c083df0
271544b
e09dffe
e0c34a6
e01473a
15f0177

@jhrozek jhrozek closed this Mar 20, 2019

@jhrozek jhrozek added the Pushed label Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.