From: Nick Porter Date: Fri, 24 Sep 2021 20:43:43 +0000 (+0100) Subject: v4: Async LDAP connection fixes (#4240) X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=46c7d832d5256da826fb3abc042a22ef3aabe14f;p=thirdparty%2Ffreeradius-server.git v4: Async LDAP connection fixes (#4240) * Improve log message for successful LDAP async bind * typo * net_timeout is still used * ldap_get_option can return LDAP_SUCCESS before the fd is known * If _ldap_bind_io_write was called without an fd look it up now * Store the connection fd for setting trunk events * Signal the connection once the bind has succeeded * Setting LDAP_OPT_NETWORK_TIMEOUT actually stops async calls * log_prefix should be const * Associate connection state handle with ldap connection * Add error message if LDAP connection does not allocate * Explain use of net_timeout * Explain behavior of ldap_get_option * Explain why we may call _ldap_bind_io_write() without a writable fd Co-authored-by: Arran Cudbard-Bell --- diff --git a/src/lib/ldap/base.h b/src/lib/ldap/base.h index 21d0627612..bfa7723cdb 100644 --- a/src/lib/ldap/base.h +++ b/src/lib/ldap/base.h @@ -302,6 +302,8 @@ typedef struct { fr_ldap_state_t state; //!< LDAP connection state machine. + int fd; //!< File descriptor for this connection. + void *uctx; //!< User data associated with the handle. } fr_ldap_connection_t; @@ -497,7 +499,7 @@ fr_ldap_rcode_t fr_ldap_sasl_interactive(request_t *request, fr_ldap_connection_t *fr_ldap_connection_alloc(TALLOC_CTX *ctx); fr_connection_t *fr_ldap_connection_state_alloc(TALLOC_CTX *ctx, fr_event_list_t *el, - fr_ldap_config_t const *config, char *log_prefix); + fr_ldap_config_t const *config, char const *log_prefix); int fr_ldap_connection_configure(fr_ldap_connection_t *c, fr_ldap_config_t const *config); diff --git a/src/lib/ldap/bind.c b/src/lib/ldap/bind.c index 6e72feeb7e..6cef0cf421 100644 --- a/src/lib/ldap/bind.c +++ b/src/lib/ldap/bind.c @@ -76,13 +76,11 @@ static void _ldap_bind_io_read(UNUSED fr_event_list_t *el, UNUSED int fd, UNUSED /* * We're I/O driven, if there's no data someone lied to us */ - status = fr_ldap_result(NULL, NULL, c, bind_ctx->msgid, LDAP_MSG_ALL, - bind_ctx->bind_dn, fr_time_delta_wrap(0)); - talloc_free(bind_ctx); /* Also removes fd events */ - + status = fr_ldap_result(NULL, NULL, c, bind_ctx->msgid, LDAP_MSG_ALL, bind_ctx->bind_dn, fr_time_delta_wrap(0)); switch (status) { case LDAP_PROC_SUCCESS: - DEBUG("Bind successful"); + DEBUG("Bind as \"%s\" to \"%s\" successful", + *bind_ctx->bind_dn? bind_ctx->bind_dn : "(anonymous)", c->config->server); fr_ldap_state_next(c); /* onto the next operation */ break; @@ -98,9 +96,10 @@ static void _ldap_bind_io_read(UNUSED fr_event_list_t *el, UNUSED int fd, UNUSED fr_ldap_state_error(c); /* Restart the connection state machine */ break; } + talloc_free(bind_ctx); /* Also removes fd events */ } -/** Send a bind request to a aserver +/** Send a bind request to a server * * @param[in] el the event occurred in. * @param[in] fd the event occurred on. @@ -123,12 +122,6 @@ static void _ldap_bind_io_write(fr_event_list_t *el, int fd, UNUSED int flags, v NUM_ELEMENTS(our_clientctrls), c, bind_ctx->serverctrls, bind_ctx->clientctrls); - /* - * Set timeout to be 0.0, which is the magic - * non-blocking value. - */ - (void) ldap_set_option(c->handle, LDAP_OPT_NETWORK_TIMEOUT, &fr_time_delta_to_timeval(fr_time_delta_wrap(0))); - if (bind_ctx->password) { memcpy(&cred.bv_val, &bind_ctx->password, sizeof(cred.bv_val)); cred.bv_len = talloc_array_length(bind_ctx->password) - 1; @@ -170,6 +163,11 @@ static void _ldap_bind_io_write(fr_event_list_t *el, int fd, UNUSED int flags, v break; case LDAP_SUCCESS: + if (fd < 0 ) { + ret = ldap_get_option(c->handle, LDAP_OPT_DESC, &fd); + if ((ret != LDAP_OPT_SUCCESS) || (fd < 0)) goto error; + } + c->fd = fd; ret = fr_event_fd_insert(bind_ctx, el, fd, _ldap_bind_io_read, NULL, @@ -220,7 +218,11 @@ int fr_ldap_bind_async(fr_ldap_connection_t *c, el = c->conn->el; - if (ldap_get_option(c->handle, LDAP_OPT_DESC, &fd) == LDAP_SUCCESS) { + /* + * ldap_get_option can return a LDAP_SUCCESS even if the fd is not yet available + * - hence the test for fd >= 0 + */ + if ((ldap_get_option(c->handle, LDAP_OPT_DESC, &fd) == LDAP_SUCCESS) && (fd >= 0)) { int ret; ret = fr_event_fd_insert(bind_ctx, el, fd, @@ -233,6 +235,11 @@ int fr_ldap_bind_async(fr_ldap_connection_t *c, return -1; } } else { + /* + * Connections initialised with ldap_init() do not have a fd until + * the first request (usually bind) occurs - so this code path + * starts the bind process to open the connection. + */ _ldap_bind_io_write(el, -1, 0, bind_ctx); } diff --git a/src/lib/ldap/connection.c b/src/lib/ldap/connection.c index 99ad6d5ea6..410fb30d15 100644 --- a/src/lib/ldap/connection.c +++ b/src/lib/ldap/connection.c @@ -448,6 +448,7 @@ static fr_connection_state_t _ldap_connection_init(void **h, fr_connection_t *co fr_ldap_state_t state; c = fr_ldap_connection_alloc(conn); + c->conn = conn; /* * Configure/allocate the libldap handle @@ -478,7 +479,7 @@ static fr_connection_state_t _ldap_connection_init(void **h, fr_connection_t *co * @param[in] log_prefix to prepend to connection state messages. */ fr_connection_t *fr_ldap_connection_state_alloc(TALLOC_CTX *ctx, fr_event_list_t *el, - fr_ldap_config_t const *config, char *log_prefix) + fr_ldap_config_t const *config, char const *log_prefix) { fr_connection_t *conn; @@ -492,7 +493,10 @@ fr_connection_t *fr_ldap_connection_state_alloc(TALLOC_CTX *ctx, fr_event_list_t .reconnection_delay = config->reconnection_delay }, log_prefix, config); - if (!conn) return NULL; + if (!conn) { + PERROR("Failed allocating state handler for new LDAP connection"); + return NULL; + } return conn; } diff --git a/src/lib/ldap/state.c b/src/lib/ldap/state.c index b9996147d4..92da48045c 100644 --- a/src/lib/ldap/state.c +++ b/src/lib/ldap/state.c @@ -109,12 +109,7 @@ again: */ case FR_LDAP_STATE_BIND: STATE_TRANSITION(FR_LDAP_STATE_RUN); - /* - if (fr_ldap_mux_async(c) < 0) { - STATE_TRANSITION(FR_LDAP_STATE_ERROR); - goto again; - } - */ + fr_connection_signal_connected(c->conn); break; /* diff --git a/src/modules/rlm_ldap/rlm_ldap.c b/src/modules/rlm_ldap/rlm_ldap.c index 6d97e43851..777755077b 100644 --- a/src/modules/rlm_ldap/rlm_ldap.c +++ b/src/modules/rlm_ldap/rlm_ldap.c @@ -150,8 +150,12 @@ static CONF_PARSER option_config[] = { { FR_CONF_OFFSET("sasl_secprops", FR_TYPE_STRING, rlm_ldap_t, handle_config.sasl_secprops) }, #ifdef LDAP_OPT_NETWORK_TIMEOUT - /* timeout on network activity */ - { FR_CONF_DEPRECATED("net_timeout", FR_TYPE_TIME_DELTA, rlm_ldap_t, handle_config.net_timeout), .dflt = "10" }, + /* + * We use this config option to populate libldap's LDAP_OPT_NETWORK_TIMEOUT - + * timeout on network activity - specifically libldap's initial call to "connect" + * Must be non-zero for async connections to start correctly. + */ + { FR_CONF_OFFSET("net_timeout", FR_TYPE_TIME_DELTA, rlm_ldap_t, handle_config.net_timeout), .dflt = "10" }, #endif #ifdef LDAP_OPT_X_KEEPALIVE_IDLE