[PATCH][WIP] Make vfs_acl_xattr use hash of the posix ACL

[Andrew Bartlett]

On Fri, 2012-10-12 at 14:05 -0400, simo wrote:
> On Fri, 2012-10-12 at 17:12 +0200, Volker Lendecke wrote:
> > On Fri, Oct 12, 2012 at 10:26:15PM +1100, Andrew Bartlett wrote:
> > > On Fri, 2012-10-12 at 13:11 +0200, Christian Ambach wrote:
> > > > Hi Andrew,
> > > > 
> > > > On 10/09/2012 01:58 PM, Andrew Bartlett wrote:
> > > > > This patch (still a work in progress, as I know it fails some tests)
> > > > > is what I've been talking about for months, and which I got VFS
> > > > > changes in (almost - one small change in this patch) into 4.0.0rc1.
> > > > >
> > > > > The idea is simple, have the ACL modules (the posix ACL modules in
> > > > > particular) provide a blob form of the ACL, and then hash that,
> > > > > rather than an NT ACL that might change if our ACL mapping code
> > > > > changes.
> > > > 
> > > > I am not sure I am getting the problem you are trying to solve here.
> > > > What is the benefit of adding another representation of an ACL that is
> > > > just intermediate?
> > > > 
> > > > Can you give some more explanations that go further than the commit
> > > > message in your attachment?
> > > 
> > > This goes back to a discussion that we have had on and off over a few
> > > months.
> > > 
> > > What I'm working on is an improved implementation of the hash in
> > > vfs_acl_common.c.  The new hash will be of the 'system' ACL, whatever
> > > that is, rather than the NT ACL it maps to.
> > 
> > Why is hashing the NT ACL not sufficient here?
> 
> I think Andrew is concerned that if we change the way we map from NT ACL
> -> Posix ACL that we will fail the hash check and will start considering
> all ACLs on file as 'modified' and therefore throw away the NT ACL and
> recalculate back from the Posix ACL, even though no ACL changed on the
> file but is the mapping that changed.
> 
> However I have to ask: *if* we do indeed change the mapping so that the
> hash check fail is it really bad to fail the check ?

If what changed is our code, and not administrator or user intent, then
I don't want to invalidate a whole NAS worth of ACLs.  

I'm also concerned by things like the nt4_compatible_acls() call that
changes the returned (and therefore hashed) NT ACL based on the client
OS.  This kind of variability doesn't belong in an on-disk hash.

> I mean if we change the mapping it means we did something wrong, so if
> we keep the NT ACL unchanged and not fail the hash check it means we
> will not show the user that the ACL really is different (from our new
> understanding of how it should be represented).
> 
> The problem has 2 sides:
> On the one hand we want to keep ACLs as close as possible to what
> Windows clients send us, I guess that's why Andrew wants to consider
> them authoritative and hash them.

That is the specific design and purpose of this not-on-by-default
module. 

The most important part of my changes are to give us options in the
future.  Specifically, we store both hash values, a timestamp and a
description of what the blob is, in the hope that when we find we have
to make some incompatible change, we can still keep the old NT ACLs on
disk valid, for upgrading users.  

> On the other hand though, the authoritative ACL from the file access POV
> is the one on the File System as that's what the kernel enforce.
> And if we consider the one on the FS the authoritative one, then it can
> be argued that it is indeed a good idea to break the mapping if the
> Posix ACL -> NT ACL mapping changes and causes the hash check to fail.
> 
> So which PoV should we subscribe to in the Filesystem ?
> Or did I fall for a false dichotomy ?

I think you did.  I hope what I've said above helps explain it better,
but the actual implementation module will be on the list again for all
to review once I sort out the issues with the previous patch and those
that cause this work in progress to fail. 

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org