From: Justin Erenkrantz Date: Sat, 3 Dec 2005 07:26:54 +0000 (+0000) Subject: Take a quick pass through the refactored authz code. No real big changes here. X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8cbc2dbb69f310d1a0ad86b7e74fde6af98b64b9;p=thirdparty%2Fapache%2Fhttpd.git Take a quick pass through the refactored authz code. No real big changes here. * modules/aaa/mod_authz_host.c: Tweak locations of some comments; reflow long lines; correct and simplify (I think) ap_some_auth_required logic. * modules/aaa/mod_auth.h: Remove AUTHZ_DECLINED for now; reflow long lines; improve comments; remove some no longer useful comments * modules/aaa/mod_authz_user.c: Reflow long lines and remove unneeded variable. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/authz-dev@351902 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/modules/aaa/mod_auth.h b/modules/aaa/mod_auth.h index 43c372bd831..48d585494ed 100644 --- a/modules/aaa/mod_auth.h +++ b/modules/aaa/mod_auth.h @@ -16,7 +16,7 @@ /** * @file mod_auth.h - * @brief uthentication Extension Module for Apache + * @brief Authentication and Authorization Extension for Apache * * @defgroup MOD_AUTH mod_auth * @ingroup APACHE_MODS @@ -38,7 +38,7 @@ extern "C" { #define AUTHZ_PROVIDER_GROUP "authz" #define AUTHN_DEFAULT_PROVIDER "file" #define AUTHZ_DEFAULT_PROVIDER "valid-user" - + #define AUTHZ_GROUP_NOTE "authz_group_note" #define AUTHN_PROVIDER_NAME_NOTE "authn_provider_name" #define AUTHZ_PROVIDER_NAME_NOTE "authz_provider_name" @@ -53,7 +53,6 @@ typedef enum { typedef enum { AUTHZ_DENIED, - AUTHZ_DECLINED, AUTHZ_GRANTED, AUTHZ_GENERAL_ERROR } authz_status; @@ -63,7 +62,7 @@ typedef struct { * if we can validate this user/password combination. */ authn_status (*check_password)(request_rec *r, const char *user, - const char *password); + const char *password); /* Given a user and realm, expected to return AUTH_USER_FOUND if we * can find a md5 hash of 'user:realm:password' @@ -83,9 +82,11 @@ struct authn_provider_list { typedef struct { /* Given a request_rec, expected to return AUTH_GRANTED - * if we can authorize user access. - */ - authz_status (*check_authorization)(request_rec *r, apr_int64_t method_mask, const char *require_line); + * if we can authorize user access. + */ + authz_status (*check_authorization)(request_rec *r, + apr_int64_t method_mask, + const char *require_line); } authz_provider; /* A linked-list of authn providers. */ @@ -95,24 +96,12 @@ struct authz_provider_list { const char *provider_name; const authz_provider *provider; authz_provider_list *next; - /** Where the require line is in the config file. */ + /** If a Limit method is in effect, this field will be set */ apr_int64_t method_mask; - /** The complete string from the command line */ + /** String following 'require ' from config file */ char *requirement; }; -/* Need to add an enum for authz_status. Convert one of the authorization - modules to deal with the new require directive. -*/ - - -#if 0 -typedef struct { - /* For a given user, return a hash of all groups the user belongs to. */ - apr_hash_t * (*get_user_groups)(request_rec *r, const char *user); -} authz_provider; -#endif - #ifdef __cplusplus } #endif diff --git a/modules/aaa/mod_authz_host.c b/modules/aaa/mod_authz_host.c index 6ccbb84f384..39ad360d9d2 100644 --- a/modules/aaa/mod_authz_host.c +++ b/modules/aaa/mod_authz_host.c @@ -96,7 +96,7 @@ static void *merge_authz_host_dir_config(apr_pool_t *a, void *basev, void *newv) authz_host_dir_conf *base = (authz_host_dir_conf *)basev; authz_host_dir_conf *new = (authz_host_dir_conf *)newv; authz_host_dir_conf *conf; - + /* Create this conf by duplicating the base, replacing elements * (or creating copies for merging) where new-> values exist. */ @@ -234,6 +234,7 @@ static const char *add_authz_provider(cmd_parms *cmd, void *config, authz_provider_list *newp; newp = apr_pcalloc(cmd->pool, sizeof(authz_provider_list)); + /* XXX: Split this out to the name and then the rest of the directive. */ newp->provider_name = apr_pstrdup(cmd->pool, arg); newp->requirement = apr_pstrdup(cmd->pool, arg); newp->method_mask = cmd->limited; @@ -242,19 +243,21 @@ static const char *add_authz_provider(cmd_parms *cmd, void *config, newp->provider = ap_lookup_provider(AUTHZ_PROVIDER_GROUP, newp->provider_name, "0"); + /* by the time the config file is used, the provider should be loaded + * and registered with us. + */ if (newp->provider == NULL) { - /* by the time they use it, the provider should be loaded and - registered with us. */ return apr_psprintf(cmd->pool, "Unknown Authz provider: %s", newp->provider_name); } + /* if the provider doesn't provide the appropriate function, reject it */ if (!newp->provider->check_authorization) { - /* if it doesn't provide the appropriate function, reject it */ return apr_psprintf(cmd->pool, - "The '%s' Authz provider is not supported by any of the " - "loaded authorization modules", newp->provider_name); + "The '%s' Authz provider is not supported by any " + "of the loaded authorization modules ", + newp->provider_name); } /* Add it to the list now. */ @@ -284,7 +287,8 @@ static const command_rec authz_host_cmds[] = AP_INIT_ITERATE2("deny", allow_cmd, NULL, OR_LIMIT, "'from' followed by hostnames or IP-address wildcards"), AP_INIT_RAW_ARGS("Require", add_authz_provider, NULL, OR_AUTHCFG, - "Selects which authenticated users or groups may access a protected space"), + "Selects which authenticated users or groups may access " + "a protected space"), {NULL} }; /* @@ -430,7 +434,7 @@ static int check_dir_access(request_rec *r) static int authorize_user(request_rec *r) { authz_host_dir_conf *conf = ap_get_module_config(r->per_dir_config, - &authz_host_module); + &authz_host_module); authz_status auth_result; authz_provider_list *current_provider; @@ -439,31 +443,39 @@ static int authorize_user(request_rec *r) const authz_provider *provider; /* For now, if a provider isn't set, we'll be nice and use the file - * provider. - */ + * provider. + */ if (!current_provider) { provider = ap_lookup_provider(AUTHZ_PROVIDER_GROUP, AUTHZ_DEFAULT_PROVIDER, "0"); if (!provider || !provider->check_authorization) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, - "No Authz provider configured"); + "No default authz provider configured"); auth_result = AUTHZ_GENERAL_ERROR; break; } - apr_table_setn(r->notes, AUTHZ_PROVIDER_NAME_NOTE, AUTHZ_DEFAULT_PROVIDER); + apr_table_setn(r->notes, AUTHZ_PROVIDER_NAME_NOTE, + AUTHZ_DEFAULT_PROVIDER); } else { provider = current_provider->provider; - apr_table_setn(r->notes, AUTHZ_PROVIDER_NAME_NOTE, current_provider->provider_name); + apr_table_setn(r->notes, AUTHZ_PROVIDER_NAME_NOTE, + current_provider->provider_name); } - auth_result = provider->check_authorization(r, current_provider->method_mask, current_provider->requirement); + auth_result = provider->check_authorization(r, + current_provider->method_mask, + current_provider->requirement); apr_table_unset(r->notes, AUTHZ_PROVIDER_NAME_NOTE); /* Something occured. Stop checking. */ + /* XXX: We need to figure out what the implications of multiple + * require directives are. Must all satisfy? Can we leverage + * satisfy here then? + */ if (auth_result != AUTHZ_DENIED) { break; } @@ -479,11 +491,6 @@ static int authorize_user(request_rec *r) if (auth_result != AUTHZ_GRANTED) { int return_code; -/* XXX need to deal with DECLINED vs DENIED. DECLINED may not even - be needed since we are only going to call registered require providers. - I assume that it will deal with passing from one provider to the next - according to the order and the Authz_xxx_Authoritative directives. -*/ switch (auth_result) { case AUTHZ_DENIED: ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, @@ -493,15 +500,16 @@ static int authorize_user(request_rec *r) break; case AUTHZ_GENERAL_ERROR: default: - /* We'll assume that the module has already said what its error - * was in the logs. - */ + /* We'll assume that the module has already said what its + * error was in the logs. + */ return_code = HTTP_INTERNAL_SERVER_ERROR; break; } /* If we're returning 403, tell them to try again. */ if (return_code == HTTP_UNAUTHORIZED) { + /* XXX: Why is this a basic auth failure? */ ap_note_basic_auth_failure (r); } return return_code; @@ -525,38 +533,20 @@ static int authz_some_auth_required(request_rec *r) authz_host_dir_conf *conf = ap_get_module_config(r->per_dir_config, &authz_host_module); authz_provider_list *current_provider; - int req_authz = 1; + int req_authz = 0; current_provider = conf->providers; - do { - const authz_provider *provider; - - /* For now, if a provider isn't set, we'll be nice and use the file - * provider. - */ - if (!current_provider) { -/* provider = ap_lookup_provider(AUTHZ_PROVIDER_GROUP, - AUTHZ_DEFAULT_PROVIDER, "0"); + while (current_provider) { - if (!provider || !provider->check_authorization) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, - "No Authz providers configured. Assmuming no authorization required."); -*/ - req_authz = 0; - break; -/* }*/ - } - else { - provider = current_provider->provider; - } - - if (current_provider->method_mask & (AP_METHOD_BIT << r->method_number)) { + /* Does this provider config apply for this method */ + if (current_provider->method_mask & + (AP_METHOD_BIT << r->method_number)) { req_authz = 1; break; } - + current_provider = current_provider->next; - } while (current_provider); + } return req_authz; } @@ -593,7 +583,7 @@ static void register_hooks(apr_pool_t *p) */ /* This can be access checker since we don't require r->user to be set. */ - ap_hook_access_checker(check_dir_access,NULL,NULL,APR_HOOK_MIDDLE); + ap_hook_access_checker(check_dir_access, NULL, NULL, APR_HOOK_MIDDLE); ap_hook_auth_checker(authorize_user, NULL, NULL, APR_HOOK_MIDDLE); } diff --git a/modules/aaa/mod_authz_user.c b/modules/aaa/mod_authz_user.c index 15efaa47d22..de8aada83df 100644 --- a/modules/aaa/mod_authz_user.c +++ b/modules/aaa/mod_authz_user.c @@ -117,9 +117,10 @@ static int check_user_access(request_rec *r) } #endif -static authz_status user_check_authorization(request_rec *r, apr_int64_t method_mask, const char *require_line) +static authz_status user_check_authorization(request_rec *r, + apr_int64_t method_mask, + const char *require_line) { - char *user = r->user; int m = r->method_number; const char *t, *w; @@ -131,11 +132,11 @@ static authz_status user_check_authorization(request_rec *r, apr_int64_t method_ w = ap_getword_white(r->pool, &t); if (!strcasecmp(w, "user")) { /* And note that there are applicable requirements - * which we consider ourselves the owner of. - */ + * which we consider ourselves the owner of. + */ while (t[0]) { w = ap_getword_conf(r->pool, &t); - if (!strcmp(user, w)) { + if (!strcmp(r->user, w)) { return AUTHZ_GRANTED; } } @@ -143,11 +144,11 @@ static authz_status user_check_authorization(request_rec *r, apr_int64_t method_ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, "access to %s failed, reason: user '%s' does not meet " - "'require'ments for user to be allowed access", - r->uri, user); + "'require'ments for user to be allowed access", + r->uri, r->user); ap_note_auth_failure(r); - return AUTHZ_GENERAL_ERROR; + return AUTHZ_DENIED; } static authz_status validuser_check_authorization(request_rec *r, apr_int64_t method_mask, const char *require_line)