You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Anderson <da...@calixo.net> on 2005/06/29 14:32:52 UTC
SoC: Path-based authz for Svnserve
Hi,
This summer, I'll be implementing path-based access control in svnserve,
as my SoC project. The following is my more detailed thoughts on this
(dare I say an implementation plan?). If you fancy a reading with some
formatting, you can read the same info on the wiki page I've set up for
my personal bookkeeping:
https://ssl.natulte.net/wiki/dave/wakka.php?wiki=SummerOfCode
====Objective====
The objective of my task is to implement path-based access control
(authz) in the standalone Subversion server. Currently, path-based
access control only works with the HTTP Repository Access method,
through the use of mod_dav_svn and mod_authz_svn in apache2. This makes
svnserve look bad, because it offers only blanket access control (all or
nothing).
I'll consider this task complete when both mod_dav_svn and svnserve can
grant or deny access to filesystem resources based on the same format of
ACL file, with as little code replication in various modules as
possible. If I finish before my SoC time is up (which I certainly
hope!), I'll move on to the enhancements part of the plan, and/or onto
other issues of svnserve.
====A summary of questions for the busy people====
- Is having a function in libsvn_repos that doesn't actually use a
svn_repos_t acceptable, or should the authz routines move to another lib?
- How do we cache the authz file (svn_config_t) in svnserve? Keep it
loaded and reload on SIGUSR1 or similar?
- What enhancements to the authz file format are badly needed by the
community?
- Is caching authz info in svnserve a stupid idea (see detailed
proposal) ?
- (for those who've had the courage to walk the rest of the proposal)
Are there points to which ou object or on which you have further
notes/comments/observations/flames ?
====Subprojects====
Getting access control into both mod_dav_svn and svnserve will require
somewhat redesigning and shifting of the current authz code.
===Authz utility routines in libsvn_repos===
The simplest way to allow both mod_dav_svn and svnserve to access authz
files in an identical fashion is to move the code that does the parsing
and analysing of authz files to libsvn_repos. There are two
possibilities here:
- Either we decide that each repository has an authz file defined in
its configuration, in which case checking authz is simply a matter of
extracting the authz file path given the svn_repos_t; or
- We go with flexibility and provide a routine that opens an authz
file given its full path, and let mod_dav_svn and svnserve decide for
themselves what path should be opened.
I can see the unifying merits of the first solution, but I'd go with
opening a path rather than systematically going via a svn_repos_t, for
both its flexibility and lesser disruptiveness to the way mod_dav_svn
currently works. This however makes the function not use a svn_repos_t.
Is this acceptable for a function in libsvn_repos?
In libsvn_repos, we basically want a single public routine that checks a
path and requested access level against the ACL definition file. There
will be a few internal functions to walk the authz file and parse the
sections to work out the authorization. No need to provide a function to
locate the repository authz file, as one can expect svnserve to be
capable of parsing its own config file to work this out. mod_dav_svn
already locates its own and caches the info, so no problem there. The
public interface changes therefore sum up to:
####
// In include/svn_repos.h
/**
* Check wether @a user can access @a path in the repository @a
* repos_name with the @a required_credentials. The access control
* lists are supplied in @a cfg, and the updated access mask is
* returned in @a *granted_access.
*
*/
svn_error_t *
svn_repos_authz_check_access(svn_config_t *cfg, const char *repos_name,
const char *path, const char *user;
int required_access, int *granted_access,
apr_pool_t *pool);
####
===Alter mod_dav_svn to use the new routines===
The foremost problem as I see it that the AuthzSVNAuthoritative
configuration directive that can be set for mod_dav_svn/mod_authz_svn
wouldn't be usable if the authz is delegated to libsvn_repos, as it
relies on other apache2 modules. This would mean keeping mod_authz_svn
with the authz hooks and URI checking code, so that mod_dav_svn can
still delegate access control to some extent. So, actually, no problem
at all :-) Just cut in the libsvn_repos routines instead of the
mod_authz_svn internal ones that are currently used, and Zark's your
brother.
===Alter the libsvn_repos commit editor to hook write operations===
The mod_dav_svn RA method doesn't use libsvn_repos' commit editor, so
there are no authz hooks on write operations. Svnserve will need these,
so we need to add them (possibly reviewing function and callback
prototypes and propagating change accordingly).
===Write the authz code in svnserve===
The code to write is split in two kinds:
- The authz callback code, which is basically glue between the commit
editor and the authz utility routines, and
- The code to call authz routines directly for certain operations
which do not go through the commit editor.
On a side note, it seems mod_authz_svn currently uses Apache/APR to
cache the authz config file, using apr_pool_userdata_set
(mod_authz_svn.c:485). This doesn't give much in the way of long term
caching (the lifetime of a few requests), but this is normal given
mod_dav_svn's mode of operation. Svnserve on the other hand is
longer-lived, and could probably load and cache the content of the authz
file on a semi-permanent basis, to avoid the overhead of opening and
parsing the file. //Should// it do so? Introducing configuration caching
for authz files implies a reloading mechanism, via SIGUSR1 or a similar
notification mechanism.
===Expand the semantics of the authz file format===
If required, this refactoring could be the occasion to implement some
new behaviour for authz files. I currently don't have the need for
enhancements, as the authz in mod_authz_svn does exactly what I need.
However, if people out there badly need something, speak up, and we can
think about getting it into the authz code.
One suggestion was to be able to distinguish "Any **authenticated**
user" from "Any user at all, anonymous or authenticated", perhaps using
* and ** to distinguish them. However, this seemed to pose some
technical problems, as outlined by Greg Hudson in (
http://svn.haxx.se/dev/archive-2005-05/0043.shtml ). I don't yet have
sufficient grasp of the mechanism of updates to understand his argument,
if someone could expand on this...
===Optimize the operation of authz code===
Currently and as far as I know, authz has a somewhat bad reputation of
slowing down access to the repository, because of all the path checks
that need to be performed. In the immediate sense, I don't really see
how this could be bettered. There may be a little optimisation possible
when the caller requests a recursive access lookup, but the speed impact
will probably stay.
Another possibility I've been loosely thinking about is adding a flag to
what the authz routine can return in the way of granted access,
something along the lines of a AUTHZ_BLANKET. The idea being that, even
when svnserve doesn't request recursive access, the authz code can
return a flag saying "Oh and by the way, if you fancy you can access
anything below this with those rights as well". The point being that if
svnserve is made aware of that flag, it can build itself a cache lookup
dictionary, with paths as keys and user/access rights as values. Any
path that falls within one of the cached paths and whose requested
rights are less or equal to the cached ones is granted access without
diving into the authz parsing code.
To clarify this, I'll take an example. Say you're running the greek tree
repository in svnserve with authz access control. An anonymous user
request triggers an authz lookup for READ access on /A for the anonymous
user. The authz code mulls over the ACL file, and decides that not only
does the anonymous user have READ access to /A, but that there are no
other ACL rules denying him read access for paths within /A . So the
authz routine returns the granted rights (READ & BLANKET). Svnserve
notices the BLANKET flag is set, and sets a cache entry: "*:/A" -> "r".
It then goes on to reply to the client as it normally would.
Then, another client (or the same one come to that) requests anonymous
READ access to /A/B/lambda . Before invoking the authz function which
checks this against the ACL file, svnserve checks wether the requested
user:path exists in the cache dictionary. In this case, *:/A exists,
which is a parent path for the requested path. Svnserve then looks up
the cached rights, and sees that the authz has already granted blanket
read access to everything contained in /A for the anonymous user. So it
returns, successfully granting READ access without making authz reparse
the whole ACL file.
Now, another request comes in which triggers an authz request for WRITE
access to /A/mu for the anonymous user. Svnserve looks up its cache,
sees that it has a cached access rule for *:/A. However, the value only
indicates that blanket READ access was granted; nothing is known about
write access. So, svnserve calls the authz routines, which parse the ACL
file and decide in the usual way. On the way back up, if the authz
routine has granted blanket access, add (or modify) an entry for that
path, adding blanket write to the cached rights.
Please note this is off the top of my head for the time being. Even now
I can see a few problems with this:
- Each and every request to the authz routines will trigger an
analysis of the complete authz file, in order to work out wether blanket
access can be granted or not. This can probably be avoided by having
svnserve specify when it wants to have blanket access looked up, so that
it can be disabled and alltogether bypassed by mod_dav_svn, which has no
possibility I know of of caching such information durably. Actually, the
caching behaviour can probably be obtained by forcing the RECURSIVE flag
for all authz requests made by svnserve, so that "blanket" access to
subdirectories is automagically checked. That way, no extra code in the
authz routines, only in svnserve.
- Caching adds another layer of information storage, which will need
to be looked after by svnserve and flushed when a reload is requested.
- The speedup I'm hoping for (because of the relative cost of a few
string comparisons and lookups vs. parsing a potentially large file)
might not actually happen. My intuition is that there would probably be
some speedup, but is it worth it?
- As I said earlier, mod_dav_svn and friends don't have, as far as I
know, a way of durably caching data over several requests. Therefore the
benefits of the cache speedup would be lost for the HTTP RA method. Is
it desirable to give such an advantage to one RA method over the other?
====Conclusion====
That's all I can think of now. As you can probably see, the grey area
for me is the commit editor and adding of hooks. I haven't explored this
much yet, and so only have a vague idea of how exactly to go about it.
I'll be looking into that later today, and once I've somewhat cleared up
my doubts I'll start with the coding... Assuming the plan I'm outlining
is satisfactory of course.
Thanks to those still reading this. :-)
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: SoC: Path-based authz for Svnserve
Posted by David Anderson <da...@calixo.net>.
Garrett Rooney wrote:
> I'd rather you just stat the file to see if it's been modified or
> something like that, requiring the user to send a signal to the process
> seems rather error prone.
It is error prone. I'll go with what Greg suggests: reread the file on
each new client connection.
> Some way to indicate that a part of the tree can be accessed by a user
> that is not logged in. I don't think mod_authz_svn can do that, but
> svnserve might want to.
Is this more badly needed than wildcards on paths? It seems those two
enhancements are mutually exclusive, or at least having both involves
some complexities.
> I'd say go without it for the first pass, just reading it in at the
> start of a session, then after you've got it working look at how much of
> a speed improvement there is when caching is added.
As Greg said, there is the risk of any caching I add ending up being
slower than the actual parsing. It'll all depend on how fast the parsing
into hierarchical hashes makes the subsequent analysis. Anyhow, I'll
look this up after all the rest is done, so there is time.
If all goes well, the first patch moving the authz routines into
libsvn_repos and updating mod_authz_svn accordingly should be posted
within a few days (but of course all will not go well ;).
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: SoC: Path-based authz for Svnserve
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
David Anderson wrote:
> ====A summary of questions for the busy people====
>
> - Is having a function in libsvn_repos that doesn't actually use a
> svn_repos_t acceptable, or should the authz routines move to another lib?
I don't see any especially pressing reason to move it to another lib.
> - How do we cache the authz file (svn_config_t) in svnserve? Keep it
> loaded and reload on SIGUSR1 or similar?
I'd rather you just stat the file to see if it's been modified or
something like that, requiring the user to send a signal to the process
seems rather error prone.
> - What enhancements to the authz file format are badly needed by the
> community?
Some way to indicate that a part of the tree can be accessed by a user
that is not logged in. I don't think mod_authz_svn can do that, but
svnserve might want to.
> - Is caching authz info in svnserve a stupid idea (see detailed
> proposal) ?
I'd say go without it for the first pass, just reading it in at the
start of a session, then after you've got it working look at how much of
a speed improvement there is when caching is added.
> - (for those who've had the courage to walk the rest of the proposal)
> Are there points to which ou object or on which you have further
> notes/comments/observations/flames ?
Seems reasonable to me, although I did only give it a quick read-through.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: SoC: Path-based authz for Svnserve
Posted by Greg Hudson <gh...@MIT.EDU>.
On Wed, 2005-06-29 at 22:49 +0200, David Anderson wrote:
> Okay. Would it be interesting to work in in a hybrid solution, with a
> standard way to locate the ACL file for a repository, but give the httpd
> and svnserve a way to override it?
Probably not; I think it opens up a sizeable can of worms, and doesn't
ultimately benefit users much.
> It is the policy afaik. I'll correct the prototype for my first patch.
> Should the possible values be documented in the function prototype or at
> the enum definition, or both?
The enum definition should be sufficient.
> I'll update my wiki page to sync it with this reply. If there are no
> other comments/objections, can I consider this tentatively approved and
> start implementing?
I think you're definitely at that point, yes.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: SoC: Path-based authz for Svnserve
Posted by David Anderson <da...@calixo.net>.
> but the die has been cast; HTTP servers find the access control file
> using HTTP configuration, and we can't reasonably change that.
Okay. Would it be interesting to work in in a hybrid solution, with a
standard way to locate the ACL file for a repository, but give the httpd
and svnserve a way to override it?
Do this by keeping the interface I gave, but adding a function that
takes an svn_repos_t and returns the path to the standard ACL file. That
way, RA servers can either use the override path if it is given, or
locate the standard path and open that otherwise.
Wether this is interesting depends on if we want the location of the ACL
file to be standardised, eventually, even if this means going through a
transition.
> file is going to be different for different repositories, and that we
> already read svnserve.conf once per client connection.
Okay, I'll go with that. I was still reasoning in the perspective of my
setup, which has an http SVNParentPath and a common ACL file for all
repositories.
> I think you mean mod_authz_svn here.
Oops, yes. s/mod_dav_svn/mod_authz_svn/ where appropriate. I shouldn't
be touching mod_dav_svn.
> No documentation of required_access and granted_access, and I'd think
> those should be enums, not bare ints. (I believe it's our style to use
> enums rather than ints and #defines.)
It is the policy afaik. I'll correct the prototype for my first patch.
Should the possible values be documented in the function prototype or at
the enum definition, or both?
> I think you mean mod_authz_svn here.
See above. My bad.
> svnserve handles much larger-grain operations than mod_dav_svn, so
> caching the authz file for the lifetime of a connection should yield
> acceptable performance.
Okay, I'll go with that.
> One option would be to look at the authz file at the beginning of the
> checkout and say "Aha! I see a path entry for trunk/foo/bar, which is
> read-protected. I bet I will run into that at some point during the
> checkout. I'd better challenge the client for authentication now."
> However, this can't really work if there's wildcard support in the authz
> file.
I see. Thanks for clearing that up. So, do people want wildcard support
or "intelligent" challenge support?
> I think most of the slowness comes from HTTP overhead, and not the
> actual lookup. I'd wait until someone measures a performance problem
> before worrying about this.
Okay.
> The authz code doesn't have to *parse* anything to answer the question;
> the authz file was loaded and parsed into a nested set of hashes at
> connection initiation time. I think if you're not careful, you'll wind
> up designing a cache which is precisely as slow as the libsvn_repos
> function you'd be calling on a cache miss.
Good point. If the parsing already works the file's contents into hashes
that are sufficiently optimal, no point in repeating the structure at a
higher level. Anyhow, I'll start with getting the actual authz in there.
Optimizations and stuff can wait until then.
I'll update my wiki page to sync it with this reply. If there are no
other comments/objections, can I consider this tentatively approved and
start implementing?
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: SoC: Path-based authz for Svnserve
Posted by Greg Hudson <gh...@MIT.EDU>.
On Wed, 2005-06-29 at 16:32 +0200, David Anderson wrote:
> - Is having a function in libsvn_repos that doesn't actually use a
> svn_repos_t acceptable, or should the authz routines move to another lib?
That's fine. Perhaps if mod_authz_svn didn't exist and we were doing
this from scratch, we'd have the libsvn_repos function take an
svn_repos_t and have a single way of finding the access control file,
but the die has been cast; HTTP servers find the access control file
using HTTP configuration, and we can't reasonably change that.
> - How do we cache the authz file (svn_config_t) in svnserve? Keep it
> loaded and reload on SIGUSR1 or similar?
I'd read it once per client connection. The structure of svnserve makes
that the easiest solution, I think. Remember that the access control
file is going to be different for different repositories, and that we
already read svnserve.conf once per client connection.
> mod_dav_svn
> already locates its own and caches the info, so no problem there.
I think you mean mod_authz_svn here.
> /**
> * Check wether @a user can access @a path in the repository @a
> * repos_name with the @a required_credentials. The access control
> * lists are supplied in @a cfg, and the updated access mask is
> * returned in @a *granted_access.
> *
> */
> svn_error_t *
> svn_repos_authz_check_access(svn_config_t *cfg, const char *repos_name,
> const char *path, const char *user;
> int required_access, int *granted_access,
> apr_pool_t *pool);
> ####
No documentation of required_access and granted_access, and I'd think
those should be enums, not bare ints. (I believe it's our style to use
enums rather than ints and #defines.)
> ===Alter mod_dav_svn to use the new routines===
I think you mean mod_authz_svn here. mod_dav_svn does not truck in
authorization files; it delegates authorization back to httpd. I don't
see any compelling reason to change that. I think you arrived at the
same conclusion in the subsequent paragraph, but your section title is
still weird.
> Svnserve on the other hand is
> longer-lived, and could probably load and cache the content of the authz
> file on a semi-permanent basis, to avoid the overhead of opening and
> parsing the file. //Should// it do so? Introducing configuration caching
> for authz files implies a reloading mechanism, via SIGUSR1 or a similar
> notification mechanism.
svnserve handles much larger-grain operations than mod_dav_svn, so
caching the authz file for the lifetime of a connection should yield
acceptable performance.
> One suggestion was to be able to distinguish "Any **authenticated**
> user" from "Any user at all, anonymous or authenticated", perhaps using
> * and ** to distinguish them. However, this seemed to pose some
> technical problems, as outlined by Greg Hudson in (
> http://svn.haxx.se/dev/archive-2005-05/0043.shtml ). I don't yet have
> sufficient grasp of the mechanism of updates to understand his argument,
> if someone could expand on this...
Update operations (as well as diff and switch and status operations)
involve a pipelined set of editing changes transmitted from the server
to the client. There is currently no room in the protocol for the
server to stop and challenge the client for authentication information.
This is true of both network RA layers and is designed to limit the
number of round trips involved in an update.
So, let's say I try to check out "trunk" and it turns out that, while
"trunk" is readable to anyone (authenticated or not), "trunk/foo/bar" is
read-protected. The way mod_dav_svn currently works, the server will
not challenge you for authentication when you get to trunk/foo/bar; it
will simply report an "absent" trunk/foo/bar directory and move on. As
I understand it, the normal workaround in DAV land is that if you want
to use read access controls, you make everyone authenticate, and you
create a guest user if you want anonymous read access to part of the
repository.
One option would be to look at the authz file at the beginning of the
checkout and say "Aha! I see a path entry for trunk/foo/bar, which is
read-protected. I bet I will run into that at some point during the
checkout. I'd better challenge the client for authentication now."
However, this can't really work if there's wildcard support in the authz
file.
> Currently and as far as I know, authz has a somewhat bad reputation of
> slowing down access to the repository, because of all the path checks
> that need to be performed.
I think most of the slowness comes from HTTP overhead, and not the
actual lookup. I'd wait until someone measures a performance problem
before worrying about this.
> The point being that if
> svnserve is made aware of that flag, it can build itself a cache lookup
> dictionary, with paths as keys and user/access rights as values. Any
> path that falls within one of the cached paths and whose requested
> rights are less or equal to the cached ones is granted access without
> diving into the authz parsing code.
The authz code doesn't have to *parse* anything to answer the question;
the authz file was loaded and parsed into a nested set of hashes at
connection initiation time. I think if you're not careful, you'll wind
up designing a cache which is precisely as slow as the libsvn_repos
function you'd be calling on a cache miss.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Should authz return errors? (Was: Re: SoC: Path-based authz for
Svnserve)
Posted by David Anderson <da...@calixo.net>.
kfogel@collab.net wrote:
> Well, unless there's some larger issue here I'm missing, this just
> sounds like a bug. Both circumstances should be detected, and return
> errors -- and, I mean, returning 'svn_error_t *' is the norm in our
> interfaces, so can it really be that much more cumbersome?
It's more cumbersome than the interface currently defined in
mod_authz_svn, which attempts to gracefully ignore the problems and
therefore doesn't need to handle errors in the authz parsing code.
That's what I meant by "more cumbersome" : svn_error_t's need to be
added to the interface. I didn't mean that I object to handling the
errors :)
So, a bug it is. It'll be corrected in my first patch then :-)
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Should authz return errors? (Was: Re: SoC: Path-based authz for Svnserve)
Posted by kf...@collab.net.
David Anderson <da...@calixo.net> writes:
> I'm working on the moving and adapting of the authz code to
> libsvn_repos, and during that, a question came up.
>
> The current code in mod_authz_svn doesn't use any error handling. In
> some cases the code runs into a malformed authz file, and when that
> happens, authz silently ignores what piece is problematic and ploughs
> on. AFAICT, there are two such cases:
> - Discovering groups with cyclic dependancies (group A contains group B
> contains group A); The second group will have the cycling dependancy
> silently ignored.
> - An ACL line for an undefined group; The undefined group is considered
> empty.
>
> While I can potentially understand why the second case is okay, the
> first seems like silently ignoring something which might bring a good
> deal of confusion to users who accidentally create a cyclic dependancy
> ("Why isn't user foo authorized? He's in group A, which group B
> includes...").
>
> So, this all comes down to: should the authz API return errors when it
> discovers a malformed ACL configuration? I'm undecided on this, because
> it changes the current behaviour (a working authz file might fail under
> the new code), and because returning svn_error_t's everywhere does
> contribute to making the interface more cumbersome.
>
> Thoughts?
Well, unless there's some larger issue here I'm missing, this just
sounds like a bug. Both circumstances should be detected, and return
errors -- and, I mean, returning 'svn_error_t *' is the norm in our
interfaces, so can it really be that much more cumbersome?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Should authz return errors? (Was: Re: SoC: Path-based authz for
Svnserve)
Posted by Joseph Galbraith <ga...@vandyke.com>.
Greg Hudson wrote:
> On Fri, 2005-07-01 at 14:13 +0200, David Anderson wrote:
>
>>So, this all comes down to: should the authz API return errors when it
>>discovers a malformed ACL configuration?
>
>
> While it's probably best to have an svn_error_t * return for the
> relevant functions, I think in practice, errors should be returned when
> the authz file is read, not when authz queries are made.
>
> That means the reading code has to go to a substantial amount of extra
> work validating everything, but:
>
> * This way you'll notice immediately if you screw up your authz file,
> instead of only noticing when you try to check out or commit to some
> obscure part of your repository.
>
> * This way will produce much more graceful failure than conking out in
> the middle of an update/commit.
While I'm not sure it is possible with this particular
file format, in general silently ignoring errors when
reading data that says who is and isn't allowed to access
data seems like a really bad idea.
(Does authz support a deny directive?)
What if because of the error bad_guy_jones now has access
to data the administrator thinks he has protected... and
the error was silently ignored so the administrator has no
idea that there is a problem.
Thanks,
Joseph
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Should authz return errors? (Was: Re: SoC: Path-based authz for Svnserve)
Posted by kf...@collab.net.
David Anderson <da...@calixo.net> writes:
> kfogel@collab.net wrote:
> > David, try to break things down into little, separate patches, please.
>
> I'm planning to. When I said "It'll be corrected in my first patch", I
> wasn't thinking. It'll be corrected in my second patch. The first one
> will move the authz code to libsvn_repos, without validation-on-open.
>
> As for little, I'll do my best, but some of these will be fairly
> large, no matter how hard I try. For example, I don't think you'll
> like a patch that adds code to libsvn_repos without removing the code
> that does the same thing from mod_authz_svn.
>
> Nevertheless, I'll try to keep changes as small as possible for review.
Oh, unifiedness takes precedence of smallness any day, don't worry
about that. "No bigger than necessary" is really the guideline, not
so much "as small as possible".
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Should authz return errors? (Was: Re: SoC: Path-based authz for
Svnserve)
Posted by David Anderson <da...@calixo.net>.
kfogel@collab.net wrote:
> David, try to break things down into little, separate patches, please.
I'm planning to. When I said "It'll be corrected in my first patch", I
wasn't thinking. It'll be corrected in my second patch. The first one
will move the authz code to libsvn_repos, without validation-on-open.
As for little, I'll do my best, but some of these will be fairly large,
no matter how hard I try. For example, I don't think you'll like a patch
that adds code to libsvn_repos without removing the code that does the
same thing from mod_authz_svn.
Nevertheless, I'll try to keep changes as small as possible for review.
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Should authz return errors? (Was: Re: SoC: Path-based authz for Svnserve)
Posted by kf...@collab.net.
Greg Hudson <gh...@MIT.EDU> writes:
> On Fri, 2005-07-01 at 14:13 +0200, David Anderson wrote:
> > So, this all comes down to: should the authz API return errors when it
> > discovers a malformed ACL configuration?
>
> While it's probably best to have an svn_error_t * return for the
> relevant functions, I think in practice, errors should be returned when
> the authz file is read, not when authz queries are made.
>
> That means the reading code has to go to a substantial amount of extra
> work validating everything, but:
>
> * This way you'll notice immediately if you screw up your authz file,
> instead of only noticing when you try to check out or commit to some
> obscure part of your repository.
>
> * This way will produce much more graceful failure than conking out in
> the middle of an update/commit.
That's what I thought I meant, sorry if my response didn't make that
clear.
David, try to break things down into little, separate patches, please.
For example, making the authz-config-file-reading detect errorful
situations should be its own patch. (You may have known this already,
I wasn't sure when you said "It'll be corrected in my first patch
then" if you meant that your first patch would be this change, or
would include this change. The former is desirable.)
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Should authz return errors? (Was: Re: SoC: Path-based authz for
Svnserve)
Posted by David Anderson <da...@calixo.net>.
kfogel@collab.net wrote:
> Why would validation be a separate function from loading? Why not
> just make the load function itself return error?
Because the authz file is loaded by the svn_config_* functions, which
load other files than just authz. So validation is not possible in those.
Unless you mean add an API function svn_repos_authz_open that opens the
svn_config_t and validates it? It mostly mounts to the same result, so
why not I guess.
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Should authz return errors? (Was: Re: SoC: Path-based
authz for Svnserve)
Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2005-07-01 at 13:08 -0500, kfogel@collab.net wrote:
> Why would validation be a separate function from loading? Why not
> just make the load function itself return error?
Right now the loading function is svn_config_read.
There's an argument for making a loading function which performs
svn_config_read and then the validation. I think I'd like that approach
better.
(There's even an argment for making authz files an abstract type,
instead of passing around just an svn_config_t *, but we don't generally
adopt that level of formalism.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Should authz return errors? (Was: Re: SoC: Path-based authz for Svnserve)
Posted by kf...@collab.net.
David Anderson <da...@calixo.net> writes:
> I propose adding a function to the authz API,
> svn_repos_authz_validate, which scans the entire given svn_config_t to
> ensure that it is a valid authz ACL file.
> The RA servers then call this as a part of their procedure for loading
> the authz file, and abort service if something comes up.
Why would validation be a separate function from loading? Why not
just make the load function itself return error?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Should authz return errors? (Was: Re: SoC: Path-based authz for
Svnserve)
Posted by David Anderson <da...@calixo.net>.
Greg Hudson wrote:
> That means the reading code has to go to a substantial amount of extra
> work validating everything, but:
I agree this is preferrable to breaking in the middle of an operation.
I propose adding a function to the authz API, svn_repos_authz_validate,
which scans the entire given svn_config_t to ensure that it is a valid
authz ACL file.
The RA servers then call this as a part of their procedure for loading
the authz file, and abort service if something comes up.
> * This way you'll notice immediately if you screw up your authz file,
> instead of only noticing when you try to check out or commit to some
> obscure part of your repository.
> * This way will produce much more graceful failure than conking out in
> the middle of an update/commit.
+1 for both arguments. Best have an operation fail immediately if the
server-side configuration is invalid.
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Should authz return errors? (Was: Re: SoC: Path-based authz
for Svnserve)
Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2005-07-01 at 14:13 +0200, David Anderson wrote:
> So, this all comes down to: should the authz API return errors when it
> discovers a malformed ACL configuration?
While it's probably best to have an svn_error_t * return for the
relevant functions, I think in practice, errors should be returned when
the authz file is read, not when authz queries are made.
That means the reading code has to go to a substantial amount of extra
work validating everything, but:
* This way you'll notice immediately if you screw up your authz file,
instead of only noticing when you try to check out or commit to some
obscure part of your repository.
* This way will produce much more graceful failure than conking out in
the middle of an update/commit.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Should authz return errors? (Was: Re: SoC: Path-based authz for Svnserve)
Posted by David Anderson <da...@calixo.net>.
Hi,
I'm working on the moving and adapting of the authz code to
libsvn_repos, and during that, a question came up.
The current code in mod_authz_svn doesn't use any error handling. In
some cases the code runs into a malformed authz file, and when that
happens, authz silently ignores what piece is problematic and ploughs
on. AFAICT, there are two such cases:
- Discovering groups with cyclic dependancies (group A contains group B
contains group A); The second group will have the cycling dependancy
silently ignored.
- An ACL line for an undefined group; The undefined group is considered
empty.
While I can potentially understand why the second case is okay, the
first seems like silently ignoring something which might bring a good
deal of confusion to users who accidentally create a cyclic dependancy
("Why isn't user foo authorized? He's in group A, which group B
includes...").
So, this all comes down to: should the authz API return errors when it
discovers a malformed ACL configuration? I'm undecided on this, because
it changes the current behaviour (a working authz file might fail under
the new code), and because returning svn_error_t's everywhere does
contribute to making the interface more cumbersome.
Thoughts?
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org