Another user at realm type issue/bug

Wed Oct 12 21:41:00 UTC 2016

On Wed, 2016-10-12 at 08:13 +0200, Andreas Schneider wrote:
> On Wednesday, 12 October 2016 07:01:26 CEST Stefan Metzmacher wrote:
> > Am 12.10.2016 um 00:30 schrieb Andrew Bartlett:
> > > On Wed, 2016-10-05 at 08:47 +0200, Stefan Metzmacher wrote:
> > > > Am 03.10.2016 um 19:42 schrieb Jeremy Allison:
> > > > > On Mon, Oct 03, 2016 at 10:30:28AM -0700, Jeremy Allison
> > > > > wrote:
> > > > > > On Mon, Oct 03, 2016 at 02:31:39PM +0200, Andreas Schneider
> > > > > > 
> > > > > > wrote:
> > > > > > > On Friday, 30 September 2016 07:07:38 CEST Andrew
> > > > > > > Bartlett
> > > > > > > 
> > > > > > > wrote:
> > > > > > > > What was the consumer in this case?
> > > > > > > > 
> > > > > > > > While very strange, this was deliberate, as it was
> > > > > > > > expected
> > > > > > > > that the
> > > > > > > > callers would try and get the principal if that was set
> > > > > > > > at a
> > > > > > > > more
> > > > > > > > certain level (eg SPECIFIED compared to GUESS).
> > > > > > > > 
> > > > > > > > The reason is that if I have a UPN of andrew.bartlett at s
> > > > > > > > amba.e
> > > > > > > > xample.com
> > > > > > > > 
> > > > > > > >  I may have a username of abartlet in samAccountName,
> > > > > > > > and so
> > > > > > > > 
> > > > > > > > logging in
> > > > > > > > over NTLM with andrew.bartlett wouldn't match, I would
> > > > > > > > have
> > > > > > > > to use andr
> > > > > > > > ew.bartlett at samba.example.com without a domain.
> > > > > > > > 
> > > > > > > > Naturally, see bugs around that handling server-side,
> > > > > > > > but
> > > > > > > > that was the
> > > > > > > > idea, and it was hoped that very few codepaths would be
> > > > > > > > asking for
> > > > > > > > either directly, hopefully only the gensec modules and
> > > > > > > > the
> > > > > > > > client SMB1
> > > > > > > > NTLM session setup code.
> > > > > > > > 
> > > > > > > > This is why the patch to make the s3 session setup code
> > > > > > > > take
> > > > > > > > cli_credentials (and so pass that down to NTLMSSP and
> > > > > > > > krb5)
> > > > > > > > is so
> > > > > > > > important.
> > > > > > > > 
> > > > > > > > I hope this clarifies things, and reminds me that I
> > > > > > > > should
> > > > > > > > write a good
> > > > > > > > python testsuite to encode these expectations.
> > > > > > > 
> > > > > > > As this parses a string obtained from the commanline with
> > > > > > > -U we
> > > > > > > should set
> > > > > > > username here! If you do not want to do that you should
> > > > > > > not use
> > > > > > > that function
> > > > > > > and call the function to set username directly! On the
> > > > > > > commandline there is
> > > > > > > only one option to set the username/principal and that is
> > > > > > > -U!
> > > > > > 
> > > > > > Yep, I have to agree. This is being called with
> > > > > > CRED_SPECIFIED
> > > > > > from the command line means "shut up and do as I say here".
> > > > > > The
> > > > > > patch is correct IMHO to fix the command line handling.
> > > > > 
> > > > > So if it doesn't break anything in current make test
> > > > > I plan to push. If we need a test around differing
> > > > > samAccountName and UPN behavior we should add that
> > > > > and a separate function to use it IMHO. The pre-patch
> > > > > behavior is too strange to remain for general command
> > > > > line processing.
> > > > 
> > > > Please hold on on this.
> > > > 
> > > > I'm also looking at this currently and I think the topic is
> > > > much
> > > > more complex.
> > > 
> > > Thanks metze!
> > > 
> > > I've not forgotten this either, and I'm currently writing a new
> > > test
> > > for the credentials python bindings to help lock down the current
> > > behaviour.
> > 
> > One important thing is that we need support
> > 
> > -U'MicrosoftAccount\user at example.com' and that
> > should result in domain "MicrosoftAccount" and
> > username "user at example.com", I'm unsure about the value
> > for principal. (Maybe principal and realm should be both NULL,
> > so that we don't try kerberos)
> > For NTLMSSP we need to send "MicrosoftAccount" as Domain.
> > 
> > and
> > 
> > -U'user at example.com' should result in domain = NULL (or ""?),
> > username = NULL and principal = "user at example.com", realm needs to
> > be
> > unchanged (the value from smb.conf), after a successful kinit
> > we need to update realm (and maybe principal) to reflect
> > the values we got from the kdc.
> > For NTLMSSP we need to send "" as Domain.
> > 
> > 
> > The other thing is that with 'smbclient -k -Uuser' or
> > 'smbclient -k -Uuser at example.com' we need to check that the
> > ccache values match the given values.
> > For these cases and the 'smbclient -k' case we
> > need to pickup the principal and realm from the ccache
> > and invalidate username and domain.
> 
> To reflect that, I've started to implement this here:
> 
> https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/master
> -libsmb-ok

What about using cli_credentials_parse_string() instead of checking for
an @ in the username?

Wouldn't that mean we never put the @ into set_username() and avoid
this issue?

Attached are some extensions to our testsuite to try and cover the
expected behaviour here. 

(Please get my explicit OK on this patch set before it goes in.  I'm
incredibly excited, but just want to make sure we get this right,
and/or agree on the new semantics). 

Thanks,

Andrew Bartlett

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-pycredentials-Add-bindings-for-get-set-_principal-ge.patch
Type: text/x-patch
Size: 3175 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20161013/2a521959/0001-pycredentials-Add-bindings-for-get-set-_principal-ge.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-credentials-Add-test-for-credentials-behaviour.patch
Type: text/x-patch
Size: 5927 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20161013/2a521959/0002-credentials-Add-test-for-credentials-behaviour.bin>