Re: [PATCH] Fix for issue 2753 "SVNListParentPath feature doesn't work when svn authz is used."
On 04/08/2010 09:23 PM, Philip Martin wrote:
Kamesh Jayachandran writes:
[issue2753] Fix issue 2753.
Let me see if I understand: The issue is that when SVNListParentPath
and AuthzSVNAccessFile are configured then GET requests for the parent
path get passed through the authz stuff. This is a bug because the
authz file doesn't control parent path.
Your patch recognises this request and avoids doing the authz check.
Yes, exactly.
Relax requests aimed at the repo Parent path from authz control.
* subversion/mod_authz_svn/mod_authz_svn.c
(req_check_access): When canonicalized 'uri' and 'root_path' are same
allow the request.
]]]
If there are no objections will commit this in next couple of days.
Thanks
With regards
Kamesh Jayachandran
Index: subversion/mod_authz_svn/mod_authz_svn.c
===
--- subversion/mod_authz_svn/mod_authz_svn.c(revision 931820)
+++ subversion/mod_authz_svn/mod_authz_svn.c(working copy)
@@ -210,6 +210,8 @@
svn_authz_t *access_conf = NULL;
svn_error_t *svn_err;
char errbuf[256];
+ const char *canonicalized_uri;
+ const char *canonicalized_root_path;
const char *username_to_authorize = get_username_to_authorize(r, conf);
switch (r->method_number)
@@ -249,6 +251,15 @@
break;
}
+ canonicalized_uri = svn_uri_canonicalize(r->uri, r->pool);
+ canonicalized_root_path = svn_uri_canonicalize(conf->base_path, r->pool);
Can conf->base_path be canonicalised once in
create_authz_svn_dir_config rather than for every request?
Yes done. Attached patch has this.
+ if (strcmp(canonicalized_uri, canonicalized_root_path) == 0)
+{
+ /*Do no access control when root_path(as configured in) and
+ given uri are same.*/
+ return OK;
+}
What happens if SVNParentPath is not being used? Is base_path is the
root of the repository? Does this disable authz on the root of that
repository? Perhaps you should be checking dav_svn__get_list_parentpath?
As said in my other email this works without any issue, the comment
added in the patch details the same.
I think this check would make more sense in access_checker rather than
req_check_access.
May be, I prefer req_check_access as there are 3 callers for this function.
The code needs a comment to say why no access control is neccessary in
this case.
Check my new comment in the attached patch.
The attached patch fixes one segfault(introduced in my earlier patch) also.
With regards
Kamesh Jayachandran
Index: subversion/mod_authz_svn/mod_authz_svn.c
===
--- subversion/mod_authz_svn/mod_authz_svn.c(revision 932331)
+++ subversion/mod_authz_svn/mod_authz_svn.c(working copy)
@@ -66,6 +66,9 @@
authz_svn_config_rec *conf = apr_pcalloc(p, sizeof(*conf));
conf->base_path = d;
+ if (d)
+conf->base_path = svn_uri_canonicalize(d, p);
+
/* By default keep the fortress secure */
conf->authoritative = 1;
conf->anonymous = 1;
@@ -210,6 +213,7 @@
svn_authz_t *access_conf = NULL;
svn_error_t *svn_err;
char errbuf[256];
+ const char *canonicalized_uri;
const char *username_to_authorize = get_username_to_authorize(r, conf);
switch (r->method_number)
@@ -249,6 +253,22 @@
break;
}
+ canonicalized_uri = svn_uri_canonicalize(r->uri, r->pool);
+ if (strcmp(canonicalized_uri, conf->base_path) == 0)
+{
+ /* Do no access control when conf->base_path(as configured in )
+ * and given uri are same. The reason for such relaxation of access
+ * control is "This module is meant to control access inside the
+ * repository path, in this case inside PATH is empty and hence
+ * dav_svn_split_uri fails saying no repository name present".
+ * One may ask it will allow access to '/' inside the repository if
+ * repository is served via SVNPath instead of SVNParentPath.
+ * It does not, The other methods(PROPFIND, MKACTIVITY) for
+ * accomplishing the operation takes care of making a request to
+ * proper URL */
+ return OK;
+}
+
dav_err = dav_svn_split_uri(r,
r->uri,
conf->base_path,
@@ -554,7 +574,7 @@
{
authz_svn_config_rec *conf = ap_get_module_config(r->per_dir_config,
&authz_svn_module);
- const char *repos_path;
+ const char *repos_path = NULL;
const char *dest_repos_path = NULL;
int status;
@@ -611,7 +631,7 @@
{
authz_svn_config_rec *conf = ap_get_module_config(r->per_dir_config,
&authz_svn_module);
- const char *repos_path;
+ const char *repos_path = NULL;
const char *dest_repos_path = NULL;
int status;
@@ -639,7 +659,7 @@
{
authz_svn_config_rec *conf = ap_get_module_config(r->per_dir_con
RE: [PATCH] Fix for issue 2753 "SVNListParentPath feature doesn't work when svn authz is used."
Thanks for the review Philip.
>Let me see if I understand: The issue is that when SVNListParentPath
>and AuthzSVNAccessFile are configured then GET requests for the parent
>path get passed through the authz stuff. This is a bug because the
>authz file doesn't control parent path.
>Your patch recognises this request and avoids doing the authz check.
Yes, exactly.
>> + canonicalized_uri = svn_uri_canonicalize(r->uri, r->pool);
>> + canonicalized_root_path = svn_uri_canonicalize(conf->base_path, r->pool);
>Can conf->base_path be canonicalised once in
>create_authz_svn_dir_config rather than for every request?
Yes should be, Will update my patch to handle this.
>> + if (strcmp(canonicalized_uri, canonicalized_root_path) == 0)
>> +{
>> + /*Do no access control when root_path(as configured in )
>> and
>> + given uri are same.*/
>> + return OK;
>> +}
>What happens if SVNParentPath is not being used? Is base_path is the
>root of the repository? Does this disable authz on the root of that
>repository? Perhaps you should be checking dav_svn__get_list_parentpath?
I tested this
$svn co http://localhost/svn <-- Repo itself instead of parent of repositories.
$cd svn
$svn ps 'a' 'b' .
$svn ci -m "commit" <-This worked as per the authz rules. Anyway will do the
directory/file creations to check in case!.
>I think this check would make more sense in access_checker rather than
>req_check_access.
Let me see and do if needed.
>The code needs a comment to say why no access control is neccessary in
>this case.
Will update the comment.
With regards
Kamesh Jayachandran
Re: [PATCH] Fix for issue 2753 "SVNListParentPath feature doesn't work when svn authz is used."
Kamesh Jayachandran writes:
> [issue2753] Fix issue 2753.
Let me see if I understand: The issue is that when SVNListParentPath
and AuthzSVNAccessFile are configured then GET requests for the parent
path get passed through the authz stuff. This is a bug because the
authz file doesn't control parent path.
Your patch recognises this request and avoids doing the authz check.
> Relax requests aimed at the repo Parent path from authz control.
>
> * subversion/mod_authz_svn/mod_authz_svn.c
> (req_check_access): When canonicalized 'uri' and 'root_path' are same
>allow the request.
> ]]]
>
> If there are no objections will commit this in next couple of days.
>
> Thanks
> With regards
> Kamesh Jayachandran
>
> Index: subversion/mod_authz_svn/mod_authz_svn.c
> ===
> --- subversion/mod_authz_svn/mod_authz_svn.c (revision 931820)
> +++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
> @@ -210,6 +210,8 @@
>svn_authz_t *access_conf = NULL;
>svn_error_t *svn_err;
>char errbuf[256];
> + const char *canonicalized_uri;
> + const char *canonicalized_root_path;
>const char *username_to_authorize = get_username_to_authorize(r, conf);
>
>switch (r->method_number)
> @@ -249,6 +251,15 @@
> break;
> }
>
> + canonicalized_uri = svn_uri_canonicalize(r->uri, r->pool);
> + canonicalized_root_path = svn_uri_canonicalize(conf->base_path, r->pool);
Can conf->base_path be canonicalised once in
create_authz_svn_dir_config rather than for every request?
> + if (strcmp(canonicalized_uri, canonicalized_root_path) == 0)
> +{
> + /*Do no access control when root_path(as configured in ) and
> + given uri are same.*/
> + return OK;
> +}
What happens if SVNParentPath is not being used? Is base_path is the
root of the repository? Does this disable authz on the root of that
repository? Perhaps you should be checking dav_svn__get_list_parentpath?
I think this check would make more sense in access_checker rather than
req_check_access.
The code needs a comment to say why no access control is neccessary in
this case.
> +
>dav_err = dav_svn_split_uri(r,
>r->uri,
>conf->base_path,
--
Philip
[PATCH] Fix for issue 2753 "SVNListParentPath feature doesn't work when svn authz is used."
Hi All,
Attached patch fixes issue 2753.
Quick description of 2753.
DAV svn
SVNParentPath /repositories
AuthType Basic
AuthName "My SVN"
AuthUserFile /etc/httpd/conf.d/users
allow from all
AuthzSVNAccessFile /etc/httpd/conf.d/svn_access_file
With the above configuration 'wget http://localhost/svn' gets 403 Access
forbidden.
Thrown from the following stack trace.
mod_dav_svn/repos.c:dav_svn_split_uri() <-- This function throws this
403 logging the following in the error_log
"The URI does not contain the name "
"of a repository.");
mod_authz_svn:req_check_access()
mod_authz_svn:access_checker()
The suggested work around for this issue is to define a with
a trailing slash i.e
Why this work around works?
Whatever that is defined in the or is
passed as is in the variable name 'root_path'.
dav_svn_split_uri() always removes the trailing slash of the uri.
So uri becomes '/svn' and root_path becomes '/svn' or '/svn/' based on
how it is configured in the Location block.
In the work around case
relative = ap_stripprefix("/svn", "/svn/"); //relative becomes '/svn'
and hence passes rest of the code path without error.
While 'relative' becomes empty string for ap_stripprefix("/svn", "/svn")
and hence this 403.
About the fix:
Fix is to 'relax' mod_authz_svn for 'requests' that are for the repo parent.
I tested the following cases with this patch:
With the restrictive(read-only) authz, tried to set prop on the '/' of
the repo(configured to serve via SVNPath), it failed as expected.
Ran through the testsuite, It did not break any new tests.
[[[
[issue2753] Fix issue 2753.
Relax requests aimed at the repo Parent path from authz control.
* subversion/mod_authz_svn/mod_authz_svn.c
(req_check_access): When canonicalized 'uri' and 'root_path' are same
allow the request.
]]]
If there are no objections will commit this in next couple of days.
Thanks
With regards
Kamesh Jayachandran
Index: subversion/mod_authz_svn/mod_authz_svn.c
===
--- subversion/mod_authz_svn/mod_authz_svn.c(revision 931820)
+++ subversion/mod_authz_svn/mod_authz_svn.c(working copy)
@@ -210,6 +210,8 @@
svn_authz_t *access_conf = NULL;
svn_error_t *svn_err;
char errbuf[256];
+ const char *canonicalized_uri;
+ const char *canonicalized_root_path;
const char *username_to_authorize = get_username_to_authorize(r, conf);
switch (r->method_number)
@@ -249,6 +251,15 @@
break;
}
+ canonicalized_uri = svn_uri_canonicalize(r->uri, r->pool);
+ canonicalized_root_path = svn_uri_canonicalize(conf->base_path, r->pool);
+ if (strcmp(canonicalized_uri, canonicalized_root_path) == 0)
+{
+ /*Do no access control when root_path(as configured in ) and
+ given uri are same.*/
+ return OK;
+}
+
dav_err = dav_svn_split_uri(r,
r->uri,
conf->base_path,
