Opened 10 years ago

Closed 8 years ago

#11501 closed enhancement (duplicate)

User authentication via LDAP

Reported by: rmartinjak Owned by: jason, mpatel, was
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: notebook Keywords: notebook, auth, ldap
Cc: rkirov, kini, jasonbhill, novoselt Merged in:
Authors: Reviewers: Robin Martinjak
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Support (optional) user authentication via LDAP or other external backends.

This will be useful for i.e. universities (especially once the notebook is scalable).

Users should also be able to search for (i.e. when adding collaborators to a ws) that are available but not yet known to sage.

See #14330 instead.

Attachments (2)

trac_11501_ldap_auth.patch (23.7 KB) - added by rmartinjak 10 years ago.
ldapauth.py (3.1 KB) - added by rmartinjak 10 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 10 years ago by rmartinjak

patch is based on Rado Kirov's module "python-ldap" (available via easy_install like in Rado's install instructions)

major changes in the patch:

user_manager.py: renamed "valid_login_names" to "known_users" (imho the old name is kind of misleading now) new UserManager? (ExtAuthUsermanager?) new abstract class AuthMethod? (all auth methods used by ExtAuthUM should implement its methods) class LdapAuthMethod? for LDAP authentication

user.py new User.account_type: "external" new methods may_change_email(), may_change_password()

conf.py/server_conf.py added (optional) key POS that allows ordering of items in a config group added config items for LDAP auth made openid login configurable

account_settings.html users only see the "change email/password" form if above mentioned methods return true

worksheet_share.html added a user lookup form

comment:2 Changed 10 years ago by rmartinjak

