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

Already on GitHub? Sign in to your account

Rewrite LDAP Authentication against ldap3 #843

Merged
merged 1 commit into from Jun 22, 2016

Conversation

Projects
None yet
6 participants
Contributor

mweinelt commented Jun 6, 2016

This makes synapse ldap capable without requiring a system dependency.

Offer a search mode in addition to the pre-existing simple_bind mode.
Searching requires a valid bind_dn and bind_password within the configuration
but allows subtree searches for valid user_dn

Signed-of-by: Martin Weinelt hexa@darmstadt.ccc.de

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

@mweinelt mweinelt changed the title from Rewrite LDAP Authentication against ldap3 to [WIP] Rewrite LDAP Authentication against ldap3 Jun 6, 2016

@mweinelt mweinelt changed the title from [WIP] Rewrite LDAP Authentication against ldap3 to Rewrite LDAP Authentication against ldap3 Jun 6, 2016

@erikjohnston erikjohnston self-assigned this Jun 10, 2016

Owner

erikjohnston commented Jun 10, 2016

@matrixbot ok to test

Can one of the admins verify this patch?

Owner

erikjohnston commented Jun 10, 2016

I think the unit tests failure is spurious.

There are some PEP8 code style violation: http://matrix.org/jenkins/job/SynapseFlake8Packaging/526/violations/file/synapse/handlers/auth.py/

Owner

erikjohnston commented Jun 10, 2016

Does this depend on any particular version of python-ldap3? In particular, will it work with 0.9 (which is the version packaged for older ubuntu/debians)

Contributor

mweinelt commented Jun 10, 2016

Indeed this will break when using the dist-package on Debian Jessie (python3-ldap==0.9.4.2):

Traceback (most recent call last):
  File "/home/matrix/.synapse/bin/synctl", line 4, in <module>
    __import__('pkg_resources').require('matrix-synapse==0.16.0rc1')
  File "/home/matrix/.synapse/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2927, in <module>
    @_call_aside
  File "/home/matrix/.synapse/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2913, in _call_aside
    f(*args, **kwargs)
  File "/home/matrix/.synapse/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2940, in _initialize_master_working_set
    working_set = WorkingSet._build_master()
  File "/home/matrix/.synapse/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 635, in _build_master
    ws.require(__requires__)
  File "/home/matrix/.synapse/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 943, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/home/matrix/.synapse/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 829, in resolve
    raise DistributionNotFound(req, requirers)
pkg_resources.DistributionNotFound: The 'ldap3' distribution was not found and is required by matrix-synapse

This, I believe, is because the package name was changed in 0.9.7.1 2015.01.05:

  • Project renamed from python3-ldap to ldap3 to avoid name clashing with the existing python-ldap library
Contributor

mweinelt commented Jun 12, 2016

  • moved ldap3>=1.0 into optional dependencies
  • rewrote the config parser, stating requirements and testing for ldap3 availability
  • try/except for ldap3 import in auth.py

@erikjohnston erikjohnston commented on an outdated diff Jun 13, 2016

