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.

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

Added patch for connecting to AD-server with self-signed certificates. #24115

Merged
merged 11 commits into from
Apr 23, 2019

Conversation

rickyosser
Copy link
Contributor

Pull Request for Issue # .

Summary of Changes

Added patches for connecting to AD-server with self-signed certificates.

Testing Instructions

Just install and enable the Ignore Certificate option.

Expected result

Working authentication using TLS/LDAPS with servers that are configured with self-signed certificates. For example recent SAMBA4.

Actual result

Working authentication using TLS/LDAPS with servers that are configured with self-signed certificates. For example recent SAMBA4.

Documentation Changes Required

Option:
"Ignore Certificate"
Desc:
When enabled ignore the server certificate, this is useful when running for example Samba 4 with a self-signed certificate.

@rickyosser rickyosser changed the title Added patch for connecting to AD-server with self-signed certificates. [plugin/authetication/ldap] Added patch for connecting to AD-server with self-signed certificates. Mar 7, 2019
{
putenv('LDAPTLS_REQCERT=never');
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove tabs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the spaces.
Hmm, emacs acting up. php-mode seems to default to PEAR-style. Which uses spaces.

Copy link
Member

@HLeithner HLeithner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the language keys to match the field name, atm the key says the opposite then the description.

plugins/authentication/ldap/ldap.xml Outdated Show resolved Hide resolved
plugins/authentication/ldap/ldap.xml Outdated Show resolved Hide resolved
@HLeithner
Copy link
Member

Now we need 2 testers...

@brianteeman
Copy link
Contributor

@HLeithner it is very unlikely that we will get 2 testers for this as it is very specialised and not used often. I suspect the only option is to code review

@HLeithner
Copy link
Member

Yeah I have the same feeling

@HLeithner
Copy link
Member

Would you at least test if the plugin screen doesn't break?

@brianteeman
Copy link
Contributor

Tested and the screen doesnt break. Not sure if debug should be the very first option. But I have never used LDAP authentication so dont know if it is important or not

@HLeithner
Copy link
Member

It's the first? request for this feature ;-) so properly the first position is not perfect....

I would suggest moving debug to the last option and the tls thing before debug.

@@ -16,6 +16,10 @@ PLG_LDAP_FIELD_HOST_DESC="Eg: openldap.example.com."
PLG_LDAP_FIELD_HOST_LABEL="Host"
PLG_LDAP_FIELD_NEGOCIATE_DESC="Negotiate TLS encryption with the LDAP server. This requires all traffic to and from the LDAP server to be encrypted."
PLG_LDAP_FIELD_NEGOCIATE_LABEL="Negotiate TLS"
PLG_LDAP_FIELD_LDAPDEBUG_DESC="Enables debug hardcoded to level 7"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sort keys in alpha order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the first? request for this feature ;-) so properly the first position is not perfect....

I would suggest moving debug to the last option and the tls thing before debug.

I moved the debug option to last but the Ignore certificate is closely connected to enabling TLS and thus should be the next option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should still be sorted in alpha order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the file after last nights commit 224ec41, the file is in order.

* Ignore TLS Certificate (encrypted communications)
*
* @var boolean
* @since 1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @since 1.0
* @since __DEPLOY_VERSION__

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

* Enable LDAP debug (encrypted communications)
*
* @var boolean
* @since 1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @since 1.0
* @since __DEPLOY_VERSION__

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@mbabker
Copy link
Contributor

mbabker commented Mar 8, 2019

A Composer dependency is changed without having updated the lock file. That should be a red flag to somebody...

@HLeithner
Copy link
Member

So good that you are here^^

@rickyosser parts of the PR have to be made against https://github.com/joomla-framework/ldap/blob/master/src/LdapClient.php

@rickyosser
Copy link
Contributor Author

A Composer dependency is changed without having updated the lock file. That should be a red flag to somebody...

Can you please point to where the composer dependency would change, I'm looking at the diff-file I originally created and it closeley looks like the git PR I'm trying to create. It only touches 3 files and none of them is a Composer control-file.

@brianteeman
Copy link
Contributor

It is this file libraries/vendor/joomla/ldap/src/LdapClient.php
which should be made against https://github.com/joomla-framework/ldap/blob/master/src/LdapClient.php

@rickyosser
Copy link
Contributor Author

rickyosser commented Mar 10, 2019

So good that you are here^^

@rickyosser parts of the PR have to be made against https://github.com/joomla-framework/ldap/blob/master/src/LdapClient.php

@brianteeman and @HLeithner , I've created a pull request in the LDAP tree for the changes in that file.
Sorry for not knowing the structure of the Joomla development project. I created some patches to solve a real customer case and thought it would be good to add them upstream so future upgrades will be seamless.

Anyway this problem is relatively new as the security hardening of SAMBA4 and the openldap-client library have been made in the last year/years.

@rickyosser
Copy link
Contributor Author

rickyosser commented Mar 10, 2019

My PR joomla-framework/ldap@c7e30ce has been accepted in joomla-framework/ldap, what do you need me to do here now?

@HLeithner
Copy link
Member

HLeithner commented Mar 10, 2019

Remove the changes in the ldapclient file, I will update the lib after releasing 3.9.4.

@rickyosser
Copy link
Contributor Author

Remove the changes in the ldapclient file, I will update the lib after releasing 3.9.4.

@HLeithner, the file is now reverted.

@ghost ghost added J3 Issue and removed J3 Issue labels Apr 5, 2019
@@ -12,8 +12,12 @@ PLG_LDAP_FIELD_EMAIL_DESC="LDAP attribute which has the User's email address."
PLG_LDAP_FIELD_EMAIL_LABEL="Map: Email"
PLG_LDAP_FIELD_FULLNAME_DESC="LDAP attribute which has the User's full name."
PLG_LDAP_FIELD_FULLNAME_LABEL="Map: Full Name"
PLG_LDAP_FIELD_IGNORE_REQCERT_TLS_DESC="When enabled ignore the server certificate, this is useful when running for example Samba 4 with a self-signed certificate."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move lines 15-16 after line 18.

@ghost ghost changed the title [plugin/authetication/ldap] Added patch for connecting to AD-server with self-signed certificates. Added patch for connecting to AD-server with self-signed certificates. Apr 19, 2019
@ghost ghost removed the J3 Issue label Apr 19, 2019
@HLeithner HLeithner merged commit 380fd61 into joomla:staging Apr 23, 2019
@HLeithner
Copy link
Member

thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants