DNM: pass "UserPerm" struct throughout Client for security checks #11218
Conversation
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
We need to get rid of the internal uid/gid selection and switch to permission-checking that handles multiple groups -- while also preserving the client_mount_(uid|gid) config params. So expose a function which can be called externally that replicates our current uid/gid selection, but uses our new UserPerm struct. Signed-off-by: Greg Farnum <gfarnum@redhat.com>
…kers Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
And also _mknod, _symlink, _rmdir. Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
And also Client::_do_setattr() Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
We also recast our private _setattr() stuff to be implemented in terms of UserPerm instead of uid/gid, and translate the other direction. Signed-off-by: Greg Farnum <gfarnum@redhat.com>
By the time we're done we don't want to be setting any permissions in the Client code directly, and this open-coding will make it more obvious if we miss something (when removing the get_uid() and get_gid() functions). Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Also remove the lstatlite() declaration, since it's not defined. Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
…ts request We were previously iterating through all the GIDs allowed by a particular MDSAuthCap::match, but there are plenty of situations where a GID might be *allowed* but not actually assigned. Take the set intersection of what's allowed and what's specified instead. Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Conflicts: src/client/Client.cc src/client/Client.h src/client/Makefile.am src/client/fuse_ll.cc Signed-off-by: Greg Farnum <gfarnum@redhat.com>
|
This is DNM as I still need to run tests against the post-master-merge version, but it should be ready for review. Sorry about the messy history, but it wasn't very feasible to squash after a couple rebases. Hopefully it's not too bad! Note that this needs to run against ceph/ceph-qa-suite#1187 as there are some changes to security options. |
|
I took a look over the set earlier and had a look at the merge commit and it all looks pretty sane to me. One minor nit: I think we ought to call the UserPerm object something different (at least in the context of the client). Maybe a "PosixCred" object, since that's what we're really emulating here. One thing we need to think about is how to add new C API variants that take a full credential. I think we ought to create a generic auth object that apps can use to set the creds properly for the call. The old API would then become a wrapper that would just use trivial cred objects. Any thoughts on a naming convention there? We currently have ceph_* functions for most of the API calls that operate on a path or file descriptor. We also have some "lowlevel" API functions that operate on Inode objects directly that are prefaced with ceph_ll_*. Maybe we could tack "cred" or "auth" to the end of the function names? Or "ceph2*" ? "cephfs" ? Alternately, we could consider using a combination of thread local storage and global variables to pass this info along: Have a global "cephfs_default_cred" pointer to such an object and a thread-local object. Have the API fetch those variables in the C shim layer, and pick/setup the right ones to use. That would require callers that change creds to be careful about keeping track of them though, which I don't really like... Anyway, handing back to @gregsfortytwo for merge when it's ready. |
|
This is looking good, scanned over some of the patches. |
…ding behavior Signed-off-by: Greg Farnum <gfarnum@redhat.com>
Fixes: http://tracker.ceph.com/issues/17368 Signed-off-by: Greg Farnum <gfarnum@redhat.com>
gregsfortytwo
force-pushed
the
wip-getuid
branch
from
fa54c8a
to
a67f61f
Compare
|
@jtlayton, you'll want to note the freshly-disabled tests. Right now we don't have a way of specifying multiple GIDs in our test suite because of the libcephfs limitations. We may also decide we need to let our internal Client interface specify multiple GIDs to use on each op, but...I don't think we will? |
|
@jcsp, did you want to take a look at this before merging? |
|
(Still DNM as we need a fresh test run, coming shortly, but I believe I've squashed all the bugs that showed up last time so we should be good.) |
|
👍 I'm happy if you guys are both happy |
| @@ -83,6 +83,14 @@ enum { | |||
| }; | |||
|
|
|||
|
|
|||
| struct UserPerm | |||
There was a problem hiding this comment.
Erm, maybe we should call this "PosixCred" or "UnixCred" instead? This is really a credential object. Also we might want to make this a versioned structure. Will we ever want to add end-to-end kerberos support, for instance? If so, then being able to append or union in a krb5 ticket would be much simpler and opaque to most functions.
Erm on second thought -- nm on the versioning. I guess we'll want to have an API for creating a cred object to pass around, so there's no real point in versioning the structure.
gregsfortytwo
force-pushed
the
wip-getuid
branch
from
b200d0e
to
d15ea7b
Compare
|
Ok, I was seeing some bad frees happening, and I think the UserPerm code still needs a little work. This patch seems to fix it. @gregsfortytwo , want to squash that into an existing commit? |
gids is an array. We also need to clean it up when deep_copy()ing, which requires initializing our member fields in the copy-constructor. Signed-off-by: Greg Farnum <gfarnum@redhat.com>
gregsfortytwo
force-pushed
the
wip-getuid
branch
from
d15ea7b
to
42b1572
Compare
|
Squashed a slightly different commit in that also cleans up the silly practice of passing *this in to deep_copy, since it's not a static function. Gitbuilders are way behind but it passed a local ceph_test_libcephfs (which broke on the last go-round) and assuming next run is happy I'll merge. |
This PR creates a new "UserPerm" struct that can handle a UID and an array of GIDs.
It passes it throughout the request pipeline, which is a significant improvement over our existing uid/gid param pair.
Additionally, it stops the practice of converting "-1" UID/GID params into whatever the Client process is running as. That resulted in problems such as breaking Ganesha's root squash configuration (http://tracker.ceph.com/issues/16367), in addition to just being weird.
We do not yet export UserPerms or similar interfaces to libcephfs users; that is the next step.