Add option to sort dacl into canonical order in nfs4_acls

[Christof Schmitt]

On Tue, Aug 27, 2019 at 10:49:19AM -0400, Andrew Walker via samba-technical wrote:
> There are many ways that applications can write NFS4 ACL entries in a
> non-canonical order per MS-DTYP 2.4.5. It would be nice to have the option
> to automatically sort these into canonical order so that Windows doesn't
> complain when viewing these.  I'm honestly a bit torn regarding the best
> path forward with this. It's easy to say "you're doing it wrong - fix your
> ACLs", but I imagine that some admins would want a "stop nagging me" option.
> 
> Example of some operations resulting out-of-order ACEs:
> # file: /mnt/dozer/share/inherited
> # owner: root
> # group: smbuser
>       user:smbuser:rwxpDdaARWcCos:-------:allow
>             owner@:rwxpDdaARWcCos:fd----I:allow
>             group@:rwxpDdaARWcCos:fd----I:allow
>          everyone@:--------------:fd----I:allow
> root at freenas[/mnt/dozer]# setfacl -m u:root:full_set:fd:allow
> /mnt/dozer/share/inherited
> root at freenas[/mnt/dozer]# getfacl /mnt/dozer/share/inherited
> 
> # file: /mnt/dozer/share/inherited
> # owner: root
> # group: smbuser
>          user:root:rwxpDdaARWcCos:fd-----:allow
>       user:smbuser:rwxpDdaARWcCos:-------:allow
>             owner@:rwxpDdaARWcCos:fd----I:allow
>             group@:rwxpDdaARWcCos:fd----I:allow
>          everyone@:--------------:fd----I:allow
> root at freenas[/mnt/dozer]# chmod 777 /mnt/dozer/share/inherited
> root at freenas[/mnt/dozer]# getfacl /mnt/dozer/share/inherited
> # file: /mnt/dozer/share/inherited
> # owner: root
> # group: smbuser
>          user:root:rwxpDdaARWcCos:fd-----:allow
>       user:smbuser:rwxpDdaARWcCos:-------:allow
>             owner@:rwxpDdaARWcCos:fdi---I:allow
>             group@:rwxpDdaARWcCos:fdi---I:allow
>          everyone@:--------------:fdi---I:allow
>             owner@:rwxp--aARWcCos:-------:allow
>             group@:rwxp--a-R-c--s:-------:allow
>          everyone@:rwxp--a-R-c--s:-------:allow


I am not quite familiar with the output here, but i assume this is about
having inherited ACL entries after explicit ACL entries. If all entries
are "allow", then reordering them should not be a problem. On the other
hand, if there are "allow" and "deny" entries, then the order is
important and reordering would change the meaning of the ACL.

What is the problem to solve here? That an administrator changes the ACL
through setfacl while not adhering to the order expected by the Windows
clients?  Ideally, there could be a way in setfacl or even in the file
system to only allow the Windows ACL order. As this is likely not
feasible, maybe the "sort_dacl" option could be restricted to only
reorder if there are only "allow" entries, but skip the reordering if
there are "deny" entries. With that, the meaning of the ACL would not
change. In any case, the behavior should also be documented in the man
pages.

Christof

> diff --git a/source3/modules/nfs4_acls.c b/source3/modules/nfs4_acls.c
> index 4d50223..6011a72 100644
> --- a/source3/modules/nfs4_acls.c
> +++ b/source3/modules/nfs4_acls.c
> @@ -107,6 +107,10 @@ int smbacl4_get_vfs_params(struct connection_struct *conn,
>  
>  	params->map_full_control = lp_acl_map_full_control(SNUM(conn));
>  
> +	params->sort_dacl = lp_parm_bool(SNUM(conn),
> +					 SMBACL4_PARAM_TYPE_NAME,
> +					 "sort_dacl", False);
> +
>  	DEBUG(10, ("mode:%s, do_chown:%s, acedup: %s map full control:%s\n",
>  		enum_smbacl4_modes[params->mode].name,
>  		params->do_chown ? "true" : "false",
> @@ -532,6 +536,11 @@ static NTSTATUS smb_get_nt_acl_nfs4_common(const SMB_STRUCT_STAT *sbuf,
>  		return NT_STATUS_NO_MEMORY;
>  	}
>  
> +	if ((*ppdesc)->dacl && params->sort_dacl) {
> +		dacl_sort_into_canonical_order((*ppdesc)->dacl->aces,
> +					       (unsigned int)(*ppdesc)->dacl->num_aces);
> +	}
> +
>  	DEBUG(10, ("smb_get_nt_acl_nfs4_common successfully exited with "
>  		   "sd_size %d\n",
>  		   (int)ndr_size_security_descriptor(*ppdesc, 0)));
> diff --git a/source3/modules/nfs4_acls.h b/source3/modules/nfs4_acls.h
> index c9fcf6d..ad981a3 100644
> --- a/source3/modules/nfs4_acls.h
> +++ b/source3/modules/nfs4_acls.h
> @@ -113,6 +113,7 @@ struct smbacl4_vfs_params {
>  	bool do_chown;
>  	enum smbacl4_acedup_enum acedup;
>  	bool map_full_control;
> +	bool sort_dacl;
>  };
>  
>  int smbacl4_get_vfs_params(struct connection_struct *conn,