Handle IDMAP_BOTH in posix_acls.c

[Andrew Bartlett]

On Sun, 2012-05-06 at 16:43 +1000, Andrew Bartlett wrote:
> On Fri, 2012-05-04 at 21:07 +0200, Christian Ambach wrote:
> > On 05/04/2012 04:17 AM, Andrew Bartlett wrote:
> > 
> > > I've looked at this, and I have three concerns:
> > >   - How is this code tested
> > >   - What is the purpose of the sidmap?
> > 
> > I think the sidmap was initially added to give users the possibility to 
> > define that a certain SID should better be replaced by another one when 
> > compiling the ACL. I am not aware of anyone actually using the function 
> > and I have troubles remembering why this functionality was added five 
> > years ago. I have crawled my mail archives without success.
> > 
> > sidmap was added to the code in the process of syncing Tridge's 
> > samba-ctdb bzr tree to Samba, so the history is lost as the bzr repo is 
> > not accessible any more. Maybe someone still has a copy around?
> > 
> > Maybe it was meant to be able to deal with SIDs which cannot be mapped 
> > for whatever reason (e.g. somebody copies file from its local machine to 
> > the share and the ACLs contain SIDs of the local machine).
> > 
> > >   - What is the purpose of the lookup_sid() call?
> > I would guess this relates to the sidmapping functionality above: if the 
> > SID cannot be found, then check if there is a replacement to use defined 
> > in the sidmap database.
> > 
> > > On the last point, it seems we have a potentially quite inefficient
> > > lookup_sid call in nfs4_acls.c:smbacl4_fill_ace4().  This is called for
> > > every ACE on every ACL set.
> > >
> > > I'm hoping this was just added out of paranoia, but it seems to be tied
> > > in to the sidmap.  In any case the posix ACL code doesn't do this, and
> > > it would work best if we remove it to support IDMAP_BOTH (but, due to
> > > doing this lookup, it also means that idmap both will simply be ignored
> > > for nfs4 in the interim).
> > 
> > I would vote to remove the whole sidmap code. I do not think this has 
> > seen real use yet and usecases for it seem to be very rare.
> > It's also not documented in README.nfs4acls.txt
> 
> Removing the sidmap, and the lookup_sids calls would be great, and make
> this much easier to patch.

The attached, untested patch implements this.

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-s3-nfs4acls-Remove-lookup_sid-and-sidmap-from-NFSv4-.patch
Type: text/x-patch
Size: 5160 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20120507/61bf99ad/attachment.bin>