If we don't re-connect, it's an error.
[freeradius.git] / src / modules / rlm_ldap / rlm_ldap.c
index b4e3559..d3fddbf 100644 (file)
@@ -1,10 +1,7 @@
 /*
- * rlm_ldap.c  LDAP authorization and authentication module.
- *
- *   This program is free software; you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
+ *   This program is is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License, version 2 if the
+ *   License as published by the Free Software Foundation.
  *
  *   This program is distributed in the hope that it will be useful,
  *   but WITHOUT ANY WARRANTY; without even the implied warranty of
  *   You should have received a copy of the GNU General Public License
  *   along with this program; if not, write to the Free Software
  *   Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+ */
+/**
+ * $Id$
+ * @file rlm_ldap.c
+ * @brief LDAP authorization and authentication module.
  *
- *   Copyright 1999-2012 The FreeRADIUS Server Project.
- *
- *   Copyright 2012 Alan DeKok <aland@freeradius.org>
- *   Copyright 2012 Arran Cudbard-Bell <a.cudbardb@freeradius.org>
+ * @copyright 1999-2013 The FreeRADIUS Server Project.
+ * @copyright 2012 Alan DeKok <aland@freeradius.org>
+ * @copyright 2012-2013 Arran Cudbard-Bell <a.cudbardb@freeradius.org>
  */
-
 #include <freeradius-devel/ident.h>
 RCSID("$Id$")
 
