[PATCH] nfs4acl: Fix owner mapping with ID_TYPE_BOTH

[Christof Schmitt]

On Sat, Sep 17, 2016 at 05:28:34AM +1200, Andrew Bartlett wrote:
> On Mon, 2016-09-12 at 17:34 -0700, Christof Schmitt wrote:
> > From f1883adf6ed027a03be2a3d4f1631d8bdb283e38 Mon Sep 17 00:00:00
> > 2001
> > From: Christof Schmitt <cs at samba.org>
> > Date: Mon, 12 Sep 2016 16:22:16 -0700
> > Subject: [PATCH] nfs4acl: Fix owner mapping with ID_TYPE_BOTH
> > 
> > This fixes a corner case when using NFS4 ACLs with
> > ID_TYPE_BOTH.  Before
> > this patch, the owner entry in the ACL would be mapped to a gid entry
> > in
> > the NFSv4 ACL, and not the expected special owner entry. This is
> > caused
> > by the id mapping returning a valid gid and the nfs4 mapping assumed
> > that this was actually a group.
> > 
> > Fix this by asking for the uid first, and explicitly checking if the
> > mapped uid matches the owner. That creates a uid entry in the NFSv4
> > ACL
> > that can be changed later in smbacl4_substitute_{simple,special} to
> > the
> > expected special owner entry.
> > 
> > Signed-off-by: Christof Schmitt <cs at samba.org>
> > ---
> >  source3/modules/nfs4_acls.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/source3/modules/nfs4_acls.c
> > b/source3/modules/nfs4_acls.c
> > index 71f4d8d..996dbab 100644
> > --- a/source3/modules/nfs4_acls.c
> > +++ b/source3/modules/nfs4_acls.c
> > @@ -715,11 +715,16 @@ static bool smbacl4_fill_ace4(
> >  		uid_t uid;
> >  		gid_t gid;
> >  
> > -		if (sid_to_gid(&ace_nt->trustee, &gid)) {
> > +		/*
> > +		 * ID_TYPE_BOTH returns both uid and gid. Explicitly
> > +		 * check for ownerUID to allow the mapping of the
> > +		 * owner to a special entry in this idmap config.
> > +		 */
> > +		if (sid_to_uid(&ace_nt->trustee, &uid) && uid ==
> > ownerUID) {
> > +			ace_v4->who.uid = uid;
> > +		} else if (sid_to_gid(&ace_nt->trustee, &gid)) {
> >  			ace_v4->aceFlags |=
> > SMB_ACE4_IDENTIFIER_GROUP;
> >  			ace_v4->who.gid = gid;
> > -		} else if (sid_to_uid(&ace_nt->trustee, &uid)) {
> > -			ace_v4->who.uid = uid;
> >  		} else if (dom_sid_compare_domain(&ace_nt->trustee,
> >  						  &global_sid_Unix_N
> > FS) == 0) {
> >  			return false;
> 
> Just be aware that we did want to keep the gid entry - this was
> deliberately added.
> 
> The translation to @owner may still be required, but the idea was to
> eventually permit sidHistory to work, and therefore not deny access to
> the user if the 'user' SID becomes an additional SID in a token at some
> later point, then only visible as a gid in NFSv4 space.
> 
> See the commit that originally introduced this:
> 
> commit f36e28d1316bc0bd210933bbdb77241376fe3500
> Author: Andrew Bartlett <abartlet at samba.org>
> Date:   Mon May 7 08:48:24 2012 +1000
> 
>     s3-nfs4acls: Remove lookup_sid and sidmap from NFSv4 ACL mapping
> and check gid first
>     
>     By checking just the IDMAP, and by removing the sidmap and
> lookup_sid calls, we support
>     IDMAP_BOTH.  This is because by checking for a mapping to a GID
> first, we can rely on
>     the fact that IDMAP_BOTH will resolve to a GID.
>     
>     If the sidmap idea is valued - it allows multiple SIDs to map to a
> single unix ID, this should
>     be done in the IDMAP layer.
>     
>     Andrew Bartlett
>     
>     Signed-off-by: Jeremy Allison <jra at samba.org>
>     
>     Autobuild-User(master): Jeremy Allison <jra at samba.org>
>     Autobuild-Date(master): Sat Aug 11 01:17:36 CEST 2012 on sn-devel-
> 104

The requirement here was to keep the owner mapping to the special user
entry, since that has a special meaning in the gpfs file system (the
POSIX modebits are derived from the special entries).

SID history is also a topic that keeps popping up. This would require
having the entry stored for a gid to allow access from the old and new
SID. In theory we could get there by mapping an entry for the owner SID
to two entries in NFS4: special user and a group entry. I am not yet
sure what implications that would have.

Christof