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

[Andrew Bartlett]

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.
> 
> 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...

Andrew Bartlett

-- 
Andrew Bartlett                       https://samba.org/~abartlet/
Authentication Developer, Samba Team  https://samba.org
Samba Developer, Catalyst IT          
https://catalyst.net.nz/services/samba