You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Doug Robinson <do...@wandisco.com> on 2019/01/18 22:07:57 UTC
SVN 1.10 AuthZ file parsing too strict?!
Honored committers (and the rest of us):
It's come to my attention that if a group is defined in an AuthZ
file without an associated account that SVN is, as of 1.10, generating
an error and failing to allow the use of that AuthZ file.
Example:
[groups]
goodGroup = acct1
goodGroup2 = acct1, acct2
badGroup =
[repoName:/someplace]
@badGroup = rw
svnauthz: E220003: Error while parsing authz file: ...
svnauthz: E220003: Access entry refers to undefined group ...
My thoughts:
1. From a compatibility standpoint it really should be a Warning,
not an Error. If there's no accounts then certainly it can have
no impact on the security of the repository/ies.
2. From a usability standpoint it really should simply be supported.
The AuthZ file is a representation of a team structure. There are
times when teams will get reduced headcount down to zero and then
back up again. To deal with that use case with SVN 1.10 means
either:
a) stripping out all references to the team and losing all of the
places where that team requires access
b) configuring a dummy account for the team and hoping that the
account will never be created
c) leaving the team around and fixing SVN to allow an empty team
My preference would be first 2c and, if not, then 1. But that's
me.
Not sure about the history of why this change was made? I'd like
to better understand.
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: SVN 1.10 AuthZ file parsing too strict?!
Posted by Branko Čibej <br...@apache.org>.
On 19.01.2019 22:24, Branko Čibej wrote:
> On 18.01.2019 23:07, Doug Robinson wrote:
>> Honored committers (and the rest of us):
>>
>> It's come to my attention that if a group is defined in an AuthZ
>> file without an associated account that SVN is, as of 1.10, generating
>> an error and failing to allow the use of that AuthZ file.
>>
>> Example:
>>
>> [groups]
>> goodGroup = acct1
>> goodGroup2 = acct1, acct2
>> badGroup =
>>
>> [repoName:/someplace]
>> @badGroup = rw
>>
>> svnauthz: E220003: Error while parsing authz file: ...
>> svnauthz: E220003: Access entry refers to undefined group ...
>
> Thinking about this some more: If we add a warning callback to the
> parser (I have that implemented locally), we may as well relax the
> requirement for a group being /defined/, not just non-empty. For
> example, this authz file will also fail to parse with the same error as
> shown above:
>
> [/]
> @nosuchgroup = rw
>
>
> and this one will also complain about an undefined group, but with a
> different message:
>
> [groups]
> somegroup = @nosuchgroup
>
>
> In both cases, instead of exiting with an error, just ignoring the ACE
> or group membership wouldn't affect the semantics of the authz file. If
> we can print warnings about such issues in 'svnauthz', users would still
> have a way to find such bugs in their authz files.
>
> Relaxing the restrictions on undefined groups (and maybe aliases, too?)
> would be a change for the next minor release.
>
> Thoughts?
If we're going to change the public API of the authz parser, we could
also add an equivalent of a -Werror flag to svnauthz. That could be
useful for scripts so that warnings don't get ignored. We could also add
such a flag to the parser itself, but I'm not sure how such a thing
could be used at the repository level.
-- Brane
Re: SVN 1.10 AuthZ file parsing too strict?!
Posted by Branko Čibej <br...@apache.org>.
On 27.01.2019 19:58, Daniel Shahaf wrote:
> [ Sorry for the late reply; I've been offline. ]
>
> Branko Čibej wrote on Tue, Jan 22, 2019 at 15:54:40 +0100:
>> On 19.01.2019 22:24, Branko Čibej wrote:
>>> Thinking about this some more: If we add a warning callback to the
>>> parser (I have that implemented locally), we may as well relax the
>>> requirement for a group being /defined/, not just non-empty. For
>>> example, this authz file will also fail to parse with the same error as
>>> shown above:
>>>
>>> [/]
>>> @nosuchgroup = rw
> I think that in the case the RHS is empty (= "no" in `svnauthz accessof`
> terms), converting an error to a silent no-op could be a regression,
> whereby an @nosuchgroup=no directive would result in some authenticated
> principals having read access that they shouldn't have, rather than in
> a hard error. That is: it would be better to hard fail and block all
> access, than to silently give more access than the admin intended.
>
> From PEP 20:
>
> Errors should never pass silently.
>
> In the face of ambiguity, refuse the temptation to guess.
>
> Next, «@nosuchgroup=(empty string)» and «@nosuchgroup=rw» should be
> either both valid or both errors, so I think the latter shouldn't be
> changed either.
>
> Anyone who wants the proposed semantics can achieve them today by
> defining nosuchgroup to an empty group.
>
>>> and this one will also complain about an undefined group, but with a
>>> different message:
>>>
>>> [groups]
>>> somegroup = @nosuchgroup
>>>
>>>
>>> In both cases, instead of exiting with an error, just ignoring the ACE
>>> or group membership wouldn't affect the semantics of the authz file. If
>>> we can print warnings about such issues in 'svnauthz', users would still
>>> have a way to find such bugs in their authz files.
>>>
>>> Relaxing the restrictions on undefined groups (and maybe aliases, too?)
>>> would be a change for the next minor release.
>>>
>>> Thoughts?
>>
>> If no-one has an opinion, I'm going to do this (closing #4803) and also
>> fix #4794 in the same way.
> Brane and I discussed this on IRC
> <https://colabti.org/irclogger/irclogger_log/svn-dev?date=2019-01-22#l36>;
> the gist is:
>
> - #4794:
>
> + is a regression whereby the semantics of authz file syntax were
> changed
>
> + we shouldn't change the semantics in a patch release
>
> + in the next minor release, we could either restore the original
> semantics, or make the syntax in question a fatal error
>
> - no objections to warning about using defined, but empty, groups
> («@definedtoempty» on the LHS)
>
> I think that covers it…
It does. The current state is that empty groups are allowed but ignored,
and the use of undefined groups is an error.
For #4794, I propose to solve it by reverting back to the 1.9.x
behaviour (probably in 1.12) and issuing a warning, since we now have
the infrastructure to do that. The overriding reason for this is being
that we already have a last-match-wins rule for ACLs, so we may as well
have the same rule for ACEs, so that the logic of reading the authz file
is consistent.
-- Brane
Re: SVN 1.10 AuthZ file parsing too strict?!
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
[ Sorry for the late reply; I've been offline. ]
Branko Čibej wrote on Tue, Jan 22, 2019 at 15:54:40 +0100:
> On 19.01.2019 22:24, Branko Čibej wrote:
> > Thinking about this some more: If we add a warning callback to the
> > parser (I have that implemented locally), we may as well relax the
> > requirement for a group being /defined/, not just non-empty. For
> > example, this authz file will also fail to parse with the same error as
> > shown above:
> >
> > [/]
> > @nosuchgroup = rw
I think that in the case the RHS is empty (= "no" in `svnauthz accessof`
terms), converting an error to a silent no-op could be a regression,
whereby an @nosuchgroup=no directive would result in some authenticated
principals having read access that they shouldn't have, rather than in
a hard error. That is: it would be better to hard fail and block all
access, than to silently give more access than the admin intended.
From PEP 20:
Errors should never pass silently.
In the face of ambiguity, refuse the temptation to guess.
Next, «@nosuchgroup=(empty string)» and «@nosuchgroup=rw» should be
either both valid or both errors, so I think the latter shouldn't be
changed either.
Anyone who wants the proposed semantics can achieve them today by
defining nosuchgroup to an empty group.
> > and this one will also complain about an undefined group, but with a
> > different message:
> >
> > [groups]
> > somegroup = @nosuchgroup
> >
> >
> > In both cases, instead of exiting with an error, just ignoring the ACE
> > or group membership wouldn't affect the semantics of the authz file. If
> > we can print warnings about such issues in 'svnauthz', users would still
> > have a way to find such bugs in their authz files.
> >
> > Relaxing the restrictions on undefined groups (and maybe aliases, too?)
> > would be a change for the next minor release.
> >
> > Thoughts?
>
>
> If no-one has an opinion, I'm going to do this (closing #4803) and also
> fix #4794 in the same way.
Brane and I discussed this on IRC
<https://colabti.org/irclogger/irclogger_log/svn-dev?date=2019-01-22#l36>;
the gist is:
- #4794:
+ is a regression whereby the semantics of authz file syntax were
changed
+ we shouldn't change the semantics in a patch release
+ in the next minor release, we could either restore the original
semantics, or make the syntax in question a fatal error
- no objections to warning about using defined, but empty, groups
(«@definedtoempty» on the LHS)
I think that covers it…
Cheers,
Daniel
Re: SVN 1.10 AuthZ file parsing too strict?!
Posted by Branko Čibej <br...@apache.org>.
On 19.01.2019 22:24, Branko Čibej wrote:
> On 18.01.2019 23:07, Doug Robinson wrote:
>> Honored committers (and the rest of us):
>>
>> It's come to my attention that if a group is defined in an AuthZ
>> file without an associated account that SVN is, as of 1.10, generating
>> an error and failing to allow the use of that AuthZ file.
>>
>> Example:
>>
>> [groups]
>> goodGroup = acct1
>> goodGroup2 = acct1, acct2
>> badGroup =
>>
>> [repoName:/someplace]
>> @badGroup = rw
>>
>> svnauthz: E220003: Error while parsing authz file: ...
>> svnauthz: E220003: Access entry refers to undefined group ...
>
> Thinking about this some more: If we add a warning callback to the
> parser (I have that implemented locally), we may as well relax the
> requirement for a group being /defined/, not just non-empty. For
> example, this authz file will also fail to parse with the same error as
> shown above:
>
> [/]
> @nosuchgroup = rw
>
>
> and this one will also complain about an undefined group, but with a
> different message:
>
> [groups]
> somegroup = @nosuchgroup
>
>
> In both cases, instead of exiting with an error, just ignoring the ACE
> or group membership wouldn't affect the semantics of the authz file. If
> we can print warnings about such issues in 'svnauthz', users would still
> have a way to find such bugs in their authz files.
>
> Relaxing the restrictions on undefined groups (and maybe aliases, too?)
> would be a change for the next minor release.
>
> Thoughts?
If no-one has an opinion, I'm going to do this (closing #4803) and also
fix #4794 in the same way.
-- Brane
Re: SVN 1.10 AuthZ file parsing too strict?!
Posted by Branko Čibej <br...@apache.org>.
On 18.01.2019 23:07, Doug Robinson wrote:
> Honored committers (and the rest of us):
>
> It's come to my attention that if a group is defined in an AuthZ
> file without an associated account that SVN is, as of 1.10, generating
> an error and failing to allow the use of that AuthZ file.
>
> Example:
>
> [groups]
> goodGroup = acct1
> goodGroup2 = acct1, acct2
> badGroup =
>
> [repoName:/someplace]
> @badGroup = rw
>
> svnauthz: E220003: Error while parsing authz file: ...
> svnauthz: E220003: Access entry refers to undefined group ...
Thinking about this some more: If we add a warning callback to the
parser (I have that implemented locally), we may as well relax the
requirement for a group being /defined/, not just non-empty. For
example, this authz file will also fail to parse with the same error as
shown above:
[/]
@nosuchgroup = rw
and this one will also complain about an undefined group, but with a
different message:
[groups]
somegroup = @nosuchgroup
In both cases, instead of exiting with an error, just ignoring the ACE
or group membership wouldn't affect the semantics of the authz file. If
we can print warnings about such issues in 'svnauthz', users would still
have a way to find such bugs in their authz files.
Relaxing the restrictions on undefined groups (and maybe aliases, too?)
would be a change for the next minor release.
Thoughts?
-- Brane
Re: SVN 1.10 AuthZ file parsing too strict?!
Posted by Branko Čibej <br...@apache.org>.
On 19.01.2019 21:12, Branko Čibej wrote:
> On 19.01.2019 17:17, Branko Čibej wrote:
>> Hi Doug!
>>
>> On 18.01.2019 23:07, Doug Robinson wrote:
>>> Honored committers (and the rest of us):
>>>
>>> It's come to my attention that if a group is defined in an AuthZ
>>> file without an associated account that SVN is, as of 1.10, generating
>>> an error and failing to allow the use of that AuthZ file.
>>>
>>> Example:
>>>
>>> [groups]
>>> goodGroup = acct1
>>> goodGroup2 = acct1, acct2
>>> badGroup =
>>>
>>> [repoName:/someplace]
>>> @badGroup = rw
>>>
>>> svnauthz: E220003: Error while parsing authz file: ...
>>> svnauthz: E220003: Access entry refers to undefined group ...
>>
>>
>> Thanks for bringing this up. It's most definitely a bug ... the group
>> is defined, it's just empty. If the parser accepts an empty group
>> definition, it should also allow its use in rules. That ACE should
>> just be ignored, since it has no effect.
>>
>> Emitting a warning from svnauthz would be a bonus, but I'm not sure
>> offhand how to do that, as I don't think we have a warning callback
>> in the parser; it might involve some API changes, but nothing drastic.
>>
>> Can you please write this up as a Jira issue and assign it to me?
>>
>>> My thoughts:
>>>
>>> 1. From a compatibility standpoint it really should be a Warning,
>>> not an Error. If there's no accounts then certainly it can have
>>> no impact on the security of the repository/ies.
>>
>> And further, if one generates the 'groups' file from LDAP, for
>> example, some unintentional intermediate state could break Subversion
>> authz even though there's nothing specifically wrong with it.
>>
>>> 2. From a usability standpoint it really should simply be supported.
>>> The AuthZ file is a representation of a team structure. There are
>>> times when teams will get reduced headcount down to zero and then
>>> back up again. To deal with that use case with SVN 1.10 means
>>> either:
>>>
>>> a) stripping out all references to the team and losing all of the
>>> places where that team requires access
>>>
>>> b) configuring a dummy account for the team and hoping that the
>>> account will never be created
>>>
>>> c) leaving the team around and fixing SVN to allow an empty team
>>>
>>> My preference would be first 2c and, if not, then 1. But that's
>>> me.
>>>
>>> Not sure about the history of why this change was made? I'd like
>>> to better understand.
>>
>> It's an unintentional consequence of the parser rewrite. It appears
>> that we don't have a test for this case, and code inspection didn't
>> catch the change in semantics ... and so it landed in released code.
>>
>> This is about to change. :)
>
> Fixed in r1851687; I'll nominate this for backport to 1.10.x and 1.11.x.
>
> Emitting a warning from svnauthz requires a public API change that
> can't be released before 1.12, so please do write that Jira issue.
r1851823
Re: SVN 1.10 AuthZ file parsing too strict?!
Posted by Doug Robinson <do...@wandisco.com>.
Brane:
See below.
On Sat, Jan 19, 2019 at 15:12 Branko Čibej <br...@apache.org> wrote:
> On 19.01.2019 17:17, Branko Čibej wrote:
>
> Hi Doug!
>
> On 18.01.2019 23:07, Doug Robinson wrote:
>
> Honored committers (and the rest of us):
>
> It's come to my attention that if a group is defined in an AuthZ
> file without an associated account that SVN is, as of 1.10, generating
> an error and failing to allow the use of that AuthZ file.
>
> Example:
>
> [groups]
> goodGroup = acct1
> goodGroup2 = acct1, acct2
> badGroup =
>
> [repoName:/someplace]
> @badGroup = rw
>
> svnauthz: E220003: Error while parsing authz file: ...
> svnauthz: E220003: Access entry refers to undefined group ...
>
>
>
> Thanks for bringing this up. It's most definitely a bug ... the group is
> defined, it's just empty. If the parser accepts an empty group definition,
> it should also allow its use in rules. That ACE should just be ignored,
> since it has no effect.
>
> Emitting a warning from svnauthz would be a bonus, but I'm not sure
> offhand how to do that, as I don't think we have a warning callback in the
> parser; it might involve some API changes, but nothing drastic.
>
> Can you please write this up as a Jira issue and assign it to me?
>
> My thoughts:
>
> 1. From a compatibility standpoint it really should be a Warning,
> not an Error. If there's no accounts then certainly it can have
> no impact on the security of the repository/ies.
>
>
> And further, if one generates the 'groups' file from LDAP, for example,
> some unintentional intermediate state could break Subversion authz even
> though there's nothing specifically wrong with it.
>
> 2. From a usability standpoint it really should simply be supported.
> The AuthZ file is a representation of a team structure. There are
> times when teams will get reduced headcount down to zero and then
> back up again. To deal with that use case with SVN 1.10 means
> either:
>
> a) stripping out all references to the team and losing all of the
> places where that team requires access
>
> b) configuring a dummy account for the team and hoping that the
> account will never be created
>
> c) leaving the team around and fixing SVN to allow an empty team
>
> My preference would be first 2c and, if not, then 1. But that's
> me.
>
> Not sure about the history of why this change was made? I'd like
> to better understand.
>
>
> It's an unintentional consequence of the parser rewrite. It appears that
> we don't have a test for this case, and code inspection didn't catch the
> change in semantics ... and so it landed in released code.
>
> This is about to change. :)
>
>
> Fixed in r1851687; I'll nominate this for backport to 1.10.x and 1.11.x.
>
Nice!
>
> Emitting a warning from svnauthz requires a public API change that can't
> be released before 1.12, so please do write that Jira issue.
>
Will do - on Monday! Cheers!
Thank you again.
Doug
>
> -- Brane
>
> --
*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: SVN 1.10 AuthZ file parsing too strict?!
Posted by Branko Čibej <br...@apache.org>.
On 19.01.2019 17:17, Branko Čibej wrote:
> Hi Doug!
>
> On 18.01.2019 23:07, Doug Robinson wrote:
>> Honored committers (and the rest of us):
>>
>> It's come to my attention that if a group is defined in an AuthZ
>> file without an associated account that SVN is, as of 1.10, generating
>> an error and failing to allow the use of that AuthZ file.
>>
>> Example:
>>
>> [groups]
>> goodGroup = acct1
>> goodGroup2 = acct1, acct2
>> badGroup =
>>
>> [repoName:/someplace]
>> @badGroup = rw
>>
>> svnauthz: E220003: Error while parsing authz file: ...
>> svnauthz: E220003: Access entry refers to undefined group ...
>
>
> Thanks for bringing this up. It's most definitely a bug ... the group
> is defined, it's just empty. If the parser accepts an empty group
> definition, it should also allow its use in rules. That ACE should
> just be ignored, since it has no effect.
>
> Emitting a warning from svnauthz would be a bonus, but I'm not sure
> offhand how to do that, as I don't think we have a warning callback in
> the parser; it might involve some API changes, but nothing drastic.
>
> Can you please write this up as a Jira issue and assign it to me?
>
>> My thoughts:
>>
>> 1. From a compatibility standpoint it really should be a Warning,
>> not an Error. If there's no accounts then certainly it can have
>> no impact on the security of the repository/ies.
>
> And further, if one generates the 'groups' file from LDAP, for
> example, some unintentional intermediate state could break Subversion
> authz even though there's nothing specifically wrong with it.
>
>> 2. From a usability standpoint it really should simply be supported.
>> The AuthZ file is a representation of a team structure. There are
>> times when teams will get reduced headcount down to zero and then
>> back up again. To deal with that use case with SVN 1.10 means
>> either:
>>
>> a) stripping out all references to the team and losing all of the
>> places where that team requires access
>>
>> b) configuring a dummy account for the team and hoping that the
>> account will never be created
>>
>> c) leaving the team around and fixing SVN to allow an empty team
>>
>> My preference would be first 2c and, if not, then 1. But that's
>> me.
>>
>> Not sure about the history of why this change was made? I'd like
>> to better understand.
>
> It's an unintentional consequence of the parser rewrite. It appears
> that we don't have a test for this case, and code inspection didn't
> catch the change in semantics ... and so it landed in released code.
>
> This is about to change. :)
Fixed in r1851687; I'll nominate this for backport to 1.10.x and 1.11.x.
Emitting a warning from svnauthz requires a public API change that can't
be released before 1.12, so please do write that Jira issue.
-- Brane
Re: SVN 1.10 AuthZ file parsing too strict?!
Posted by Branko Čibej <br...@apache.org>.
On 19.01.2019 17:17, Branko Čibej wrote:
> This is about to change. :)
I added a test for this:
https://svn.apache.org/viewvc?view=revision&revision=1851676
-- Brane
Re: SVN 1.10 AuthZ file parsing too strict?!
Posted by Branko Čibej <br...@apache.org>.
Hi Doug!
On 18.01.2019 23:07, Doug Robinson wrote:
> Honored committers (and the rest of us):
>
> It's come to my attention that if a group is defined in an AuthZ
> file without an associated account that SVN is, as of 1.10, generating
> an error and failing to allow the use of that AuthZ file.
>
> Example:
>
> [groups]
> goodGroup = acct1
> goodGroup2 = acct1, acct2
> badGroup =
>
> [repoName:/someplace]
> @badGroup = rw
>
> svnauthz: E220003: Error while parsing authz file: ...
> svnauthz: E220003: Access entry refers to undefined group ...
Thanks for bringing this up. It's most definitely a bug ... the group is
defined, it's just empty. If the parser accepts an empty group
definition, it should also allow its use in rules. That ACE should just
be ignored, since it has no effect.
Emitting a warning from svnauthz would be a bonus, but I'm not sure
offhand how to do that, as I don't think we have a warning callback in
the parser; it might involve some API changes, but nothing drastic.
Can you please write this up as a Jira issue and assign it to me?
> My thoughts:
>
> 1. From a compatibility standpoint it really should be a Warning,
> not an Error. If there's no accounts then certainly it can have
> no impact on the security of the repository/ies.
And further, if one generates the 'groups' file from LDAP, for example,
some unintentional intermediate state could break Subversion authz even
though there's nothing specifically wrong with it.
> 2. From a usability standpoint it really should simply be supported.
> The AuthZ file is a representation of a team structure. There are
> times when teams will get reduced headcount down to zero and then
> back up again. To deal with that use case with SVN 1.10 means
> either:
>
> a) stripping out all references to the team and losing all of the
> places where that team requires access
>
> b) configuring a dummy account for the team and hoping that the
> account will never be created
>
> c) leaving the team around and fixing SVN to allow an empty team
>
> My preference would be first 2c and, if not, then 1. But that's
> me.
>
> Not sure about the history of why this change was made? I'd like
> to better understand.
It's an unintentional consequence of the parser rewrite. It appears that
we don't have a test for this case, and code inspection didn't catch the
change in semantics ... and so it landed in released code.
This is about to change. :)
-- Brane