[python/tornado] ssl improvements #7061
Conversation
Nope I don't think so. |
| ca_certs = configuration.ssl_ca_cert | ||
| else: | ||
| # if not set certificate file, use Mozilla's root certificates. | ||
| ca_certs = certifi.where() |
There was a problem hiding this comment.
@tomplus shall we keep the default certificate certifi.where()?
There was a problem hiding this comment.
This is also handled by the tornado client it seems based on the link provided.
There was a problem hiding this comment.
Latest versions of Tornado use certifi. But it's worth to mention that unstable version from master doesn't have this dependency, certificates from system will be used. A developer will still be able to add certifiy if she/he really needs to do it.
|
|
||
| if hasattr(ssl, 'create_default_context'): | ||
| # require Python 2.7.9+, 3.4+ | ||
| self.ssl_context = ssl.create_default_context() |
There was a problem hiding this comment.
Does the actual tornado client handle creating the ssl_context now, thereby making this redundant?
There was a problem hiding this comment.
Yes, Tornado creates context: https://github.com/tornadoweb/tornado/blob/v4.5.2/tornado/simple_httpclient.py#L248
|
LGTM |
3 participants
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\.3.0.0branch for changes related to OpenAPI spec 3.0. Default:master.Description of the PR
In my last PR (#6968) I had to add some hack to disable SSL in older versions of Python. I looked into the implementation and I found that Tornado also didn't support older version of SSL in Python for security reasons (tornadoweb/tornado#2177). Moreover it implements everything we need and also there is a notice about pycurl which doesn't work with SSLContext. Finally I switched to their implementation and it's simple and works correctly with SimpleAsyncHTTPClient and CurlAsyncHTTPClient.
Question: Do we have something to test SSL in CI pipeline ?
Please take a look: @wing328 @taxpon @frol @mbohlool @cbornet @kenjones-cisco @toumorokoshi