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

[Pranith Kumar Karampuri]

On Mon, Sep 26, 2016 at 4:49 PM, Niels de Vos <ndevos at redhat.com> wrote:

> 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.
>

Could you list down the pros/cons with each approach?


>
> Niels
>



-- 
Pranith
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.gluster.org/pipermail/gluster-devel/attachments/20160926/2d29a69c/attachment.html>