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.

Sign up for GitHub

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

Jump to bottom

[4.x] Fix LDAP over SSL #37962

Merged
merged 26 commits into from
Jan 30, 2023
Merged

[4.x] Fix LDAP over SSL #37962

merged 26 commits into from
Jan 30, 2023

Conversation

tatankat
Copy link
Contributor

@tatankat tatankat commented Jun 2, 2022 •
edited

Pull Request for Issue (none created)

Summary of Changes

Convert negotiate TLS option to encryption protocol option to re-enable the use of ldap over ssl (ldaps).
I am not too sure about the filename of the database changes, please review and comment.

Testing Instructions

Use an LDAP server with LDAPS. When entering the full ldap URI (ldaps://example.com) in the Host field in V3, it was working.

Actual result BEFORE applying this Pull Request

Joomla was trying to connect to ldap://ldaps://example.com
When only entering the hostname, Joomla was trying to connect to ldap://example.com
The prefix "ldap://" is added by the symfony library.

Expected result AFTER applying this Pull Request

Joomla connects to ldaps://example.com (only) when the SSL encryption protocol is selected - changed behavior wrt V3. Behavior for no encryption and TLS negotiation has not changed.

Documentation Changes Required

Possibly

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.1-dev labels Jun 2, 2022
@richard67
Copy link
Member

richard67 commented Jun 2, 2022 •
edited

I am not too sure about the filename of the database changes, please review and comment.

I can do that tomorrow or on weekend. The file names look ok at a first quick look, and SQL syntax and style looks ok, too, but I have to check the replace for the parameter value because it needs to be very careful with that. We have to make sure to really match the complete parameter. Am too tired today to do that now.

@tatankat
[4.x] Fix LDAP over SSL
1364b8f 
Co-authored-by: Quy <quy@fluxbb.org>
@tatankat tatankat mentioned this pull request Jun 8, 2022
Merged
@tatankat
Copy link
Contributor Author

@richard67 Can you please review?
Is there anything else I can / have to do to make this PR and #37959 to be included in the first possible coming release?

@richard67
Copy link
Member

@richard67 Can you please review? Is there anything else I can / have to do to make this PR and #37959 to be included in the first possible coming release?

Each PR needs 2 successful human tests. It could be hard to find testers for LDAP authentication since it's rarely used. So this alone might take some time.

For the other PR people can maybe use https://www.forumsys.com/2022/05/10/online-ldap-test-server/ , but I'm not sure if it is also suitable for this one for LDAPS.

@tatankat @richard67
More precise WHERE clause and a bit code style.
34e2f4d 
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@tatankat
Copy link
Contributor Author

Thanks for the review and suggestions, I applied them. Now let's hope someone wants to test this...

@tatankat tatankat mentioned this pull request Jun 16, 2022
Closed
@tatankat @richard67
Select correct encryption type with uri ldaps://
28efc35 
and fix element

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@HLeithner HLeithner changed the base branch from 4.1-dev to 4.2-dev June 27, 2022 13:04
@HLeithner
Copy link
Member

This pull requests has automatically rebased to 4.2-dev.

@joomla-bot
Copy link
Contributor

This pull requests has been automatically converted to the PSR-12 coding standard.

@HLeithner
Copy link
Member

@tatankat 2 things

first can you change the sql files to 4.2.0 with date.
2nd do you have experience with ldap server and docker and tests^^ We would really need automated tests for ldap, so having a ldap server automatically provisioned with a users so joomla can login would be great but I don't know if something like this could be made easily.

@richard67
Copy link
Member

@tatankat 2 things

first can you change the sql files to 4.2.0 with date.

@tatankat I suggest you rename both update SQL scripts to "4.2.0-2022-06-28.sql".

@tatankat
Copy link
Contributor Author

Hi @nickdring, those were the wrong files, which explains the bad naming. You are right about the option. If it fails, do not forget to also enable the ldap debug logging and provide it's complete output.
If ICT added a local certificate to your server's trusted certificate store used by ldap, there a chance it will work this time.

@nickdring
Copy link

ok so where are the right files :)

@tatankat
Copy link
Contributor Author

at the same place, I replaced them, you will see the updated names:
Joomla_4.3.0-alpha3-dev-*

@nickdring
Copy link

at the same place, I replaced them, you will see the updated names:
Joomla_4.3.0-alpha3-dev-*

