[SSSD] [PATCH] LDAP: retain external members

Sumit Bose sbose at redhat.com
Fri Dec 12 16:19:00 UTC 2014


On Fri, Dec 12, 2014 at 04:45:13PM +0100, Pavel Reichl wrote:
> 
> On 12/12/2014 04:08 PM, Sumit Bose wrote:
> >On Fri, Dec 12, 2014 at 01:16:29PM +0100, Pavel Reichl wrote:
> >>On 12/11/2014 06:47 PM, Sumit Bose wrote:
> >>>On Thu, Dec 11, 2014 at 06:07:07PM +0100, Pavel Reichl wrote:
> >>>>On 12/10/2014 11:28 PM, Lukas Slebodnik wrote:
> >>>>>On (10/12/14 21:32), Sumit Bose wrote:
> >>>>>>On Wed, Dec 10, 2014 at 05:43:37PM +0100, Lukas Slebodnik wrote:
> >>>>>>>On (10/12/14 11:26), Sumit Bose wrote:
> >>>>>>>>On Wed, Dec 10, 2014 at 10:44:32AM +0100, 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
> >>>>>>>>>Thanks for review.
> >>>>>>>>>Please see updated patch.
> >>>>>>>>>
> >>>>>>>>Thank you, ACK.
> >>>>>>>NACK, sssd crashed with this patch.
> >>>>>>>How to reproduce:
> >>>>>>>sssd with ipa server mode:
> >>>>>>>
> >>>>>>>id admin at rdustv1911.test  //ipa admin
> >>>>>>>id aduser1 at ipaad2012r2.test // aduser
> >>>>>>>id aduser2 at ipaad2012r2.test
> >>>>>>>sss_cache -E
> >>>>>>>id aduser1 at ipaad2012r2.test // aduser
> >>>>>>good catch, thank you. Looks like I always only tested with one external
> >>>>>>member. Does
> >>>>>>
> >>>>>>diff --git a/src/providers/ldap/sdap_async_groups.c
> >>>>>>b/src/providers/ldap/sdap_async_groups.c
> >>>>>>index 43744ac..4bc2aa7 100644
> >>>>>>--- a/src/providers/ldap/sdap_async_groups.c
> >>>>>>+++ b/src/providers/ldap/sdap_async_groups.c
> >>>>>>@@ -861,7 +861,7 @@ retain_extern_members(TALLOC_CTX *mem_ctx,
> >>>>>>
> >>>>>>             (*_nuserdns)++;
> >>>>>>             *_userdns = talloc_realloc(mem_ctx, *_userdns, char*, *_nuserdns);
> >>>>>>-            *_userdns[(*_nuserdns)-1] = talloc_steal(mem_ctx, dns[i]);
> >>>>>>+            (*_userdns)[(*_nuserdns)-1] = talloc_steal(mem_ctx, dns[i]);
> >>>>>>         }
> >>>>>>     }
> >>>>>>
> >>>>>>
> >>>>>>fix the issue for you?
> >>>>>Yet, it fixed.
> >>>>>I could see the mistake in code and I was looking at the code at least fo 10
> >>>>>minutes. Someone should write 100 times I will learn C operator precedence
> >>>>>http://en.cppreference.com/w/c/language/operator_precedence
> >>>>>
> >>>>>>Maybe this should be better written with local variables to avoid this
> >>>>>>kind of issues?
> >>>>>+1
> >>>>>
> >>>>>>Additionally there is a NULL check missing after talloc_realloc().
> >>>>>+1
> >>>>>
> >>>>>LS
> >>>>>_______________________________________________
> >>>>>sssd-devel mailing list
> >>>>>sssd-devel at lists.fedorahosted.org
> >>>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >>>>I'm really sorry about the crash.
> >>>>Please see if the new patch is crash free.
> >>>>
> >>>>Thanks!
> >>>Hi Pavel,
> >>>
> >>>thank you for the new version, I haven't had a close look yet, but I hav
> >>>a nitpick about using the memory contexts ...
> >>>
> >>>...
> >>>
> >>>With the if (userdns == NULL), there is a chance to leak memory. I would
> >>>suggest to following:
> >>>
> >>>>+
> >>>>+    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, const char*, nuserdns);
> >>>                userdns = talloc_realloc(tmp_ctx, userdns, const char*, nuserdns);
> >>>>+            if (userdns == NULL) {
> >>>>+                return ENOMEM;
> >>>>+            }
> >>>>+            userdns[nuserdns-1] = talloc_steal(mem_ctx, dns[i]);
> >>>                userdns[nuserdns-1] = talloc_steal(userdns, dns[i]);
> >>>>+        }
> >>>>+    }
> >>>>+    *_nuserdns = nuserdns;
> >>>        *_nuserdns = talloc_steal(mem_ctx, nuserdns);
> >>>>+    *_userdns = discard_const(userdns);
> >>>>+    ret = EOK;
> >>>>+
> >>>talloc_steal() is not recursive, but with the scheme above the items of
> >>>userdns are children of userdns talloc-wise and stealing userdns on
> >>>mem_ctx does not change it and a talloc_free(userdns) will free userdns
> >>>and all its elements.
> >>>
> >>>bye,
> >>>Sumit
> >>Thank you for patience.  Updated patch attached.
> >Thank you for the new version, it is working as expected and I didn't
> >see a crash neither in IPA clients nor on a server.
> >
> >I think all of Lukas' comments are addressed as well. I just have a
> >minor comment but imo it does not has to be fixed.
> >
> >bye,
> >Sumit
> >
> >>+
> >>+        if (sidstr != NULL) {
> >>+            sids[n] = talloc_steal(sids, sidstr);
> >Currently talloc_steal() only returns NULL if the 2nd argument is NULL
> >and since we know that sidstr != NULL the following check will never be
> >true. But since it is quite cheap it can be left here.
> I think that the check was there forgotten from previous version when
> tallo_strdup was  used.
> >
> >>+            if (sids[n] == NULL) {
> >>+                ret = ENOMEM;
> >>+                goto done;
> >>+            }
> >>+
> >>+            dns[n] = talloc_steal(dns,
> >>+                                  ldb_dn_get_linearized(members[i]->dn));
> >In this case the check is valid because ldb_dn_get_linearized() can
> >return NULL.
> >
> >>+
> >>+            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) {
> >>+        DEBUG(SSSDBG_OP_FAILURE, "Error: %d (%s)\n", ret, strerror(ret));
> >>+    }
> >>+    talloc_free(tmp_ctx);
> >>+    return ret;
> >>+}
> >_______________________________________________
> >sssd-devel mailing list
> >sssd-devel at lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> Thanks for noticing. Updated patch attached.

ACK

bye,
Sumit



More information about the sssd-devel mailing list