Yes we do (so long as it doesn't open up security). ZFS-based filers are very popular, and so long as Linux doesn't have RichACLs this is the only game in town. So +1 from me ! (Jeremy).

Thanks, Ralph, we are going to look. We have related problem with the snapshots and extended attributes, could be that it lays here in fact.

(In reply to Timur Bakeyev from comment #2)
Thanks Timur! Btw, I actually built and tested the patch this time. :)

Timur, any chance for an update from your side? Afaict my patch is correct and Jeremy ACKed as well so we could push this upstream.

(In reply to Ralph Böhme from comment #4)

Hi, Ralph!

Sorry for the delay with the answer, seems I mis-configured my Gmail filters, so updates on the Bugzilla silently ended up in the folder.


It seems this issue attracted quite an attention 


There is a patch for ZFS at least for FreeBSD, that should eliminate the said error in the future versions of the OS:

https://svnweb.freebsd.org/base?view=revision&revision=329265

My colleague Andrew Walker also made his own patch, similar in principal to yours, to address the same issue:

https://github.com/freenas/samba/commit/a4f00c272a8b53d4ace40837250910d814d4813c

His patch somewhat similar to the call to make_default_filesystem_acl(DEFAULT_ACL_EVERYONE) with the exception that also owner and group ACEs are set. Also it's handy that there is a configuration option that allows to enable and disable this feature.

Originally this patch was even more sophisticated as it tried to convert mode bits into corresponding NFSv4 ACEs:

https://github.com/freenas/samba/commit/ef281ed1b7768733462adf241423a74dd23aee58#diff-6c22034d24aa266aa851875374b7a5d6

(In reply to Timur Bakeyev from comment #5)

I would suggest to take ZFS in-kernel implementation as a sample for emulation on zfsacl level. In the perfect world we wouldn't need any emulation, as ZFS(at least on FreeBSD) would perfectly handle the situation by itself, but there still be old versions of FreeBSD, ZFS on Linux and Illumos.

So, we have:

/* Common access mode for all virtual directories under the ctldir */
const u_short zfsctl_ctldir_mode = S_IRUSR | S_IXUSR | S_IRGRP | S_IXGRP |
    S_IROTH | S_IXOTH;

aka 0555.

This is translated to NFSv4 ACL with some cleanups:

777 	int
778 	zfsctl_common_getacl(ap)
779 	        struct vop_getacl_args /* {
780 	                struct vnode *vp;
781 	                acl_type_t a_type;
782 	                struct acl *a_aclp;
783 	                struct ucred *cred;
784 	                struct thread *td;
785 	        } */ *ap;
786 	{
787 	        int i;
788 	
789 	        if (ap->a_type != ACL_TYPE_NFS4)
790 	                return (EINVAL);
791 	
792 	        acl_nfs4_sync_acl_from_mode(ap->a_aclp, zfsctl_ctldir_mode, 0);
793 	        /*
794 	         * acl_nfs4_sync_acl_from_mode assumes that the owner can always modify
795 	         * attributes.  That is not the case for the ctldir, so we must clear
796 	         * those bits.  We also must clear ACL_READ_NAMED_ATTRS, because xattrs
797 	         * aren't supported by the ctldir.
798 	         */
799 	        for (i = 0; i < ap->a_aclp->acl_cnt; i++) {
800 	                struct acl_entry *entry;
801 	                entry = &(ap->a_aclp->acl_entry[i]);
802 	                uint32_t old_perm = entry->ae_perm;
803 	                entry->ae_perm &= ~(ACL_WRITE_ACL | ACL_WRITE_OWNER |
804 	                    ACL_WRITE_ATTRIBUTES | ACL_WRITE_NAMED_ATTRS |
805 	                    ACL_READ_NAMED_ATTRS );
806 	        }
807 	
808 	        return (0);
809 	}

I can imagine a separate ACL type option for the `make_default_filesystem_acl()` that would generate such a special NFSv4 ACL or, which is possibly cleaner, take Andrew's approach and implement creation of such an ACL in the `zfsacl` module itself, as it's quite specific to it.

What I miss in both implementations is the check that we are actually dealing with `.zfs/` or `.zfs/snapshot` or `.zfs/shares` directories. Not sure how critical is this. I can imagine some weird mounts of NFS or UFS file systems into ZFS share that possibly can give similar ENOSYS error.

Just for the reference:

Illumos:

https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/fs/zfs/zfs_ctldir.c

And FreeBSD:

https://svnweb.freebsd.org/base/head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c

What is the state of this one ? Seems to have stalled, and I want to make sure we fully enable ZFS ACLs as a 'first class' citizen in Samba ?

(In reply to Jeremy Allison from comment #8)
We'd love to see ZFSACL as a first class citizen as well. Or, maybe, NFSv4 in general next to POSIX, like FreeBSD does.

I've left in the comments how we'd like to see this feature to be implemented in the ZFSACL module, so next question who and how is going to implement it. I'd again suggest to join the approaches and implement emulation of the FreeBSD ZFS behavior regarding .zfs directories in the ZFSACL itself, using mocked up NFSv4 ACLs.

Not sure now about the need of the configuration switch here, as in some versions of FreeBSD we'll get those NVSv4 ACLs naturally from the OS itself without any switch, so we can only try to transparently emulate OS in the case we are on an older system.

That shouldn't affect ZFS on Linux as so far they still don't have NFSv4 ACLs, hence ZFSACL module isn't applicable.

So, the question is who is going to write the code? We can do this, but also Ralph seems interested in it. What would be your advise?

(In reply to Timur Bakeyev from comment #9)
Timur, if you have the resources, go for it. :)

(In reply to Ralph Böhme from comment #10)
Not much, but I'll prepare yet another WIP, so we can discuss it farther :)

It would also be useful to ensure this works when using vfs_acl_xattr with acl_xattr:ignore system acls = yes.  In this case, Samba has full control of the ACLs, but ZFS snapshot directories are still inaccessible (getfattr -n security.NTACL .zfs returns "Operation not supported"), and mapping the ZFS snapshots into Windows Volume Shadow Copies via vfs_shadow_copy2 also fails.

(In reply to Ted Cabeen from comment #12)

Ted, can you specify what system/OS are you referring to and what is your setup then, that uses vfs_acl_xattr?

So far we were talking about vfs_zfsacl, mostly applicable to FreeBSD(as Solaris is half-way dead and has it's own SMB implementation AFAIK).

This is on CentOS 7.4 with the ZFS on Linux modules.  In my case, I'm not using ZFS or Unix ACLs at all.  The filesystems in question are only accessed by Samba, and the POSIX mode and owner/group are largely unused.  I was concerned that a solution would be developed that mocked up ACLs from POSIX data that's unused in this scenario.

I did open an issue with the ZoL group, as I think it might make more sense to just fix this at the ZFS layer, by returning the ACL/xattrs of the ZFS filesystem  mountpoint when accessing ACLs or xattrs on the .zfs and .zfs/snapshot directories: https://github.com/zfsonlinux/zfs/issues/7253

(In reply to Timur Bakeyev from comment #5)
Picking this up again...

Timur, afaict the only difference between the patch I proposed (attachment 13845 [details]) and the patch from Andrew that you ended up using, is that Andrew's patch sets SEC_DESC_DACL_PROTECTED which is a very good idea.

Other then that, both versions should set a similar ACL with ACEs for the owner, group and everyone. Note that my patch already uses DEFAULT_ACL_POSIX not DEFAULT_ACL_EVERYONE.

I'm therefor proposing to move on with a version of my patch, that sets SEC_DESC_DACL_PROTECTED.

Any cons?

Created attachment 14186 [details]
Patch for master

Something like this. It also has the parametic option, but note that I'm defaulting to enable the behaviour.

It's an undocumented parametric option and I don't want to make it unnecessarily hard for our users to get a well working Samba on FreeBSD. :)

Created attachment 14204 [details]
Possible patch for master

(In reply to Ralph Böhme from comment #17)

Hi, Ralph!

I tried to test your patch on the recent FreeBSD system, seems it works, but there are few comments I got. 

One thing is that it would be handy to have DBG_DEBUG() statement there that indicates that we are actually synthesizing the ACLs for the given path as now it's a bit difficult to say which route was taken. With newer versions of FreeBSD OS actually produces the necessary ACLs itself, so you'd like to know.

Saying that here is difference in behavior I now observe:

# smbcacls //localhost/books .zfs -U timur
Enter CAVE\timur's password:
Failed to open \.zfs: NT_STATUS_NOT_SUPPORTED

Old behavior, deprecated.


# smbcacls //localhost/test1 .zfs -U timur
Enter CAVE\timur's password:
REVISION:1
CONTROL:SR|PD|DP
OWNER:Unix User\root
GROUP:Unix Group\wheel
ACL:Unix User\root:ALLOWED/0x0/READ
ACL:Unix Group\wheel:ALLOWED/0x0/READ
ACL:Everyone:ALLOWED/0x0/READ
ACL:NT Authority\SYSTEM:ALLOWED/0x0/FULL

This is the old FreeBSD system with the Ralph's patch applied.

# smbcacls //localhost/test1 .zfs -U timur
Enter CAVE\timur's password:
REVISION:1
CONTROL:SR|DP
OWNER:Unix User\root
GROUP:Unix Group\wheel
ACL:Unix User\root:ALLOWED/0x0/0x001200a1
ACL:Unix Group\wheel:ALLOWED/0x0/0x001200a1
ACL:Everyone:ALLOWED/0x0/0x001200a1

This is a newer FreeBSD 11.2, that synthesizes ACLs for ".zfs/" itself.

For the last two cases we see some difference in the produced ACLs. Most significant differences are PD control flag and extra ACE:

ACL:NT Authority\SYSTEM:ALLOWED/0x0/FULL

The rest flags are almost, but not exactly the same:

SEC_RIGHTS_FILE_READ    ( SEC_STD_READ_CONTROL|SEC_STD_SYNCHRONIZE|SEC_FILE_READ_DATA|SEC_FILE_READ_ATTRIBUTE|SEC_FILE_READ_EA )

vs.

SEC_RIGHTS_FILE_READ_EXEC ( SEC_STD_READ_CONTROL|SEC_STD_SYNCHRONIZE|SEC_FILE_READ_DATA|SEC_FILE_READ_ATTRIBUTE|SEC_FILE_EXECUTE )

I.e. difference is the SEC_FILE_READ_EA in your patch and SEC_FILE_EXECUTE is set by OS.

In general it would be great those code paths to match exactly, but not sure, how critical is that.

(In reply to Timur Bakeyev from comment #18)
Commit f93cc232377d4c686ac35ee5e14e798974bc0700 is now in master.

I don't think there are any critical differences, we're at least making progress. :)

Older FreeBSD versions will use the proven make_default_filesystem_acl() which is known to generate useful ACLs in vfs_acl_xattr. With newer FreeBSD versions you get a slightly different ACL with little functional difference.

We can certainly add a follow-up patch with a DEBUG statement. Do you want to provide one?

Ralph's flags are the correct ones IMHO. The native ones missing READ_EA permissions will cause problems as Samba uses EA's a lot these days.

(In reply to Jeremy Allison from comment #20)

Please note that ctldirs(.zfs/, .zfs/snapshot, .zfs/share) are purely virtual and don't have real representation in the FS hierarchy. Here is the quote from the ZFS patch that exposes .zfs/* directory:


        /*
         * acl_nfs4_sync_acl_from_mode assumes that the owner can always modify
         * attributes.  That is not the case for the ctldir, so we must clear
         * those bits.  We also must clear ACL_READ_NAMED_ATTRS, because xattrs
         * aren't supported by the ctldir.
         */

Also see https://svnweb.freebsd.org/base/head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c?view=markup

So, in case some piece of code will try to read xattrs for that directories it'll fail anyhow, being mislead by the SEC_FILE_READ_EA.

In general, variant of the DEFAULT_ACL_EVERYONE could be a better option here, as there is no much use to examine stat info for.zfs/ as it's ALWAYS will be 0555(and doesn't reflect reality in any form).

What error message will a getxattr() call return on a snapshot file ?

(In reply to Jeremy Allison from comment #22)

Not sure what you called "snapshot file".

If we are talking about actual snapshot under `.zfs/snapshot/`, for example, `.zfs/snapshot/snap1/` directory then you'll get normal xattrs and everything, as actual snapshots are automounted copies of the dataset, taken in some moment of the time and which do have all the normal ZFS dataset attributes, including xattrs and NFSv4 ACLs.

But if we are talking about `.zfs/` or `.zfs/snapshot/` or `.zfs/share/` control directories - those are purely virtual, not backed by any real FS and which exist only on the VFS level. And, being purely virtual they are crippled in certain aspects, like they don't have(and support) xattrs, you can't really change their permissions from 0555 and, possibly, some other limitations.

So, if we do:

# lsextattr user snapshot
lsextattr: snapshot: failed: Operation not supported

# getextattr user NTacl snapshot
getextattr: snapshot: failed: Operation not supported

And so on. That's behavior is the same for the old and new FreeBSD versions.

Created attachment 14552 [details]
Patch for 4.8 cherry-picked from master

The patch is already in 4.9, just 4.8 was missing the backport.

*** Bug 11658 has been marked as a duplicate of this bug. ***

Re-assigning to Karolin for inclusion in 4.8.next.

Pushed to autobuild-v4-8-test.

(In reply to Karolin Seeger from comment #27)
Pushed to v4-8-test.
Closing out bug report.

Thanks!