[SSSD] [PATCH] LDAP: tokengroups do not work with id_provider=ldap

Pavel Reichl preichl at redhat.com
Mon Jul 21 06:28:01 UTC 2014


On 07/20/2014 10:05 PM, Jakub Hrozek wrote:
> On Fri, Jul 18, 2014 at 05:10:53PM +0200, Jakub Hrozek wrote:
>> On Thu, Jul 17, 2014 at 05:41:23PM +0200, Pavel Reichl wrote:
>>> On Thu, 2014-07-17 at 07:59 +0200, Jakub Hrozek wrote:
>>>> On 10 Jul 2014, at 16:38, Pavel Reichl <preichl at redhat.com> wrote:
>>>>
>>>>> Hello,
>>>>>
>>>>> please see attached patches.
>>>>>
>>>>> I found out that if we go with approach introduced in previous version
>>>>> (in case of LDAP provider assume SID comes from default domain) this can
>>>>> lead to resolutions of SIDs like S-1-5-32-545 and also SIDs of non-posix
>>>>> groups which in case of disabled id mapping leads to failure which will
>>>>> end request prematurely. 2nd patch should make SID resolution more
>>>>> resilient to handle this.
>>>>>
>>>> The only complaint I have about the code is that the domain NULL check is not needed, I actually think we should fail if there is a NULL domain in the provider code, the provider handler should already take care of finding the right domain. If not, we’re in big trouble anyway.
>>>>
>>>> Otherwise the code solves the problem — I can’t say it looks great. That’s not your fault Pavel, just yet another reminder that we need to work on the LDAP provider refactoring no later than 1.13.
>>>>
>>>> I tested with both id_provider=ldap and =ad using both POSIX attributes and ID mapping. Both worked fine in my testing.
>>>>
>>>> Can you re-send the patches without the domain check? Then I’ll ack.
>>> Sure, attached patches address your and Lukas' comments.
>> They indeed do, ACK
>>
>>> [snip]
>>>>>>>>>> Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
>>>>>>>>>> to find the primary domain.
>>>>>>>>> I think find_subdomain_by_sid() does what the name says and of course it
>>>>>>>>> can return the primary domain as long as the SID of the domain is know
>>>>>>>   ^^^^^^
>>>>>>> fwiw, this was my concern, the function is named "find_subdomain" yet it
>>>>>>> can find both main domain and subdomain. But I won't bikeshed any further.
>>>>>> ah, sorry, now I see your point. I agree that the name misleading but I
>>>>>> think this can be fixed after the release.
>>> Would 's/find_subdomain_by_sid/find_domain_by_sid/' be a sufficient
>>> solution?
>> Yes, but in a separate patch, please.
> Sorry Pavel, I pushed your rename patch, because it was simple and
> applied on origin/master and didn't realize this one should took
> precedence...can you rebase it one last time?
>
> btw it's fine to say patch A depends on patch B, especially if B is
> significantly more complex than A ;_)
Sorry, the patch dependence somehow slipped my mind, I'll try to be more 
careful next time.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140721/5cdfb417/attachment.html>


More information about the sssd-devel mailing list