Added tls certs and keys settings #87
Conversation
|
Wrong repo! 🙃, feel free to delete. |
|
Where did you mean to open this PR?? It seems this might solve #49? |
|
Yes, it should, i was in a similar situation that OP of #49 is (our LDAP server uses self signed certificates and client keys/certs). However i should add some validation to the inputs before this is merged. PS: Initially i was trying to merge this in the |
| ca_certs_file=ca_cert, validate=ssl.CERT_REQUIRED) | ||
| server = ldap3.Server(self.server_address, port=self.server_port, use_ssl=True, tls=tlsSettings) | ||
| else: | ||
| server = ldap3.Server(self.server_address, port=self.server_port, use_ssl=False) |
There was a problem hiding this comment.
With the last changes it should be now possible to only supply certain options, before that, it was needed to use all the three new settings to work with tls endpoints.
| If you're receiving `SSLV3_ALERT_HANDSHAKE_FAILURE` errors, you have to use this setting. | ||
| """ | ||
| ) | ||
|
|
There was a problem hiding this comment.
I'm still not satisfied with the documentation that i wrote, but i think this could be left for another moment.
|
Hey it would have been really nice if you guys merged this back in 2018 :) Any chance this PR might be reconsidered/updated for merge? |
|
This PR seem to solve a relevant issue for users! Thanks for your work on this @capgadsx and others commenting in to verify the case! I'm not knowledgeable enough to review this =/ |
|
My company is going to force LDAPs as well and this would be great to have in master ;). I vouch to help if necessary |
There was a problem hiding this comment.
If anyone would like to take this on its probably best to open a new PR with these changes and any other fixes. In addition tests would be good, ldaps is supported in the LDAP Docker image used for testing https://github.com/rroemhild/docker-test-openldap
|
Hi, I see that the PR has attracted more interest, so I want to make some changes and write the integration tests that @manics mentions. I'll pick this up when I have some free time. |
|
@capgadsx I might take a look at writing integration tests. |
|
This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there: https://discourse.jupyter.org/t/jupyterhub-ldap-auth-using-certificate-how-to/13604/4 |
| if self.use_ssl: | ||
| ca_cert = None | ||
| client_cert = None | ||
| client_key = None | ||
| if self.server_ca_file != '': | ||
| ca_cert = self.server_ca_file | ||
| if self.client_certificate_file != '': | ||
| client_cert = self.client_certificate_file | ||
| if self.client_key_file != '': | ||
| client_key = self.client_key_file |
There was a problem hiding this comment.
Assuming we go for a merge of this PR, we should also simplify this logic to by letting the new traitlets based config have allow_none=True and letting the default value be None.
This changes will allow us to use
ldaps://endpoints.Closes #49