Don't create default SSLContext if ssl module isn't present#6724
Don't create default SSLContext if ssl module isn't present#6724nateprewitt merged 1 commit intomainfrom
Conversation
| if verify is False: | ||
| cert_reqs = "CERT_NONE" | ||
| elif verify is True and not has_poolmanager_ssl_context: | ||
| elif verify is True and should_use_default_ssl_context: |
There was a problem hiding this comment.
Missed this earlier but do we need any logic for verify=True without a condition here?
There was a problem hiding this comment.
Are you asking about the behavior if verify is True but should_use_default_ssl_context is False? This was a no-op prior to #6655 which fellback to letting the SSLContext be created when fetching the connection from the pool. _ssl_wrap_socket_and_match_hostname handles this for us, so I don't know if there's any additional concern from our previous behavior unless I'm missing part of the question.
There was a problem hiding this comment.
Makes sense. I've a terrible headache so just looking at the branches and concerned about missing something is all.
There was a problem hiding this comment.
Nope, that's a good call out. I'll give it one more look before merging but I think we're ok for that case.
There was a problem hiding this comment.
Oh, looking again, not on a phone, and without a migraine, I see the cert_reqs = "CERT_REQUIRED" line on L110 that I clearly wrote, and which is what I was thinking we may want to be concerned about. So, I'm not nearly as worried.
| if verify is False: | ||
| cert_reqs = "CERT_NONE" | ||
| elif verify is True and not has_poolmanager_ssl_context: | ||
| elif verify is True and should_use_default_ssl_context: |
There was a problem hiding this comment.
Oh, looking again, not on a phone, and without a migraine, I see the cert_reqs = "CERT_REQUIRED" line on L110 that I clearly wrote, and which is what I was thinking we may want to be concerned about. So, I'm not nearly as worried.
| elif verify is True and should_use_default_ssl_context: | ||
| pool_kwargs["ssl_context"] = _preloaded_ssl_context | ||
| elif isinstance(verify, str): | ||
| if not os.path.isdir(verify): |
There was a problem hiding this comment.
Looking at this again with something of a performance mindset, do we want to cache somehow lookups to isdir? I'm not sure this hurts us at all, but just thinking about things that could slow us down in certain cases now. (Not for this pull request, just putting it out there)
There was a problem hiding this comment.
That seems like a reasonable optimization, I guess we'd need to check how much time we're actually spending on the dir check. I assume we'll get feedback if we have cases where this is a bottleneck.
|
We also need to do this for |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This PR is to address a recent regression in Emscripten support for urllib3. We began unilaterally creating an SSLContext without consideration for Python versions built without an ssl module. This is handled in urllib3 but missed in our usage of
create_urllib3_context.This PR expands on the fix in #6716 by also evaluating both the presence of the
sslmodule before creating and setting the default SSLContext.