On Fri, 2016-11-11 at 14:48 +0000, Sage Weil wrote:
> On Fri, 11 Nov 2016, Jeff Layton wrote:
> > On Mon, 2016-11-07 at 23:21 +0000, Sage Weil wrote:
> > > On Mon, 7 Nov 2016, Gregory Farnum wrote:
> > > > On Mon, Nov 7, 2016 at 1:21 PM, Sage Weil <sweil@xxxxxxxxxx> wrote:
> > > > > On Mon, 7 Nov 2016, Jeff Layton wrote:
> > > > >> On Mon, 2016-11-07 at 20:09 +0000, Sage Weil wrote:
> > > > >> > On Mon, 7 Nov 2016, Gregory Farnum wrote:
> > > > >> > > On Mon, Nov 7, 2016 at 10:39 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > > > >> > > > On Mon, 2016-11-07 at 14:36 +0000, Sage Weil wrote:
> > > > >> > > >> On Mon, 7 Nov 2016, Jeff Layton wrote:
> > > > >> > > >> > On Mon, 2016-11-07 at 14:05 +0000, Sage Weil wrote:
> > > > >> > > >> > > On Mon, 7 Nov 2016, Jeff Layton wrote:
> > > > >> > > >> > > > On Mon, 2016-11-07 at 16:43 +0800, Yan, Zheng wrote:
> > > > >> > > >> > > > > On Fri, Nov 4, 2016 at 8:57 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > > > >> > > >> > > > > >
> > >
> > > [okay, time to prune this a bit]
> > >
> > > > >> It still seems to me like that should just be a check for superuser
> > > > >> status. Something like:
> > > > >>
> > > > >> if (mask & MAY_CHOWN) {
> > > > >> // only root can chown
> > > > >> if (i->match.uid != 0 || caller_uid != 0)
> > > > >> continue;
> > > > >> }
> > > > >> }
> > > > >>
> > > > >> i.e. only allow chown if the capability has a uid of 0 and the
> > > > >> caller_uid is also 0.
> > > > >>
> > > > >> I don't think we want to ever grant an unprivileged user the ability to
> > > > >> chown, do we?
> > > > >
> > > > > Ah, yep. Except that the Locker.cc caller needs to be fixed to only ask
> > > > > for MAY_CHOWN if the uid is changing. Right now it's only passing
> > > > > MAY_WRITE which looks wrong too...
> > > >
> > > > Don't we want to let users chown between their own UIDs? A POSIX
> > > > superuser — ie root — really has very little meaning in terms of CephX
> > > > permissions. But it's perfectly legitimate for a tenant with 3 users
> > > > to chmod files between those 3. :/
> > >
> > > Maybe, but it'd be a different kind of check though, because it depends on
> > > multiple caps in order to permit the operation. So we'd need a couple
> > > function-global bools like chown_src_allowed and chown_dst_allowed or
> > > something like that.
> > >
> > > I'm not sure it would work even then, though, because on the client the
> > > user would still have to sudo chown ... to get past the client kernel's
> > > checks (I assume?).
> > >
> > >
> >
> > Not necessarily. Most kernel filesystems call setattr_prepare, which is
> > where the CAP_CHOWN check happens, but (e.g.) NFS doesn't call it as it
> > always delegates the permission checking for setattr to the server.
> >
> > Looks like both the in-kernel and Ceph client and FUSE always call that
> > function, so CAP_CHOWN permissions are always checked client-side there.
> > Still, with libcephfs a program need not deal with the kernel at all, so
> > you can't really rely on that.
>
> In the libcephfs case it's still doing the Client.cc checks, though,
> right? Both the Linux VFS (on ceph.ko) and Client.cc are implementing
> client-side checks...
>
Only if fuse_default_permissions is set (which is a misnomer since it
affects more than just ceph-fuse). It's not set by default, but probably
should be.
> > Isn't this potentially silent metadata corruption? You could end up with
> > a program that issues an ownership change that appears to work, only to
> > find later that it got reverted because the mds decided it didn't want
> > to apply it once the caps got updated.
> >
> > I think the simplest solution would be to just not issue exclusive caps
> > at all if you aren't prepared to accept cached updates from the client
> > for the corresponding metadata. Those clients would get shared caps
> > only. So, you could have clients to which you allow Fx caps, but only
> > As?
>
> That would be safe, but it would kill performance for any user writing
> files and restricted to a particular user by their caps. I think it's
> definitely worth maintaining the Client code do the checks so that
> unpermitted updates aren't dropped... which, if I'm not mistaken,
> is already in place.
>
I don't see why that would kill performance. chown/chgrp/chmod would all
come under Auth caps, whereas read/write (which is where we really care
about performance) come under File caps.
chown/chgrp/chmod are all pretty rare in most workloads, so having to
call the mds to apply them in those cases doesn't seem like a heavy
burden.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
