Enable ACL support for libfuse3

[vimist]

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:

• https://github.com/libfuse/libfuse/issues/4
• https://sourceforge.net/p/fuse/mailman/fuse-devel/thread/OFCA6FE496.708107AF-ONC125826A.004BBDD8-C125826A.004DF1B0%40notes.na.collabserv.com/#msg36288609

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.

[vimist]

Take note of the last line of each code block below. Before the PR, the /working/mount/both_data file is only accessible to aragon, after the PR it's (correctly) also available to boromir.

Before PR

Creating directories
[ SUCCESS ] mkdir -p /working/source /working/mount

Creating files
[ SUCCESS ] touch /working/source/aragon_only
[ SUCCESS ] chown aragon:aragon /working/source/aragon_only
[ SUCCESS ] chmod 700 /working/source/aragon_only

[ SUCCESS ] touch /working/source/boromir_only
[ SUCCESS ] chown boromir:boromir /working/source/boromir_only
[ SUCCESS ] chmod 700 /working/source/boromir_only

[ SUCCESS ] touch /working/source/both_data
[ SUCCESS ] chown aragon:aragon /working/source/both_data
[ SUCCESS ] chmod 700 /working/source/both_data
[ SUCCESS ] setfacl -m user:boromir:rwx /working/source/both_data

Mounting "source" to "mount"
[ SUCCESS ] bindfs /working/source /working/mount

Listing file permissions
total 8
drwxr-xr-x    2 root     root          4096 May  8 11:28 .
drwxr-xr-x    1 root     root          4096 May  8 11:28 ..
-rwx------    1 aragon   aragon           0 May  8 12:24 aragon_only
-rwx------    1 boromir  boromir          0 May  8 12:24 boromir_only
-rwxrwx---    1 aragon   aragon           0 May  8 12:24 both_data
[ SUCCESS ] ls -la /working/source
total 8
drwxr-xr-x    2 root     root          4096 May  8 11:28 .
drwxr-xr-x    1 root     root          4096 May  8 11:28 ..
-rwx------    1 aragon   aragon           0 May  8 12:24 aragon_only
-rwx------    1 boromir  boromir          0 May  8 12:24 boromir_only
-rwxrwx---    1 aragon   aragon           0 May  8 12:24 both_data
[ SUCCESS ] ls -la /working/mount

getfacl: Removing leading '/' from absolute path names
# file: working/source/both_data
# owner: aragon
# group: aragon
user::rwx
user:boromir:rwx
group::---
mask::rwx
other::---

[ SUCCESS ] getfacl /working/source/both_data
getfacl: Removing leading '/' from absolute path names
# file: working/mount/both_data
# owner: aragon
# group: aragon
user::rwx
user:boromir:rwx
group::---
mask::rwx
other::---

[ SUCCESS ] getfacl /working/mount/both_data

Testing accesses in source directory
====================================

Testing aragon permissions
[ SUCCESS ] sudo -u aragon cat /working/source/aragon_only
cat: can't open '/working/source/boromir_only': Permission denied
[ FAILURE ] sudo -u aragon cat /working/source/boromir_only
[ SUCCESS ] sudo -u aragon cat /working/source/both_data

Testing boromir permissions
cat: can't open '/working/source/aragon_only': Permission denied
[ FAILURE ] sudo -u boromir cat /working/source/aragon_only
[ SUCCESS ] sudo -u boromir cat /working/source/boromir_only
[ SUCCESS ] sudo -u boromir cat /working/source/both_data

Testing accesses in mount directory
====================================

Testing aragon permissions
[ SUCCESS ] sudo -u aragon cat /working/mount/aragon_only
cat: can't open '/working/mount/boromir_only': Permission denied
[ FAILURE ] sudo -u aragon cat /working/mount/boromir_only
[ SUCCESS ] sudo -u aragon cat /working/mount/both_data

Testing boromir permissions
cat: can't open '/working/mount/aragon_only': Permission denied
[ FAILURE ] sudo -u boromir cat /working/mount/aragon_only
[ SUCCESS ] sudo -u boromir cat /working/mount/boromir_only
cat: can't open '/working/mount/both_data': Permission denied
[ FAILURE ] sudo -u boromir cat /working/mount/both_data

After PR

Creating directories
[ SUCCESS ] mkdir -p /working/source /working/mount

Creating files
[ SUCCESS ] touch /working/source/aragon_only
[ SUCCESS ] chown aragon:aragon /working/source/aragon_only
[ SUCCESS ] chmod 700 /working/source/aragon_only

[ SUCCESS ] touch /working/source/boromir_only
[ SUCCESS ] chown boromir:boromir /working/source/boromir_only
[ SUCCESS ] chmod 700 /working/source/boromir_only

[ SUCCESS ] touch /working/source/both_data
[ SUCCESS ] chown aragon:aragon /working/source/both_data
[ SUCCESS ] chmod 700 /working/source/both_data
[ SUCCESS ] setfacl -m user:boromir:rwx /working/source/both_data

Mounting "source" to "mount"
[ SUCCESS ] bindfs /working/source /working/mount

Listing file permissions
total 8
drwxr-xr-x    2 root     root          4096 May  8 11:28 .
drwxr-xr-x    1 root     root          4096 May  8 11:28 ..
-rwx------    1 aragon   aragon           0 May  8 12:26 aragon_only
-rwx------    1 boromir  boromir          0 May  8 12:26 boromir_only
-rwxrwx---    1 aragon   aragon           0 May  8 12:26 both_data
[ SUCCESS ] ls -la /working/source
total 8
drwxr-xr-x    2 root     root          4096 May  8 11:28 .
drwxr-xr-x    1 root     root          4096 May  8 11:28 ..
-rwx------    1 aragon   aragon           0 May  8 12:26 aragon_only
-rwx------    1 boromir  boromir          0 May  8 12:26 boromir_only
-rwxrwx---    1 aragon   aragon           0 May  8 12:26 both_data
[ SUCCESS ] ls -la /working/mount

getfacl: Removing leading '/' from absolute path names
# file: working/source/both_data
# owner: aragon
# group: aragon
user::rwx
user:boromir:rwx
group::---
mask::rwx
other::---

[ SUCCESS ] getfacl /working/source/both_data
getfacl: Removing leading '/' from absolute path names
# file: working/mount/both_data
# owner: aragon
# group: aragon
user::rwx
user:boromir:rwx
group::---
mask::rwx
other::---

[ SUCCESS ] getfacl /working/mount/both_data

Testing accesses in source directory
====================================

Testing aragon permissions
[ SUCCESS ] sudo -u aragon cat /working/source/aragon_only
cat: can't open '/working/source/boromir_only': Permission denied
[ FAILURE ] sudo -u aragon cat /working/source/boromir_only
[ SUCCESS ] sudo -u aragon cat /working/source/both_data

Testing boromir permissions
cat: can't open '/working/source/aragon_only': Permission denied
[ FAILURE ] sudo -u boromir cat /working/source/aragon_only
[ SUCCESS ] sudo -u boromir cat /working/source/boromir_only
[ SUCCESS ] sudo -u boromir cat /working/source/both_data

Testing accesses in mount directory
====================================

Testing aragon permissions
[ SUCCESS ] sudo -u aragon cat /working/mount/aragon_only
cat: can't open '/working/mount/boromir_only': Permission denied
[ FAILURE ] sudo -u aragon cat /working/mount/boromir_only
[ SUCCESS ] sudo -u aragon cat /working/mount/both_data

Testing boromir permissions
cat: can't open '/working/mount/aragon_only': Permission denied
[ FAILURE ] sudo -u boromir cat /working/mount/aragon_only
[ SUCCESS ] sudo -u boromir cat /working/mount/boromir_only
[ SUCCESS ] sudo -u boromir cat /working/mount/both_data

[mpartel]

Hi!

I'll take a closer look later, but this worries me:

Enabling this feature implicitly turns on the default_permissions mount option (even if it was not passed to mount(2)).

because in #120 we discovered that default_permissions has race conditions when bindfs presents different permissions to different users. Even with all FUSE caching turned off, the kernel can still momentarily cache permissions. The FUSE devs didn't want to consider this a bug, so I'm planning to turn off default_permissions and do permission checks myself.

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.

[vimist]

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 access is the way forward, the acl(5) man page is a really good resource for the access check algorithm (apologies if you're already aware of this). I've also had a bit of a play with libacl, which (as a novice C user) seems like it would be valuable here.

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 access where we would implement the ACL checks ourselves (which (afaik) is a superset of the standard file permissions)? Are there specific things that stand out to you as being non-trivial in this endeavour?

[vimist]

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 access was called by something automatically, which would determine the access to the file for other operations (such as open, read, etc), but that doesn't appear to be the case from my testing.

It appears to me that we would need to implement access and manually call it everywhere a permission check is required. Is that a more correct representation of the required work?

[mpartel]

I think that's correct. We'd have to

1. implement access
2. add a call to most operations (e.g. to open but not to read AIUI)
3. unit test carefully*

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:

for file_type in [file, dir, symlink, ...]:
  for owner in [root, test_user_1, test_user_2]:
    for group in [root, test_group_1, test_group_2]:
      for perms in all_possible_permission_bits:
        for operation in all_relevant_operations:

            for dir in [real_directory, bindfs_mount]:
              create_test_file(file_type, owner, group, perms)

            do operation in real_directory
            do operation in bindfs_mount
            check that the outcomes (or errors) are identical

[vimist]

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 get_acls binary) within an SSHFS mount (a filesystem I know doesn't support ACLs) and it just returns the standard unix permissions, which makes sense, as they're just a subset of ACLs, so I think I'll probably go for the full ACL implementation and just make use of libacl to do as much as possible if that's acceptable to you?

I'll keep you posted with any progress.

 ~/mount $  ll

total 44K
-rwxr-xr-x 1 user  user  16K May  9 20:02 get_acls
-rw-r-xr-- 1 user  user    0 May  9 20:02 test_file

 ~/mount $  ./get_acls

ACL_USER_OBJ
    Read   : 1
    Write  : 1
    Execute: 0
ACL_GROUP_OBJ
    Read   : 1
    Write  : 0
    Execute: 1
ACL_OTHER
    Read   : 1
    Write  : 0
    Execute: 0

 ~/mount $  getfacl test_file

# file: test_file
# owner: user
# group: user
user::rw-
group::r-x
other::r--

 ~/mount $  setfacl -m user:1:rwx test_file

setfacl: test_file: Operation not supported

[mpartel]

:+1: :+1:

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.

[vimist]

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 posix_acl.h file doesn't actually contain the posix_acl_permission declaration.

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.

[mpartel]

Good find!

my posix_acl.h file doesn't actually contain the posix_acl_permission declaration

The headers in /usr/include/linux define the userspace API, but posix_acl_permission is an internal function in the kernel i.e. not part of the API.

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, posix_acl_permission is only called the more general permission check functions in fs/namei.c. The ultimate caller is generic_permission, which is probably the higher-level function you suspected.

generic_permission is called from a bunch of filesystem implementations (including fs/fuse/dir.c), but I don't think it's available from userspace. In fact, I don't think the kernel ever exports "pure logic" functions to userspace, because it doesn't make sense to cross the syscall boundary just to run a convenient computation.

It's probably best to use posix_acl_permission as a reference/checklist for your own implementation. It may be good to skim through generic_permission and its callees too, but I think most of it is irrelevant, e.g. the namespace uid/gid mapping (we optionally do our own mapping with our own usermap_ functions), and the capability checks (FUSE doesn't seem to give us the caller's capabilities).

[vimist]

The headers in /usr/include/linux define the userspace API, but posix_acl_permission is an internal function in the kernel i.e. not part of the API.

That makes sense, thanks for explaining.

It's probably best to use posix_acl_permission as a reference/checklist for your own implementation.

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.

[vimist]

Apologies for the longer than anticipated time since my last message, but I've made some progress and a few more questions have emerged.

1. I've more thoroughly scripted up tests for my particular use-case (in bash for now, I've not looked at the existing test suite yet) and have written a small helper program that's just a wrapper around access(2) to aid in the testing. As per this screenshot (and now obvious in retrospect), it's possible to report different file permissions than are actually available. Using access vs cat in the screenshot highlights this nicely; I feel like they should report the same values.

2. I have a very rough and ready nested loop test suite implemented (in bash just for the minute) and while useful, I've calculated the number of tests to be somewhere in the 330,000,000 range, so we'll need to be selective about what we permute here.

3. I spent a little while implementing the access check algorithm we spoke about above and now had it in a pretty reasonable place (bar some edge cases around root accessing things I think); it passed all of my tests (including resolving the discrepancy I mentioned in point 1). Whilst feeling reasonably pleased with my implementation, I questioned why I wasn't just using the access(2) function to do all this, after all, it's what I've been using to test things... I quickly tested return access(real_path, wants) == 0 ? 0 : -errno; and similarly, all my tests passed... I shortly after descended into the familiar place of questioning my sanity and wondering why/how I had missed that. The only reason I can think that I missed this is that I subconsciously made the incorrect assumption that libfuse used it as their default; this is the same implementation they've used in example/passthrough.c. I've not dug into the libfuse codebase to confirm whether this is the case though.

So at this point, my questions to you are:

1. Do you agree that access and cat (for example) should report the same values?
2. Given my test script, are you happy that if these tests pass (along with your existing test suite (that I haven't yet ran)), we have a viable solution (i.e. just calling access)?

Thanks again and apologies for the wall of text!

[mpartel]

Good progress!

1. Do you agree that access and cat (for example) should report the same values?

I'm not sure. The access man page says there are subtle differences:

In other words, access() does not answer the "can I read/write/execute this file?" question. It answers a slightly different question [..]

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 access and cat should work differently e.g. with the ACL stuff. Since all your tests pass with both when not using bindfs, I think they ought to be made to pass also with bindfs (as long as setuid is not involved).

I questioned why I wasn't just using the access(2) function to do all this, [..]

Unless I'm missing something, we can't just call access() because bindfs has various options to modify the permissions it presents. That means your permission check needs to run one of the getattr functions in bindfs.c on the underlying file's permissions before checking anything.

I've calculated the number of tests to be somewhere in the 330,000,000 range, so we'll need to be selective about what we permute here.

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 (tests/test_bindfs.rb).

P.S. did you mean to push your permission check code? If not yet, no rush.

[vimist]

The access man page says there are subtle differences

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:

• If a user mounts a directory and their EUID grants access, but their RUID doesn't I think we still want to allow access and we should report that they have access.
• If a user mounts a directory and their EUID denies access and their RUID allows access (if that can ever happen), it makes sense that access would be denied and also reported that they don't have access.

FUSE disables (or at least used to disable) setuid/setgid inside the mount

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 sudo -u aragon bindfs /usr/bin /mnt, I ran these tests:

• /mnt/ping google.co.uk - Successfully ran (as root)
• sudo -u aragon /mnt/ping google.co.uk - Operation not permitted
• sudo -u aragon /usr/bin/ping google.co.uk - Successfully ran

I was expecting the first test to have failed due to the mount being performed/owned by aragon, but that didn't seem to be the case.

we can't just call access() because bindfs has various options to modify the permissions it presents

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 access function, it does make sense to use it, so it's not just a black box.

I will have to take a better look at the getattr functions you're referring to, as I've not made use of them yet. At a quick glance, it looks like I should be able to use them to do all of that mapping work for me?

did you mean to push your permission check code? If not yet, no rush.

I didn't at this stage, I wanted to just run the access thing by you before I pushed anything up. I'll push up my initial implementation in a minute; it's not complete, but would be good to get your initial impression (I'm not really a C programmer).

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.

[mpartel]

[setuid/setgid]

By disable I mean that setuid/setgid has no effect. That matches what your test shows. If your ping is setuid (weird - mine isn't) then inside the mount ping runs with the caller's UID, which is why it crashes with "Operation not permitted".

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

I will have to take a better look at the getattr functions you're referring to, as I've not made use of them yet. At a quick glance, it looks like I should be able to use them to do all of that mapping work for me?

Yes, actually it should be a simple call to getattr_comon to modify your struct stat before checking anything. That should handle the vast majority of bindfs flags. The only exception off the top of my head is when setting ACLs (via setxattr?), we might want to apply uid/gid maps, similar to how they're applied in bindfs_chown, but definitely not something to worry about in the first version.

The code looks good! I'll add review comments plus some notes to self.

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.

I know this feeling very well :smile:

[vimist]

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?

By disable I mean that setuid/setgid has no effect. That matches what your test shows.

If the mount is owned by a non-root user, it appears as though root can still execute binaries that require setuid to work. I expected it to fail, as I thought bindfs should be accessing the file with the permissions of the non-root user, but it seems that's not the case.

If your ping is setuid (weird - mine isn't)

I believe this can be done with capabilities too, which might explain this.

when setting ACLs (via setxattr?), we might want to apply uid/gid maps

Yeah, I believe you're right with regard to the setxattr call. This is what I get when I setfacl a file inside the mount:

unique: 24, opcode: SETXATTR (21), nodeid: 2, insize: 124, pid: 5316
setxattr /file system.posix_acl_access 52 0x0
DEBUG: setxattr /file system.posix_acl_access=
   unique: 24, success, outsize: 16

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!

[mpartel]

:+1:

If the mount is owned by a non-root user, it appears as though root can still execute binaries that require setuid to work. I expected it to fail, as I thought bindfs should be accessing the file with the permissions of the non-root user, but it seems that's not the case.

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 nosuid mount)". A filesystem plays no role beyond telling the kernel whether the bit is there or not (and providing the bytes in the file).

[vimist]

the next major step is to add a call to the access check code to the other bindfs_ functions.

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 :)

[mpartel]

Each operation's man page (man 2 mkdir for bindfs_mkdir etc) has fairly good documentation about these, especially where they talk about EACCES. Even if the first implementation feels uncertain, we can lean on the fairly exhaustive tests that we planned to catch almost all bugs.

[vimist]

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.

[vimist]

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 :)

[mpartel]

Your approach looks good :+1:

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.

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.

Thanks again :)

Thank you!

[vimist]

Just rebased onto master. There are a few other commits there though that do address some of the other issues.

[Victorious3]

Any Progress on this? I'm trying to get it to work with macOS but ACLs seem to work a bit differently there.