[SSSD] [PATCH] LDAP: retain external members

Pavel Reichl preichl at redhat.com
Fri Dec 12 15:45:13 UTC 2014


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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-LDAP-retain-external-members.patch
Type: text/x-patch
Size: 8050 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141212/1a7cf62b/attachment.bin>


More information about the sssd-devel mailing list