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