You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Fuhrmann <st...@wandisco.com> on 2015/05/13 08:21:37 UTC
Recursive operations and authz
Hi devs,
At WANdisco, people have been playing with the new
wildcard support for authz (see authz-performance branch)
and ran into an interesting problem.
Today, recursive operations (COPY, DELETE and MOVE)
require recursive access rights on the respective trees.
For instance, COPY requires recursive read rights on the
source - which is fine b/c we don't want the copy to expose
previously protected data.
The problem with authz, however, is that we don't perform
a full tree crawl (and don't intend to) but check the authz
file for rules that *may* affect the respective sub-tree. A
rule like [:glob:/**/yeti] will *always* match whether there
are "yeti"s in your repo or not. Without wildcard support
the problem exists as well but is less prevalent because
tend to write explicit rules for paths that don't exist. For
wildcard rules, OTOH, it is almost the whole point to cover
existing and not-yet existing, future paths was well.
My suggestion is to relax the requirements as follows:
COPY & MOVE still require recursive read access on
the source but only non-recursive write access on the
destination. Thus it becomes possible to protect data
in branches and tags right from their inception without
relaxing read access requirements to existing data. The
attached patch achieves just that.
I have 3 questions:
(1) Is there something fundamentally wrong with this
approach, e.g. braking major use-cases?
(2) Should we require write access to target and target's
parent folder (as done by the patch) or just to the target's
parent folder?
(3) Should we (optionally?) reduce the access rights reqs
for DELETE similarly such that no recursive write access
is needed? That seems risky but would be symmetrical
to creating copies with r/o or n/a sub-paths. MOVE would
be updated accordingly (always the same reqs as a COPY
+ DELETE).
-- Stefan^2.
Re: Recursive operations and authz
Posted by Branko Čibej <br...@wandisco.com>.
On 13.05.2015 08:21, Stefan Fuhrmann wrote:
> I have 3 questions:
>
> (1) Is there something fundamentally wrong with this
> approach, e.g. braking major use-cases?
It will certainly change how some authz files work, but that can be
fixed. The net effect is be that users users would be able to copy or
move to a location from which they are note able to move away from or
delete afterwards, whereas currently they wouldn't be able to perform
the copy or move in the first place.
This reduces authz restrictions for the same authz configuration, but
since it's a server-side change and easily compensated for in the authz
config if the current behaviour is what the admin intended, I don't see
a real problem.
> (2) Should we require write access to target and target's
> parent folder (as done by the patch) or just to the target's
> parent folder?
This is a tricky question. Since there are valid arguments for both
behaviours, I vote for consistency: we currently require write to
target+parent, so let's keep it that way.
> (3) Should we (optionally?) reduce the access rights reqs
> for DELETE similarly such that no recursive write access
> is needed? That seems risky but would be symmetrical
> to creating copies with r/o or n/a sub-paths. MOVE would
> be updated accordingly (always the same reqs as a COPY
> + DELETE).
Unlike the answer to your question (1) this is a definite no. If we do
this, admins will have no way to restore current behaviour.
The answer might be different it we had an explicit 'delete' permission;
but we don't, it's implied by the 'write' permission so it has to remain
recursive.
-- Brane
Re: Recursive operations and authz
Posted by Branko Čibej <br...@wandisco.com>.
On 16.05.2015 22:32, Ivan Zhakov wrote:
>> In most CM workflows I've ever seen, a tag is assumed to be a read-only
>> snapshot since its creation. FWIW, even with the required authz support
>> in place, we still wouldn't have real tags, just as we don't have real
>> branches; there's more to the semantics of these concepts than just
>> access patterns.
>>
> I meant it would be nice to have option in authz file to say "I allow
> creating tags in this folder, but I don't allow to modify or delete
> them". The create permissions looks like solution for this use. But it
> doesn't cover case when tag created from working, i.e. 'copy then
> modify some files'
That's an interesting point: WC-to-repos copy is tricky. I wonder how
hard it would be to somehow incorporate the fact that the copy and
modification are happening in the same transaction/edit drive into the
authz decisions, without exposing the concept of transactions outside
the repository layer.
-- Brane
Re: Recursive operations and authz
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 16 May 2015 at 22:30, Branko Čibej <br...@wandisco.com> wrote:
> On 16.05.2015 20:34, Ivan Zhakov wrote:
>> On 15 May 2015 at 07:04, Stefan Fuhrmann <st...@wandisco.com> wrote:
>>> Thank you to everyone who answered! From what I gathered
>>> so far is this:
>>>
>>> to (1) Requirering recursive or non-recursive write on a copy target
>>> should not make a difference to a typical authz setup with the
>>> current /trunk code. However, the provided paths *is* a change
>>> that should not be committed to /trunk as is.
>>>
>>> Reducing the required access rights to just non-recursive write
>>> to a copy target makes sense. So, 3rd party distributions may
>>> use the patch (e.g. to solve issues with wildcard use cases)
>>> if they are able to communicate the change in behavior to their
>>> users. Their risk is that new SVN releases will address this
>>> problem differently.
>>>
>>> to (2) Requirering write access to the target *and* its parent, i.e.
>>> today's behavior, should be kept for the time being. No compelling
>>> argument can be given at this time that makes checking only the
>>> parent the clearly better option
>>>
>>> to (3) Delete (hence, move) should continue to require recursive
>>> write access to the (source) path. Otherwise, we would change
>>> the intended behavior of existing setups.
>>>
>>> Further feedback:
>>>
>>> * Ideally, we would traverse the actuall sub-tree and check against
>>> the authz rules instead of using the authz recursion approximation.
>>>
>>> * mod_dav might perform that recursion by default. From talking
>>> to Ben, it seems though that is probably not the case.
>>>
>>> Idea how to solve the issue in SVN proper:
>>>
>>> * Introduce c(reate) and d(elete) access rights. The data structures
>>> on the authzperf branch have plenty of unused bit flags left.
>>>
>> FWIW: Note that tag creation usually consist copy + modify some files
>> in commit. Like svn_version.h in our case [1]. I mean that granting
>> 'create' access to tags folder wouldn't be enough to allow tags
>> creation, but prevent commits to them.
>
> Sure. But that's a case of adapting the workflow to the tool; we take
> advantage of the fact that Subversion doesn't have real tags. If it did,
> our workflow would probably have been different, too.
>
Yes, but Subversion users already adopted this workflow and it would
be nice if any authz improvements will be useful in their workflow.
> In most CM workflows I've ever seen, a tag is assumed to be a read-only
> snapshot since its creation. FWIW, even with the required authz support
> in place, we still wouldn't have real tags, just as we don't have real
> branches; there's more to the semantics of these concepts than just
> access patterns.
>
I meant it would be nice to have option in authz file to say "I allow
creating tags in this folder, but I don't allow to modify or delete
them". The create permissions looks like solution for this use. But it
doesn't cover case when tag created from working, i.e. 'copy then
modify some files'
--
Ivan Zhakov
Re: Recursive operations and authz
Posted by Branko Čibej <br...@wandisco.com>.
On 16.05.2015 20:34, Ivan Zhakov wrote:
> On 15 May 2015 at 07:04, Stefan Fuhrmann <st...@wandisco.com> wrote:
>> Thank you to everyone who answered! From what I gathered
>> so far is this:
>>
>> to (1) Requirering recursive or non-recursive write on a copy target
>> should not make a difference to a typical authz setup with the
>> current /trunk code. However, the provided paths *is* a change
>> that should not be committed to /trunk as is.
>>
>> Reducing the required access rights to just non-recursive write
>> to a copy target makes sense. So, 3rd party distributions may
>> use the patch (e.g. to solve issues with wildcard use cases)
>> if they are able to communicate the change in behavior to their
>> users. Their risk is that new SVN releases will address this
>> problem differently.
>>
>> to (2) Requirering write access to the target *and* its parent, i.e.
>> today's behavior, should be kept for the time being. No compelling
>> argument can be given at this time that makes checking only the
>> parent the clearly better option
>>
>> to (3) Delete (hence, move) should continue to require recursive
>> write access to the (source) path. Otherwise, we would change
>> the intended behavior of existing setups.
>>
>> Further feedback:
>>
>> * Ideally, we would traverse the actuall sub-tree and check against
>> the authz rules instead of using the authz recursion approximation.
>>
>> * mod_dav might perform that recursion by default. From talking
>> to Ben, it seems though that is probably not the case.
>>
>> Idea how to solve the issue in SVN proper:
>>
>> * Introduce c(reate) and d(elete) access rights. The data structures
>> on the authzperf branch have plenty of unused bit flags left.
>>
> FWIW: Note that tag creation usually consist copy + modify some files
> in commit. Like svn_version.h in our case [1]. I mean that granting
> 'create' access to tags folder wouldn't be enough to allow tags
> creation, but prevent commits to them.
Sure. But that's a case of adapting the workflow to the tool; we take
advantage of the fact that Subversion doesn't have real tags. If it did,
our workflow would probably have been different, too.
In most CM workflows I've ever seen, a tag is assumed to be a read-only
snapshot since its creation. FWIW, even with the required authz support
in place, we still wouldn't have real tags, just as we don't have real
branches; there's more to the semantics of these concepts than just
access patterns.
-- Brane
Re: Recursive operations and authz
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 15 May 2015 at 07:04, Stefan Fuhrmann <st...@wandisco.com> wrote:
> Thank you to everyone who answered! From what I gathered
> so far is this:
>
> to (1) Requirering recursive or non-recursive write on a copy target
> should not make a difference to a typical authz setup with the
> current /trunk code. However, the provided paths *is* a change
> that should not be committed to /trunk as is.
>
> Reducing the required access rights to just non-recursive write
> to a copy target makes sense. So, 3rd party distributions may
> use the patch (e.g. to solve issues with wildcard use cases)
> if they are able to communicate the change in behavior to their
> users. Their risk is that new SVN releases will address this
> problem differently.
>
> to (2) Requirering write access to the target *and* its parent, i.e.
> today's behavior, should be kept for the time being. No compelling
> argument can be given at this time that makes checking only the
> parent the clearly better option
>
> to (3) Delete (hence, move) should continue to require recursive
> write access to the (source) path. Otherwise, we would change
> the intended behavior of existing setups.
>
> Further feedback:
>
> * Ideally, we would traverse the actuall sub-tree and check against
> the authz rules instead of using the authz recursion approximation.
>
> * mod_dav might perform that recursion by default. From talking
> to Ben, it seems though that is probably not the case.
>
> Idea how to solve the issue in SVN proper:
>
> * Introduce c(reate) and d(elete) access rights. The data structures
> on the authzperf branch have plenty of unused bit flags left.
>
FWIW: Note that tag creation usually consist copy + modify some files
in commit. Like svn_version.h in our case [1]. I mean that granting
'create' access to tags folder wouldn't be enough to allow tags
creation, but prevent commits to them.
--
Ivan Zhakov
Re: Recursive operations and authz
Posted by Branko Čibej <br...@wandisco.com>.
On 15.05.2015 06:04, Stefan Fuhrmann wrote:
> Thank you to everyone who answered! From what I gathered
> so far is this:
>
> to (1) Requirering recursive or non-recursive write on a copy target
> should not make a difference to a typical authz setup with the
> current /trunk code. However, the provided paths *is* a change
> that should not be committed to /trunk as is.
>
> Reducing the required access rights to just non-recursive write
> to a copy target makes sense. So, 3rd party distributions may
> use the patch (e.g. to solve issues with wildcard use cases)
> if they are able to communicate the change in behavior to their
> users. Their risk is that new SVN releases will address this
> problem differently.
>
> to (2) Requirering write access to the target *and* its parent, i.e.
> today's behavior, should be kept for the time being. No compelling
> argument can be given at this time that makes checking only the
> parent the clearly better option
>
> to (3) Delete (hence, move) should continue to require recursive
> write access to the (source) path. Otherwise, we would change
> the intended behavior of existing setups.
>
> Further feedback:
>
> * Ideally, we would traverse the actuall sub-tree and check against
> the authz rules instead of using the authz recursion approximation.
> * mod_dav might perform that recursion by default. From talking
> to Ben, it seems though that is probably not the case.
>
> Idea how to solve the issue in SVN proper:
>
> * Introduce c(reate) and d(elete) access rights. The data structures
> on the authzperf branch have plenty of unused bit flags left.
>
> * If "c" is set on a path, you may create sub-nodes in it, e.g. by
> copying another node into it. No further rights are required on the
> target path (e.g. the whole tag including base folder is r/o).
Oops. Typically, "create" permission applies to the target, not its parent.
> * If "d" is set on a path, you may delete any sub-node - sub-tree
> included and no further rights are required.
As above; "delete" permission applies to the target. If we change that,
this would /really/ mean a change in philosophy.
Note that in a typical filesystem "move" and "delete" behave
differently: "move" requires non-recursive write access on the source
and target, whereas "delete" requires recursive write access on the
source. Our current authz implementation implicitly treats "move" as
"copy+delete".
> * Recursive write access to a path implies "c" and "d" are granted
> on that folder. This provides backward compat and allows authz
> users to only check for the new flags. The new authzperf data
> structs already comprise min/max sub-tree rights, making derived
> c&d rights available at virtually no extra costs. If there is no
> recursive
> write access on the parent (rare case), fall back to "write on parent,
> rec.write on target" check.
>
> * c and d do not imply any of the existing rights. While you probably
> want to set "r" with them, there is no reason that you have to. As a
> result, script users only need to know the path and have those new
> rights to manage branches & tags. No need to give them access to
> their contents.
The DAG of access rights should probably look like this, with each node
implying all its predecessors:
none
|
+-> traverse (lookup) ------+
| |
| +-> write contents ----|---------+
| | | |
+-> read contents -------+ | |
| | | |
| +--+-> read -+-> write
| | |
+-> read properties -----+ |
| | |
| +-> write properties ------------+
| |
+-> create ---------------------------+
| |
+-> delete ---------------------------+
Some systems treat create and delete permissions as separate from (not
implied by) write permission; many systems (notably Unix-like) do not
imply read permission by write permission. But we probably won't do that
because it would be a quite fundamental change in authz semantics, and
'write-only' probably doesn't make much sense in Subversion. On the
other hand ... create-only or delete-only are just as far-fetched, so
maybe we should change the graph such that they imply the read permission.
-- Brane
Re: Recursive operations and authz
Posted by Stefan Fuhrmann <st...@wandisco.com>.
Thank you to everyone who answered! From what I gathered
so far is this:
to (1) Requirering recursive or non-recursive write on a copy target
should not make a difference to a typical authz setup with the
current /trunk code. However, the provided paths *is* a change
that should not be committed to /trunk as is.
Reducing the required access rights to just non-recursive write
to a copy target makes sense. So, 3rd party distributions may
use the patch (e.g. to solve issues with wildcard use cases)
if they are able to communicate the change in behavior to their
users. Their risk is that new SVN releases will address this
problem differently.
to (2) Requirering write access to the target *and* its parent, i.e.
today's behavior, should be kept for the time being. No compelling
argument can be given at this time that makes checking only the
parent the clearly better option
to (3) Delete (hence, move) should continue to require recursive
write access to the (source) path. Otherwise, we would change
the intended behavior of existing setups.
Further feedback:
* Ideally, we would traverse the actuall sub-tree and check against
the authz rules instead of using the authz recursion approximation.
* mod_dav might perform that recursion by default. From talking
to Ben, it seems though that is probably not the case.
Idea how to solve the issue in SVN proper:
* Introduce c(reate) and d(elete) access rights. The data structures
on the authzperf branch have plenty of unused bit flags left.
* If "c" is set on a path, you may create sub-nodes in it, e.g. by
copying another node into it. No further rights are required on the
target path (e.g. the whole tag including base folder is r/o).
* If "d" is set on a path, you may delete any sub-node - sub-tree
included and no further rights are required.
* Recursive write access to a path implies "c" and "d" are granted
on that folder. This provides backward compat and allows authz
users to only check for the new flags. The new authzperf data
structs already comprise min/max sub-tree rights, making derived
c&d rights available at virtually no extra costs. If there is no recursive
write access on the parent (rare case), fall back to "write on parent,
rec.write on target" check.
* c and d do not imply any of the existing rights. While you probably
want to set "r" with them, there is no reason that you have to. As a
result, script users only need to know the path and have those new
rights to manage branches & tags. No need to give them access to
their contents.
-- Stefan^2.
Re: Recursive operations and authz
Posted by Branko Čibej <br...@wandisco.com>.
On 13 May 2015 at 12:08, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Stefan Fuhrmann wrote on Wed, May 13, 2015 at 08:21:37 +0200:
>> Hi devs,
>>
>> At WANdisco, people have been playing with the new
>> wildcard support for authz (see authz-performance branch)
>> and ran into an interesting problem.
>>
>> Today, recursive operations (COPY, DELETE and MOVE)
>> require recursive access rights on the respective trees.
>> For instance, COPY requires recursive read rights on the
>> source - which is fine b/c we don't want the copy to expose
>> previously protected data.
>>
>> The problem with authz, however, is that we don't perform
>> a full tree crawl (and don't intend to) but check the authz
>> file for rules that *may* affect the respective sub-tree. A
>> rule like [:glob:/**/yeti] will *always* match whether there
>> are "yeti"s in your repo or not. Without wildcard support
>> the problem exists as well but is less prevalent because
>> tend to write explicit rules for paths that don't exist. For
>> wildcard rules, OTOH, it is almost the whole point to cover
>> existing and not-yet existing, future paths was well.
>>
>> My suggestion is to relax the requirements as follows:
>> COPY & MOVE still require recursive read access on
>> the source but only non-recursive write access on the
>> destination. Thus it becomes possible to protect data
>> in branches and tags right from their inception without
>> relaxing read access requirements to existing data. The
>> attached patch achieves just that.
>>
>> I have 3 questions:
>>
>> (1) Is there something fundamentally wrong with this
>> approach, e.g. braking major use-cases?
>
> How about inventing a 'c' permission, in addition to the existing 'r'
> and 'rw', with the following semantics: if the authz file contains
> '[/tags] alice=c', then alice is authorized to create immediate children
> of /tags, possibly as adds-with-history, without needing recursive write
> access to the copy destination. Would this address your use-case?
The place to add new kinds of access rights, different rule semantics,
etc. is the authzperf branch, not trunk/1.9.0.
-- Brane
AW: Recursive operations and authz
Posted by Markus Schaber <m....@codesys.com>.
Hi, Brane,
Von: Branko Čibej [mailto:brane@wandisco.com]
> On 13 May 2015 at 12:19, Markus Schaber <m....@codesys.com>
> wrote:
> >> Von: Daniel Shahaf [mailto:d.s@daniel.shahaf.name] Stefan Fuhrmann
> >> wrote on Wed, May 13, 2015 at 08:21:37 +0200:
> >> > Hi devs,
> >> >
> >> > [...]
> >> >
> >> > (1) Is there something fundamentally wrong with this
> >> > approach, e.g. braking major use-cases?
> >>
> >> How about inventing a 'c' permission, in addition to the existing
> 'r'
> >> and 'rw', with the following semantics: if the authz file contains
> >> '[/tags] alice=c', then alice is authorized to create immediate
> >> children of /tags, possibly as adds-with-history, without needing
> >> recursive write access to the copy destination. Would this
> address
> >> your use-case?
> >
> > I like the general suggestion, but 'c' made me immediately think
> > 'copy', which could be the right to copy the tree to somewhere
> else.
> >
> > Thus, I suggest 'a' for 'add', the right to add children (including
> their subtrees).
>
>
> It's 'c' for 'create', pretty standard in the ACL world. Let's not
> invent our own terminology.
If it's a standard term in the ACL world, then I fully accept it.
Best regards
Markus Schaber
CODESYS® a trademark of 3S-Smart Software Solutions GmbH
Inspiring Automation Solutions
3S-Smart Software Solutions GmbH
Dipl.-Inf. Markus Schaber | Product Development Core Technology
Memminger Str. 151 | 87439 Kempten | Germany
Tel. +49-831-54031-979 | Fax +49-831-54031-50
E-Mail: m.schaber@codesys.com | Web: http://www.codesys.com | CODESYS store: http://store.codesys.com
CODESYS forum: http://forum.codesys.com
Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner | Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
This e-mail may contain confidential and/or privileged information. If you are not the intended recipient (or have received
this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorised copying, disclosure
or distribution of the material in this e-mail is strictly forbidden.
Re: Recursive operations and authz
Posted by Branko Čibej <br...@wandisco.com>.
On 13 May 2015 at 12:19, Markus Schaber <m....@codesys.com> wrote:
> Hi, Daniel,
>
>> Von: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>> Stefan Fuhrmann wrote on Wed, May 13, 2015 at 08:21:37 +0200:
>> > Hi devs,
>> >
>> > [...]
>> >
>> > (1) Is there something fundamentally wrong with this
>> > approach, e.g. braking major use-cases?
>>
>> How about inventing a 'c' permission, in addition to the existing 'r'
>> and 'rw', with the following semantics: if the authz file contains
>> '[/tags] alice=c', then alice is authorized to create immediate
>> children of /tags, possibly as adds-with-history, without needing
>> recursive write access to the copy destination. Would this address
>> your use-case?
>
> I like the general suggestion, but 'c' made me immediately think 'copy',
> which could be the right to copy the tree to somewhere else.
>
> Thus, I suggest 'a' for 'add', the right to add children (including their subtrees).
It's 'c' for 'create', pretty standard in the ACL world. Let's not
invent our own terminology.
-- Brane
AW: Recursive operations and authz
Posted by Markus Schaber <m....@codesys.com>.
Hi, Daniel,
> Von: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Stefan Fuhrmann wrote on Wed, May 13, 2015 at 08:21:37 +0200:
> > Hi devs,
> >
> > [...]
> >
> > (1) Is there something fundamentally wrong with this
> > approach, e.g. braking major use-cases?
>
> How about inventing a 'c' permission, in addition to the existing 'r'
> and 'rw', with the following semantics: if the authz file contains
> '[/tags] alice=c', then alice is authorized to create immediate
> children of /tags, possibly as adds-with-history, without needing
> recursive write access to the copy destination. Would this address
> your use-case?
I like the general suggestion, but 'c' made me immediately think 'copy',
which could be the right to copy the tree to somewhere else.
Thus, I suggest 'a' for 'add', the right to add children (including their subtrees).
If this right is non-recursive, people may add files, directories or whole trees (in the same transaction), but not modify them later.
Best regards
Markus Schaber
CODESYS(r) a trademark of 3S-Smart Software Solutions GmbH
Inspiring Automation Solutions
3S-Smart Software Solutions GmbH
Dipl.-Inf. Markus Schaber | Product Development Core Technology
Memminger Str. 151 | 87439 Kempten | Germany
Tel. +49-831-54031-979 | Fax +49-831-54031-50
E-Mail: m.schaber@codesys.com | Web: http://www.codesys.com | CODESYS store: http://store.codesys.com
CODESYS forum: http://forum.codesys.com
Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner | Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
This e-mail may contain confidential and/or privileged information. If you are not the intended recipient (or have received
this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorised copying, disclosure
or distribution of the material in this e-mail is strictly forbidden.
Re: Recursive operations and authz
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Wed, May 13, 2015 at 08:21:37 +0200:
> Hi devs,
>
> At WANdisco, people have been playing with the new
> wildcard support for authz (see authz-performance branch)
> and ran into an interesting problem.
>
> Today, recursive operations (COPY, DELETE and MOVE)
> require recursive access rights on the respective trees.
> For instance, COPY requires recursive read rights on the
> source - which is fine b/c we don't want the copy to expose
> previously protected data.
>
> The problem with authz, however, is that we don't perform
> a full tree crawl (and don't intend to) but check the authz
> file for rules that *may* affect the respective sub-tree. A
> rule like [:glob:/**/yeti] will *always* match whether there
> are "yeti"s in your repo or not. Without wildcard support
> the problem exists as well but is less prevalent because
> tend to write explicit rules for paths that don't exist. For
> wildcard rules, OTOH, it is almost the whole point to cover
> existing and not-yet existing, future paths was well.
>
> My suggestion is to relax the requirements as follows:
> COPY & MOVE still require recursive read access on
> the source but only non-recursive write access on the
> destination. Thus it becomes possible to protect data
> in branches and tags right from their inception without
> relaxing read access requirements to existing data. The
> attached patch achieves just that.
>
> I have 3 questions:
>
> (1) Is there something fundamentally wrong with this
> approach, e.g. braking major use-cases?
How about inventing a 'c' permission, in addition to the existing 'r'
and 'rw', with the following semantics: if the authz file contains
'[/tags] alice=c', then alice is authorized to create immediate children
of /tags, possibly as adds-with-history, without needing recursive write
access to the copy destination. Would this address your use-case?
Unlike your suggestion, this way is provably backwards compatible:
admins who don't put 'c' in their authz rules will get exactly the same
behaviour as today.
Could you describe the use-case in more detail? All I understood was
that it has something to do with yetis and with immutable tags, but I'd
really like to understand in more detail what's the problem being solved
here before I try to think about how to solve it...
Daniel
> (2) Should we require write access to target and target's
> parent folder (as done by the patch) or just to the target's
> parent folder?
> (3) Should we (optionally?) reduce the access rights reqs
> for DELETE similarly such that no recursive write access
> is needed? That seems risky but would be symmetrical
> to creating copies with r/o or n/a sub-paths. MOVE would
> be updated accordingly (always the same reqs as a COPY
> + DELETE).
>
> -- Stefan^2.
Re: Recursive operations and authz
Posted by Branko Čibej <br...@wandisco.com>.
On 13.05.2015 18:18, C. Michael Pilato wrote:
> On 05/13/2015 10:35 AM, Branko Čibej wrote:
>> On 13 May 2015 at 15:24, C. Michael Pilato <cm...@collab.net> <ma...@collab.net> wrote:
>>>
>>> Well, the use-case being broken here is kinda the obvious one: I
>>> shouldn't have permission to create/delete some path /foo/bar (it's a
>>> system-critical file that shouldn't go away, or a password-bearing file
>>> path blocked by the VC system because devs keep stupidly committing
>>> them, or...), but you've provided and end-around for me to do so. :-)
>>> In the copy and move scenarios, you're allowing me to create a path I
>>> can't later delete or further modify. Now, while this is a very handy
>>> semantic for tag creation[1], it's also something an administrator
>>> should be able to make a conscious decision about, not just an
>>> unfortunate side-effect of an effort to make the code faster.
>> This proposal really has nothing to do with performance; it's about
>> intended/consistent authz semantics, which we never really bothered to
>> define, let alone prove. IMO, the fact that you can have authz rules
>> that allow you to create a directory but not its children is
>> unintended. I bet no-one thought about it during the initial
>> development of mod_authz_svn.
>
> Dude. The Subversion authz subsystem was discussed nearly *to death*
> before its implementation. I sat less than 10 feet away from the two
> guys who drove the formulation and implementation of both the public
> authz subsystem and CollabNet's private manifestation thereof. I have
> the T-shirt. And the scars. :-) This ground was covered.
I stand corrected.
> As you might recall, though, there was really no attempt to model
> in-filesystem ACLs, so you'll find all kinds of disparity with such a
> system when you compare it to ours. Create a directory but not its
> children? Obviously silly in an ACL system. Less so in a system
> where paths themselves are considered a part of the information whose
> access is controlled. But, seriously, what could be more consistent
> than the policy Subversion has right now? "If I don't have write
> access to a path, I can't modify (add, delete, change) anything at
> that path" seems about as straightforward as is semantically possible.
>
> But I need to back up a bit and confess that I failed to process a bit
> of the original mail, namely the bit about how the authz code naively
> disallows a recursive write if there are any rules that would block
> writes to some path deep in that tree *without even seeing if there
> actually is/would-be such a path*. I mean, that's the claim, right?
Yup. That's the case, for both the source (recursive read for copy,
recursive write for move) and the target (recursive write) of the
copy/move operation.
> If so, that's ... weird. And yeah, wildly inconsistent with ...
> really any sane reasoning. CollabNet's authz module did not behave
> this way -- we accepted the recursive tree walk as a necessary evil.
> Though, we did at least try to optimize it a bit: you can check read
> access at the source and write access at the target of a copy with the
> same walk, using a bit of path-math; and you can of course check
> multiple permissions -- read, add, modify, delete, copy -- as needed
> at the same time, too.
Ack. Note that on the authzperf branch -- which is, in some ways,
equivalent to your authz-improvements branch but with (initially) a
different set of priorities -- we still don't do tree walks, but the
authz parser and internal representation do predict, propagate and
separate the individual access rights. Not that we have more than two
there yet.
>>> I lean towards thinking
>>> that falls outside the scope of acceptable changes in behavior in the
>>> 1.x line *unless* you can find a way to, via configuration, allow
>>> administrators to explicitly toggle this new paradigm.
>> Assuming we agree that it's not a new paradigm, would proper
>> documentation, as I proposed, be sufficient?
>
> I was going to suggest that perhaps you tie the change in behavior to
> the use of wildcard rules, but as was explained in the bit I failed to
> process originally, this problem predates the new syntax. It still
> reads to me as if you're trying to fix a relatively straightforward
> problem (we do recursive path checks without actually considering the
> actual paths) in a way that runs counter to the original authz design
> principles (and by that, I mean authz_policy.txt, not the much-younger
> libsvn_repos/authz.c) just to avoid the more-expensive-yet-correct fix
> predicted in that very file:
>
> 3. Manually implement authz when receiving network requests that
> represent calls to a commit editor:
>
> - do write checks for most editor operations
> - do read *and* write checks for copy operations.
>
> (Note that doing full-out authz on whole trees fundamentally
> contradicts Subversion's O(1) copy philosophy; in practice,
> however, specific authz implementations are able to get the same
> effect while being less expensive than O(N).)
>
> In other words: "To fully implement the write side of this policy,
> you have to be recursive -- but maybe you'll find a way to avoid a
> tree walk. Good luck!"
There are ways to prevent a whole-tree walk, but these dwindle amazingly
quickly as soon as you have any kind of pattern matching in the rules.
> Now, I'm a bit disconnected from the project these days, so I accept a
> reduction in the weight of my vote.
Eh? There's no reduction, unless you want to exercise it yourself. :)
> I'm confident that the world will continue to spin regardless of
> what you guys decide. But just to be clear, you are proposing to
> silently remove existing access control protections that have been in
> place for over a decade, and introduce a change of behavior that
> contradicts the written authz_policy.txt document, and then cover for
> it with just a bit of documentation that sits on a website far removed
> from nearly everyone's package distribution system. :-)
I'm very acutely aware of this, yes ... in this case, the "we told you
so!" argument isn't very solid.
> SIDEBAR: I didn't see any changes in the patch to mod_authz_svn's use
> of recursive checks in these same ways. If you move forward with this
> change, don't forget to update that code as necessary, too.
Other than not checking for recursive write on the target of a copy? No;
if mod_authz_svn, by itself or with the "help" of mod_dav, do the
recursive walk, nothing would change.
-- Brane
Re: Recursive operations and authz
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 05/13/2015 10:35 AM, Branko Čibej wrote:
On 13 May 2015 at 15:24, C. Michael Pilato <cm...@collab.net>
<cm...@collab.net> wrote:
Well, the use-case being broken here is kinda the obvious one: I
shouldn't have permission to create/delete some path /foo/bar (it's a
system-critical file that shouldn't go away, or a password-bearing file
path blocked by the VC system because devs keep stupidly committing
them, or...), but you've provided and end-around for me to do so. :-)
In the copy and move scenarios, you're allowing me to create a path I
can't later delete or further modify. Now, while this is a very handy
semantic for tag creation[1], it's also something an administrator
should be able to make a conscious decision about, not just an
unfortunate side-effect of an effort to make the code faster.
This proposal really has nothing to do with performance; it's about
intended/consistent authz semantics, which we never really bothered to
define, let alone prove. IMO, the fact that you can have authz rules
that allow you to create a directory but not its children is
unintended. I bet no-one thought about it during the initial
development of mod_authz_svn.
Dude. The Subversion authz subsystem was discussed nearly *to death*
before its implementation. I sat less than 10 feet away from the two guys
who drove the formulation and implementation of both the public authz
subsystem and CollabNet's private manifestation thereof. I have the
T-shirt. And the scars. :-) This ground was covered.
As you might recall, though, there was really no attempt to model
in-filesystem ACLs, so you'll find all kinds of disparity with such a
system when you compare it to ours. Create a directory but not its
children? Obviously silly in an ACL system. Less so in a system where
paths themselves are considered a part of the information whose access is
controlled. But, seriously, what could be more consistent than the policy
Subversion has right now? "If I don't have write access to a path, I can't
modify (add, delete, change) anything at that path" seems about as
straightforward as is semantically possible.
But I need to back up a bit and confess that I failed to process a bit of
the original mail, namely the bit about how the authz code naively
disallows a recursive write if there are any rules that would block writes
to some path deep in that tree *without even seeing if there actually
is/would-be such a path*. I mean, that's the claim, right? If so, that's
... weird. And yeah, wildly inconsistent with ... really any sane
reasoning. CollabNet's authz module did not behave this way -- we accepted
the recursive tree walk as a necessary evil. Though, we did at least try
to optimize it a bit: you can check read access at the source and write
access at the target of a copy with the same walk, using a bit of
path-math; and you can of course check multiple permissions -- read, add,
modify, delete, copy -- as needed at the same time, too.
I lean towards thinking
that falls outside the scope of acceptable changes in behavior in the
1.x line *unless* you can find a way to, via configuration, allow
administrators to explicitly toggle this new paradigm.
Assuming we agree that it's not a new paradigm, would proper
documentation, as I proposed, be sufficient?
I was going to suggest that perhaps you tie the change in behavior to the
use of wildcard rules, but as was explained in the bit I failed to process
originally, this problem predates the new syntax. It still reads to me as
if you're trying to fix a relatively straightforward problem (we do
recursive path checks without actually considering the actual paths) in a
way that runs counter to the original authz design principles (and by that,
I mean authz_policy.txt, not the much-younger libsvn_repos/authz.c) just to
avoid the more-expensive-yet-correct fix predicted in that very file:
3. Manually implement authz when receiving network requests that
represent calls to a commit editor:
- do write checks for most editor operations
- do read *and* write checks for copy operations.
(Note that doing full-out authz on whole trees fundamentally
contradicts Subversion's O(1) copy philosophy; in practice,
however, specific authz implementations are able to get the same
effect while being less expensive than O(N).)
In other words: "To fully implement the write side of this policy, you
have to be recursive -- but maybe you'll find a way to avoid a tree walk.
Good luck!"
Now, I'm a bit disconnected from the project these days, so I accept a
reduction in the weight of my vote. I'm confident that the world will
continue to spin regardless of what you guys decide. But just to be clear,
you are proposing to silently remove existing access control protections
that have been in place for over a decade, and introduce a change of
behavior that contradicts the written authz_policy.txt document, and then
cover for it with just a bit of documentation that sits on a website far
removed from nearly everyone's package distribution system. :-)
SIDEBAR: I didn't see any changes in the patch to mod_authz_svn's use of
recursive checks in these same ways. If you move forward with this change,
don't forget to update that code as necessary, too.
--
C. Michael Pilato <cm...@collab.net> <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: Recursive operations and authz
Posted by Branko Čibej <br...@wandisco.com>.
On 13 May 2015 at 15:24, C. Michael Pilato <cm...@collab.net> wrote:
> On 05/13/2015 02:21 AM, Stefan Fuhrmann wrote:
>> Hi devs,
>>
>> At WANdisco, people have been playing with the new
>> wildcard support for authz (see authz-performance branch)
>> and ran into an interesting problem.
>
> [Details snipped. Welcome to 2004, where CollabNet ran into the same
> issues with the proprietary regexp-based authz implementation we shipped
> in our enterprise products.]
>
>> My suggestion is to relax the requirements as follows:
>> COPY & MOVE still require recursive read access on
>> the source but only non-recursive write access on the
>> destination. Thus it becomes possible to protect data
>> in branches and tags right from their inception without
>> relaxing read access requirements to existing data. The
>> attached patch achieves just that.
>>
>> I have 3 questions:
>>
>> (1) Is there something fundamentally wrong with this
>> approach, e.g. braking major use-cases?
>> (2) Should we require write access to target and target's
>> parent folder (as done by the patch) or just to the target's
>> parent folder?
>> (3) Should we (optionally?) reduce the access rights reqs
>> for DELETE similarly such that no recursive write access
>> is needed? That seems risky but would be symmetrical
>> to creating copies with r/o or n/a sub-paths. MOVE would
>> be updated accordingly (always the same reqs as a COPY
>> + DELETE).
>
> Well, the use-case being broken here is kinda the obvious one: I
> shouldn't have permission to create/delete some path /foo/bar (it's a
> system-critical file that shouldn't go away, or a password-bearing file
> path blocked by the VC system because devs keep stupidly committing
> them, or...), but you've provided and end-around for me to do so. :-)
> In the copy and move scenarios, you're allowing me to create a path I
> can't later delete or further modify. Now, while this is a very handy
> semantic for tag creation[1], it's also something an administrator
> should be able to make a conscious decision about, not just an
> unfortunate side-effect of an effort to make the code faster.
This proposal really has nothing to do with performance; it's about
intended/consistent authz semantics, which we never really bothered to
define, let alone prove. IMO, the fact that you can have authz rules
that allow you to create a directory but not its children is
unintended. I bet no-one thought about it during the initial
development of mod_authz_svn.
> Subversion's path-based authz subsystem is about paths, not nodes as we
> devs think of them. Our users have generally not been asked to consider
> a copy, addition, or deletion of an item to be changes made to the
> parent of that item. So the changes you're suggesting are not merely
> ones of functionality, but ones of philosophy.
Interesting point, but I disagree: we're still talking about authz in
terms of paths, the proposed change just removes the restriction that
you cannot copy something to a location where you can create
something. Currently create requires non-recursive write access to the
target, but copy (and move) require recursive write access. IMO,
that's inconsistent and unintuitive, something I'd classify as a
design bug rather than a change in philosophy.
> I lean towards thinking
> that falls outside the scope of acceptable changes in behavior in the
> 1.x line *unless* you can find a way to, via configuration, allow
> administrators to explicitly toggle this new paradigm.
Assuming we agree that it's not a new paradigm, would proper
documentation, as I proposed, be sufficient?
> [1] Along with regexp access rules, CollabNet has for many years
> supported this tag creation use-case by splitting the "write" permission
> into multiple ones: "add", "copy", "delete", and "modify". Something
> Subversion should consider?
That's what the authzperf branch is for, indeed. Hopefully many of
these things will land in 1.10.
-- Brane
Re: Recursive operations and authz
Posted by Branko Čibej <br...@wandisco.com>.
On 18.05.2015 15:08, C. Michael Pilato wrote:
> On 05/14/2015 08:12 AM, Branko Čibej wrote:
>> On 13.05.2015 15:24, C. Michael Pilato wrote:
>>> I lean towards thinking that falls outside the scope of acceptable
>>> changes in behavior in the 1.x line *unless* you can find a way to,
>>> via configuration, allow administrators to explicitly toggle this new
>>> paradigm.
>> I've been thinking about a reasonable way to do this. Config files are
>> out, IMO, because this should really be per-authz-file behaviour, not
>> per-server or per-repository.
>>
>> On the authzperf branch, we came up with an extended,
>> backward-compatible authz rule syntax that allows us to add different
>> options to authz rules without causing clashes with existing syntax
>> (see: "New Rule Syntax for Wildcard Rules" in
>> http://wiki.apache.org/subversion/AuthzImprovements).
>>
>> With that in mind, we could teach the trunk/1.9 authz parser to
>> recognize an optional "[:config:]" section, in which we could add
>> options that control the behaviour of the rules in the authz file; a
>> 'copy-target-requires-recursive-access' option could then control the
>> behaviour we've been discussing; whit the default, of course, being
>> 'true' for backwards compatibility.
> Heh, this is precisely what I meant with "via configuration". I just
> thought we'd already found a way to do in-authz-file configuration.
> Well, glad you guys were able to find a way to make that daydream a
> reality! :-)
Yes, well, it'll be a reality iff we radically change the authz callback
API in libsvn_repos; otherwise, no deal ... the callers of that API have
no access to the parsed authz structure, and the callback doesn't know
which operation we're checking authz for.
I think the solution is to stop asking what kind of access the user has
for a certain path, but instead to ask if the user can use the path for
the given operation (create, modify, delete, copy source, move source,
copy target, move target, etc.). This would let us nicely encapsulate
the authz "philosophy". Obviously this is not 1.9 material.
-- Brane
Re: Recursive operations and authz
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 05/14/2015 08:12 AM, Branko Čibej wrote:
> On 13.05.2015 15:24, C. Michael Pilato wrote:
>> I lean towards thinking that falls outside the scope of acceptable
>> changes in behavior in the 1.x line *unless* you can find a way to,
>> via configuration, allow administrators to explicitly toggle this new
>> paradigm.
> I've been thinking about a reasonable way to do this. Config files are
> out, IMO, because this should really be per-authz-file behaviour, not
> per-server or per-repository.
>
> On the authzperf branch, we came up with an extended,
> backward-compatible authz rule syntax that allows us to add different
> options to authz rules without causing clashes with existing syntax
> (see: "New Rule Syntax for Wildcard Rules" in
> http://wiki.apache.org/subversion/AuthzImprovements).
>
> With that in mind, we could teach the trunk/1.9 authz parser to
> recognize an optional "[:config:]" section, in which we could add
> options that control the behaviour of the rules in the authz file; a
> 'copy-target-requires-recursive-access' option could then control the
> behaviour we've been discussing; whit the default, of course, being
> 'true' for backwards compatibility.
Heh, this is precisely what I meant with "via configuration". I just
thought we'd already found a way to do in-authz-file configuration.
Well, glad you guys were able to find a way to make that daydream a
reality! :-)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: Recursive operations and authz
Posted by Branko Čibej <br...@wandisco.com>.
On 14.05.2015 14:12, Branko Čibej wrote:
> With that in mind, we could teach the trunk/1.9 authz parser to
> recognize an optional "[:config:]" section, in which we could add
> options that control the behaviour of the rules in the authz file; a
> 'copy-target-requires-recursive-access' option could then control the
> behaviour we've been discussing; whit the default, of course, being
> 'true' for backwards compatibility.
Well, it turns out that something like this could work in mod_authz_svn,
but not in libsvn_repos, becaus the authz callback in the editor doesn't
know what kind of operation we're doing the authz check for, and the
code that does know about the operation can't get at the options stored
in the parsed authz structure. Without drastically changing the authz
callback API. :(
-- Brane
Re: Recursive operations and authz
Posted by Branko Čibej <br...@wandisco.com>.
On 13.05.2015 15:24, C. Michael Pilato wrote:
> I lean towards thinking that falls outside the scope of acceptable
> changes in behavior in the 1.x line *unless* you can find a way to,
> via configuration, allow administrators to explicitly toggle this new
> paradigm.
I've been thinking about a reasonable way to do this. Config files are
out, IMO, because this should really be per-authz-file behaviour, not
per-server or per-repository.
On the authzperf branch, we came up with an extended,
backward-compatible authz rule syntax that allows us to add different
options to authz rules without causing clashes with existing syntax
(see: "New Rule Syntax for Wildcard Rules" in
http://wiki.apache.org/subversion/AuthzImprovements).
With that in mind, we could teach the trunk/1.9 authz parser to
recognize an optional "[:config:]" section, in which we could add
options that control the behaviour of the rules in the authz file; a
'copy-target-requires-recursive-access' option could then control the
behaviour we've been discussing; whit the default, of course, being
'true' for backwards compatibility.
-- Brane
Re: Recursive operations and authz
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 05/13/2015 02:21 AM, Stefan Fuhrmann wrote:
> Hi devs,
>
> At WANdisco, people have been playing with the new
> wildcard support for authz (see authz-performance branch)
> and ran into an interesting problem.
[Details snipped. Welcome to 2004, where CollabNet ran into the same
issues with the proprietary regexp-based authz implementation we shipped
in our enterprise products.]
> My suggestion is to relax the requirements as follows:
> COPY & MOVE still require recursive read access on
> the source but only non-recursive write access on the
> destination. Thus it becomes possible to protect data
> in branches and tags right from their inception without
> relaxing read access requirements to existing data. The
> attached patch achieves just that.
>
> I have 3 questions:
>
> (1) Is there something fundamentally wrong with this
> approach, e.g. braking major use-cases?
> (2) Should we require write access to target and target's
> parent folder (as done by the patch) or just to the target's
> parent folder?
> (3) Should we (optionally?) reduce the access rights reqs
> for DELETE similarly such that no recursive write access
> is needed? That seems risky but would be symmetrical
> to creating copies with r/o or n/a sub-paths. MOVE would
> be updated accordingly (always the same reqs as a COPY
> + DELETE).
Well, the use-case being broken here is kinda the obvious one: I
shouldn't have permission to create/delete some path /foo/bar (it's a
system-critical file that shouldn't go away, or a password-bearing file
path blocked by the VC system because devs keep stupidly committing
them, or...), but you've provided and end-around for me to do so. :-)
In the copy and move scenarios, you're allowing me to create a path I
can't later delete or further modify. Now, while this is a very handy
semantic for tag creation[1], it's also something an administrator
should be able to make a conscious decision about, not just an
unfortunate side-effect of an effort to make the code faster.
Subversion's path-based authz subsystem is about paths, not nodes as we
devs think of them. Our users have generally not been asked to consider
a copy, addition, or deletion of an item to be changes made to the
parent of that item. So the changes you're suggesting are not merely
ones of functionality, but ones of philosophy. I lean towards thinking
that falls outside the scope of acceptable changes in behavior in the
1.x line *unless* you can find a way to, via configuration, allow
administrators to explicitly toggle this new paradigm.
-- Mike
[1] Along with regexp access rules, CollabNet has for many years
supported this tag creation use-case by splitting the "write" permission
into multiple ones: "add", "copy", "delete", and "modify". Something
Subversion should consider?