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

Fix TLS dialing #16

Merged
merged 3 commits into from
Sep 24, 2018
Merged

Fix TLS dialing #16

merged 3 commits into from
Sep 24, 2018

Conversation

Luzifer
Copy link
Owner

@Luzifer Luzifer commented Sep 22, 2018

refs #14

Signed-off-by: Knut Ahlers <knut@ahlers.me>
@Luzifer Luzifer self-assigned this Sep 22, 2018
@Luzifer
Copy link
Owner Author

Luzifer commented Sep 22, 2018

@Xaroth this should fix the TLS issues you're having with LDAPs connections. Sadly I don't have a LDAPs server to test the change. Do you have a chance to test it?

@Xaroth
Copy link

Xaroth commented Sep 24, 2018

It works in part; however, there is a caveat:

To avoid DNS hijacking, a common configuration setup is to point ldap to a fixed IP address rather than a domain name (i.e. ldaps://127.0.0.1 ). By setting ServerName to host, verification fails, because the certificate for the LDAP server is issued for 'localhost' (or 'ldap.domain.tld'), not '127.0.0.1'. Now this can be worked around by hard-locking the dns name in /etc/hosts , but a cleaner method would be to introduce a config setting server_name, where you can configure the expected server name of the LDAP host (defaulting to the host part of the server URI, possibly).

@Xaroth
Copy link

Xaroth commented Sep 24, 2018

Adding to this, it might also be an option to allow to set the InsecureSkipVerify setting, for self-signed certificates. I don't recommend people using that in production envs, but in test envs it's very common to see self-signed certs being used to test.

Signed-off-by: Knut Ahlers <knut@ahlers.me>
@Luzifer
Copy link
Owner Author

Luzifer commented Sep 24, 2018

I've added configuration to support both validations: Either override the hostname for the certificate validation or disable the validation.

Please have another look. 🙂

@Xaroth
Copy link

Xaroth commented Sep 24, 2018

That seems to work fine, I left one remark on the commit, but other than that it works in my test environments

👍

Signed-off-by: Knut Ahlers <knut@ahlers.me>
@Luzifer
Copy link
Owner Author

Luzifer commented Sep 24, 2018

Now both parameters are taken into account. I'm not entirely sure what happens in the TLS connection when both parameters are set but they should work.

If you could test the behaviour with both options set this would be great!

@Xaroth
Copy link

Xaroth commented Sep 24, 2018

InsecureSkipVerify will (if memory serves me well) cause the TLS validation step to skip, therefore not checking for cert validity, CA validity or hostname comparison, so ServerName then becomes nothing more than an indicator (and could even be wrong/missing/empty).

The potential issue that it solves is that how you have it now, people can keep their config, but just add the allow_insecure option to debug.

I checked the changes, they seem to work fine 👍

@Luzifer
Copy link
Owner Author

Luzifer commented Sep 24, 2018

In that case: Thank you very much for bringing up this issue and for the assistance testing the changes! 🙂

@Luzifer Luzifer merged commit 05fe4f2 into master Sep 24, 2018
@Luzifer Luzifer deleted the fix-tls-dialing branch September 24, 2018 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants