-
Notifications
You must be signed in to change notification settings - Fork 178
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
Add tls_strategy
and deprecate use_ssl
#258
Add tls_strategy
and deprecate use_ssl
#258
Conversation
f05a98f
to
d9459db
Compare
tls_strategy
and deprecate use_ssl
@loic-vial what do you think about this PR? I hope to resolve your issue with an alternative approach, deprecating |
@@ -58,7 +58,26 @@ async def test_ldap_auth_blank_template(authenticator): | |||
|
|||
async def test_ldap_auth_ssl(authenticator): | |||
authenticator.use_ssl = True | |||
authenticator.server_port = 636 | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment to these two tests saying we're not actually verifying SSL or TLS? for example if someone were to incorrectly modify the tls_strategy logic these tests would still pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d097e74
to
df67ba7
Compare
Hi @consideRatio ! This seems perfect, nice and clean 😄 I did not test it but I'm pretty sure the "before_bind" option will fit our case :) |
df67ba7
to
36ea8ef
Compare
With `tls_strategy` we have three choices on how to secure our connection to the LDAP server. - "on_connect", it is "use_ssl=True" implied - "before_bind", it is what "use_ssl=False" implied - "insecure", this wasn't an option before, but has been requested by users.
8c8a174
to
537d012
Compare
Thank you for the feedback @loic-vial and working towards this in the first place!! Did a self-review and fixed some docs formatting and added a docstring for get_connection -- going for a self-merge here to keep up the maintenance momentum |
Thank you @consideRatio for incorporating these changes! |
Hi, Below is my Jupyterhub config. I have configured it in an AWS EC2 machine using TLJH. I have used the tls_strategy = "insecure". I am using AWS AD (Without TLS). Getting the below error when I tried to log in as an AD user
config.yaml
Am I missing anything? Thanks |
I think it could be a failure where the tljh config isnt propegating that properly, can you try to configure this kot using tljh's custom config, but instead using jupyterhub config directly? I'm on mobile and cant search and link you a documentation reference :/ |
With
tls_strategy
we have three choices on how to secure our connection to the LDAP server. Btls_strategy="insecure"
can disable SSL/TLS entirely, which was the purpose of the new config.tls_strategy="insecure"
being available:use_ssl = False
if it can't startTLS #241auto_bind
config option #179