Add ldap backend to st2auth #1790
Conversation
|
Nice! Thanks for the code. Please make sure that all tests are passing. We'll take a look and provide you some comments so we can merge this in. Stay tuned! |
|
Yep, what james said - thanks for your contribution :) I will review it and add inline comments shortly. |
| self._group_dn = group_dn | ||
|
|
||
| def authenticate(self, username, password): | ||
| search_filter = "uniqueMember=uid=" + username + "," + self._base_dn |
There was a problem hiding this comment.
I'm not that familiar with Python ldap package, but it seems like this opens up a potential injection and security vulnerability if you pass in bad value for username argument.
Is there any safer way to do it (e.g. some way to bind the parameters in the string) or do we need to validate and sanitize the username ourselves?
There was a problem hiding this comment.
The search filter should also allow specifying which attribute of an LDAP entry is queried against. sAMAccountName for Active Directory comes to mind
|
I've added some in-line comments. The pull request looks like a good start - thanks. After the comments are addressed, it would also be good to add some tests and documentation on how to use this backend (see |
|
I didn't find anything how to make the bind safer. And about the tests: I can write a unit test for the backend, but it will work only with my ldap server. How do I write a test which will work everywhere? |
|
@ruslantum I will research bind situation later on. As far as the tests go - unit tests where we mock some of the LDAP client methods should be sufficient for now (integration / end to end tests would also be nice, but that's not a big priority right now). |
|
Btw, if we do plan on using python-ldap then the system on which the build (pip install python-ldap) will need to satisfy http://www.python-ldap.org/doc/html/installing.html#prerequisites. This might need some work to update all our dev environments etc. Also, perhaps some changes will be necessary to update the travis and scrutinizer scripts :) |
| def authenticate(self, username, password): | ||
| search_filter = "uniqueMember=uid=" + username + "," + self._base_dn | ||
| try: | ||
| connect = ldap.initialize(self._ldap_server) |
There was a problem hiding this comment.
I've checked the LDAP package documentation and it would also be good if we supported TLS (we can make it a config option).
See http://www.python-ldap.org/doc/html/ldap.html#ldap.LDAPObject.start_tls_s
|
@ruslantum Sorry for the delay. There is more work which needs to be done on my side before this can get merged - I need to add support for per-backend requirements file. This requires quite a bit of work and right now I'm busy with other things so I can't promise an ETA. In any case, in the mean time I will also try to come up with a quick work-around so we can get this merged without breaking the build and installation without requiring me to do per-backed requirements stuff. |
Note: This backend was originally contributed by Ruslan Tumarkin (StackStorm/st2#1790).
|
Sorry for the delay, but I finally have some good news :) I starting working on the "package per backend" model which means this pull request is now superseded by https://github.com/StackStorm/st2-auth-backend-ldap. You will be able to install this backend when this PR is merged - #1881 (either when a new release with this change is available or as soon as the PR is merged if you are OK with using "dev" packages). Thanks again. |
|
@ruslantum When you have some time it would also be great if you can add some unit tests and open a PR against this repo - https://github.com/StackStorm/st2-auth-backend-ldap Thanks! |
Backend which reads authentication information from a ldap server