|
|
|
|
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
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.
|
|
|
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
|
|
|
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
|
|
|
See below.
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
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.
|
|
|
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
|
|
|
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
|
|
|
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
|
|
|
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
|
|
|
[ 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
|
|
|
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
|
|
|
On 2019/01/18 22:07:57, 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 ...>
>
> 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* [hidden email]>
>
> -- >
>
>
> * *>
>
> **The LIVE DATA Company>
> *Find out more >
> *wandisco.com *>
>
>
>
> >
> >
> *>
>
>
> 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.>
>
Sent from my iPhone
|
|
Locked
