Bug 12177 - Unexpected synthesized default ACL from vfs_acl_xattr
Summary: Unexpected synthesized default ACL from vfs_acl_xattr
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on: 12028
Blocks: 12181
  Show dependency treegraph
 
Reported: 2016-08-25 17:37 UTC by Ralph Böhme
Modified: 2016-09-22 08:19 UTC (History)
4 users (show)

See Also:


Attachments
WIP patchset for master (64.49 KB, patch)
2016-08-25 17:37 UTC, Ralph Böhme
no flags Details
Patch for 4.5 cherry-picked from master (75.89 KB, patch)
2016-09-06 15:02 UTC, Ralph Böhme
jra: review+
Details
Patch for 4.4, backported from master (74.82 KB, patch)
2016-09-06 16:12 UTC, Ralph Böhme
jra: review+
Details
Patch for 4.3, backported from master (74.82 KB, patch)
2016-09-06 16:13 UTC, Ralph Böhme
jra: review+
Details
Patch for 4.4 backported from master (74.84 KB, patch)
2016-09-15 09:41 UTC, Ralph Böhme
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralph Böhme 2016-08-25 17:37:01 UTC
Created attachment 12401 [details]
WIP patchset for master

Bug #12028 changed the default ACL we return in vfs_acl_xattr|tdb for a file or directory that lacks a ACL xattr blob. Cf #12028 for the rationale behind that.

Discussion on the mailing list [1] concluded that the changed behaviour is the right thing to do, but because it can cause problems for existing setups, we revert the default behaviour and put the modified behaviour behind an option.

Patchset mostly ready, currently running private autobuilds and polishing commit messages, sneak preview attached.

[1] <https://lists.samba.org/archive/samba/2016-August/202154.html>
Comment 1 Uri Simchoni 2016-08-25 19:29:23 UTC
Really nice cleanup (esp the config handle).
In the WIP patch set - if the "windows" style is the right thing to do, why is the default "posix"?

Thanks,
Uri.
Comment 2 Jeremy Allison 2016-08-25 19:54:22 UTC
Comment on attachment 12401 [details]
WIP patchset for master

As we're commenting on the WIP :-).

Can you change get_nt_acl_internal() in patch [PATCH 06/12] to do an early
return (goto fail) if get_acl_blob returns an error status ?

Then do status = validate_nt_acl_blob() at function scope, not inside the if (NT_STATUS_IS_OK(status)) { scope.

That always looks a lot cleaner to me than nested function calls inside an if clause.
Comment 3 Ralph Böhme 2016-08-25 20:08:34 UTC
(In reply to Uri Simchoni from comment #1)
As explained in comment #0 "...because it can cause problems for existing setups, we revert the default behaviour and put the modified behaviour behind an option.".

But if others feel like making the "windows" style ACL the new default is the better decision, I'd concur.

We put the change in WHATSNEW and affected users could switch to the previous behaviour.
Comment 4 Ralph Böhme 2016-08-25 20:11:35 UTC
(In reply to Jeremy Allison from comment #2)
That won't work, because get_acl_blob() failing must be handled by continuing with fetch the sd from the filesystem in the following "if (psd == NULL) {...}" clause.
Comment 5 Ralph Böhme 2016-08-25 20:13:34 UTC
...
[1929(13428)/1958 at 3h7m8s] samba4.drs.repl_move.python(promoted_dc)(promoted_dc)

go, go, go! :)
Comment 6 Jeremy Allison 2016-08-25 20:15:12 UTC
(In reply to Ralph Böhme from comment #4)

Fair enough, wasn't looking at it in full function context, just the diff.

I'll wait until I see the finished product and shut up then :-).
Comment 7 Ralph Böhme 2016-09-06 15:02:16 UTC
Created attachment 12445 [details]
Patch for 4.5 cherry-picked from master

4.3 and 4.4 still in the loop, backporting for those versions is a little more effort.
Comment 8 Ralph Böhme 2016-09-06 16:12:45 UTC
Created attachment 12447 [details]
Patch for 4.4, backported from master
Comment 9 Ralph Böhme 2016-09-06 16:13:30 UTC
Created attachment 12448 [details]
Patch for 4.3, backported from master
Comment 10 Jeremy Allison 2016-09-06 21:48:48 UTC
Reassigning to Karolin for inclusion in 4.5.0, 4.4.next, 4.3.next.
Comment 11 Stefan Metzmacher 2016-09-07 15:12:30 UTC
Pushed to v4-5-test.
Comment 12 Karolin Seeger 2016-09-13 10:23:24 UTC
Patch does not apply on current v4-4-test:

user@host:/data/git/samba/v4-4-test$ git am v44-bug12177.patch
Applying: Revert "vfs_acl_xattr: objects without NT ACL xattr"
Applying: vfs_acl_common: rename psd to psd_blob in get_nt_acl_internal()
Applying: vfs_acl_common: rename pdesc_next to psd_fs
Applying: vfs_acl_common: remove redundant NULL assignment
Applying: vfs_acl_common: simplify ACL logic, cleanup and talloc hierarchy
Applying: vfs_acl_common: move the ACL blob validation to a helper function
Applying: vfs_acl_tdb|xattr: use a config handle
Applying: vfs_acl_common: move stat stuff to a helper function
Applying: vfs_acl_common: check for ignore_system_acls before fetching filesystem ACL
Applying: vfs_acl_xattr|tdb: add option to control default ACL style
Applying: vfs_acl_common: Windows style default ACL
Applying: s4/torture: tests for vfs_acl_xattr default ACL styles
error: patch failed: selftest/target/Samba3.pm:1690
error: selftest/target/Samba3.pm: patch does not apply
Patch failed at 0012 s4/torture: tests for vfs_acl_xattr default ACL styles
Comment 13 Karolin Seeger 2016-09-13 10:23:40 UTC
Pushed to autobuild-v4-3-test.
Comment 14 Karolin Seeger 2016-09-13 10:25:04 UTC
Re-assigning to Ralph, because the patch for 4.4 does not apply.
Comment 15 Ralph Böhme 2016-09-15 09:41:21 UTC
Created attachment 12466 [details]
Patch for 4.4 backported from master
Comment 16 Jeremy Allison 2016-09-15 18:37:05 UTC
Re-assigning to Karolin for inclusion in 4.5.next, 4.4.next, 4.3.next.
Comment 17 Karolin Seeger 2016-09-20 07:20:33 UTC
(In reply to Jeremy Allison from comment #16)
Pushed to v4-5-test.
Comment 18 Karolin Seeger 2016-09-20 07:20:46 UTC
Pushed to autobuild-v4-4-test.
Comment 19 Karolin Seeger 2016-09-22 08:19:45 UTC
Pushed to all branches.
Closing out bug report.

Thanks!