[PATCH] prototype replay authz checks

[Garrett Rooney <ro...@electricjellyfish.net>]

Ok, so here's the first cut of a patch to add the ability to control
replay access to our authz system.  It takes the form of a new
permission (p, for 'play', since r is taken) and is implemented by
adding appropriate authz checks in the ra_svn replay impl and in
mod_authz_svn (by adding a filter that looks for replay reports).

This is not my final word on this subject, there are a number of
issues I want to resolve before going forward, but I wanted to get the
patch out there so that people can take a look at the direction I'm
going in.

Known problems/deviations from previous discussions:

This defaults to turning replay off, not on.  Why?  Well, it seems to
fit better with our authz system, otherwise if you turn replay off at
the top level it's too easy to accidentally turn it on at a lower
level of the tree as part of a rule that's intended to turn access
off.  For example, a common idiom now is to add a * = line to keep
people from accessing a directory, if replay was on by default and you
had to add a p permission to turn it off, that idiom would suddenly
ALLOW replay.

This doesn't do anything about the problem of people being able to
check out huge parts of the tree.  The update report (used for
checkout) is really complex, and I haven't had time to revisit it and
find good ways to detect that sort of behavior yet.  I'm not sure if
I'll have time to get to that before 1.4 branches, but we'll see.  I
figured getting something out there for replay alone was better than
not getting anything out at all.

The report filter is not very generic.  Right now it's located in
mod_authz_svn and specifically looks for just the replay report.  I'd
like to make it more general and callback driven, and move it to
mod_dav_svn, so it can be reused, since filtering the report bodies is
kind of tricky and might be useful for other purposes in the future. 
I can envision custom logging modules that might want it, or
specialized authz modules, and there's no reason to make them
duplicate that code.

The report filter's error is not very friendly.  I'd like to figure
out how to make it send back an XML serialized error that'll show up
nicely in the client's output, but I haven't gotten around to that
yet.

The report filter's xml parsing doesn't handle namespaces correctly. 
I may just steal this code from ra_serf, since Justin has already
written it, but it would be nice if we could move it someplace generic
instead of duplicating it.

Anyway, let me know what you think.  Log message and diff follows.

-garrett

[[[
Add the concept of replay permission to our authz system.

* subversion/include/svn_repos.h
  (svn_authz_recursive): New authz access type.

* subversion/libsvn_repos/authz.c
  (authz_access_is_granted): Handle replay authz.
  (authz_parse_line): Parse replay permissions out of the line.

* subversion/libsvn_ra_svn/client.c
  (ra_svn_replay): Add in an extra read_cmd_response so we can bail
   out before starting the editor drive.

* subversion/svnserve/serve.c
  (authz_check_access_cb_func): Fix indentation.
  (replay): Check replay authz, add a write_cmd_response so we can
   successfully bail if replay isn't allowed.

* subversion/mod_authz_svn/mod_authz_svn.c
  (report_type_t): New enum.
  (report_filter_ctx): New struct, holds data for our report filter.
  (report_filter): New filter function, parses incoming reports to
   determine their type and takes appropriate action.
  (start_element): New function, figures out what type of report this
   is.
  (end_element, cdata): Dummy expat callback functions, empty for now.
  (req_check_access): Make formatting consistent, set up a report
   filter if this is a report request.
  (register_hooks): Register our report filter.

* subversion/tests/cmdline/svnsync_tests.py
  (basic_authz, copy_from_unreadable_dir): Turn replay on for these
   tests.
  (blocked_replay): New test for the ability to block replay.
  (test_list): Add the new test.
]]]

Re: [PATCH] prototype replay authz checks

[Garrett Rooney <ro...@electricjellyfish.net>]

