Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Don't create default SSLContext if ssl module isn't present #6724

Merged
merged 1 commit into from
May 29, 2024
Merged

Don't create default SSLContext if ssl module isn't present #6724

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions src/requests/adapters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,18 @@ def SOCKSProxyManager(*args, **kwargs):
DEFAULT_RETRIES = 0
DEFAULT_POOL_TIMEOUT = None

_preloaded_ssl_context = create_urllib3_context()
_preloaded_ssl_context.load_verify_locations(
extract_zipped_paths(DEFAULT_CA_BUNDLE_PATH)
)

try:
import ssl # noqa: F401

_preloaded_ssl_context = create_urllib3_context()
_preloaded_ssl_context.load_verify_locations(
extract_zipped_paths(DEFAULT_CA_BUNDLE_PATH)
)
except ImportError:
# Bypass default SSLContext creation when Python
# interpreter isn't built with the ssl module.
_preloaded_ssl_context = None


def _urllib3_request_context(
Expand All @@ -90,13 +98,19 @@ def _urllib3_request_context(
parsed_request_url = urlparse(request.url)
scheme = parsed_request_url.scheme.lower()
port = parsed_request_url.port

# Determine if we have and should use our default SSLContext
# to optimize performance on standard requests.
poolmanager_kwargs = getattr(poolmanager, "connection_pool_kw", {})
has_poolmanager_ssl_context = poolmanager_kwargs.get("ssl_context")
should_use_default_ssl_context = (
_preloaded_ssl_context is not None and not has_poolmanager_ssl_context
)

cert_reqs = "CERT_REQUIRED"
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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this earlier but do we need any logic for verify=True without a condition here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I've a terrible headache so just looking at the branches and concerned about missing something is all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pool_kwargs["ssl_context"] = _preloaded_ssl_context
elif isinstance(verify, str):
if not os.path.isdir(verify):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
Loading