Bug 10134 - Samba 4.0 is stricter in checking acls for "open for execution"
Samba 4.0 is stricter in checking acls for "open for execution"
Status: RESOLVED FIXED
Product: Samba 4.0
Classification: Unclassified
Component: File services
4.0.0
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-10 21:40 UTC by Michael Adam
Modified: 2013-09-30 10:38 UTC (History)
1 user (show)

See Also:


Attachments
patchset for master (6.81 KB, patch)
2013-09-10 21:42 UTC, Michael Adam
ddiss: review+
vl: review+
Details
patchset for v4-0-test (6.81 KB, patch)
2013-09-10 21:56 UTC, Michael Adam
vl: review+
Details
docs: Fix typos (1.87 KB, patch)
2013-09-12 07:22 UTC, Karolin Seeger
obnox: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Adam 2013-09-10 21:40:18 UTC
In Samba 3.6, "open for execution" was successful even if the
user had no execute permissions.

In Samba 4.0 this was fixed by doing a proper ACL-check against
the provided access_mask.

While this is correct, this change in behaviour is a potential
problem for those migrating their fileserver from Samba 3.6 (or older)
to Samba 4.0 (or newer), since they need to audit their files
for missing x-bits in acls/permissions...

An option is needed to re-establish the old behaviour
in order to ease migrations.
Comment 1 Michael Adam 2013-09-10 21:42:33 UTC
Created attachment 9202 [details]
patchset for master

This patchset has been pushed to autobuild for master.
Comment 2 Michael Adam 2013-09-10 21:50:16 UTC
Comment on attachment 9202 [details]
patchset for master

The patchset applies cleanly to v4-1-test.
So once it has landed in master, the patches can be cherry-picked.
Comment 3 Michael Adam 2013-09-10 21:51:21 UTC
Comment on attachment 9202 [details]
patchset for master

asking for review for this patchset to be applied to v4-1-test
Comment 4 Michael Adam 2013-09-10 21:54:16 UTC
Comment on attachment 9202 [details]
patchset for master

asking for review for this patchset to be applied to v4-1-test
Comment 5 Michael Adam 2013-09-10 21:56:32 UTC
Created attachment 9203 [details]
patchset for v4-0-test

Patchset for 4.0:
There is one tiny difference in context here, hence the extra patchset.
If we want this for 4.0, we should redo this with cherry-pick-info once code has landed in master.

Do we want it for 4.0?
Comment 6 Michael Adam 2013-09-11 07:06:43 UTC
The patches have now gone into master as:

de3bc10ef69f23e7dab9fc3f6990bb403824b14e
1e29d730663382875d96c275c60e022a1c33a2d1
a2a3c9f36d7a19d75924cff25fa1b450d85ee6d6

Karo: for 4.1, these can be cherry-picked in this order.

For 4.0, would you like me to redo the ACKed patch with cherry-pick-info?

