[PATCH] passdb: handle UPN in lookup_name correctly

Mon Feb 18 13:03:04 UTC 2019

On 16.02.19 16:26, Rowland Penny via samba-technical wrote:
> On Fri, 15 Feb 2019 22:01:43 +0000
> Rowland Penny via samba-technical <samba-technical at lists.samba.org>
> wrote:
> 
>> On Fri, 15 Feb 2019 14:42:16 -0700
>> Christof Schmitt <cs at samba.org> wrote:
>>
>>> On Fri, Feb 15, 2019 at 09:31:14PM +0000, Rowland Penny via
>>> samba-technical wrote:
>>>> On Fri, 15 Feb 2019 18:07:40 +0100
>>>> Ralph Wuerthner <ralphw at de.ibm.com> wrote:
>>>>
>>>>> 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.
>>>>>
>>>>
>>>> I have been doing further testing (just out of curiosity) with a
>>>> 2012R2 DC and a Samba Unix domain member:
>>>>
>>>> root at fileserver:~# rpcclient 192.168.0.43
>>>> -U'example\administrator%xxxxxxxxxx' -c 'lookupnames
>>>> "rowland at example.com"' rowland at example.com
>>>> S-1-5-21-1739413417-3060112075-1959733387-1104 (User: 1)
>>>>
>>>> fileserver is the Samba Unix domain member
>>>> 192.168.0.43 is the 2012R2 DC
>>>>
>>>> So I repeat, could there be something wrong with the test or the
>>>> Unix domain member ?
>>>
>>> The scenario is that the initial 'rpcclient' call asks for
>>> user at domain.com. That goes to smbd, which sends a request for
>>> domain\user at domain.com to winbindd, which gets forwarded to the DC.
>>>
>>> The Samba DC seems to silently accept this request. I assume that a
>>> Windows DC would reject a lookupname for domain\user at domain.com.
>>> Then the additional fix required would be having the Samba DC also
>>> reject queries for domain\user at domain.com.
>>>
>>> Christof
>>
>> The original post said this was run:
>>
>> rpcclient localhost -U'virtual1\administrator%*****' -c 'lookupnames
>> "testuser1 at virtual1.com"'
>>
>> and produced this:
>> testuser1 at virtual1.com S-1-22-1-12001118 (User: 1)
>>
>> It was later said this was from a Samba Unix domain member against a
>> Windows DC.
>>
>> I run a similar command from a Samba Unix domain member against a
>> Windows 2012DC:
>>
>> rpcclient localhost -U'example\administrator%241155nov*' -c
>> 'lookupnames "rowland at example.com"' and get:
>>
>> rowland at example.com S-1-5-21-1739413417-3060112075-1959733387-1104
>> (User: 1)
>>
>> So if you are saying that the command, when run in a test, doesn't
>> produce the correct SID, then, to me, it looks like the test
>> environment is wrong.
>>
>> Rowland
>>   
>>
> 
> Nope, it isn't the test environment, something changed between 4.5.16
> and 4.7.12
> 
> root at fileserver:~# smbd -V
> Version 4.5.16-Debian
> root at fileserver:~# rpcclient localhost -U'example\administrator%xxxxxxxxxx' -c 'lookupnames "rowland at example.com"'
> rowland at example.com S-1-5-21-1739413417-3060112075-1959733387-1104 (User: 1)
> 
> root at fileserver:~# smbd -V
> Version 4.7.12-Debian
> root at fileserver:~# rpcclient localhost -U'example\administrator%xxxxxxxxxx' -c 'lookupnames "rowland at example.com"'
> rowland at example.com S-1-22-1-11104 (User: 1)

The change is 1775ac8aa4dc0 "winbindd: Do not ignore domain in the 
LOOKUPNAME request". As stated by Christof we would also need a fix in 
the Samba AD DC to match the behavior with a Windows DC. I will try to 
come up with a test case to test "user at domain.com" queries.

-- 
Regards

    Ralph Wuerthner