the first comment should actually say:
patch is based on Rado Kirov's "rkirov-flask" repo.
requires module "python-ldap" (available via easy_install like in Rado's install instructions)
(and the list of changes is missing newlines, sorry for that)

comment:3 follow-up: Changed 10 years ago by ijstokes

Copied over from discussion on sage-notebook.

  1. Which python-ldap instructions from Rado are you referring to? I had trouble easy_installing python-ldap with Sage 4.7, and had to download it, configure setup.cfg by hand, and compile it. I had to get a recent version of OpenLDAP and BerkeleyDB for this to work. If those steps are not required, I'd love to know what I did wrong.
  1. Why did you do:

class ExtAuthUserManager?(OpenIDUserManager):

It would seem to me that this should be reversed. ExtAuthUserManager? should be the generic class, and OpenIDUserManager should turn into an OpenIDAuth otion in the _auth_methods dict.

  1. I would suggest making a list of _auth_methods keys which can be ordered so it is predictable which auth_method is being used first, second, etc.
  1. It doesn't seem very nice to have OpenID-specific references anywhere inside ExtAuthUserManager?. (init and a few OpenID specific methods).
  1. Can you point me at some details of how the "conf" system works? Where does this come from on disk? What is passed to the constructor?
  1. I would have the conf file specify the full URL to the LDAP host, rather than have "host" and "port" configs. We use:

ldaps://hostname

which is impossible to configure for in your current code: it is fixed with "ldap".

  1. Please can you provide an example configuration file? In particular, I'm interested in:

result = self._ldap_search("(%s=%s)" % (self._confldap_username_attrib?, username), attrlist)

My best guess is that "ldap_username_attrib" will typically be "uid", but I can tell you that in our LDAP server the same user may have multiple entries for different LDAP namespaces. We have one namespace for system users, and another for web portal users, so my username ijstokes shows up twice:

uid=ijstokes,cn=users,cn=portal,dc=nebiogrid,dc=org uid=ijstokes,cn=users,cn=accounts,dc=nebiogrid,dc=org

Plus on a big LDAP server this is an expensive search. The admin will know where users are kept, and the search should be limited just to that part of the LDAP tree. In my LDAP implementation, I did a synchronous search as follows:

results = self._ldap_con.search_s("uid=%s,cn=users,cn=portal,dc=nebiogrid,dc=org" % username, ldap.SCOPE_BASE, attrlist=['uid','cn','mail'])

  1. I think you should just setup the LDAP connection once in the constructor. I *think* Python LDAP will manage the connection intelligently.
  1. check_password seems a bit convoluted -- it does two binds: once to get the DN, and once to check the password. I'd prefer the model above.
  1. Sage stores a little bit of additional per-user state: suspended/active and worksheet timeouts or auto-save (can't remember). TTBOMK the OpenID system gets around this by creating a Sage user when someone authenticates for the first time. Do you inherit the same behavior? I can't see where this happens.

comment:4 in reply to: ↑ 3 Changed 10 years ago by rmartinjak

Replying to ijstokes:

Copied over from discussion on sage-notebook. 1. Which python-ldap instructions from Rado are you referring to? I had trouble easy_installing python-ldap with Sage 4.7, and had to download it, configure setup.cfg by hand, and compile it. I had to get a recent version of OpenLDAP and BerkeleyDB for this to work. If those steps are not required, I'd love to know what I did wrong.

My bad, the comment went horribly wrong.

I meant Rado's instruction for getting his flask notebook running (he also uses easy_install from sage's ipython) on top of which my patch is based.

To get python-ldap easy_install'ed you need OpenLDAP's libldap but I can't confirm if that suffices.

  1. Why did you do: class ExtAuthUserManager(OpenIDUserManager): It would seem to me that this should be reversed. ExtAuthUserManager should be the generic class, and OpenIDUserManager should turn into an OpenIDAuth otion in the _auth_methods dict.

You are right. I'm not experienced with OpenID so I have no idea if/how this can be done.

  1. I would suggest making a list of _auth_methods keys which can be ordered so it is predictable which auth_method is being used first, second, etc.

I'd favour that too but unfortunately lists of preset values can not be configured in the UI (yet).

  1. It doesn't seem very nice to have OpenID-specific references anywhere inside ExtAuthUserManager. (init and a few OpenID specific methods).

Indeed. I didn't want to break functionality and OpenID seems to be a quite special case. This should be changed if possible

  1. Can you point me at some details of how the "conf" system works? Where does this come from on disk? What is passed to the constructor?

A reference to a ServerConfiguration (server_conf.py object is passed. Configuration is done in the browser UI (settings -> notebook settings).

  1. I would have the conf file specify the full URL to the LDAP host, rather than have "host" and "port" configs. We use: ldaps://hostname

Right! I will change that immediately, completely forgot about ldaps

  1. Please can you provide an example configuration file? In particular, I'm interested in: result = self._ldap_search ![...] 8. I think you should just setup the LDAP connection once in the constructor. I *think* Python LDAP will manage the connection intelligently.
  1. check_password seems a bit convoluted -- it does two binds: once to get the DN, and once to check the password. I'd prefer the model above.

The first bind is with a "generic" DN (i.e. a non-user account).
After the bind succeeds, LDAP is queried for an object which (italic == configurable item):

  • is below ldap_basedn (this could be cn=users,cn=portal,dc=nebiogrid,dc=org)
  • attribute ldap_username_attribute exactly matches <username>

The generic DN is then unbound and either one ldap object or "None" is returned. After unbinding the connection must be reset with ldap.initialize

If a unique object is returned, we use that object's DN and the provided <password> to try and bind with ldap. If that succeeds, the user has successfully logged in.

See this screenshot for a config example: http://rmartinjak.de/notebooksettings.png

  1. Sage stores a little bit of additional per-user state: suspended/active and worksheet timeouts or auto-save (can't remember). TTBOMK the OpenID system gets around this by creating a Sage user when someone authenticates for the first time. Do you inherit the same behavior? I can't see where this happens.

Done in ExtAuthUM's "_user()"

Changed 10 years ago by rmartinjak

comment:5 follow-up: Changed 10 years ago by dimpase

they say I have to build ldap 2.13 from source on MacOSX 10.6, otherwise ldap doesn't work. see http://stackoverflow.com/questions/6475118/python-ldap-os-x-10-6-and-python-2-6

comment:6 in reply to: ↑ 5 Changed 10 years ago by dimpase

Replying to dimpase:

they say I have to build ldap 2.13 from source on MacOSX 10.6, otherwise ldap doesn't work. see http://stackoverflow.com/questions/6475118/python-ldap-os-x-10-6-and-python-2-6

oops, I meant 2.3.13 After I took the source and did

sage -sh 
python setup.py install

in its source directory, I was able to get to

	  File "/usr/local/src/sage/current/devel/sagenb/flask_version/authentication.py", line 53, in login
	    elif g.notebook.user_manager().check_password(username, password):
	  File "/usr/local/src/sage/sage-4.7.alpha5/devel/rkirov-flask/sagenb/notebook/user_manager.py", line 503, in check_password
	    return self._check_password(username, password)
	  File "/usr/local/src/sage/sage-4.7.alpha5/devel/rkirov-flask/sagenb/notebook/user_manager.py", line 612, in _check_password
	    u = self._auth_methods[a].check_user(username)
	  File "/usr/local/src/sage/sage-4.7.alpha5/devel/rkirov-flask/sagenb/notebook/user_manager.py", line 729, in check_user
	    u = self._get_ldapuser(username)
	  File "/usr/local/src/sage/sage-4.7.alpha5/devel/rkirov-flask/sagenb/notebook/user_manager.py", line 709, in _get_ldapuser
	    result = self._ldap_search("(%s=%s)" % (self._conf['ldap_username_attrib'], username), attrlist)
	  File "/usr/local/src/sage/sage-4.7.alpha5/devel/rkirov-flask/sagenb/notebook/user_manager.py", line 690, in _ldap_search
	    raise ValueError, "invalid LDAP credentials"
	exceptions.ValueError: invalid LDAP credentials

after attempting to log in (well, it could well be that the "Bind DN" is set wrongly in my case).

by the way, there is obvious typo in the patch in flask_version/authentication.py

password = request.form['password'True] 

we guessed that "'True" should be gone there.

comment:7 Changed 10 years ago by dimpase

  • Cc rkirov added

comment:8 Changed 10 years ago by kini

  • Cc kini added

comment:9 Changed 10 years ago by kini

Just by the way, we are currently using this patch (rebased and modified a bit) to do authentication for our Sage-based undergraduate class. Thanks for your work, rmartinjak! Hopefully we can eventually review this and get it into the main notebook codebase (but that will probably take some more discussion).

comment:10 Changed 10 years ago by rmartinjak

I'm glad to hear that.

Actually, the username normalization to ascii is problematic, as it allows logging in with invalid usernames (i.e. rmärtinjak or rm*rtinjak instead of rmartinjak)

I attached a newer class that uses python-ldaps filter_format() method to build valid ldap queries

@dimpase: you're right with the typo and throwing an exception with "invalid LDAP credentials" occurs when either your bind DN or the corresponding bind password is wrong (as you suspected)

Changed 10 years ago by rmartinjak

comment:11 Changed 10 years ago by jasonbhill

  • Cc jasonbhill added

comment:12 Changed 10 years ago by dimpase

One more thing that we just noticed: our LDAP has case-insensitive login names, resulting in nb creating separate sws directories for, say, user bond007, if he logs in as BOND007, or as Bond007, or as bond07.

This has to be a feature of ldapauth, to canonise login names in some way for such LDAP (our is an AD Windows thing).

comment:13 Changed 9 years ago by novoselt

  • Cc novoselt added

comment:14 Changed 9 years ago by kcrisman

  • Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.

This is now basically happening upstream. See this pull request, apparently unrelated to anything here, and the continuation of this material at this fork by rmartinjak.

comment:15 Changed 9 years ago by kini

Correction: the pull request is based on rmartinjak's patch from this ticket, though it was done by someone else (Konstantin Podshumok, who also submitted a nice gettext cleanup and Russian translation).

Personally I am wondering what happened to ijstokes's list of concerns, all of which seem very reasonable to me.

comment:16 follow-up: Changed 9 years ago by kini

Looking at rmartinjak's current "ldap" branch on sagenb, it seems that some of ijstokes's comments have been addressed... OpenIDUserManager now inherits from ExtAuthUserManager rather than vice versa, for example. So apparently the patch here is out of date.

Last edited 9 years ago by kini (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 9 years ago by rmartinjak

Replying to kini:

Looking at rmartinjak's current "ldap" branch on sagenb , it seems that some of ijstokes's comments have been addressed... OpenIDUserManager now inherits from ExtAuthUserManager rather than vice versa, for example. So apparently the patch here is out of date.

This is true, anything but "3." (ordered list of auth methods) should've been taken care of. Maybe also part 1, getting python-ldap, might be an issue. Building it requires libldap (with headers) iirc

Last edited 9 years ago by rmartinjak (previous) (diff)

comment:18 Changed 9 years ago by pipedream

Note from this thread https://groups.google.com/forum/?fromgroups#!topic/sage-support/6DmaZW8cY98 That python-dev needs to be installed (on Ubuntu) to use the instructions in this ticket else import crypto fails.

Last edited 9 years ago by pipedream (previous) (diff)

comment:19 Changed 8 years ago by kcrisman

  • Milestone set to sage-duplicate/invalid/wontfix
  • Status changed from new to needs_review

This is now clearly a duplicate of #14330, or depends on it, or something like that.

comment:20 Changed 8 years ago by kcrisman

  • Dependencies set to #14430
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-pending
  • Status changed from needs_review to positive_review

comment:21 Changed 8 years ago by jdemeyer

  • Authors Robin Martinjak deleted
  • Dependencies #14430 deleted
  • Description modified (diff)
  • Milestone changed from sage-pending to sage-duplicate/invalid/wontfix
  • Reviewers set to Robin Martinjak

comment:22 Changed 8 years ago by jdemeyer

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.