[PATCH] passdb: handle UPN in lookup_name correctly

[Ralph Wuerthner]

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?


-- 
Regards

    Ralph Wuerthner