[PATCH] Introduce AuthzSVNGroupsFile configuration option for mod_authz_svn

[Evgeny Kotkov <ev...@visualsvn.com>]

Hi,

When AuthzSVNReposRelativeAccessFile directive is being used and
authorization rules are stored per-repository, it is usually required to
have a single set of groups for all repositories.

In other words, there can be a 'developers' group, whose members should
have access to all repositories.  To avoid the duplication of the 'developers'
group definition across multiple authz files, it would be great to have a
single place to define these groups.

The attached patch adds the 'AuthzSVNGroupsFile' option to specify the
dedicated file where the group definitions are stored.

Having such an option seems to be a logical step forward since we already
have the AuthzSVNReposRelativeAccessFile option.  The introduced option does
not affect the existing behavior � if it is not present, everything will
remain the same.

It might seem that the patch is pretty big, but the actual change is quite
local.  Group definitions are just copied when the authz config is loaded.
When the 'AuthzSVNGroupsFile' option is used, it is prohibited to define
groups in the authz files.

All tests (local, svn, http) including the new ones pass.


Thanks and regards,
Evgeny Kotkov

Re: [PATCH] Introduce AuthzSVNGroupsFile configuration option for mod_authz_svn

[Ben Reser <be...@reser.org>]

On Fri, Jan 25, 2013 at 2:30 AM, Philip Martin
<ph...@wandisco.com> wrote:
> Are administrators going to want both relative path and absolute path
> versions of this directive?

If you want repo unique groups you can just put it in authz file and
not use this directive.  So I kinda doubt there's a use for the repos
relative directive.  I guess the exception would be if you wanted to
share some groups between repos and not with other repos.

That said, it shouldn't be that hard to add the repos relative option
and just have it in case anyone wants it.

> I wonder if we should implement some sort of generic include mechanism
> instead.  Trac uses an INI compatible mechanism:
>
> [inherit]
> file = path/to/another/file
>
> we could
>
> [inherit]
> file = path/to/another/file
> relative_file = path/to/another/file

If we go down this route we have a couple of things to deal with:

1) Where would we root the relative paths? http and svnserve have
different ideas of the root for relative paths.
AuthzSVNReposRelativeAccessFile helped by making the most common root
for svnserve available to http as well, but it doesn't really solve
the problem.  http roots the files either in the ServerRoot
(AuthzSVNAccessFile) or in the conf dir of the repository
(AuthzSVNReposRelativeAccessFile).  svnserve roots the files relative
to the directory which svnserve.conf is located in, which if you
aren't providing --config-file is the conf dir of the repository.  I
suppose the best thing to do is root them in the repositories conf
directory.

2) Doing this means pushing the relative path root into the
svn_repos_authz_* functions since the repository layer doesn't know
the full context.  As it is I feel like how I added the repos_root
argument in there is almost wrong and have thought about removing it
and making the servers have to resolve the repository relative paths
themselves (similar to how the cmdline client has to resolve
repository relative paths before handing them to libsvn_client).
Unlike the repos_root argument there's really no way to get rid of the
ugliness of passing the root for included paths in.

3) Within svnauthz you'll have to allow the relative root to be set
from the command line since it also won't have the context from the
server.  Thus adding more arguments to a command and making it less
intuitive.

4) We parse the authz files as just Subversion conf files (basically
ini files).  It's not until the conf is loaded into memory that we
process it as an authz file.  You couldn't put an [include] anywhere
in the file and have it override things that were already set but not
override things that hadn't been set.  You'd have to say that includes
override anything that is set.  While it would be functional it just
wouldn't be intuitive.  So if we didn't like that it'd mean writing a
new parser.

Obviously we can deal with these things but it becomes a lot messier
than what he's done right now.

Re: [PATCH] Introduce AuthzSVNGroupsFile configuration option for mod_authz_svn

[Daniel Shahaf <d....@daniel.shahaf.name>]

Philip Martin wrote on Fri, Jan 25, 2013 at 10:30:00 +0000:
> I suppose this approach would break the meaning of existing authz files
> already using '[inherit]'.   Another approach would be to use some
> non-INI syntax to define include files.

That's not a problem.

'inherit' is not "groups" or "aliases", doesn't start with a slash, and
doesn't contain a colon; so it has no defined meaning in existing authz
files.

Re: [PATCH] Introduce AuthzSVNGroupsFile configuration option for mod_authz_svn

[Ben Reser <be...@reser.org>]

On Mon, Jan 28, 2013 at 3:10 AM, Philip Martin
<ph...@wandisco.com> wrote:
> Evgeny Kotkov <ev...@visualsvn.com> writes:
>>   * With includes in the configuration files an evil-doer could perform
>>     cross-repository configuration includes.  That theoretically allows
>>     examininig the authorization rules for restricted repositories (e.g. via
>>     bruteforce).
>
> Are you claiming the evil-doer could access the included files but not
> the authz file itself?  How would that be possible?  Why does the same
> argument not apply to the new directive?
>
>>   * As far as I understand, including huge arbitrary files for a single
>>     repository could potentially hang the whole server.
>
> How does breaking an single authz file into multiple files makes this
> worse?  The include system could give the exact same behaviour as your
> new directive if that is what the admin wants.

While it does make it slightly more difficult to secure since you have
to check that all the files have the appropriate permissions, I don't
think it's really that big of a deal.  Lots of configuration systems
have include file functionality.  I agree with Philip this is not a
particularly strong argument against includes.

>>   * Includes add dynamic behavior in the authz scheme.  Without them, the
>>     server administrator configures a static set of files used for the
>>     authorization process.  With includes, however, this set it is no longer
>>     static — users can tell the server, which files it should use to perform
>>     the authz process.
>
> I don't see what is "dynamic" about the include proposal. It's just
> reading multiple files instead of one in svn_repos__authz_read.

I agree with Philip here, I don't really see the difference here.
Both let you use multiple files for authz.  The fact that you can set
the file location in the authz file vs the server configuration
doesn't really change much.

>> - Includes might require merging of access rules and configuration chunks from
>>   multiple files.  With merging the whole authorization scheme could easily
>>   become unobvious and sort of counterintuitive.
>
> It's simple text concatenation.  We have an authz validation utility
> that could spit out the combined file if really necessary.

I think most people would expect that an include option would insert
the text of the other include file as though it was in the same place
in the original file.

Consider an authz file like so:
[/]
* = rw

[include]
file=somefile

[/]
* =

With somefile having:
[/]
* = r

If you just concatenate somefile on the end and replace any already
set values / will be readable.  If you processed the values in
somefile where the [include] was and then continued parsing the
original file you'd end up with no access.

Either implementation is probably going to confuse someone.  We can
discuss which one we prefer but I think Evgeny's point is valid here.
The behavior may not be obvious.

>> - Finally, the solution with a new directive follows the pattern
>>   already used (just add a new option — as it was done with
>>   AuthzSVNReposRelativeAccessFile).
>
> So we now have 3 directives, do we need another directive for
> relative/absolute path to the groups file?  What about the next
> enhancement?  Suppose somebody wants to split "common" rules and
> "per-repo" rules into separate files.  Do we introduce yet another
> directive for that?  Two more directives if we want relative paths?  Or
> 4 directives if we want common groups as well as common rules?

This is a point I agree with.  I'm not really fond of growing more and
more directives.  At some point I think we expect to grow in-repo ACLs
as opposed to this bolted on authz system.

> Also your directive needs a separate implementation in mod_authz_svn,
> svnserve, svnauthz.c and any 3rd party users.  The include system would
> probably be implemented once in the libsvn_repos.

Include would still need a path passed to it for rooting the included
files if they provide relative paths.  So either way we're making
changes all over the place.

Overall I'm really not sure what should be done.  Given that we're
trying to get 1.8 out the door I don't think we really should be
adding new features right now and concentrating on finishing up the
things that we have.

Re: [PATCH] Introduce AuthzSVNGroupsFile configuration option for mod_authz_svn

[Philip Martin <ph...@wandisco.com>]

Evgeny Kotkov <ev...@visualsvn.com> writes:

>   * With includes in the configuration files an evil-doer could perform
>     cross-repository configuration includes.  That theoretically allows
>     examininig the authorization rules for restricted repositories (e.g. via
>     bruteforce).

Are you claiming the evil-doer could access the included files but not
the authz file itself?  How would that be possible?  Why does the same
argument not apply to the new directive?

>   * As far as I understand, including huge arbitrary files for a single
>     repository could potentially hang the whole server.

How does breaking an single authz file into multiple files makes this
worse?  The include system could give the exact same behaviour as your
new directive if that is what the admin wants.

>   * Includes add dynamic behavior in the authz scheme.  Without them, the
>     server administrator configures a static set of files used for the
>     authorization process.  With includes, however, this set it is no longer
>     static — users can tell the server, which files it should use to perform
>     the authz process.

I don't see what is "dynamic" about the include proposal. It's just
reading multiple files instead of one in svn_repos__authz_read.

> - Includes might require merging of access rules and configuration chunks from
>   multiple files.  With merging the whole authorization scheme could easily
>   become unobvious and sort of counterintuitive.

It's simple text concatenation.  We have an authz validation utility
that could spit out the combined file if really necessary.

> - Finally, the solution with a new directive follows the pattern
>   already used (just add a new option — as it was done with
>   AuthzSVNReposRelativeAccessFile).

So we now have 3 directives, do we need another directive for
relative/absolute path to the groups file?  What about the next
enhancement?  Suppose somebody wants to split "common" rules and
"per-repo" rules into separate files.  Do we introduce yet another
directive for that?  Two more directives if we want relative paths?  Or
4 directives if we want common groups as well as common rules?

Also your directive needs a separate implementation in mod_authz_svn,
svnserve, svnauthz.c and any 3rd party users.  The include system would
probably be implemented once in the libsvn_repos.

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: [PATCH] Introduce AuthzSVNGroupsFile configuration option for mod_authz_svn

