[prev in list] [next in list] [prev in thread] [next in thread]
List: cyrus-devel
Subject: Re: The ldapdb plug-in initialization for clients is too noisy!
From: "Greg A. Woods" <woods-cyrus () weird ! com>
Date: 2009-05-30 16:17:52
Message-ID: m1MARFr-000koTC () most ! weird ! com
[Download RAW message or body]
At Fri, 29 May 2009 12:52:56 +0200 (CEST), "Andreas Winkelmann" <ml@awinkelmann.de> wrote:
Subject: Re: The ldapdb plug-in initialization for clients is too noisy!
>
> > Greg, you don't need to change anything, because there is a solution to
> > you problem already
> > Just set the "auxprop_plugin" option to exclude ldapdb.
>
> This will not stop the auxprop-plugins from beeing loaded/initialized. And
> exactly this gives the warnings/errors.
>
> If it is really needed to have the unused files on disk, someone can add
> some dummy-options to the config file(s).
>
> For ldabdb:
>
> ldapdb_uri: dummy or wahtever
>
> For sql:
>
> sql_select: dummy or whatever
The more I look at this code, the more I think that the ldapdb (and sql)
plug-in is horribly abusing the *_*_plug_init() functions for things it
should not be doing at that point.
(there's also a whole huge botch on how plug-in version checking is done
as currently both the caller and the callee are doing it!)
I think the proper thing to do, at least the minimal proper thing
without rully re-engineering everything so that the *_plug_init() API is
better respected, is as I suggested: return SASL_NOMECH when the
configuration isn't set (instead of the bogus SASL_BADPARAM value) and
then don't scream so loudly for the log message when it the result is
simply SASL_NOMECH (some cleanup and other changes here too, but none
for SQL as I don't and won't use it).
This code really needs a huge amount more TLC and re-factoring throughout.
Index: plugins/ldapdb.c
===================================================================
RCS file: /cvs/src/sasl/plugins/ldapdb.c,v
retrieving revision 1.4
diff -u -r1.4 ldapdb.c
--- plugins/ldapdb.c 23 Nov 2008 17:20:52 -0000 1.4
+++ plugins/ldapdb.c 29 May 2009 22:52:43 -0000
@@ -393,8 +393,7 @@
if (ret != LDAP_SUCCESS) goto done;
- for(msg=ldap_first_message(cp.ld, res); msg; msg=ldap_next_message(cp.ld, msg))
- {
+ for(msg=ldap_first_message(cp.ld, res); msg; msg=ldap_next_message(cp.ld, msg)) {
if (ldap_msgtype(msg) != LDAP_RES_SEARCH_ENTRY) continue;
bvals = ldap_get_values_len(cp.ld, msg, attrs[0]);
if (!bvals) continue;
@@ -457,41 +456,42 @@
const char *s;
unsigned len;
- if(p->inited) return SASL_OK;
+ if (p->inited)
+ return SASL_OK;
utils->getopt(utils->getopt_context, ldapdb, "ldapdb_uri", &p->uri, NULL);
- if(!p->uri) return SASL_BADPARAM;
+ if (!p->uri)
+ return SASL_NOMECH;
utils->getopt(utils->getopt_context, ldapdb, "ldapdb_id",
- (const char **)&p->id.bv_val, &len);
+ (const char **)&p->id.bv_val, &len);
p->id.bv_len = len;
utils->getopt(utils->getopt_context, ldapdb, "ldapdb_pw",
- (const char **)&p->pw.bv_val, &len);
+ (const char **)&p->pw.bv_val, &len);
p->pw.bv_len = len;
utils->getopt(utils->getopt_context, ldapdb, "ldapdb_mech",
- (const char **)&p->mech.bv_val, &len);
+ (const char **)&p->mech.bv_val, &len);
p->mech.bv_len = len;
utils->getopt(utils->getopt_context, ldapdb, "ldapdb_starttls", &s, NULL);
- if (s)
- {
- if (!strcasecmp(s, "demand")) p->use_tls = 2;
- else if (!strcasecmp(s, "try")) p->use_tls = 1;
+ if (s) {
+ if (!strcasecmp(s, "demand"))
+ p->use_tls = 2;
+ else if (!strcasecmp(s, "try"))
+ p->use_tls = 1;
}
utils->getopt(utils->getopt_context, ldapdb, "ldapdb_rc", &s, &len);
- if (s)
- {
+ if (s) {
char *str = utils->malloc(sizeof("LDAPRC=")+len);
if (!str) return SASL_NOMEM;
strcpy( str, "LDAPRC=" );
strcpy( str + sizeof("LDAPRC=")-1, s );
- if (putenv(str))
- {
+ if (putenv(str)) {
utils->free(str);
return SASL_NOMEM;
}
}
utils->getopt(utils->getopt_context, ldapdb, "ldapdb_canon_attr",
- (const char **)&p->canon.bv_val, &len);
+ (const char **)&p->canon.bv_val, &len);
p->canon.bv_len = len;
p->inited = 1;
Index: lib/server.c
===================================================================
RCS file: /cvs/src/sasl/lib/server.c,v
retrieving revision 1.160
diff -u -r1.160 server.c
--- lib/server.c 8 Apr 2009 19:36:20 -0000 1.160
+++ lib/server.c 29 May 2009 22:52:44 -0000
@@ -423,15 +423,17 @@
if ((result != SASL_OK) && (result != SASL_NOUSER)
&& (result != SASL_CONTINUE)) {
_sasl_log(NULL, SASL_LOG_DEBUG,
- "server add_plugin entry_point error %z\n", result);
+ "%s_client_plug_init() failed in sasl_server_add_plugin(): %z\n",
+ plugname, result);
return result;
}
/* Make sure plugin is using the same SASL version as us */
+ /* XXX why check this again? *_server_plug_init() functions do this check already! */
if (version != SASL_SERVER_PLUG_VERSION)
{
_sasl_log(NULL, SASL_LOG_ERR,
- "version mismatch on plugin");
+ "version mismatch in sasl_server_add_plugin() for plugin '%s'", plugname);
return SASL_BADVERS;
}
Index: lib/client.c
===================================================================
RCS file: /cvs/src/sasl/lib/client.c,v
retrieving revision 1.74
diff -u -r1.74 client.c
--- lib/client.c 8 Apr 2009 19:36:20 -0000 1.74
+++ lib/client.c 29 May 2009 22:52:44 -0000
@@ -157,11 +157,12 @@
if (result != SASL_OK)
{
_sasl_log(NULL, SASL_LOG_WARN,
- "entry_point failed in sasl_client_add_plugin for %s",
- plugname);
+ "%s_client_plug_init(): failed in sasl_client_add_plugin(): %z",
+ plugname, result);
return result;
}
+ /* XXX why check this again? *_client_plug_init() functions do this check already! */
if (version != SASL_CLIENT_PLUG_VERSION)
{
_sasl_log(NULL, SASL_LOG_WARN,
@@ -442,6 +443,17 @@
* Apps should be encouraged to simply use space or comma space
* though
*/
+/*
+ * XXX Debugging client/server "No worthy mechs found" failures from the logic
+ * herein is next to impossible!
+ *
+ * Error flags should be set so that users can determine which requirements
+ * were met and which were not.
+ *
+ * In the mean time the best one can do is stop execution in this function,
+ * print *conn, *mechlist, etc., and then, if necessary, i.e. it's not
+ * blatantly obvious, step through each rule to see what happens.
+ */
int sasl_client_start(sasl_conn_t *conn,
const char *mechlist,
sasl_interact_t **prompt_need,
@@ -507,7 +519,7 @@
/* foreach in client list */
for (m = cmechlist->mech_list; m != NULL; m = m->next) {
- int myflags;
+ unsigned int myflags;
/* Is this the mechanism the server is suggesting? */
if (strcasecmp(m->m.plug->mech_name, name))
@@ -521,15 +533,16 @@
if (minssf > m->m.plug->max_ssf)
break;
- /* Does it meet our security properties? */
+ /* if there's an external layer with a better SSF then this is no
+ * longer considered a plaintext mechanism
+ */
myflags = conn->props.security_flags;
-
- /* if there's an external layer this is no longer plaintext */
if ((conn->props.min_ssf <= conn->external.ssf) &&
(conn->external.ssf > 1)) {
myflags &= ~SASL_SEC_NOPLAINTEXT;
}
+ /* Does it meet our security properties? */
if (((myflags ^ m->m.plug->security_flags) & myflags) != 0) {
break;
}
Index: lib/canonusr.c
===================================================================
RCS file: /cvs/src/sasl/lib/canonusr.c,v
retrieving revision 1.20
diff -u -r1.20 canonusr.c
--- lib/canonusr.c 10 Mar 2009 16:27:52 -0000 1.20
+++ lib/canonusr.c 29 May 2009 22:52:44 -0000
@@ -307,14 +307,15 @@
&out_version, &plug, plugname);
if(result != SASL_OK) {
- _sasl_log(NULL, SASL_LOG_ERR, "canonuserfunc error %i\n",result);
+ _sasl_log(NULL, SASL_LOG_ERR, "%s_canonuser_plug_init() failed in sasl_canonuser_add_plugin(): %z\n",
+ plugname, result);
return result;
}
if(!plug->canon_user_server && !plug->canon_user_client) {
/* We need at least one of these implemented */
_sasl_log(NULL, SASL_LOG_ERR,
- "canonuser plugin without either client or server side");
+ "canonuser plugin '%s' without either client or server side", plugname);
return SASL_BADPROT;
}
Index: lib/auxprop.c
===================================================================
RCS file: /cvs/src/sasl/lib/auxprop.c,v
retrieving revision 1.19
diff -u -r1.19 auxprop.c
--- lib/auxprop.c 28 Jan 2009 22:49:14 -0000 1.19
+++ lib/auxprop.c 29 May 2009 22:52:45 -0000
@@ -813,13 +813,15 @@
/* Check if out_version is too old.
We only support the current at the moment */
+ /* XXX why check this again? *_auxprop_plug_init() functions do this check already! */
if (result == SASL_OK && out_version < SASL_AUXPROP_PLUG_VERSION) {
result = SASL_BADVERS;
}
if(result != SASL_OK) {
- _sasl_log(NULL, SASL_LOG_ERR, "auxpropfunc error %s\n",
- sasl_errstring(result, NULL, NULL));
+ _sasl_log(NULL, (result != SASL_NOMECH) ? SASL_LOG_WARN : SASL_LOG_NOTE,
+ "%s_auxprop_plug_init() failed in sasl_auxprop_add_plugin(): %z\n",
+ plugname, result);
return result;
}
--
Greg A. Woods
+1 416 218-0098 VE3TCP RoboHack <woods@robohack.ca>
Planix, Inc. <woods@planix.com> Secrets of the Weird <woods@weird.com>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic