672375 - Authenticate against LDAP

Status: RESOLVED FIXED

(Testing Graveyard :: Sisyphus, defect)

Description

[Mark Côté [:mcote]]

Since sisyphus contains sensitive information, we need to limit the users who can access it.  The most sensible solution appears to be authenticating against the main corporate LDAP server and limiting access to users in the security group(s).

Django provides an LDAP backend, though it looks like we'll have to subclass it for our purposes.

Comment 1

[Mark Côté [:mcote]]

CCing :peterbe, who is alleged to have written something very similar, and :pike, who is also an interested party.

Comment 2

[Axel Hecht [:Pike]]

doh, should've thought about that earlier. check develop branch, not master.

https://github.com/peterbe/sheriffs/commits/develop has a bunch of ldap-related commits.

Comment 3

[Peter Bengtsson [:peterbe]]

Correct. Sheriff is using django-auth-ldap which is awesome. I wrangled some subclasses in there to make it play nicely with the fact that at Mozilla we use email addresses and not usernames. Also, on the Sheriffs app users can change their usernames (e.g. I'm pbengtsson from '[e-mail]' but I prefer to be "called" peterbe)

The relevant code is https://github.com/peterbe/sheriffs/blob/develop/apps/users/auth/backends.py
Ping me if you need any other help related to the connection.

Comment 4

[Mark Côté [:mcote]]

Attachment: Auth backends and views

Here are a couple new backends and the required settings along with two new views to handle logging in and logging out.  All the apis are now locked down to logged-in users.

You'll need to install the django_auth_ldap python package, and you'll need some environment variables that I'll send you separate.

Comment 5

[Peter Bengtsson [:peterbe]]

Comment on attachment 547214 [details] [diff] [review]
Auth backends and views

Review of attachment 547214 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/sisyphus/webapp/bughunter/views.py
@@ +34,5 @@
>          # fall back to parsedatetime
>          date, x = cal.parse(datestring)
>      return time.mktime(date)
>  
> +def login_required(func):

this is already available in stock django under `from django.contrib.auth.decorators import login_required`

@@ +43,5 @@
> +    return wrap
> +
> +@csrf_exempt
> +def log_in(request):
> +    import sys

why import sys?

Comment 6

[Mark Côté [:mcote]]

(In reply to comment #5)
> Comment on attachment 547214 [details] [diff] [review] [review]
> Auth backends and views
> 
> Review of attachment 547214 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: python/sisyphus/webapp/bughunter/views.py
> @@ +34,5 @@
> >          # fall back to parsedatetime
> >          date, x = cal.parse(datestring)
> >      return time.mktime(date)
> >  
> > +def login_required(func):
> 
> this is already available in stock django under `from
> django.contrib.auth.decorators import login_required`

Yeah, it doesn't do what I want though.  The stock decorator will redirect to a login page if the login fails.  However we're designing a client-rich single-page application, so everything will be accessed via AJAX, in which case it makes no sense to return a redirect.  Instead we just return an error, at which point the client will redirect to a login view or the like.

> 
> @@ +43,5 @@
> > +    return wrap
> > +
> > +@csrf_exempt
> > +def log_in(request):
> > +    import sys
> 
> why import sys?

Oops, no reason. :)  I must have been using sys for something while debugging.  I'll remove it.  Thanks!

Comment 7

[Mark Côté [:mcote]]

Attachment: Auth backends and views

Removed unnecessary import.

Comment 8

[Mark Côté [:mcote]]

Attachment: Auth front and back ends along with some major cleanup

Okay this is a big patch that fully implements auth on the back and front ends.

I also spent some time reorganizing and cleaning up the code and architecture, now that I understand Backbone better.  I switched the JS templating system from tempo to ICanHaz, which is better suited for this kind of site.  In general the code should be more comprehensible now.

Comment 9

[Bob Clary [:bc] (inactive)]

This requires openldap, python-ldap, django-auth-ldap not only on the database server but also on the workers.  I'm having a bit of a problem getting openldap, python-ldap and django-auth-ldap installed on Mac OS X and Windows/Cygwin and I'm thinking I really don't need to install them on the workers. 

We aren't yet requiring the workers to login to access post_files right? I presume that it would be a security requirement for the workers to login when posting files. They will just be acting as http clients that use the auth.backends.EmailOrUsernameModelBackend. They won't be authenticating via ldap and will send their credentials to post_files via urlib2 where django will authenticate them against its own auth_user table.

I think we can conditionally exclude the ldap specific code on the workers.


+    def authenticate(self, username=None, password=None):
+        if '@' in username:
+            kwargs = {'email__iexact': username}
+        else:
+            kwargs = {'username': username}
+        try:
+            user = User.objects.get(**kwargs)
+            if user.check_password(password):
+                return user
+        except User.DoesNotExist:
+            return None

If user.check_password(password) returns False, there is no explicit return None. Is one required?

+def log_in(request):
+    if request.method != 'POST':
+        return HttpResponseNotAllowed(['POST'])
+    post = simplejson.loads(request.raw_post_data)
+    username = post['username']
+    password = post['password']
+    user = authenticate(username=username, password=password)
+    if not user or not user.is_active:
+        response = {}
+    else:
+        login(request, user)
+        response = {'username': user.username}
+    json = simplejson.dumps(response)

It isn't clear to me at the moment how to do the authentication for the workers though. If we change the workers to authenticate, would they need to login by posting their username and password to log_in prior to posting content to post_files? Then subsequently send the username and csrf token with all subsequent requests?

If our human users and our python workers are all logging in, shouldn't we use the csrf protection and remove the @csrf_exempt decorators where possible?

If we use csrf, log_in would need to send back the csrf token and the web application and workers would need to save the csrf token and send it back with each request, right?

+def log_out(request):
+    logout(request)
+    return HttpResponse('{}', mimetype='applicatin/json')

misspelled applicatin.

I'm not worthy to comment on the web application part. I'll study it though and test it out when I can get it fully running.

in settings.py, do you think we could consider the workers to not have the LDAP specific environment variables set and to conditionally exclude all of the ldap related code below?

+import ldap
+from django_auth_ldap.config import LDAPSearch, GroupOfNamesType
+
+AUTHENTICATION_BACKENDS = (
+    'auth.backends.EmailOrUsernameModelBackend',
+    'auth.backends.MozillaLDAPBackend',
+    'django.contrib.auth.backends.ModelBackend',
+)

We would need to conditionally create the tuple to exclude the MozillaLDAPBackend.

Move these variables set from the environment to above the import ldap. Then use a conditional on AUTH_LDAP_SERVER_URI ?

+
+AUTH_LDAP_SERVER_URI = os.environ['SISYPHUS_LDAP_SERVER_URI']
+AUTH_LDAP_BIND_DN = os.environ['SISYPHUS_LDAP_BIND_DN']
+AUTH_LDAP_BIND_PASSWORD = os.environ['SISYPHUS_LDAP_BIND_PASSWORD']
+
+AUTH_LDAP_USER_ATTR_MAP = {
+    "first_name": "givenName",
+    "last_name": "sn",
+    "email": "mail",
+}
+
+AUTH_LDAP_USER_SEARCH = LDAPSearch(
+    "dc=mozilla",
+    ldap.SCOPE_SUBTREE,
+    "mail=%(user)s"
+)
+
+AUTH_LDAP_GROUP_SEARCH = LDAPSearch("ou=groups,dc=mozilla",
+    ldap.SCOPE_SUBTREE, "(objectClass=groupOfNames)"
+)
+AUTH_LDAP_GROUP_TYPE = GroupOfNamesType()
+AUTH_LDAP_REQUIRE_GROUP = os.environ['SISYPHUS_LDAP_RESTRICTED_GROUP']

I'll see if I can't tweak the patch over the weekend to get it working on the database server and workers.

Thanks!

Comment 10

[Peter Bengtsson [:peterbe]]

(In reply to Bob Clary [:bc:] from comment #9)
> 
> +    def authenticate(self, username=None, password=None):
> +        if '@' in username:
> +            kwargs = {'email__iexact': username}
> +        else:
> +            kwargs = {'username': username}
> +        try:
> +            user = User.objects.get(**kwargs)
> +            if user.check_password(password):
> +                return user
> +        except User.DoesNotExist:
> +            return None
> 
> If user.check_password(password) returns False, there is no explicit return
> None. Is one required?
>
Nope. The last line `return None` should really be `pass`. Not returning anything in a function automatically becomes the same as `return None`.
 
> +def log_in(request):
> +    if request.method != 'POST':
> +        return HttpResponseNotAllowed(['POST'])
> +    post = simplejson.loads(request.raw_post_data)
> +    username = post['username']
> +    password = post['password']
> +    user = authenticate(username=username, password=password)
> +    if not user or not user.is_active:
> +        response = {}
> +    else:
> +        login(request, user)
> +        response = {'username': user.username}
> +    json = simplejson.dumps(response)
> 
> It isn't clear to me at the moment how to do the authentication for the
> workers though. If we change the workers to authenticate, would they need to
> login by posting their username and password to log_in prior to posting
> content to post_files? Then subsequently send the username and csrf token
> with all subsequent requests?
> 
> If our human users and our python workers are all logging in, shouldn't we
> use the csrf protection and remove the @csrf_exempt decorators where
> possible?
> 

Pretty sure that you still need the CSRF protection either way. Otherwise someone could trick you to click on something on another site, have it posted to this site and because you previously logged in, it'll go ahead with the POST even though you never saw the form.


> If we use csrf, log_in would need to send back the csrf token and the web
> application and workers would need to save the csrf token and send it back
> with each request, right?
> 

> 
> +
> +AUTH_LDAP_SERVER_URI = os.environ['SISYPHUS_LDAP_SERVER_URI']
> +AUTH_LDAP_BIND_DN = os.environ['SISYPHUS_LDAP_BIND_DN']
> +AUTH_LDAP_BIND_PASSWORD = os.environ['SISYPHUS_LDAP_BIND_PASSWORD']
> +
> +AUTH_LDAP_USER_ATTR_MAP = {
> +    "first_name": "givenName",
> +    "last_name": "sn",
> +    "email": "mail",
> +}
> +
> +AUTH_LDAP_USER_SEARCH = LDAPSearch(
> +    "dc=mozilla",
> +    ldap.SCOPE_SUBTREE,
> +    "mail=%(user)s"
> +)
> +
> +AUTH_LDAP_GROUP_SEARCH = LDAPSearch("ou=groups,dc=mozilla",
> +    ldap.SCOPE_SUBTREE, "(objectClass=groupOfNames)"
> +)
> +AUTH_LDAP_GROUP_TYPE = GroupOfNamesType()
> +AUTH_LDAP_REQUIRE_GROUP = os.environ['SISYPHUS_LDAP_RESTRICTED_GROUP']
> 
> I'll see if I can't tweak the patch over the weekend to get it working on
> the database server and workers.
> 
> Thanks!

Comment 11

[Mark Côté [:mcote]]

(In reply to Bob Clary [:bc:] from comment #9)
> This requires openldap, python-ldap, django-auth-ldap not only on the
> database server but also on the workers.  I'm having a bit of a problem
> getting openldap, python-ldap and django-auth-ldap installed on Mac OS X and
> Windows/Cygwin and I'm thinking I really don't need to install them on the
> workers.

Ah sorry, I didn't realize this ran on all the workers.  Yeah no reason to run this on the workers.  I should be able to make all the LDAP stuff conditional on the ability to import the required modules.
 
> If user.check_password(password) returns False, there is no explicit return
> None. Is one required?

As Peter said, it's not necessary, although I admit that it isn't consistent in that function.  I will actually change it to explicitly return None--I think that will make it clearer that auth failed, even though 'return None' is implicit.

> 
> +def log_in(request):
> +    if request.method != 'POST':
> +        return HttpResponseNotAllowed(['POST'])
> +    post = simplejson.loads(request.raw_post_data)
> +    username = post['username']
> +    password = post['password']
> +    user = authenticate(username=username, password=password)
> +    if not user or not user.is_active:
> +        response = {}
> +    else:
> +        login(request, user)
> +        response = {'username': user.username}
> +    json = simplejson.dumps(response)
> 
> It isn't clear to me at the moment how to do the authentication for the
> workers though. If we change the workers to authenticate, would they need to
> login by posting their username and password to log_in prior to posting
> content to post_files? Then subsequently send the username and csrf token
> with all subsequent requests?

Yeah, it would have to be two calls, one to log in, and one to actually send the file.  How are the results being posted now?  If it's by script, then the script will have to have a little bit more logic to grab the session header returned from login and use it when posting the file.  Not hard, though, and I can take a look if you show me where this is done.

And yes, come to think of it, I don't know why we would require csrf exemption anywhere---unless files are being posted via a webpage served from another machine?

> +def log_out(request):
> +    logout(request)
> +    return HttpResponse('{}', mimetype='applicatin/json')
> 
> misspelled applicatin.

Good call, this should probably be a string variable rather than a literal to prevent mistakes like this, since that string is used so often.

> in settings.py, do you think we could consider the workers to not have the
> LDAP specific environment variables set and to conditionally exclude all of
> the ldap related code below?

Yup should be possible.  I'll play with it today.

Comment 12

[Mark Côté [:mcote]]

Adding jeads to cc list.

Comment 13

[Mark Côté [:mcote]]

Attachment: Auth front and back ends along with some major cleanup

New patch with issues addressed.  Also one or two minor fixes (resetting DEBUG back to False, removed unused function parameter).

Comment 14

[Bob Clary [:bc] (inactive)]

Comment on attachment 554945 [details] [diff] [review]
Auth front and back ends along with some major cleanup

r+

Comment 15

[Mark Côté [:mcote]]

Pushed as http://hg.mozilla.org/automation/sisyphus/rev/319a06e076ef with LDAP-group restriction removed.  Still a couple little things to clean up.

Comment 16

[Bob Clary [:bc] (inactive)]

mcote: how much more do we need to do here? can we close this out as fixed?

Comment 17

[Mark Côté [:mcote]]

As far as I recall, this was pretty much good to go. I have no idea what has changed on the back and front ends since then, though.

Comment 18

[Mark Côté [:mcote]]

Old and long since taken care of, one way or another.