[PATCH] passdb: handle UPN in lookup_name correctly

Fri Feb 15 19:51:21 UTC 2019

On Fri, 15 Feb 2019 12:20:19 -0700
Christof Schmitt <cs at samba.org> wrote:

> On Fri, Feb 15, 2019 at 06:07:40PM +0100, Ralph Wuerthner via
> samba-technical 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.
> 
> When using the Samba AD DC, are you using the selftest environment?

Well no, but then if that environment is using something that a normal
domain doesn't. it isn't fit for purpose.

> 
> I did a quick check with master:
> make testenv SELFTEST_TESTENV=ad_member WINBINDD_OPTIONS='-d10'
> to run a selftest with an AD member and winbindd debug logs.
> 
> Test command:
> bin/rpcclient 127.0.0.29 -U% -c 'lookupnames
> administrator at ADDOM.SAMBA.EXAMPLE.COM'
> 
> The winbindd trace shows now that winbindd received the request:
> lookupname ADDOMAIN/administrator at ADDOM.SAMBA.EXAMPLE.COM
> 
> And it a lsa_LookupNames4 to the DC with:
> string                   :
> 'ADDOMAIN\ADMINISTRATOR at ADDOM.SAMBA.EXAMPLE.COM'
> 
> Probably the Samba AD DC accepts this weird syntax, but a Windows DC
> does not. We could look into having the Samba AD DC reject this as
> well.
> 
> Christof

If Windows does something differently, then Samba should be changed to
match.

Rowland