[PATCH 1/2] s3-auth: fix force user for AD user

[Andrew Bartlett]

On Thu, 2014-01-30 at 09:40 +1300, Andrew Bartlett wrote:
> On Wed, 2014-01-29 at 17:33 +0100, Andreas Schneider wrote:
> > On Wednesday 22 January 2014 10:25:46 Andrew Bartlett wrote:
> > > I still don't understand/see how it addresses the code paths I was
> > > concerned about, so I think the way to best address that and to keep
> > > this working is to add an automated test for them.  That is, one for
> > > plaintext passwords and then one for the case you are fixing (ktest
> > > covers the kerberos case that worried me, which assuming this passes a
> > > make test improves my confidence considerably).  I realise it may be
> > > hard to fully test given the limitations of the non-root environment,
> > > but at the very least have it walk over the code paths.
> > 
> > Hi Andrew,
> > 
> > I'm sorry, but I'm not able to trigger the codepath you're concerned about at 
> > all, even in master!
> > 
> > The reason is that the plaintext password in the user struct is always set to 
> > NULL passed to pass_check() in source3/auth/auth_unix.c
> > 
> > 
> > [2014/01/29 17:28:28.495413, 100, pid=10495, effective(0, 0), real(0, 0), 
> > class=auth] ../source3/auth/pass_check.c:618(pass_check)
> >   checking user=[asn] pass=[(null)]
> 
> You would also need 'encrypt passwords = no'. 
> 
> > [global]
> >         workgroup = LEVEL1
> >         security = user
> >         map to guest = Bad User
> >         logon path = \\%L\profiles\.msprofile
> >         logon home = \\%L\%U\.9xprofile
> >         logon drive = P:
> >         usershare allow guests = Yes
> > 
> > 
> >         #log file = /var/log/samba/log.%m
> >         max log size = 0
> >         log level = 100
> >         debug pid = yes
> > 
> >         client plaintext auth = yes
> >         passwd chat debug = Yes
> 
> >         auth methods = unix
> 
> You shouldn't need that once we set 'encrypt passwords = no'.
> 
> > [test]
> >         path = /srv/samba/test
> >         writeable = Yes
> > 
> > 
> > I would also argue that 'force user' is a more common used feature of the 
> > Samba file server than 'auth methods = unix' with plaintext passwords.
> 
> Certainly.  I was just trying not to break one while fixing the other. 
> 
> > > I suggest start by copying the simpleserver environment, and split
> > > auth_unix into a wrapper of auth_passwd and auth_pam, so you can set
> > > "auth_methods = auth_passwd" to test plaintext.  (Or successfully
> > > propose ditching plaintext, but I tried and failed to do this).
> > 
> > 
> > Sorry, I don't have time for that. 
> 
> OK.
> 
> > I guess removing plaintext passwords would 
> > be more appropriate.
> > 
> > The patchset is here:
> > 
> > https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/force_user
> 
> If you can re-test the plaintext case manually with the suggestion I
> included, and in the meantime I'll look over this again.

A re-reading helped me understand why my other concerns (aside from the
need for testing) were unfounded:

Can you add some comments here:

+       ok = winbind_lookup_usersids(tmp_ctx,
+                                    &user_sid,
+                                    &num_sids,
+                                    &user_sids);
+       if (ok) {
+               if (num_sids > 0) {
+                       group_sid = user_sids[0];
+               }
+       } else {
+               gid_to_sid(&group_sid, pwd->pw_gid);
+       }
+
+       ok = !is_null_sid(&group_sid);
+       if (!ok) {
+               status = NT_STATUS_NO_SUCH_USER;
+               goto done;
+       }

To indicate how 'ok' saves us from the case when we have no winbind?
That is, that ok == false is quite a reasonable state, and simply means
we are not using winbind?

Also, can you just fix up the commit message with 
https://git.samba.org/?p=asn/samba.git;a=commitdiff;h=cac409f521ec6d2229f1bf38cb306bbc1eb166e8 to be 'use passwd_to_SamInfo3' and to explain that this avoids issues with pushing winbind users via the passdb layer, which simply isn't designed for remote users?

Thanks!

Please do the test I mentioned above, but in anticipation of that and
the small changes I suggest above and in thanks for the incredible
patience you have shown:

Reviewed-by: Andrew Bartlett <abartlet at samba.org>

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