=== subversion/mod_authz_svn/mod_authz_svn.c ================================================================== --- subversion/mod_authz_svn/mod_authz_svn.c (revision 116) +++ subversion/mod_authz_svn/mod_authz_svn.c (revision 127) @@ -104,7 +104,7 @@ dav_error *dav_err; svn_repos_authz_access_t authz_svn_type = svn_authz_none; svn_boolean_t authz_access_granted = FALSE; - svn_config_t *access_conf = NULL; + svn_authz_t *access_conf = NULL; svn_error_t *svn_err; const char *cache_key; void *user_data; @@ -227,8 +227,9 @@ apr_pool_userdata_get(&user_data, cache_key, r->connection->pool); access_conf = user_data; if (access_conf == NULL) { - svn_err = svn_config_read(&access_conf, conf->access_file, TRUE, - r->connection->pool); + svn_err = svn_repos_authz_read(&access_conf, + conf->access_file, TRUE, + r->connection->pool); if (svn_err) { ap_log_rerror(APLOG_MARK, APLOG_ERR, /* If it is an error code that APR can make sense === subversion/include/svn_error_codes.h ================================================================== --- subversion/include/svn_error_codes.h (revision 116) +++ subversion/include/svn_error_codes.h (revision 127) @@ -1,7 +1,7 @@ /** * @copyright * ==================================================================== - * Copyright (c) 2000-2004 CollabNet. All rights reserved. + * Copyright (c) 2000-2005 CollabNet. All rights reserved. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -784,6 +784,9 @@ SVN_ERR_AUTHZ_CATEGORY_START + 2, "Item is partially readable") + SVN_ERRDEF (SVN_ERR_AUTHZ_INVALID_CONFIG, + SVN_ERR_AUTHZ_CATEGORY_START + 3, + "Invalid authz configuration") /* svndiff errors */ === subversion/include/svn_repos.h ================================================================== --- subversion/include/svn_repos.h (revision 116) +++ subversion/include/svn_repos.h (revision 127) @@ -30,7 +30,6 @@ #include "svn_types.h" #include "svn_error.h" #include "svn_version.h" -#include "svn_config.h" #ifdef __cplusplus @@ -1689,14 +1688,30 @@ svn_authz_recursive = 4 } svn_repos_authz_access_t; +/** A data type which stores the authz information. */ +typedef struct svn_authz_t svn_authz_t; + +/** Read authz configuration data from @a file (a file or registry + * path) into @a *cfgp, allocated in @a pool. + * + * If the read file is not a valid authz rule file, then abort loading + * and return an error. + * + * If @a file does not exist, then if @a must_exist, return an error, + * otherwise return an empty @c svn_config_t. + */ +svn_error_t * +svn_repos_authz_read (svn_authz_t **authzp, const char *file, + svn_boolean_t must_exist, apr_pool_t *pool); + /** - * Check wether @a user can access @a path in the repository @a + * Check whether @a user can access @a path in the repository @a * repos_name with the @a required_access. @a cfg lists the ACLs to * check against. Set @a *access_granted to indicate if the requested * access is granted. */ svn_error_t * -svn_repos_authz_check_access (svn_config_t *cfg, const char *repos_name, +svn_repos_authz_check_access (svn_authz_t *authz, const char *repos_name, const char *path, const char *user, svn_repos_authz_access_t required_access, svn_boolean_t *access_granted, === subversion/tests/libsvn_repos/repos-test.c ================================================================== --- subversion/tests/libsvn_repos/repos-test.c (revision 116) +++ subversion/tests/libsvn_repos/repos-test.c (revision 127) @@ -1031,6 +1031,47 @@ } +/* Helper for the authz test. Return a svn_config_t handle containing + a parsed representation of the given authz_contents. */ +static svn_error_t * +authz_get_handle (svn_authz_t **authzp, const char *authz_contents, + apr_pool_t *pool) +{ + char *authz_file_tmpl; + apr_file_t *authz_file; + apr_status_t apr_err; + const char *authz_file_path; + + /* Create a temporary file. */ + authz_file_tmpl = apr_pstrdup (pool, "authz_test_XXXXXX"); + apr_err = apr_file_mktemp (&authz_file, authz_file_tmpl, 0, pool); + if (apr_err != APR_SUCCESS) + return svn_error_wrap_apr (apr_err, "Opening temporary file"); + + /* Write the authz ACLs to the file. */ + apr_err = apr_file_write_full (authz_file, authz_contents, + strlen(authz_contents), NULL); + if (apr_err != APR_SUCCESS) + return svn_error_wrap_apr (apr_err, "Writing test authz file"); + + /* Read the authz configuration back and start testing. */ + apr_err = apr_file_name_get (&authz_file_path, authz_file); + if (apr_err != APR_SUCCESS) + return svn_error_wrap_apr (apr_err, "Getting authz file path"); + + SVN_ERR_W (svn_repos_authz_read (authzp, authz_file_path, TRUE, pool), + "Opening test authz file"); + + /* Close the temporary descriptor, which'll delete the file. */ + apr_err = apr_file_close (authz_file); + if (apr_err != APR_SUCCESS) + return svn_error_wrap_apr (apr_err, "Removing test authz file"); + + return SVN_NO_ERROR; +} + + + /* Test that authz is giving out the right authorizations. */ static svn_error_t * authz (const char **msg, @@ -1038,12 +1079,9 @@ svn_test_opts_t *opts, apr_pool_t *pool) { - char *authz_file_tmpl; - apr_file_t *authz_file; - apr_status_t apr_err; - const char *authz_file_path; const char *contents; - svn_config_t *cfg; + svn_authz_t *authz; + svn_error_t *err; svn_boolean_t access_granted; apr_pool_t *subpool = svn_pool_create (pool); int i; @@ -1080,15 +1118,21 @@ if (msg_only) return SVN_NO_ERROR; - /* The test logic: We dump a test authz file to disk, then load it - * and perform various access tests on it. Each test has a known - * outcome and tests different aspects of authz, such as inheriting - * parent-path authz, pan-repository rules or recursive access. - * 'plato' is our friendly neighborhood user with more access rights - * than other anonymous philosophers. + /* The test logic: + * + * 1. Perform various access tests on a set of authz rules. Each + * test has a known outcome and tests different aspects of authz, + * such as inheriting parent-path authz, pan-repository rules or + * recursive access. 'plato' is our friendly neighborhood user with + * more access rights than other anonymous philosophers. + * + * 2. Load an authz file containing a cyclic dependency in groups + * and another containing a reference to an undefined group. Verify + * that svn_repos_authz_read fails to load both and returns an + * "invalid configuration" error. */ - /* The authz rules for the test. */ + /* The authz rules for the phase 1 tests. */ contents = "[greek:/A]" APR_EOL_STR @@ -1129,41 +1173,13 @@ APR_EOL_STR APR_EOL_STR; - /* Create a temporary file and retrieve its path. */ - authz_file_tmpl = apr_pstrdup(subpool, "authz_test_XXXXXX"); - apr_err = apr_file_mktemp (&authz_file, authz_file_tmpl, - APR_CREATE | APR_READ | APR_WRITE, subpool); - if (apr_err != APR_SUCCESS) - return svn_error_wrap_apr(apr_err, "Opening temporary file"); + /* Load the test authz rules. */ + SVN_ERR (authz_get_handle(&authz, contents, subpool)); - /* Write the authz ACLs to the file. */ - apr_err = apr_file_write_full (authz_file, contents, - strlen(contents), NULL); - if (apr_err != APR_SUCCESS) - return svn_error_wrap_apr(apr_err, "Writing test authz file"); - - /* Read the authz configuration back and start testing. */ - apr_err = apr_file_name_get (&authz_file_path, authz_file); - if (apr_err != APR_SUCCESS) - return svn_error_wrap_apr(apr_err, "Getting authz file path"); - - /* Close the temporary descriptor. */ - apr_err = apr_file_close (authz_file); - if (apr_err != APR_SUCCESS) - return svn_error_wrap_apr(apr_err, "Closing test authz file"); - - SVN_ERR_W (svn_config_read (&cfg, authz_file_path, TRUE, subpool), - "Opening test authz file"); - - /* Delete the file. */ - apr_err = apr_file_remove (authz_file_path, subpool); - if (apr_err != APR_SUCCESS) - return svn_error_wrap_apr(apr_err, "Removing test authz file"); - /* Loop over the test array and test each case. */ for (i = 0; test_set[i].path != NULL; i++) { - SVN_ERR (svn_repos_authz_check_access (cfg, "greek", + SVN_ERR (svn_repos_authz_check_access (authz, "greek", test_set[i].path, test_set[i].user, test_set[i].required, @@ -1188,6 +1204,56 @@ } } + + /* The authz rules for the phase 2 tests, first case (cyclic + dependency). */ + contents = + "[groups]" + APR_EOL_STR + "slaves = cooks,scribes,@gladiators" + APR_EOL_STR + "gladiators = equites,thraces,@slaves" + APR_EOL_STR + APR_EOL_STR + "[greek:/A]" + APR_EOL_STR + "@slaves = r" + APR_EOL_STR; + + /* Load the test authz rules and check that group cycles are + reported. */ + err = authz_get_handle (&authz, contents, subpool); + if (!err || err->apr_err != SVN_ERR_AUTHZ_INVALID_CONFIG) + return svn_error_createf (SVN_ERR_TEST_FAILED, err, + "Got %s error instead of expected " + "SVN_ERR_AUTHZ_INVALID_CONFIG", + err ? "unexpected" : "no"); + svn_error_clear (err); + + /* The authz rules for the phase 2 tests, second case (missing group + definition). */ + contents = + "[groups]" + APR_EOL_STR + "slaves = cooks,scribes,@gladiators" + APR_EOL_STR + "gladiators = equites,thraces,@slaves" + APR_EOL_STR + APR_EOL_STR + "[greek:/A]" + APR_EOL_STR + "@senate = r" + APR_EOL_STR; + + /* Check that references to undefined groups are reported. */ + err = authz_get_handle (&authz, contents, subpool); + if (!err || err->apr_err != SVN_ERR_AUTHZ_INVALID_CONFIG) + return svn_error_createf (SVN_ERR_TEST_FAILED, err, + "Got %s error instead of expected " + "SVN_ERR_AUTHZ_INVALID_CONFIG", + err ? "unexpected" : "no"); + svn_error_clear (err); + /* That's a wrap! */ svn_pool_destroy (subpool); return SVN_NO_ERROR; === subversion/libsvn_repos/authz.c ================================================================== --- subversion/libsvn_repos/authz.c (revision 116) +++ subversion/libsvn_repos/authz.c (revision 127) @@ -24,11 +24,13 @@ #include "svn_error.h" #include "svn_path.h" #include "svn_repos.h" +#include "svn_config.h" struct authz_baton { apr_pool_t *pool; svn_config_t *config; + svn_error_t *err; const char *user; svn_repos_authz_access_t allow; @@ -43,6 +45,16 @@ +/* The data type storing the internal representation of an authz + * configuration. Currently this is just a wrapper around a + * svn_config_t. */ +struct svn_authz_t +{ + svn_config_t *cfg; +}; + + + /* Determine whether the required access is granted given what authz * are allowed and denied. Return TRUE if the required access is * granted. @@ -88,18 +100,19 @@ } -static svn_boolean_t -authz_group_contains_user_internal (svn_config_t *cfg, - const char *group, - const char *user, - apr_hash_t *checked_groups, - apr_pool_t *pool) +static svn_error_t * +authz_group_contains_user (svn_config_t *cfg, + const char *group, + const char *user, + svn_boolean_t *is_in_group, + apr_pool_t *pool) { const char *value; apr_array_header_t *list; int i; - svn_config_get (cfg, &value, "groups", group, ""); + svn_config_get (cfg, &value, "groups", group, NULL); + list = svn_cstring_split (value, ",", TRUE, pool); for (i = 0; i < list->nelts; i++) @@ -109,44 +122,27 @@ /* If the 'user' is a subgroup, recurse into it. */ if (*group_user == '@') { - /* Guard against circular dependencies by checking group - name against hash. */ - /* XXX: Should this return an error? */ - if (apr_hash_get (checked_groups, &group_user[1], - APR_HASH_KEY_STRING)) - continue; - - /* Add group to hash of checked groups. */ - apr_hash_set (checked_groups, &group_user[1], - APR_HASH_KEY_STRING, ""); - /* Recurse on that group. */ - if (authz_group_contains_user_internal (cfg, &group_user[1], - user, checked_groups, - pool)) - return TRUE; + SVN_ERR (authz_group_contains_user (cfg, &group_user[1], + user, is_in_group, + pool)); + if (*is_in_group) + return SVN_NO_ERROR; } /* If the user matches, stop. */ else if (strcmp (user, group_user) == 0) - return TRUE; + { + *is_in_group = TRUE; + return SVN_NO_ERROR; + } } - return FALSE; + *is_in_group = FALSE; + return SVN_NO_ERROR; } -static svn_boolean_t -authz_group_contains_user(svn_config_t *cfg, const char *group, - const char *user, apr_pool_t *pool) -{ - return authz_group_contains_user_internal (cfg, group, user, - apr_hash_make (pool), - pool); -} - - - /* Callback to parse one line of an authz file and update the * authz_baton accordingly. */ @@ -165,8 +161,19 @@ /* Group rule and user not in group. Stop. */ if (*name == '@') { - if (!authz_group_contains_user (b->config, &name[1], - b->user, b->pool)) + svn_boolean_t in_group; + svn_error_t *group_err; + + group_err = authz_group_contains_user (b->config, &name[1], + b->user, &in_group, + b->pool); + if (group_err) + { + /* Order our caller to stop parsing rules. */ + b->err = group_err; + return FALSE; + } + if (!in_group) return TRUE; } /* User rule for wrong user. Stop. */ @@ -211,6 +218,10 @@ svn_config_enumerate (b->config, section_name, authz_parse_line, b); + /* Errors abort any further processing. */ + if (b->err) + return FALSE; + /* Has the section explicitely determined an access? */ conclusive = authz_access_is_determined (b->allow, b->deny, b->required_access); @@ -231,15 +242,16 @@ * to access a section specific to the given repository before falling * back to pan-repository rules. * - * Return a boolean which tells the caller whether we were able to - * determine the requested access rights. Access status is in - * *access_granted. + * Update *access_granted and *access_determined to inform the caller + * of the outcome of the lookup. If an error is returned, the value + * of these two variables is undefined. */ -static svn_boolean_t +static svn_error_t * authz_get_path_access (svn_config_t *cfg, const char *repos_name, const char *path, const char *user, svn_repos_authz_access_t required_access, svn_boolean_t *access_granted, + svn_boolean_t *access_determined, apr_pool_t *pool) { const char *qualified_path; @@ -248,27 +260,35 @@ baton.pool = pool; baton.config = cfg; baton.user = user; + baton.err = SVN_NO_ERROR; /* Try to locate a repository-specific block first. */ qualified_path = apr_pstrcat (pool, repos_name, ":", path, NULL); svn_config_enumerate (cfg, qualified_path, authz_parse_line, &baton); + SVN_ERR(baton.err); *access_granted = authz_access_is_granted (baton.allow, baton.deny, required_access); /* If the first test has determined access, stop now. */ - if (authz_access_is_determined (baton.allow, baton.deny, - required_access)) - return TRUE; + *access_determined = authz_access_is_determined (baton.allow, + baton.deny, + required_access); + if (*access_determined) + return SVN_NO_ERROR; /* No repository specific rule, try pan-repository rules. */ svn_config_enumerate (cfg, path, authz_parse_line, &baton); + SVN_ERR(baton.err); *access_granted = authz_access_is_granted (baton.allow, baton.deny, required_access); - return authz_access_is_determined (baton.allow, baton.deny, - required_access); + *access_determined = authz_access_is_determined (baton.allow, + baton.deny, + required_access); + + return SVN_NO_ERROR; } @@ -279,9 +299,11 @@ * requested access. * * As soon as one is found, or else when the whole ACL file has been - * searched, return the updated authorization in access_granted. + * searched, return the updated authorization in access_granted. If + * an error is thrown during the authz lookup, access_granted is left + * untouched. */ -static void +static svn_error_t * authz_get_tree_access (svn_config_t *cfg, const char *repos_name, const char *path, const char *user, svn_repos_authz_access_t required_access, @@ -293,6 +315,7 @@ baton.pool = pool; baton.config = cfg; baton.user = user; + baton.err = SVN_NO_ERROR; baton.required_access = required_access; baton.repos_path = path; baton.qualified_repos_path = apr_pstrcat (pool, repos_name, @@ -301,14 +324,168 @@ baton.access = TRUE; svn_config_enumerate_sections (cfg, authz_parse_section, &baton); + SVN_ERR(baton.err); *access_granted = baton.access; + return SVN_NO_ERROR; } +/* Examine the entire definition of a group, checking for errors. The + * errors detected are references to non-existent groups and circular + * dependencies between groups. */ +static svn_error_t * +authz_group_walk (svn_config_t *cfg, + const char *group, + apr_hash_t *checked_groups, + apr_pool_t *pool) +{ + const char *value; + apr_array_header_t *list; + int i; + + svn_config_get (cfg, &value, "groups", group, NULL); + /* Having a non-existent group in the ACL configuration might be the + sign of a typo. Refuse to perform authz on uncertain rules. */ + if (!value) + return svn_error_createf (SVN_ERR_AUTHZ_INVALID_CONFIG, NULL, + "An authz rule refers to group '%s', " + "which is undefined", + group); + + list = svn_cstring_split (value, ",", TRUE, pool); + + for (i = 0; i < list->nelts; i++) + { + const char *group_user = APR_ARRAY_IDX(list, i, char *); + + /* If the 'user' is a subgroup, recurse into it. */ + if (*group_user == '@') + { + /* A circular dependency between groups is a Bad Thing. We + don't do authz with invalid ACL files. */ + if (apr_hash_get (checked_groups, &group_user[1], + APR_HASH_KEY_STRING)) + return svn_error_createf (SVN_ERR_AUTHZ_INVALID_CONFIG, + NULL, + "Circular dependency between " + "groups '%s' and '%s'", + &group_user[1], group); + + /* Add group to hash of checked groups. */ + apr_hash_set (checked_groups, &group_user[1], + APR_HASH_KEY_STRING, ""); + + /* Recurse on that group. */ + SVN_ERR (authz_group_walk (cfg, &group_user[1], + checked_groups, pool)); + } + } + + return SVN_NO_ERROR; +} + + + +/* Callback to check whether groups mentioned in the given authz rule + exist. */ +static svn_boolean_t authz_validate_rule (const char *name, + const char *value, + void *baton) +{ + const char *val; + struct authz_baton *b = baton; + + /* If the rule applies to a group, check its existence. */ + if (*name == '@') + { + svn_config_get (b->config, &val, "groups", + &name[1], NULL); + /* Having a non-existent group in the ACL configuration might be + the sign of a typo. Refuse to perform authz on uncertain + rules. */ + if (!value) + { + b->err = svn_error_createf (SVN_ERR_AUTHZ_INVALID_CONFIG, NULL, + "An authz rule refers to group " + "'%s', which is undefined", + name); + return FALSE; + } + } + + return TRUE; +} + + + +/* Callback to check whether groups defined don't cyclicly depend on + each other. */ +static svn_boolean_t authz_validate_group (const char *name, + const char *value, + void *baton) +{ + struct authz_baton *b = baton; + + b->err = authz_group_walk (b->config, name, apr_hash_make (b->pool), + b->pool); + if (b->err) + return FALSE; + + return TRUE; +} + + + +/* Callback to validate a section of the authz ruleset. */ +static svn_boolean_t authz_validate_section (const char *name, + void *baton) +{ + struct authz_baton *b = baton; + + /* If the section is the groups definition, use the group checking + callback. Otherwise, use the rule checking callback. */ + if (strncmp (name, "groups", 6) == 0) + svn_config_enumerate (b->config, name, authz_validate_group, baton); + else + svn_config_enumerate (b->config, name, authz_validate_rule, baton); + + if (b->err) + return FALSE; + + return TRUE; +} + + + svn_error_t * -svn_repos_authz_check_access (svn_config_t *cfg, const char *repos_name, +svn_repos_authz_read (svn_authz_t **authzp, const char *file, + svn_boolean_t must_exist, apr_pool_t *pool) +{ + svn_authz_t *authz = apr_palloc (pool, sizeof(*authz)); + struct authz_baton baton = { 0 }; + + baton.pool = pool; + baton.err = SVN_NO_ERROR; + + /* Load the rule file. */ + SVN_ERR (svn_config_read (&authz->cfg, file, must_exist, pool)); + baton.config = authz->cfg; + + /* Step through the entire rule file, aborting on error. */ + svn_config_enumerate_sections (authz->cfg, authz_validate_section, + &baton); + SVN_ERR (baton.err); + + *authzp = authz; + return SVN_NO_ERROR; +} + + + +svn_error_t * +svn_repos_authz_check_access (svn_authz_t *authz, const char *repos_name, const char *path, const char *user, svn_repos_authz_access_t required_access, svn_boolean_t *access_granted, @@ -321,10 +498,12 @@ /* Determine the granted access for the requested path. */ do { - access_determined = authz_get_path_access (cfg, repos_name, - current_path, user, - required_access, - access_granted, pool); + SVN_ERR (authz_get_path_access (authz->cfg, repos_name, + current_path, user, + required_access, + access_granted, + &access_determined, + pool)); if (!access_determined) { @@ -345,11 +524,10 @@ /* If the caller requested recursive access, we need to walk through the entire authz config to see whether any child paths are denied to the requested user. */ - if (*access_granted - && (required_access & svn_authz_recursive)) - authz_get_tree_access (cfg, repos_name, path, user, - required_access, access_granted, - pool); + if (*access_granted && (required_access & svn_authz_recursive)) + SVN_ERR (authz_get_tree_access (authz->cfg, repos_name, path, user, + required_access, access_granted, + pool)); return SVN_NO_ERROR; }