Use use ssl_context or don't but don't mix #714
Conversation
| if not ssl_context: | ||
| # if no SSLContext we must make one | ||
|
|
||
| if not ca_certs and verify_certs: |
There was a problem hiding this comment.
ca_certs will only be Empty is certifi is not installed. Might be better if passed into the create_ssl_context function.
| raise ImproperlyConfigured("Root certificates are missing for certificate " | ||
| "validation. Either pass them in using the ca_certs parameter or " | ||
| "install certifi to use it automatically.") | ||
| if ssl_version: |
There was a problem hiding this comment.
if ssl_verison is passed in, we need to make sure this gets applied to the SSLContext object. Fixing coming soon.
| @@ -116,9 +125,6 @@ def __init__(self, host='localhost', port=9200, http_auth=None, | |||
| 'assert_hostname': ssl_assert_hostname, | |||
| 'assert_fingerprint': ssl_assert_fingerprint, | |||
| 'ssl_context': ssl_context, | |||
| 'cert_file': client_cert, | |||
| 'ca_certs': ca_certs, | |||
| 'key_file': client_key, | |||
There was a problem hiding this comment.
by leaving these here in the previous version of the SSLContext iterations, we're "half-assing" the ssl context implemetation. And as such it's causing issues. Since we are creating the SSLContext for the user if they don't pass it in, we shouldn't need to worry about passing through these variables anyway. So i'm removing them
| @@ -105,7 +114,7 @@ def __init__(self, host='localhost', port=9200, http_auth=None, | |||
| except AttributeError: | |||
| ssl_context = None | |||
|
|
|||
| if not verify_certs and ssl_context is not None: | |||
| if not verify_certs: | |||
| ssl_context.check_hostname = False | |||
There was a problem hiding this comment.
this will fail if ssl_context is None. Please add tests
There was a problem hiding this comment.
Ssl_context should never be None, if it is we need to fail loud.
I’ll raise an exception in that case.
fxdgear
changed the title
| @@ -80,7 +80,9 @@ def __init__(self, host='localhost', port=9200, http_auth=None, | |||
| kw = {} | |||
|
|
|||
| # if providing an SSL context, raise error if any other SSL related flag is used | |||
| if ssl_context and (ca_certs or ssl_version): | |||
| if ssl_context and ( use_ssl or verify_certs or ca_certs or client_cert or | |||
There was a problem hiding this comment.
verify_certs defaults to True
fxdgear
force-pushed
the
nick/dontbedumb
branch
from
d7249a3
to
49e1d0d
Compare
Not going to deprecate and replace with SSLContext. But instead give option for using SSLContext next to the original way of handling SSL.
fxdgear
force-pushed
the
nick/dontbedumb
branch
from
46033d1
to
2a175d1
Compare
fxdgear
changed the title
* Use original SSL process and add SSLContext Not going to deprecate and replace with SSLContext. But instead give option for using SSLContext next to the original way of handling SSL.
No description provided.