@@ -66,16 +67,17 @@ typedef struct {
        int             chase_referrals;
        int             rebind;
 
-       int             ldap_debug; /* Debug flag for LDAP SDK */
+       int             ldap_debug; //!< Debug flag for the SDK.
 
-       const char      *xlat_name; /* name used to xlat */
+       const char      *xlat_name; //!< Instance name.
 
        int             expect_password;
        
        /*
         *      RADIUS attribute to LDAP attribute maps
         */
-       VALUE_PAIR_MAP  *user_map;      /* Applied to users and profiles */
+       value_pair_map_t *user_map; //!< Attribute map applied to users and
+                                   //!< profiles.
        
        /*
         *      Access related configuration
@@ -329,7 +331,7 @@ typedef struct ldap_conn {
 } LDAP_CONN;
 
 typedef struct xlat_attrs {
-       const VALUE_PAIR_MAP *maps;
+       const value_pair_map_t *maps;
        const char *attrs[MAX_ATTRMAP];
 } xlat_attrs_t;
 
@@ -338,15 +340,17 @@ typedef struct rlm_ldap_result {
        int     count;
 } rlm_ldap_result_t;
 
-#define LDAP_PROC_SUCCESS 0
-#define LDAP_PROC_ERROR        -1
-#define LDAP_PROC_RETRY        -2
-#define LDAP_PROC_REJECT -3
+typedef enum {
+       LDAP_PROC_SUCCESS = 0,
+       LDAP_PROC_ERROR = -1,
+       LDAP_PROC_RETRY = -2,
+       LDAP_PROC_REJECT = -3
+} ldap_rcode_t;
 
-static int process_ldap_errno(ldap_instance *inst, LDAP_CONN **pconn,
+static ldap_rcode_t process_ldap_errno(ldap_instance *inst, LDAP_CONN **pconn,
                              const char *operation)
 {
-       int     ldap_errno;
+       int ldap_errno;
        
        ldap_get_option((*pconn)->handle, LDAP_OPT_ERROR_NUMBER,
                        &ldap_errno);
@@ -355,12 +359,6 @@ static int process_ldap_errno(ldap_instance *inst, LDAP_CONN **pconn,
        case LDAP_NO_SUCH_OBJECT:
                return LDAP_PROC_SUCCESS;
 
-       case LDAP_SERVER_DOWN:
-       do_reconnect:
-               *pconn = fr_connection_reconnect(inst->pool, *pconn);
-               if (!*pconn) return -1;
-               return LDAP_PROC_RETRY;
-
        case LDAP_INSUFFICIENT_ACCESS:
                radlog(L_ERR, "rlm_ldap (%s): %s failed: Insufficient access. "
                       "Check the identity and password configuration "
@@ -381,6 +379,7 @@ static int process_ldap_errno(ldap_instance *inst, LDAP_CONN **pconn,
 
        case LDAP_TIMELIMIT_EXCEEDED:
                exec_trigger(NULL, inst->cs, "modules.ldap.timeout", TRUE);
+               /* FALL-THROUGH */
 
        case LDAP_BUSY:
        case LDAP_UNAVAILABLE:
@@ -390,12 +389,18 @@ static int process_ldap_errno(ldap_instance *inst, LDAP_CONN **pconn,
                 */
                radlog(L_ERR, "rlm_ldap (%s): %s failed: %s",
                       inst->xlat_name, operation, ldap_err2string(ldap_errno));
-               goto do_reconnect;
+       case LDAP_SERVER_DOWN:
+               return LDAP_PROC_RETRY;
                
        case LDAP_INVALID_CREDENTIALS:
        case LDAP_CONSTRAINT_VIOLATION:
                return LDAP_PROC_REJECT;
 
+       case LDAP_OPERATIONS_ERROR:
+               DEBUGW("Please set 'chase_referrals=yes' and 'rebind=yes'");
+               DEBUGW("See the ldap module configuration for details");
+               /* FALL-THROUGH */
+
        default:
                radlog(L_ERR, "rlm_ldap (%s): %s failed: %s",
                       inst->xlat_name, operation, ldap_err2string(ldap_errno));
@@ -414,7 +419,7 @@ static int ldap_bind_wrapper(LDAP_CONN **pconn, const char *user,
        LDAPMessage     *result = NULL;
        struct timeval tv;
 
-bind:
+retry:
        msg_id = ldap_bind(conn->handle, user, password, LDAP_AUTH_SIMPLE);
        if (msg_id < 0) goto get_error;
 
@@ -454,7 +459,10 @@ error:
        
                                break;
                        case LDAP_PROC_RETRY:
-                               if (retry) goto bind;
+                               if (retry) {
+                                       *pconn = fr_connection_reconnect(inst->pool, *pconn);
+                                       if (*pconn) goto retry;
+                               }
                                
                                module_rcode = RLM_MODULE_FAIL;
                                break;
@@ -563,11 +571,13 @@ static void *ldap_conn_create(void *ctx)
                        do_ldap_option(LDAP_OPT_REFERRALS, "chase_referrals",
                                       LDAP_OPT_ON);
                        
-#if LDAP_SET_REBIND_PROC_ARGS == 3
                        if (inst->rebind == 1) {
+#if LDAP_SET_REBIND_PROC_ARGS == 3
                                ldap_set_rebind_proc(handle, ldap_rebind, inst);
-                       }
+#else
+                               DEBUGW("The flag 'rebind = yes' is not supported by the system LDAP library.  Ignoring.");
 #endif
+                       }
                } else {
                        do_ldap_option(LDAP_OPT_REFERRALS, "chase_referrals",
                                       LDAP_OPT_OFF);
@@ -650,7 +660,7 @@ static void *ldap_conn_create(void *ctx)
        }
 #endif /* HAVE_LDAP_START_TLS */
 
-       conn = rad_malloc(sizeof(*conn));
+       conn = talloc(NULL, LDAP_CONN);
        conn->inst = inst;
        conn->handle = handle;
        conn->rebound = FALSE;
@@ -674,7 +684,7 @@ static int ldap_conn_delete(UNUSED void *ctx, void *connection)
        LDAP_CONN *conn = connection;
 
        ldap_unbind_s(conn->handle);
-       free(conn);
+       talloc_free(conn);
 
        return 0;
 }
