selftest: re-enable nss_winbind via nss_wrapper in the test-envs.

[Andrew Bartlett]

On Wed, 2015-02-18 at 09:43 +0100, Michael Adam wrote:
> On 2015-02-18 at 10:09 +1300, Andrew Bartlett wrote:
> > On Tue, 2015-02-17 at 21:17 +0100, Michael Adam wrote:
> > > On 2015-02-18 at 07:27 +1300, Andrew Bartlett wrote:
> > > > On Tue, 2015-02-17 at 18:32 +0100, Michael Adam wrote:
> > > > > 
> > > > > More concretely, the attached patch..
> > > > 
> > > > This is much more like what I expected the fix to be (changing NSS
> > > > mappings might change some aspect of the stored posix ACL), but why does
> > > > the permission change from 6 to 7 (adding execute?)
> > > 
> > > I don't really have a clue. I had hoped someone here has. :)
> > > The only idea I have is that the domain admin now having an
> > > nss entity with the same UID Number as the calling user (root),
> > > this might be related to the domain admin being member of the
> > > builtin administrators group which gets full access by the acls.
> > > Not sure what the acl mapping code really does here.
> > > I'd need to find some time to dive through it.
> > > 
> > > Since the environment is *now* as it should be (with
> > > libnss_winbindd active), I think we can as well push
> > > the patch for a start.
> > 
> > If we can't get to the bottom of it during this thread, I would rather
> > we reverted the original patch until we do.  This area *is* tricky, and
> > the reason I added all the tests wasn't because I fully understood it
> > either, but because I wanted to nail down the behaviour so we noticed if
> > we changed it.  Previously we never noticed at all, it could change
> > unintentionally as happened here, and that worried me.
> 
> I don't think I agree to revert the original patch:
> Reading to the bottom of what you are saying, the test
> was originally created by comparing what came out of
> the NT->POSIX acl conversion and hard-wiring that into
> the checks. Now that we fixed the environment to more
> closely reflect a real setup (add missing nss_winbind),
> we adapted the test the same way. My guess is that you
> would have come up with the exact same test in the first
> place had the environment been correct when you did it.
> 
> So my approach would be to take this patch now, and then
> possibly try to understand afterwards what was going on.
> Reverting back to a broken environment in order to keep
> a test that is broken because it reflects the broken
> environment is not a good option imho. :-)

I don't see the way were running things before as broken - many sites do
run without nss_winbindd on the DC, as we strongly discourage using it
as a general purpose file server. 

What I don't agree with is leaving the change and hoping that we may
'possibly try to understand afterwards what was going on'.  We are all
busy, and I fear that just means accepting the change without any
assurance of a proper investigation.  

There may well be something funny going on here - the purpose of the
test was to highlight such things, and it has done so.  We shouldn't
'just fix it so it passes'.  

Can you please investigate and explain to me why the NSS change made a
difference here (it isn't meant to), fix that, and then run the tests
with and without nss_winbind?

Thanks,

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