[PATCH] zfsacl: Fix ACL behavior issues with vfs_zfsacl
Ralph Boehme
slow at samba.org
Fri May 17 05:44:26 UTC 2019
On Sat, May 11, 2019 at 03:19:48PM -0400, Andrew Walker via samba-technical wrote:
>This patch addresses two problems that we've seen with ZFS / samba users
>for a while.
>1) It's not possible in Windows explorer to disable inheritance. I've
>introduced a new zfsacl parameter "zfsacl:map_dacl_protected" to allow this.
>
>2) If admins remove all special aces (owner@, group@, everyone@), then ZFS
>will automatically append them to the ACL of newly created subdirectories /
>files. In this case, it's just default ZFS inheritance behavior in the
>absence of inheritable special entries. I've introduced a new parameter
>"zfsacl:block_zfs_acl_chmod" to block this behavior. It does so by adding /
>maintaining a hidden empty inheriting everyone@ ACL entry "everyone@
>::fd:allow".
>
>I believe they are both necessary to avoid POLA violations for Windows
>admins, but have made them default to off (so that we don't affect existing
>install base). I'm happy to make any changes you suggest.
thanks for working on this. I thing I've seen one of the issues described myself. :)
A few comments:
- commit message is all in one line
- this is a hot code path and parmetric options are expensive, can you please use a
configuration object, the existing parametric option denymissingspecial should
be converted to the config object option while at it. Cf other VFS modules on how
to do it, eg gpfs or fruit
- please check mixed space/tabs indentation
- please check trailing space
- pleaseuse common struct initializer:
SMB_ACE4PROP_T hidden_ace;
if (S_ISDIR(fsp->fsp_name->st.st_ex_mode)) {
hidden_ace.flags = SMB_ACE4_ID_SPECIAL;
hidden_ace.who.id = SMB_ACE4_WHO_EVERYONE;
hidden_ace.aceType = SMB_ACE4_ACCESS_ALLOWED_ACE_TYPE;
hidden_ace.aceFlags = (SMB_ACE4_FILE_INHERIT_ACE|SMB_ACE4_DIRECTORY_INHERIT_ACE);
hidden_ace.aceMask = 0;
DBG_DEBUG("zfsacl: setting empty everyone@ ace on dir %s \n", fsp->fsp_name->base_name);
} else {
hidden_ace.flags = SMB_ACE4_ID_SPECIAL;
hidden_ace.who.id = SMB_ACE4_WHO_EVERYONE;
hidden_ace.aceType = SMB_ACE4_ACCESS_ALLOWED_ACE_TYPE;
hidden_ace.aceFlags = 0;
hidden_ace.aceMask = 0;
DBG_DEBUG("zfsacl: setting empty everyone@ ace on file %s \n", fsp->fsp_name->base_name);
}
rewrite as:
SMB_ACE4PROP_T hidden_ace = (SMB_ACE4PROP_T) {
hidden_ace.flags = SMB_ACE4_ID_SPECIAL;
hidden_ace.who.id = SMB_ACE4_WHO_EVERYONE;
hidden_ace.aceType = SMB_ACE4_ACCESS_ALLOWED_ACE_TYPE;
};
if (S_ISDIR(fsp->fsp_name->st.st_ex_mode)) {
hidden_ace.aceFlags = (SMB_ACE4_FILE_INHERIT_ACE|SMB_ACE4_DIRECTORY_INHERIT_ACE);
}
DBG_DEBUG("zfsacl: setting empty everyone@ ace on file %s \n", fsp->fsp_name->base_name);
- please split the patch in two patches, each just adding one option
- please add the new options to the manpage
- I don't understand the mechanics of setting the SEC_DESC_DACL_PROTECTED flag
when no inheritable ACEs are present. If ZFS does support the flag, why not
just map the flag from the wire like gpfs? I might be missing something...
- the option name block_zfs_acl_chmod seems a bit misleading. Can we come up
with something else that better describes the intended semantics? Eg
"ignore_empty_mode".
Thanks!
-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG-Fingerprint FAE2C6088A24252051C559E4AA1E9B7126399E46
More information about the samba-technical
mailing list