>>> On Mon, Dec 4, 2006 at 1:00 PM, in message
<[EMAIL PROTECTED]>, Johanna Bromberg Craig
<[EMAIL PROTECTED]> wrote:
> Hi,
>
> I've addressed the feedback I received on my patch from Brad Nicholes
> as follows:
>
> I've reviewed all instances of util_ldap_compare() and
> util_ldap_cache_comparedn() to confirm that each is protected from
> cases where req- >dn might be NULL or '\0'.
>
> I've addressed the differences between AuthLDAPGroupAttributeDN,
> AuthLDAPGroupAttribute, and AuthzLDAPRequireDN.
>
> Thanks,
> Johanna
I finally got some time to take a closer look at the patch. Although I like
the concept, I am still uncomfortable with the implementation from a
configuration point of view. I have attached a patch which is actually closer
to your first patch except it maintains the original functionality while
enhancing the AuthLDAPGroupAttribute directive to support attributes that may
contain a full DN. Actually, I think that was the original intent of
AuthLDAPGroupAttributeIsDN but it appears to have been broken along the way.
Anyway the proposed new syntax for AuthLDAPGroupAttribute is:
AuthLDAPGroupAttribute attribute [DN | UN] ...
where the keywords "DN" (Distinguished Name) and "UN" (User Name) can
optionally follow each attribute in the list. If neither of the keywords are
specified, then the attribute type follows the AuthLDAPGroupAttributeIsDN
setting. The AuthLDAPGroupAttributeIsDN setting determines if a DN is required
in the group comparison or not. If the AuthLDAPGroupAttribute list contains
any UN's, then AuthLDAPGroupAttributeIsDN must be set to OFF otherwise the
authorization will fail since it would be expecting to be able to resolve the
user object to a DN within the LDAP directory.
Let me know if this works for you,
BTW, this patch is against trunk rather than the 2.2.x branch.
Brad
Index: mod_authnz_ldap.c
===================================================================
--- mod_authnz_ldap.c (revision 489925)
+++ mod_authnz_ldap.c (working copy)
@@ -84,6 +84,7 @@
struct mod_auth_ldap_groupattr_entry_t {
char *name;
+ char *type;
};
module AP_MODULE_DECLARE_DATA authnz_ldap_module;
@@ -647,8 +648,10 @@
#endif
grp = apr_array_push(sec->groupattr);
grp->name = "member";
+ grp->type = NULL;
grp = apr_array_push(sec->groupattr);
grp->name = "uniquemember";
+ grp->type = NULL;
#if APR_HAS_THREADS
apr_thread_mutex_unlock(sec->lock);
#endif
@@ -682,7 +685,6 @@
if(result != LDAP_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
"auth_ldap authorise: User DN not found, %s", ldc->reason);
- return AUTHZ_DENIED;
}
req = (authn_ldap_request_t *)apr_pcalloc(r->pool,
@@ -719,13 +721,30 @@
getpid(), t);
for (i = 0; i < sec->groupattr->nelts; i++) {
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
- "[%" APR_PID_T_FMT "] auth_ldap authorize: require
group: "
- "testing for %s: %s (%s)", getpid(),
- ent[i].name, sec->group_attrib_is_dn ? req->dn :
req->user, t);
+ result = 0;
- result = util_ldap_cache_compare(r, ldc, sec->url, t, ent[i].name,
- sec->group_attrib_is_dn ? req->dn : req->user);
+ if (ent[i].type == NULL) {
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "[%" APR_PID_T_FMT "] auth_ldap authorize: require
group: "
+ "testing for %s: %s (%s)", getpid(),
+ ent[i].name, sec->group_attrib_is_dn ? req->dn :
req->user, t);
+
+ result = util_ldap_cache_compare(r, ldc, sec->url, t, ent[i].name,
+ sec->group_attrib_is_dn ? req->dn
: req->user);
+ } else if (req->dn != NULL && strlen(req->dn) != 0 &&
strcasecmp(ent[i].type, "dn") == 0) {
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "[%" APR_PID_T_FMT "] auth_ldap authorise: require
group: "
+ "testing for %s: %s (%s)", getpid(),
+ ent[i].name, req->dn, t);
+ result = util_ldap_cache_compare(r, ldc, sec->url, t, ent[i].name,
req->dn);
+ } else if (req->user != NULL && strlen(req->user) != 0 &&
strcasecmp(ent[i].type, "un") == 0) {
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "[%" APR_PID_T_FMT "] auth_ldap authorise: require
group: "
+ "testing for %s: %s (%s)", getpid(),
+ ent[i].name, req->user, t);
+ result = util_ldap_cache_compare(r, ldc, sec->url, t, ent[i].name,
req->user);
+ }
+
switch(result) {
case LDAP_COMPARE_TRUE: {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
@@ -1252,15 +1271,29 @@
static const char *mod_auth_ldap_add_group_attribute(cmd_parms *cmd, void
*config, const char *arg)
{
struct mod_auth_ldap_groupattr_entry_t *new;
+ const char *t;
+ char *w;
+ int cmpsize;
authn_ldap_config_t *sec = config;
- if (sec->groupattr->nelts > GROUPATTR_MAX_ELTS)
- return "Too many AuthLDAPGroupAttribute directives";
+ t = arg;
+ while ((w = ap_getword_white(cmd->pool, &t)) && w[0]) {
+ new = apr_array_push(sec->groupattr);
+ new->name = apr_pstrdup(cmd->pool, w);
+ new->type = NULL;
- new = apr_array_push(sec->groupattr);
- new->name = apr_pstrdup(cmd->pool, arg);
+ cmpsize = (strlen(t) < 3) ? 2 : 3;
+ if ((strncasecmp(t, "DN ", cmpsize) == 0) || (strncasecmp(t, "UN ",
cmpsize) == 0)) {
+ w = ap_getword_white(cmd->pool, &t);
+ new->type = apr_pstrdup(cmd->pool, w);
+ }
+ if (sec->groupattr->nelts > GROUPATTR_MAX_ELTS)
+ return "Too many AuthLDAPGroupAttribute directives";
+ }
+
+
return NULL;
}
@@ -1325,7 +1358,7 @@
"(at the expense of possible false matches). See the
documentation for "
"a complete description of this option."),
- AP_INIT_ITERATE("AuthLDAPGroupAttribute",
mod_auth_ldap_add_group_attribute, NULL, OR_AUTHCFG,
+ AP_INIT_RAW_ARGS("AuthLDAPGroupAttribute",
mod_auth_ldap_add_group_attribute, NULL, OR_AUTHCFG,
"A list of attributes used to define group membership -
defaults to "
"member and uniquemember"),