On 4/28/06, David Anderson <da...@calixo.net> wrote:
> * Garrett Rooney <ro...@electricjellyfish.net> [2006-04-27 22:00:45]:
> > To get this kind of behavior we either need to have nonrecursive authz
> > permissions, which means each and every path that you want to block
> > checkout of needs to be tagged in the authz conf file, which would
> > really suck, or we need wildcards of some sort.  Adding wildcard
> > support without having it devolve into "ok, now we enumerate all
> > sections in the file and see if any of them match" seems like it will
> > require some thought...
> >
> > So before I dive into the whole "how should wildcards work" mess any
> > further than I already have, has anyone thought about that sort of
> > thing yet?  Anyone got a design proposal sitting around that they
> > haven't sent out yet?
>
> There is a proposal for wildcards around, yes.  The gist of it is that
> when the authz code loads, it reads and scans through the whole authz
> file once.  So, we add a check that sees if there are wildcard chars
> anywhere in the configured paths.  If so, we remember that there were.
>
> Then, during a nonrecursive authz lookup, we first do the usual
> dereferencing attempts of exact matches.  If those return no
> conclusive answer, and if we found wildcards during the initial scan,
> then we do a full sweep to attempt to match on wildcard terms.  For
> recursive authz lookups, do the normal full sweep, with extra wildcard
> handling if necessary.
>
> Yeah, it remains slow and horrible, but I don't really see a way that
> we can shy away from that (special wildcard lookup hash map built
> during init? Is it worth it?), and at least that way you only get the
> performance hit if you use the wildcards.
>
> Exact matches have priority over wildcard matches, and the proposal is
> to implement only the * wildcard, no ? or PCRE matching.  That should
> cover a lot of potential uses.
>
> The one thing still open for discussion is whether '*' matches a
> single level of depth, or any number of path levels. ie. does /*/trunk
> match /project/trunk *and* /foo/bar/baz/trunk, or just the former?
>
> What do you think about the intended behaviour, and about the '*'
> matching question?

I've been thinking of something slightly different.  I'm not sure we
can just add wildcards to the section names where we currently store
the paths in question since people might actually have paths that
contain *'s.  What I was thinking of though was something like this:

[wildcards]
/*/*/trunk = star-star-trunk
/*/*/tags/* = star-star-tags-star
/*/*/branches/* = star-star-branches-star

[star-star-trunk]
* = rx

[star-star-tags-star]
* = rx

[star-star-branches-star]
* = rx

[/]
* = r

[/project]
@project = rwx

[/private]
* =
@special = rwx

We have a special section (like [groups]) that holds a list of the
wildcards in question, and those point to other specific sections that
hold the list of rules for each wildcard.

As for the "what can a * match?" question, I was leaning towards a *
matches a single path segment.  If we do that, we can also optimize
based on the number of segments in the path, no reason to check
against a given wildcard if it matches a different number.  Not sure
if that'd help noticably though.

I was only intending to implement * wildcards, nothing more than that.

I'm not sure about the intended behavior though.  If we do things that
way we basically need to ensure that the topmost rules in an authz
file are wildcards, otherwise a rule on [/] will always trump them. 
So my example would have to be rewritten a bit with a /* wildcard
instead of a [/] rule.  Not that it's an especially horrible idea,
just a little different from what I'd hoped for.  Not that I've come
up with an exact set of rules for applying the wildcards that would
work with my proposed config mind you, it was just something I threw
together while watching TV last night.

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] prototype replay authz checks

[David Anderson <da...@calixo.net>]

* Garrett Rooney <ro...@electricjellyfish.net> [2006-04-27 22:00:45]:
> To get this kind of behavior we either need to have nonrecursive authz
> permissions, which means each and every path that you want to block
> checkout of needs to be tagged in the authz conf file, which would
> really suck, or we need wildcards of some sort.  Adding wildcard
> support without having it devolve into "ok, now we enumerate all
> sections in the file and see if any of them match" seems like it will
> require some thought...
>
> So before I dive into the whole "how should wildcards work" mess any
> further than I already have, has anyone thought about that sort of
> thing yet?  Anyone got a design proposal sitting around that they
> haven't sent out yet?

There is a proposal for wildcards around, yes.  The gist of it is that
when the authz code loads, it reads and scans through the whole authz
file once.  So, we add a check that sees if there are wildcard chars
anywhere in the configured paths.  If so, we remember that there were.

Then, during a nonrecursive authz lookup, we first do the usual
dereferencing attempts of exact matches.  If those return no
conclusive answer, and if we found wildcards during the initial scan,
then we do a full sweep to attempt to match on wildcard terms.  For
recursive authz lookups, do the normal full sweep, with extra wildcard
handling if necessary.

Yeah, it remains slow and horrible, but I don't really see a way that
we can shy away from that (special wildcard lookup hash map built
during init? Is it worth it?), and at least that way you only get the
performance hit if you use the wildcards.

Exact matches have priority over wildcard matches, and the proposal is
to implement only the * wildcard, no ? or PCRE matching.  That should
cover a lot of potential uses.

The one thing still open for discussion is whether '*' matches a
single level of depth, or any number of path levels. ie. does /*/trunk
match /project/trunk *and* /foo/bar/baz/trunk, or just the former?

What do you think about the intended behaviour, and about the '*'
matching question?

- Dave.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] prototype replay authz checks

[Danny van Heumen <da...@hccnet.nl>]

Garrett Rooney wrote:
> To get this kind of behavior we either need to have nonrecursive authz
> permissions, which means each and every path that you want to block
> checkout of needs to be tagged in the authz conf file, which would
> really suck, or we need wildcards of some sort.  Adding wildcard
> support without having it devolve into "ok, now we enumerate all
> sections in the file and see if any of them match" seems like it will
> require some thought...
> 
> So before I dive into the whole "how should wildcards work" mess any
> further than I already have, has anyone thought about that sort of
> thing yet?  Anyone got a design proposal sitting around that they
> haven't sent out yet?
> 
> -garrett

Couldn't we just work with an attribute that indicates that the current
setting is nonrecursive. For example use the - to indicate a
non-recursive attribute.

BTW i noticed the use of the 'x' for checkout rights (is this correct?)
but this is a slightly different meaning than the x in a unix/linux
environment indicating whether or not it's allowed to be accessed. And
because in the svnserve-authz code there was a hint about implementing
an 'x' "under water" it might be a better idea to use an 'o' for
check-Out or some other character.

Example where I will indicate that a certain directory is read-only,
because you don't want to disturb this directory structure:

dir-structure:
/
/project/
/project/trunk/
/project/branches/
/project/tags/

authz-file:
('-' means that this attribute is non-recursive and is part of both r and x)
--start--
[/]
* = rwx

[/project]
* = rx-
--end--

First part:
everyone gets read+write access to the repository.

Second part:
indicates that in the project-directory (and only this directory)
everyone gets read-only access.

When someone wants to write to '/project/trunk' the non-recursive will
just be ignored when it's on a parent folder, so:
[/]
* = rwx

will indicate that the user has write-access on the folder.

This could work well together with Garrett's proposition for wildcards
so a user can choose the most efficient method for indicating special
access situations.

Danny


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] prototype replay authz checks

[Garrett Rooney <ro...@electricjellyfish.net>]

On 4/25/06, Greg Hudson <gh...@mit.edu> wrote:
> On Tue, 2006-04-25 at 14:56 -0700, Garrett Rooney wrote:
> > I figured getting something out there for replay alone was better than
> > not getting anything out at all.
>
> Disagree.  There is already code out there, in wide use, to synthesize
> replay using less efficient operations.

Ok, I can buy that argument.  I've played around with versions of this
that block update style requests, and I'm pretty sure I can make it at
least sort of work (the generality of update-report is totally
screwing us here, because it's going to be really hard to block
checkout/update/switch without blocking things that are actually
useful to allow, like diff, but whatever, I can make something work),
but I've hit a usability snag.  I think in order for this stuff to be
useful we're really going to need wildcards in paths in authz config
files.

For example, say you're the ASF Subversion repository.  You'd really
like to block checkouts of anything from / to the root of any given
project.  So it's invalid to check out /, /apr, /apr/apr-util,
/apr/apr-util/tags or /apr/apr-util/branches but not
/apr/apr-util/trunk or anything under /apr/apr-util/tags and
/apr/apr-util/branches.

To get this kind of behavior we either need to have nonrecursive authz
permissions, which means each and every path that you want to block
checkout of needs to be tagged in the authz conf file, which would
really suck, or we need wildcards of some sort.  Adding wildcard
support without having it devolve into "ok, now we enumerate all
sections in the file and see if any of them match" seems like it will
require some thought...

So before I dive into the whole "how should wildcards work" mess any
further than I already have, has anyone thought about that sort of
thing yet?  Anyone got a design proposal sitting around that they
haven't sent out yet?

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] prototype replay authz checks

[Greg Hudson <gh...@MIT.EDU>]

On Tue, 2006-04-25 at 14:56 -0700, Garrett Rooney wrote:
> I figured getting something out there for replay alone was better than
> not getting anything out at all.

Disagree.  There is already code out there, in wide use, to synthesize
replay using less efficient operations.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org