[PATCH] passdb: handle UPN in lookup_name correctly

[Ralph Wuerthner]

On 15.02.19 17:23, Rowland Penny wrote:
> On Fri, 15 Feb 2019 17:06:04 +0100
> Ralph Wuerthner via samba-technical <samba-technical at lists.samba.org>
> wrote:
> 
>> On 13.02.19 16:18, Andreas Schneider wrote:
>>> On Tuesday, February 12, 2019 1:38:09 PM CET Ralph Wuerthner via
>>> samba- technical wrote:
>>>> Hi Andreas!
>>>>
>>>> On 12.02.19 10:39, Andreas Schneider wrote:
>>>>> On Monday, February 11, 2019 5:39:33 PM CET Ralph Wuerthner wrote:
>>>>>> Hi Andreas!
>>>>>>
>>>>>> On 11.02.19 16:07, Andreas Schneider wrote:
>>>>>>> Hi Ralph,
>>>>>>>
>>>>>>>> Please see attached patchset:
>>>>>>>> The fix for Samba bugzilla 13312 (commit 1775ac8aa4) caused a
>>>>>>>> regression
>>>>>>>> when looking up names in UPN notation: Because
>>>>>>>> winbind_lookup_name is called with lp_workgroup as domain name
>>>>>>>> the lookup is now failing and the SID for an unmapped Unix
>>>>>>>> user is returned by lookup_name. Fixed by calling
>>>>>>>> winbind_lookup_name with an empty domain name in case the name
>>>>>>>> is in UPN notation.
>>>>>>>>
>>>>>>>> The patchset already passed a CI run:
>>>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_samba-2Dteam_devel_samba_pipelines_46689980&d=DwICAg&c=jf_iaSHvJObTbx-siA1ZOg&r=91G8BCIr_Gua7QkxI_O-dlz8T-mXwIVnjFb7EjeIK7M&m=cE08ROyNiXvMje49eaTVj5qguLqqeNc9L0waXJZlLU8&s=HzsOgBRt8y3O1Jco7WX-FCBZUY8W7L24Vwu5jmYeGP8&e=
>>>>>>>
>>>>>>> Thanks for your contribution!
>>>>>>>
>>>>>>> Please use DGB_DEBUG() instead of DEBUG(10, ...)
>>>>>>>
>>>>>>> In lookup_upn() use a helper variable 'bool ok'. And check
>>>>>>> talloc_strdup()
>>>>>>> for NULL.
>>>>>>
>>>>>> Thanks for your feedback! I prepared a new version of the
>>>>>> patchset with the following changes:
>>>>>> - using a helper variable in lookup_upn()
>>>>>> - use DBG_DEBUG() instead of DEBUG(10, ...)
>>>>>>
>>>>>> I didn't add a NULL check for talloc_strdup() because there is
>>>>>> already a NULL check right after the ok: label. This check is
>>>>>> used by other sequence steps in lookup_name() too.
>>>>>
>>>>> But then the function would need documentation that the caller is
>>>>> responsible for the NULL check.
>>>>>
>>>>> I think this is strange code and we should do the check in place
>>>>> and not defer it to later.
>>>>
>>>> Ahh, I got your point. I thought you were referring to the
>>>> talloc_strdup() call from patch #2 in lookup_name(), instead you
>>>> are referring to the talloc_strdup() call in lookup_upn(). You are
>>>> right, there should be a check in lookup_upn(). I didn't pay much
>>>> attention to this because patch #3 is elimination the
>>>> talloc_strdup() call in lookup_upn(). Please see the attached
>>>> patchset with the missing check (though it will be gone after
>>>> applying patch #3).
>>>
>>> This looks good. Now we need tests which verify this :-)
>>
>> This will be the harder part...
>>
>> I tried to recreate this in the ad_member test environment but
>> without success. Samba's AD DC seems to behave slightly different
>> than a Windows AD server. With or without my patchset I get the same
>> results:
>>
>> ./bin/rpcclient 127.0.0.29 -U% -c 'lookupnames "alice at ADDOMAIN"'
>> alice at ADDOMAIN S-1-5-21-2858285458-1632643436-2480850723-1106 (User:
>> 1)
>>
>> But when doing this on my test system (without my patchset) I get a
>> Unix SID when querying a Samba AD member:
>>
>> rpcclient localhost -U'virtual1\administrator%*****' -c 'lookupnames
>> "testuser1 at virtual1.com"'
>> testuser1 at virtual1.com S-1-22-1-12001118 (User: 1)
>>
>> and the expected SID when querying the AD server directly:
>>
>> rpcclient 10.0.100.8 -U'virtual1\administrator%*****' -c 'lookupnames
>> "testuser1 at virtual1.com"'
>> testuser1 at virtual1.com S-1-5-21-3478218634-1770281059-659661689-1118
>> (User: 1)
>>
>> Do we have to change the behavior of the Samba AD DC or what's the
>> way forward here?
>>
>>
> 
> Could there be something wrong with your test system:
> 
> rowland at devstation:~$ rpcclient localhost -U'samdom\administrator%xxxxxxxxxx' -c 'lookupnames "rowland at samdom.example.com"'
> rowland at samdom.example.com S-1-5-21-1768301897-3342589593-1064908849-1107 (User: 1)
> rowland at devstation:~$ rpcclient 192.168.0.6 -U'samdom\administrator%xxxxxxxxxx' -c 'lookupnames "rowland at samdom.example.com"'
> rowland at samdom.example.com S-1-5-21-1768301897-3342589593-1064908849-1107 (User: 1)
> 
> devstation is a Unix domain member
> 192.168.0.6 is a Samba AD DC

Maybe I was not precises enough above: when querying a Samba domain 
member joined to a Samba AD DC everything is fine. But when querying a 
Samba domain member joined to a Windows AD server I get a Unix SID.

-- 
Regards

    Ralph Wuerthner