synapse/python_dependencies.py
@@ -48,6 +48,9 @@
"Jinja2>=2.8": ["Jinja2>=2.8"],
"bleach>=1.4.2": ["bleach>=1.4.2"],
},
+ "ldap": {
+ "ldap3": ["ldap3>=1.0"],
@erikjohnston

erikjohnston Jun 13, 2016

Owner

I think you probably want "ldap3>=1.0": ["ldap3>=1.0"], ? As the first number is passed to pip and the second is used to check at runtime

@erikjohnston erikjohnston commented on an outdated diff Jun 13, 2016

synapse/handlers/auth.py
@@ -50,17 +55,16 @@ def __init__(self, hs):
self.INVALID_TOKEN_HTTP_STATUS = 401
self.ldap_enabled = hs.config.ldap_enabled
- self.ldap_server = hs.config.ldap_server
- self.ldap_port = hs.config.ldap_port
- self.ldap_tls = hs.config.ldap_tls
- self.ldap_search_base = hs.config.ldap_search_base
- self.ldap_search_property = hs.config.ldap_search_property
- self.ldap_email_property = hs.config.ldap_email_property
- self.ldap_full_name_property = hs.config.ldap_full_name_property
-
- if self.ldap_enabled is True:
- import ldap
- logger.info("Import ldap version: %s", ldap.__version__)
+ if self.ldap_enabled:
+ self.ldap_mode = hs.config.ldap_mode
@erikjohnston

erikjohnston Jun 13, 2016

Owner

Would be good to fail here if ldap3 isn't imported. See https://github.com/matrix-org/synapse/blob/master/synapse/config/repository.py for an example of how we do it elsewhere.

Owner

erikjohnston commented Jun 13, 2016

@matrixbot retest this please

@erikjohnston erikjohnston commented on an outdated diff Jun 13, 2016

synapse/handlers/auth.py
@@ -50,17 +55,16 @@ def __init__(self, hs):
self.INVALID_TOKEN_HTTP_STATUS = 401
self.ldap_enabled = hs.config.ldap_enabled
- self.ldap_server = hs.config.ldap_server
- self.ldap_port = hs.config.ldap_port
- self.ldap_tls = hs.config.ldap_tls
- self.ldap_search_base = hs.config.ldap_search_base
- self.ldap_search_property = hs.config.ldap_search_property
- self.ldap_email_property = hs.config.ldap_email_property
- self.ldap_full_name_property = hs.config.ldap_full_name_property
-
- if self.ldap_enabled is True:
- import ldap
- logger.info("Import ldap version: %s", ldap.__version__)
+ if ldap3 and self.ldap_enabled:
@erikjohnston

erikjohnston Jun 13, 2016

Owner

ldap3 won't be defined if ldap3 failed to import. I think its probably safe to either a) assume that ldap3 is a thing given we check in the config, b) set ldap3 = None if we fail to import, c) add a assert ldap3 line inside the if block, d) check if ldap3 is defined or e) have a ldap_imported bool set in the try .. except of the import

Contributor

DoubleMalt commented Jun 22, 2016

I endorse this message ... erm pull request ;)

@erikjohnston erikjohnston and 1 other commented on an outdated diff Jun 22, 2016

synapse/handlers/auth.py
defer.returnValue(False)
- import ldap
+ if self.ldap_mode not in ('simple_bind', 'search'):
+ logger.warn('Invalid ldap mode specified: %s', self.ldap_mode)
@erikjohnston

erikjohnston Jun 22, 2016

Owner

I would raise an exception here, given its synapse that sets the ldap_mode

@mweinelt

mweinelt Jun 22, 2016

Contributor

Agreed, that I missed.

Contributor

DoubleMalt commented Jun 22, 2016

There is still a mix of "simple_bind" and "simple" for the modes. Is there a file in Matrix, that holds global constants?

Owner

erikjohnston commented Jun 22, 2016

There is still a mix of "simple_bind" and "simple" for the modes. Is there a file in Matrix, that holds global constants?

There is a synapse/api/constants.py

Contributor

DoubleMalt commented Jun 22, 2016

But that holds """Contains constants from the specification.""" ... These "simple" and "search" are very much implementation detail.

Contributor

DoubleMalt commented Jun 22, 2016

Probably best in synapse/config/ldap.py

Rework ldap integration with ldap3
Use the pure-python ldap3 library, which eliminates the need for a
system dependency.

Offer both a `search` and `simple_bind` mode, for more sophisticated
ldap scenarios.
- `search` tries to find a matching DN within the `user_base` while
  employing the `user_filter`, then tries the bind when a single
  matching DN was found.
- `simple_bind` tries the bind against a specific DN by combining the
  localpart and `user_base`

Offer support for STARTTLS on a plain connection.

The configuration was changed to reflect these new possibilities.

Signed-off-by: Martin Weinelt <hexa@darmstadt.ccc.de>
Owner

erikjohnston commented Jun 22, 2016

Thanks again for this :)

@erikjohnston erikjohnston merged commit 9fe8944 into matrix-org:develop Jun 22, 2016

4 of 5 checks passed

Sytest Dendron (Merged PR) Build started sha1 is merged.
Details
Flake8 + Packaging (Merged PR) Build finished.
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Merged PR) Build finished.
Details
Unit Tests (Merged PR) Build finished.
Details