cool, i see the new changes to the plugin now.
But I'm still getting an error, here is what the log says:
2023-01-24T15:58:25+00:00 DEBUG 172.25.0.5 ldap Creating LDAP session with options: {"host":"10.255.8.31","port":3269,"version":2,"referrals":true,"encryption":"ssl","debug":true}
2023-01-24T15:58:25+00:00 DEBUG 172.25.0.5 ldap Creating LDAP session to connect to "ldaps://10.255.8.31:3269" while binding
2023-01-24T15:58:25+00:00 DEBUG 172.25.0.5 ldap Direct binding to LDAP server with entered user dn "nicholas.dring@iit.it" and user entered password
2023-01-24T15:58:25+00:00 ERROR 172.25.0.5 ldap Can't contact LDAP server
2023-01-24T15:58:25+00:00 INFO 172.25.0.5 ldapfailure Username and password do not match or you do not have an account yet.
2023-01-24T15:58:25+00:00 WARNING 172.25.0.5 jerror Username and password do not match or you do not have an account yet.

@nickdring
Copy link

I'm getting my colleagues in IT to check that the LDAP server is actually reachable

@tatankat
Copy link
Contributor Author

tatankat commented Jan 24, 2023 •
edited

@nickdring The kind of log you provided in #37959 (comment) should give more details (that can't be provided by Joomla or php). Please provide it completely.

@nickdring
Copy link

Hi here is the apache:

