-
Notifications
You must be signed in to change notification settings - Fork 360
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
[Generic] tls verify not being honored at the httprequest level when internal_ssl is enabled #326
Conversation
It's also confusing that the code has an |
You're right that it's confusing that there's OAUTH_TLS_VERIFY and OAUTH2_TLS_VERIFY and they behave differently. The GenericOAuthenticator implementation doesn't conform to standards used elsewhere in the code (e.g. OAUTH2 prefix instead of OAUTH), so the OAUTH_TLS_VERIFY is ignored, only OAUTH2_TLS_VERIFY is used. I'll work on unifying those. It is strange that verify_cert is not honored at the HTTPRequest level, since that's explicitly part of the tornado API. We may need to look into what's going on there. |
@minrk How shall we fix this? my PR does, but if you have a better idea I would be up for it. |
Hm, when I set out to test this the tls verify settings worked as intended, so there was nothing to fix. Could you share some configuration and environment details? My env:
and configuration (I used JupyterHub for both the oauth and provider): # oauth consumer
c.JupyterHub.authenticator_class = 'generic-oauth'
c.JupyterHub.spawner_class = 'simple'
c.JupyterHub.ssl_key = 'tls.key'
c.JupyterHub.ssl_cert = 'tls.crt'
c.JupyterHub.port = 9000
c.ConfigurableHTTPProxy.api_url = 'http://127.0.0.1:9010'
c.ConfigurableHTTPProxy.pid_file = 'oauth-chp.pid'
c.JupyterHub.hub_port = 9090
c.JupyterHub.log_level = 10
c.JupyterHub.db_url = 'sqlite://'
c.GenericOAuthenticator.client_id = 'jupyterhub-oauth-client-service'
c.GenericOAuthenticator.client_secret = 'abc1234567'
c.GenericOAuthenticator.basic_auth = False
c.GenericOAuthenticator.userdata_url = 'https://127.0.0.1:8000/hub/api/user'
c.GenericOAuthenticator.authorize_url = 'https://127.0.0.1:8000/hub/api/oauth2/authorize'
c.GenericOAuthenticator.token_url = 'https://127.0.0.1:8000/hub/api/oauth2/token'
c.GenericOAuthenticator.oauth_callback_url = 'https://oauth.local.minrk.net:9000/hub/oauth_callback'
c.GenericOAuthenticator.tls_verify = False
c.GenericOAuthenticator.username_key = 'name'
c.GenericOAuthenticator.extra_params = {
'client_id': c.GenericOAuthenticator.client_id,
'client_secret': c.GenericOAuthenticator.client_secret,
} # oauth provider
c.JupyterHub.authenticator_class = 'dummy'
c.JupyterHub.spawner_class = 'simple'
c.JupyterHub.ssl_key = 'tls.key'
c.JupyterHub.ssl_cert = 'tls.crt'
c.JupyterHub.services = [
{
'name': 'oauth',
'api_token': 'abc1234567',
'oauth_client_id': "jupyterhub-oauth-client-service",
'oauth_redirect_uri': 'https://oauth.local.minrk.net:9000/hub/oauth_callback',
}
] |
For debugging I took master of this repo and just made a single change
|
Aha! This only happens with the curl httpclient, not the default simple httpclient. That narrows it down and I can reproduce it. I'll try to see what the root of it is. |
Wait, no, nevermind, I cannot reproduce it with pycurl, I had accidentally re-enabled tls_verify when I was testing. tls_verify does indeed behave as intended with pycurl in my tests. Is it possible to share some of your z2jh config? Do you perhaps have internal ssl enabled? Since you have inserted some debugging, can you check after creating the request, the values of: req.validate_cert
req.ssl_options
http_client.__class__
http_client.defaults |
Since I'm using zero-to-jupyter they do alot of dynamic config. I do have internal ssl enabled.
|
So is the default client overriding the req... that certainly seems like a bug.
|
I think the reason there's an issue is that there are two "levels" of ssl config, the single params and the ssl options, and only one can be used at a time. So the issue is that the ssl_options being set from defaults configured when internal_ssl is enabled has priority over single flags, even at the request level. I'm not sure this is a bug other than that there's no warning that setting ssl options in two incompatible ways isn't going to work. I think we probably shouldn't be setting global defaults when internal_ssl is enabled, but barring that, overriding |
@minrk I removed the force_instance and it failed. |
Thanks for your patience and testing! |
I don't know if this is a bug in tornado or how it's supposed to function, but tls_verify is not being honored at the HTTPRequest level and is only functional when overriding it at the AsyncHTTPClient level.