cli_credentials_parse_name... (Re: [SCM] Samba Shared Repository - branch master updated)

[Alexander Bokovoy]

On to, 05 marras 2020, Andrew Bartlett wrote:
> On Wed, 2020-11-04 at 19:23 +0200, Alexander Bokovoy wrote:
> > On ke, 04 marras 2020, Stefan Metzmacher wrote:
> > > Am 04.11.20 um 17:24 schrieb Alexander Bokovoy:
> > > > The branch, master has been updated
> > > >        via  f9016912098 lookup_name: allow lookup for own realm
> > > >        via  00f4262ed0b cli_credentials: add a helper to parse
> > > > user or group names
> > > >        via  eb0474d27ba cli_credentials_parse_string: fix parsing
> > > > of principals
> > > >       from  a1b021200e3 selftest: add test for new "samba-tool
> > > > user unlock" command
> > > > 
> > > > https://git.samba.org/?p=samba.git;a=shortlog;h=master
> > > > 
> > > > 
> > > > - Log ---------------------------------------------------------
> > > > --------
> > > > commit f901691209867b32c2d7c5c9274eee196f541654
> > > > Author: Alexander Bokovoy <ab at samba.org>
> > > > Date:   Wed Nov 4 14:21:33 2020 +0200
> > > > 
> > > >     lookup_name: allow lookup for own realm
> > > >     
> > > >     When using a security tab in Windows Explorer, a lookup over
> > > > a trusted
> > > >     forest might come as realm\name instead of NetBIOS domain
> > > > name:
> > > >     
> > > >     -------------------------------------------------------------
> > > > -------
> > > >     [2020/01/13 11:12:39.859134,  1, pid=33253,
> > > > effective(1732401004, 1732401004), real(1732401004, 0),
> > > > class=rpc_parse]
> > > > ../../librpc/ndr/ndr.c:471(ndr_print_function_debug)
> > > >            lsa_LookupNames3: struct lsa_LookupNames3
> > > >               in: struct lsa_LookupNames3
> > > >                   handle                   : *
> > > >                       handle: struct policy_handle
> > > >                           handle_type              : 0x00000000
> > > > (0)
> > > >                           uuid                     : 0000000e-
> > > > 0000-0000-1c5e-a750e5810000
> > > >                   num_names                : 0x00000001 (1)
> > > >                   names: ARRAY(1)
> > > >                       names: struct lsa_String
> > > >                           length                   : 0x001e (30)
> > > >                           size                     : 0x0020 (32)
> > > >                           string                   : *
> > > >                               string                   :
> > > > 'ipa.test\admins'
> > > >                   sids                     : *
> > > >                       sids: struct lsa_TransSidArray3
> > > >                           count                    : 0x00000000
> > > > (0)
> > > >                           sids                     : NULL
> > > >                   level                    :
> > > > LSA_LOOKUP_NAMES_UPLEVEL_TRUSTS_ONLY2 (6)
> > > >                   count                    : *
> > > >                       count                    : 0x00000000 (0)
> > > >                   lookup_options           :
> > > > LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES (0)
> > > >                   client_revision          :
> > > > LSA_CLIENT_REVISION_2 (2)
> > > > 
> > > > ...
> > > > 
> > > > diff --git a/auth/credentials/tests/test_creds.c
> > > > b/auth/credentials/tests/test_creds.c
> > > > index d2d3d30d73d..38550d6ecf9 100644
> > > > --- a/auth/credentials/tests/test_creds.c
> > > > +++ b/auth/credentials/tests/test_creds.c
> > > > @@ -187,7 +187,7 @@ static void torture_creds_parse_string(void
> > > > **state)
> > > >  	assert_string_equal(creds->domain, "");
> > > >  	assert_int_equal(creds->domain_obtained, CRED_SPECIFIED);
> > > >  
> > > > -	assert_string_equal(creds->username, "wurst at brot.realm");
> > > > +	assert_string_equal(creds->username, "wurst");
> > > 
> > > I'm sorry but this is wrong!
> > > I'm wondering why this wasn't covered by any high level test.
> > > 
> > > This needs to result in domain="" and username="wurst at brot.realm"
> > > and that's exactly what we need to use for NTLMSSP.
> > > Also note that "brot.realm" may not be a realm and "wurst" may not
> > > be a sAMAccountName. A userPrincipalName can be 
> > > anything at anydomain-of-msDS-SPNSuffixes.
> 
> cli_credentials_get_ntlm_username_domain() does this already.
> 
> > > I fear we need to revert these changes.
> > > From the merge request (
> > > https://gitlab.com/samba-team/samba/-/merge_requests/1658)
> > > I didn't really look at the whole patchset (with behavior change)
> > > I only focused on CRED_NO_PASSWORD.
> > > 
> > > I think we need to logic we have in wb_irpc_lsa_LookupNames4_call()
> > > and/or parse_domain_user() here.

One thing I need for lookup_name() is to be able to lookup those
realm/NetBIOS-qualified requests along with UPN-based queries through
passdb when they are done with LookupNames3 using
LSA_LOOKUP_NAMES_UPLEVEL_TRUSTS_ONLY2 level. We don't see the level
passed, as lookup_name() operates on own flags only.  Without
lookup_name() on DC (FreeIPA DC) being able to lookup UPNs against
passdb, we don't even get the request sent to ipasam. As a result,
Windows UI doesn't resolve IPA users and groups found in Global Catalog.

> > 
> > I'm pushing a revert for now and will look at those.
> 
> I'm not so sure this is totally wrong.  Can I have a look over these
> paths at the office?  I need any possible distraction from US election
> results anyway...

My autobuild failed in 
UNEXPECTED(failure): samba3.smbtorture_s3.plain.OPLOCK-CANCEL.smbtorture(nt4_dc)

Please share your findings on the list or on the MR. I'm going to pick
this up tomorrow and see what could be done. If we all decide it has to
completely reverted then, I'll resubmit the revert in the morning.

-- 
/ Alexander Bokovoy