You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Graham Leggett <mi...@sharp.fm> on 2014/11/10 22:49:27 UTC
Re: svn commit: r1608202 - in /httpd/httpd/trunk: CHANGES modules/aaa/mod_authnz_ldap.c
On 06 Jul 2014, at 4:06 PM, covener@apache.org wrote:
> Author: covener
> Date: Sun Jul 6 14:06:50 2014
> New Revision: 1608202
>
> URL: http://svn.apache.org/r1608202
> Log:
> Consolidate common code that got duplicated by 2.3.x authz refactoring.
>
> Arrange for backend LDAP connections to be returned
> to the pool by a fixup hook rather than staying locked
> until the end of (a potentially slow) request.
>
> Add a little more trace4 to the authnz_ldap side of LDAP connection obtain/release.
> - if (sec->host) {
> - ldc = get_connection_for_authz(r, LDAP_COMPARE);
> - apr_pool_cleanup_register(r->pool, ldc,
> - authnz_ldap_cleanup_connection_close,
> - apr_pool_cleanup_null);
> - }
> - else {
> + if (!sec->host) {
> ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01698)
> "auth_ldap authorize: no sec->host - weird...?");
> return AUTHZ_DENIED;
> }
>
> + if (!req) {
> + authz_status rv = AUTHZ_DENIED;
> + req = build_request_config(r);
> + if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
> + return rv;
> + }
> + }
> + ldc = get_connection_for_authz(r, LDAP_COMPARE);
I just tripped over this - ldc is passed to get_dn_for_nonldap_authn() where, still NULL, it triggers a segfault. The ldc is set later in the line above.
> +
> /*
> * If we have been authenticated by some other module than mod_authnz_ldap,
> * the req structure needed for authorization needs to be created
> @@ -674,38 +746,6 @@ static authz_status ldapuser_check_autho
> r->ap_auth_type);
> }
>
> - if(!req) {
> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01700)
> - "ldap authorize: Creating LDAP req structure");
> -
> - req = (authn_ldap_request_t *)apr_pcalloc(r->pool,
> - sizeof(authn_ldap_request_t));
> -
> - /* Build the username filter */
> - if (APR_SUCCESS != authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec)) {
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02623)
> - "auth_ldap authorize: ldap filter too long (>%d): %s",
> - FILTER_LENGTH, filtbuf);
> - return AUTHZ_DENIED;
> - }
> -
> - /* Search for the user DN */
> - result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
> - sec->scope, sec->attributes, filtbuf, &dn, &(req->vals));
> -
> - /* Search failed, log error and return failure */
> - if(result != LDAP_SUCCESS) {
> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01701)
> - "auth_ldap authorise: User DN not found, %s", ldc->reason);
> - return AUTHZ_DENIED;
> - }
> -
> - ap_set_module_config(r->request_config, &authnz_ldap_module, req);
> - req->dn = apr_pstrdup(r->pool, dn);
> - req->user = r->user;
> -
> - }
> -
> if (req->dn == NULL || strlen(req->dn) == 0) {
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01702)
> "auth_ldap authorize: require user: user's DN has not "
> @@ -791,8 +831,6 @@ static authz_status ldapgroup_check_auth
>
> const char *t;
>
> - char filtbuf[FILTER_LENGTH];
> - const char *dn = NULL;
> struct mod_auth_ldap_groupattr_entry_t *ent;
> int i;
>
> @@ -804,18 +842,21 @@ static authz_status ldapgroup_check_auth
> return AUTHZ_DENIED;
> }
>
> - if (sec->host) {
> - ldc = get_connection_for_authz(r, LDAP_COMPARE); /* for the top-level group only */
> - apr_pool_cleanup_register(r->pool, ldc,
> - authnz_ldap_cleanup_connection_close,
> - apr_pool_cleanup_null);
> - }
> - else {
> + if (!sec->host) {
> ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01708)
> "auth_ldap authorize: no sec->host - weird...?");
> return AUTHZ_DENIED;
> }
>
> + if (!req) {
> + authz_status rv = AUTHZ_DENIED;
> + req = build_request_config(r);
> + if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
> + return rv;
> + }
> + }
> + ldc = get_connection_for_authz(r, LDAP_COMPARE);
Here too.
> +
> /*
> * If there are no elements in the group attribute array, the default should be
> * member and uniquemember; populate the array now.
> @@ -864,36 +905,7 @@ static authz_status ldapgroup_check_auth
> r->ap_auth_type);
> }
>
> - if(!req) {
> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01710)
> - "ldap authorize: Creating LDAP req structure");
> -
> - req = (authn_ldap_request_t *)apr_pcalloc(r->pool,
> - sizeof(authn_ldap_request_t));
> - /* Build the username filter */
> - if (APR_SUCCESS != authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec)) {
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02624)
> - "auth_ldap authorize: ldap filter too long (>%d): %s",
> - FILTER_LENGTH, filtbuf);
> - return AUTHZ_DENIED;
> - }
> -
> - /* Search for the user DN */
> - result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
> - sec->scope, sec->attributes, filtbuf, &dn, &(req->vals));
> -
> - /* Search failed, log error and return failure */
> - if(result != LDAP_SUCCESS) {
> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01711)
> - "auth_ldap authorise: User DN not found, %s", ldc->reason);
> - return AUTHZ_DENIED;
> - }
> -
> - ap_set_module_config(r->request_config, &authnz_ldap_module, req);
> - req->dn = apr_pstrdup(r->pool, dn);
> - req->user = r->user;
> - }
> -
> +
> ent = (struct mod_auth_ldap_groupattr_entry_t *) sec->groupattr->elts;
>
> if (sec->group_attrib_is_dn) {
> @@ -957,21 +969,18 @@ static authz_status ldapgroup_check_auth
> }
> }
>
> - for (i = 0; i < sec->groupattr->nelts; i++) {
> - /* nested groups need searches and compares, so grab a new handle */
> - authnz_ldap_cleanup_connection_close(ldc);
> - apr_pool_cleanup_kill(r->pool, ldc,authnz_ldap_cleanup_connection_close);
> -
> + /* nested groups need searches and compares, so grab a new handle */
> + if (sec->groupattr->nelts > 0) {
> + release_ldc(r, ldc);
> ldc = get_connection_for_authz(r, LDAP_COMPARE_AND_SEARCH);
> - apr_pool_cleanup_register(r->pool, ldc,
> - authnz_ldap_cleanup_connection_close,
> - apr_pool_cleanup_null);
> -
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01716)
> - "auth_ldap authorise: require group \"%s\": "
> - "failed [%s][%d - %s], checking sub-groups",
> - t, ldc->reason, result, ldap_err2string(result));
> + "auth_ldap authorise: require group \"%s\": "
> + "failed [%s][%d - %s], checking sub-groups",
> + t, ldc->reason, result, ldap_err2string(result));
> + }
> +
>
> + for (i = 0; i < sec->groupattr->nelts; i++) {
> result = util_ldap_cache_check_subgroups(r, ldc, sec->url, t, ent[i].name,
> sec->group_attrib_is_dn ? req->dn : req->user,
> sec->sgAttributes[0] ? sec->sgAttributes : default_attributes,
> @@ -1023,9 +1032,6 @@ static authz_status ldapdn_check_authori
>
> const char *t;
>
> - char filtbuf[FILTER_LENGTH];
> - const char *dn = NULL;
> -
> if (!r->user) {
> return AUTHZ_DENIED_NO_USER;
> }
> @@ -1034,13 +1040,7 @@ static authz_status ldapdn_check_authori
> return AUTHZ_DENIED;
> }
>
> - if (sec->host) {
> - ldc = get_connection_for_authz(r, LDAP_SEARCH); /* _comparedn is a searche */
> - apr_pool_cleanup_register(r->pool, ldc,
> - authnz_ldap_cleanup_connection_close,
> - apr_pool_cleanup_null);
> - }
> - else {
> + if (!sec->host) {
> ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01721)
> "auth_ldap authorize: no sec->host - weird...?");
> return AUTHZ_DENIED;
> @@ -1058,35 +1058,14 @@ static authz_status ldapdn_check_authori
> r->ap_auth_type);
> }
>
> - if(!req) {
> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01723)
> - "ldap authorize: Creating LDAP req structure");
> -
> - req = (authn_ldap_request_t *)apr_pcalloc(r->pool,
> - sizeof(authn_ldap_request_t));
> - /* Build the username filter */
> - if (APR_SUCCESS != authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec)) {
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02625)
> - "auth_ldap authorize: ldap filter too long (>%d): %s",
> - FILTER_LENGTH, filtbuf);
> - return AUTHZ_DENIED;
> - }
> -
> - /* Search for the user DN */
> - result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
> - sec->scope, sec->attributes, filtbuf, &dn, &(req->vals));
> -
> - /* Search failed, log error and return failure */
> - if(result != LDAP_SUCCESS) {
> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01724)
> - "auth_ldap authorise: User DN not found with filter %s: %s", filtbuf, ldc->reason);
> - return AUTHZ_DENIED;
> + if (!req) {
> + authz_status rv = AUTHZ_DENIED;
> + req = build_request_config(r);
> + if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
> + return rv;
> }
> -
> - ap_set_module_config(r->request_config, &authnz_ldap_module, req);
> - req->dn = apr_pstrdup(r->pool, dn);
> - req->user = r->user;
> }
> + ldc = get_connection_for_authz(r, LDAP_SEARCH); /* comparedn is a search */
Here too.
>
> require = ap_expr_str_exec(r, expr, &err);
> if (err) {
> @@ -1150,9 +1129,6 @@ static authz_status ldapattribute_check_
> const char *t;
> char *w, *value;
>
> - char filtbuf[FILTER_LENGTH];
> - const char *dn = NULL;
> -
> if (!r->user) {
> return AUTHZ_DENIED_NO_USER;
> }
> @@ -1161,13 +1137,7 @@ static authz_status ldapattribute_check_
> return AUTHZ_DENIED;
> }
>
> - if (sec->host) {
> - ldc = get_connection_for_authz(r, LDAP_COMPARE);
> - apr_pool_cleanup_register(r->pool, ldc,
> - authnz_ldap_cleanup_connection_close,
> - apr_pool_cleanup_null);
> - }
> - else {
> + if (!sec->host) {
> ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01729)
> "auth_ldap authorize: no sec->host - weird...?");
> return AUTHZ_DENIED;
> @@ -1185,35 +1155,14 @@ static authz_status ldapattribute_check_
> r->ap_auth_type);
> }
>
> - if(!req) {
> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01731)
> - "ldap authorize: Creating LDAP req structure");
> -
> - req = (authn_ldap_request_t *)apr_pcalloc(r->pool,
> - sizeof(authn_ldap_request_t));
> - /* Build the username filter */
> - if (APR_SUCCESS != authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec)) {
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02626)
> - "auth_ldap authorize: ldap filter too long (>%d): %s",
> - FILTER_LENGTH, filtbuf);
> - return AUTHZ_DENIED;
> - }
> -
> - /* Search for the user DN */
> - result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
> - sec->scope, sec->attributes, filtbuf, &dn, &(req->vals));
> -
> - /* Search failed, log error and return failure */
> - if(result != LDAP_SUCCESS) {
> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01732)
> - "auth_ldap authorise: User DN not found with filter %s: %s", filtbuf, ldc->reason);
> - return AUTHZ_DENIED;
> + if (!req) {
> + authz_status rv = AUTHZ_DENIED;
> + req = build_request_config(r);
> + if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
> + return rv;
> }
> -
> - ap_set_module_config(r->request_config, &authnz_ldap_module, req);
> - req->dn = apr_pstrdup(r->pool, dn);
> - req->user = r->user;
> }
> + ldc = get_connection_for_authz(r, LDAP_COMPARE);
Here too.
>
> if (req->dn == NULL || strlen(req->dn) == 0) {
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01733)
> @@ -1294,13 +1243,7 @@ static authz_status ldapfilter_check_aut
> return AUTHZ_DENIED;
> }
>
> - if (sec->host) {
> - ldc = get_connection_for_authz(r, LDAP_SEARCH);
> - apr_pool_cleanup_register(r->pool, ldc,
> - authnz_ldap_cleanup_connection_close,
> - apr_pool_cleanup_null);
> - }
> - else {
> + if (!sec->host) {
> ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01738)
> "auth_ldap authorize: no sec->host - weird...?");
> return AUTHZ_DENIED;
> @@ -1318,35 +1261,14 @@ static authz_status ldapfilter_check_aut
> r->ap_auth_type);
> }
>
> - if(!req) {
> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01740)
> - "ldap authorize: Creating LDAP req structure");
> -
> - req = (authn_ldap_request_t *)apr_pcalloc(r->pool,
> - sizeof(authn_ldap_request_t));
> - /* Build the username filter */
> - if (APR_SUCCESS != authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec)) {
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02627)
> - "auth_ldap authorize: ldap filter too long (>%d): %s",
> - FILTER_LENGTH, filtbuf);
> - return AUTHZ_DENIED;
> - }
> -
> - /* Search for the user DN */
> - result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
> - sec->scope, sec->attributes, filtbuf, &dn, &(req->vals));
> -
> - /* Search failed, log error and return failure */
> - if(result != LDAP_SUCCESS) {
> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01741)
> - "auth_ldap authorise: User DN not found with filter %s: %s", filtbuf, ldc->reason);
> - return AUTHZ_DENIED;
> + if (!req) {
> + authz_status rv = AUTHZ_DENIED;
> + req = build_request_config(r);
> + if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
> + return rv;
> }
> -
> - ap_set_module_config(r->request_config, &authnz_ldap_module, req);
> - req->dn = apr_pstrdup(r->pool, dn);
> - req->user = r->user;
> }
> + ldc = get_connection_for_authz(r, LDAP_SEARCH);
Here too.
>
> if (req->dn == NULL || strlen(req->dn) == 0) {
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01742)
> @@ -1387,8 +1309,7 @@ static authz_status ldapfilter_check_aut
> "auth_ldap authorize: checking dn match %s", dn);
> if (sec->compare_as_user) {
> /* ldap-filter is the only authz that requires a search and a compare */
> - apr_pool_cleanup_kill(r->pool, ldc, authnz_ldap_cleanup_connection_close);
> - authnz_ldap_cleanup_connection_close(ldc);
> + release_ldc(r, ldc);
> ldc = get_connection_for_authz(r, LDAP_COMPARE);
> }
> result = util_ldap_cache_comparedn(r, ldc, sec->url, req->dn, dn,
> @@ -1449,9 +1370,6 @@ static authz_status ldapsearch_check_aut
>
> if (sec->host) {
> ldc = get_connection_for_authz(r, LDAP_SEARCH);
> - apr_pool_cleanup_register(r->pool, ldc,
> - authnz_ldap_cleanup_connection_close,
> - apr_pool_cleanup_null);
> }
> else {
> ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(02636)
> @@ -2022,6 +1940,18 @@ static void ImportULDAPOptFn(void)
> util_ldap_cache_check_subgroups = APR_RETRIEVE_OPTIONAL_FN(uldap_cache_check_subgroups);
> }
>
> +
> +/** Cleanup LDAP connections before EOR. Note, if the authorization is unsuccessful,
> + * this will not run, but EOR is unlikely to be delayed as in a successful request.
> + */
> +static apr_status_t authnz_ldap_fixups(request_rec *r) {
> + authn_ldap_request_t *req =
> + (authn_ldap_request_t *)ap_get_module_config(r->request_config, &authnz_ldap_module);
> + if (req && req->ldc_pool) {
> + apr_pool_destroy(req->ldc_pool);
> + }
> + return OK;
> +}
> static void register_hooks(apr_pool_t *p)
> {
> /* Register authn provider */
> @@ -2056,6 +1986,7 @@ static void register_hooks(apr_pool_t *p
> AP_AUTH_INTERNAL_PER_CONF);
>
> ap_hook_post_config(authnz_ldap_post_config,NULL,NULL,APR_HOOK_MIDDLE);
> + ap_hook_fixups(authnz_ldap_fixups,NULL,NULL,APR_HOOK_MIDDLE);
>
> ap_hook_optional_fn_retrieve(ImportULDAPOptFn,NULL,NULL,APR_HOOK_MIDDLE);
> }
>
>
Regards,
Graham
—
Re: svn commit: r1608202 - in /httpd/httpd/trunk: CHANGES modules/aaa/mod_authnz_ldap.c
Posted by Eric Covener <co...@gmail.com>.
On Mon, Nov 10, 2014 at 4:49 PM, Graham Leggett <mi...@sharp.fm> wrote:
> I just tripped over this - ldc is passed to get_dn_for_nonldap_authn() where, still NULL, it triggers a segfault. The ldc is set later in the line above.
Sorry, see r1637990.