vfs_acl_[xattr|tdb] and timestamp in ACL hash

[Jeremy Allison]

On Fri, Dec 18, 2020 at 04:31:33PM +0100, Ralph Boehme wrote:
>Hi Jeremy,
>hi Andrew,
>
>here comes another question related to vfs_acl_xattr.
>
>This is triggered by a customer request who is storing xattrs in some 
>kind of database (external to Samba) and they're trying to leverage 
>xattr deduping.
>
>They have a xattr dedupe feature in their backend such then when to 
>files have the same xattr it's only stored once in the backend.
>
>But when using this with vfs_acl_xattr they stumbled upon the fact 
>that we skew the ACL blob bash with timestamp, so even if two files 
>have an identical ACL, they will have a different timestamp so the 
>overall xattr blob will be different.
>
>        typedef [public] struct {
>                security_descriptor *sd;
>                uint16 hash_type;
>                uint8 hash[64]; /* 64 bytes hash. */
>                utf8string description; /* description of what created
>                                         * this hash (to allow
>                                         * forensics later, if we have
>                                         * a bug in one codepath */
>                NTTIME time;
>                uint8 sys_acl_hash[64]; /* 64 bytes hash. */
>        } security_descriptor_hash_v4;
>
>They can hack around this by applying a vendor specific patch that 
>always sets time to 0.
>
>Afaict the hash should not include the time of the creation of the 
>ACL, the hash should just be over the ACL.
>
>So the question is if we want to change our behaviour?
>
>I wouldn't wanna go up to security_descriptor_hash_v5 that drops the 
>time field, maybe just stay at v4, but set time to 0?
>
>Thoughts?

Yes, along with what Andrew said - as we don't use it just setting
to zero when setting the ACL would work. That would still be
compatible with existing code reading existing ACLs.