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

[Andreas Schneider]

On Thursday 30 January 2014 21:59:18 Andrew Bartlett wrote:
> On Thu, 2014-01-30 at 09:12 +0100, Andreas Schneider wrote:
> > On Thursday 30 January 2014 10:16:44 you wrote:
> > > 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
> > > > > _use
> > > > > r
> > > > 
> > > > 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?
> > 
> > Yes, !ok means winbind is not running and we try to create the group sid
> > from the gid from the passwd struct.
> > 
> > I've added comments to the code.
> 
> This is great.
> 
> > > Also, can you just fix up the commit message with
> > > https://git.samba.org/?p=asn/samba.git;a=commitdiff;h=cac409f521ec6d2229
> > > f1bf 38cb306bbc1eb166e8 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?
> > 
> > I've improved the commit message.
> 
> Did you push that?  I can't see the changed commit message in your
> branch.

I've added a comment to the top commit too and pushed it again.


	-- andreas


-- 
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             asn at samba.org
www.samba.org