-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable ACL support for libfuse3 #128
base: master
Are you sure you want to change the base?
Conversation
Take note of the last line of each code block below. Before the PR, the Before PR
After PR
|
Hi! I'll take a closer look later, but this worries me:
because in #120 we discovered that I don't know how much work it would be to check ACLs manually. Sounds non-trivial. But perhaps we can lift code from some other filesystem that predates FUSE ACL support. |
Thanks for the quick reply. I've just read through the issue (and associated libfuse issue) that you linked to. As I mentioned in my previous comment, I don't have a enough knowledge of either of these codebases to judge, but it would seem a real shame (and quite problematic if implemented incorrectly) to have to re-implement this logic. Assuming implementing I notice in #120 you're saying you're hoping you'll get around to looking at it soon, have you got any updates on this / would it be of any value to you if I pitched in here? Just so I'm clear (from my naive understanding), we're looking to implement |
Having played around a bit more this afternoon, I think I understand why you've said this is non-trivial. I made the incorrect assumption that It appears to me that we would need to implement |
I think that's correct. We'd have to
Looking at the helpful docs you linked, I don't think any of these steps will be excessively difficult. Just lots of attention to detail will be needed. I agree, it's disappointing to have to re-implement this, but the only other option seems to be to patch FUSE (which probably involes kernel changes). Doing that and getting the changes approved seems much more difficult. It might make sense to implement classic permissions first and only then ACLs, but both approaches should be fine as long as we support underlying filesystems with and without ACLs enabled. I've been pretty terrible lately at taking on medium-to-large tasks like this with bindfs. So many other things taking priority. Do have at it if you're interested! And if you do, don't hesitate to ask questions. I've been less terrible at answering in a reasonable time. * An idea for unit tests: if we could do something like the following pseudocode, we could cover a lot of cases with relatively little work:
|
Sounds good; I agree with what you've said. I'll try to get a few things together and will push my changes as I go, so you've got a bit of oversight. As a very quick test (below), I tested libacl (with my own I'll keep you posted with any progress.
|
👍 👍 Looks like libacl is readily available in distros, and we could even optionally bundle it since both projects are (L)GPL. But we can worry about all that later. It's worth double-checking that libacl doesn't already have a permission checking function. I didn't see one, but it wouldn't be the first time I've missed something. And if it doesn't, they may be interested in a copy of what you're about to write. |
Just sat down to do a bit of work on this and took your advice for looking around for an existing permission checking function and came across this: https://github.com/torvalds/linux/blob/master/fs/posix_acl.c#L371-L444 The bare-bones outline of the function looks vaguely like what I was in the process of writing, but this is where I'm bumping up against my knowledge of C/the Kernel. I get the feeling that there's possibly a higher level function in the Kernel that you're supposed to use, rather than calling it directly (though I could be wrong) and I'm also finding that my If you have any ideas here, I'd be grateful to hear them, no worries if not though, I'll keep looking around and see what I come up with. |
Good find!
The headers in I'm not a kernel expert, but if GitHub's fancy new reference lookup (which appears when you click on the function name) is to be believed,
It's probably best to use |
That makes sense, thanks for explaining.
Yeah, sure. I'll try to align as closely as possible with their implementation. Hopefully I'll have a bit of time to work on this tomorrow. |
Apologies for the longer than anticipated time since my last message, but I've made some progress and a few more questions have emerged.
So at this point, my questions to you are:
Thanks again and apologies for the wall of text! |
Good progress!
I'm not sure. The
Also FUSE disables (or at least used to disable) setuid/setgid inside the mount for security reasons. Other than these, I don't see a reason why
Unless I'm missing something, we can't just call
Good point, I hadn't realized that. Fortunately that number looks almost manageable. If we can get it comfortably below 1 million and ideally avoid spawning a subprocess on each iteration (e.g. by writing the test in Ruby like the existing ones), then I think we'll be good. If in the end the test takes significantly more than ~10 seconds, we could perhaps write it as a separate test and the smaller set of test cases you just presented could be part of the main test suite ( P.S. did you mean to push your permission check code? If not yet, no rush. |
I did spot that and probably should have given it more thought than I did. Looking back at it now, I'm definitely not certain on it, but I don't think it affects us:
Yeah, I've read that somewhere as well. I haven't looked into the implications of this up to this point, but I don't think this PR should change the existing behaviour of this. I wasn't sure what "disable" meant in this context, so I ran a few tests. After issuing this command
I was expecting the first test to have failed due to the mount being performed/owned by
Yeah, you're correct. I was thinking of going for an MVP where we just get the basic access control working correctly and leave the extra functionality (i.e. mapping) for further down the line (though maybe that's not possible without breaking the standard bindfs behaviour come to think of it). Either way, as I've already pretty much written the I will have to take a better look at the
I didn't at this stage, I wanted to just run the Again, apologies for the slow replies, I thought I was going to have more free time to work on this than I actually do; I'll keep working on it when I do get time though. |
By disable I mean that setuid/setgid has no effect. That matches what your test shows. If your According to this, it seems setuid is not disabled if the mounter is root. So I think we should include setuid behaviour in our tests, but only when running the test as root (so the mount is made as root).
Yes, actually it should be a simple call to The code looks good! I'll add review comments plus some notes to self.
I know this feeling very well 😄 |
@@ -36,7 +36,7 @@ AM_CONDITIONAL(BUILD_OS_IS_DARWIN, [test x"$build_os" = darwin]) | |||
my_CPPFLAGS="-D_REENTRANT -D_FILE_OFFSET_BITS=64 -D_XOPEN_SOURCE=700 -D__BSD_VISIBLE=1 -D_BSD_SOURCE -D_DEFAULT_SOURCE -D_DARWIN_BETTER_REALPATH" | |||
|
|||
my_CFLAGS="-std=c99 -Wall -Wpedantic -fno-common" | |||
my_LDFLAGS="-pthread" | |||
my_LDFLAGS="-pthread -lacl" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's check for and link libacl using pkg-config i.e. with PKG_CHECK_MODULES
. I can do this later.
@@ -63,6 +63,8 @@ | |||
#ifdef HAVE_SETXATTR | |||
#include <sys/xattr.h> | |||
#endif | |||
#include <sys/acl.h> | |||
#include <acl/libacl.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: check whether these are available and work the same on FreeBSD and MacOS. Probably not => needs ifdefs.
while (getgrouplist(pwd->pw_name, egid, groups, &ngroups) == -1) { | ||
DPRINTF("Reallocating space for groups to %d entries", ngroups); | ||
groups = realloc(groups, sizeof(*groups) * ngroups); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: consider caching group lists later (see userinfo.h)
Thanks for the review. I'll address as many of your comments as I can and push the changes. I'll probably make individual commits at this stage, so it's clear what I'm changing, but we can squash them before merge to get a cleaner commit history if that suits you?
If the mount is owned by a non-root user, it appears as though
I believe this can be done with capabilities too, which might explain this.
Yeah, I believe you're right with regard to the
I'll make the changes you've outlined in your final comment (line 1876) and push the changes. Thanks again for the quick turn around on the review! |
👍
My mental model is this: all setuid does is instruct the kernel "when executing this file, do so as the file's owner (unless in a |
Just started to look at this and while I can see a way forward, my current method of determining the required permissions is to create a directory structure in a standard file system and find the minimum permissions required to perform a given operation. While this works, it's not particularly slick. Before I go all in on this, I thought I'd ask if you knew of any good resources where these permissions might be documented? I've attempted looking through Kernel source code and while I'm sure if I took the time to follow all of the function calls, I would find what I'm looking for, it seems even more time consuming. No worries if you don't know of anything, but I thought I'd ask before I put the time into this. Thanks :) |
Each operation's man page ( |
Ah good spot, thanks for the pointer. Like you say, hopefully that'll be enough to get going with and the tests will catch any inconsistencies. |
I've implemented the access checks in (what I currently believe to be) all of the necessary functions. Some (a lot) of my own tests are failing, but at this stage I would appreciate your take on the overall approach and whether you have any major concerns with the general implementation? Bar any of your suggestions, I think it makes sense to get the test suite fleshed out now, so I've got some concrete metrics to work towards. I haven't really spent any time looking at your tests yet, so I'll familiarise myself with those and get started on a permuted test suite that'll hopefully give us good coverage and something for me to work towards. Happy to take any pointers/tips/direction on this. Thanks again :) |
return path_search_permission; | ||
|
||
// TODO: Pretty sure we need more access checks here. Will let the tests | ||
// highlight the issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: now that I look at this function, it's almost identical to bindfs_open
. Should probably factor out a helper.
Your approach looks good 👍
The test code is .. not in exemplary condition. But it's not too bad - at least the setup is done and there are plenty of examples.
Thank you! |
Just rebased onto |
Any Progress on this? I'm trying to get it to work with macOS but ACLs seem to work a bit differently there. |
Hi again @mpartel,
I've been looking into getting ACL support integrated into bindfs for the past couple of days, as I've found it to be a blocker for a personal project of mine. Issue #91 covers the issues, though this PR specifically aims to address point 1 of @likan999's original comment.
I don't typically develop in C and I'm not particularly familiar with libfuse or the bindfs codebase, so I've currently only put together a very bare bones PR. Thankfully, libfuse >= 3.2 and @McBane87's work (PR #95) has seemingly done all of the heavy lifting for us, so it's just a one-liner.
The docs for FUSE_CAP_POSIX_ACL (see code diff) does mention a couple of points that I would imagine you'd be better placed to judge the implications of, but it looked workable to me.
I put together a test case (in the comment below) to depict the before and after of this change.
Just for your reference, these are the most relevant couple of links that lead me to the change I've made:
Pending your approval, I can update the docs and perform any other relevant changes in order to get the change ready for merging.
Many thanks.