search

DirectoryTree / LdapRecord

A fully-featured LDAP framework.
https://ldaprecord.com
MIT License
512 stars 44 forks source link

[Bug] Using port 636 enforces use of schema ldap:// and handshake fails #732

Closed vrob01 closed 2 months ago

vrob01 commented 2 months ago

Environment:

Describe the bug: elabftw uses LdapRecord as its library to enable LDAP-based SSO. In our case, the LDAP server needs to be connected with ldaps://LDAPSERVER:636 (for security reasons I've redacted the actual hostname of the LDAP server).

No matter which settings we use in elabftw, as soon as we change the port to 636 the connection string generated by LdapRecord starts with ldap:// and the handshake with our LDAP server fails. As a very hacky workaround, we patch the source code of LdapRecord by replacing

public const PROTOCOL = 'ldap://

with

public const PROTOCOL = 'ldaps://

in LdapInterface.php. We believe there should be a more sustainable way of achieving an ldaps:// connection string from elabftw.

For reference, here are some discussions in elabftw's repo that have already talked about the issue without finally resolving it: https://github.com/elabftw/elabftw/issues/2787, https://github.com/elabftw/elabftw/discussions/3242

Also pinging @NicolasCARPi for assistance 😄

Thank you for your assistance!

NicolasCARPi commented 2 months ago

For reference, this is how the Connection is made in eLab:

https://github.com/elabftw/elabftw/blob/c4f2d14c6e0eeca9984452f5b890ea0798f78e61/src/controllers/LoginController.php#L238-L262

@stevebauman a pragmatic solution would be to add a protocol key in the array given to Connection object, so we can override it regardless of the TLS/STARTSSL config. What do you think?

Side note: @vrob01 isn't the only eLabFTW user that had to monkeypatch this lib to make it work with their LDAP implementation, so this issue is very important to us. If you don't have the time to look into it and agree with my proposition, I'm willing to submit a PR, but I have a feeling it would be easier for you to do this adjustment ;)

stevebauman commented 2 months ago

Thanks for the ping guys!

Yeah I agree this needs to be changed. Will have a fix out tonight for this!

stevebauman commented 2 months ago

Ok you're all set! I've just released v3.7.0 with a patch for this.

You can now force a protocol manually via the protocol configuration option:

$connection = new \LdapRecord\Connection([
    // ...
    'protocol'  => 'ldaps://',
])
NicolasCARPi commented 2 months ago

Amazing, thank you Steve!

stevebauman commented 2 months ago

Happy to help @NicolasCARPi! 🙏

NicolasCARPi commented 2 months ago

@vrob01 Implemented in https://github.com/elabftw/elabftw/commit/db9ccf9c153dad1bb13e6ded4dc5a8dc6ac246a1