Closed Bug 467992 Opened 16 years ago Closed 14 years ago

Login fails if the users LDAP account is denied search in LDAP

Categories

(Bugzilla :: User Accounts, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Bugzilla 3.6

People

(Reporter: dan, Assigned: adam)

Details

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4
Build Identifier: 3.2

For security and performance reasons its best to configure ACL's in LDAP that will only allow certain LDAP accounts to search LDAP. The code that is currently implemented assumes that if the ldap bind using the Bugzilla users LDAP account is successful that the subsequent ldap search using the Bugzilla users LDAP account will succeed as well.  This assumption is incorrect, especially if the LDAPbinddn is configured. This means that using Bugzilla's LDAP capabilities won't work in secure environments or environments were LDAP searching is limited to specific accounts.

Reproducible: Always

Steps to Reproduce:
1. On the LDAP server disable anonymous bind and define LDAP ACL's that only permit a specific LDAPbinddn the rights to search LDAP.
2. Configure Bugzilla to use the LDAPbinddn.
3. Try and login using an LDAP user.
Actual Results:  
The user will receive an error 500 "Can't call method "exists"; on an undefined value at Bugzilla/Auth/Verify/LDAP.pm line 88, <DATA>; line 466." because on line 74 of LDAP.pm there is an attempt to bind using the Bugzilla users LDAP account.  Assuming the bind is a success and the Bugzilla users LDAP account doesn't have search rights in ldap, $detail_result will be populated with 0 results and the method call shift_entry will return "undef".

    # Check the password.   **THIS PASSES**
    my $pw_result = $self->ldap->bind($dn, password => $params->{password});
    return { failure => AUTH_LOGINFAILED } if $pw_result->code;

    # And now we fill in the user's details.  **LDAP RETURN 0 RESULTS BECAUSE BUGZILLA USERS LDAP ACCOUNT DOESN'T ALLOW SEARCH**
    my $detail_result = $self->ldap->search(_bz_search_params($username));
    return { failure => AUTH_ERROR, error => "ldap_search_error",
             details => {errstr => $detail_result->error, username => $username}
    } if $detail_result->code;

**CALLING shift_entry WILL RETURN "undef" BECAUSE NO RESULTS WHERE RETURNED FROM LDAP**
    my $user_entry = $detail_result->shift_entry;

    my $mail_attr = Bugzilla->params->{"LDAPmailattribute"};
    if ($mail_attr) {
**THIS NEXT LINE FAILS BECAUSE "exists" CAN'T BE CALLED ON AN UNDEFINED VALUE**
       if (!$user_entry->exists($mail_attr)) {
            return { failure => AUTH_ERROR,
                     error   => "ldap_cannot_retreive_attr",
                     details => {attr => $mail_attr} };
        }

Expected Results:  
After validating that the username and password is valid and before wasting anymore time searching LDAP make sure and rebind using anonymous or LDAPbinddn to complete the login process. Don't use the Bugzilla users LDAP account to search LDAP.
Version: unspecified → 3.2
Hmm. I don't think that will work in many cases, though--anonymous binders almost certainly can't get the information we need about the user, in any secure environment, whereas I don't exactly understand why there would be a user who couldn't access their own full name and email address. Is there any way to get that info other than a search?
Attached patch v1 - rebind (obsolete) — Splinter Review
Patch to rebind using anonymous or LDAPbinddn after password verification complete.
Attachment #351467 - Flags: review?(mkanat)
If there are LDAP ACL's in place that only allow search for specific LDAP bind accounts and you try and searching using any account other than the users with the rights to search then you won't get any results back from LDAP regardless of if you are trying to search or your own account.  

It is especially not okay to try and use the Bugzilla users LDAP account to search LDAP if I've explicitly stated that I want the LDAPbinddn to be used.

Hope this helps clear things up.
Search is the only way to get users full name and email address and therefore you must use an LDAP account that has rights to search. This is the entire concept behind providing the LDAPbinddn account to any application.  It's my way of telling the application which bind user has the rights to bind and search my LDAP structure.
Attachment #351467 - Flags: review?(emmanuel.seyman)
Attachment #351467 - Flags: review?(emmanuel.seyman) → review-
Comment on attachment 351467 [details] [diff] [review]
v1 - rebind


>-    # Check the password.   
>+    # Check the username and password first so not to waste time
>+    # searching LDAP if the username or password is bad.

This change sounds gratuitous.

>+		
>+	# Rebind using _bind_ldap_anonymously because it cannot be assumed
>+	# that the Bugzilla user will have LDAP search rights.
>+	$self->_bind_ldap_anonymously();

While this makes sense if you're always using a user to bind to your LDAP,
it doesn't make much sense if you're binding anonymously. In these cases,
we're assuming here that an anonymous search will succeed where one
conducted by the LDAP user won't, which I find difficult to believe.

I'ld prefer if this was handled in the following way :

- search with the Bugzilla user's credentials
- if that fails, _bind_ldap_anonymously()
- search again, fail with ldap_search_error if that doesn't work

This way, we cover all our options and don't waste time rebinding if the user
does have the rights to search for his entry.
Yeah, I'm in agreement with manu about what should be done.
Severity: major → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #351467 - Flags: review?(mkanat)
I would agree that making an anonymous bind is unwarranted if the users account can't search, but _bind_ldap_anonymously() doesn't always make an anonymous bind (the name "_bind_ldap_anonymously()" is misleading); if LDAPbinddn is configured then _bind_ldap_anonymously() will use it to bind and search instead.  In our environment we don't allow anonymous bind and nobody but an ldap administrator or a specific application account with search rights is allowed to search ldap. The only  way bugzilla works in a secured ldap environment is with the attached patch.  We've been running with this patch since I submitted it and have even upgraded to 3.2.1 and reapplied the patch and it's worked just fine.

Once you dig deeper into  _bind_ldap_anonymously(), I think you may agree the call to it isn't unwarranted.   

...maybe _bind_ldap_anonymously() should be renamed so it isn't so misleading?
(In reply to comment #7)
>
> if LDAPbinddn is configured then _bind_ldap_anonymously() will use it to
> bind and search instead.

This is only when LDAPbinddn and LDAPbindpass are defined. When they're not,
the bind is anonymous.

> In our environment we don't allow anonymous bind and nobody but an
> ldap administrator or a specific application account with search rights is
> allowed to search ldap.

This strikes me as a special case. Of all the LDAP installations I've worked
on, I've never come across one that was so secured that users could not search
for their own entry and I'ld prefer we did not consider this the genral case.

Please rework your patch to use the steps I outlined in comment #5
I've read comment #5 and I still don't understand why you want to ignore the fact that the LDAPbinddn and LDAPbindpass are set.  If a user configures a specific bind account it's probably for a good reason; the current approach you've defined in comment #5 continues to ignore the fact that the LDAPbinddn has been explicitly configured by the user.  There are numerous reasons I would prefer to use a specific LDAPbindbn:

- Easier to debug ldap issues if I see everything coming from an application specific bind account; remember I may have thousands of applications out there making queries to LDAP.
- For security reasons LDAP ACL's may limit searching to only a specific set of accounts.
- For security reasons I want to limit the number of bind's that individual user accounts make to LDAP.

Seems to me a better approach might be:

- bind using Bugzilla user's credentials **primarily to validate credentials**
- if LDAPbinddn is set, re-bind using _bind_ldap_binddn() and search using the LDAPbinddn credentials
- else, search using the Bugzilla user's credentials
- if that fails, _bind_ldap_anonymously()
- search again, fail with ldap_search_error if that doesn't work
(In reply to comment #9)
> I've read comment #5 and I still don't understand why you want to ignore the
> fact that the LDAPbinddn and LDAPbindpass are set.

I've seen several instances of Bugzilla where authentication is done via LDAP
and LDAPbinddn is not set.
(In reply to comment #8)
> This strikes me as a special case. Of all the LDAP installations I've worked
> on, I've never come across one that was so secured that users could not search
> for their own entry and I'ld prefer we did not consider this the genral case.

  Manu is correct here and I agree.
Attached patch LDAPrebindanonymously.patch (obsolete) — Splinter Review
Here is a new patch. It adds a new boolean parameter, LDAPrebindanonymously which defaults to false. If the parameter is set to true, then Bugzilla will re-bind anonymously (which of course doesn't necessarily mean anonymously) using $self->_bind_ldap_anonymously() after successfully authenticating the user.

Patch also includes parameters template changes and documentation update for consistency.
We have run across a similar issue with our LDAP implementation (Zimbra). Bugzilla can bind and search using a special service account, but the user's uid/e-mail address is stored in the zimbraMailCanonicalAddress which the user doesn't have access to. Rather than messing with Zimbra's ACLs to give the account access to that attribute, it's easier to just re-bind as the service account which already has access.
Hey Adam. Thanks for the patch! To get it checked in to Bugzilla, you'll have to go through the review process, which is described as part of our general development process here:

  http://wiki.mozilla.org/Bugzilla:Developers

If you have any questions about the process, feel free to post them here or email me directly.
Assignee: user-accounts → adam
Attachment #419576 - Flags: review?(mkanat)
Comment on attachment 419576 [details] [diff] [review]
LDAPrebindanonymously.patch

Thanks, Adam.

Though this solves the problem presented, the parameter's existence will be too confusing to most LDAP/Bugzilla admins, who won't always have an excellent understanding of how everything works "behind the scenes". I suspect that the solution detailed by manu in an earlier comment, where we always attempt an anonymous rebind if we fail and have an LDAPbinddn, will be the more broadly-successful solution.
Attachment #419576 - Flags: review?(mkanat) → review-
Attached patch LDAPrebindanonymously-2.patch (obsolete) — Splinter Review
Here is a new patch, which implements what Max suggested in comment 15. I'm fine if it needs more cleanup but I wanted to know if this is on the right track.
Attachment #419576 - Attachment is obsolete: true
Attachment #419668 - Flags: review?(mkanat)
Attachment #351467 - Attachment is obsolete: true
Comment on attachment 419668 [details] [diff] [review]
LDAPrebindanonymously-2.patch

  Yeah, this is on the right track.

>Index: Bugzilla/Auth/Verify/LDAP.pm
>@@ -71,22 +71,45 @@ sub check_credentials {
>+    # First try the search as the (already binded) user in question.

  Nit: "bound" instead of "binded".

>+    my $user_entry = undef;

  "= undef" is unnecessary.

>+    my $error_string = "Unable to find user details";

  Probably best to just get the string that the LDAP server returns instead of specifying an English string here--I don't think this English string is used anywhere, in any case.

  This looks good otherwise, so these three things can be fixed on checkin or you can post another patch with them fixed.
Attachment #419668 - Flags: review?(mkanat) → review+
Since this is a behavior change, I think it's OK to restrict it just to HEAD.
Flags: approval+
Target Milestone: --- → Bugzilla 3.6
My only question is--does this patch actually fix your problem? It probably fixes Dan's problem, but if you can *search* with your user but not access the LDAPmailattribute with your user, I don't think that this patch will fix it, no?
(In reply to comment #17)
> (From update of attachment 419668 [details] [diff] [review])
> >+    my $error_string = "Unable to find user details";
> 
>   Probably best to just get the string that the LDAP server returns instead of
> specifying an English string here--I don't think this English string is used
> anywhere, in any case.

$error_string would be used if somehow the first search was successful but
didn't return any results, and the second search failed either due to an LDAP
failure or it returned 0 results (unlikely, since the exact search managed to
return a result the first time around before it even tried to bind as the user
in question).

If I post a new patch to address the other two comments (and possibly this
$error_string one if we can figure out how to handle the degenerate case I
described above), do I mark it as superseding this patch with the review flag
back at "?"?


(In reply to comment #19)
> My only question is--does this patch actually fix your problem?

Yes, but it's weird: The users in question have permission to search, but not on the LDAPmailattribute field. So the search succeeds but returns 0 results, which causes it to re-bind as the original binddn user and re-run the search.
(In reply to comment #20)
> If I post a new patch to address the other two comments (and possibly this
> $error_string one if we can figure out how to handle the degenerate case I
> described above), do I mark it as superseding this patch with the review flag
> back at "?"?

  Yeah, do that, if it's going to contain changes other than the ones from the original review comment.

> Yes, but it's weird: The users in question have permission to search, but not
> on the LDAPmailattribute field. So the search succeeds but returns 0 results,
> which causes it to re-bind as the original binddn user and re-run the search.

  That's weird. But I suppose it works.


  I think the solution to the $error_string thing is to include some sort of default error message if errstr is empty, in the error template.
Holding approval for a new patch.
Flags: approval+
Address issues from comment 17 and comment 21.
Attachment #419668 - Attachment is obsolete: true
Attachment #419678 - Flags: review?(mkanat)
Comment on attachment 419678 [details] [diff] [review]
LDAPrebindonsearchfailure.patch

This looks great! Thank you, Adam. :-)
Attachment #419678 - Flags: review?(mkanat) → review+
Flags: approval+
Checking in Bugzilla/Auth/Verify/LDAP.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/LDAP.pm,v  <--  LDAP.pm
new revision: 1.19; previous revision: 1.18
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.127; previous revision: 1.126
done
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: