Bug 40075 - unable to use ldap groups that contain DNs and usernames for AuthZ
Summary: unable to use ldap groups that contain DNs and usernames for AuthZ
Status: RESOLVED LATER
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_authz_ldap (show other bugs)
Version: 2.2.3
Hardware: Other Linux
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: MassUpdate, PatchAvailable
Depends on:
Blocks:
 
Reported: 2006-07-19 17:55 UTC by johanna bromberg craig
Modified: 2018-11-07 21:09 UTC (History)
0 users



Attachments
Patch Available - allows mixed dn/userid groups for AuthZ (4.74 KB, patch)
2006-07-19 18:04 UTC, johanna bromberg craig
Details | Diff
adds AuthLDAPGroupAttributeDN and AuthzLDAPRequireDN directives (5.35 KB, patch)
2006-11-02 07:40 UTC, johanna bromberg craig
Details | Diff
Improved null DN Checking (6.07 KB, patch)
2006-11-27 12:03 UTC, johanna bromberg craig
Details | Diff
Enhance AuthLDAPGroupAttribute (4.57 KB, patch)
2006-12-29 20:09 UTC, Brad Nicholes
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description johanna bromberg craig 2006-07-19 17:55:34 UTC
We have identities at the University of Michigan that don't have their own entries in
our LDAP directory, but they do appear in groups. This brings up a
few issues in regards to authorization with mod_authnz_ldap:

1) We'd like some way to say "if we can't find a DN
for this identity, that's OK."

2) Since some of our users are in the directory ( have a person
entry ) and some are not,  AuthLDAPGroupAttributeisDN is not rich
enough for us. Many of our groups contain both DNs and usernames.
We'd like to extend "AuthLDAPGroupAttribute" to say whether the
attribute in question is a DN or username, and thus be able to
authorize both DNs and usernames for the same resource.
Comment 1 johanna bromberg craig 2006-07-19 18:04:02 UTC
Created attachment 18619 [details]
Patch Available - allows mixed dn/userid groups for AuthZ

Here is the diff for the feature we've requested. There is a new directive
"AuthzLDAPRequireDN" which takes on/off. We've also modified
AuthLDAPGroupAttribute to take an optional second argument, "dn" which
indicates the attribute is a dn. If this type is not set, the global
"AuthLDAPGroupAttributeisDN" is observed.
Comment 2 Brad Nicholes 2006-07-24 21:00:27 UTC
I haven't actually applied the patch and tried it yet, but just from a visual 
review of the code the main problem that I see is switching the 
AuthLDAPGroupAttribute directive from AP_INIT_ITERATE to AP_INIT_TAKE12.  This 
change would break backward compatibility.  As AP_INIT_ITERATE the directive 
allows for a list of attributes which during configuration, iterates through 
each attribute and adds it to the group attribute list.  Changing this 
directive to AP_INIT_TAKE12 would not allow for an attribute list but rather 
only a single attribute with an optional second parameter.  This would break 
any configuration that currently specifies this directive with an attribute 
list.  You may need to define a different way to qualify whether the attribute 
is a DN or username.
Comment 3 johanna bromberg craig 2006-07-24 21:19:14 UTC
According to the documentation here http://httpd.apache.org/docs/2.2/mod/
mod_authnz_ldap.html#authldapgroupattribute it looked like the way to use the 
AuthLDAPGroupAttribute was multiple instances of the directive, not a single list of attributes, though I 
noticed the code supported that. So I wasn't sure if I should obey the documentation or the code for 
backward compatibility purposes.

The other way I could implement this is to make a new directive, "AuthLDAPGroupAttributeDN" or 
possibly the reverse, "AuthLDAPGroupAttributeUid". This would be an iterate, and any attribute listed 
would be a DN or uid, depending on how you implement it. Or I could add both, and the admin would 
list which attributes were dns and which attributes were uids. And then backwards compatibility of the 
iterate of AuthLDAPGroupAttribute would be preserved. 

Shall I cut this patch instead?
Comment 4 johanna bromberg craig 2006-11-02 07:40:57 UTC
Created attachment 19073 [details]
adds AuthLDAPGroupAttributeDN and AuthzLDAPRequireDN directives

Ok, this patch restores AuthLDAPGroupAttribute to its former syntax and adds a
new directive, AuthLDAPGroupAttributeDN, whose attribute type is taken to be
"dn" regardless of the value of AuthLDAPGroupAttributeIsDN. 
AuthLDAPGroupAttributeDN uses the same syntax as AuthLDAPGroupAttribute for the
sake of clarity.
Comment 5 Brad Nicholes 2006-11-08 15:10:59 UTC
The problem I see with this patch is around the value of req->dn when the
function util_ldap_cache_getuserdn() fails.  If ldap is unable to find a user
object for a user name, obviously this function will fail and the actual value
of "dn" that is returned from this function is undetermined (could be garbage).
 Yet if sec->require_dn is set to false, the code is allowed to continue as if
