-
-
-
-
/trunk/reviewboard/accounts/backends.py (Diff revision 1) Params should be indented to the same column.
-
/trunk/reviewboard/accounts/backends.py (Diff revision 1) I'm unsure about this. Shouldn't we be using what the admin provided here? Will this string even work?
-
-
/trunk/reviewboard/accounts/backends.py (Diff revision 1) passwd[0][0] can go in simple_bind_s directly.
-
-
Fix security flaw and general design of the LDAP authentication backend (repost)
Review Request #610 — Created Oct. 24, 2008 and discarded
(This is a correct repost of http://reviews.review-board.org/r/608/ which I incorrectly uploaded.)
There is a serious bug in the current LDAP auth backend: it is possible to authenticate as any user by entering an empty password !
This is because the password is used in a bind request, but in LDAP a bind request with an empty password is equivalent to an anonymous bind request, and most LDAP servers allow anonymous bind requests.
But more generally speaking, the design of the LDAP backend is flawed and inflexible. For example with a standard Windows domain controller, a typical DN might look like:
CN=John Smith,OU=engineering,DC=company,DC=com
So if the following User Mask is specified in Review Board's LDAP settings:
CN=%s,OU=engineering,DC=company,DC=com
Endusers are forced to type their full name ("John Smith") as their username, instead of the more practical SAM Account Name (which could be "jsmith" -- Windows domain controllers store it in the sAMAccountName attribute).
The right way to implement LDAP authentication is to do what lighttpd's mod_auth does for example (see http://redmine.lighttpd.net/wiki/lighttpd/Docs:ModAuth). Basically when an enduser enters <enduser-login> and <enduser-passwd>, Review Board should:
(1) Immediately fail the authentication if <enduser-passwd> is empty.
(2) Bind anonymously, or with a given DN/password (<bind-DN> and <bind-passwd>).
(3) Find the enduser's DN <enduser-DN> by doing a SUBTREE search of a <base-DN> (eg. "OU=engineering,DC=company,DC=com") with a filter <filter> built from <enduser-login> (eg. "sAMAccountName=<enduser-login>").
(4) Retrieve <enduser-DN> from the SUBTREE search.
(5) Attempt to bind with <enduser-DN> and <enduser-passwd> to verify the validity of the auth credentials.
So a Review Board administrator needs to specify up to 4 LDAP parameters in addition to the LDAP server:
<bind-DN> (optional)
<bind-passwd> (optional)
<base-DN> (typically something like OU=foo,DC=company,DC=com)
<filter> (typically sAMAccountName=%s for Windows-based domain controllers)
I have written a patch to implement all of this correctly. I have repurposed the 3 existing parameters as follow:
Anonymous User Mask: <bind-DN>
Anonymous User Password: <bind-passwd>
User Mask: <filter>
I don't know Django that well so I have hard-coded <base-DN> in my patch, but ultimately it should be added to the LDAP configuration page /admin/settings/general/. And the description of these settings needs to be updated as well.
Let me know what you think.
-marc
I use this patch on a Review Board server (SVN r1551), the LDAP server is a Windows 2003 DC, and it seems to work well.
CL
-
I would also suggest that when using an LDAP back-end, the mapping from username -> email needs to be more sophisticated. Simply doing shortusername@domain will not work in many environments. You should have an option to retrieve each user's email address from the directory using LDAP. For example, Active Directory has an additional attribute called "mail" which stores the email address.