[PATCH] zfsacl: Fix ACL behavior issues with vfs_zfsacl

[Ralph Boehme]

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