ldap_url_parse_ext(ldap://localhost/)
ldap_init: trying /etc/ldap/ldap.conf
ldap_init: HOME env is NULL
ldap_init: trying ldaprc
ldap_init: LDAPCONF env is NULL
ldap_init: LDAPRC env is NULL
ldap_create
ldap_url_parse_ext(ldaps://10.255.8.30:3269)
ldap_sasl_bind_s
ldap_sasl_bind
ldap_send_initial_request
ldap_new_connection 1 1 0
ldap_int_open_connection
ldap_connect_to_host: TCP 10.255.8.30:3269
ldap_new_socket: 12
ldap_prepare_socket: 12
ldap_connect_to_host: Trying 10.255.8.30:3269
ldap_pvt_connect: fd: 12 tm: 60 async: 0
ldap_ndelay_on: 12
attempting to connect:
connect errno: 115
ldap_int_poll: fd: 12 tm: 60
ldap_is_sock_ready: 12
ldap_ndelay_off: 12
ldap_pvt_connect: 0
TLS: peer cert untrusted or revoked (0x42)
TLS: can't connect: (unknown error code).
ldap_err2string
ldap_err2string

@tatankat
Copy link
Contributor Author

There you go, as expected. You should configure the ldap client (on OS level) to accept the certificate.

TLS: peer cert untrusted or revoked (0x42)

@Quy Quy removed the PR-4.2-dev label Jan 26, 2023
@nickdring
Copy link

Hi there, my colleagues in ICT are asking if you have any information regarding the configutation of the ldap client.
Thanks.

@tatankat
Copy link
Contributor Author

@nickdring see #37959 (comment) and #37959 (comment)

But, as #37959 and others are being merged and @laoneo is making me happy, I basically undid #35323 and implemented #24115 again using the symfony ldap client (and new php ldap options since 7.1).
So now it is possible to either: ignore ssl certificate issues or to configure the certificate (or it's CA) directly in Joomla. It may take a restart of the webserver to take these settings into account.

New packages with all ldap patches applied are at https://github.com/tatankat/joomla-cms/releases/tag/4.3.0-alpha3-dev-patched

@nickdring
Copy link

Hi @tatankat ok upload patch. Non errors and its working for us.
From everything.php:
2023-01-30T09:56:03+00:00 DEBUG 172.25.0.5 ldap Creating LDAP session with options: {"host":"IITDCWGE001.iit.local","port":3269,"version":3,"referrals":true,"encryption":"ssl","debug":true}
2023-01-30T09:56:03+00:00 DEBUG 172.25.0.5 ldap Creating LDAP session to connect to "ldaps://IITDCWGE001.iit.local:3269" while binding
2023-01-30T09:56:03+00:00 DEBUG 172.25.0.5 ldap Direct binding to LDAP server with entered user dn "nicholas.dring@iit.it" and user entered password
2023-01-30T09:56:03+00:00 DEBUG 172.25.0.5 ldap Searching LDAP entry with filter: "userprincipalname=nicholas.dring@iit.it"
2023-01-30T09:56:03+00:00 DEBUG 172.25.0.5 ldap LDAP login succeeded; username: "nicholas.dring@iit.it", email: "nicholas.dring@iit.it", fullname: "Nicholas Dring"

From apache log:
[11:00] Davide De Marco
dap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_values_len
ldap_next_attribute
ldap_get_dn
ldap_msgfree
ldap_free_connection 1 1
ldap_send_unbind
ldap_free_connection: actually freed

This is good news, when these changes are released we can start planning the migration of our 90+ sites to J4.

Thanks for your help!

@laoneo
Copy link
Member

laoneo commented Jan 30, 2023

@tatankat can you update the update SQL file names to reflect the current date and version 4.3. Then we can merge it.

@brianteeman
Copy link
Contributor

Awesome work - thanks for perservering

@tatankat
Copy link
Contributor Author

Thanks for the tests and confirmation @nickdring.
I have done the requested changes.

@nickdring's tests also confirm that #38388 is working. Can that one also be merged?

@laoneo laoneo merged commit 5ebb39b into joomla:4.3-dev Jan 30, 2023
@laoneo
Copy link
Member

laoneo commented Jan 30, 2023

Thank you very much for help bringing the ldap plugin to a new level. On some point the variables in the plugin need to be renamed to fit more our camelCase style. But for now this is fine.

@laoneo laoneo added this to the Joomla! 4.3.0 milestone Jan 30, 2023
@jgerman-bot jgerman-bot mentioned this pull request Jan 31, 2023
Closed
@joomlapl-bot joomlapl-bot mentioned this pull request Jan 31, 2023
Closed
@noxidsoft
Copy link

Can you folks confirm this change made it to the J 4.3.x release? We are currently on 4.2.7 and looking to rely on the LDAP(S) plugin again for our SSO, rather than the miniorange one.

I'm still getting this error in the logs:

#Date: 2023-10-23 23:40:49 UTC
#Software: Joomla! 4.2.7 Stable [ Uaminifu ] 31-January-2023 15:00 GMT

#Fields: datetime       priority clientip       category        message
2023-10-23T23:40:49+00:00       CRITICAL 192.168.50.97  error   Uncaught Throwable of type TypeError thrown with message "ldap_set_option():>
#1 [ROOT]/libraries/vendor/symfony/ldap/Adapter/ExtLdap/Connection.php(153): Symfony\Component\Ldap\Adapter\ExtLdap\Connection->setOption()
#2 [ROOT]/libraries/vendor/symfony/ldap/Adapter/ExtLdap/Connection.php(73): Symfony\Component\Ldap\Adapter\ExtLdap\Connection->connect()
#3 [ROOT]/libraries/vendor/symfony/ldap/Ldap.php(37): Symfony\Component\Ldap\Adapter\ExtLdap\Connection->bind()
#4 [ROOT]/plugins/authentication/ldap/ldap.php(133): Symfony\Component\Ldap\Ldap->bind()
#5 [ROOT]/libraries/src/Authentication/Authentication.php(175): PlgAuthenticationLdap->onUserAuthenticate()
#6 [ROOT]/libraries/src/Application/CMSApplication.php(816): Joomla\CMS\Authentication\Authentication->authenticate()
#7 [ROOT]/libraries/src/Application/SiteApplication.php(665): Joomla\CMS\Application\CMSApplication->login()
#8 [ROOT]/components/com_users/src/Controller/UserController.php(90): Joomla\CMS\Application\SiteApplication->login()
#9 [ROOT]/libraries/src/MVC/Controller/BaseController.php(672): Joomla\Component\Users\Site\Controller\UserController->login()
#10 [ROOT]/libraries/src/Dispatcher/ComponentDispatcher.php(143): Joomla\CMS\MVC\Controller\BaseController->execute()
#11 [ROOT]/libraries/src/Component/ComponentHelper.php(355): Joomla\CMS\Dispatcher\ComponentDispatcher->dispatch()
#12 [ROOT]/libraries/src/Application/SiteApplication.php(200): Joomla\CMS\Component\ComponentHelper::renderComponent()
#13 [ROOT]/libraries/src/Application/SiteApplication.php(241): Joomla\CMS\Application\SiteApplication->dispatch()
#14 [ROOT]/libraries/src/Application/CMSApplication.php(294): Joomla\CMS\Application\SiteApplication->doExecute()
#15 [ROOT]/includes/app.php(61): Joomla\CMS\Application\CMSApplication->execute()
#16 [ROOT]/index.php(32): require_once('...')
#17 {main}
```<hr /><sub>This comment was created with the <a href="https://github.com/joomla/jissues">J!Tracker Application</a> at <a href="https://issues.joomla.org/tracker/joomla-cms/37962">issues.joomla.org/tracker/joomla-cms/37962</a>.</sub>

@tatankat
Copy link
Contributor Author

@noxidsoft Yes, this made it in 4.3

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

@Quy Quy Quy left review comments

@richard67 richard67 richard67 left review comments

Assignees
No one assigned
Labels
Language Change This is for Translators PR-4.3-dev Unit/System Tests
Projects
None yet
Milestone
Joomla! 4.3.0
Development

Successfully merging this pull request may close these issues.

None yet