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 <a.cudbardb@freeradius.org>

diff --git a/src/lib/ldap/base.h b/src/lib/ldap/base.h
index 21d06276125b04dfc643d96dea04db9b30d7e295..bfa7723cdb781b6abe77e7fc0546264c192922d0 100644 (file)
--- 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 6e72feeb7eb80a38d43840e790d25c66cf2f9262..6cef0cf4210d9301338bded9bcbe64639fab1b56 100644 (file)
--- 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 99ad6d5ea6f0fedffde41da8b7cb69c3f75cb288..410fb30d1548f13fb7b8a7d6abe479994f5a577f 100644 (file)
--- 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 b9996147d4ef679a807a3ae62271d60c0547a0c9..92da48045cfd6fc433dbcaaf2305fd8907fd4097 100644 (file)
--- 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 6d97e438516ed6273e8f91a88aa68092707404a3..777755077b6767a1065c075e3691330062419891 100644 (file)
--- 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