-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[4.3] Fix LDAP "Bind Directly as User" #37959
Conversation
As you use ldaps, you also need #37962 |
@nickdring Can you confirm it's working with the 2 PRs applied? |
Hi @tatankat I updated to 4.1.5-rc1 but its still not working. Sorry. |
@nickdring The code from these PRs are not yet released in a Joomla version (AFAIK), so you have to apply the changes "manually" to test. As @richard67 said in #37962, only once these changes are tested by several humans, these PRs will be merged and can be included in Joomla. |
Hi @tatankat sorry didn't realise. So I manually added the new changes but it still doesn't work. |
@nickdring You have added the changes from both PRs, #37962 and this one here? If so, you have to edit and save the ldap plugin settings once so that the right encryption setting is used. Or you would have to apply the database changes from the other PR, but that would be too complicated now. Or if you have apploed only the changes from this PR here, you should test with ldap (without s). |
@nickdring In the ldap.php you would have to use an editor and apply the changes from both PRs if you want to use ldaps. Maybe @tatankat can provide you a download of the file with the changes from both PRs if you can't do that. |
If @tatankat can do that for me I'd be happy to try it. |
@nickdring Are you testing on a testing environment or a testing copy of your life site? Or are you using your life site for testing? I'm asking in order to give you the right advise later for testing. If possible you should use a testing environment or a testing copy of your life site. |
I'm testing J4 on a staging, I can break it as much as I like ;) |
@nickdring Good to know about your test environment :) As you don't use [username] in your User's DN, this PR won't do anything. And as the other PR separately does not work, it does not work yet with the two combined. (but hold on) As I was investigating, I found another change of behavior which (probably) also explains why logging in with domain fails (which I suspect you do too). When User's DN is empty, V3 took the entered login, while V4 does not. Except when you use this PR (combined with the other, will give you that next week if still necessary) and put simply [username] in the User's DN. Can you test that? If this does not work, can you give me some more details about your installation and what type of credentials you use to login? |
Hi @tatankat I tried that, but it didn't work. This is our usual set up as per J3. As you can see, LDAP v3 is not activated, and we don't use User's DN or Connect Username |
@tatankat Should @nickdring select an encryption protocol when using a host with "ldaps://"? |
Also important to never bind with empty DN whatever the user enters
@nickdring and @richard67 , yes, the SSL encryption protocol should be selected (I will check if I can improve #37962 for that, as I am apparently not the only one using it this way). The combination of both PRs are in https://github.com/tatankat/joomla-cms/tree/patched/plugins/authentication/ldap (my "patched" branch). This PR now most probably also fixes #36074, #35573 and #35571 |
Hi @tatankat so would you like me to try with the two files in https://github.com/tatankat/joomla-cms/tree/patched/plugins/authentication/ldap ? Do I need to change any of the settings? |
Yes, please. You need to remove the "ldaps://" part in the Host and set "Encryption Protocol" to SSL. When code is accepted to J4, this will be done automatically on upgrade. |
@nickdring That is actually the beginning of a successful connection. But the part you copied is not complete. The full log of 1 session goes up to "ldap_free_connection: actually freed". Please provide the full log of 1 new session. Also, about the TLS_REQCERT option, from the log, it looks like you may have to set it in one of these files instead of C:\openldap\sysconf\ldap.conf: |
This fix works fine for me after I manually changed the 3 files. Before I had to implement the fix presented in the issue #35829 to have a working ldap. |
@mattsh61 That means you have successfully tested this pull request (PR) here? If so, could you go to the PR in the issue tracker here https://issues.joomla.org/tracker/joomla-cms/37959 , click the blue "Test this" button at the top left corner, select your test result (success) and submit? This would be needed to properly count the successful test. Thanks in advance.
@mattsh61 You mean the fix from PR #37962 ? If so: Does it mean you have also tested that PR with success? If so, could you also mark the test result in the issue tracker here https://issues.joomla.org/tracker/joomla-cms/37962 as described above for this PR? Thanks in advance. |
I have tested this item ✅ successfully on 8be9f21 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37959. No, I have not tested code changes from #37962 just the 1 line code change from #35829. And it worked for me. |
I have tested this item ✅ successfully on 8be9f21 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37959. |
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37959. |
Unfortunately this pr changes the behavior of the LDAP plugin in a way that the UI has some variables mandatory where they haven't been before. So I would rebase it to the 4.3 branch. Then we have also more time to detect regressions during the alpha/beta phase. Thanks for understanding. |
@laoneo Does the same apply to #38388 which updates the ldap dependency, and #37962 which replaces a configuration parameter of the plugin? |
Yes |
@laoneo Should the author do that or will we rebase them? |
As soon as 4.3 is up to date, we can do it. |
Thank you @tatankat for the fix! |
Hi there, I'm running the 4.3.0-alpha2-dev+pr.37959 build on a staging server and its still not working for me. In the everything l see: 2022-09-19T07:43:01+00:00 DEBUG 10.255.7.56 ldap Creating LDAP session to connect to "ldaps://10.255.8.30:3269" while binding |
Thanks for the merge @obuisard ! I am not at ease rebasing to 4.3, can someone do that for the other ldap PRs (#38388 & #37962), so it is correctly done? Or tell me how/when it can be correctly done? @nickdring You still need the other PRs applied too and to find your problem, you should look at de ldap client debug logging. Your problem is probably a non-accepted ldap server certificate. To get that working, you should configure the default ldap client options. Once all PRs are accepted (and thus mainly: tested), I can add an additional configuration option to accept non-trusted certificates. |
@tatankat Done the rebase for you :-) |
Thanks! |
Pull Request for Issue #35829 (probably) and restored broken functionality.
Summary of Changes
Simple fix to restore (some of) the LDAP functionality, not needing a full rewrite.
Replace [username] in 'users_dn' configuration as was done before by the Joomla LDAP Client (replaced by the Symfony LDAP framework).
Testing Instructions
Use the LDAP configuration as it was working with V3 with "Bind Directly as User" as Authorisation Method and a User's DN with [username] in it to be replaced as the description says (uid=[username], dc=my-domain, dc=com)
Actual result BEFORE applying this Pull Request
The entered username was used to bind with ldap, which makes no sense as the username is escaped and can't be used as full dn to login to ldap.
Expected result AFTER applying this Pull Request
The configurated users_dn is used with "[username]" replaced by the entered username.
Documentation Changes Required
None, this was broken in V4 vs V3.