Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User authentication via LDAP #11501

Closed
sagetrac-rmartinjak mannequin opened this issue Jun 16, 2011 · 22 comments
Closed

User authentication via LDAP #11501

sagetrac-rmartinjak mannequin opened this issue Jun 16, 2011 · 22 comments

Comments

@sagetrac-rmartinjak
Copy link
Mannequin

sagetrac-rmartinjak mannequin commented Jun 16, 2011

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.

Upstream: Reported upstream. Developers acknowledge bug.

CC: @rkirov @kini @sagetrac-jasonbhill @novoselt

Component: notebook

Keywords: notebook, auth, ldap

Reviewer: Robin Martinjak

Issue created by migration from https://trac.sagemath.org/ticket/11501

@sagetrac-rmartinjak
Copy link
Mannequin Author

sagetrac-rmartinjak mannequin commented Jun 16, 2011

comment:1

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

@sagetrac-rmartinjak
Copy link
Mannequin Author

sagetrac-rmartinjak mannequin commented Jun 17, 2011

comment:2

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)

@sagetrac-ijstokes
Copy link
Mannequin

sagetrac-ijstokes mannequin commented Jun 17, 2011

comment:3

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.

  2. 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.

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

  3. 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?

  4. 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._conf['ldap_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.

  2. 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.

  3. 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.

@sagetrac-rmartinjak
Copy link
Mannequin Author

sagetrac-rmartinjak mannequin commented Jun 17, 2011

comment:4

Replying to @sagetrac-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

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 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()"

@dimpase
Copy link
Member

dimpase commented Aug 1, 2011

comment:5

Attachment: trac_11501_ldap_auth.patch.gz

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

@dimpase
Copy link
Member

dimpase commented Aug 1, 2011

comment:6

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.

@kini
Copy link
Collaborator

kini commented Aug 13, 2011

comment:9

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).

@sagetrac-rmartinjak
Copy link
Mannequin Author

sagetrac-rmartinjak mannequin commented Aug 16, 2011

comment:10

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)

@sagetrac-rmartinjak
Copy link
Mannequin Author

sagetrac-rmartinjak mannequin commented Aug 16, 2011

Attachment: ldapauth.py.gz

@dimpase
Copy link
Member

dimpase commented Aug 22, 2011

comment:12

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).

@kcrisman
Copy link
Member

Upstream: Reported upstream. Developers acknowledge bug.

@kcrisman
Copy link
Member

comment:14

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.

@kini
Copy link
Collaborator

kini commented Jun 20, 2012

comment:15

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.

@kini
Copy link
Collaborator

kini commented Jun 20, 2012

comment:16

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.

@sagetrac-rmartinjak
Copy link
Mannequin Author

sagetrac-rmartinjak mannequin commented Jun 20, 2012

comment:17

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

@pipedream
Copy link

comment:18

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.

@kcrisman
Copy link
Member

comment:19

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

@kcrisman
Copy link
Member

Dependencies: #14430

@jdemeyer
Copy link

Changed dependencies from #14430 to none

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

Changed author from Robin Martinjak to none

@jdemeyer
Copy link

Reviewer: Robin Martinjak

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants