On 10 Jul 2014, at 16:38, Pavel Reichl <prei...@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.
> 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@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
>>>>
>>>>
>>> _______________________________________________
>>> 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
>
> <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@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
