On (10/12/14 10:44), Pavel Reichl wrote: > >On 12/09/2014 01:36 PM, Sumit Bose wrote: >>On Tue, Nov 25, 2014 at 03:42:14PM +0100, Pavel Reichl wrote: >>>Hello, >>> >>>please see attached patch for https://fedorahosted.org/sssd/ticket/2492 >>> >>>Thanks! >>Hi Pavel, >> >>thank you for the patch, it works well in my tests and I didn't see any >>regressions in IPA setup with and without trsut to AD, so ACK. >> >>I would just like to ask you to add a comment to >> >>>@@ -842,6 +913,23 @@ static int sdap_save_grpmem(TALLOC_CTX *memctx, >>> goto fail; >>> } >>> } >>>+ if (opts->schema_type == SDAP_SCHEMA_IPA_V1) { >>>+ ret = sysdb_attrs_get_string(attrs, SYSDB_SID_STR, &group_sid); >>>+ if (ret != EOK) { >>>+ DEBUG(SSSDBG_TRACE_FUNC, "Failed to get group sid\n"); >>>+ group_sid = NULL; >>>+ } >>>+ >>>+ if (group_sid != NULL) { >>>+ ret = retain_extern_members(memctx, dom, group_name, group_sid, >>>+ &userdns, &nuserdns); >>>+ if (ret != EOK) { >>>+ DEBUG(SSSDBG_MINOR_FAILURE, >>>+ "retain_extern_members failed: %d:[%s].\n", >>>+ ret, sss_strerror(ret)); >>>+ } >>>+ } >>>+ } >>which explains that this is a temporary solution until the IPA provider >>can resolve external group membership. I have created >>https://fedorahosted.org/sssd/ticket/2522 for this. Feel free to >>explicitly add the ticket URL into the comment. >> >>bye, >>Sumit >>_______________________________________________ >>sssd-devel mailing list >>sssd-devel@lists.fedorahosted.org >>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel >Thanks for review. >Please see updated patch. >
>From a2b941b6f03b77e92604e2709bc18d891c2365f3 Mon Sep 17 00:00:00 2001 >From: Pavel Reichl <prei...@redhat.com> >Date: Thu, 20 Nov 2014 18:27:04 +0000 >Subject: [PATCH] LDAP: retain external members > >When processing group membership check sysdb for group members from >extern domain and include them in newly processed group membership as >extern members are curently found only when initgroups() is called. > >Resolves: >https://fedorahosted.org/sssd/ticket/2492 >--- > src/db/sysdb.h | 6 +++ > src/db/sysdb_ops.c | 90 ++++++++++++++++++++++++++++++++ > src/providers/ldap/sdap_async_groups.c | 93 ++++++++++++++++++++++++++++++++++ > 3 files changed, 189 insertions(+) > >diff --git a/src/db/sysdb.h b/src/db/sysdb.h >index >5bd7f90acb685bbaff5c98f433c7dce8175c33ca..a1254bb556533245ca3a02342b775cda7f0f6f79 > 100644 >--- a/src/db/sysdb.h >+++ b/src/db/sysdb.h >@@ -1103,4 +1103,10 @@ errno_t sysdb_gpo_get_gpo_result_setting(TALLOC_CTX >*mem_ctx, > const char *policy_setting_key, > const char **policy_setting_value); > >+errno_t sysdb_get_sids_of_members(TALLOC_CTX *mem_ctx, >+ struct sss_domain_info *dom, >+ const char *group_name, >+ char ***_sids, >+ char ***_dns, >+ size_t *_n); > #endif /* __SYS_DB_H__ */ >diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c >index >998046a2ca1c746b2032f430e5f9c4a7151e1dbc..7ad8816ba93e45e998ce277ca9dc672d5fe5c39e > 100644 >--- a/src/db/sysdb_ops.c >+++ b/src/db/sysdb_ops.c >@@ -3630,3 +3630,93 @@ errno_t sysdb_search_object_by_uuid(TALLOC_CTX *mem_ctx, > return sysdb_search_object_by_str_attr(mem_ctx, domain, SYSDB_UUID_FILTER, > uuid_str, attrs, res); > } >+ >+errno_t sysdb_get_sids_of_members(TALLOC_CTX *mem_ctx, >+ struct sss_domain_info *dom, >+ const char *group_name, >+ char ***_sids, >+ char ***_dns, >+ size_t *_n) >+{ >+ errno_t ret; >+ size_t i, m_count; >+ TALLOC_CTX *tmp_ctx; >+ struct ldb_message *msg; >+ struct ldb_message **members; >+ const char *attrs[] = {SYSDB_SID_STR, NULL }; ^^ Please add space here >+ char **sids = NULL, **dns = NULL; >+ size_t n = 0; >+ >+ tmp_ctx = talloc_new(NULL); >+ if (tmp_ctx == NULL) { >+ return ENOMEM; >+ } >+ >+ ret = sysdb_search_group_by_name(tmp_ctx, dom, group_name, NULL, &msg); >+ if (ret != EOK) { >+ goto done; >+ } >+ >+ /* Get sid_str attribute of all elemets pointed to by group members */ >+ ret = sysdb_asq_search(tmp_ctx, dom, msg->dn, NULL, SYSDB_MEMBER, attrs, >+ &m_count, &members); >+ if (ret != EOK) { >+ goto done; >+ } >+ >+ sids = talloc_array(tmp_ctx, char*, m_count); >+ if (sids == NULL) { >+ ret = ENOMEM; >+ goto done; >+ } >+ >+ dns = talloc_array(tmp_ctx, char*, m_count); >+ if (dns == NULL) { >+ ret = ENOMEM; >+ goto done; >+ } >+ >+ for(i=0; i < m_count; i++) { >+ const char *sidstr; >+ >+ sidstr = ldb_msg_find_attr_as_string(members[i], SYSDB_SID_STR, NULL); >+ >+ if (sidstr != NULL) { >+ sids[n] = talloc_strdup(sids, sidstr); result of sysdb_search_group_by_name is allocated under temporary context and it is not used anywhere else. If there aren't objections from other developers we can use talloc_steal in this case. >+ if (sids[n] == NULL) { >+ ret = ENOMEM; >+ goto done; >+ } >+ >+ dns[n] = talloc_strdup(dns, >+ ldb_dn_get_linearized(members[i]->dn)); If there aren't objections from other developers we can use talloc_steal here as well. >+ >+ if (dns[n] == NULL) { >+ ret = ENOMEM; >+ goto done; >+ } >+ n++; >+ } >+ } >+ >+ if (n == 0) { >+ ret = ENOENT; >+ goto done; >+ } >+ >+ *_n = n; >+ *_sids = talloc_steal(mem_ctx, sids); >+ *_dns = talloc_steal(mem_ctx, dns); >+ >+ ret = EOK; >+ >+done: >+ if (ret == ENOENT) { >+ DEBUG(SSSDBG_TRACE_FUNC, "No such entry\n"); >+ } >+ else if (ret) { ^^^^^^^^^ } else if http://www.freeipa.org/page/Coding_Style#IF_Statements >+ DEBUG(SSSDBG_OP_FAILURE, "Error: %d (%s)\n", ret, strerror(ret)); >+ } >+ talloc_free(tmp_ctx); >+ return ret; >+} >diff --git a/src/providers/ldap/sdap_async_groups.c >b/src/providers/ldap/sdap_async_groups.c >index >8cf7f7ff1d414049f0694c7d2873556fc9dad741..e8c408253a0cc311bb84e749208180fd92b379a9 > 100644 >--- a/src/providers/ldap/sdap_async_groups.c >+++ b/src/providers/ldap/sdap_async_groups.c >@@ -801,6 +801,76 @@ done: > return ret; > } > >+static errno_t >+are_sids_from_same_dom(const char *sid1, const char *sid2, bool *_result) >+{ >+ char *rid1, *rid2; >+ bool result; >+ >+ rid1 = strrchr(sid1, '-'); >+ if (rid1 == NULL) { >+ return EINVAL; >+ } >+ >+ rid2 = strrchr(sid2, '-'); >+ if (rid2 == NULL) { >+ return EINVAL; >+ } >+ >+ result = ((rid1 - sid1) == (rid2 - sid2)) && >+ (strncmp(sid1, sid2, rid1 - sid1) == 0); (rid1 - sid1) is the lenght of "SID header" "SID header" is probably not the right terminology. I was able to find just this definition of sid. The structure of the SID looks like this: S-[Revision]-[IdentifierAuthority]-[SubAuthority0]-[SubAuthority1]-...-[SubAuthority[SubAuthorityCount]](-RID). But let's back to main point It would improve readability if you use constants for length of "SID header" (maybe slength{1,2} lenght{1,2} ... >+ >+ *_result = result; >+ >+ return EOK; >+} >+ >+static errno_t >+retain_extern_members(TALLOC_CTX *mem_ctx, >+ struct sss_domain_info *dom, >+ const char *group_name, >+ const char *group_sid, >+ char ***_userdns, >+ size_t *_nuserdns) >+{ >+ TALLOC_CTX *tmp_ctx; >+ char **sids, **dns; >+ bool same_domain; >+ errno_t ret; >+ size_t i, n; >+ >+ tmp_ctx = talloc_new(NULL); >+ if (tmp_ctx == NULL) { >+ return ENOMEM; >+ } >+ >+ ret = sysdb_get_sids_of_members(tmp_ctx, dom, group_name, &sids, &dns, >&n); >+ if (ret != EOK ) { ^ remove extra space >+ if (ret != ENOENT) { >+ DEBUG(SSSDBG_TRACE_ALL, >+ "get_sids_of_members failed: %d [%s]\n", >+ ret, sss_strerror(ret)); >+ } >+ goto done; >+ } >+ >+ for (i=0; i < n; i++) { >+ ret = are_sids_from_same_dom(group_sid, sids[i], &same_domain); >+ if (ret == EOK && !same_domain) { >+ DEBUG(SSSDBG_TRACE_ALL, "extern member: %s\n", dns[i]); >+ >+ (*_nuserdns)++; >+ *_userdns = talloc_realloc(mem_ctx, *_userdns, char*, *_nuserdns); >+ *_userdns[(*_nuserdns)-1] = talloc_steal(mem_ctx, dns[i]); ^^^^^^^^^ @see Sumit's hint in different mail. >+ } >+ } >+ >+ ret = EOK; >+ >+done: >+ talloc_free(tmp_ctx); >+ return ret; >+} > > /* ==Save-Group-Memebrs=================================================== */ > >@@ -817,6 +887,7 @@ static int sdap_save_grpmem(TALLOC_CTX *memctx, > { > struct ldb_message_element *el; > struct sysdb_attrs *group_attrs = NULL; >+ const char *group_sid; > const char *group_name; > char **userdns = NULL; > size_t nuserdns = 0; >@@ -843,6 +914,28 @@ static int sdap_save_grpmem(TALLOC_CTX *memctx, > } > } > >+ /* This is a temporal solution until the IPA provider is able to >+ * resolve external group membership. >+ * https://fedorahosted.org/sssd/ticket/2522 >+ */ >+ if (opts->schema_type == SDAP_SCHEMA_IPA_V1) { >+ ret = sysdb_attrs_get_string(attrs, SYSDB_SID_STR, &group_sid); >+ if (ret != EOK) { >+ DEBUG(SSSDBG_TRACE_FUNC, "Failed to get group sid\n"); >+ group_sid = NULL; >+ } >+ >+ if (group_sid != NULL) { >+ ret = retain_extern_members(memctx, dom, group_name, group_sid, >+ &userdns, &nuserdns); >+ if (ret != EOK) { >+ DEBUG(SSSDBG_MINOR_FAILURE, >+ "retain_extern_members failed: %d:[%s].\n", >+ ret, sss_strerror(ret)); >+ } >+ } >+ } >+ > ret = sysdb_attrs_get_el(attrs, > opts->group_map[SDAP_AT_GROUP_MEMBER].sys_name, &el); > if (ret != EOK) { >-- >1.9.3 > >_______________________________________________ >sssd-devel mailing list >sssd-devel@lists.fedorahosted.org >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
