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.
Regards,
Pavel Reichl
On Tue, 2014-07-08 at 09:57 +0200, Sumit Bose wrote:
> On Mon, Jul 07, 2014 at 10:25:23PM +0200, Jakub Hrozek wrote:
>> On Mon, Jul 07, 2014 at 07:03:01PM +0200, Pavel Reichl wrote:
>>> On Mon, 2014-06-23 at 11:41 +0200, Sumit Bose wrote:
>>>> On Mon, Jun 23, 2014 at 11:20:52AM +0200, Jakub Hrozek wrote:
>>>>> On Tue, Jun 17, 2014 at 06:30:31PM +0200, Pavel Reichl wrote:
>>>>>> Hello,
>>>>>>
>>>>>> please see attached patch.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> PR
>>>>>
>>>>> The patch solves the problem, but I think one part should be
improved:
>>>>>
>>>>>> @@ -875,7 +893,13 @@ static void
sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
>>>>>> domain =
find_subdomain_by_sid(get_domains_head(state->domain), sid);
>>>>>> if (domain == NULL) {
>>>>>> DEBUG(SSSDBG_MINOR_FAILURE, "Domain not found
for SID %s\n", sid);
>>>>>> - continue;
>>>>>> + if (state->domain->parent == NULL &&
>>>>>> + state->domain->subdomains == NULL) {
>>>>>> + domain = state->domain;
>>>>>> + DEBUG(SSSDBG_TRACE_FUNC, "Using domain
%s\n", domain->name);
>>>>>> + } else {
>>>>>> + continue;
>>>>>> + }
>>>>>> }
>>>>>
>>>>> I think this is a bit dangerous. I wonder if we should have some
>>>>> modification of find_subdomain_by_sid that would return the first
>>>>> configured domain if no subdomain provider was configured or if no
>>>>> domains had a SID. This could be a separate function.
>>>>
>>>> This sounds even more dangerous to me.
>>>>
>>>>>
>>>>> 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 sufficient
solution?
> bye,
> Sumit
>
>>
>>>> which is the case for the IPA and AD provider.
>>>>
>>>
>>>> What about adding an explicit check if the running id provider is the
>>>> plain LDAP provider?
>>> Would that be acceptable with you Jakub?
>>
>> It's a bit hackish but I don't see any other way.
>>
>>>
>>>> As an alternative the LDAP provider can add a
>>>> special value in the id member of the sss_dom_info struct and then
>>>> find_subdomain_by_sid can handle this case specially?
>>>>
>>>> bye,
>>>> Sumit
>>>>
>>>>> _______________________________________________
>>>>> sssd-devel mailing list
>>>>> sssd-devel(a)lists.fedorahosted.org
>>>>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>> _______________________________________________
>>>> sssd-devel mailing list
>>>> sssd-devel(a)lists.fedorahosted.org
>>>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>
>>>
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel(a)lists.fedorahosted.org
>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
<0001-LDAP-tokengroups-do-not-work-with-id_provider-ldap.patch><0002-SDAP-Continue-resolving-SID-even-if-some-fail.patch>_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel