Implement zfs native ACL support #9709
Conversation
|
@pbhenson thank you, I'll add it to my review list so we can move this forward. |
There was a problem hiding this comment.
Thanks for rebasing and updating this. We expect to be able to merge the bulk of the remaining FreeBSD work in January. At which point, we'll be in a good position to be able to move forward with this work. As part of that we'll be able to add back the ZTS ACL tests and run them on FreeBSD as part of the CI. I expect we'll be able to round up some help for that work. Thanks for your patience!
| if (ITOZSB(ip)->z_acl_type == ZFS_ACLTYPE_NFS4ACL) { | ||
| /* | ||
| * XXX - mask could also include | ||
| * MAY_APPEND|MAY_ACCESS|MAY_OPEN|MAY_CHDIR, do we care? |
There was a problem hiding this comment.
Are there analogs for these we should be using? For example, does MAY_APPEND map to V_APPEND?
There was a problem hiding this comment.
That's an excellent question which I hoped a reviewer with more detailed knowledge of the linux vfs implementation might have an opinion on :).
V_APPEND is a flag, which can be passed in the third argument to zfs_access. If this flag is set, and the initial permission checks in zfs_zaccess fail it calls zfs_zaccess_append. I think if the mask includes MAY_APPEND we could pass in the V_APPEND flag; but I'm not completely sure of the interaction. On the zfs side, I think it asks for write privs, which might fail, but then if you want write privs just for appending, it lets you through if the ACL allows ACE_APPEND_DATA. On the linux side, does it call with both MAY_WRITE and MAY_APPEND for an appended write or just MAY_APPEND?
Also, zfs_access in this use ends up calling zfs_zaccess_rwx which converts the mode bits mask to acl format bits and then calls zfs_zaccess. If it would work out better we could convert the modes bits and other flags into acl format bits directly in zpl_permission and bypass the conversion in zfs_zaccess_rwx by passing V_ACE_MASK in the flags for the call to zfs_access.
I just don't know enough about the linux vfs permission layer to say what's best at this point 8-/.
Sounds good, thanks. Let me know when you're ready and I'll rebase again, looks like there's already a conflict :). |
Codecov Report
@@ Coverage Diff @@
## master #9709 +/- ##
=========================================
+ Coverage 79% 79% +1%
=========================================
Files 379 418 +39
Lines 114915 123781 +8866
=========================================
+ Hits 90213 97934 +7721
- Misses 24702 25847 +1145
Continue to review full report at Codecov.
|
This replaces the placeholder ZFS_PROP_PRIVATE with ZFS_PROP_ACLMODE, matching what is done in the NFSv4 ACLs PR (openzfs#9709). On FreeBSD we hide ZFS_PROP_ACLTYPE, while on Linux we hide ZFS_PROP_ACLMODE. The tests already assume this arrangement. Signed-off-by: Ryan Moeller <[email protected]>
ghost
mentioned this pull request
This replaces the placeholder ZFS_PROP_PRIVATE with ZFS_PROP_ACLMODE, matching what is done in the NFSv4 ACLs PR (openzfs#9709). On FreeBSD we hide ZFS_PROP_ACLTYPE, while on Linux we hide ZFS_PROP_ACLMODE. The tests already assume this arrangement. Signed-off-by: Ryan Moeller <[email protected]>
This replaces the placeholder ZFS_PROP_PRIVATE with ZFS_PROP_ACLMODE, matching what is done in the NFSv4 ACLs PR (#9709). On FreeBSD we hide ZFS_PROP_ACLTYPE, while on Linux we hide ZFS_PROP_ACLMODE. The tests already assume this arrangement. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes #9913
|
How goes the FreeBSD integration? Any thoughts on a timeline for following up on this pull request? It looks like there are a few merge conflicts that popped up, but they seems pretty simple to resolve. Let me know when you're ready and I'll rebase. Thanks... |
|
@pbhenson thanks for your patience. We're close to wrapping up the FreeBSD merge, once it happens I'll be sure to let you know so you can get this rebased. |
|
@pbhenson the FreeBSD work has been merged to master and the CI enabled for FreeBSD 12 and head. You can rebase this PR on master and add back the existing ACL tests. It shouldn't be too hard to get them passing in the FreeBSD CI bots. I'm sure @freqlabs who updated the test suite for FreeBSD would be happy to help. For now you'll want to add the new tests to the |
merge upstream
|
Ok, rebased, compiles successfully and seems to function ok. Still pending the opinions of the various buildbots :). @behlendorf, are there any other changes required in terms of code review of the implementation, or are we just stalled on the tests now? @freqlabs, would love some input on how to move forward with the test component. Were any of the freebsd ACL tests integrated? I still only see the posix subdir in the functional/acl directory in the tests. illumos uses ls/chmod for the native ZFS ACL, freebsd uses getfacl/setfacl, and at least for now linux is stuck using nfs4_getfacl/nfs4_setfacl. The NFSv4 ACL tools only support NFSv4 ACL semantics, which is a slight subset of the actual ZFS ACL which might complicate test compatibility across the three. I'm assuming we don't want to just import all the illumos tests? They have some for chmod options which aren't applicable, tests of ls output syntax, tests of cp/cpio/tar copying ACLs, find being able to search by ACL, etc. Maybe we could start by reviewing the tests in the trivial/nontrivial directories in the functional/acl area of illumos and deciding which ones need to be migrated? Then after that we need to sort out what kind of intermediary layer we want between the tests and the underlying commands in each OS used for ZFS ACL manipulation. Thanks... |
Codecov Report
@@ Coverage Diff @@
## master #9709 +/- ##
==========================================
- Coverage 79.46% 79.21% -0.26%
==========================================
Files 389 389
Lines 123120 123345 +225
==========================================
- Hits 97834 97703 -131
- Misses 25286 25642 +356
Continue to review full report at Codecov.
|
I'd definitely like to carefully go over the changes in e24fd00 again, but skimming over them now I don't expect any major new issues. There's also nothing holding up merging these illumos commits from my point of view. If we could get just these commits in a PR (this one or a new one) and verify the bots are happy. Then we could focus on the Linux changes and adding the tests. OpenZFS 742 - Resurrect the ZFS "aclmode" property OpenZFS 664 - Umas…
This sounds like a good approach to me. |
Cool, PR #10266 created. Once that's merged I'll rebase this one to only have the NFSv4 ACL commit. |
|
@pbhenson no NFSv4 tests have been added yet. Take a look at the xattr tests and wrappers in libtest for an example of how to approach the getfacl/setfacl abstraction. The ZFS acl tests in FreeBSD are here: https://github.com/freebsd/freebsd/tree/master/tests/sys/cddl/zfs/tests/acl |
@pbhenson merged. |
Merge master
Ok, I updated the pull request with just the NFSv4 ACL patch. Although there are a bunch of upstream merge commits in it too that my git-fu doesn't know how to get rid of? Do they matter? |
Based on: https://github.com/freebsd/freebsd/blob/master/tests/sys/cddl/zfs/tests/acl/acl_common.kshlib - # FreeBSD doesn't support ZFS extended attributes. It also doesn't support the
# same ACL mechanisms Solaris does for testing.
if [[ $os_name != "FreeBSD" ]]; then
export ZFS_XATTR="true"
export ZFS_ACL="true"
else
log_note "On FreeBSD most xattr and ACL tests are disabled"
fiIt looks like the freebsd implementation doesn't currently run any of the upstream illumos tests... |
|
We certainly have support for the extended attributes and ACLs so it must just be the tests specifically that are incompatible. |
|
FreeBSD accesses ACLs through the ACL kernel API (see https://www.freebsd.org/cgi/man.cgi?query=acl_get_file&apropos=0&sektion=0&manpath=FreeBSD+12.1-RELEASE+and+Ports&arch=default&format=html), not by pretending they are xattr. IIRC subfiles (alternate data streams) from ZFS are mapped to FreeBSD xattr. |
Yeah, like I mentioned earlier, a lot of them test specific functionality of illumos commands like ls/chmod/tar/cpio etc. And the ones that do test general zfs functionality use ls/chmod to do so.
I think these were mentioned before in this pull request commentary, or maybe in the previous one. They do seem more "general acl" in terms of coverage than the illumos ones. |
|
@behlendorf Great idea. While we're at it, can we change the help message/documentation to use |
Absolutely. It looks like |
You want to handle this in this PR along with the nfs4 acl support, or clean up the posixacl naming stuff in a separate small PR? |
|
@pbhenson my understanding is that @freqlabs was volunteering to handle this cleanup/renaming in new small PR. |
implementation. First, change the acl_type to native rather than nfs4acl. Also, while the nfs4 compatible xattr will be optional depending upon kernel configuration, the native zfs acl xattr will always be available. Finally, don't override the system nfs4 domain when mapping users/groups for the nfs4 zfs xattr, as that will no longer be the primary interface to modify them and it will be simpler for hopeful future NFS server integration port to use the same domain as the NFS server.
ghost
mentioned this pull request
|
Isn't it problematic that this appears to be implementing NFS40 ACLs rather than NFS41 ACLs per https://tools.ietf.org/html/rfc5661#section-6.2.2? Natively ZFS has NFS41 ACLs, and that is what FreeBSD supports as well. This nuance is of particular importance if you wish to use these ACLs in Samba. The NFS41 inheritance bits that are masked off to generate a NFS40 ACL (RFC3530) are vitally important to provide correct ACL inheritance behavior for an SMB server. |
AFAIK the linux client tools for NFS ACLs don't support the 4.1 extensions? However, if you review the comments you will see that in addition to the NFSv4 ACL support there is now also going to be a native interface to them which will support the full feature set of ZFS. This will be compatible with the sun acl interface so samba should be able to just work more or less. I'm hoping to get back to this project over Christmas break. |
Samba with vfs_nfs4acl_xattr defaults to an NDR-encoded NFSv41 ACL written to the nfs4acl xattr. I'm also thinking in terms of rsync usage as well. Exposing the ACL as an xattr like this will probably allow us to rsync ACLs via rsync's xattr options, but having rsync strip permissions information (which is what will happen if we don't present the full ACL) is somewhat problematic. BTW, I believe you also may need to set |
|
I'm curious if vfs_fruit |
No. That parameter does something different. It exposes the underlying POSIX mode through special ACL entries using MS-NFS SIDs (S-1-5-88-*). See here: https://docs.microsoft.com/en-us/previous-versions/windows/it-pro/windows-server-2008-R2-and-2008/hh509017(v=ws.10)?redirectedfrom=MSDN A MacOS SMB server / Client uses these special ACEs / SIDs to look under the covers at the underlying POSIX mode so that it can accurately present it to the SMB client, and also allows client to effectively chmod() through an SMB2 setinfo request. The correct approach to Samba integration will be to either (1) present the NFSv4 ACL as a suitable xattr and use vfs_nfs4acl_xatt, (2) use acl() and facl() shim like libsunacl/FreeBSD, or (3) develop a new VFS module. (1) or (2) is preferable to something new. There are nuances to how vfs_fruit interacts with ACLs that will need to be accounted for when configuring a samba server (typically on ZFS it's better to turn off the NFS ACEs because a stray client chmod() can really muck up permissions), but that goes pretty far outside the scope of this merge request. |
This replaces the placeholder ZFS_PROP_PRIVATE with ZFS_PROP_ACLMODE, matching what is done in the NFSv4 ACLs PR (openzfs#9709). On FreeBSD we hide ZFS_PROP_ACLTYPE, while on Linux we hide ZFS_PROP_ACLMODE. The tests already assume this arrangement. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#9913
|
happy anniversary to this pull request! |
|
Is this still moving to the working direction? Or is NFSv4 ACL already in the source tree？ |
Unfortunately I've been tied up with day job work and paid contract jobs and haven't had time for fun hobby coding for quite a while :(. It's still on my list to get back to at some point but I don't have a timeframe right now, sorry. |
|
For those interested, we have a complete implementation of NFSv4 ACLs for ZFS on Linux here: truenas#10 |
|
Hi everyone, |
Nice! Awesome work! Its been already merged the forked truenas repo, are you guys @freqlabs @anodos325 planing to upstream it to the main OpenZFS repo? What feature would be awesome. |
Yes |
Cool. I'm glad somebody had the time and resources to finish this up :). I'm sure openzfs will be glad to take this, but what about the upstream kernel? The last time I asked about ACL changes, they said if there wasn't an in-kernel filesystem that would use a feature, they didn't want it 8-/. |
ghost
mentioned this pull request
Happy anniversary again. Time is flying... |
|
This pull request has been superceded by PR #13186 so I'm going to close it. Sorry I ended up leaving it half done. |
Motivation and Context
No explanation is required as to why it would be nice to support the native zfs ACL
This will resolve #4966
Description
Implement ZFS native ACL support, including both an illumos compatible and a FreeBSD compatible library API for accessing them, along with ports of the FreeBSD getfacl/setfacl utilities (installed prefaced with a z) to manage them. Also provide the standard linux system.nfs4_acl extended attribute to provide an NFSv4 compatible API that can both be used with existing NFSv4 ACL management tools and hopefully future NFS server integration.
Note that this pull request has been repurposed from its original intent and is currently only partially complete.
How Has This Been Tested?
This was built and tested under Ubuntu 18.04 LTS; only a limited amount of testing to verify basic functionality has been performed at this point pending review of the code.
Types of changes
Checklist:
Signed-off-by.