On Mon, Jan 28, 2013 at 3:10 AM, Philip Martin
<philip.martin_at_wandisco.com> wrote:
> Evgeny Kotkov <evgeny.kotkov_at_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.
Received on 2013-01-28 18:22:10 CET