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...@apache.org> on 2018/09/08 09:17:18 UTC
Re: authz changes between 1.9 and 1.10
Most of these issues have already been addressed by Brane,
but as he wished for my input, here it is.
These are the guiding principles for the 1.10 authz design:
(1) ACLs are only evaluated on a per-user bases; ACLs that
don't mention this user (or any of their groups) are ignored.
Rationale: We don't want to explicitly repeat inherited access
specs that don't change for the respective path / section.
(2) A more specific rule (fully) replaces any more general rule.
Rationale: You want to specify access exactly where it applies
and be sure that those are exactly the rights that will be applied.
(3) If there are multiple, equally specific rules, the last one
replaces any previous one.
Rationale: The last one, so you can specify catch-all denial ACLs.
Replacement, again, to ensure that exactly the rights specified
last will be in effect.
Those principles seemed quite reasonable and, most importantly,
workable with all the potential confusion caused by glob-rules.
Given that the previous authz code lacked similarly explicit
guidelines, I felt that any discrepancy between 1.9 and 1.10
would be a strong indication of a undesirable behavior in 1.9.
I still think this is true but we must neither break 1.9 authz.
On 20.07.2018 14:59, Philip Martin wrote:
> In 1.9 it was possible to repeat, or reopen, a section:
>
> [/some/path]
> user = r
> [/some/path]
> otheruser = rw
> This was equivalent to a single section:
>
> [/some/path]
> user = r
> otheruser = rw
>
> In 1.10 this is rejected by the parser and cannot be used. Is this a
> bug in 1.10 or an acceptable behaviour change?
Collides with rule (3), which is essential for reasoning on glob-rules.
Since it seemed an accidental feature inherited from the config parser,
we explicitly flagged it as invalid in 1.10. It would otherwise become
[/some/path]
otheruser = rw
> In 1.9 any repeat acl lines that were the exact same match, such as:
>
> [/some/path]
> user = rw
> user = r
>
> resulted in the last line overriding all the other lines, so user=r in
> the example above. In 1.10 the lines combine, so user=rw in the example
> above. Is this a bug in 1.10, or a bug in 1.9 that is fixed in 1.10, or
> an acceptable behaviour change?
Ouch. That is a bad one and an oversight in the design - I think.
According to (3), 1.9 behaves correctly. I guess we consider it
an unordered collection in 1.10 and then a union is the only thing
that approximates intent when a user is a member of different
groups with different access rights. Strict ordering becomes
very useful when assigning rights to groups:
[/some/path]
@Users = rw
@BadUsers = r
I guess the fix in 1.10 is simple but will change 1.10 behavior.
My proposal here:
* apply replacement semantics here as in 1.9
* error out / warn on repeated lines with the same user or group
(this is hardly ever intentional)
* provide a script / tool that takes 1.10 authz and checks for
changed behavior ("r" after "rw" rules etc.)
The last one is a bit of work but would be really handy.
> Finally, issue 4762. In 1.9 if both global and per-repository sections
> matched they were combined, so:
>
> [/some/path]
> user = rw
> [repos:/some/path]
> user = r
>
> resulted in user=rw. The issue 4762 problem is that in 1.10 the
> per-repository section overrides any global section, so user=r above. I
> believe this is a 1.10 bug and that the 1.9 behaviour should be
> reinstated.
According to (2), 1.10 behaves correctly: "user" has rw access,
except for a specific repository. I was not aware that 1.9 has
different behavior.
Now, the crux is how to unbreak 1.9. I suggest the following.
* Introduce a resolution option in the authz code:
- "union" (1.9 behavior)
- "most specific" (1.10 behavior)
- "error out" (new, will work in all non-ambiguous cases)
* default to the "error out"
* optionally specify the desired behavior as a config option
> Glob sections are new so they could have different behaviour from
> non-glob sections, but is that what we want? There is a wiki page
> https://cwiki.apache.org/confluence/display/SVN/AuthzImprovements but
> given issue 4762 I not sure whether it describes the correct behaviour.
>
User documentation on the new authz is inadequate.
-- Stefan^2.
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 02.12.2018 10:38, Branko Čibej wrote:
> On 02.12.2018 08:43, Branko Čibej wrote:
>> On 02.12.2018 08:25, Branko Čibej wrote:
>>> On 08.09.2018 11:17, Stefan Fuhrmann wrote:
>>>> These are the guiding principles for the 1.10 authz design:
>>>>
>>>> (1) ACLs are only evaluated on a per-user bases; ACLs that
>>>> don't mention this user (or any of their groups) are ignored.
>>>> Rationale: We don't want to explicitly repeat inherited access
>>>> specs that don't change for the respective path / section.
>>> This is not entirely true, as seen in the fix for SVN-4793. If a user is
>>> "not mentioned" in an inverted selector, those rights do propagate to
>>> the global level. For example:
>>>
>>> [groups]
>>> readers = foo, bar
>>>
>>> [/]
>>> ~@readers = rw
>>> @readers = r
>>>
>>>
>>> In this case 'user' has read-write access to '[/]' even though she's not
>>> mentioned anywhere in the authz file or the specific ACL for '[/]'.
>>>
>>>
>>>>> In 1.9 any repeat acl lines that were the exact same match, such as:
>>>>>
>>>>> [/some/path]
>>>>> user = rw
>>>>> user = r
>>>>>
>>>>> resulted in the last line overriding all the other lines, so user=r in
>>>>> the example above. In 1.10 the lines combine, so user=rw in the example
>>>>> above. Is this a bug in 1.10, or a bug in 1.9 that is fixed in 1.10, or
>>>>> an acceptable behaviour change?
>>>> Ouch. That is a bad one and an oversight in the design - I think.
>>>>
>>>> According to (3), 1.9 behaves correctly. I guess we consider it
>>>> an unordered collection in 1.10 and then a union is the only thing
>>>> that approximates intent when a user is a member of different
>>>> groups with different access rights. Strict ordering becomes
>>>> very useful when assigning rights to groups:
>>>>
>>>> [/some/path]
>>>> @Users = rw
>>>> @BadUsers = r
>>> We already have strict ordering within an ACL (authz_acl_t in
>>> libsvn_repos/authz.h):
>>>
>>> /* All other user- or group-specific access rights.
>>> Aliases are replaced with their definitions, rules for the same
>>> user or group are merged. */
>>> apr_array_header_t *user_access;
>>>
>>>
>>>
>>> The "merge" semantics was intentional; if we decide it's a bug (and I
>>> think it is), it's fairly easy to change. I would lean in the direction
>>> of making repeating the same access entry selection a hard error at
>>> parsing time. This requires changes to the merge semantics implemented
>>> in add_access_entry() and merge_alias_ace() in
>>> libsvn_repos/authz_parse.c. The nice part is that it would catch errors
>>> like this:
>>>
>>> [aliases]
>>> afoo = foo
>>> abar = bar
>>>
>>> [/]
>>> &afoo = rw
>>> foo = r
>>> ~&abar = rw
>>> ~bar = r
>>>
>>>
>>> With the current implementation we translate the ACL to:
>>>
>>> [/]
>>> foo = rw
>>> foo = r
>>> ~bar = rw
>>> ~bar = r
>>>
>>>
>>> and even with strict ordering I'd say this is a bug and not intentional.
>> Note that this should also be an error:
>>
>> [/]
>> $anonymous = r
>> ~$authenticated = rw
> I have a patch ready, here are some examples of what it does (currently,
> all these examples are valid and produce merged access rights):
https://issues.apache.org/jira/browse/SVN-4794
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 05.12.2018 14:27, Julian Foad wrote:
> Branko Čibej wrote:
>>> https://cwiki.apache.org/confluence/x/7IjQBQ
>> It is starting to improve. I'll be grateful to anyone who cares to take
>> a look to debug the syntax BNF.
> I appreciate the right-bracket rule change is in that domain.
>
> Could I nevertheless encourage you to jump ahead to writing the high-level rules, especially those that affected by the changes you propose in this thread relating to override/merge behaviours etc.?
>
> That's the part we need in order to review the proposed changes.
One thing at a time. FWIW, the high-level rules are *already* documented
in our wiki, as are changes from the previous authz incarnation (modulo
bugs). I'll really just summarize them in a more user-friendly way. All
known docs and issues are also linked from that page.
In fact the documentation we have now, as well as test coverage, is much
better than what we had before 1.10, when it was limited to half a
chapter in The Book and to reading the code. It's kind of "fun" trying
to document changes from undocumented behaviour.
-- Brane
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 14.12.2018 22:55, Doug Robinson wrote:
> Brane:
>
> I've decided that documenting the syntax of authz files at this level
>> doesn't really belong in this document. So I started this:
>>
>> https://cwiki.apache.org/confluence/x/oYvQBQ
>>
>> and will refer to that page instead, pointing out the differences.
>>
> The statement "Section names are case-insensitive and case-preserving." is
> deeply
> concerning as it means that the case sensitive sub-repository paths cannot
> be
> properly represented in sections (as they are today).
This is the document for configuration file syntax, not authz file
semantics. The respective parsers behave differently.
> As noted, in https://cwiki.apache.org/confluence/x/7IjQBQ , the SVN section
> headers
> became case sensitive at SVN 1.7.
>
> Not sure why 2 documents?
Becuase we also have configuration files and their syntax was never
formally defined. Now it is.
The syntax of authz files is similar — not identical — but close enoug
that we can refer from the authz document to the config file document,
noting only the differences. It does save time and space not having to
explain twice how, for example, value continuation lines work.
-- Brane
Re: authz changes between 1.9 and 1.10
Posted by Doug Robinson <do...@wandisco.com>.
Brane:
I've decided that documenting the syntax of authz files at this level
> doesn't really belong in this document. So I started this:
>
> https://cwiki.apache.org/confluence/x/oYvQBQ
>
> and will refer to that page instead, pointing out the differences.
>
The statement "Section names are case-insensitive and case-preserving." is
deeply
concerning as it means that the case sensitive sub-repository paths cannot
be
properly represented in sections (as they are today).
As noted, in https://cwiki.apache.org/confluence/x/7IjQBQ , the SVN section
headers
became case sensitive at SVN 1.7.
Not sure why 2 documents?
Cheers.
Doug
--
*DOUGLAS B ROBINSON* SENIOR PRODUCT MANAGER
T +1 925 396 1125
*E* doug.robinson@wandisco.com
--
* <http://wandisco.com>*
**The LIVE DATA Company
*Find out more
*wandisco.com <http://wandisco.com/>*
<https://www.wandisco.com/welcome-live-data-world-video>
*
THIS MESSAGE
AND ANY ATTACHMENTS ARE CONFIDENTIAL, PROPRIETARY AND MAY BE PRIVILEGED
If
this message was misdirected, WANdisco, Inc. and its subsidiaries,
("WANdisco") does not waive any confidentiality or privilege. If you are
not the intended recipient, please notify us immediately and destroy the
message without disclosing its contents to anyone. Any distribution, use or
copying of this email or the information it contains by other than an
intended recipient is unauthorized. The views and opinions expressed in
this email message are the author's own and may not reflect the views and
opinions of WANdisco, unless the author is authorized by WANdisco to
express such views or opinions on its behalf. All email sent to or from
this address is subject to electronic storage and review by WANdisco.
Although WANdisco operates anti-virus programs, it does not accept
responsibility for any damage whatsoever caused by viruses being passed.
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 05.12.2018 15:23, Branko Čibej wrote:
> On 05.12.2018 15:05, Julian Foad wrote:
>> Branko Čibej wrote:
>>> On 05.12.2018 14:44, Julian Foad wrote:
>>>> Branko Čibej wrote:
>>>>> Yes, but I'm asking for a review of the BNF to make sure that it doesn't
>>>>> contain silly bugs.
>>>> At first glance, I saw what looks like a bug: it says a comment must have a non-white-space immediately after the '#'... I didn't review further.
>>> The <text> production used imply "optional". I already fixed that and
>>> how the comment definition reads:
>>>
>>> <comment> ::= "#" <opt-text> <line-end>
>> The problem is <opt-text> isn't allowed to start with whitespace.
> Eh, you're right; I redefined <opt-text> to fix that, which also fixes
> <key-value>.
I've decided that documenting the syntax of authz files at this level
doesn't really belong in this document. So I started this:
https://cwiki.apache.org/confluence/x/oYvQBQ
and will refer to that page instead, pointing out the differences.
-- Brane
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 05.12.2018 15:05, Julian Foad wrote:
> Branko Čibej wrote:
>> On 05.12.2018 14:44, Julian Foad wrote:
>>> Branko Čibej wrote:
>>>> Yes, but I'm asking for a review of the BNF to make sure that it doesn't
>>>> contain silly bugs.
>>> At first glance, I saw what looks like a bug: it says a comment must have a non-white-space immediately after the '#'... I didn't review further.
>> The <text> production used imply "optional". I already fixed that and
>> how the comment definition reads:
>>
>> <comment> ::= "#" <opt-text> <line-end>
> The problem is <opt-text> isn't allowed to start with whitespace.
Eh, you're right; I redefined <opt-text> to fix that, which also fixes
<key-value>.
-- Brane
Re: authz changes between 1.9 and 1.10
Posted by Julian Foad <ju...@apache.org>.
Branko Čibej wrote:
> On 05.12.2018 14:44, Julian Foad wrote:
> > Branko Čibej wrote:
> >> Yes, but I'm asking for a review of the BNF to make sure that it doesn't
> >> contain silly bugs.
> > At first glance, I saw what looks like a bug: it says a comment must have a non-white-space immediately after the '#'... I didn't review further.
>
> The <text> production used imply "optional". I already fixed that and
> how the comment definition reads:
>
> <comment> ::= "#" <opt-text> <line-end>
The problem is <opt-text> isn't allowed to start with whitespace.
--
- Julian
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 05.12.2018 14:59, Branko Čibej wrote:
> On 05.12.2018 14:44, Julian Foad wrote:
>> Branko Čibej wrote:
>>> Yes, but I'm asking for a review of the BNF to make sure that it doesn't
>>> contain silly bugs.
>> At first glance, I saw what looks like a bug: it says a comment must have a non-white-space immediately after the '#'... I didn't review further.
> The <text> production used imply "optional". I already fixed that and
> how the comment definition reads:
>
> <comment> ::= "#" <opt-text> <line-end>
However <key> /is/ wrong ... it implies that keys can't contain "#" or
"[", where actually they can, they just can't start with one of those
two. Whet they really can't contain is "=".
I will have to check the code though ...
-- Brane
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 05.12.2018 14:44, Julian Foad wrote:
> Branko Čibej wrote:
>> Yes, but I'm asking for a review of the BNF to make sure that it doesn't
>> contain silly bugs.
> At first glance, I saw what looks like a bug: it says a comment must have a non-white-space immediately after the '#'... I didn't review further.
The <text> production used imply "optional". I already fixed that and
how the comment definition reads:
<comment> ::= "#" <opt-text> <line-end>
-- Brane
Re: authz changes between 1.9 and 1.10
Posted by Julian Foad <ju...@apache.org>.
Branko Čibej wrote:
> Yes, but I'm asking for a review of the BNF to make sure that it doesn't
> contain silly bugs.
At first glance, I saw what looks like a bug: it says a comment must have a non-white-space immediately after the '#'... I didn't review further.
--
- Julian
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 05.12.2018 14:27, Julian Foad wrote:
> Branko Čibej wrote:
>>> https://cwiki.apache.org/confluence/x/7IjQBQ
>> It is starting to improve. I'll be grateful to anyone who cares to take
>> a look to debug the syntax BNF.
> I appreciate the right-bracket rule change is in that domain.
>
> Could I nevertheless encourage you to jump ahead to writing the high-level rules, especially those that affected by the changes you propose in this thread relating to override/merge behaviours etc.?
>
> That's the part we need in order to review the proposed changes.
Yes, but I'm asking for a review of the BNF to make sure that it doesn't
contain silly bugs.
-- Brane
Re: authz changes between 1.9 and 1.10
Posted by Julian Foad <ju...@apache.org>.
Branko Čibej wrote:
>> https://cwiki.apache.org/confluence/x/7IjQBQ
>
> It is starting to improve. I'll be grateful to anyone who cares to take
> a look to debug the syntax BNF.
I appreciate the right-bracket rule change is in that domain.
Could I nevertheless encourage you to jump ahead to writing the high-level rules, especially those that affected by the changes you propose in this thread relating to override/merge behaviours etc.?
That's the part we need in order to review the proposed changes.
--
- Julian
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 02.12.2018 17:28, Branko Čibej wrote:
> On 02.12.2018 16:49, Branko Čibej wrote:
> Seriously though: I started this document
> https://cwiki.apache.org/confluence/x/7IjQBQ
> Yes, it's empty. It will improve.
It is starting to improve. I'll be grateful to anyone who cares to take
a look to debug the syntax BNF.
-- Brane
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 02.12.2018 16:49, Branko Čibej wrote:
> On 02.12.2018 15:15, Julian Foad wrote:
>> Branko Čibej wrote:
>>> I have a patch ready, here are some examples of what it does [...]
>> Not following the details, all I would ask ("all", he says) is that a complete description of the new semantics be available for review, and should be published along with the changes. Otherwise I fear the community has no real way of reviewing the actual semantics against the intended semantics.
>>
>> One basis we have for such documentation could be the document that Doug Robinson contributed in dev@ thread "Subversion AuthZ Wildcards" on 2017-02-27 [1]. AFAIK it was the best or only documentation we had of the new semantics.
>>
>> Might you or anyone else be prepared to do that?
>>
>> [1] https://svn.haxx.se/dev/archive-2017-02/0188.shtml or https://lists.apache.org/thread.html/be50f6e5b1a92a244033bd7f13449c8d02e92bbe7e29dc89209f62f8@%3Cdev.subversion.apache.org%3E
>>
>
> Oooh, Yeah. I started by making the page tree in confluence make some
> kind of sense, and found we have two roadmap pages, both by the same
> author. :) There are a few pages I haven't been able to classify, but
> certainly a new chapter is necessary to gather actual implemented
> feature documentation (which may, or may not, be different from the
> related design notes).
Seriously though: I started this document
https://cwiki.apache.org/confluence/x/7IjQBQ
Yes, it's empty. It will improve.
-- Brane
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 02.12.2018 15:15, Julian Foad wrote:
> Branko Čibej wrote:
>> I have a patch ready, here are some examples of what it does [...]
> Not following the details, all I would ask ("all", he says) is that a complete description of the new semantics be available for review, and should be published along with the changes. Otherwise I fear the community has no real way of reviewing the actual semantics against the intended semantics.
>
> One basis we have for such documentation could be the document that Doug Robinson contributed in dev@ thread "Subversion AuthZ Wildcards" on 2017-02-27 [1]. AFAIK it was the best or only documentation we had of the new semantics.
>
> Might you or anyone else be prepared to do that?
>
> [1] https://svn.haxx.se/dev/archive-2017-02/0188.shtml or https://lists.apache.org/thread.html/be50f6e5b1a92a244033bd7f13449c8d02e92bbe7e29dc89209f62f8@%3Cdev.subversion.apache.org%3E
>
Oooh, Yeah. I started by making the page tree in confluence make some
kind of sense, and found we have two roadmap pages, both by the same
author. :) There are a few pages I haven't been able to classify, but
certainly a new chapter is necessary to gather actual implemented
feature documentation (which may, or may not, be different from the
related design notes).
-- Brane
Re: authz changes between 1.9 and 1.10
Posted by Julian Foad <ju...@apache.org>.
Branko Čibej wrote:
> I have a patch ready, here are some examples of what it does [...]
Not following the details, all I would ask ("all", he says) is that a complete description of the new semantics be available for review, and should be published along with the changes. Otherwise I fear the community has no real way of reviewing the actual semantics against the intended semantics.
One basis we have for such documentation could be the document that Doug Robinson contributed in dev@ thread "Subversion AuthZ Wildcards" on 2017-02-27 [1]. AFAIK it was the best or only documentation we had of the new semantics.
Might you or anyone else be prepared to do that?
[1] https://svn.haxx.se/dev/archive-2017-02/0188.shtml or https://lists.apache.org/thread.html/be50f6e5b1a92a244033bd7f13449c8d02e92bbe7e29dc89209f62f8@%3Cdev.subversion.apache.org%3E
--
- Julian
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 02.12.2018 08:43, Branko Čibej wrote:
> On 02.12.2018 08:25, Branko Čibej wrote:
>> On 08.09.2018 11:17, Stefan Fuhrmann wrote:
>>> These are the guiding principles for the 1.10 authz design:
>>>
>>> (1) ACLs are only evaluated on a per-user bases; ACLs that
>>> don't mention this user (or any of their groups) are ignored.
>>> Rationale: We don't want to explicitly repeat inherited access
>>> specs that don't change for the respective path / section.
>> This is not entirely true, as seen in the fix for SVN-4793. If a user is
>> "not mentioned" in an inverted selector, those rights do propagate to
>> the global level. For example:
>>
>> [groups]
>> readers = foo, bar
>>
>> [/]
>> ~@readers = rw
>> @readers = r
>>
>>
>> In this case 'user' has read-write access to '[/]' even though she's not
>> mentioned anywhere in the authz file or the specific ACL for '[/]'.
>>
>>
>>>> In 1.9 any repeat acl lines that were the exact same match, such as:
>>>>
>>>> [/some/path]
>>>> user = rw
>>>> user = r
>>>>
>>>> resulted in the last line overriding all the other lines, so user=r in
>>>> the example above. In 1.10 the lines combine, so user=rw in the example
>>>> above. Is this a bug in 1.10, or a bug in 1.9 that is fixed in 1.10, or
>>>> an acceptable behaviour change?
>>> Ouch. That is a bad one and an oversight in the design - I think.
>>>
>>> According to (3), 1.9 behaves correctly. I guess we consider it
>>> an unordered collection in 1.10 and then a union is the only thing
>>> that approximates intent when a user is a member of different
>>> groups with different access rights. Strict ordering becomes
>>> very useful when assigning rights to groups:
>>>
>>> [/some/path]
>>> @Users = rw
>>> @BadUsers = r
>> We already have strict ordering within an ACL (authz_acl_t in
>> libsvn_repos/authz.h):
>>
>> /* All other user- or group-specific access rights.
>> Aliases are replaced with their definitions, rules for the same
>> user or group are merged. */
>> apr_array_header_t *user_access;
>>
>>
>>
>> The "merge" semantics was intentional; if we decide it's a bug (and I
>> think it is), it's fairly easy to change. I would lean in the direction
>> of making repeating the same access entry selection a hard error at
>> parsing time. This requires changes to the merge semantics implemented
>> in add_access_entry() and merge_alias_ace() in
>> libsvn_repos/authz_parse.c. The nice part is that it would catch errors
>> like this:
>>
>> [aliases]
>> afoo = foo
>> abar = bar
>>
>> [/]
>> &afoo = rw
>> foo = r
>> ~&abar = rw
>> ~bar = r
>>
>>
>> With the current implementation we translate the ACL to:
>>
>> [/]
>> foo = rw
>> foo = r
>> ~bar = rw
>> ~bar = r
>>
>>
>> and even with strict ordering I'd say this is a bug and not intentional.
>
> Note that this should also be an error:
>
> [/]
> $anonymous = r
> ~$authenticated = rw
I have a patch ready, here are some examples of what it does (currently,
all these examples are valid and produce merged access rights):
$ cat authz.conf
[/]
user = rw
user = r
$ svnauthz validate authz.conf
svnauthz: E220003: Error while parsing authz file: 'authz.conf':
svnauthz: E220003: Duplicate access entry 'user' in rule [/]
$ cat authz.conf
[/]
$authenticated = rw
~$anonymous = r
$ svnauthz validate authz.conf
svnauthz: E220003: Error while parsing authz file: 'authz.conf':
svnauthz: E220003: Duplicate access entry '~$anonymous' (matches '$authenticated') in rule [/]
$ cat authz.conf
[aliases]
resu = user
[/]
~&resu = rw
~user = r
$ svnauthz validate authz.conf
svnauthz: E220003: Error while parsing authz file: 'authz.conf':
svnauthz: E220003: Duplicate access entry '~&resu' (matches '~user') in rule [/]
-- Brane
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 02.12.2018 08:25, Branko Čibej wrote:
> On 08.09.2018 11:17, Stefan Fuhrmann wrote:
>> These are the guiding principles for the 1.10 authz design:
>>
>> (1) ACLs are only evaluated on a per-user bases; ACLs that
>> don't mention this user (or any of their groups) are ignored.
>> Rationale: We don't want to explicitly repeat inherited access
>> specs that don't change for the respective path / section.
>
> This is not entirely true, as seen in the fix for SVN-4793. If a user is
> "not mentioned" in an inverted selector, those rights do propagate to
> the global level. For example:
>
> [groups]
> readers = foo, bar
>
> [/]
> ~@readers = rw
> @readers = r
>
>
> In this case 'user' has read-write access to '[/]' even though she's not
> mentioned anywhere in the authz file or the specific ACL for '[/]'.
>
>
>>> In 1.9 any repeat acl lines that were the exact same match, such as:
>>>
>>> [/some/path]
>>> user = rw
>>> user = r
>>>
>>> resulted in the last line overriding all the other lines, so user=r in
>>> the example above. In 1.10 the lines combine, so user=rw in the example
>>> above. Is this a bug in 1.10, or a bug in 1.9 that is fixed in 1.10, or
>>> an acceptable behaviour change?
>> Ouch. That is a bad one and an oversight in the design - I think.
>>
>> According to (3), 1.9 behaves correctly. I guess we consider it
>> an unordered collection in 1.10 and then a union is the only thing
>> that approximates intent when a user is a member of different
>> groups with different access rights. Strict ordering becomes
>> very useful when assigning rights to groups:
>>
>> [/some/path]
>> @Users = rw
>> @BadUsers = r
> We already have strict ordering within an ACL (authz_acl_t in
> libsvn_repos/authz.h):
>
> /* All other user- or group-specific access rights.
> Aliases are replaced with their definitions, rules for the same
> user or group are merged. */
> apr_array_header_t *user_access;
>
>
>
> The "merge" semantics was intentional; if we decide it's a bug (and I
> think it is), it's fairly easy to change. I would lean in the direction
> of making repeating the same access entry selection a hard error at
> parsing time. This requires changes to the merge semantics implemented
> in add_access_entry() and merge_alias_ace() in
> libsvn_repos/authz_parse.c. The nice part is that it would catch errors
> like this:
>
> [aliases]
> afoo = foo
> abar = bar
>
> [/]
> &afoo = rw
> foo = r
> ~&abar = rw
> ~bar = r
>
>
> With the current implementation we translate the ACL to:
>
> [/]
> foo = rw
> foo = r
> ~bar = rw
> ~bar = r
>
>
> and even with strict ordering I'd say this is a bug and not intentional.
Note that this should also be an error:
[/]
$anonymous = r
~$authenticated = rw
-- Brane
Re: authz changes between 1.9 and 1.10
Posted by Branko Čibej <br...@apache.org>.
On 08.09.2018 11:17, Stefan Fuhrmann wrote:
> These are the guiding principles for the 1.10 authz design:
>
> (1) ACLs are only evaluated on a per-user bases; ACLs that
> don't mention this user (or any of their groups) are ignored.
> Rationale: We don't want to explicitly repeat inherited access
> specs that don't change for the respective path / section.
This is not entirely true, as seen in the fix for SVN-4793. If a user is
"not mentioned" in an inverted selector, those rights do propagate to
the global level. For example:
[groups]
readers = foo, bar
[/]
~@readers = rw
@readers = r
In this case 'user' has read-write access to '[/]' even though she's not
mentioned anywhere in the authz file or the specific ACL for '[/]'.
>> In 1.9 any repeat acl lines that were the exact same match, such as:
>>
>> [/some/path]
>> user = rw
>> user = r
>>
>> resulted in the last line overriding all the other lines, so user=r in
>> the example above. In 1.10 the lines combine, so user=rw in the example
>> above. Is this a bug in 1.10, or a bug in 1.9 that is fixed in 1.10, or
>> an acceptable behaviour change?
> Ouch. That is a bad one and an oversight in the design - I think.
>
> According to (3), 1.9 behaves correctly. I guess we consider it
> an unordered collection in 1.10 and then a union is the only thing
> that approximates intent when a user is a member of different
> groups with different access rights. Strict ordering becomes
> very useful when assigning rights to groups:
>
> [/some/path]
> @Users = rw
> @BadUsers = r
We already have strict ordering within an ACL (authz_acl_t in
libsvn_repos/authz.h):
/* All other user- or group-specific access rights.
Aliases are replaced with their definitions, rules for the same
user or group are merged. */
apr_array_header_t *user_access;
The "merge" semantics was intentional; if we decide it's a bug (and I
think it is), it's fairly easy to change. I would lean in the direction
of making repeating the same access entry selection a hard error at
parsing time. This requires changes to the merge semantics implemented
in add_access_entry() and merge_alias_ace() in
libsvn_repos/authz_parse.c. The nice part is that it would catch errors
like this:
[aliases]
afoo = foo
abar = bar
[/]
&afoo = rw
foo = r
~&abar = rw
~bar = r
With the current implementation we translate the ACL to:
[/]
foo = rw
foo = r
~bar = rw
~bar = r
and even with strict ordering I'd say this is a bug and not intentional.
> I guess the fix in 1.10 is simple but will change 1.10 behavior.
> My proposal here:
>
> * apply replacement semantics here as in 1.9
> * error out / warn on repeated lines with the same user or group
> (this is hardly ever intentional)
^^^ this
> * provide a script / tool that takes 1.10 authz and checks for
> changed behavior ("r" after "rw" rules etc.)
>
> The last one is a bit of work but would be really handy.
The remaining ambiguity that I would prefer _not_ to resolve at parsing
time is this:
[groups]
@readers = foo, bar, user
@writers = baz, quz, user
[/]
@writers = rw
@readers = r
With strict ordering and replacement semantics 'user' would get
read-only rights. But that doesn't seem right; surely if someone is a
member of both 'readers' and 'writers' groups, they should get merged
access rights of both?
Note that the current behaviour is to merge.
-- Brane