it had a valid "dn" and "vals".  Later when checking authorization for
ldap-group, req->dn is passed back into util_ldap_cache_compare() which could
end up causing a segfault.  The other problem is that if other authorization
types are used along side of ldap-group, req->dn would be reference within those
checks as well which could segfault.

Another issue is the addition of AuthLDAPGroupAttributeDN which is basically the
same thing as AuthLDAPGroupAttribute except it sets an additional flag.  Could
you do the same thing by just referencing the flag that is already being set by
AuthzLDAPRequireDN?  AuthLDAPGruopAttributeDN doesn't seem very intuitive.  I
also might change AuthzLDAPRequireDN to something like AuthzLDAPRequireGroupDN
but then that kind of conflicts with AuthLDAPGroupAttributeIsDN.  I not sure how
best to implement this.
Comment 6 johanna bromberg craig 2006-11-27 12:03:21 UTC
Created attachment 19182 [details]
Improved null DN Checking
Comment 7 johanna bromberg craig 2006-11-27 12:04:25 UTC
Comment on attachment 19182 [details]
Improved null DN Checking

Reviewing uldap_cache_getuserdn() in modules/ldap/ldap_util.c, neither binddn
nor retattrs is modified if an error is returned. They are both initialized to
NULL as well.

As for calling into util_ldap_cache_compare() with a NULL req->dn, there are 6
calls to util_ldap_cache_compare() and/or util_ldap_cache_comparedn() in the
original code. The patch makes it 7. In each case, the original code and the
patch check for the case where req->dn is NULL. Since the original code uses:

if (req->dn == NULL || strlen(req->dn) == 0) {

I'll resubmit the patch to use that stronger statement -- the original
submission only checked for req->dn == NULL.

Regarding using AuthzLDAPRequireDN, no, that flag can't be used because it
globally changes the meaning of all values in the groupattr array. The desired
functionality is to allow both DN and UID attributes to be used for membership
comparison. The current code base allows for group attributes to be DNs or UIDs
(basically strings), but not both at the same time. The patch allows groups
attributes of both types at the same time.

Another possible implementation was to make a separate groupattr array, but
that method would have required quite a bit of repeated code. It was
considerably simpler to add a type field to the groupattr array and utilize the
existing loops over the groupattr array.

I agree that the syntax is not terribly intuitive, but the functionality is not
exactly basic. The earlier patch has certain advantages in that regard.
However, the current patch has the advantage of leaving the old syntax entirely
alone.

AuthzLDAPRequireDN should not be called AuthzLDAPRequireGroupDN, as it is
actually managing completely different functionality, not related to groups.
The original code base required all users to have an entry in the directory.
AuthzLDAPRequireDN allows users to not be in the directory. Perhaps renaming it
AuthzLDAPRequireUserDN would make that more clear.
Comment 8 Brad Nicholes 2006-11-27 16:11:17 UTC
Just out of curiosity, how do expect the authentication side of this to work?  
Obviously if the user that you are trying to authorize does not have an entry 
in the LDAP directory, then using ldap authentication will fail which means 
that the authorization stage will never be called.  Are you planning on using 
some other type of authentication (file, db, etc.) and then just use ldap to 
manage the groups?
Comment 9 johanna bromberg craig 2006-11-27 17:33:34 UTC
Precisely! That one option (AuthzLDAPRequireDN Off) allows the AuthN and AuthZ
functions of mod_authnz_ldap to be independently controlled. At University of
Michigan we use a Web Single Sign-On System (Cosign) that provides only AuthN.
At the same time, we have a rich LDAP environment used for AuthZ. These changes
are applicable to any site that wants to use LDAP for AuthZ and any other
mechanism for AuthN.
Comment 10 Brad Nicholes 2006-12-29 20:09:32 UTC
Created attachment 19329 [details]
Enhance AuthLDAPGroupAttribute
Comment 11 Brad Nicholes 2006-12-29 20:11:05 UTC
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. If accepted, it
would then need to be back ported.
Comment 12 johanna bromberg craig 2007-01-10 11:32:19 UTC
As far as changing the syntax of AuthLDAPGroupAttribute to AP_INIT_RAW_ARGS and allowing optional 
args indicating type, the proposed patch is fine. However, your patch doesn't retain the 
AuthzLDAPRequireDN functionality, and that lack of functionality would be a show stopper for us. If that 
functionality were also included, I think we'd be all set. Thanks!
Comment 13 Brad Nicholes 2007-01-11 08:03:05 UTC
Unless I am missing something, the AuthLDAPRequireDN functionality is being 
handled by AuthLDAPGroupAttributeIsDN.  If AuthLDAPGroupAttributeIsDN is set 
to ON (which is the default) then AuthnzLDAP will expect the user object to 
exist in the directory and for that user ID to be resolved to a full DN.  
Otherwise it will not be able to do a DN comparison which is what 
AuthLDAPGroupAttributeIsDN ON implies.  If AuthLDAPGroupAttributeIsDN is set 
to OFF, then the user ID that is passed in, does not have to be resolved to a 
full DN which means that the user object does not have to exist in the 
directory but will be resolved to a DN if it does exist.  The group membership 
comparison will then follow the DN or UN specifier.  If DN is specified then a 
full DN comparison will occur.  If UN is specified then a simple user id 
comparison will occur.  If neither is specified then the comparison follows 
the AuthLDAPGroupAttributeIsDN setting which would default to a UN 
comparison.  

What additional functionality is AuthzLDAPRequireDN performing than that?  
From what I could see in the original patch, AuthzLDAPRequireDN simple 
determined whether a failed search for the user object forced the entire 
request to fail or was ignored.  AuthLDAPGroupAttributeIsDN is allowing for 
the same functionality.
Comment 14 johanna bromberg craig 2007-01-15 11:55:23 UTC
Sadly, you'll never get that far.  In particular, when you get here, around line
571 in httpd-2.2.3:

        /* Search failed, log error and return failure */
        if(result != LDAP_SUCCESS) {
            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
                "auth_ldap authorise: User DN not found, %s", ldc->reason);
            return sec->auth_authoritative? HTTP_UNAUTHORIZED : DECLINED;
        }

you'll return.  Our proposed AuthLDAPRequireDN (off) patch allows this return to
be bypassed.  Just below this code is where the requirements array is traversed,
so if we can't get there, no requirements can be checked.  Perhaps this return
is an oversight, and there's no need for AuthLDAPRequireDN?  The comment at 547:

    /*
     * If we have been authenticated by some other module than mod_auth_ldap,
     * the req structure needed for authorization needs to be created
     * and populated with the userid and DN of the account in LDAP
     */

certainly suggests that it may be OK to rely on an external authN, but obviously
the code at 571 requires that the user exist in LDAP.
Comment 15 Brad Nicholes 2007-01-16 10:41:50 UTC
In the last patch that I included against TRUNK, that return has been removed.
This return statement as well as the comment that you are referring to is
exactly why I stated in reply #11 that I think that the original intent of
AuthLDAPGroupAttributeIsDN was broken.  

In the attached patch, if the LDAP search fails, a DEBUG level message will be
written but the request processing won't stop.  The check that replicates the
RequireDN-like functionality comes about 10 lines below there when
sec->group_attrib_is_dn is checked.  If sec->group_attrib_is_dn (ie.
AuthLDAPGroupAttributeIsDN) is true, the request is denied.  If it is false and
a user id exists, then the request is allowed to continue and the user id is
compared against the membership attributes.  Take a look at the 12/19 patch that
I attached against TRUNK.

FYI, this patch (or any other patch) will have to be applied against TRUNK first
and then backported to 2.2 if accepted.  So all further coding and evaluation
should be done with TRUNK and not the 2.2 branch.
Comment 16 Brad Nicholes 2007-01-16 10:44:05 UTC
(In reply to comment #15)
 and
> compared against the membership attributes.  Take a look at the 12/19 patch 

Sorry, I meant the 12/29 patch.
Comment 17 William A. Rowe Jr. 2018-11-07 21:09:46 UTC
Please help us to refine our list of open and current defects; this is a mass update of old and inactive Bugzilla reports which reflect user error, already resolved defects, and still-existing defects in httpd.

As repeatedly announced, the Apache HTTP Server Project has discontinued all development and patch review of the 2.2.x series of releases. The final release 2.2.34 was published in July 2017, and no further evaluation of bug reports or security risks will be considered or published for 2.2.x releases. All reports older than 2.4.x have been updated to status RESOLVED/LATER; no further action is expected unless the report still applies to a current version of httpd.

If your report represented a question or confusion about how to use an httpd feature, an unexpected server behavior, problems building or installing httpd, or working with an external component (a third party module, browser etc.) we ask you to start by bringing your question to the User Support and Discussion mailing list, see [https://httpd.apache.org/lists.html#http-users] for details. Include a link to this Bugzilla report for completeness with your question.

If your report was clearly a defect in httpd or a feature request, we ask that you retest using a modern httpd release (2.4.33 or later) released in the past year. If it can be reproduced, please reopen this bug and change the Version field above to the httpd version you have reconfirmed with.

Your help in identifying defects or enhancements still applicable to the current httpd server software release is greatly appreciated.