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

[Andrew Bartlett]

On Tue, 2012-10-16 at 11:28 -0400, simo wrote:
> On Tue, 2012-10-16 at 13:49 +1100, Andrew Bartlett wrote:
> > On Sat, 2012-10-13 at 16:27 -0400, simo wrote:
> > > On Sat, 2012-10-13 at 15:09 +1100, Andrew Bartlett wrote:
> > 
> > 
> > > Well, the one way to do it without 2 hashes is to keep around the old
> > > mapping method in case a change is needed and use that as a fallback
> > > to
> > > check the validity of the hash.
> > 
> > The reason that this area concerns me is that it is impractical to keep
> > a copy of the previous POSIX -> NT mapping code around, because it isn't
> > a simple algorithm embodied in one function, but a quite large body of
> > code with interactions with other subsystems including idmap. 
> 
> It looks like this is a problem are then, however it was just to show
> there are other options, I am not saying we need to go there.
> 
> > > > As this module was specifically to avoid (in the normal case) returning
> > > > the POSIX -> NT mapping to the client, my argument is that we should be
> > > > able to, if need by improve that mapping without invaliding the hash. 
> > > 
> > > This is what I have a problem with. I think that if you change the
> > > mapping you ought to change the ACL on the file, as that is the
> > > authoritative one. You can detect and fix the ACL on the fs instead of
> > > doing a lossy Posix -> NT ACL, that is ok.
> > > 
> > > > If your concern is that changes in idmap values should also invalidate
> > > > the hash, then it would not be difficult to include a table of idmap ID
> > > > -> SID mappings in the hash. 
> > > 
> > > No we can't. My concern is that we add more data here and on a lot of
> > > file systems EAs are limited in size (we already have limitations on the
> > > size of the NT ACL we can store).
> > 
> > It would simply be extra data on the input to the sha256 hash, not extra
> > data in the xattr. 
> 
> Ok this was my main concern, increase data size.
> However it is not clear to me what's the point of hashing stuff beyond
> the actual ACL, how do you know what changed then ?

In attempting to address what I thought were your concerns, I was
thinking to hash ID MAP info, but would prefer not to include anything
more then ACL/user/group/mode. 

> Assume you do a 'compatible' change in one of the other things you hash
> together with the ACL blob, you then couldn't tell if what changes it he
> actual ACL or the additional info.

Indeed.  To fully pick out 'compatible' changes we can't use a hash at
all. 

> Can you please send a brief document with the exact algorithm and data
> you envision to use ?

At this stage the proposal is simply:

SHA256(ACL BLOB, user, group, mode). 

> > > To be clear I do not have a concern about storing the Posix ACL hash,
> > > that's fine, but I have a concern in what you seem to imply, that if the
> > > Posix ACL hash matches we are not going to change the ACL even if the
> > > mapping changed
> > 
> > To which mapping do you refer?
> > 
> > NT -> POSIX or POSIX -> NT?
> 
> NT -> Posix of course. We all know Posix -> NT is lossy and we have
> acl_xattr to do that.
> 
> > If the NT -> POSIX mapping changes, we now have the data to reset the
> > posix ACL, which the kernel will use.
> 
> Right, so is part of your plan to refresh the ACL when this situation is
> detected ?

No.  There is no way to do this at the moment, and there is no plan to
change that right now.  This is a different problem to the problem I am
trying to solve. 

However, as a helpful side-effect, this module will provide some data an
external tool could use to improve this situation, and allow a pass over
the filesystem to fix such ACLs.

> > If the POSIX -> NT mapping changes, we now have the data to continue to
> > ignore the result of this mapping, as the administrator requested (by
> > choosing to invoke vfs_acl_xattr). 
> 
> As long as we change the Posix ACL if the NT -> Posix mapping changes
> this would be fine of course.

As I'm not proposing to change anything about when the posix ACL is
changed, then I hope we can agree on this point. 

> > > And this is why I am concerned about having 2 hashes, it will be easy to
> > > use them interchangeably and 'forget' to check if the mapping still
> > > matches, and if there are changes to be made on disk.
> > 
> > I'm not using them interchangeably.  Indeed, I'm proposing to use only
> > the system (posix) ACL hash.  The hash of the NT ACL is kept only for
> > redundancy. 
> 
> Ok can you explain what it would be used for ? If it is not used I would
> rather go all the way and eliminate it.

I would like to, for example, support a situation where the posix ACL
hash appears to have changed, but hasn't really.

It's a belts and braces thing - giving us two chances, now and in the
future, to match the ACL.  

That change could have come from moving between filesystems and
matchings VFS modules (say GPFS to ext4) for example.  It seems a
worthwhile use of 32 bytes, and allows us to safely revert to the old
approach if a serious issue is found in this new scheme.

If we must loose bytes here, then I would rather start by removing the
NTTIME timestamp I proposed, or trimming both hash types back to SHA1. 

> > > I hope this explains my concern.
> > 
> > I'm still horribly lost as to the specific concerns you have.  I'm not
> > sure we can resolve them, as some of them go to the fundamental design
> > and purpose of the module (that the posix -> NT ACL mapping can be
> > disregarded, as long as we know the posix ACL and associated metadata
> > hasn't been changed).  
> > 
> > At this stage, I'm not really sure how to proceed.  Could you prepare a
> > patch with the changes you would like to see?
> > 
> > Otherwise, I can only propose not to proceed, as I don't seem to have
> > any way to resolve your concerns, and pending any actual change to the
> > POSIX -> NT ACL mapping I can live with what we have for now.  
> 
> Main concerns (for me) were:
> - size of xattr
> - changing posix acl if mapping changes
> 
> I also have the concern that this is too posix ACL specific, I would
> like a scheme that is compatible with any arbitrary ACLs, but it seem
> that this is taken care of by using a blob from the VFS module and
> treating it as an opaque structure to hash, so assuming nothing else in
> your proposal makes assumption on what is the ACL on disk or what
> mapping is used (I assume a vfs module that store a different type of
> acl will be able to replace the whole mapping).

The scheme is really meant to be for arbitrary ACL schemes, and I've
tried hard not to encode anything specific to one ACL scheme in it. 

I'll follow up with Christian's note however that all ACL types we have
seen so far would benefit from the user/group/mode being added to the
hash, so that will be put in common. 

Thanks,

Andrew Bartlett

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