[PATCH] nfs4acl: Fix owner mapping with ID_TYPE_BOTH

[Andrew Bartlett]

On Fri, 2016-09-16 at 10:45 -0700, Christof Schmitt wrote:
> 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_Un
> > > ix_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.

Yes.  

Oddly, I actually spent quite some time on this whole area back in
2012/13 trying to sort out the simple and special mapping stuff.  It
continues to be a mind-bending area, trying to both keep compatibility
and correctly map the @owner stuff is good for producing headaches.

I would say that this isn't an area to rush, and ideally we would have
some expected value tests.  (I can't find any however, unlike the posix
acl tests). 

Andrew Bartlett

> Christof
> 
-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba