[PATCH] passdb: handle UPN in lookup_name correctly

Fri Feb 15 22:01:43 UTC 2019

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