Cheers - Michael
Comment 7 Karolin Seeger 2013-09-11 07:12:28 UTC
(In reply to comment #5)
> Created attachment 9203 [details]
> patchset for v4-0-test
> 
> Patchset for 4.0:
> There is one tiny difference in context here, hence the extra patchset.
> If we want this for 4.0, we should redo this with cherry-pick-info once code
> has landed in master.
> 
> Do we want it for 4.0?

We need to discuss it, because usually, we do not add new parameters in minor releases. But I think in this case, it makes sense.

Are there any objections?
Comment 8 Michael Adam 2013-09-11 07:25:35 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > Created attachment 9203 [details] [details]
> > patchset for v4-0-test
> > 
> > Patchset for 4.0:
> > There is one tiny difference in context here, hence the extra patchset.
> > If we want this for 4.0, we should redo this with cherry-pick-info once code
> > has landed in master.
> > 
> > Do we want it for 4.0?
> 
> We need to discuss it, because usually, we do not add new parameters in minor
> releases. But I think in this case, it makes sense.
> 
> Are there any objections?

I am in favour of adding the parameter.

For those who would like to comment, here is the argument again:

The reason for adding the parameter is just that
an upgrade from 3.6 or earlier to a 4.0 without this switch
can present bad traps for the user.
Comment 9 Volker Lendecke 2013-09-11 07:54:41 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > Created attachment 9203 [details] [details] [details]
> > > patchset for v4-0-test
> > > 
> > > Patchset for 4.0:
> > > There is one tiny difference in context here, hence the extra patchset.
> > > If we want this for 4.0, we should redo this with cherry-pick-info once code
> > > has landed in master.
> > > 
> > > Do we want it for 4.0?
> > 
> > We need to discuss it, because usually, we do not add new parameters in minor
> > releases. But I think in this case, it makes sense.
> > 
> > Are there any objections?
> 
> I am in favour of adding the parameter.
> 
> For those who would like to comment, here is the argument again:
> 
> The reason for adding the parameter is just that
> an upgrade from 3.6 or earlier to a 4.0 without this switch
> can present bad traps for the user.

We could turn this into an undocumented parametric smbd:compat parameter. This will avoid the problem that we can not add new documented parameters within a release stream.
Comment 10 Karolin Seeger 2013-09-11 07:56:32 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #5)
> > > > Created attachment 9203 [details] [details] [details] [details]
> > > > patchset for v4-0-test
> > > > 
> > > > Patchset for 4.0:
> > > > There is one tiny difference in context here, hence the extra patchset.
> > > > If we want this for 4.0, we should redo this with cherry-pick-info once code
> > > > has landed in master.
> > > > 
> > > > Do we want it for 4.0?
> > > 
> > > We need to discuss it, because usually, we do not add new parameters in minor
> > > releases. But I think in this case, it makes sense.
> > > 
> > > Are there any objections?
> > 
> > I am in favour of adding the parameter.
> > 
> > For those who would like to comment, here is the argument again:
> > 
> > The reason for adding the parameter is just that
> > an upgrade from 3.6 or earlier to a 4.0 without this switch
> > can present bad traps for the user.
> 
> We could turn this into an undocumented parametric smbd:compat parameter. This
> will avoid the problem that we can not add new documented parameters within a
> release stream.

I prefer documented parameters.
Comment 11 Volker Lendecke 2013-09-11 08:18:32 UTC
(In reply to comment #10)
> I prefer documented parameters.

Me too. But as you pointed out, we can't add parameters within a release stream. So should we better leave this as a recommended patch somewhere on the website for people who hit this?
Comment 12 Michael Adam 2013-09-11 08:22:49 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > I prefer documented parameters.
> 
> Me too. But as you pointed out, we can't add parameters within a release
> stream. So should we better leave this as a recommended patch somewhere on the
> website for people who hit this?

Volker, there is a subtle but important difference between
"usually, we do not ..." as Karolin has put it and
your "we can't" which Karolin has not written. :-)

I think we should not make it unnecessarily complicated.
Either we add the proper option or we don't.

Cheers - Michael
Comment 13 Karolin Seeger 2013-09-11 08:26:34 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > I prefer documented parameters.
> 
> Me too. But as you pointed out, we can't add parameters within a release
> stream. So should we better leave this as a recommended patch somewhere on the
> website for people who hit this?

Of course we can addit, but we need to agree on this. And if there are no objections, I am happy to add it, because it makes sense in this case.
I just wanted to ask before skipping the commitment...
Comment 14 Jeremy Allison 2013-09-11 16:33:03 UTC
+1 on adding the parameter. Rules are not absolute, and this helps the upgrade path for users. Users are kings (and queens of course :-), without them we have no project :-).
Jeremy.
Comment 15 Karolin Seeger 2013-09-12 07:12:07 UTC
(In reply to comment #6)
> The patches have now gone into master as:
> 
> de3bc10ef69f23e7dab9fc3f6990bb403824b14e
> 1e29d730663382875d96c275c60e022a1c33a2d1
> a2a3c9f36d7a19d75924cff25fa1b450d85ee6d6
> 
> Karo: for 4.1, these can be cherry-picked in this order.
> 
> For 4.0, would you like me to redo the ACKed patch with cherry-pick-info?
> 
> Cheers - Michael

No, thanks. I added the hashes to the latest commit. I think that's sufficient.
Comment 16 Karolin Seeger 2013-09-12 07:12:37 UTC
(In reply to comment #14)
> +1 on adding the parameter. Rules are not absolute, and this helps the upgrade
> path for users. Users are kings (and queens of course :-), without them we have
> no project :-).
> Jeremy.

Pushed to autobuild-v4-1-test and autobuild-v4-0-test.
Comment 17 Karolin Seeger 2013-09-12 07:22:43 UTC
Created attachment 9206 [details]
docs: Fix typos

Follow-up patch to fix minor typos in the man page introduced by a2a3c9f36d (docs: document "acl allow execute always").
Comment 18 Karolin Seeger 2013-09-12 07:28:40 UTC
(In reply to comment #17)
> Created attachment 9206 [details]
> docs: Fix typos
> 
> Follow-up patch to fix minor typos in the man page introduced by a2a3c9f36d
> (docs: document "acl allow execute always").

Note: Patch not yet in master, sending to samba-technical first...
Comment 19 Michael Adam 2013-09-12 08:12:16 UTC
Comment on attachment 9206 [details]
docs: Fix typos

I just pushed the corresponding patch to master.

ACK from me to cherry-pick it for the release!
Comment 20 Karolin Seeger 2013-09-12 10:26:55 UTC
(In reply to comment #19)
> Comment on attachment 9206 [details]
> docs: Fix typos
> 
> I just pushed the corresponding patch to master.
> 
> ACK from me to cherry-pick it for the release!

4af7b709e in master, cherry-picked and pushed to autobuild-v4-1-test.
Comment 21 Karolin Seeger 2013-09-12 10:28:05 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > Comment on attachment 9206 [details] [details]
> > docs: Fix typos
> > 
> > I just pushed the corresponding patch to master.
> > 
> > ACK from me to cherry-pick it for the release!
> 
> 4af7b709e in master, cherry-picked and pushed to autobuild-v4-1-test.

and pushed to autobuild-v4-0-test.
Comment 22 Karolin Seeger 2013-09-16 07:07:58 UTC
Pushed to v4-0-test and v4-1-test.
Leaving bug report open as a reminder to add something to the 4.0 release notes.
Comment 23 Karolin Seeger 2013-09-30 10:38:05 UTC
(In reply to comment #22)
> Pushed to v4-0-test and v4-1-test.
> Leaving bug report open as a reminder to add something to the 4.0 release
> notes.

Added hint to the release notes.
Closing out bug report.

Thanks!