@@ -789,7 +799,6 @@ static int perform_search(ldap_instance *inst, REQUEST *request,
 {
        int             ldap_errno;
        int             count = 0;
-       LDAP_CONN       *conn = *pconn;
        struct timeval  tv;
 
        /*
@@ -804,16 +813,15 @@ static int perform_search(ldap_instance *inst, REQUEST *request,
        /*
         *      Do all searches as the default admin user.
         */
-       if (conn->rebound) {
+       if ((*pconn)->rebound) {
                ldap_errno = ldap_bind_wrapper(pconn, inst->login,
                                               inst->password, TRUE);
                if (ldap_errno != RLM_MODULE_OK) {
                        return -1;
                }
 
-               rad_assert(*pconn != NULL);
-               conn = *pconn;
-               conn->rebound = FALSE;
+               rad_assert(*pconn);
+               (*pconn)->rebound = FALSE;
        }
 
        tv.tv_sec = inst->timeout;
@@ -823,7 +831,7 @@ static int perform_search(ldap_instance *inst, REQUEST *request,
                filter);
 
 retry:
-       ldap_errno = ldap_search_ext_s(conn->handle, search_basedn, scope,
+       ldap_errno = ldap_search_ext_s((*pconn)->handle, search_basedn, scope,
                                       filter, search_attrs, 0, NULL, NULL,
                                       &tv, 0, presult);
        if (ldap_errno != LDAP_SUCCESS) {
@@ -836,14 +844,15 @@ retry:
                        case LDAP_PROC_ERROR:
                                return -1;
                        case LDAP_PROC_RETRY:
-                               conn = *pconn;
-                               goto retry;
+                               *pconn = fr_connection_reconnect(inst->pool, *pconn);
+                               if (*pconn) goto retry;
+                               return -1;
                        default:
                                rad_assert(0);
                }
        }
                
-       count = ldap_count_entries(conn->handle, *presult);
+       count = ldap_count_entries((*pconn)->handle, *presult);
        if (count == 0) {
                ldap_msgfree(*presult);
                RDEBUG("Search returned no results");
@@ -1312,7 +1321,7 @@ check_attr:
        ldap_msgfree(result);
        ldap_release_socket(inst, conn);
 
-       if (!found){
+       if (!found) {
                RDEBUG("User is not a member of specified group");
                return 1;
        }
@@ -1320,94 +1329,19 @@ check_attr:
        return 0;
 }
 
-/** Parse update section
- *
- * Verify that the ldap update section makes sense, and add attribute names
- * to array of attributes for efficient querying later.
- */
-static int build_attrmap(CONF_SECTION *cs, VALUE_PAIR_MAP **head)
-{
-       const char *cs_list, *p;
-
-       request_refs_t request_def = REQUEST_CURRENT;
-       pair_lists_t list_def = PAIR_LIST_REQUEST;
-
-       CONF_ITEM *ci = cf_sectiontoitem(cs);
-       CONF_PAIR *cp;
-
-       unsigned int total = 0;
-       VALUE_PAIR_MAP **tail, *map;
-       *head = NULL;
-       tail = head;
-       
-       if (!cs) return 0;
-       
-       cs_list = p = cf_section_name2(cs);
-       if (cs_list) {
-               request_def = radius_request_name(&p, REQUEST_UNKNOWN);
-               if (request_def == REQUEST_UNKNOWN) {
-                       cf_log_err(ci, "rlm_ldap: Default request specified "
-                                  "in mapping section is invalid");
-                       return -1;
-               }
-               
-               list_def = fr_str2int(pair_lists, p, PAIR_LIST_UNKNOWN);
-               if (list_def == PAIR_LIST_UNKNOWN) {
-                       cf_log_err(ci, "rlm_ldap: Default list specified "
-                                  "in mapping section is invalid");
-                       return -1;
-               }
-       }
-
-       for (ci = cf_item_find_next(cs, NULL);
-            ci != NULL;
-            ci = cf_item_find_next(cs, ci)) {
-               if (total++ == MAX_ATTRMAP) {
-                       cf_log_err(ci, "rlm_ldap: Attribute map size exceeded");
-                       goto error;
-               }
-               
-               if (!cf_item_is_pair(ci)) {
-                       cf_log_err(ci, "rlm_ldap: Entry is not in \"attribute ="
-                                      " ldap-attribute\" format");
-                       goto error;
-               }
-       
-               cp = cf_itemtopair(ci);
-               map = radius_cp2map(cp, REQUEST_CURRENT, list_def);
-               if (!map) {
-                       goto error;
-               }
-               
-               *tail = map;
-               tail = &(map->next);
-       }
-
-       return 0;
-       
-       error:
-               radius_mapfree(head);
-               return -1;
-}
-
 /** Detach from the LDAP server and cleanup internal state.
  *
  */
 static int ldap_detach(void *instance)
 {
        ldap_instance *inst = instance;
-
-       if (inst->postauth) free(inst->postauth);
-       if (inst->accounting) free(inst->accounting);
        
        fr_connection_pool_delete(inst->pool);
-       
+
        if (inst->user_map) {
                radius_mapfree(&inst->user_map);
        }
 
-       free(inst);
-
        return 0;
 }
 
@@ -1429,14 +1363,10 @@ static int parse_sub_section(CONF_SECTION *parent,
                return 0;
        }
        
-       *config = rad_calloc(sizeof(**config));
+       *config = talloc_zero(inst, ldap_acct_section_t);
        if (cf_section_parse(cs, *config, acct_section_config) < 0) {
                radlog(L_ERR, "rlm_ldap (%s): Failed parsing configuration for "
                       "section %s", inst->xlat_name, name);
-               
-               free(*config);
-               *config = NULL;
-               
                return -1;
        }
                
@@ -1445,6 +1375,65 @@ static int parse_sub_section(CONF_SECTION *parent,
        return 0;
 }
 
+static int ldap_map_verify(ldap_instance *inst, value_pair_map_t **head)
+{
+       value_pair_map_t *map;
+       
+       if (radius_attrmap(inst->cs, head, PAIR_LIST_REPLY,
+                          PAIR_LIST_REQUEST, MAX_ATTRMAP) < 0) {
+               return -1;
+       }
+       /*
+        *      Attrmap only performs some basic validation checks, we need
+        *      to do rlm_ldap specific checks here.
+        */
+       for (map = *head; map != NULL; map = map->next) {
+               if (map->dst->type != VPT_TYPE_ATTR) {
+                       cf_log_err(map->ci, "Left operand must be an "
+                                    "attribute ref");
+                       
+                       return -1;
+               }
+               
+               if (map->src->type == VPT_TYPE_LIST) {
+                       cf_log_err(map->ci, "Right operand must not be "
+                                    "a list");
+                       
+                       return -1;
+               }
+               
+               switch (map->src->type) 
+               {
+               /*
+                *      Only =, :=, += and -= operators are supported for
+                *      cache entries.
+                */
+               case VPT_TYPE_LITERAL:
+               case VPT_TYPE_XLAT:
+               case VPT_TYPE_ATTR:
+                       switch (map->op) {
+                       case T_OP_SET:
+                       case T_OP_EQ:
+                       case T_OP_SUB:
+                       case T_OP_ADD:
+                               break;
+               
+                       default:
+                               cf_log_err(map->ci, "Operator \"%s\" not "
+                                          "allowed for %s values",
+                                          fr_int2str(fr_tokens, map->op,
+                                                     "¿unknown?"),
+                                          fr_int2str(vpt_types, map->src->type,
+                                                     "¿unknown?"));
+                               return -1;
+                       }
+               default:
+                       break;
+               }
+       }
+       return 0;
+}
+
 /** Parses config
  * Uses section of radiusd config file passed as parameter to create an
  * instance of the module.
@@ -1452,9 +1441,10 @@ static int parse_sub_section(CONF_SECTION *parent,
 static int ldap_instantiate(CONF_SECTION * conf, void **instance)
 {
        ldap_instance *inst;
-       CONF_SECTION *cs;
 
-       inst = rad_calloc(sizeof *inst);
+       *instance = inst = talloc_zero(conf, ldap_instance);
+       if (!inst) return -1;
+
        inst->cs = conf;
 
        inst->chase_referrals = 2; /* use OpenLDAP defaults */
@@ -1464,8 +1454,6 @@ static int ldap_instantiate(CONF_SECTION * conf, void **instance)
        if (!inst->xlat_name) {
                inst->xlat_name = cf_section_name1(conf);
        }
-               
-       rad_assert(inst->xlat_name);
 
        /*
         *      If the configuration parameters can't be parsed, then fail.
@@ -1529,11 +1517,8 @@ static int ldap_instantiate(CONF_SECTION * conf, void **instance)
        /*
         *      Build the attribute map
         */
-       cs = cf_section_sub_find(conf, "update");
-       if (cs) {       
-               if (build_attrmap(cs, &(inst->user_map)) < 0) {
-                       goto error;
-               }
+       if (ldap_map_verify(inst, &(inst->user_map)) < 0) {
+               goto error;
        }
 
        /*
@@ -1541,7 +1526,7 @@ static int ldap_instantiate(CONF_SECTION * conf, void **instance)
         */
        paircompare_register(PW_LDAP_GROUP, PW_USER_NAME, ldap_groupcmp, inst); 
        if (cf_section_name2(conf)) {
-               DICT_ATTR *da;
+               const DICT_ATTR *da;
                ATTR_FLAGS flags;
                char buffer[256];
 
@@ -1575,10 +1560,9 @@ static int ldap_instantiate(CONF_SECTION * conf, void **instance)
                return -1;
        }
        
-       *instance = inst;
        return 0;
-       
-       error:
+
+error:
        ldap_detach(inst);
        return -1;
 }
@@ -1618,7 +1602,7 @@ static int check_access(ldap_instance *inst, REQUEST* request, LDAP_CONN *conn,
 }
 
 
-static VALUE_PAIR *ldap_getvalue(REQUEST *request, const VALUE_PAIR_TMPL *dst,
+static VALUE_PAIR *ldap_getvalue(REQUEST *request, const value_pair_map_t *map,
                                 void *ctx)
 {
        rlm_ldap_result_t *self = ctx;
@@ -1636,7 +1620,7 @@ static VALUE_PAIR *ldap_getvalue(REQUEST *request, const VALUE_PAIR_TMPL *dst,
         *      just use whatever was set in the attribute map. 
         */
        for (i = 0; i < self->count; i++) {
-               vp = pairalloc(dst->da);
+               vp = pairalloc(NULL, map->dst->da);
                rad_assert(vp);
 
                pairparsevalue(vp, self->values[i]);
@@ -1651,7 +1635,7 @@ static VALUE_PAIR *ldap_getvalue(REQUEST *request, const VALUE_PAIR_TMPL *dst,
 
 static void xlat_attrsfree(const xlat_attrs_t *expanded)
 {
-       const VALUE_PAIR_MAP *map;
+       const value_pair_map_t *map;
        unsigned int total = 0;
        
        const char *name;
@@ -1661,44 +1645,81 @@ static void xlat_attrsfree(const xlat_attrs_t *expanded)
                name = expanded->attrs[total++];
                if (!name) return;
                
-               if (map->src->do_xlat) {
+               switch (map->src->type)
+               {
+               case VPT_TYPE_XLAT:             
+               case VPT_TYPE_ATTR:
                        rad_cfree(name);
+                       break;
+               default:
+                       break;
                }
        }
 }
 
 
-static int xlat_attrs(REQUEST *request, const VALUE_PAIR_MAP *maps,
+static int xlat_attrs(REQUEST *request, const value_pair_map_t *maps,
                      xlat_attrs_t *expanded)
 {
-       const VALUE_PAIR_MAP *map;
+       const value_pair_map_t *map;
        unsigned int total = 0;
        
        size_t len;
        char *buffer;
 
+       VALUE_PAIR *found, **from = NULL;
+       REQUEST *context;
+
        for (map = maps; map != NULL; map = map->next)
        {
-               if (map->src->do_xlat) {
+               switch (map->src->type)
+               {
+               case VPT_TYPE_XLAT:
                        buffer = rad_malloc(MAX_ATTR_STR_LEN);
                        len = radius_xlat(buffer, MAX_ATTR_STR_LEN,
                                          map->src->name, request, NULL, NULL);
                                          
-                       if (!len) {
+                       if (len <= 0) {
                                RDEBUG("Expansion of LDAP attribute "
                                       "\"%s\" failed", map->src->name);
                                       
-                               expanded->attrs[total] = NULL;
-                               
-                               xlat_attrsfree(expanded);
-                               
-                               return -1;
+                               goto error;
                        }
                        
                        expanded->attrs[total++] = buffer;
-               } else {
+                       break;
+
+               case VPT_TYPE_ATTR:
+                       context = request;
+                       
+                       if (radius_request(&context, map->src->request) == 0) {
+                               from = radius_list(context, map->src->list);
+                       }
+                       if (!from) continue;
+                       
+                       found = pairfind(*from, map->src->da->attr,
+                                        map->src->da->vendor, TAG_ANY);
+                       if (!found) continue;
+                       
+                       buffer = rad_malloc(MAX_ATTR_STR_LEN);
+                       strlcpy(buffer, found->vp_strvalue, MAX_ATTR_STR_LEN);
+                       
+                       expanded->attrs[total++] = buffer;
+                       break;
+                       
+               case VPT_TYPE_LITERAL:
                        expanded->attrs[total++] = map->src->name;
+                       break;
+               default:
+                       rad_assert(0);
+               error:
+                       expanded->attrs[total] = NULL;
+                       
+                       xlat_attrsfree(expanded);
+                       
+                       return -1;
                }
+                       
        }
        
        expanded->attrs[total] = NULL;
@@ -1720,7 +1741,7 @@ static void do_attrmap(UNUSED ldap_instance *inst, REQUEST *request,
                       LDAP *handle, const xlat_attrs_t *expanded,
                       LDAPMessage *entry)
 {
-       const VALUE_PAIR_MAP    *map;
+       const value_pair_map_t  *map;
        unsigned int            total = 0;
        
        rlm_ldap_result_t       result;
@@ -1769,14 +1790,14 @@ static void do_check_reply(ldap_instance *inst, REQUEST *request)
        */
        if (inst->expect_password && (debug_flag > 1)) {
                if (!pairfind(request->config_items, PW_CLEARTEXT_PASSWORD, 0, TAG_ANY) &&
-                       !pairfind(request->config_items, PW_NT_PASSWORD, 0, TAG_ANY) &&
-                       !pairfind(request->config_items, PW_USER_PASSWORD, 0, TAG_ANY) &&
-                       !pairfind(request->config_items, PW_PASSWORD_WITH_HEADER, 0, TAG_ANY) &&
-                       !pairfind(request->config_items, PW_CRYPT_PASSWORD, 0, TAG_ANY)) {
-                               RDEBUG("WARNING: No \"known good\" password "
-                                      "was found in LDAP.  Are you sure that "
-                                      "the user is configured correctly?");
-              }
+                   !pairfind(request->config_items, PW_NT_PASSWORD, 0, TAG_ANY) &&
+                   !pairfind(request->config_items, PW_USER_PASSWORD, 0, TAG_ANY) &&
+                   !pairfind(request->config_items, PW_PASSWORD_WITH_HEADER, 0, TAG_ANY) &&
+                   !pairfind(request->config_items, PW_CRYPT_PASSWORD, 0, TAG_ANY)) {
+                       RDEBUGW("No \"known good\" password "
+                              "was found in LDAP.  Are you sure that "
+                               "the user is configured correctly?");
+               }
        }
 }
 
@@ -1955,13 +1976,12 @@ static rlm_rcode_t ldap_authorize(void *instance, REQUEST * request)
 
                /* Add Cleartext-Password attribute to the request */
                vp = radius_paircreate(request, &request->config_items,
-                                      PW_CLEARTEXT_PASSWORD, 0,
-                                      PW_TYPE_STRING);
+                                      PW_CLEARTEXT_PASSWORD, 0);
                strlcpy(vp->vp_strvalue, buffer, sizeof(vp->vp_strvalue));
                vp->length = strlen(vp->vp_strvalue);
                
                RDEBUG2("Added eDirectory password in check items as %s = %s",
-                       vp->name, vp->vp_strvalue);
+                       vp->da->name, vp->vp_strvalue);
                        
                if (inst->edir_autz) {
                        RDEBUG2("Binding as user for eDirectory authorization "
@@ -2072,10 +2092,10 @@ static rlm_rcode_t ldap_authenticate(void *instance, REQUEST * request)
                return RLM_MODULE_INVALID;
        }
 
-       if (request->password->attribute != PW_USER_PASSWORD) {
+       if (request->password->da->attr != PW_USER_PASSWORD) {
                radlog(L_AUTH, "rlm_ldap (%s): Attribute \"User-Password\" "
                       "is required for authentication. Cannot use \"%s\".",
-                      inst->xlat_name, request->password->name);
+                      inst->xlat_name, request->password->da->name);
                return RLM_MODULE_INVALID;
        }
 
@@ -2126,7 +2146,7 @@ static rlm_rcode_t user_modify(ldap_instance *inst, REQUEST *request,
        int             ldap_errno, rcode, msg_id;
        LDAPMessage     *result = NULL;
        
-       LDAP_CONN       *conn;
+       LDAP_CONN       *conn = NULL;
        
        LDAPMod         *mod_p[MAX_ATTRMAP + 1], mod_s[MAX_ATTRMAP];
        LDAPMod         **modify = mod_p;
@@ -2247,10 +2267,7 @@ static rlm_rcode_t user_modify(ldap_instance *inst, REQUEST *request,
                        passed[last_pass] = NULL;
                } else if (do_xlat) {
                        p = rad_malloc(1024);
-                       radius_xlat(p, 1024, value, request, NULL, NULL);
-                       
-                       if (*p == '\0') {
-                       
+                       if (radius_xlat(p, 1024, value, request, NULL, NULL) <= 0) {
                                RDEBUG("xlat failed or empty value string, "
                                       "skipping attribute \"%s\"", attr);
                                       
@@ -2277,32 +2294,35 @@ static rlm_rcode_t user_modify(ldap_instance *inst, REQUEST *request,
                
                switch (op)
                {
-                       /*
-                        *  T_OP_EQ is *NOT* supported, it is impossible to
-                        *  support because of the lack of transactions in LDAP
-                        */
-                       case T_OP_ADD:
-                               mod_s[total].mod_op = LDAP_MOD_ADD;
+               /*
+                *  T_OP_EQ is *NOT* supported, it is impossible to
+                *  support because of the lack of transactions in LDAP
+                */
+               case T_OP_ADD:
+                       mod_s[total].mod_op = LDAP_MOD_ADD;
                        break;
-                       case T_OP_SET:
-                               mod_s[total].mod_op = LDAP_MOD_REPLACE;
+
+               case T_OP_SET:
+                       mod_s[total].mod_op = LDAP_MOD_REPLACE;
                        break;
-                       case T_OP_SUB:
-                       case T_OP_CMP_FALSE:
-                               mod_s[total].mod_op = LDAP_MOD_DELETE;
+
+               case T_OP_SUB:
+               case T_OP_CMP_FALSE:
+                       mod_s[total].mod_op = LDAP_MOD_DELETE;
                        break;
+
 #ifdef LDAP_MOD_INCREMENT
-                       case T_OP_INCRM:
-                               mod_s[total].mod_op = LDAP_MOD_INCREMENT;
+               case T_OP_INCRM:
+                       mod_s[total].mod_op = LDAP_MOD_INCREMENT;
                        break;
 #endif
-                       default:
-                               radlog(L_ERR, "rlm_ldap (%s): Operator '%s' "
-                                      "is not supported for LDAP modify "
-                                      "operations", inst->xlat_name,
-                                      fr_int2str(fr_tokens, op, "¿unknown?"));
-                                      
-                               goto error;
+               default:
+                       radlog(L_ERR, "rlm_ldap (%s): Operator '%s' "
+                              "is not supported for LDAP modify "
+                              "operations", inst->xlat_name,
+                              fr_int2str(fr_tokens, op, "¿unknown?"));
+                              
+                       goto error;
                }
                
                /*
@@ -2330,9 +2350,8 @@ static rlm_rcode_t user_modify(ldap_instance *inst, REQUEST *request,
         *      Perform all modifications as the default admin user.
         */
        if (conn->rebound) {
-               ldap_errno = ldap_bind_wrapper(&conn,
-                                              inst->login, inst->password,
-                                              TRUE);
+               ldap_errno = ldap_bind_wrapper(&conn, inst->login,
+                                              inst->password, TRUE);
                if (ldap_errno != RLM_MODULE_OK) {
                        goto error;
                }