522349 - Add LDAP authentication to Socorro UI

Status: RESOLVED FIXED

(Socorro :: General, task)

Description

[Austin King [:ozten]]

Add LDAP as an optional feature. Logging in will enable restricted features and pages.

Reuse code from Phonebook
http://svn.mozilla.org/projects/phonebook/trunk/
and MCC
http://svn.mozilla.org/projects/mcc/trunk/
where applicable.

Comment 1

[Austin King [:ozten]]

The phonebook does the following:

ldap host: pm-ns.mozilla.org
takes input and makes
mail=[e-mail],o=com,dc=mozilla

Is the ldap host correct?
Should this be broader or only mozilla.com address?

Comment 2

[Samuel Sidler (old account; do not CC)]

We'll want to open it up to community members eventually. Not sure if that should gate us from shipping for just mozilla.com members though. (And actually, is all of mozilla.com too broad?)

Comment 3

[(not currently active) Ted Mielczarek]

I think we should assume that all MoCo employees can be trusted with the privacy of our users. If not, we probably have other problems. But yes, we should get this enabled for other trusted community folks ASAP, but if that's hard and blocks this work then you can probably punt on it, as long as we have a plan.

Comment 4

[Austin King [:ozten]]

Okay, I think I see that I need to bind to 
o=net,dc=mozilla to pickup the community at large.

Comment 5

[Samuel Sidler (old account; do not CC)]

I don't really know how that LDAP data represents the community. i.e., is that everyone with commit access? Or is it some other subset (like MoCo, MeMo, and MoFo)? Either way, nothing to block on, but we should make sure we have the right set eventually.

Comment 6

[Ludovic Hirlimann [:Usul]]

(In reply to comment #2)
> We'll want to open it up to community members eventually. Not sure if that
> should gate us from shipping for just mozilla.com members though. (And
> actually, is all of mozilla.com too broad?)

My entry in the LDAP is with mozillamessaging.com , I'd like to have access.

Comment 7

[Michael Morgan [:morgamic]]

Aravind - any pointers for Austin?

Comment 8

[(not currently active) Ted Mielczarek]

I assume we'll need to either overload an existing permission bit, or create a new one, since we don't want to necessarily grant access to "everyone with an LDAP account", and we'd like to grant access to people without commit access like Ludovic and Wayne.

Comment 9

[Reed Loden [:reed]]

(In reply to comment #8)
> I assume we'll need to either overload an existing permission bit, or create a
> new one, since we don't want to necessarily grant access to "everyone with an
> LDAP account", and we'd like to grant access to people without commit access
> like Ludovic and Wayne.

If everybody involved is a member of the security group, could use the cn=SecurityWiki,ou=groups,dc=mozilla group. Otherwise, yes, would need a new ou=groups,dc=mozilla group.

Comment 10

[Samuel Sidler (old account; do not CC)]

(In reply to comment #3)
> I think we should assume that all MoCo employees can be trusted with the
> privacy of our users. 

Oh, for the record (since I didn't say it in this bug), I didn't mean we couldn't *trust* all MoCo employees, I meant that the more people that had access, the more concern there is that user's privacy could be compromised with a compromised password. Not the most-likely scenario, but when privacy is concerned, I think it's worth the effort. I'd prefer this be "If you don't need access, you don't get it", but I understand if we go for something easier.

Comment 11

[(not currently active) Ted Mielczarek]

Ok, that makes sense. Not to belabor the point, but then Reed's suggestion in comment 9 of using the security group as an access control makes sense as a first pass. We can add a separate group for non-SG people that want access later.

Comment 12

[Austin King [:ozten]]

(In reply to comment #9)
The Auth module we are going to use has some ACL features, but I'd rather not use them. I like this or another LDAP security group is a better way to manage this access.

Comment 13

[Austin King [:ozten]]

Attachment: Adds LDAP authentication

This adds MCC's Auth module. The new files under webapp-php/modules are:
config/ldap.php
libraries/drivers/Auth/LDAP.php
webapp-php/modules/auth/libraries/drivers/Auth/NoAuth.txt

models, controllers, and views can be ignored. libraries MCC code - only change is $_SESSION to use Kohana Session library.

under webapp-php/application - new configs, controller, etc.
If you want to run the patch ping me for config values.

Comment 14

[Reed Loden [:reed]]

Comment on attachment 407588 [details] [diff] [review]
Adds LDAP authentication

Uh, really not sure how this patch is supposed to help things. Most of this patch is completely useless to what we actually need and want. It also does a lot of incorrect assumptions about general LDAP accounts and is extremely specific to MoCo-only (o=com). I also see nothing in this code that handles groups, which is necessary for this bug.

Comment 15

[Austin King [:ozten]]

(In reply to comment #14)
I'll prepare a 2nd revision which 
* doesn't do the login fixed ups (_user_to_dn)
* creates ldap config values for default o and dc values
* creates ldap config values for additional bind variables.

$config['ldap.extra_bind'] = "cn=SecurityWiki,ou=groups";

Does that address the groups issue?

Also: I think I need to file a bug to be added to cn=SecurityWiki,ou=groups. I started to work on comment #9 and was denied access... Ended up forgetting that requirement and it didn't make it into patch attempt #1.

Comment 16

[Reed Loden [:reed]]

(In reply to comment #15)
> (In reply to comment #14)
> I'll prepare a 2nd revision which 
> * doesn't do the login fixed ups (_user_to_dn)
> * creates ldap config values for default o and dc values

Default o isn't an issue and shouldn't be used. You should use a binduser to search based on 'mail' to get the dn to use for each user. Don't try to create the dn yourself; get it from LDAP. Search path (dc) should be in a config, yes.

You need config variables for the following:
* LDAP server (e.g., ldap.mozilla.org) [ldap.server]
* bind dn (e.g., uid=blahblah,dc=logins,dc=mozilla) [ldap.bind_dn]
* bind password (...) [ldap.bind_pass]
* search base (e.g., dc=mozilla) [ldap.search_dn]
* attribute in user to use for searching for a user's dn (e.g., mail) [ldap.user_attr]
* group to require (e.g., cn=SecurityWiki,ou=groups,dc=mozilla) [ldap.require_group]
* attribute in group to use for searching to see if user is a member (e.g., member) [ldap.group_attr]

I recommend making all of these generalized in the config and not Mozilla-specific. IT can fill in the real values at launch time.

Also, make it so if the group field is empty, it's ignored and the check isn't done (so people outside of Mozilla can easily use this same code and not have to use a group like ours if they don't want to).

Sorry if my earlier comment was a bit harsh, but it took me by surprise to see an 80+ KB patch for something that's actually pretty simple.

> * creates ldap config values for additional bind variables.
> 
> $config['ldap.extra_bind'] = "cn=SecurityWiki,ou=groups";
> 
> Does that address the groups issue?

cn=SecurityWiki,ou=groups,dc=mozilla

I wouldn't call it "extra_bind". I'd call it "ldap.require_group" or something (probably pick a better name). Each group is nothing but a list of 'dn's allowed, so you'll need to make sure the user's dn is in that list.

> Also: I think I need to file a bug to be added to cn=SecurityWiki,ou=groups. I
> started to work on comment #9 and was denied access... Ended up forgetting that
> requirement and it didn't make it into patch attempt #1.

If you're not a member of the security group, you shouldn't have access to that group, as it gives access to several other security-group only things. If we're going to need to give people outside the security group access to this admin interface, we might should just go with a new copy that is a copy of SecurityWiki group plus anybody else who needs access.

Hope this helps. Let me know if you don't understand something or need more specifics as to how Mozilla's LDAP schema is set up, and I can help you.

Comment 17

[Austin King [:ozten]]

(In reply to comment #16)
Agreed this is a very simple thing to hack in.

The reason the patch is 88k is that Socorro should work out of the box for other organizations, so I've included MCC Auth (the bulk of the code).

On top of that I've added an LDAP driver and a "NoAuth" mode.

http://code.google.com/p/socorro/wiki/SocorroUIAuthentication - high level background

Comment 18

[Austin King [:ozten]]

Attachment: Second attempt at LDAP patch

LDAP code now uses ldap_list for Authorization. If 'ldap.admin_group' is configured, then the user must be in that group to login in.

I didn't take the time to integrate this into Kohana Auth roles, but that is a natural extension if we later want to do multiple groups for different purposes. For now we will assume 'ldap.admin_group' is set to 'CrashReportsAdmin'.

To test this patch, you can grab config files from /home/aking/breakpad/socorro/webapp-php/config/

Comment 19

[Austin King [:ozten]]

One known issue with this patch...

ldap_logout doesn't work as expected. 

Logout destroys the server session and client's cookie, but it doesn't clear LDAP authentication credentials from the Browser (Fx). If you go to /auth/login your browser will do BASIC Auth w/o prompting you for a username and password. You have to restart to force the browser to prompt for username/password.

I didn't think it was a blocker for this patch, but forgot to mention it.

Background: ldap_logout sends a '401 Unauthorized' which is "supposed" to work. Needs more investigation.

Comment 20

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Comment on attachment 407850 [details] [diff] [review]
Second attempt at LDAP patch

Looking good...  A few minor things...

* Yeah, I was confused when I logged out, and when I tried to login again I was already logged in.  Not a blocker, per se, but will confuse the user.
* Logout will occasionally cause the user to 404 - will send the user to /reporter (http://rsnyder.khan.mozilla.org/reporter/reporter)
* Code should conform to coding standards outlined at - http://code.google.com/p/socorro/wiki/SocorroUI
** Add header documentation to all methods.
** Method names do not always inherently describe method functionality.  e.g. "_ask()" and "_wail_and_bail()"

Comment 21

[Austin King [:ozten]]

(In reply to comment #20)
I've updated doc strings in LDAP.php and auth.php (app controller).
I've renamed these methods to:
_ask_for_credentials
_ask_for_credentials_and_exit
_is_LDAP_login_successful
_handle_errors

Comment 22

[Jeremy Orem [:oremj]]

Comment on attachment 407850 [details] [diff] [review]
Second attempt at LDAP patch

I was just asked to review "modules/auth/libraries/drivers/Auth/LDAP.php". I didn't see any red flags. One nit:

Line 186:
            $password = $_SERVER["PHP_AUTH_PW"];
            
What's the purpose of storing that in a variable?

Comment 23

[Jeremy Orem [:oremj]]

Comment on attachment 407850 [details] [diff] [review]
Second attempt at LDAP patch

Is it neccesary to guess at an e-mail address if "preg_match('/^[a-z]+$/', $user)"?

We shouldn't be creating the dn through an if statement.  You should bind as the anonymous ldap user, search ldap for mail=$user in dc=mozilla and then return the users dn. That mail filter could be configurable, so it's not mozilla specific. Also, the list_dn could come from the config instead of dynamically generating it each run.

In addition, we might as well make "realm" configurable as well instead of making it Mozilla specific.

Comment 24

[Austin King [:ozten]]

(In reply to comment #22)
This is where PHP stores the BASIC auth decrypted password.
http://php.net/manual/en/features.http-auth.php

Comment 25

[Austin King [:ozten]]

Attachment: 3rd version - uses search to find user's dn

This 3rd patch changes a few of the config names and adds a "shared user" for initial bind. It searches for the user by email address and then grabs their dn. function names have been updated and missing docstrings added.

You can grab the actual shared user from my dev environment, patch only contains -dist config files.

Comment 26

[Reed Loden [:reed]]

Comment on attachment 408863 [details] [diff] [review]
3rd version - uses search to find user's dn

Getting better!

>+/**
>+ * The username, email address, or other DN that you use to 
>+ * bind 'anonymously' to your LDAP server for searches
>+ */

Change comment to "The DN used to perform user lookup searches" or something like that.

>+/**
>+ * The password that matches search_dn for binding to LDAP server
>+ */

s/search_dn/bind_dn/

>+/**
>+ * The group dn used with group level authorization such as admin_group option below. 
>+ * Unused if not using group authorization.
>+ */
>+$config['group_dn'] = "ou=groups,dc=example";
>+
>+/**
>+ * To authorize only 1 group of people amoung all of your LDAP authenticated
>+ * users, uncomment this and set it to your LDAP group
>+ */
>+$config['admin_group'] = "CrashCrew";

Combine these into one config option ('group_dn' is fine). Example would be "cn=CrashCrew,ou=groups,dc=example".

You still need to add config options for 'user_attr' and 'group_attr', as per comment #16. Respectively, the options in Mozilla's case will be "mail" and "member". "mail" is extremely non-standard, which is why it definitely should be a config option. "member" is more common, but should pull it out to a config option, too.

>+/** 
>+ * The email address to be used for sending out authentication-related emails.
>+ */
>+$config['email'] = '[e-mail]';
>+
>+/** 
>+ * The site name that will be used to throughout the auth process and in 
>+ * particular to be sent out with each email.
>+ */
>+$config['site_name'] = 'Mozilla Creative Collective';

Get rid of Mozilla-specific stuff for these options.

>+	$realm = Kohana::config('ldap.realm', 'Mozilla Corporation - LDAP Login');

Use $realm = Kohana::config('ldap.realm');

>+	    //Anonymous LDAP access

Technically, we're doing the opposite of anonymous LDAP access by using a bind user for searching, so that comment is wrong.

>+	    $search_dn = Kohana::config('ldap.search_dn', 'dc=mozilla');

Use Kohana::config('ldap.search_dn');

>+	    $user_filter = "mail=" . trim($_SERVER['PHP_AUTH_USER']);

Use the yet-to-be-made Kohana::config('ldap.user_attr'); for "mail". Also, add a filter for objectClass=inetOrgPerson, as it will help with the filter. This doesn't need to be in the config, as it's part of the normal schema.

>+     * @param string $serach_dn - base dn used for searching for users

s/serach/search/

>+	$ldapconn = ldap_connect(Kohana::config("ldap.host"));

hmm, what about SSL? Should we support it?

>+	$search_bind = ldap_bind($ldapconn, $bind_dn, $bind_pass);
>+	if($search_bind) {

If you're feeling really generous, you could make bind_dn and bind_pass optional, meaning that you'd actually do an anonymous bind. However, I don't care, personally.

>+	$filter = "(&(member=" . $user_dn . ")(cn=${cn}))";
>+	$list_dn = Kohana::config('ldap.group_dn', 'ou=groups,dc=mozilla');

Use something like this instead:
$group_dn = Kohana::config('ldap.group_dn');
$group_attr = Kohana::config('ldap.group_attr');
$group_filter = "(&(" . $group_attr . "=" . $user_dn . ")(${group_dn}))";

Also, you probably want to add a filter check on objectClass=groupOfNames (like I mentioned with inetOrgPerson above).

>+	$rs = ldap_list($ldapconn, $list_dn, $filter);//thing about putting in all params here

$list_dn should be $search_dn (dc=example).
$filter should be $group_filter, as per change above.
s/thing/think/

Again, good iteration. You're almost there! :)

As always, if you don't understand something, feel free to ask.

Comment 27

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Comment on attachment 408863 [details] [diff] [review]
3rd version - uses search to find user's dn

Patching is looking good.

Before it rolls, HTTPS should be required on for any basic-auth or form-driven authentication.  Otherwise, passwords will be sent over the wire in clear text.

This concern also raises another question from my end - Is there data that will be shown to the logged-in user that could be considered sensitive data?  If so, then we may want to consider using SSL whenever the potentially sensitive data is displayed on the site.

Comment 28

[Austin King [:ozten]]

(In reply to comment #26)
Updated comments and generalized admin_group to
$config['admin_group'] = "cn=CrashCrew";

(In reply to comment #27)
Added a new config param
$config['proto'] = 'https' and using in layout.php. Devs can set this value to http to avoid using https.

Committing revised version of patch3 in r1411.

Comment 29

[Reed Loden [:reed]]

Huh? I'm confused. Why did you commit a patch that had two review- flags set on it?

Comment 30

[Reed Loden [:reed]]

(In reply to comment #29)
> Huh? I'm confused. Why did you commit a patch that had two review- flags set on
> it?

You even included "r=rsnyder,oremj,reed" in your commit message (http://code.google.com/p/socorro/source/detail?r=1411). Neither oremj nor myself gave this r+, and rsnyder gave the most recent patch r-.

Comment 31

[Austin King [:ozten]]

(In reply to comment #30)
I wanted to recognize the time you put into reviewing this.

Comment 32

[Michael Morgan [:morgamic]]

Austin - we shouldn't have checked it in if people had issues with the patch, but let's work it out.  Ryan - can you pick up this bug so Austin can focus on trending?

Comment 33

[Austin King [:ozten]]

Attachment: 4th attempt with https, updated comments, and generalized admin_group

Comment 34

[Austin King [:ozten]]

(In reply to comment #32)
Sorry for the aggressive check-in. This 4th patch is what was committed. Let me know if I should back this commit out, but this bug is a dependency on the admin panel work so it would be best for Ryan to patch on top of trunk to resolve any review issues.

In terms of why I committed... I talked with oremj and rsnyder outside of bugzilla. As far as I was concerned the only remaining issues were addressed with comment #28 the most serious of which was https. I will do a better job of documenting changes going forward.

Comment 35

[Jeremy Orem [:oremj]]

Comment on attachment 409095 [details] [diff] [review]
4th attempt with https, updated comments, and generalized admin_group

>+	    $user_filter = "mail=" . trim($_SERVER['PHP_AUTH_USER']);
Not a big deal, but we could probably make mail= configurable, so this isn't Mozilla specific.

Comment 36

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Adding minor mods to LDAP in patch for #454620.  Namely fixes some UI issues and redirect problems with login / logout.

Patch is available at https://bugzilla.mozilla.org/show_bug.cgi?id=454620#c18

Comment 37

[Michael Morgan [:morgamic]]

Stage shows:
Fatal error: Class 'Auth' not found in /data/www/crash-reports-php.mozilla.org/webapp-php/application/views/layout.php on line 97

Can we not do an existence check for Auth::* when in a view?

Comment 38

[Austin King [:ozten]]

I'll follow up. This should be working since Bug#525145

Comment 39

[Austin King [:ozten]]

I think Bug#525477 is the issue.

Comment 40

[Austin King [:ozten]]

Attachment: Detects if Auth hasn't been installed via config and issues warnings

This patch checks for Auth and if it doesn't exist, issues warnings on how to fix as well as skipping the site wide messaging slot which depends on the new library "client" which we are using that comes from the auth module.

Comment 41

[Michael Morgan [:morgamic]]

This handles the case where the config file hasn't been udpated, correct?

Comment 42

[Austin King [:ozten]]

That is correct. If config.php hasn't had 'auth' added to the modules list, then it logs a Kohana error and prints an HTML comment with instructions on how to configure Auth.

Comment 43

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

We shouldn't have to add the logic to check for existing modules and correct configuration into the code.  There should be a README.txt in the site checkout that would explain configuration settings for the site and its modules.

Comment 44

[Austin King [:ozten]]

(In reply to comment #43)
Yes I agree. Maybe an "installation health check page" would be the right place.

The root cause is solved:
* config-dist has this value for new installs
* I'll include this change in the deployment directions for the prod release

Also updating the Socorro UI Installation instructions was part of this bug and I had Ryan test the instructions...

Comment 45

[Michael Morgan [:morgamic]]

File a bug for this and we can handle it later (nov or dec).

Comment 46

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Committing final changes for LDAP authentication; see commit associated with ticket #454620