[Evgeny Kotkov <ev...@visualsvn.com>]

Philip, sorry for the delay in response (I was out of office).

I have carefully considered the include-based approach for this feature,
however, there probably are some drawbacks compared to the approach with the
groups file directive:

- Potential security issues in certain delegation scenarios.  Consider a
  case when there is a Subversion server with multiple per-project repositories,
  an administrator, who performed its initial configuration and project
  managers, who are responsible for configuring the access rules for
  those repositories:

  * With includes in the configuration files an evil-doer could perform
    cross-repository configuration includes.  That theoretically allows
    examininig the authorization rules for restricted repositories (e.g. via
    bruteforce).

  * As far as I understand, including huge arbitrary files for a single
    repository could potentially hang the whole server.

  * Includes add dynamic behavior in the authz scheme.  Without them, the
    server administrator configures a static set of files used for the
    authorization process.  With includes, however, this set it is no longer
    static — users can tell the server, which files it should use to perform
    the authz process.

- Includes might require merging of access rules and configuration chunks from
  multiple files.  With merging the whole authorization scheme could easily
  become unobvious and sort of counterintuitive.

- Finally, the solution with a new directive follows the pattern already used
  (just add a new option — as it was done with AuthzSVNReposRelativeAccessFile).


Regards,
Evgeny Kotkov

On Fri, Jan 25, 2013 at 2:30 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> On Wed, Jan 23, 2013 at 7:27 PM, Evgeny Kotkov
>> <ev...@visualsvn.com> wrote:
>>> When AuthzSVNReposRelativeAccessFile directive is being used and
>>> authorization rules are stored per-repository, it is usually required to
>>> have a single set of groups for all repositories.
>>>
>>> In other words, there can be a 'developers' group, whose members should
>>> have access to all repositories.  To avoid the duplication of the 'developers'
>>> group definition across multiple authz files, it would be great to have a
>>> single place to define these groups.
>>>
>>> The attached patch adds the 'AuthzSVNGroupsFile' option to specify the
>>> dedicated file where the group definitions are stored.
>>>
>> Committed in r1438407. Thanks!
>
> Are administrators going to want both relative path and absolute path
> versions of this directive?
>
> I wonder if we should implement some sort of generic include mechanism
> instead.  Trac uses an INI compatible mechanism:
>
> [inherit]
> file = path/to/another/file
>
> we could
>
> [inherit]
> file = path/to/another/file
> relative_file = path/to/another/file
>
> and combines the values from the other files with the current file.
>
> I suppose this approach would break the meaning of existing authz files
> already using '[inherit]'.   Another approach would be to use some
> non-INI syntax to define include files.
>
> --
> Certified & Supported Apache Subversion Downloads:
> http://www.wandisco.com/subversion/download

Re: [PATCH] Introduce AuthzSVNGroupsFile configuration option for mod_authz_svn

[Philip Martin <ph...@wandisco.com>]

Ivan Zhakov <iv...@visualsvn.com> writes:

> On Wed, Jan 23, 2013 at 7:27 PM, Evgeny Kotkov
> <ev...@visualsvn.com> wrote:
>> When AuthzSVNReposRelativeAccessFile directive is being used and
>> authorization rules are stored per-repository, it is usually required to
>> have a single set of groups for all repositories.
>>
>> In other words, there can be a 'developers' group, whose members should
>> have access to all repositories.  To avoid the duplication of the 'developers'
>> group definition across multiple authz files, it would be great to have a
>> single place to define these groups.
>>
>> The attached patch adds the 'AuthzSVNGroupsFile' option to specify the
>> dedicated file where the group definitions are stored.
>>
> Committed in r1438407. Thanks!

Are administrators going to want both relative path and absolute path
versions of this directive?

I wonder if we should implement some sort of generic include mechanism
instead.  Trac uses an INI compatible mechanism:

[inherit]
file = path/to/another/file

we could 

[inherit]
file = path/to/another/file
relative_file = path/to/another/file

and combines the values from the other files with the current file.

I suppose this approach would break the meaning of existing authz files
already using '[inherit]'.   Another approach would be to use some
non-INI syntax to define include files.

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: [PATCH] Introduce AuthzSVNGroupsFile configuration option for mod_authz_svn

[Ivan Zhakov <iv...@visualsvn.com>]

On Wed, Jan 23, 2013 at 7:27 PM, Evgeny Kotkov
<ev...@visualsvn.com> wrote:
> When AuthzSVNReposRelativeAccessFile directive is being used and
> authorization rules are stored per-repository, it is usually required to
> have a single set of groups for all repositories.
>
> In other words, there can be a 'developers' group, whose members should
> have access to all repositories.  To avoid the duplication of the 'developers'
> group definition across multiple authz files, it would be great to have a
> single place to define these groups.
>
> The attached patch adds the 'AuthzSVNGroupsFile' option to specify the
> dedicated file where the group definitions are stored.
>
Committed in r1438407. Thanks!

-- 
Ivan Zhakov