[Asterisk-code-review] res config ldap: Fix erroneous LDAP MOD REPLACE in LDAP modify (asterisk[13])
Sean Bright
asteriskteam at digium.com
Fri Feb 17 11:53:15 CST 2017
Sean Bright has uploaded a new change for review. ( https://gerrit.asterisk.org/4994 )
Change subject: res_config_ldap: Fix erroneous LDAP_MOD_REPLACE in LDAP modify
......................................................................
res_config_ldap: Fix erroneous LDAP_MOD_REPLACE in LDAP modify
There are a few issues resolved here, the primary one being the issue
described in the associated JIRA issue. We always treat the first
change of our modification batch as a replacement when it sometimes is
actually a delete. So we have to pass the correct arguments to the
OpenLDAP library.
In setting this up in my development environment, I discovered a few
additional things that I resolved as well:
* OpenLDAP will raise an error when we try to delete an LDAP attribute
that doesn't exist. We need to filter out LDAP_MOD_DELETE requests
based on which attributes the current LDAP entry actually has. There
is of course a small window of opportunity for this to still fail,
but it is much less likely now.
* The code in update_ldap() and update2_ldap() was using both Asterisk's
memory allocation routines as well as OpenLDAP's. I've changed it so
that everything that is passed to OpenLDAP's functions are allocated
with their routines.
* The "_general" configuration section allows administrators to provide
both general configuration options (host, port, url, etc.) as well as
a global realtime-to-LDAP-attribute mapping that is a fallback if one
of the later sections do not override it. This neglected to exclude
the general configuration options from the mapping. As an example,
during my testing, chan_sip requested 'port' from realtime, and
because I did not have it defined, it pulled in the 'port'
configuration option from "_general." We now filter those out
explicitly.
ASTERISK-26580 #close
Reported by: Nicholas John Koch
Patches:
res_config_ldap.c-11.24.1.patch (license #6833) patch uploaded
by Nicholas John Koch
Change-Id: I0a534a60c7a5d79876bc75eff6588ce7db0d8c2e
---
M res/res_config_ldap.c
1 file changed, 154 insertions(+), 49 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/94/4994/2
diff --git a/res/res_config_ldap.c b/res/res_config_ldap.c
index fd21aab..3f21c8f 100644
--- a/res/res_config_ldap.c
+++ b/res/res_config_ldap.c
@@ -1219,6 +1219,89 @@
return cfg;
}
+/*!
+ * \internal
+ * \brief Remove LDAP_MOD_DELETE modifications that will not succeed
+ *
+ * \details
+ * A LDAP_MOD_DELETE operation will fail if the LDAP entry does not already have
+ * the corresponding attribute. Because we may be updating multiple LDAP entries
+ * in a single call to update_ldap(), we may need our own copy of the
+ * modifications array for each one.
+ *
+ * \note
+ * This function dynamically allocates memory. If it returns a non-NULL pointer,
+ * it is up to the caller to free it with ldap_mods_free()
+ *
+ * \returns an LDAPMod * if modifications needed to be removed, NULL otherwise.
+ */
+static LDAPMod **massage_mods_for_entry(LDAPMessage *entry, LDAPMod **mods, size_t count)
+{
+ size_t i;
+ int remove[count];
+ size_t remove_count = 0;
+
+ for (i = 0; i < count; i++) {
+ BerElement *ber = NULL;
+ char *attribute;
+ int exists = 0;
+
+ if (mods[i]->mod_op != LDAP_MOD_DELETE) {
+ continue;
+ }
+
+ /* If we are deleting something, it has to exist */
+ attribute = ldap_first_attribute(ldapConn, entry, &ber);
+ while (attribute) {
+ if (!strcasecmp(attribute, mods[i]->mod_type)) {
+ /* OK, we have the attribute */
+ exists = 1;
+ ldap_memfree(attribute);
+ break;
+ }
+
+ ldap_memfree(attribute);
+ attribute = ldap_next_attribute(ldapConn, entry, ber);
+ }
+
+ if (!exists) {
+ remove[remove_count++] = i;
+ }
+ }
+
+ if (remove_count) {
+ size_t k, remove_index;
+ LDAPMod **x = ldap_memcalloc(count - remove_count + 1, sizeof(LDAPMod *));
+ for (i = 0, k = 0; i < count; i++) {
+ int skip = 0;
+ /* Is this one we have to remove? */
+ for (remove_index = 0; !skip && remove_index < remove_count; remove_index++) {
+ skip = (remove[remove_index] == i);
+ }
+
+ if (skip) {
+ ast_debug(3, "Skipping %s deletion because it doesn't exist\n",
+ mods[i]->mod_type);
+ continue;
+ }
+
+ x[k] = ldap_memcalloc(1, sizeof(LDAPMod));
+ x[k]->mod_op = mods[i]->mod_op;
+ x[k]->mod_type = ldap_strdup(mods[i]->mod_type);
+ if (mods[i]->mod_values) {
+ x[k]->mod_values = ldap_memcalloc(2, sizeof(char *));
+ x[k]->mod_values[0] = ldap_strdup(mods[i]->mod_values[0]);
+ }
+ k++;
+ }
+ /* NULL terminate */
+ x[k] = NULL;
+ return x;
+ }
+
+ return NULL;
+}
+
/* \brief Function to update a set of values in ldap static mode
*/
static int update_ldap(const char *basedn, const char *table_name, const char *attribute,
@@ -1292,14 +1375,18 @@
}
mods_size = 2; /* one for the first param/value pair and one for the the terminating NULL */
- ldap_mods = ldap_memcalloc(sizeof(LDAPMod *), mods_size);
- ldap_mods[0] = ldap_memcalloc(1, sizeof(LDAPMod));
+ ldap_mods = ldap_memcalloc(mods_size, sizeof(LDAPMod *));
- ldap_mods[0]->mod_op = LDAP_MOD_REPLACE;
+ ldap_mods[0] = ldap_memcalloc(1, sizeof(LDAPMod));
ldap_mods[0]->mod_type = ldap_strdup(newparam);
- ldap_mods[0]->mod_values = ast_calloc(sizeof(char *), 2);
- ldap_mods[0]->mod_values[0] = ldap_strdup(field->value);
+ if (strlen(field->value) == 0) {
+ ldap_mods[0]->mod_op = LDAP_MOD_DELETE;
+ } else {
+ ldap_mods[0]->mod_op = LDAP_MOD_REPLACE;
+ ldap_mods[0]->mod_values = ldap_memcalloc(2, sizeof(char *));
+ ldap_mods[0]->mod_values[0] = ldap_strdup(field->value);
+ }
while ((field = field->next)) {
newparam = convert_attribute_name_to_ldap(table_config, field->name);
@@ -1311,7 +1398,7 @@
ldap_mods[i]->mod_values[0] = ldap_memrealloc(ldap_mods[i]->mod_values[0], sizeof(char) * (strlen(ldap_mods[i]->mod_values[0]) + strlen(field->value) + 2));
strcat(ldap_mods[i]->mod_values[0], ";");
strcat(ldap_mods[i]->mod_values[0], field->value);
- mod_exists = 1;
+ mod_exists = 1;
break;
}
}
@@ -1320,22 +1407,20 @@
if (!mod_exists) {
mods_size++;
ldap_mods = ldap_memrealloc(ldap_mods, sizeof(LDAPMod *) * mods_size);
- ldap_mods[mods_size - 1] = NULL;
-
- ldap_mods[mods_size - 2] = ldap_memcalloc(1, sizeof(LDAPMod));
- ldap_mods[mods_size - 2]->mod_type = ldap_memcalloc(sizeof(char), strlen(newparam) + 1);
- strcpy(ldap_mods[mods_size - 2]->mod_type, newparam);
+ ldap_mods[mods_size - 2] = ldap_memcalloc(1, sizeof(LDAPMod));
+ ldap_mods[mods_size - 2]->mod_type = ldap_strdup(newparam);
if (strlen(field->value) == 0) {
ldap_mods[mods_size - 2]->mod_op = LDAP_MOD_DELETE;
} else {
ldap_mods[mods_size - 2]->mod_op = LDAP_MOD_REPLACE;
-
- ldap_mods[mods_size - 2]->mod_values = ldap_memcalloc(sizeof(char *), 2);
- ldap_mods[mods_size - 2]->mod_values[0] = ldap_memcalloc(sizeof(char), strlen(field->value) + 1);
- strcpy(ldap_mods[mods_size - 2]->mod_values[0], field->value);
+ ldap_mods[mods_size - 2]->mod_values = ldap_memcalloc(2, sizeof(char *));
+ ldap_mods[mods_size - 2]->mod_values[0] = ldap_strdup(field->value);
}
+
+ /* NULL terminate */
+ ldap_mods[mods_size - 1] = NULL;
}
}
/* freeing ldap_mods further down */
@@ -1365,10 +1450,10 @@
ast_log(LOG_WARNING, "Query: %s\n", ast_str_buffer(filter));
ast_mutex_unlock(&ldap_lock);
- free(filter);
- free(clean_basedn);
+ ast_free(filter);
+ ast_free(clean_basedn);
ldap_msgfree(ldap_result_msg);
- ldap_mods_free(ldap_mods, 0);
+ ldap_mods_free(ldap_mods, 1);
return -1;
}
/* Ready to update */
@@ -1383,12 +1468,24 @@
}
ldap_entry = ldap_first_entry(ldapConn, ldap_result_msg);
- for (i = 0; ldap_entry; i++) {
+ for (i = 0; ldap_entry; i++) {
+ LDAPMod **working = ldap_mods;
+ LDAPMod **massaged = massage_mods_for_entry(ldap_entry, ldap_mods, mods_size - 1);
+
+ if (massaged) {
+ working = massaged;
+ }
+
dn = ldap_get_dn(ldapConn, ldap_entry);
- if ((error = ldap_modify_ext_s(ldapConn, dn, ldap_mods, NULL, NULL)) != LDAP_SUCCESS) {
+ if ((error = ldap_modify_ext_s(ldapConn, dn, working, NULL, NULL)) != LDAP_SUCCESS) {
ast_log(LOG_ERROR, "Couldn't modify '%s'='%s', dn:%s because %s\n",
attribute, lookup, dn, ldap_err2string(error));
}
+
+ if (massaged) {
+ ldap_mods_free(massaged, 1);
+ }
+
ldap_memfree(dn);
ldap_entry = ldap_next_entry(ldapConn, ldap_entry);
}
@@ -1398,7 +1495,7 @@
ast_free(filter);
ast_free(clean_basedn);
ldap_msgfree(ldap_result_msg);
- ldap_mods_free(ldap_mods, 0);
+ ldap_mods_free(ldap_mods, 1);
return num_entries;
}
@@ -1478,16 +1575,13 @@
}
mods_size = 2; /* one for the first param/value pair and one for the the terminating NULL */
- ldap_mods = ast_calloc(sizeof(LDAPMod *), mods_size);
- ldap_mods[0] = ast_calloc(1, sizeof(LDAPMod));
+ ldap_mods = ldap_memcalloc(mods_size, sizeof(LDAPMod *));
+ ldap_mods[0] = ldap_memcalloc(1, sizeof(LDAPMod));
ldap_mods[0]->mod_op = LDAP_MOD_REPLACE;
- ldap_mods[0]->mod_type = ast_calloc(sizeof(char), strlen(newparam) + 1);
- strcpy(ldap_mods[0]->mod_type, newparam);
-
- ldap_mods[0]->mod_values = ast_calloc(sizeof(char), 2);
- ldap_mods[0]->mod_values[0] = ast_calloc(sizeof(char), strlen(field->value) + 1);
- strcpy(ldap_mods[0]->mod_values[0], field->value);
+ ldap_mods[0]->mod_type = ldap_strdup(newparam);
+ ldap_mods[0]->mod_values = ldap_memcalloc(2, sizeof(char *));
+ ldap_mods[0]->mod_values[0] = ldap_strdup(field->value);
while ((field = field->next)) {
newparam = convert_attribute_name_to_ldap(table_config, field->name);
@@ -1496,10 +1590,10 @@
for (i = 0; i < mods_size - 1; i++) {
if (ldap_mods[i]&& !strcmp(ldap_mods[i]->mod_type, newparam)) {
/* We have the parameter allready, adding the value as a semicolon delimited value */
- ldap_mods[i]->mod_values[0] = ast_realloc(ldap_mods[i]->mod_values[0], sizeof(char) * (strlen(ldap_mods[i]->mod_values[0]) + strlen(field->value) + 2));
+ ldap_mods[i]->mod_values[0] = ldap_memrealloc(ldap_mods[i]->mod_values[0], sizeof(char) * (strlen(ldap_mods[i]->mod_values[0]) + strlen(field->value) + 2));
strcat(ldap_mods[i]->mod_values[0], ";");
strcat(ldap_mods[i]->mod_values[0], field->value);
- mod_exists = 1;
+ mod_exists = 1;
break;
}
}
@@ -1507,18 +1601,16 @@
/* create new mod */
if (!mod_exists) {
mods_size++;
- ldap_mods = ast_realloc(ldap_mods, sizeof(LDAPMod *) * mods_size);
- ldap_mods[mods_size - 1] = NULL;
- ldap_mods[mods_size - 2] = ast_calloc(1, sizeof(LDAPMod));
+ ldap_mods = ldap_memrealloc(ldap_mods, sizeof(LDAPMod *) * mods_size);
+ ldap_mods[mods_size - 2] = ldap_memcalloc(1, sizeof(LDAPMod));
ldap_mods[mods_size - 2]->mod_op = LDAP_MOD_REPLACE;
+ ldap_mods[mods_size - 2]->mod_type = ldap_strdup(newparam);
+ ldap_mods[mods_size - 2]->mod_values = ldap_memcalloc(2, sizeof(char *));
+ ldap_mods[mods_size - 2]->mod_values[0] = ldap_strdup(field->value);
- ldap_mods[mods_size - 2]->mod_type = ast_calloc(sizeof(char), strlen(newparam) + 1);
- strcpy(ldap_mods[mods_size - 2]->mod_type, newparam);
-
- ldap_mods[mods_size - 2]->mod_values = ast_calloc(sizeof(char *), 2);
- ldap_mods[mods_size - 2]->mod_values[0] = ast_calloc(sizeof(char), strlen(field->value) + 1);
- strcpy(ldap_mods[mods_size - 2]->mod_values[0], field->value);
+ /* NULL terminate */
+ ldap_mods[mods_size - 1] = NULL;
}
}
/* freeing ldap_mods further down */
@@ -1552,7 +1644,7 @@
ast_free(filter);
ast_free(clean_basedn);
ldap_msgfree(ldap_result_msg);
- ldap_mods_free(ldap_mods, 0);
+ ldap_mods_free(ldap_mods, 1);
return -1;
}
/* Ready to update */
@@ -1574,14 +1666,10 @@
}
ast_mutex_unlock(&ldap_lock);
- if (filter) {
- ast_free(filter);
- }
- if (clean_basedn) {
- ast_free(clean_basedn);
- }
+ ast_free(filter);
+ ast_free(clean_basedn);
ldap_msgfree(ldap_result_msg);
- ldap_mods_free(ldap_mods, 0);
+ ldap_mods_free(ldap_mods, 1);
return num_entries;
}
@@ -1683,6 +1771,21 @@
return 0;
}
+static int config_can_be_inherited(const char *key)
+{
+ int i;
+ static const char * const config[] = {
+ "basedn", "host", "pass", "port", "protocol", "url", "user", "version", NULL
+ };
+
+ for (i = 0; config[i]; i++) {
+ if (!strcasecmp(key, config[i])) {
+ return 0;
+ }
+ }
+ return 1;
+}
+
/*! \brief parse the configuration file
*/
static int parse_config(void)
@@ -1773,7 +1876,9 @@
if (!strcasecmp(var->name, "additionalFilter")) {
table_config->additional_filter = ast_strdup(var->value);
} else {
- ldap_table_config_add_attribute(table_config, var->name, var->value);
+ if (!is_general || config_can_be_inherited(var->name)) {
+ ldap_table_config_add_attribute(table_config, var->name, var->value);
+ }
}
}
}
--
To view, visit https://gerrit.asterisk.org/4994
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0a534a60c7a5d79876bc75eff6588ce7db0d8c2e
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
More information about the asterisk-code-review
mailing list