You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2008/01/21 03:00:27 UTC
[PATCH + RFC] authz scenario on 'reintegrate' branch?
For way too long now, I've been trying to construct a scenario by
which I could test a patch I've written to address this "TODO(reint)"
comment in fs-wrap.c on the 'reintegrate' branch:
/* We consciously do not perform authz checks on the paths returned
in *MERGEINFO, avoiding massive authz overhead which would allow
us to protect the name of where a change was merged from, but not
the change itself. */
/* ### TODO(reint): ... but how about descendant merged-to paths? */
if (readable_paths->nelts > 0)
SVN_ERR(svn_fs_get_mergeinfo(mergeinfo, root, readable_paths, inherit,
include_descendants, pool));
Well, er, yes, how about descendant merged-to paths?
I could describe the zillion combinations of authz files and branch
maneuvers I've tried, hoping to stimulate the new code I've written
(see patch below). But that would take a lot of time, for probably
little benefit. Instead, can anyone *think* of a scenario by which
either of the two apr_hash_set() calls below might be reached? (Note
the debugging prints, in case you want to trace the process yourself.)
I ask knowing that I may be embarrassed by the obviousness of the
answer. However, spinning my wheels by myself has so far not proven
effective :-). When I try to construct scenarios, I can't ever make
one in which a pre-existing authz check (e.g. must_have_access() in
svnserve) doesn't fail first -- yet if some pre-existing check always
fails first, then we clearly don't need this patch.
Am I missing something?
-Karl
Index: subversion/libsvn_repos/fs-wrap.c
===================================================================
--- subversion/libsvn_repos/fs-wrap.c (revision 28964)
+++ subversion/libsvn_repos/fs-wrap.c (working copy)
@@ -23,9 +23,11 @@
#include "svn_error.h"
#include "svn_fs.h"
#include "svn_path.h"
+#include "svn_hash.h"
#include "svn_props.h"
#include "svn_repos.h"
#include "repos.h"
+#include "svn_mergeinfo.h"
#include "svn_private_config.h"
@@ -595,10 +597,143 @@
in *MERGEINFO, avoiding massive authz overhead which would allow
us to protect the name of where a change was merged from, but not
the change itself. */
- /* ### TODO(reint): ... but how about descendant merged-to paths? */
- if (readable_paths->nelts > 0)
- SVN_ERR(svn_fs_get_mergeinfo(mergeinfo, root, readable_paths, inherit,
- include_descendants, pool));
+ if (readable_paths->nelts > 0)
+ {
+ SVN_ERR(svn_fs_get_mergeinfo(mergeinfo, root, readable_paths, inherit,
+ include_descendants, pool));
+
+ if (include_descendants)
+ {
+ /* The mergeinfo might have paths that never got authz-checked;
+ if so, check them now. But only check the ones where we
+ can't know by examining 'paths' and 'readable_paths',
+ since an authz check is (relatively) expensive. */
+
+ apr_hash_index_t *hi;
+ apr_hash_t *original_paths_hash, *readable_paths_hash;
+
+#define KFF_DEBUG 1
+
+ if (KFF_DEBUG)
+ {
+ FILE *fp = fopen("/tmp/kff.out", "a");
+ if (fp == NULL)
+ exit(1);
+ fprintf(fp, "KFF: include_descendants is true\n");
+ fprintf(fp, "KFF: paths: '%s'\n",
+ svn_cstring_join((apr_array_header_t *) paths,
+ ", ", pool));
+ fprintf(fp, "KFF: readable_paths: '%s'\n",
+ svn_cstring_join((apr_array_header_t *) readable_paths,
+ ", ", pool));
+ fprintf(fp, "KFF: *mergeinfo count: %d\n",
+ apr_hash_count(*mergeinfo));
+ fclose(fp);
+ }
+
+ /* Make lookups easy by converting to hash. */
+ SVN_ERR(svn_hash_from_cstring_keys(&original_paths_hash,
+ paths, pool));
+ if (readable_paths == paths)
+ {
+ readable_paths_hash = original_paths_hash;
+ }
+ else
+ {
+ SVN_ERR(svn_hash_from_cstring_keys(&readable_paths_hash,
+ readable_paths, pool));
+ }
+
+ for (hi = apr_hash_first(pool, *mergeinfo); hi;
+ hi = apr_hash_next(hi))
+ {
+ const void *key;
+ void *val;
+ const char *path;
+ const char *raw_mergeinfo_string;
+ apr_array_header_t *mergeinfo_value_paths;
+ int j;
+
+ svn_pool_clear(iterpool);
+ apr_hash_this(hi, &key, NULL, &val);
+ path = key;
+ raw_mergeinfo_string = val;
+
+ if (KFF_DEBUG)
+ {
+ FILE *fp = fopen("/tmp/kff.out", "a");
+ if (fp == NULL)
+ exit(1);
+ fprintf(fp, "KFF: loop hit '%s'\n", path);
+ fprintf(fp, "KFF: mergeinfo for '%s':\n", path);
+ fprintf(fp, "'%s'\n",
+ (char *) apr_hash_get(*mergeinfo, path,
+ APR_HASH_KEY_STRING));
+ fprintf(fp, "KFF: (rp_h == op_h? --> %d)\n",
+ readable_paths_hash == original_paths_hash);
+ fclose(fp);
+ }
+
+ if (! apr_hash_get(readable_paths_hash, path,
+ APR_HASH_KEY_STRING))
+ {
+ if (KFF_DEBUG)
+ {
+ FILE *fp = fopen("/tmp/kff.out", "a");
+ if (fp == NULL)
+ exit(1);
+ fprintf(fp, "KFF: '%s' is not in readable_paths\n",
+ path);
+ fclose(fp);
+ }
+
+ /* It's not in readable_paths, so if it *is* in
+ the original paths, that means it was already
+ authz-checked and found unreadable -- in which
+ case remove it from the mergeinfo. */
+ if (readable_paths_hash != original_paths_hash
+ && apr_hash_get(original_paths_hash, path,
+ APR_HASH_KEY_STRING))
+ {
+ if (KFF_DEBUG)
+ {
+ FILE *fp = fopen("/tmp/kff.out", "a");
+ if (fp == NULL)
+ exit(1);
+ fprintf(fp, "KFF: '%s' is easily unreadable\n",
+ path);
+ fclose(fp);
+ }
+
+ apr_hash_set(*mergeinfo, path, APR_HASH_KEY_STRING,
+ NULL);
+ }
+ else
+ {
+ /* We have to actually do the authz check,
+ because we have no other information about
+ the path's readability. */
+ svn_boolean_t readable;
+ SVN_ERR(authz_read_func(&readable, root, path,
+ authz_read_baton, iterpool));
+ if (KFF_DEBUG)
+ {
+ FILE *fp = fopen("/tmp/kff.out", "a");
+ if (fp == NULL)
+ exit(1);
+ fprintf(fp, "KFF: '%s' authz-checked --> %d\n",
+ path, readable);
+ fclose(fp);
+ }
+
+ if (! readable)
+ apr_hash_set(*mergeinfo, path, APR_HASH_KEY_STRING,
+ NULL);
+ }
+ }
+ }
+ }
+ }
else
*mergeinfo = NULL;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH + RFC] authz scenario on 'reintegrate' branch?
Posted by David Glasser <gl...@davidglasser.net>.
On Jan 20, 2008 7:00 PM, Karl Fogel <kf...@red-bean.com> wrote:
> For way too long now, I've been trying to construct a scenario by
> which I could test a patch I've written to address this "TODO(reint)"
> comment in fs-wrap.c on the 'reintegrate' branch:
>
> /* We consciously do not perform authz checks on the paths returned
> in *MERGEINFO, avoiding massive authz overhead which would allow
> us to protect the name of where a change was merged from, but not
> the change itself. */
> /* ### TODO(reint): ... but how about descendant merged-to paths? */
> if (readable_paths->nelts > 0)
> SVN_ERR(svn_fs_get_mergeinfo(mergeinfo, root, readable_paths, inherit,
> include_descendants, pool));
>
> Well, er, yes, how about descendant merged-to paths?
>
> I could describe the zillion combinations of authz files and branch
> maneuvers I've tried, hoping to stimulate the new code I've written
> (see patch below). But that would take a lot of time, for probably
> little benefit. Instead, can anyone *think* of a scenario by which
> either of the two apr_hash_set() calls below might be reached? (Note
> the debugging prints, in case you want to trace the process yourself.)
>
> I ask knowing that I may be embarrassed by the obviousness of the
> answer. However, spinning my wheels by myself has so far not proven
> effective :-). When I try to construct scenarios, I can't ever make
> one in which a pre-existing authz check (e.g. must_have_access() in
> svnserve) doesn't fail first -- yet if some pre-existing check always
> fails first, then we clearly don't need this patch.
>
> Am I missing something?
I'm imagining:
anon-access=none
auth-access=write
user jrandom has explicit read access to / but no access at all to /foo/bar
/foo and /foo/bar both have svn:mergeinfo explicitly set on them.
jrandom calls svn_ra_get_mergeinfo ("/foo", include_descendents=true).
Does that not do the trick?
--dave
> -Karl
>
> Index: subversion/libsvn_repos/fs-wrap.c
> ===================================================================
> --- subversion/libsvn_repos/fs-wrap.c (revision 28964)
> +++ subversion/libsvn_repos/fs-wrap.c (working copy)
> @@ -23,9 +23,11 @@
> #include "svn_error.h"
> #include "svn_fs.h"
> #include "svn_path.h"
> +#include "svn_hash.h"
> #include "svn_props.h"
> #include "svn_repos.h"
> #include "repos.h"
> +#include "svn_mergeinfo.h"
> #include "svn_private_config.h"
>
>
> @@ -595,10 +597,143 @@
> in *MERGEINFO, avoiding massive authz overhead which would allow
> us to protect the name of where a change was merged from, but not
> the change itself. */
> - /* ### TODO(reint): ... but how about descendant merged-to paths? */
> - if (readable_paths->nelts > 0)
> - SVN_ERR(svn_fs_get_mergeinfo(mergeinfo, root, readable_paths, inherit,
> - include_descendants, pool));
> + if (readable_paths->nelts > 0)
> + {
> + SVN_ERR(svn_fs_get_mergeinfo(mergeinfo, root, readable_paths, inherit,
> + include_descendants, pool));
> +
> + if (include_descendants)
> + {
> + /* The mergeinfo might have paths that never got authz-checked;
> + if so, check them now. But only check the ones where we
> + can't know by examining 'paths' and 'readable_paths',
> + since an authz check is (relatively) expensive. */
> +
> + apr_hash_index_t *hi;
> + apr_hash_t *original_paths_hash, *readable_paths_hash;
> +
> +#define KFF_DEBUG 1
> +
> + if (KFF_DEBUG)
> + {
> + FILE *fp = fopen("/tmp/kff.out", "a");
> + if (fp == NULL)
> + exit(1);
> + fprintf(fp, "KFF: include_descendants is true\n");
> + fprintf(fp, "KFF: paths: '%s'\n",
> + svn_cstring_join((apr_array_header_t *) paths,
> + ", ", pool));
> + fprintf(fp, "KFF: readable_paths: '%s'\n",
> + svn_cstring_join((apr_array_header_t *) readable_paths,
> + ", ", pool));
> + fprintf(fp, "KFF: *mergeinfo count: %d\n",
> + apr_hash_count(*mergeinfo));
> + fclose(fp);
> + }
> +
> + /* Make lookups easy by converting to hash. */
> + SVN_ERR(svn_hash_from_cstring_keys(&original_paths_hash,
> + paths, pool));
> + if (readable_paths == paths)
> + {
> + readable_paths_hash = original_paths_hash;
> + }
> + else
> + {
> + SVN_ERR(svn_hash_from_cstring_keys(&readable_paths_hash,
> + readable_paths, pool));
> + }
> +
> + for (hi = apr_hash_first(pool, *mergeinfo); hi;
> + hi = apr_hash_next(hi))
> + {
> + const void *key;
> + void *val;
> + const char *path;
> + const char *raw_mergeinfo_string;
> + apr_array_header_t *mergeinfo_value_paths;
> + int j;
> +
> + svn_pool_clear(iterpool);
> + apr_hash_this(hi, &key, NULL, &val);
> + path = key;
> + raw_mergeinfo_string = val;
> +
> + if (KFF_DEBUG)
> + {
> + FILE *fp = fopen("/tmp/kff.out", "a");
> + if (fp == NULL)
> + exit(1);
> + fprintf(fp, "KFF: loop hit '%s'\n", path);
> + fprintf(fp, "KFF: mergeinfo for '%s':\n", path);
> + fprintf(fp, "'%s'\n",
> + (char *) apr_hash_get(*mergeinfo, path,
> + APR_HASH_KEY_STRING));
> + fprintf(fp, "KFF: (rp_h == op_h? --> %d)\n",
> + readable_paths_hash == original_paths_hash);
> + fclose(fp);
> + }
> +
> + if (! apr_hash_get(readable_paths_hash, path,
> + APR_HASH_KEY_STRING))
> + {
> + if (KFF_DEBUG)
> + {
> + FILE *fp = fopen("/tmp/kff.out", "a");
> + if (fp == NULL)
> + exit(1);
> + fprintf(fp, "KFF: '%s' is not in readable_paths\n",
> + path);
> + fclose(fp);
> + }
> +
> + /* It's not in readable_paths, so if it *is* in
> + the original paths, that means it was already
> + authz-checked and found unreadable -- in which
> + case remove it from the mergeinfo. */
> + if (readable_paths_hash != original_paths_hash
> + && apr_hash_get(original_paths_hash, path,
> + APR_HASH_KEY_STRING))
> + {
> + if (KFF_DEBUG)
> + {
> + FILE *fp = fopen("/tmp/kff.out", "a");
> + if (fp == NULL)
> + exit(1);
> + fprintf(fp, "KFF: '%s' is easily unreadable\n",
> + path);
> + fclose(fp);
> + }
> +
> + apr_hash_set(*mergeinfo, path, APR_HASH_KEY_STRING,
> + NULL);
> + }
> + else
> + {
> + /* We have to actually do the authz check,
> + because we have no other information about
> + the path's readability. */
> + svn_boolean_t readable;
> + SVN_ERR(authz_read_func(&readable, root, path,
> + authz_read_baton, iterpool));
> + if (KFF_DEBUG)
> + {
> + FILE *fp = fopen("/tmp/kff.out", "a");
> + if (fp == NULL)
> + exit(1);
> + fprintf(fp, "KFF: '%s' authz-checked --> %d\n",
> + path, readable);
> + fclose(fp);
> + }
> +
> + if (! readable)
> + apr_hash_set(*mergeinfo, path, APR_HASH_KEY_STRING,
> + NULL);
> + }
> + }
> + }
> + }
> + }
> else
> *mergeinfo = NULL;
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org