Can one of the admins verify this patch?

@mweinelt mweinelt deleted the mweinelt:ldap3-rewrite branch Jun 22, 2016

simsasaile commented Aug 11, 2016 edited

Hi @mweinelt,
thanks for the improvement for the ldap-authentication.

How can I get python-ldap3 >=1.0 on debian jessie?

  • With apt-get install python-ldap3 the version is 0.9.4.2 as you already commented above
  • also tried pip install ldap3 but matrix-synapse still doesn't start with the ldap-config, so maybe that's not the problem?

This is my new synapse ldap-config:

ldap_config:
   enabled: true
   uri: "ldap://127.0.0.1:389"
   start_tls: false
   base: "ou=users,dc=example,dc=com"
   attributes:
        uid: "cn"
        mail: "mail"
        name: "givenName"

That's the config which worked till synapse v0.17.0:

ldap_config:
enabled: true
server: "ldap://127.0.0.1"
port: 389
tls: false
search_base: "ou=users,dc=example,dc=com"
search_property: "cn"
email_property: "mail"
full_name_property: "givenName"
Contributor

mweinelt commented Aug 11, 2016

Hi.

Make sure to uninstall python3-ldap3 via apt. Then run pip install --upgrade ldap3.

What issue does synapse report on start?

Best regards

Martin

On 11 August 2016 03:05:06 CEST, simsasaile notifications@github.com wrote:

Hi @mweinelt,
thanks for the improvement for the ldap-authentication.

How can I get python-ldap3 >=1.0 on debian jessie?

  • With apt-get install python-ldap3 the version is 0.9.4.2 as you
    already commented above
  • also tried pip install ldap3 but matrix-synapse still doesn't start
    with the ldap-config

Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

Hi Martin,

thanks for the fast reply even quite late in the night! :)
The package python3-ldap3 wasn't installed.
Synapse doesn't show much at start:

name@server:~$ sudo service matrix-synapse restart 
Job for matrix-synapse.service failed. See 'systemctl status matrix-synapse.service' and 'journalctl -xn' for details.
name@server:~$ sudo service matrix-synapse status -l
● matrix-synapse.service - Synapse Matrix homeserver
   Loaded: loaded (/lib/systemd/system/matrix-synapse.service; enabled)
   Active: activating (auto-restart) (Result: exit-code) since Do 2016-08-11 03:27:31 CEST; 2s ago
  Process: 3291 ExecStart=/usr/bin/python -m synapse.app.homeserver --config-path=/etc/matrix-synapse/homeserver.yaml --config-path=/etc/matrix-synapse/conf.d/ (code=exited, status=0/SUCCESS)
  Process: 6786 ExecStartPre=/usr/bin/python -m synapse.app.homeserver --config-path=/etc/matrix-synapse/homeserver.yaml --config-path=/etc/matrix-synapse/conf.d/ --generate-keys (code=exited, status=1/FAILURE)
 Main PID: 3291 (code=exited, status=0/SUCCESS)

Aug 11 03:27:31 server systemd[1]: Failed to start Synapse Matrix homeserver.
Aug 11 03:27:31 server systemd[1]: Unit matrix-synapse.service entered failed state.

I did add some more infos about my config in the first post, maybe there is something wrong?
If I comment out all ldap config lines, the server is starting normal. Just setting enabled: false is not enough.

Found one problem in my config:
I used tabs for

        uid: "cn"
        mail: "mail"
        name: "givenName"

that's the reason that synapse did not start even with ldap enabled set to false.

Unfortunately synapse is still not running with my ldap-config.

Ok, it's all working now! Added bind_dn and bind_password.

Thanks again for your work and your help!

alephlg commented Oct 10, 2016

This patch broke my LDAP authentication.
I install the synapse 0.18.0 and setup the LDAP some days.
Today, it don't work.

Please help me: what's the news in this ldap auth?

@anmol26s anmol26s referenced this pull request in YunoHost-Apps/synapse_ynh Mar 29, 2017

Closed

502 Bad Gateway error #10

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