You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Hudson <gh...@MIT.EDU> on 2005/07/01 16:55:21 UTC
Re: Should authz return errors? (Was: Re: SoC: Path-based authz
for Svnserve)
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
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