From: Graham Leggett Date: Thu, 20 May 2004 22:41:25 +0000 (+0000) Subject: Overhaul handling of LDAP error conditions, so that the util_ldap_* X-Git-Tag: pre_ajp_proxy~248 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=431e61008aa3d5d62480e45d767858ccc4391c23;p=apache Overhaul handling of LDAP error conditions, so that the util_ldap_* functions leave the connections in a sane state after errors have occurred. PR: 27748, 17274, 17599, 18661, 21787, 24595, 24683, 27134, 27271 Obtained from: Submitted by: Reviewed by: git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@103706 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/CHANGES b/CHANGES index 5c789d607a..26b754d955 100644 --- a/CHANGES +++ b/CHANGES @@ -2,6 +2,11 @@ Changes with Apache 2.1.0-dev [Remove entries to the current 2.0 section below, when backported] + *) Overhaul handling of LDAP error conditions, so that the util_ldap_* + functions leave the connections in a sane state after errors have + occurred. PR 27748, 17274, 17599, 18661, 21787, 24595, 24683, 27134, + 27271 [Graham Leggett] + *) RPM spec file changes: changed default dependancy to link to db4 instead of db3. Fixed complaints about unpackaged files. [Graham Leggett] diff --git a/include/util_ldap.h b/include/util_ldap.h index 483496cf49..1f3089fca1 100644 --- a/include/util_ldap.h +++ b/include/util_ldap.h @@ -155,14 +155,25 @@ LDAP_DECLARE(int) util_ldap_connection_open(request_rec *r, LDAP_DECLARE(void) util_ldap_connection_close(util_ldap_connection_t *ldc); /** - * Destroy a connection to an LDAP server + * Unbind a connection to an LDAP server + * @param ldc A structure containing the expanded details of the server + * that was connected. + * @tip This function unbinds the LDAP connection, and disconnects from + * the server. It is used during error conditions, to bring the LDAP + * connection back to a known state. + * @deffunc apr_status_t util_ldap_connection_unbind(util_ldap_connection_t *ldc) + */ +LDAP_DECLARE_NONSTD(apr_status_t) util_ldap_connection_unbind(void *param); + +/** + * Cleanup a connection to an LDAP server * @param ldc A structure containing the expanded details of the server * that was connected. * @tip This function is registered with the pool cleanup to close down the * LDAP connections when the server is finished with them. - * @deffunc apr_status_t util_ldap_connection_destroy(util_ldap_connection_t *ldc) + * @deffunc apr_status_t util_ldap_connection_cleanup(util_ldap_connection_t *ldc) */ -LDAP_DECLARE_NONSTD(apr_status_t) util_ldap_connection_destroy(void *param); +LDAP_DECLARE_NONSTD(apr_status_t) util_ldap_connection_cleanup(void *param); /** * Find a connection in a list of connections diff --git a/modules/experimental/NWGNUauthldap b/modules/experimental/NWGNUauthldap index f4f2c22b7f..6115ccbe2c 100644 --- a/modules/experimental/NWGNUauthldap +++ b/modules/experimental/NWGNUauthldap @@ -209,7 +209,8 @@ FILE_nlm_copyright = FILES_nlm_Ximports = \ util_ldap_connection_find \ util_ldap_connection_close \ - util_ldap_connection_destroy \ + util_ldap_connection_unbind \ + util_ldap_connection_cleanup \ util_ldap_cache_checkuserid \ util_ldap_cache_compare \ util_ldap_cache_comparedn \ diff --git a/modules/experimental/NWGNUutilldap b/modules/experimental/NWGNUutilldap index 5093964cd2..aa15476e4e 100644 --- a/modules/experimental/NWGNUutilldap +++ b/modules/experimental/NWGNUutilldap @@ -223,7 +223,8 @@ FILES_nlm_exports = \ ldap_module \ util_ldap_connection_find \ util_ldap_connection_close \ - util_ldap_connection_destroy \ + util_ldap_connection_unbind \ + util_ldap_connection_cleanup \ util_ldap_cache_checkuserid \ util_ldap_cache_compare \ util_ldap_cache_comparedn \ diff --git a/modules/experimental/mod_auth_ldap.c b/modules/experimental/mod_auth_ldap.c index b2fed9f694..400aea8da3 100644 --- a/modules/experimental/mod_auth_ldap.c +++ b/modules/experimental/mod_auth_ldap.c @@ -329,7 +329,6 @@ start_over: /* sanity check - if server is down, retry it up to 5 times */ if (result == LDAP_SERVER_DOWN) { - util_ldap_connection_destroy(ldc); if (failures++ <= 5) { goto start_over; } diff --git a/modules/experimental/util_ldap.c b/modules/experimental/util_ldap.c index 7b57c26b31..0c755c3f10 100644 --- a/modules/experimental/util_ldap.c +++ b/modules/experimental/util_ldap.c @@ -185,44 +185,53 @@ LDAP_DECLARE(void) util_ldap_connection_close(util_ldap_connection_t *ldc) /* - * Destroys an LDAP connection by unbinding. This function is registered - * with the pool cleanup function - causing the LDAP connections to be - * shut down cleanly on graceful restart. + * Destroys an LDAP connection by unbinding and closing the connection to + * the LDAP server. It is used to bring the connection back to a known + * state after an error, and during pool cleanup. */ -LDAP_DECLARE_NONSTD(apr_status_t) util_ldap_connection_destroy(void *param) +LDAP_DECLARE_NONSTD(apr_status_t) util_ldap_connection_unbind(void *param) { util_ldap_connection_t *ldc = param; if (ldc) { - - /* unbinding from the LDAP server */ if (ldc->ldap) { ldap_unbind_s(ldc->ldap); - ldc->bound = 0; ldc->ldap = NULL; } + ldc->bound = 0; + } + + return APR_SUCCESS; +} + + +/* + * Clean up an LDAP connection by unbinding and unlocking the connection. + * This function is registered with the pool cleanup function - causing + * the LDAP connections to be shut down cleanly on graceful restart. + */ +LDAP_DECLARE_NONSTD(apr_status_t) util_ldap_connection_cleanup(void *param) +{ + util_ldap_connection_t *ldc = param; + + if (ldc) { + + /* unbind and disconnect from the LDAP server */ + util_ldap_connection_unbind(ldc); + /* free the username and password */ if (ldc->bindpw) { free((void*)ldc->bindpw); } - if (ldc->binddn) { free((void*)ldc->binddn); } - /* release the lock we were using. The lock should have - already been released in the close connection call. - But just in case it wasn't, we first try to get the lock - before unlocking it to avoid unlocking an unheld lock. - Unlocking an unheld lock causes problems on NetWare. The - other option would be to assume that close connection did - its job. */ -#if APR_HAS_THREADS - apr_thread_mutex_trylock(ldc->lock); - apr_thread_mutex_unlock(ldc->lock); -#endif - + /* unlock this entry */ + util_ldap_connection_close(ldc); + } + return APR_SUCCESS; } @@ -342,10 +351,10 @@ LDAP_DECLARE(int) util_ldap_connection_open(request_rec *r, ldc->bound = 0; ldc->reason = "LDAP: ldap_simple_bind_s() failed"; } - else { - ldc->bound = 1; - ldc->reason = "LDAP: connection open successful"; - } + else { + ldc->bound = 1; + ldc->reason = "LDAP: connection open successful"; + } return(result); } @@ -461,7 +470,7 @@ LDAP_DECLARE(util_ldap_connection_t *)util_ldap_connection_find(request_rec *r, /* add the cleanup to the pool */ apr_pool_cleanup_register(l->pool, l, - util_ldap_connection_destroy, + util_ldap_connection_cleanup, apr_pool_cleanup_null); if (p) { @@ -565,8 +574,8 @@ start_over: if ((result = ldap_search_ext_s(ldc->ldap, const_cast(reqdn), LDAP_SCOPE_BASE, "(objectclass=*)", NULL, 1, NULL, NULL, NULL, -1, &res)) == LDAP_SERVER_DOWN) { - util_ldap_connection_close(ldc); ldc->reason = "DN Comparison ldap_search_ext_s() failed with server down"; + util_ldap_connection_unbind(ldc); goto start_over; } if (result != LDAP_SUCCESS) { @@ -694,8 +703,8 @@ start_over: if ((result = ldap_compare_s(ldc->ldap, const_cast(dn), const_cast(attrib), const_cast(value))) == LDAP_SERVER_DOWN) { /* connection failed - try again */ - util_ldap_connection_close(ldc); ldc->reason = "ldap_compare_s() failed with server down"; + util_ldap_connection_unbind(ldc); goto start_over; } @@ -815,6 +824,7 @@ start_over: const_cast(filter), attrs, 0, NULL, NULL, NULL, -1, &res)) == LDAP_SERVER_DOWN) { ldc->reason = "ldap_search_ext_s() for user failed with server down"; + util_ldap_connection_unbind(ldc); goto start_over; } @@ -869,6 +879,7 @@ start_over: LDAP_SERVER_DOWN) { ldc->reason = "ldap_simple_bind_s() to check user credentials failed with server down"; ldap_msgfree(res); + util_ldap_connection_unbind(ldc); goto start_over; } @@ -876,22 +887,17 @@ start_over: if (result != LDAP_SUCCESS) { ldc->reason = "ldap_simple_bind_s() to check user credentials failed"; ldap_msgfree(res); - ldap_unbind_s(ldc->ldap); - ldc->ldap = NULL; - ldc->bound = 0; + util_ldap_connection_unbind(ldc); return result; } else { /* - * Since we just bound the connection to the authenticating user id, update the - * ldc->binddn and ldc->bindpw to reflect the change and also to allow the next - * call to util_ldap_connection_open() to handle the connection reuse appropriately. - * Otherwise the next time that this connection is reused, it will indicate that - * it is bound to the original user id specified ldc->binddn when in fact it is - * bound to a completely different user id. + * We have just bound the connection to a different user and password + * combination, which might be reused unintentionally next time this + * connection is used from the connection pool. To ensure no confusion, + * we mark the connection as unbound. */ - util_ldap_strdup((char**)&(ldc->binddn), *binddn); - util_ldap_strdup((char**)&(ldc->bindpw), bindpw); + ldc->bound = 0; } /* @@ -938,6 +944,7 @@ start_over: return LDAP_SUCCESS; } + /* * Reports if ssl support is enabled *