I observed the following regression in current master and 4.0 code: With the default settings, one can not give write permissions (in acls) to other users than the owner. In commit c251a6b0442abc13bc8be4ff8de324c1d7706a78 in master, the use of the security mask (and directory security mask) in the code for calculating posix acls were replaced by uses of create mask and directory mask. Therefore all additional aces were restricted by the create mask and directory mask, respectively. Now the defaults of these are set to 0744 and 0755, respectively, so one can only set r-- or r-x permissions to named aces via the nt_set_acl calls. This does not match the original intention of the create/directory mask parameters, which was to only restrict the maximum rights for newly created files (like the umask in unix). Currently the only workaround would be to set create mask and directory mask to 0777, therenby making all new files "shared by default". We should change this to only apply the create mask/directory mask values when the set_nt_acl code is called as part of a create operation (as opposed to an explicit setting set secdesc call). For 4.0, this will probably need to be a slightly hacky patch, because any conceptually better change I currently can imagine, would require adding a means to hand in to the SMB_VFS_FSET_NT_ACL() call the information that the call is done as part of a create operation.
Hideous patch forthcoming :-).
Created attachment 8280 [details] git-am fixes for 4.0.0 Patch that works here. Yes, I know how ugly this is (you don't need to point that out). Merely submit a better fix if you can see one :-). Cheers, Jeremy.
What about using no mask at all? If we'll have a NTACL we'll need to have a working posix acl.
I am currently doing more testing with this patch and working on an alternative patch: Metze is right that this change changes the result of the NT-ACL-->posixACL mapping depending on whether we are in a create or not. This has impact on the acl_xattr functionality! Thinking a bit about it: Why don't we let the mask parameters only in the original posix file creation (open_file()). As long as the share is posix-permissions only (no acls), the mask will stick. But as soon as we have ACLS / inheritance, we should not apply the masks, also not in create: The acls are there for a purpose. We should treat them correctly. Note that currently, we only apply the masks for acls when they come through inheritance, _not_ in the case where the client specifies an acl on create. This is also inconsistent. Currently testing a patch that does precisely this... more later...
I don't understand some of the comments. In the default (non-create) case we have no mask at all. Can you explain more fully please ? I understand the inconsistency when create + ACL is done, yes - we should ensure the masks are applied there. Jeremy,
Actually, I've been thinking about this further, and I finally understand metze's somewhat cryptic comment :-). Probably not using masks at all is the best solution. I'll update a new patchset. Applying the masks on create+SD is also the wrong thing to do (I'm reversing my original opinion here). Jeremy.
Created attachment 8286 [details] git-am fix for master and 4.0.0 I think this is all we need to do this right.. Jeremy.
Created attachment 8287 [details] my version of a patch for master This is the my version of the change I have been working on earlier today. It looks bigger but is similar to jeremy's patch. But it reduces the apply_default_perms() to two specialized functions: - ensure_minimal_owner_ace_perms() which does what jeremy's patch changes apply_default_perms() to do, but reduces the argument list. - trim_ace_perms() that does the masking of the ace perms so that they only contain the USR portion of the access bits (not 100% certain whether this is needed at all). Then the places where these specialized functions are called are cleaned.
Created attachment 8288 [details] patchset for 4.0 Patchset for 4.0. This also backports the prerequisite patch 9eef19a24853bbb0ce800f92d4e372b7e5977fbf "Modify ensure_canon_entry_valid() into ensure_canon_entry_valid_on_set() - makes the logic clearer." Still testing / verifying.
Created attachment 8289 [details] final patchset for master This is the final patchset for master including documentation fix after testing and discussion with Jeremy. Jeremy: Please push to master if you agree.
Created attachment 8290 [details] final patchset for v4-0-test Final patchset for 4.0 after discussion with Jeremy and testing. This picks one additional prerequisite patch and includes the docs patch.
Comment on attachment 8290 [details] final patchset for v4-0-test this patch accidentially reverts one change that had been applied in v4-0-test already but came after the "ensure_canon_entry_valid_on_set()" patch in master. correct patch following...
Created attachment 8292 [details] final. corrected patchset for v4-0-test The original patchset accidentially introduced a revert commit e122c7d24b10119c9ea4d65e0099ff1690394457 in conflict resolution. The reason was that the patches had been applied in different order. For my patchset, I need to cherry-pick patch 47ebc8fbc93ee1eb9640d9ca30275fcfc3b50026 which creates ensure_canon_entry_valid_on_set(). Originally with conflict resolution. This accidentially introduced an implicit revert of commit e122c7d24b10119c9ea4d65e0099ff1690394457. The reason was that the patches had been applied in different order. This led to the creation of bug #9469... This new patchset first reverts 2 patches that had been applied in wrong order, then cleanly cherry-picks the ensure_canon_entry_valid_on_set() patch and the two reverted ones again, cleanly. Then the two patches for this bug are cleanly cherry-picked from master on top. The diff of the resulting branch against v4-0-test with the old patchset is exaclty the accidentially reverted patch e122c7d24b10119c9ea4d65e0099ff1690394457, so this one is correct.
Comment on attachment 8292 [details] final. corrected patchset for v4-0-test Looks good
Karolin, please pick this to v4-0-test
Pushed to autobuild-v4-0-test.
Pushed to v4-0-test. Closing out bug report. Thanks!