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
From bd7a5b9531937b58c2b739d4d3b4f114aaf09b7f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Tue, 21 Aug 2018 16:00:34 +0200 Subject: [PATCH 1/6] 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 --- src/db/sysdb_ops.c | 12 ++++++------ src/db/sysdb_search.c | 6 +++--- src/providers/ad/ad_id.c | 2 +- src/providers/ad/ad_subdomains.c | 2 +- src/providers/ipa/ipa_s2n_exop.c | 4 ++-- src/providers/ipa/ipa_subdomains_id.c | 2 +- src/providers/ldap/ldap_id.c | 2 +- src/providers/ldap/sdap_async_users.c | 6 +++--- src/responder/nss/nss_protocol_sid.c | 4 +++- src/util/domain_info_utils.c | 5 +++++ src/util/util.h | 2 ++ 11 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index df0fb83c5..d0c4f35b4 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -504,7 +504,7 @@ static int sysdb_search_by_name(TALLOC_CTX *mem_ctx, break; case SYSDB_GROUP: def_attrs[1] = SYSDB_GIDNUM; - if (domain->mpg && + if (sss_domain_is_mpg(domain) && (!local_provider_is_built() || strcasecmp(domain->provider, "local") != 0)) { /* When searching a group by name in a MPG domain, we also @@ -1984,7 +1984,7 @@ int sysdb_add_user(struct sss_domain_info *domain, int ret; bool posix; - if (domain->mpg) { + if (sss_domain_is_mpg(domain)) { if (gid != 0) { DEBUG(SSSDBG_FATAL_FAILURE, "Cannot add user with arbitrary GID in MPG domain!\n"); @@ -2021,7 +2021,7 @@ int sysdb_add_user(struct sss_domain_info *domain, return ret; } - if (domain->mpg) { + if (sss_domain_is_mpg(domain)) { /* In MPG domains you can't have groups with the same name or GID * as users, search if a group with the same name exists. * Don't worry about users, if we try to add a user with the same @@ -2106,7 +2106,7 @@ int sysdb_add_user(struct sss_domain_info *domain, ret = sysdb_attrs_add_uint32(id_attrs, SYSDB_UIDNUM, id); if (ret) goto done; - if (domain->mpg) { + if (sss_domain_is_mpg(domain)) { ret = sysdb_attrs_add_uint32(id_attrs, SYSDB_GIDNUM, id); if (ret) goto done; } @@ -2240,7 +2240,7 @@ int sysdb_add_group(struct sss_domain_info *domain, return ret; } - if (domain->mpg) { + if (sss_domain_is_mpg(domain)) { /* In MPG domains you can't have groups with the same name as users, * search if a group with the same name exists. * Don't worry about users, if we try to add a user with the same @@ -2857,7 +2857,7 @@ static errno_t sysdb_store_user_attrs(struct sss_domain_info *domain, if (ret) return ret; } - if (uid && !gid && domain->mpg) { + if (uid && !gid && sss_domain_is_mpg(domain)) { ret = sysdb_attrs_add_uint32(attrs, SYSDB_GIDNUM, uid); if (ret) return ret; } diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 43341d446..f0918bf9a 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -909,7 +909,7 @@ int sysdb_getgrnam(TALLOC_CTX *mem_ctx, goto done; } - if (domain->mpg) { + if (sss_domain_is_mpg(domain)) { /* In case the domain supports magic private groups we *must* * check whether the searched name is the very same as the * originalADname attribute. @@ -1108,7 +1108,7 @@ int sysdb_getgrgid_attrs(TALLOC_CTX *mem_ctx, } } - if (domain->mpg) { + if (sss_domain_is_mpg(domain)) { /* In case the domain supports magic private groups we *must* * check whether the searched gid is the very same as the * originalADgidNumber attribute. @@ -1216,7 +1216,7 @@ int sysdb_enumgrent_filter(TALLOC_CTX *mem_ctx, return ENOMEM; } - if (domain->mpg) { + if (sss_domain_is_mpg(domain)) { base_filter = SYSDB_GRENT_MPG_FILTER; base_dn = sysdb_domain_dn(tmp_ctx, domain); } else { diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c index 1da48433e..c3bda1662 100644 --- a/src/providers/ad/ad_id.c +++ b/src/providers/ad/ad_id.c @@ -1100,7 +1100,7 @@ ad_get_account_domain_send(TALLOC_CTX *mem_ctx, state->attrs[0] = "objectclass"; state->attrs[1] = NULL; - if (params->be_ctx->domain->mpg == true + if (sss_domain_is_mpg(params->be_ctx->domain) == true || state->entry_type == BE_REQ_USER_AND_GROUP) { state->twopass = true; if (state->entry_type == BE_REQ_USER_AND_GROUP) { diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c index 549c2c1f7..cde8d8db1 100644 --- a/src/providers/ad/ad_subdomains.c +++ b/src/providers/ad/ad_subdomains.c @@ -506,7 +506,7 @@ ad_subdom_store(struct sdap_idmap_ctx *idmap_ctx, * inherit the MPG setting from the parent domain so that the * auto_private_groups options works for trusted domains as well */ - mpg = domain->mpg; + mpg = sss_domain_is_mpg(domain); } ret = sysdb_subdomain_store(domain->sysdb, name, realm, flat, sid_str, diff --git a/src/providers/ipa/ipa_s2n_exop.c b/src/providers/ipa/ipa_s2n_exop.c index 6f3974637..0b1b9a045 100644 --- a/src/providers/ipa/ipa_s2n_exop.c +++ b/src/providers/ipa/ipa_s2n_exop.c @@ -2352,7 +2352,7 @@ static errno_t ipa_s2n_save_objects(struct sss_domain_info *dom, } gid = 0; - if (dom->mpg == false) { + if (sss_domain_is_mpg(dom) == false) { gid = attrs->a.user.pw_gid; } else { /* The extdom plugin always returns the objects with the @@ -2423,7 +2423,7 @@ static errno_t ipa_s2n_save_objects(struct sss_domain_info *dom, missing[0] == NULL ? NULL : discard_const(missing), dom->user_timeout, now); - if (ret == EEXIST && dom->mpg == true) { + if (ret == EEXIST && sss_domain_is_mpg(dom) == true) { /* This handles the case where getgrgid() was called for * this user, so a group was created in the cache */ diff --git a/src/providers/ipa/ipa_subdomains_id.c b/src/providers/ipa/ipa_subdomains_id.c index a16eed284..3e79cd5fd 100644 --- a/src/providers/ipa/ipa_subdomains_id.c +++ b/src/providers/ipa/ipa_subdomains_id.c @@ -979,7 +979,7 @@ apply_subdomain_homedir(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom, for (c = 0; c < msg_el->num_values; c++) { if (strncmp(SYSDB_USER_CLASS, (const char *)msg_el->values[c].data, msg_el->values[c].length) == 0 - || (dom->mpg + || (sss_domain_is_mpg(dom) && strncmp(SYSDB_GROUP_CLASS, (const char *)msg_el->values[c].data, msg_el->values[c].length) == 0)) { diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c index 9e8289904..c37048258 100644 --- a/src/providers/ldap/ldap_id.c +++ b/src/providers/ldap/ldap_id.c @@ -955,7 +955,7 @@ static void groups_get_done(struct tevent_req *subreq) } if (ret == ENOENT - && state->domain->mpg == true + && sss_domain_is_mpg(state->domain) == true && !state->conn->no_mpg_user_fallback) { /* The requested filter did not find a group. Before giving up, we must * also check if the GID can be resolved through a primary group of a diff --git a/src/providers/ldap/sdap_async_users.c b/src/providers/ldap/sdap_async_users.c index 5ffba7770..92eeda1d3 100644 --- a/src/providers/ldap/sdap_async_users.c +++ b/src/providers/ldap/sdap_async_users.c @@ -389,7 +389,7 @@ int sdap_save_user(TALLOC_CTX *memctx, goto done; } - if (IS_SUBDOMAIN(dom) || dom->mpg == true) { + if (IS_SUBDOMAIN(dom) || sss_domain_is_mpg(dom) == true) { /* For subdomain users, only create the private group as * the subdomain is an MPG domain. * But we have to save the GID of the original primary group @@ -411,7 +411,7 @@ int sdap_save_user(TALLOC_CTX *memctx, */ ret = sysdb_attrs_add_uint32(attrs, SYSDB_GIDNUM, gid); if (ret != EOK) goto done; - } else if (dom->mpg) { + } else if (sss_domain_is_mpg(dom)) { /* Likewise, if a domain is set to contain 'magic private groups', do * not process the real GID, but save it in the cache as originalGID * (if available) @@ -470,7 +470,7 @@ int sdap_save_user(TALLOC_CTX *memctx, /* check that the gid is valid for this domain */ if (is_posix == true && IS_SUBDOMAIN(dom) == false - && dom->mpg == false + && sss_domain_is_mpg(dom) == false && OUT_OF_ID_RANGE(gid, dom->id_min, dom->id_max)) { DEBUG(SSSDBG_CRIT_FAILURE, "User [%s] filtered out! (primary gid out of range)\n", diff --git a/src/responder/nss/nss_protocol_sid.c b/src/responder/nss/nss_protocol_sid.c index 3f60967d7..96cb8617b 100644 --- a/src/responder/nss/nss_protocol_sid.c +++ b/src/responder/nss/nss_protocol_sid.c @@ -75,7 +75,9 @@ nss_get_id_type(struct nss_cmd_ctx *cmd_ctx, return EOK; } - ret = find_sss_id_type(result->msgs[0], result->domain->mpg, _type); + ret = find_sss_id_type(result->msgs[0], + sss_domain_is_mpg(result->domain), + _type); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "Unable to find ID type [%d]: %s\n", ret, sss_strerror(ret)); diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c index 8bef6c9af..4a8089350 100644 --- a/src/util/domain_info_utils.c +++ b/src/util/domain_info_utils.c @@ -934,3 +934,8 @@ bool is_files_provider(struct sss_domain_info *domain) return domain->provider != NULL && strcasecmp(domain->provider, "files") == 0; } + +bool sss_domain_is_mpg(struct sss_domain_info *domain) +{ + return domain->mpg; +} diff --git a/src/util/util.h b/src/util/util.h index 867acf26f..0d9eb4549 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -574,6 +574,8 @@ void sss_domain_info_set_output_fqnames(struct sss_domain_info *domain, bool sss_domain_info_get_output_fqnames(struct sss_domain_info *domain); +bool sss_domain_is_mpg(struct sss_domain_info *domain); + #define IS_SUBDOMAIN(dom) ((dom)->parent != NULL) #define DOM_HAS_VIEWS(dom) ((dom)->has_views) From 85d1ba718cdf0f61e495ecfac92d3a56b7c70227 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Sun, 2 Sep 2018 22:37:42 +0200 Subject: [PATCH 2/6] 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 --- src/confdb/confdb.c | 11 ++++-- src/confdb/confdb.h | 7 +++- src/db/sysdb.h | 2 +- src/db/sysdb_private.h | 2 +- src/db/sysdb_subdomains.c | 34 +++++++++++-------- src/providers/ad/ad_subdomains.c | 13 ++++--- src/providers/ipa/ipa_subdomains.c | 16 +++++++-- src/tests/cmocka/test_fqnames.c | 3 +- src/tests/cmocka/test_ipa_subdomains_server.c | 4 +-- src/tests/cmocka/test_ldap_id_cleanup.c | 2 +- src/tests/cmocka/test_nss_srv.c | 2 +- src/tests/cmocka/test_responder_cache_req.c | 4 +-- src/tests/cmocka/test_sysdb_subdomains.c | 28 +++++++-------- src/tests/cmocka/test_sysdb_views.c | 6 ++-- src/tests/sysdb-tests.c | 14 ++++---- src/util/domain_info_utils.c | 7 +++- src/util/util.h | 2 ++ 17 files changed, 98 insertions(+), 59 deletions(-) diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c index 22068cacc..28ce4d026 100644 --- a/src/confdb/confdb.c +++ b/src/confdb/confdb.c @@ -873,6 +873,7 @@ static int confdb_get_domain_internal(struct confdb_ctx *cdb, bool fqnames_default = false; int memcache_timeout; bool enum_default; + bool is_mpg; tmp_ctx = talloc_new(mem_ctx); if (!tmp_ctx) return ENOMEM; @@ -937,7 +938,7 @@ static int confdb_get_domain_internal(struct confdb_ctx *cdb, goto done; } - ret = get_entry_as_bool(res->msgs[0], &domain->mpg, + ret = get_entry_as_bool(res->msgs[0], &is_mpg, CONFDB_DOMAIN_AUTO_UPG, 0); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, @@ -945,6 +946,12 @@ static int confdb_get_domain_internal(struct confdb_ctx *cdb, goto done; } + if (is_mpg) { + domain->mpg_mode = MPG_ENABLED; + } else { + domain->mpg_mode = MPG_DISABLED; + } + if (local_provider_is_built() && strcasecmp(domain->provider, "local") == 0) { /* If this is the local provider, we need to ensure that @@ -982,7 +989,7 @@ static int confdb_get_domain_internal(struct confdb_ctx *cdb, } /* The LOCAL provider use always Magic Private Groups */ - domain->mpg = true; + domain->mpg_mode = MPG_ENABLED; } domain->timeout = ldb_msg_find_attr_as_int(res->msgs[0], diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index 22665013a..a4c248669 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -299,6 +299,11 @@ enum sss_domain_type { DOM_TYPE_APPLICATION, }; +enum sss_domain_mpg_mode { + MPG_DISABLED, + MPG_ENABLED, +}; + /** * Data structure storing all of the basic features * of a domain. @@ -313,7 +318,7 @@ struct sss_domain_info { bool enumerate; char **sd_enumerate; bool fqnames; - bool mpg; + enum sss_domain_mpg_mode mpg_mode; bool ignore_group_members; uint32_t id_min; uint32_t id_max; diff --git a/src/db/sysdb.h b/src/db/sysdb.h index d72af5a05..a1d415d97 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -526,7 +526,7 @@ 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, - bool mpg, 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_private.h b/src/db/sysdb_private.h index d1f2bd40a..c297715cd 100644 --- a/src/db/sysdb_private.h +++ b/src/db/sysdb_private.h @@ -198,7 +198,7 @@ struct sss_domain_info *new_subdomain(TALLOC_CTX *mem_ctx, const char *realm, const char *flat_name, const char *id, - bool mpg, + enum sss_domain_mpg_mode mpg_mode, bool enumerate, const char *forest, const char **upn_suffixes, diff --git a/src/db/sysdb_subdomains.c b/src/db/sysdb_subdomains.c index bdd5245f2..b0f654058 100644 --- a/src/db/sysdb_subdomains.c +++ b/src/db/sysdb_subdomains.c @@ -34,7 +34,7 @@ struct sss_domain_info *new_subdomain(TALLOC_CTX *mem_ctx, const char *realm, const char *flat_name, const char *id, - bool mpg, + enum sss_domain_mpg_mode mpg_mode, bool enumerate, const char *forest, const char **upn_suffixes, @@ -126,7 +126,7 @@ struct sss_domain_info *new_subdomain(TALLOC_CTX *mem_ctx, dom->enumerate = enumerate; dom->fqnames = true; - dom->mpg = mpg; + dom->mpg_mode = mpg_mode; dom->state = DOM_ACTIVE; /* use fully qualified names as output in order to avoid causing @@ -320,7 +320,8 @@ errno_t sysdb_update_subdomains(struct sss_domain_info *domain, const char *flat; const char *id; const char *forest; - bool mpg; + bool is_mpg; + enum sss_domain_mpg_mode mpg_mode; bool enumerate; uint32_t trust_direction; struct ldb_message_element *tmp_el; @@ -376,8 +377,13 @@ errno_t sysdb_update_subdomains(struct sss_domain_info *domain, id = ldb_msg_find_attr_as_string(res->msgs[i], SYSDB_SUBDOMAIN_ID, NULL); - mpg = ldb_msg_find_attr_as_bool(res->msgs[i], - SYSDB_SUBDOMAIN_MPG, false); + is_mpg = ldb_msg_find_attr_as_bool(res->msgs[i], + SYSDB_SUBDOMAIN_MPG, false); + if (is_mpg) { + mpg_mode = MPG_ENABLED; + } else { + mpg_mode = MPG_DISABLED; + } enumerate = ldb_msg_find_attr_as_bool(res->msgs[i], SYSDB_SUBDOMAIN_ENUM, false); @@ -440,12 +446,12 @@ errno_t sysdb_update_subdomains(struct sss_domain_info *domain, } } - if (dom->mpg != mpg) { + if (dom->mpg_mode != mpg_mode) { DEBUG(SSSDBG_TRACE_INTERNAL, "MPG state change from [%s] to [%s]!\n", - dom->mpg ? "true" : "false", - mpg ? "true" : "false"); - dom->mpg = mpg; + dom->mpg_mode == MPG_ENABLED ? "true" : "false", + mpg_mode == MPG_ENABLED ? "true" : "false"); + dom->mpg_mode = mpg_mode; } if (dom->enumerate != enumerate) { @@ -515,7 +521,7 @@ errno_t sysdb_update_subdomains(struct sss_domain_info *domain, /* If not found in loop it is a new subdomain */ if (dom == NULL) { dom = new_subdomain(domain, domain, name, realm, - flat, id, mpg, enumerate, forest, + flat, id, mpg_mode, enumerate, forest, upn_suffixes, trust_direction, confdb); if (dom == NULL) { ret = ENOMEM; @@ -894,7 +900,7 @@ errno_t sysdb_master_domain_add_info(struct sss_domain_info *domain, errno_t sysdb_subdomain_store(struct sysdb_ctx *sysdb, const char *name, const char *realm, const char *flat_name, const char *domain_id, - bool mpg, 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) { @@ -985,8 +991,8 @@ errno_t sysdb_subdomain_store(struct sysdb_ctx *sysdb, } tmp_bool = ldb_msg_find_attr_as_bool(res->msgs[0], SYSDB_SUBDOMAIN_MPG, - !mpg); - if (tmp_bool != mpg) { + !(mpg_mode == MPG_ENABLED)); + if (tmp_bool != (mpg_mode == MPG_ENABLED)) { mpg_flags = LDB_FLAG_MOD_REPLACE; } tmp_bool = ldb_msg_find_attr_as_bool(res->msgs[0], SYSDB_SUBDOMAIN_ENUM, @@ -1099,7 +1105,7 @@ errno_t sysdb_subdomain_store(struct sysdb_ctx *sysdb, } ret = ldb_msg_add_string(msg, SYSDB_SUBDOMAIN_MPG, - mpg ? "TRUE" : "FALSE"); + (mpg_mode == MPG_ENABLED) ? "TRUE" : "FALSE"); if (ret != LDB_SUCCESS) { ret = sysdb_error_to_errno(ret); goto done; diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c index cde8d8db1..bbd358f67 100644 --- a/src/providers/ad/ad_subdomains.c +++ b/src/providers/ad/ad_subdomains.c @@ -451,7 +451,8 @@ ad_subdom_store(struct sdap_idmap_ctx *idmap_ctx, struct ldb_message_element *el; char *sid_str = NULL; uint32_t trust_type; - bool mpg; + bool use_id_mapping; + enum sss_domain_mpg_mode mpg_mode; tmp_ctx = talloc_new(NULL); if (tmp_ctx == NULL) { @@ -500,17 +501,19 @@ ad_subdom_store(struct sdap_idmap_ctx *idmap_ctx, goto done; } - mpg = sdap_idmap_domain_has_algorithmic_mapping(idmap_ctx, name, sid_str); - if (mpg == false) { + use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(idmap_ctx, name, sid_str); + if (use_id_mapping == true) { + mpg_mode = MPG_ENABLED; + } else { /* Domains that use the POSIX attributes set by the admin must * inherit the MPG setting from the parent domain so that the * auto_private_groups options works for trusted domains as well */ - mpg = sss_domain_is_mpg(domain); + mpg_mode = get_domain_mpg_mode(domain); } ret = sysdb_subdomain_store(domain->sysdb, name, realm, flat, sid_str, - mpg, enumerate, domain->forest, 0, NULL); + mpg_mode, enumerate, domain->forest, 0, NULL); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "sysdb_subdomain_store failed.\n"); goto done; diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c index 1b443559e..2d96572f0 100644 --- a/src/providers/ipa/ipa_subdomains.c +++ b/src/providers/ipa/ipa_subdomains.c @@ -571,7 +571,8 @@ static errno_t ipa_subdom_store(struct sss_domain_info *parent, const char *id; char *forest = NULL; int ret; - bool mpg; + bool use_id_mapping; + enum sss_domain_mpg_mode mpg_mode; bool enumerate; uint32_t direction; struct ldb_message_element *alternative_domain_suffixes = NULL; @@ -611,7 +612,16 @@ static errno_t ipa_subdom_store(struct sss_domain_info *parent, goto done; } - mpg = 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 { + /* Domains that use the POSIX attributes set by the admin must + * inherit the MPG setting from the parent domain so that the + * auto_private_groups options works for trusted domains as well + */ + mpg_mode = get_domain_mpg_mode(parent); + } ret = ipa_subdom_get_forest(tmp_ctx, sysdb_ctx_get_ldb(parent->sysdb), attrs, &forest); @@ -639,7 +649,7 @@ static errno_t ipa_subdom_store(struct sss_domain_info *parent, } ret = sysdb_subdomain_store(parent->sysdb, name, realm, flat, - id, mpg, enumerate, forest, + id, mpg_mode, enumerate, forest, direction, alternative_domain_suffixes); if (ret) { DEBUG(SSSDBG_OP_FAILURE, "sysdb_subdomain_store failed.\n"); diff --git a/src/tests/cmocka/test_fqnames.c b/src/tests/cmocka/test_fqnames.c index b6f58a771..b374be210 100644 --- a/src/tests/cmocka/test_fqnames.c +++ b/src/tests/cmocka/test_fqnames.c @@ -309,7 +309,8 @@ static int parse_name_test_setup(void **state) * discovered */ test_ctx->subdom = new_subdomain(dom, dom, SUBDOMNAME, NULL, SUBFLATNAME, - NULL, false, false, NULL, NULL, 0, NULL); + NULL, MPG_DISABLED, false, + NULL, NULL, 0, NULL); assert_non_null(test_ctx->subdom); check_leaks_push(test_ctx); diff --git a/src/tests/cmocka/test_ipa_subdomains_server.c b/src/tests/cmocka/test_ipa_subdomains_server.c index 11cec6721..e034df15b 100644 --- a/src/tests/cmocka/test_ipa_subdomains_server.c +++ b/src/tests/cmocka/test_ipa_subdomains_server.c @@ -253,14 +253,14 @@ static void add_test_subdomains(struct trust_test_ctx *test_ctx, ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, SUBDOM_NAME, SUBDOM_REALM, NULL, SUBDOM_SID, - true, false, SUBDOM_REALM, + MPG_ENABLED, false, SUBDOM_REALM, direction, NULL); assert_int_equal(ret, EOK); ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, CHILD_NAME, CHILD_REALM, CHILD_FLAT, CHILD_SID, - true, false, SUBDOM_REALM, + MPG_ENABLED, false, SUBDOM_REALM, direction, NULL); assert_int_equal(ret, EOK); diff --git a/src/tests/cmocka/test_ldap_id_cleanup.c b/src/tests/cmocka/test_ldap_id_cleanup.c index 177022e20..d8956f2c8 100644 --- a/src/tests/cmocka/test_ldap_id_cleanup.c +++ b/src/tests/cmocka/test_ldap_id_cleanup.c @@ -121,7 +121,7 @@ static int test_sysdb_setup(void **state) ret = setup_sysdb_tests(&test_ctx); assert_int_equal(ret, EOK); - test_ctx->domain->mpg = false; + test_ctx->domain->mpg_mode = MPG_DISABLED; /* set options */ test_ctx->opts = talloc_zero(test_ctx, struct sdap_options); diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c index 1f7de7bd9..0ae177571 100644 --- a/src/tests/cmocka/test_nss_srv.c +++ b/src/tests/cmocka/test_nss_srv.c @@ -3480,7 +3480,7 @@ static int nss_subdom_test_setup_common(void **state, bool nonfqnames) ret = sysdb_subdomain_store(nss_test_ctx->tctx->sysdb, testdom[0], testdom[1], testdom[2], testdom[3], - false, false, NULL, 0, NULL); + MPG_DISABLED, false, NULL, 0, NULL); assert_int_equal(ret, EOK); ret = sysdb_update_subdomains(nss_test_ctx->tctx->dom, diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c index f564c6582..b4cf7f9fe 100644 --- a/src/tests/cmocka/test_responder_cache_req.c +++ b/src/tests/cmocka/test_responder_cache_req.c @@ -686,13 +686,13 @@ int test_subdomain_setup(void **state) test_ctx->subdomain = new_subdomain(test_ctx, test_ctx->tctx->dom, testdom[0], testdom[1], testdom[2], testdom[3], - false, false, NULL, NULL, 0, + MPG_DISABLED, false, NULL, NULL, 0, test_ctx->tctx->confdb); assert_non_null(test_ctx->subdomain); ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, testdom[0], testdom[1], testdom[2], testdom[3], - false, false, NULL, 0, NULL); + MPG_DISABLED, false, NULL, 0, NULL); assert_int_equal(ret, EOK); ret = sysdb_update_subdomains(test_ctx->tctx->dom, diff --git a/src/tests/cmocka/test_sysdb_subdomains.c b/src/tests/cmocka/test_sysdb_subdomains.c index ead76bfff..d2116dc23 100644 --- a/src/tests/cmocka/test_sysdb_subdomains.c +++ b/src/tests/cmocka/test_sysdb_subdomains.c @@ -100,7 +100,7 @@ static void test_sysdb_subdomain_create(void **state) ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, dom1[0], dom1[1], dom1[2], dom1[3], - false, false, NULL, 0, NULL); + MPG_DISABLED, false, NULL, 0, NULL); assert_int_equal(ret, EOK); ret = sysdb_update_subdomains(test_ctx->tctx->dom, test_ctx->tctx->confdb); @@ -112,7 +112,7 @@ static void test_sysdb_subdomain_create(void **state) ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, dom2[0], dom2[1], dom2[2], dom2[3], - false, false, NULL, 1, NULL); + MPG_DISABLED, false, NULL, 1, NULL); assert_int_equal(ret, EOK); ret = sysdb_update_subdomains(test_ctx->tctx->dom, test_ctx->tctx->confdb); @@ -125,12 +125,12 @@ static void test_sysdb_subdomain_create(void **state) /* Reverse the trust directions */ ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, dom1[0], dom1[1], dom1[2], dom1[3], - false, false, NULL, 1, NULL); + MPG_DISABLED, false, NULL, 1, NULL); assert_int_equal(ret, EOK); ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, dom2[0], dom2[1], dom2[2], dom2[3], - false, false, NULL, 0, NULL); + MPG_DISABLED, false, NULL, 0, NULL); assert_int_equal(ret, EOK); ret = sysdb_update_subdomains(test_ctx->tctx->dom, test_ctx->tctx->confdb); @@ -212,26 +212,26 @@ static void test_sysdb_link_forest_root_ipa(void **state) ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, dom1[0], dom1[1], dom1[2], dom1[3], - false, false, dom1[4], 0, NULL); + MPG_DISABLED, false, dom1[4], 0, NULL); assert_int_equal(ret, EOK); ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, child_dom1[0], child_dom1[1], child_dom1[2], child_dom1[3], - false, false, child_dom1[4], + MPG_DISABLED, false, child_dom1[4], 0, NULL); assert_int_equal(ret, EOK); ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, dom2[0], dom2[1], dom2[2], dom2[3], - false, false, dom2[4], + MPG_DISABLED, false, dom2[4], 0, NULL); assert_int_equal(ret, EOK); ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, child_dom2[0], child_dom2[1], child_dom2[2], child_dom2[3], - false, false, child_dom2[4], + MPG_DISABLED, false, child_dom2[4], 0, NULL); assert_int_equal(ret, EOK); @@ -304,14 +304,14 @@ static void test_sysdb_link_forest_root_ad(void **state) ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, child_dom[0], child_dom[1], child_dom[2], child_dom[3], - false, false, child_dom[4], + MPG_DISABLED, false, child_dom[4], 0, NULL); assert_int_equal(ret, EOK); ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, sub_dom[0], sub_dom[1], sub_dom[2], sub_dom[3], - false, false, sub_dom[4], + MPG_DISABLED, false, sub_dom[4], 0, NULL); assert_int_equal(ret, EOK); @@ -381,14 +381,14 @@ static void test_sysdb_link_forest_member_ad(void **state) ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, sub_dom[0], sub_dom[1], sub_dom[2], sub_dom[3], - false, false, sub_dom[4], + MPG_DISABLED, false, sub_dom[4], 0, NULL); assert_int_equal(ret, EOK); ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, forest_root[0], forest_root[1], forest_root[2], forest_root[3], - false, false, forest_root[4], + MPG_DISABLED, false, forest_root[4], 0, NULL); assert_int_equal(ret, EOK); @@ -466,7 +466,7 @@ static void test_sysdb_link_ad_multidom(void **state) ret = sysdb_subdomain_store(main_dom1->sysdb, child_dom[0], child_dom[1], child_dom[2], child_dom[3], - false, false, child_dom[4], + MPG_DISABLED, false, child_dom[4], 0, NULL); assert_int_equal(ret, EOK); @@ -487,7 +487,7 @@ static void test_sysdb_link_ad_multidom(void **state) ret = sysdb_subdomain_store(main_dom2->sysdb, dom2_forest_root[0], dom2_forest_root[1], dom2_forest_root[2], dom2_forest_root[3], - false, false, dom2_forest_root[4], 0, NULL); + MPG_DISABLED, false, dom2_forest_root[4], 0, NULL); assert_int_equal(ret, EOK); ret = sysdb_master_domain_update(main_dom2); diff --git a/src/tests/cmocka/test_sysdb_views.c b/src/tests/cmocka/test_sysdb_views.c index 8ef69b271..72daa4450 100644 --- a/src/tests/cmocka/test_sysdb_views.c +++ b/src/tests/cmocka/test_sysdb_views.c @@ -158,7 +158,7 @@ static void test_sysdb_store_override(void **state) struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state, struct sysdb_test_ctx); - test_ctx->domain->mpg = false; + test_ctx->domain->mpg_mode = MPG_DISABLED; name = sss_create_internal_fqname(test_ctx, TEST_USER_NAME, test_ctx->domain->name); assert_non_null(name); @@ -388,7 +388,7 @@ void test_sysdb_delete_view_tree(void **state) struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state, struct sysdb_test_ctx); - test_ctx->domain->mpg = false; + test_ctx->domain->mpg_mode = MPG_DISABLED; ret = sysdb_update_view_name(test_ctx->domain->sysdb, TEST_VIEW_NAME); assert_int_equal(ret, EOK); @@ -455,7 +455,7 @@ void test_sysdb_invalidate_overrides(void **state) struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state, struct sysdb_test_ctx); - test_ctx->domain->mpg = false; + test_ctx->domain->mpg_mode = MPG_DISABLED; name = sss_create_internal_fqname(test_ctx, TEST_USER_NAME, test_ctx->domain->name); assert_non_null(name); diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c index 933a07e1f..214ad02a9 100644 --- a/src/tests/sysdb-tests.c +++ b/src/tests/sysdb-tests.c @@ -1095,7 +1095,7 @@ START_TEST(test_user_group_by_name) * ldap provider differently with auto_private_groups. */ test_ctx->domain->provider = discard_const_p(char, "ldap"); - test_ctx->domain->mpg = true; + test_ctx->domain->mpg_mode = MPG_ENABLED; data = test_data_new_user(test_ctx, _i); fail_if(data == NULL); @@ -1337,7 +1337,7 @@ START_TEST (test_sysdb_enumgrent) return; } - test_ctx->domain->mpg = true; + test_ctx->domain->mpg_mode = MPG_ENABLED; ret = sysdb_enumgrent(test_ctx, test_ctx->domain, @@ -1541,7 +1541,7 @@ START_TEST (test_sysdb_get_user_attr_subdomain) /* Create subdomain */ subdomain = new_subdomain(test_ctx, test_ctx->domain, "test.sub", "TEST.SUB", "test", "S-3", - false, false, NULL, NULL, 0, NULL); + MPG_DISABLED, false, NULL, NULL, 0, NULL); fail_if(subdomain == NULL, "Failed to create new subdomain."); ret = sss_names_init_from_args(test_ctx, @@ -5821,7 +5821,7 @@ START_TEST(test_sysdb_search_object_by_id) fail_unless(ret == EOK, "sysdb_add_group failed with [%d][%s].", ret, strerror(ret)); - test_ctx->domain->mpg = false; + test_ctx->domain->mpg_mode = MPG_DISABLED; ret = sysdb_add_user(test_ctx->domain, "user1", 4001, id, "User 1", "/home/user1", "/bin/bash", NULL, NULL, 0, 0); @@ -6146,7 +6146,7 @@ START_TEST(test_sysdb_subdomain_store_user) subdomain = new_subdomain(test_ctx, test_ctx->domain, testdom[0], testdom[1], testdom[2], testdom[3], - false, false, NULL, NULL, 0, NULL); + MPG_DISABLED, false, NULL, NULL, 0, NULL); fail_unless(subdomain != NULL, "Failed to create new subdomain."); ret = sysdb_subdomain_store(test_ctx->sysdb, testdom[0], testdom[1], testdom[2], testdom[3], @@ -6225,7 +6225,7 @@ START_TEST(test_sysdb_subdomain_user_ops) subdomain = new_subdomain(test_ctx, test_ctx->domain, testdom[0], testdom[1], testdom[2], testdom[3], - false, false, NULL, NULL, 0, NULL); + MPG_DISABLED, false, NULL, NULL, 0, NULL); fail_unless(subdomain != NULL, "Failed to create new subdomain."); ret = sysdb_subdomain_store(test_ctx->sysdb, testdom[0], testdom[1], testdom[2], testdom[3], @@ -6298,7 +6298,7 @@ START_TEST(test_sysdb_subdomain_group_ops) subdomain = new_subdomain(test_ctx, test_ctx->domain, testdom[0], testdom[1], testdom[2], testdom[3], - false, false, NULL, NULL, 0, NULL); + MPG_DISABLED, false, NULL, NULL, 0, NULL); fail_unless(subdomain != NULL, "Failed to create new subdomain."); ret = sysdb_subdomain_store(test_ctx->sysdb, testdom[0], testdom[1], testdom[2], testdom[3], diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c index 4a8089350..c3b1c62f6 100644 --- a/src/util/domain_info_utils.c +++ b/src/util/domain_info_utils.c @@ -937,5 +937,10 @@ bool is_files_provider(struct sss_domain_info *domain) bool sss_domain_is_mpg(struct sss_domain_info *domain) { - return domain->mpg; + return domain->mpg_mode == MPG_ENABLED; +} + +enum sss_domain_mpg_mode get_domain_mpg_mode(struct sss_domain_info *domain) +{ + return domain->mpg_mode; } diff --git a/src/util/util.h b/src/util/util.h index 0d9eb4549..b3f94bc8a 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -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); + #define IS_SUBDOMAIN(dom) ((dom)->parent != NULL) #define DOM_HAS_VIEWS(dom) ((dom)->has_views) From fc7a1cb64af6c074426c92d10d67e122e035c94c Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Sun, 2 Sep 2018 17:37:43 +0200 Subject: [PATCH 3/6] 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 --- src/confdb/confdb.c | 19 +++++----- src/db/sysdb_subdomains.c | 46 ++++++++++++++++++------ src/tests/cmocka/test_sysdb_subdomains.c | 19 ++++++++++ src/util/domain_info_utils.c | 12 +++++++ src/util/util.h | 1 + 5 files changed, 78 insertions(+), 19 deletions(-) diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c index 28ce4d026..294a36777 100644 --- a/src/confdb/confdb.c +++ b/src/confdb/confdb.c @@ -873,7 +873,6 @@ static int confdb_get_domain_internal(struct confdb_ctx *cdb, bool fqnames_default = false; int memcache_timeout; bool enum_default; - bool is_mpg; tmp_ctx = talloc_new(mem_ctx); if (!tmp_ctx) return ENOMEM; @@ -938,18 +937,20 @@ static int confdb_get_domain_internal(struct confdb_ctx *cdb, goto done; } - ret = get_entry_as_bool(res->msgs[0], &is_mpg, - CONFDB_DOMAIN_AUTO_UPG, 0); - if (ret != EOK) { - DEBUG(SSSDBG_FATAL_FAILURE, - "Invalid value for %s\n", CONFDB_DOMAIN_AUTO_UPG); - goto done; + tmp = ldb_msg_find_attr_as_string(res->msgs[0], CONFDB_DOMAIN_AUTO_UPG, NULL); + if (tmp == NULL || *tmp == '\0') { + tmp = "false"; } - if (is_mpg) { + if (strcasecmp(tmp, "FALSE") == 0) { + domain->mpg_mode = MPG_DISABLED; + } else if (strcasecmp(tmp, "TRUE") == 0) { domain->mpg_mode = MPG_ENABLED; } else { - domain->mpg_mode = MPG_DISABLED; + DEBUG(SSSDBG_FATAL_FAILURE, + "Invalid value for %s\n", CONFDB_DOMAIN_AUTO_UPG); + ret = EINVAL; + goto done; } if (local_provider_is_built() diff --git a/src/db/sysdb_subdomains.c b/src/db/sysdb_subdomains.c index b0f654058..cde91cd5d 100644 --- a/src/db/sysdb_subdomains.c +++ b/src/db/sysdb_subdomains.c @@ -320,7 +320,7 @@ errno_t sysdb_update_subdomains(struct sss_domain_info *domain, const char *flat; const char *id; const char *forest; - bool is_mpg; + const char *str_mpg_mode; enum sss_domain_mpg_mode mpg_mode; bool enumerate; uint32_t trust_direction; @@ -377,11 +377,20 @@ errno_t sysdb_update_subdomains(struct sss_domain_info *domain, id = ldb_msg_find_attr_as_string(res->msgs[i], SYSDB_SUBDOMAIN_ID, NULL); - is_mpg = ldb_msg_find_attr_as_bool(res->msgs[i], - SYSDB_SUBDOMAIN_MPG, false); - if (is_mpg) { + str_mpg_mode = ldb_msg_find_attr_as_string(res->msgs[i], + SYSDB_SUBDOMAIN_MPG, NULL); + if (str_mpg_mode == NULL || *str_mpg_mode == '\0') { + str_mpg_mode = "false"; + } + + if (strcasecmp(str_mpg_mode, "FALSE") == 0) { + mpg_mode = MPG_DISABLED; + } else if (strcasecmp(str_mpg_mode, "TRUE") == 0) { mpg_mode = MPG_ENABLED; } else { + DEBUG(SSSDBG_MINOR_FAILURE, + "Invalid value for %s\n, assuming disabled", + SYSDB_SUBDOMAIN_MPG); mpg_mode = MPG_DISABLED; } @@ -990,11 +999,22 @@ errno_t sysdb_subdomain_store(struct sysdb_ctx *sysdb, } } - tmp_bool = ldb_msg_find_attr_as_bool(res->msgs[0], SYSDB_SUBDOMAIN_MPG, - !(mpg_mode == MPG_ENABLED)); - if (tmp_bool != (mpg_mode == MPG_ENABLED)) { - mpg_flags = LDB_FLAG_MOD_REPLACE; + tmp_str = ldb_msg_find_attr_as_string(res->msgs[0], + SYSDB_SUBDOMAIN_MPG, + "false"); + switch (mpg_mode) { + case MPG_ENABLED: + if (strcasecmp(tmp_str, "true") != 0) { + mpg_flags = LDB_FLAG_MOD_REPLACE; + } + break; + case MPG_DISABLED: + if (strcasecmp(tmp_str, "false") != 0) { + mpg_flags = LDB_FLAG_MOD_REPLACE; + } + break; } + tmp_bool = ldb_msg_find_attr_as_bool(res->msgs[0], SYSDB_SUBDOMAIN_ENUM, !enumerate); if (tmp_bool != enumerate) { @@ -1104,8 +1124,14 @@ errno_t sysdb_subdomain_store(struct sysdb_ctx *sysdb, goto done; } - ret = ldb_msg_add_string(msg, SYSDB_SUBDOMAIN_MPG, - (mpg_mode == MPG_ENABLED) ? "TRUE" : "FALSE"); + tmp_str = str_domain_mpg_mode(mpg_mode); + if (tmp_str == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, "Couldn't convert mpg_mode to string\n"); + ret = EINVAL; + goto done; + } + + ret = ldb_msg_add_string(msg, SYSDB_SUBDOMAIN_MPG, tmp_str); if (ret != LDB_SUCCESS) { ret = sysdb_error_to_errno(ret); goto done; diff --git a/src/tests/cmocka/test_sysdb_subdomains.c b/src/tests/cmocka/test_sysdb_subdomains.c index d2116dc23..f91909198 100644 --- a/src/tests/cmocka/test_sysdb_subdomains.c +++ b/src/tests/cmocka/test_sysdb_subdomains.c @@ -109,6 +109,7 @@ static void test_sysdb_subdomain_create(void **state) assert_non_null(test_ctx->tctx->dom->subdomains); assert_string_equal(test_ctx->tctx->dom->subdomains->name, dom1[0]); assert_int_equal(test_ctx->tctx->dom->subdomains->trust_direction, 0); + assert_true(test_ctx->tctx->dom->subdomains->mpg_mode == MPG_DISABLED); ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, dom2[0], dom2[1], dom2[2], dom2[3], @@ -121,6 +122,7 @@ static void test_sysdb_subdomain_create(void **state) assert_non_null(test_ctx->tctx->dom->subdomains->next); assert_string_equal(test_ctx->tctx->dom->subdomains->next->name, dom2[0]); assert_int_equal(test_ctx->tctx->dom->subdomains->next->trust_direction, 1); + assert_true(test_ctx->tctx->dom->subdomains->next->mpg_mode == MPG_DISABLED); /* Reverse the trust directions */ ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, @@ -153,6 +155,23 @@ static void test_sysdb_subdomain_create(void **state) assert_int_equal( sss_domain_get_state(test_ctx->tctx->dom->subdomains->next), DOM_DISABLED); + + /* Test that changing the MPG status works */ + ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, + dom1[0], dom1[1], dom1[2], dom1[3], + MPG_ENABLED, false, NULL, 1, NULL); + assert_int_equal(ret, EOK); + + ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, + dom2[0], dom2[1], dom2[2], dom2[3], + MPG_ENABLED, false, NULL, 0, NULL); + assert_int_equal(ret, EOK); + + ret = sysdb_update_subdomains(test_ctx->tctx->dom, test_ctx->tctx->confdb); + assert_int_equal(ret, EOK); + + assert_true(test_ctx->tctx->dom->subdomains->mpg_mode == MPG_ENABLED); + assert_true(test_ctx->tctx->dom->subdomains->next->mpg_mode == MPG_ENABLED); } static void test_sysdb_master_domain_ops(void **state) diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c index c3b1c62f6..b4ee5d91c 100644 --- a/src/util/domain_info_utils.c +++ b/src/util/domain_info_utils.c @@ -944,3 +944,15 @@ enum sss_domain_mpg_mode get_domain_mpg_mode(struct sss_domain_info *domain) { return domain->mpg_mode; } + +const char *str_domain_mpg_mode(enum sss_domain_mpg_mode mpg_mode) +{ + switch (mpg_mode) { + case MPG_ENABLED: + return "true"; + case MPG_DISABLED: + return "false"; + } + + return NULL; +} diff --git a/src/util/util.h b/src/util/util.h index b3f94bc8a..8d5292e41 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -577,6 +577,7 @@ 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); +const char *str_domain_mpg_mode(enum sss_domain_mpg_mode mpg_mode); #define IS_SUBDOMAIN(dom) ((dom)->parent != NULL) From 8630451ca1a139390dc9d8f4bd426efe9aaf279d Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Sun, 2 Sep 2018 17:25:23 +0200 Subject: [PATCH 4/6] 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 --- src/confdb/confdb.c | 12 +--- src/confdb/confdb.h | 1 + src/db/sysdb_subdomains.c | 17 ++--- src/man/sssd.conf.5.xml | 35 ++++++++-- src/responder/nss/nss_protocol_grent.c | 4 ++ src/responder/nss/nss_protocol_pwent.c | 11 +++ src/tests/cmocka/test_sysdb_subdomains.c | 16 +++++ src/tests/intg/test_ldap.py | 89 ++++++++++++++++++++++++ src/util/domain_info_utils.c | 25 ++++++- src/util/util.h | 1 + 10 files changed, 184 insertions(+), 27 deletions(-) diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c index 294a36777..e48541678 100644 --- a/src/confdb/confdb.c +++ b/src/confdb/confdb.c @@ -941,17 +941,7 @@ static int confdb_get_domain_internal(struct confdb_ctx *cdb, if (tmp == NULL || *tmp == '\0') { tmp = "false"; } - - if (strcasecmp(tmp, "FALSE") == 0) { - domain->mpg_mode = MPG_DISABLED; - } else if (strcasecmp(tmp, "TRUE") == 0) { - domain->mpg_mode = MPG_ENABLED; - } else { - DEBUG(SSSDBG_FATAL_FAILURE, - "Invalid value for %s\n", CONFDB_DOMAIN_AUTO_UPG); - ret = EINVAL; - goto done; - } + domain->mpg_mode = str_to_domain_mpg_mode(tmp); if (local_provider_is_built() && strcasecmp(domain->provider, "local") == 0) { diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index a4c248669..85b952dd1 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -302,6 +302,7 @@ enum sss_domain_type { enum sss_domain_mpg_mode { MPG_DISABLED, MPG_ENABLED, + MPG_HYBRID, }; /** diff --git a/src/db/sysdb_subdomains.c b/src/db/sysdb_subdomains.c index cde91cd5d..e134adaf8 100644 --- a/src/db/sysdb_subdomains.c +++ b/src/db/sysdb_subdomains.c @@ -382,17 +382,7 @@ errno_t sysdb_update_subdomains(struct sss_domain_info *domain, if (str_mpg_mode == NULL || *str_mpg_mode == '\0') { str_mpg_mode = "false"; } - - if (strcasecmp(str_mpg_mode, "FALSE") == 0) { - mpg_mode = MPG_DISABLED; - } else if (strcasecmp(str_mpg_mode, "TRUE") == 0) { - mpg_mode = MPG_ENABLED; - } else { - DEBUG(SSSDBG_MINOR_FAILURE, - "Invalid value for %s\n, assuming disabled", - SYSDB_SUBDOMAIN_MPG); - mpg_mode = MPG_DISABLED; - } + mpg_mode = str_to_domain_mpg_mode(str_mpg_mode); enumerate = ldb_msg_find_attr_as_bool(res->msgs[i], SYSDB_SUBDOMAIN_ENUM, false); @@ -1013,6 +1003,11 @@ errno_t sysdb_subdomain_store(struct sysdb_ctx *sysdb, mpg_flags = LDB_FLAG_MOD_REPLACE; } break; + case MPG_HYBRID: + if (strcasecmp(tmp_str, "hybrid") != 0) { + mpg_flags = LDB_FLAG_MOD_REPLACE; + } + break; } tmp_bool = ldb_msg_find_attr_as_bool(res->msgs[0], SYSDB_SUBDOMAIN_ENUM, diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index 881ffc6ab..d856b0360 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -2980,10 +2980,37 @@ subdomain_inherit = ldap_purge_cache_timeout <term>auto_private_groups (string)</term> <listitem> <para> - If this option is enabled, SSSD will automatically - create user private groups based on user's - UID number. The GID number is ignored in this case. - </para> + This option takes any of three available values: + <variablelist> + <varlistentry> + <term>true</term> + <listitem> + <para> + Create user's private group unconditionally from user's UID number. + The GID number is ignored in this case. + </para> + </listitem> + </varlistentry> + <varlistentry> + <term>false</term> + <listitem> + <para> + Always use the user's primary GID number. + </para> + </listitem> + </varlistentry> + <varlistentry> + <term>hybrid</term> + <listitem> + <para> + Use the user's primary GID number if available. If the primary GID + number is not available, generate a private user group using the + user's UID number. + </para> + </listitem> + </varlistentry> + </variablelist> + </para> <para> For POSIX subdomains, setting the option in the main domain is inherited in the subdomain. diff --git a/src/responder/nss/nss_protocol_grent.c b/src/responder/nss/nss_protocol_grent.c index 59cdd800d..09b227d98 100644 --- a/src/responder/nss/nss_protocol_grent.c +++ b/src/responder/nss/nss_protocol_grent.c @@ -348,6 +348,10 @@ nss_protocol_fill_initgr(struct nss_ctx *nss_ctx, orig_gid = sss_view_ldb_msg_find_attr_as_uint64(domain, user, SYSDB_PRIMARY_GROUP_GIDNUM, 0); + if (get_domain_mpg_mode(domain) == MPG_HYBRID && orig_gid != 0) { + /* In the hybrid mode, we use the original GID, if available */ + gid = orig_gid; + } /* If the GID of the original primary group is available but equal to the * current primary GID it must not be added. */ diff --git a/src/responder/nss/nss_protocol_pwent.c b/src/responder/nss/nss_protocol_pwent.c index af9e74fc8..e76247fe1 100644 --- a/src/responder/nss/nss_protocol_pwent.c +++ b/src/responder/nss/nss_protocol_pwent.c @@ -41,6 +41,17 @@ nss_get_gid(struct sss_domain_info *domain, return domain->override_gid; } + /* If this is a hybrid-MPG domain, and the original GID is available, + * use that */ + if (get_domain_mpg_mode(domain) == MPG_HYBRID) { + gid = sss_view_ldb_msg_find_attr_as_uint64(domain, msg, + SYSDB_PRIMARY_GROUP_GIDNUM, + 0); + if (gid != 0) { + return gid; + } + } + /* Return original gid. */ return ldb_msg_find_attr_as_uint64(msg, SYSDB_GIDNUM, 0); } diff --git a/src/tests/cmocka/test_sysdb_subdomains.c b/src/tests/cmocka/test_sysdb_subdomains.c index f91909198..7830425af 100644 --- a/src/tests/cmocka/test_sysdb_subdomains.c +++ b/src/tests/cmocka/test_sysdb_subdomains.c @@ -172,6 +172,22 @@ static void test_sysdb_subdomain_create(void **state) assert_true(test_ctx->tctx->dom->subdomains->mpg_mode == MPG_ENABLED); assert_true(test_ctx->tctx->dom->subdomains->next->mpg_mode == MPG_ENABLED); + + ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, + dom1[0], dom1[1], dom1[2], dom1[3], + MPG_HYBRID, false, NULL, 1, NULL); + assert_int_equal(ret, EOK); + + ret = sysdb_subdomain_store(test_ctx->tctx->sysdb, + dom2[0], dom2[1], dom2[2], dom2[3], + MPG_HYBRID, false, NULL, 0, NULL); + assert_int_equal(ret, EOK); + + ret = sysdb_update_subdomains(test_ctx->tctx->dom, test_ctx->tctx->confdb); + assert_int_equal(ret, EOK); + + assert_true(test_ctx->tctx->dom->subdomains->mpg_mode == MPG_HYBRID); + assert_true(test_ctx->tctx->dom->subdomains->next->mpg_mode == MPG_HYBRID); } static void test_sysdb_master_domain_ops(void **state) diff --git a/src/tests/intg/test_ldap.py b/src/tests/intg/test_ldap.py index 3385feee1..0e5e378de 100644 --- a/src/tests/intg/test_ldap.py +++ b/src/tests/intg/test_ldap.py @@ -35,6 +35,7 @@ import sssd_id import sssd_ldb from util import unindent +from util import run_shell from sssd_nss import NssReturnCode from sssd_passwd import call_sssd_getpwnam, call_sssd_getpwuid from sssd_group import call_sssd_getgrnam, call_sssd_getgrgid @@ -1451,6 +1452,94 @@ def test_ldap_auto_private_groups_direct_no_gid(ldap_conn, mpg_setup_no_gid): ) +@pytest.fixture +def mpg_setup_hybrid(request, ldap_conn): + """ + This setup stores the primari GID in the "mail" attribute to simulate + the scenario where only some user have the gidNumber set at all. + """ + ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) + + ent_list.add_user("user_with_gid", 1001, 5555, mail='2001') + ent_list.add_group_bis("group1", 2001) + ent_list.add_group_bis("with_gid_group1", 10010, ["user_with_gid"]) + ent_list.add_group_bis("with_gid_group2", 10011, ["user_with_gid"]) + + ent_list.add_user("user_with_no_gid", 1002, 6666) + ent_list.add_group_bis("no_gid_group1", 10020, ["user_with_no_gid"]) + ent_list.add_group_bis("no_gid_group2", 10021, ["user_with_no_gid"]) + + create_ldap_entries(ldap_conn, ent_list) + create_ldap_cleanup(request, ldap_conn, None) + + conf = \ + format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS) + \ + unindent(""" + [domain/LDAP] + auto_private_groups = hybrid + ldap_user_gid_number = mail + """).format(**locals()) + create_conf_fixture(request, conf) + create_sssd_fixture(request) + return None + + +def test_ldap_auto_private_groups_hybrid_direct(ldap_conn, mpg_setup_hybrid): + """ + Integration test for auto_private_groups=hybrid. This test checks the + resolution of the users and their groups. + + See also ticket https://pagure.io/SSSD/sssd/issue/1872 + """ + # Make sure the user's GID is taken from their gidNumber, if available + ent.assert_passwd_by_name("user_with_gid", + dict(name="user_with_gid", uid=1001, gid=2001)) + + # The user's secondary groups list must be correct as well and include + # the primary gid, too + user_with_gid_ids = [2001, 10010, 10011] + (res, errno, gids) = sssd_id.call_sssd_initgroups("user_with_gid", 2001) + assert res == sssd_id.NssReturnCode.SUCCESS + + assert sorted(gids) == sorted(user_with_gid_ids), \ + "result: %s\n expected %s" % ( + ", ".join(["%s" % s for s in sorted(gids)]), + ", ".join(["%s" % s for s in sorted(user_with_gid_ids)]) + ) + + # On the other hand, if no gidNumber is available, sssd should generate + # the private group on its own + ent.assert_passwd_by_name("user_with_no_gid", + dict(name="user_with_no_gid", + uid=1002, gid=1002)) + + # The user's secondary groups list must be correct as well. Since there was + # no original GID, it is not added to the list + user_without_gid_ids = [1002, 10020, 10021] + (res, errno, gids) = sssd_id.call_sssd_initgroups("user_with_no_gid", 1002) + assert res == sssd_id.NssReturnCode.SUCCESS + + assert sorted(gids) == sorted(user_without_gid_ids), \ + "result: %s\n expected %s" % ( + ", ".join(["%s" % s for s in sorted(gids)]), + ", ".join(["%s" % s for s in sorted(user_without_gid_ids)]) + ) + + +def test_ldap_auto_private_groups_hybrid_priv_group(ldap_conn, + mpg_setup_hybrid): + """ + Integration test for auto_private_groups=hybrid. This test checks the + resolution of user private groups. + + See also ticket https://pagure.io/SSSD/sssd/issue/1872 + """ + # Make sure the private group of user who has this group set in their + # gidNumber is resolvable by name and by GID + ent.assert_group_by_name("group1", dict(gid=2001, mem=ent.contains_only())) + ent.assert_group_by_gid(2001, dict(name="group1", mem=ent.contains_only())) + + def rename_setup_no_cleanup(request, ldap_conn, cleanup_ent=None): ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) ent_list.add_user("user1", 1001, 2001) diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c index b4ee5d91c..afbb03fd2 100644 --- a/src/util/domain_info_utils.c +++ b/src/util/domain_info_utils.c @@ -937,7 +937,12 @@ bool is_files_provider(struct sss_domain_info *domain) bool sss_domain_is_mpg(struct sss_domain_info *domain) { - return domain->mpg_mode == MPG_ENABLED; + if (domain->mpg_mode == MPG_ENABLED + || domain->mpg_mode == MPG_HYBRID) { + return true; + } + + return false; } enum sss_domain_mpg_mode get_domain_mpg_mode(struct sss_domain_info *domain) @@ -952,7 +957,25 @@ const char *str_domain_mpg_mode(enum sss_domain_mpg_mode mpg_mode) return "true"; case MPG_DISABLED: return "false"; + case MPG_HYBRID: + return "hybrid"; } return NULL; } + +enum sss_domain_mpg_mode str_to_domain_mpg_mode(const char *str_mpg_mode) +{ + if (strcasecmp(str_mpg_mode, "FALSE") == 0) { + return MPG_DISABLED; + } else if (strcasecmp(str_mpg_mode, "TRUE") == 0) { + return MPG_ENABLED; + } else if (strcasecmp(str_mpg_mode, "HYBRID") == 0) { + return MPG_HYBRID; + } + + DEBUG(SSSDBG_MINOR_FAILURE, + "Invalid value for %s\n, assuming disabled", + SYSDB_SUBDOMAIN_MPG); + return MPG_DISABLED; +} diff --git a/src/util/util.h b/src/util/util.h index 8d5292e41..d44460e32 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -578,6 +578,7 @@ bool sss_domain_is_mpg(struct sss_domain_info *domain); enum sss_domain_mpg_mode get_domain_mpg_mode(struct sss_domain_info *domain); const char *str_domain_mpg_mode(enum sss_domain_mpg_mode mpg_mode); +enum sss_domain_mpg_mode str_to_domain_mpg_mode(const char *str_mpg_mode); #define IS_SUBDOMAIN(dom) ((dom)->parent != NULL) From 5f8b6d88cf75afc30b2c9ef780a621b96e4a3c87 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Tue, 4 Sep 2018 13:50:32 +0200 Subject: [PATCH 5/6] 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 --- src/db/sysdb_search.c | 215 ++++++++++++++++++++++++++++++------------ 1 file changed, 153 insertions(+), 62 deletions(-) diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index f0918bf9a..779955e12 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -883,6 +883,74 @@ int sysdb_getgrnam_with_views(TALLOC_CTX *mem_ctx, return ret; } +static int sysdb_getgrnam_int(TALLOC_CTX *mem_ctx, + struct sss_domain_info *domain, + struct ldb_dn *base_dn, + const char *fmt_filter, + const char *sanitized_name, + const char *lc_sanitized_name, + struct ldb_result **_res) +{ + static const char *attrs[] = SYSDB_GRSRC_ATTRS; + int ret; + + ret = ldb_search(domain->sysdb->ldb, mem_ctx, _res, base_dn, + LDB_SCOPE_SUBTREE, attrs, fmt_filter, + lc_sanitized_name, sanitized_name, sanitized_name); + if (ret != EOK) { + ret = sysdb_error_to_errno(ret); + goto done; + } + + ret = EOK; +done: + return ret; +} + +static int sysdb_getgrnam_mpg(TALLOC_CTX *mem_ctx, + struct sss_domain_info *domain, + const char *sanitized_name, + const char *lc_sanitized_name, + struct ldb_result **_res) +{ + struct ldb_dn *base_dn = NULL; + const char *fmt_filter = SYSDB_GRNAM_MPG_FILTER; + int ret; + + base_dn = sysdb_base_dn(domain->sysdb, mem_ctx); + if (base_dn == NULL) { + return ENOMEM; + } + + ret = sysdb_getgrnam_int(mem_ctx, domain, base_dn, fmt_filter, + sanitized_name, lc_sanitized_name, + _res); + talloc_free(base_dn); + return ret; +} + +static int sysdb_getgrnam_grp(TALLOC_CTX *mem_ctx, + struct sss_domain_info *domain, + const char *sanitized_name, + const char *lc_sanitized_name, + struct ldb_result **_res) +{ + struct ldb_dn *base_dn = NULL; + const char *fmt_filter = SYSDB_GRNAM_FILTER; + int ret; + + base_dn = sysdb_group_base_dn(mem_ctx, domain); + if (base_dn == NULL) { + return ENOMEM; + } + + ret = sysdb_getgrnam_int(mem_ctx, domain, base_dn, fmt_filter, + sanitized_name, lc_sanitized_name, + _res); + talloc_free(base_dn); + return ret; +} + int sysdb_getgrnam(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *name, @@ -890,9 +958,7 @@ int sysdb_getgrnam(TALLOC_CTX *mem_ctx, { TALLOC_CTX *tmp_ctx; static const char *attrs[] = SYSDB_GRSRC_ATTRS; - const char *fmt_filter; char *sanitized_name; - struct ldb_dn *base_dn; struct ldb_result *res = NULL; char *lc_sanitized_name; const char *originalad_sanitized_name; @@ -918,18 +984,10 @@ int sysdb_getgrnam(TALLOC_CTX *mem_ctx, * override and in order to return the proper overridden group * we must use the very same search used by a non-mpg domain */ - fmt_filter = SYSDB_GRNAM_MPG_FILTER; - base_dn = sysdb_domain_dn(tmp_ctx, domain); - if (base_dn == NULL) { - ret = ENOMEM; - goto done; - } - - ret = ldb_search(domain->sysdb->ldb, tmp_ctx, &res, base_dn, - LDB_SCOPE_SUBTREE, attrs, fmt_filter, - lc_sanitized_name, sanitized_name, sanitized_name); + ret = sysdb_getgrnam_mpg(tmp_ctx, domain, + sanitized_name, lc_sanitized_name, + &res); if (ret != EOK) { - ret = sysdb_error_to_errno(ret); goto done; } @@ -939,29 +997,19 @@ int sysdb_getgrnam(TALLOC_CTX *mem_ctx, if (originalad_sanitized_name != NULL && strcmp(originalad_sanitized_name, sanitized_name) != 0) { - fmt_filter = SYSDB_GRNAM_FILTER; - base_dn = sysdb_group_base_dn(tmp_ctx, domain); - res = NULL; + ret = sysdb_getgrnam_grp(tmp_ctx, domain, + sanitized_name, lc_sanitized_name, + &res); + if (ret != EOK) { + goto done; + } } } } else { - fmt_filter = SYSDB_GRNAM_FILTER; - base_dn = sysdb_group_base_dn(tmp_ctx, domain); - } - if (base_dn == NULL) { - ret = ENOMEM; - goto done; - } - - /* We just do the ldb_search here in case domain is *not* a MPG *or* - * it's a MPG and we're dealing with a overriden group, which has to - * use the very same filter as a non MPG domain. */ - if (res == NULL) { - ret = ldb_search(domain->sysdb->ldb, tmp_ctx, &res, base_dn, - LDB_SCOPE_SUBTREE, attrs, fmt_filter, - lc_sanitized_name, sanitized_name, sanitized_name); + ret = sysdb_getgrnam_grp(tmp_ctx, domain, + sanitized_name, lc_sanitized_name, + &res); if (ret != EOK) { - ret = sysdb_error_to_errno(ret); goto done; } } @@ -1076,6 +1124,71 @@ int sysdb_getgrgid_with_views(TALLOC_CTX *mem_ctx, return ret; } + +static int sysdb_getgrgid_attrs_int(TALLOC_CTX *mem_ctx, + struct sss_domain_info *domain, + struct ldb_dn *base_dn, + const char *fmt_filter, + gid_t gid, + const char **attrs, + struct ldb_result **_res) +{ + int ret; + + ret = ldb_search(domain->sysdb->ldb, mem_ctx, _res, base_dn, + LDB_SCOPE_SUBTREE, attrs, fmt_filter, gid); + if (ret != EOK) { + ret = sysdb_error_to_errno(ret); + goto done; + } + + ret = EOK; +done: + return ret; +} + +static int sysdb_getgrgid_attrs_mpg(TALLOC_CTX *mem_ctx, + struct sss_domain_info *domain, + gid_t gid, + const char **attrs, + struct ldb_result **_res) +{ + struct ldb_dn *base_dn = NULL; + const char *fmt_filter = SYSDB_GRGID_MPG_FILTER; + int ret; + + base_dn = sysdb_base_dn(domain->sysdb, mem_ctx); + if (base_dn == NULL) { + return ENOMEM; + } + + ret = sysdb_getgrgid_attrs_int(mem_ctx, domain, base_dn, fmt_filter, + gid, attrs, _res); + talloc_free(base_dn); + return ret; +} + +static int sysdb_getgrgid_attrs_grp(TALLOC_CTX *mem_ctx, + struct sss_domain_info *domain, + gid_t gid, + const char **attrs, + struct ldb_result **_res) +{ + struct ldb_dn *base_dn = NULL; + const char *fmt_filter = SYSDB_GRGID_FILTER; + int ret; + + base_dn = sysdb_group_base_dn(mem_ctx, domain); + if (base_dn == NULL) { + return ENOMEM; + } + + ret = sysdb_getgrgid_attrs_int(mem_ctx, domain, base_dn, fmt_filter, + gid, attrs, _res); + talloc_free(base_dn); + return ret; +} + int sysdb_getgrgid_attrs(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, gid_t gid, @@ -1085,8 +1198,6 @@ int sysdb_getgrgid_attrs(TALLOC_CTX *mem_ctx, TALLOC_CTX *tmp_ctx; unsigned long int ul_gid = gid; unsigned long int ul_originalad_gid; - const char *fmt_filter; - struct ldb_dn *base_dn; struct ldb_result *res = NULL; int ret; static const char *default_attrs[] = SYSDB_GRSRC_ATTRS; @@ -1117,17 +1228,8 @@ int sysdb_getgrgid_attrs(TALLOC_CTX *mem_ctx, * override and in order to return the proper overridden group * we must use the very same search used by a non-mpg domain */ - fmt_filter = SYSDB_GRGID_MPG_FILTER; - base_dn = sysdb_domain_dn(tmp_ctx, domain); - if (base_dn == NULL) { - ret = ENOMEM; - goto done; - } - - ret = ldb_search(domain->sysdb->ldb, tmp_ctx, &res, base_dn, - LDB_SCOPE_SUBTREE, attrs, fmt_filter, ul_gid); + ret = sysdb_getgrgid_attrs_mpg(mem_ctx, domain, gid, attrs, &res); if (ret != EOK) { - ret = sysdb_error_to_errno(ret); goto done; } @@ -1136,28 +1238,17 @@ int sysdb_getgrgid_attrs(TALLOC_CTX *mem_ctx, res->msgs[0], ORIGINALAD_PREFIX SYSDB_GIDNUM, 0); if (ul_originalad_gid != 0 && ul_originalad_gid != ul_gid) { - fmt_filter = SYSDB_GRGID_FILTER; - base_dn = sysdb_group_base_dn(tmp_ctx, domain); - res = NULL; + ret = sysdb_getgrgid_attrs_grp(mem_ctx, domain, + gid, attrs, + &res); + if (ret != EOK) { + goto done; + } } } } else { - fmt_filter = SYSDB_GRGID_FILTER; - base_dn = sysdb_group_base_dn(tmp_ctx, domain); - } - if (base_dn == NULL) { - ret = ENOMEM; - goto done; - } - - /* We just do the ldb_search here in case domain is *not* a MPG *or* - * it's a MPG and we're dealing with a overriden group, which has to - * use the very same filter as a non MPG domain. */ - if (res == NULL) { - ret = ldb_search(domain->sysdb->ldb, tmp_ctx, &res, base_dn, - LDB_SCOPE_SUBTREE, attrs, fmt_filter, ul_gid); + ret = sysdb_getgrgid_attrs_grp(mem_ctx, domain, gid, attrs, &res); if (ret != EOK) { - ret = sysdb_error_to_errno(ret); goto done; } } From f5b1f85add2a7adf707004214ad9c0c05073ad10 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Tue, 4 Sep 2018 17:09:42 +0200 Subject: [PATCH 6/6] 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 --- src/db/sysdb.h | 1 + src/db/sysdb_ops.c | 89 +++++++++++++++++++++++-- src/db/sysdb_search.c | 127 ++++++++++++++++++++++++++++++++++-- src/tests/intg/test_ldap.py | 30 +++++++-- src/tests/sysdb-tests.c | 52 +++++++++++++++ 5 files changed, 287 insertions(+), 12 deletions(-) diff --git a/src/db/sysdb.h b/src/db/sysdb.h index a1d415d97..b387ab6ac 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -249,6 +249,7 @@ NULL} #define SYSDB_GRSRC_ATTRS {SYSDB_NAME, SYSDB_GIDNUM, \ + SYSDB_PRIMARY_GROUP_GIDNUM, \ SYSDB_MEMBERUID, \ SYSDB_MEMBER, \ SYSDB_GHOST, \ diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index d0c4f35b4..fd98205e9 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -473,6 +473,75 @@ static errno_t cleanup_dn_filter(TALLOC_CTX *mem_ctx, return ret; } +static int sysdb_group_search_hybrid_retry(TALLOC_CTX *mem_ctx, + struct sss_domain_info *domain, + const char *sanitized_name, + const char *lc_sanitized_name, + const char **additional_attrs, + size_t *_msgs_count, + struct ldb_message ***_msgs) +{ + gid_t orig_gid; + struct ldb_dn *basedn = NULL; + int ret; + TALLOC_CTX *tmp_ctx = NULL; + char *filter = NULL; + struct ldb_message **msgs = NULL; + size_t msgs_count = 0; + static const char *default_attrs[] = { SYSDB_PRIMARY_GROUP_GIDNUM, NULL }; + const char **attrs = NULL; + + tmp_ctx = talloc_new(NULL); + if (!tmp_ctx) { + return ENOMEM; + } + + basedn = sysdb_domain_dn(tmp_ctx, domain); + if (basedn == NULL) { + ret = ENOMEM; + goto done; + } + + filter = talloc_asprintf(tmp_ctx, SYSDB_GRNAM_MPG_FILTER, lc_sanitized_name, + sanitized_name, sanitized_name); + if (filter == NULL) { + ret = ENOMEM; + goto done; + } + + ret = add_strings_lists(tmp_ctx, additional_attrs, default_attrs, + false, discard_const(&attrs)); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Cannot combine string lists [%d]: %s\n", ret, sss_strerror(ret)); + goto done; + } + + ret = sysdb_search_entry(tmp_ctx, domain->sysdb, basedn, LDB_SCOPE_SUBTREE, + filter, attrs, + &msgs_count, &msgs); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "sysdb_search_entry failed [%d]: %s\n", ret, sss_strerror(ret)); + goto done; + } + + orig_gid = sss_view_ldb_msg_find_attr_as_uint64(domain, msgs[0], + SYSDB_PRIMARY_GROUP_GIDNUM, + 0); + if (orig_gid != 0) { + DEBUG(SSSDBG_TRACE_LIBS, "User entry has a GID set, not usable as a MPG group\n"); + ret = ENOENT; + goto done; + } + + *_msgs_count = msgs_count; + *_msgs = talloc_steal(mem_ctx, msgs); +done: + talloc_zfree(tmp_ctx); + return ret; +} + static int sysdb_search_by_name(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *name, @@ -481,7 +550,7 @@ static int sysdb_search_by_name(TALLOC_CTX *mem_ctx, struct ldb_message **msg) { TALLOC_CTX *tmp_ctx; - const char *def_attrs[] = { SYSDB_NAME, NULL, NULL }; + const char *def_attrs[] = { SYSDB_NAME, NULL, NULL, NULL }; const char *filter_tmpl = NULL; struct ldb_message **msgs = NULL; struct ldb_dn *basedn; @@ -490,6 +559,7 @@ static int sysdb_search_by_name(TALLOC_CTX *mem_ctx, char *lc_sanitized_name; char *filter; int ret; + enum sss_domain_mpg_mode mpg_mode = get_domain_mpg_mode(domain); tmp_ctx = talloc_new(NULL); if (!tmp_ctx) { @@ -504,9 +574,12 @@ static int sysdb_search_by_name(TALLOC_CTX *mem_ctx, break; case SYSDB_GROUP: def_attrs[1] = SYSDB_GIDNUM; - if (sss_domain_is_mpg(domain) && - (!local_provider_is_built() - || strcasecmp(domain->provider, "local") != 0)) { + if (mpg_mode == MPG_HYBRID) { + filter_tmpl = SYSDB_GRNAM_FILTER; + basedn = sysdb_group_base_dn(tmp_ctx, domain); + } else if (mpg_mode == MPG_ENABLED + && (!local_provider_is_built() + || strcasecmp(domain->provider, "local") != 0)) { /* When searching a group by name in a MPG domain, we also * need to search the user space in order to be able to match * a user private group/ @@ -544,6 +617,14 @@ static int sysdb_search_by_name(TALLOC_CTX *mem_ctx, ret = sysdb_search_entry(tmp_ctx, domain->sysdb, basedn, LDB_SCOPE_SUBTREE, filter, attrs?attrs:def_attrs, &msgs_count, &msgs); + if (ret == ENOENT && mpg_mode == MPG_HYBRID) { + DEBUG(SSSDBG_TRACE_INTERNAL, "Retrying for MPG in a hybrid domain\n"); + ret = sysdb_group_search_hybrid_retry(mem_ctx, domain, + sanitized_name, lc_sanitized_name, + attrs?attrs:def_attrs, + &msgs_count, &msgs); + } + if (ret) { goto done; } diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 779955e12..48653d1f2 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -963,6 +963,8 @@ int sysdb_getgrnam(TALLOC_CTX *mem_ctx, char *lc_sanitized_name; const char *originalad_sanitized_name; int ret; + enum sss_domain_mpg_mode mpg_mode = get_domain_mpg_mode(domain); + gid_t orig_gid; tmp_ctx = talloc_new(NULL); if (!tmp_ctx) { @@ -975,7 +977,64 @@ int sysdb_getgrnam(TALLOC_CTX *mem_ctx, goto done; } - if (sss_domain_is_mpg(domain)) { + switch (mpg_mode) { + case MPG_HYBRID: + /* In the hybrid MPG domain, we first search for the real group + * and only if it's not found, we fall back to searching the + * MPG group. This is because the hybrid mode stores information + * internally like a MPG domain, but if the entry does have the + * original GID set, we want to avoid returning the MPG group + * by name. We first search the 'real' group space explicitly + * to make sure that we handle the situation where a real group + * exists with the same name as the MPG group. + */ + ret = sysdb_getgrnam_grp(tmp_ctx, domain, + sanitized_name, lc_sanitized_name, + &res); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "sysdb_getgrnam_grp failed [%d]: %s\n", + ret, sss_strerror(ret)); + goto done; + } + + if (res->count > 0) { + DEBUG(SSSDBG_TRACE_INTERNAL, "Found a real group in a hybrid-MPG domain\n"); + /* Found something, let's process the result */ + break; + } + + /* Otherwise try the MPG space */ + ret = sysdb_getgrnam_mpg(tmp_ctx, domain, + sanitized_name, lc_sanitized_name, + &res); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "sysdb_getgrnam_mpg failed [%d]: %s\n", + ret, sss_strerror(ret)); + goto done; + } + + if (res->count > 0) { + /* If this is a user with the original gid set, we should ignore this result */ + orig_gid = sss_view_ldb_msg_find_attr_as_uint64( + domain, res->msgs[0], + SYSDB_PRIMARY_GROUP_GIDNUM, + 0); + if (orig_gid != 0) { + DEBUG(SSSDBG_TRACE_INTERNAL, + "Ignoring MPG group for a user with %s set\n", + SYSDB_PRIMARY_GROUP_GIDNUM); + res->count = 0; + talloc_zfree(res->msgs); + break; + } + /* We found a user with no original GID, we can return the result as + * their MPG group + */ + } + break; + case MPG_ENABLED: /* In case the domain supports magic private groups we *must* * check whether the searched name is the very same as the * originalADname attribute. @@ -988,6 +1047,9 @@ int sysdb_getgrnam(TALLOC_CTX *mem_ctx, sanitized_name, lc_sanitized_name, &res); if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "sysdb_getgrnam_mpg failed [%d]: %s\n", + ret, sss_strerror(ret)); goto done; } @@ -1001,17 +1063,25 @@ int sysdb_getgrnam(TALLOC_CTX *mem_ctx, sanitized_name, lc_sanitized_name, &res); if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "sysdb_getgrnam_grp failed [%d]: %s\n", + ret, sss_strerror(ret)); goto done; } } } - } else { + break; + case MPG_DISABLED: ret = sysdb_getgrnam_grp(tmp_ctx, domain, sanitized_name, lc_sanitized_name, &res); if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "sysdb_getgrnam_grp failed [%d]: %s\n", + ret, sss_strerror(ret)); goto done; } + break; } ret = mpg_res_convert(res); @@ -1202,6 +1272,8 @@ int sysdb_getgrgid_attrs(TALLOC_CTX *mem_ctx, int ret; static const char *default_attrs[] = SYSDB_GRSRC_ATTRS; const char **attrs = NULL; + enum sss_domain_mpg_mode mpg_mode = get_domain_mpg_mode(domain); + gid_t orig_gid; tmp_ctx = talloc_new(NULL); if (!tmp_ctx) { @@ -1219,7 +1291,52 @@ int sysdb_getgrgid_attrs(TALLOC_CTX *mem_ctx, } } - if (sss_domain_is_mpg(domain)) { + switch (mpg_mode) { + case MPG_HYBRID: + /* In the hybrid MPG domain, we first search for the real group + * and only if it's not found, we fall back to searching the + * MPG group. This is because the hybrid mode stores information + * internally like a MPG domain, but if the entry does have the + * original GID set, we want to avoid returning the MPG group + * by ID. We first search the 'real' group space explicitly + * to make sure that we handle the situation where a real group + * exists with the same ID as the MPG group. + */ + ret = sysdb_getgrgid_attrs_grp(mem_ctx, domain, + gid, attrs, + &res); + if (ret != EOK) { + goto done; + } + + if (res->count > 0) { + /* Found something, let's process the result */ + break; + } + + /* Otherwise try the MPG space */ + ret = sysdb_getgrgid_attrs_mpg(mem_ctx, domain, gid, attrs, &res); + if (ret != EOK) { + goto done; + } + + if (res->count > 0) { + /* If this is a user with the original gid set, we should ignore this result */ + orig_gid = sss_view_ldb_msg_find_attr_as_uint64( + domain, res->msgs[0], + SYSDB_PRIMARY_GROUP_GIDNUM, + 0); + if (orig_gid != 0) { + res->count = 0; + talloc_zfree(res->msgs); + break; + } + /* We found a user with no original GID, we can return the result as + * their MPG group + */ + } + break; + case MPG_ENABLED: /* In case the domain supports magic private groups we *must* * check whether the searched gid is the very same as the * originalADgidNumber attribute. @@ -1246,11 +1363,13 @@ int sysdb_getgrgid_attrs(TALLOC_CTX *mem_ctx, } } } - } else { + break; + case MPG_DISABLED: ret = sysdb_getgrgid_attrs_grp(mem_ctx, domain, gid, attrs, &res); if (ret != EOK) { goto done; } + break; } ret = mpg_res_convert(res); diff --git a/src/tests/intg/test_ldap.py b/src/tests/intg/test_ldap.py index 0e5e378de..f375d884c 100644 --- a/src/tests/intg/test_ldap.py +++ b/src/tests/intg/test_ldap.py @@ -1526,19 +1526,41 @@ def test_ldap_auto_private_groups_hybrid_direct(ldap_conn, mpg_setup_hybrid): ) -def test_ldap_auto_private_groups_hybrid_priv_group(ldap_conn, - mpg_setup_hybrid): +def test_ldap_auto_private_groups_hybrid_priv_group_byname(ldap_conn, + mpg_setup_hybrid): """ Integration test for auto_private_groups=hybrid. This test checks the - resolution of user private groups. + resolution of user private groups by name. See also ticket https://pagure.io/SSSD/sssd/issue/1872 """ # Make sure the private group of user who has this group set in their - # gidNumber is resolvable by name and by GID + # gidNumber is resolvable by name ent.assert_group_by_name("group1", dict(gid=2001, mem=ent.contains_only())) + + # ..but since this user /has/ a gidNumber set, their autogenerated group + # should not be resolvable + with pytest.raises(KeyError): + grp.getgrnam("user_with_gid") + + +def test_ldap_auto_private_groups_hybrid_priv_group_byid(ldap_conn, + mpg_setup_hybrid): + """ + Integration test for auto_private_groups=hybrid. This test checks the + resolution of user private groups by name. + + See also ticket https://pagure.io/SSSD/sssd/issue/1872 + """ + # Make sure the private group of user who has this group set in their + # gidNumber is resolvable by ID ent.assert_group_by_gid(2001, dict(name="group1", mem=ent.contains_only())) + # ..but since this user /has/ a gidNumber set, their autogenerated group + # should not be resolvable + with pytest.raises(KeyError): + grp.getgrgid(1001) + def rename_setup_no_cleanup(request, ldap_conn, cleanup_ent=None): ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c index 214ad02a9..85b80c1d6 100644 --- a/src/tests/sysdb-tests.c +++ b/src/tests/sysdb-tests.c @@ -1113,6 +1113,56 @@ START_TEST(test_user_group_by_name) } END_TEST +START_TEST(test_user_group_by_name_hybrid) +{ + struct sysdb_test_ctx *test_ctx; + struct ldb_message *msg; + int ret; + struct sysdb_attrs *attrs; + + /* Setup */ + ret = setup_sysdb_tests(&test_ctx); + if (ret != EOK) { + fail("Could not set up the test"); + return; + } + + test_ctx->domain->provider = discard_const_p(char, "ldap"); + test_ctx->domain->mpg_mode = MPG_HYBRID; + + /* giduser has a group GID stored in SYSDB_PRIMARY_GROUP_GIDNUM where his + * original primary group is referenced + */ + ret = sysdb_store_group(test_ctx->domain, + "gidgroup", 8764, NULL, 0, 0); + ck_assert_int_eq(ret, EOK); + + attrs = sysdb_new_attrs(test_ctx); + ck_assert_ptr_nonnull(attrs); + ret = sysdb_attrs_add_uint32(attrs, SYSDB_PRIMARY_GROUP_GIDNUM, 8764); + ck_assert_int_eq(ret, EOK); + + ret = sysdb_store_user(test_ctx->domain, + "giduser", "x", + 8765, 0, + "giduser", + "/home/giduser", + "/bin/bash", + NULL, attrs, NULL, -1, 0); + ck_assert_int_eq(ret, EOK); + + /* We shouldn't be able to find this user's MPG, because in hybrid domain, + * because this entry has the original group GID set + */ + ret = sysdb_search_group_by_name(test_ctx, + test_ctx->domain, + "giduser", + NULL, + &msg); + ck_assert_int_eq(ret, ENOENT); +} +END_TEST + START_TEST(test_user_group_by_name_local) { struct sysdb_test_ctx *test_ctx; @@ -7396,6 +7446,8 @@ Suite *create_sysdb_suite(void) tcase_add_test(tc_sysdb, test_sysdb_add_nonposix_user); tcase_add_test(tc_sysdb, test_sysdb_add_nonposix_group); + /* Tests for the hybrid MPG mapping */ + tcase_add_test(tc_sysdb, test_user_group_by_name_hybrid); /* ===== NETGROUP TESTS ===== */ /* Create a new netgroup */
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
