[Gluster-devel] Fixing setfsuid/gid problems in posix xlator

[Niels de Vos]

On Fri, Sep 23, 2016 at 08:44:14PM +0530, Pranith Kumar Karampuri wrote:
> On Fri, Sep 23, 2016 at 6:12 PM, Jeff Darcy <jdarcy at redhat.com> wrote:
> 
> > > Jiffin found an interesting problem in posix xlator where we have never
> > been
> > > using setfsuid/gid ( http://review.gluster.org/#/c/15545/ ), what I am
> > > seeing regressions after this is, if the files are created using non-root
> > > user then the file creation fails because that user doesn't have
> > permissions
> > > to create the gfid-link. So it seems like the correct way forward for
> > this
> > > patch is to write wrappers around sys_<syscall> to do setfsuid/gid do the
> > > actual operation requested and then set it back to old uid/gid and then
> > do
> > > the internal operations. I am planning to write posix_sys_<syscall>() to
> > do
> > > the same, may be a macro?
> >
> > Kind of an aside, but I'd prefer to see a lot fewer macros in our code.
> > They're not type-safe, and multi-line macros often mess up line numbers for
> > debugging or error messages.  IMO it's better to use functions whenever
> > possible, and usually to let the compiler worry about how/when to inline.
> >
> > > I need inputs from you guys to let me know if I am on the right path and
> > if
> > > you see any issues with this approach.
> >
> > I think there's a bit of an interface problem here.  The sys_xxx wrappers
> > don't have arguments that point to the current frame, so how would they get
> > the correct uid/gid?  We could add arguments to each function, but then
> > we'd have to modify every call.  This includes internal calls which don't
> > have a frame to pass, so I guess they'd have to pass NULL.  Alternatively,
> > we could create a parallel set of functions with frame pointers.  Contrary
> > to what I just said above, this might be a case where macros make sense:
> >
> >    int
> >    sys_writev_fp (call_frame_t *frame, int fd, void *buf, size_t len)
> >    {
> >       if (frame) { setfsuid(...) ... }
> >       int ret = writev (fd, buf, len);
> >       if (frame) { setfsuid(...) ... }
> >       return ret;
> >    }
> >    #define sys_writev(fd,buf,len) sys_writev_fp (NULL, fd, buf, len)
> >
> > That way existing callers don't have to change, but posix can use the
> > extended versions to get the right setfsuid behavior.
> >
> >
> After trying to do these modifications to test things out, I am now under
> the impression to remove setfsuid/gid altogether and depend on posix-acl
> for permission checks. It seems too cumbersome as the operations more often
> than not happen on files inside .glusterfs and non-root users/groups don't
> have permissions at all to access files in that directory.

But the files under .glusterfs are hardlinks. Except for creation and
removal, should the users not have access to read/write and update
attributes and xattrs?

I would prefer to rely on the VFS permission checking on the bricks, and
not bother with the posix-acl xlator when the filesystem on the brick
supports POSIX ACLs.

Niels
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://www.gluster.org/pipermail/gluster-devel/attachments/20160926/7e8b21ff/attachment-0001.sig>