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

[Michael Adam]

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

> > But it still leaves me with a strange feeling:
> > 
> > - Why does provision give root's uid to the domain admin?
> >   This seems to be a bug/bad thing to do.
> 
> So that the kernel gives administrator the kind of powers you would
> expect it to have, like the ability to change ownerships. 

I understand that. But i still don't really like it.

> > - Why does the sysvol ACL not specify ACEs for the owner/
> >   for the domain admin. It seems to be slightly strange
> >   for the test to check the resulting acl for specific
> >   entries for this entity since this seems to be random
> >   to some extent, since apparently external influcences
> >   come into play here?! ...
> 
> The ACLs come from Windows, so the test needs to ensure that given that
> ACL, we have a deterministic result on the sysvol share, given that the
> posix ACL is what the users will end up being restricted by.
> 
> It isn't good that adding an nss module changes it, but we need to
> understand the change, not just codify it as the new normal.  
> 
> Given this, I think it is important we run the test with and without
> nss_winbind, and ensure it doesn't change either way. 

I think that we need to test the normal case first of all.
If we want to test an invalid setup (the one w/o nss_winbind)
too, can be discussed, but is not mandatory for a first
step, imho.

Cheers - Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150218/a8cc59d5/attachment.pgp>