Even though setting "inherit permissions = yes" correctly sets the owner of a newly created file, the code in modules/vfs_acl_common will overwrite it with the owner sid from the currently connected user token. Jeremy.
It turns out that the vfs_acl_common code doesn't actually have to handle the CHOWN or FCHOWN calls, as these are currently made only from codepaths triggered by "inherit owner = yes", which we will handle inside the inherit_new_acl() code inside modules/vfs_acl_common.c. I will post a small patch for 3.5.next here, and post a larger patch set for metze to review for master/v3-6-test on samba-technical. Jeremy.
Arg. I meant "inherit owner", not inherit permissions of course. Fixed the bug subject line.. Jeremy.
Created attachment 6402 [details] git-am fix for 3.5.next Metze - please review. If you're ok with this I'll create a similar fix for master/v3-6-test. For master v-3-6-test I'm working on adding more secure versions of the chown_fsp call, to reduce the risk of symlink races on pathnames when root, but this is a separate change to this fix. "If "inherit owner = yes", pass in the directory owner and group owner as the target for CREATOR_OWNER and CREATOR_GROUP substitutions, and also as the owner and primary group of the new security descriptor being applied to the object." Jeremy.
Jeremy, thank you for the patch! I applied it to my test env and did the tests again. I've a test directory, owned by root:root and the ace which allows my user bbaumba to write to the dir. I can create files and sub directories in the directory and the ownership is inherited to root:root without any errors on the Windows client side, great! But if I create a file (not a directory), there are NT_STATUS_ACCESS_DENIED messages on first open_acl_common() call in Sambas log file. After playing with the NT acls of the parent on windows side (added inheritance to sub dirs/files and removed the rule to delete and change the ownership) I get files with -r--rwx---+ root root permissions and ACCESS_DENIED messages if I read the NT ACLS with Windows. But I can see bbaumba's permissions and get no errors on windows side.
I did some additional tests. I created a directory and set the ownership to root:bbaumba With Windows I added the rules for the group, that allows users to create, write, read, ... and NOT to delete files or change the permissions/ownership (and inherit them). getfacl tells me now default:group:bbaumba:rwx which is correct. I can now create files in the directory, can NOT remove them (fine), but unfortunately not write to them. If I edit a text file and try to safe my changes Samba says: [2011/04/13 16:39:34.419047, 5] smbd/open.c:1769(open_file_ntcreate) open_file_ntcreate: write access requested for file testdir3/Neu Textdokument (5).txt on read only file [2011/04/13 16:39:34.419069, 5] smbd/files.c:497(file_free) freed files structure 18316 (1 used) [2011/04/13 16:39:34.419085, 10] smbd/open.c:3209(create_file_unixpath) create_file_unixpath: NT_STATUS_ACCESS_DENIED [2011/04/13 16:39:34.419100, 10] smbd/open.c:3462(create_file_default) create_file: NT_STATUS_ACCESS_DENIED But getfacl says it should be allowed: # owner: root # group: bbaumba user::r-x group::rwx other::r-x Same with smbcacl and Windows: OWNER:BBASDF\root GROUP:Unix Group\bbaumba ACL:Unix Group\bbaumba:ALLOWED/0x0/RWX
This error: "open_file_ntcreate: write access requested for file testdir3/Neu Textdokument (5).txt on read only file" is a check on DOS attributes, not on the permissions of a file. Using a GUI text editor may be trying to do other things than simply open the file for write. You might want to make sure you also have WRITE_ATTRIBUTES set on the inherited ACL, as well as WRITE_DATA. Don't use a GUI test, try just doing: copy file.txt \\server\dropbox_share\dir and then: echo >>\\server\dropbox_share\dir\file.txt to test this. That should match more what a dropbox can realistically do. Remember, trying to use complex apps. on this will fail, as things like MS-Office do: create new file with tmp name write data rename tmp name -> orig name when editing a file. The "rename" here will fail, as it requires DELETE access to the file (which the dropbox denies). Dropboxes are limited. I still think this patch is good :-). Jeremy.
Okay, you're right, this patch is good. Dropped files with your patches and the owner was inherited successfully. :-) Thank you very much :-)
Metze please review asap as I'd like to get this fix into master and v3-6-test (and I'm honoring your request not to do that without review :-). Jeremy.
I guess that's fine for 3.6 and master.
Can you also bless this for 3.5.next and I'll reassign to Karolin for inclusion. Thanks, Jeremy.
Comment on attachment 6402 [details] git-am fix for 3.5.next Looks good
Karolin, please pick for the next release
Patch does not apply (HEAD 4a46715): user@host:/data/git/samba/v3-5-test> git am 0001-Fix-bug-8083-inherit-owner-yes-doesn-t-interact-corr.patch Applying: Fix bug #8083 - "inherit owner = yes" doesn't interact correctly with vfs_acl_xattr or vfs_acl_tdb module. error: patch failed: source3/modules/vfs_acl_common.c:472 error: source3/modules/vfs_acl_common.c: patch does not apply Patch failed at 0001 Fix bug #8083 - "inherit owner = yes" doesn't interact correctly with vfs_acl_xattr or vfs_acl_tdb module. When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort".
Created attachment 6417 [details] git-am fix for 3.5.next Karolin - please try this version (should apply to v3-5-test cleanly). It should be identical to the version that won't apply (don't know why that failed). Jeremy.
Comment on attachment 6417 [details] git-am fix for 3.5.next Looks ok, too
Karolin, your turn again :-)
Pushed to v3-5-test. Closing out bug report. Thanks!
with inherit permissions = yes and inherit acls = yes inherit owner = yes does also not work. Jeremy, can you have another look at that combination?
Ok, reproduced this. I'll look into it asap. Jeremy.
Created attachment 6540 [details] git-am fix for 3.6.0
Created attachment 6541 [details] git-am fix for 3.5.9. Fix for 3.5.9 as well. If the fix goes into 3.5.9 it also needs to go into 3.6.0-final so we don't regress between 3.5.9 -> 3.6.0. Jeremy.
just tried the 3.5 patch for now and that patch doesn't fix it. By the way - the problem occurs only with created directories. For created files "inherit owner" works for me.
Very strange. It fixes it perfectly for me (plus it also occurred on files). What settings do you have in the share stanza for your smb.conf ? I have: path = /tmp/foo vfs objects = acl_xattr inherit permissions = yes inherit acls = yes inherit owner = yes read only = no guest ok = true The logic added should certainly fix this issue. Can you describe more how you're reproducing this ? Jeremy.
the main difference that probably matters is that I testes without the acl_xattr module but just POSIX ACLs
Oh, then that is a new bug. Look at the title of this bug : ""inherit owner = yes" doesn't interact correctly with vfs_acl_xattr or vfs_acl_tdb module" Kind of implies the vfs_acl is part of the bug report :-). Ok, please open a new bug - the logic changes here are certainly needed for use with acl_xattr. The new bug can track the posix acl-only problem. Please confirm the fix on this bug works with acl_xattr, and we can get this change into 3.5.9 and 3.6.0-final. Jeremy.
Comment on attachment 6540 [details] git-am fix for 3.6.0 works for me with the xattr acl module, thanks!
Re-assigning to Karolin. Karolin, patch : https://attachments.samba.org/attachment.cgi?id=6541 needs to go into 3.5.9, and patch: https://attachments.samba.org/attachment.cgi?id=6540 needs to go into 3.6.0-final. Thanks ! Jeremy.
(In reply to comment #27) > Re-assigning to Karolin. > > Karolin, patch : > > https://attachments.samba.org/attachment.cgi?id=6541 Unfortunately, it's too late for 3.5.9. Do we need to delay the release or is shipping the fix with 3.5.10 ok? > needs to go into 3.5.9, and patch: > > https://attachments.samba.org/attachment.cgi?id=6540 > > needs to go into 3.6.0-final. Pushed, thanks! > Thanks ! > > Jeremy.
(In reply to comment #28) > (In reply to comment #27) > > Re-assigning to Karolin. > > > > Karolin, patch : > > > > https://attachments.samba.org/attachment.cgi?id=6541 > > Unfortunately, it's too late for 3.5.9. > Do we need to delay the release or is shipping the fix with 3.5.10 ok? Pushed to v3-5-test. If there are no objections, it will be included in 3.5.9. Closing out bug report. Thanks!