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

Address certificate loading regression #6731

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Address certificate loading regression #6731

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2769cb6
Consider cert settings when using default context
nateprewitt May 29, 2024
e341df3
Set default ca_cert bundle if verify is True
nateprewitt May 31, 2024
da96a92
Correct comment to match actual behavior
nateprewitt May 31, 2024
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
46 changes: 31 additions & 15 deletions src/requests/adapters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,23 @@ def SOCKSProxyManager(*args, **kwargs):
_preloaded_ssl_context = None


def _should_use_default_context(
verify: "bool | str | None",
client_cert: "typing.Tuple[str, str] | str | None",
poolmanager_kwargs: typing.Dict[str, typing.Any],
) -> bool:
# Determine if we have and should use our default SSLContext
# to optimize performance on standard requests.
has_poolmanager_ssl_context = poolmanager_kwargs.get("ssl_context")
should_use_default_ssl_context = (
verify is True
and _preloaded_ssl_context is not None
and not has_poolmanager_ssl_context
and client_cert is None
)
return should_use_default_ssl_context


def _urllib3_request_context(
request: "PreparedRequest",
verify: "bool | str | None",
Expand All @@ -98,25 +115,26 @@ 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"
cert_loc = None
if verify is False:
cert_reqs = "CERT_NONE"
elif verify is True and should_use_default_ssl_context:
elif _should_use_default_context(verify, client_cert, poolmanager_kwargs):
pool_kwargs["ssl_context"] = _preloaded_ssl_context
elif verify is True:
# Set default ca cert location if none provided
cert_loc = extract_zipped_paths(DEFAULT_CA_BUNDLE_PATH)
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is what fixes #6730 right? Because the custome Context + passing this should make their life better, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this should address #6730. #6667 assumed we would always be using the default context which lead to removing these two lines. That was "ok" because we were always forcing a load on the default context at import time.

Starting in 2.32.3, we've opted out of the default in the failure cases reported between 2.32.0-2.32.2. These are cases we're taking a custom context from the user but no longer setting the DEFAULT_CA_BUNDLE on the connection returned from what was previously get_connection. Since we don't know if we have the default SSLContext in cert_verify, I ported this into _urllib3_request_context to ensure we're passing this as pool_kwargs when we've disabled the default.

I validated this against the issue reported in #6726 which was showing this regression after the cert fix in 2769cb6. The other reported issues like #6730 are also passing cleanly now.

elif isinstance(verify, str):
if not os.path.isdir(verify):
pool_kwargs["ca_certs"] = verify
cert_loc = verify

if cert_loc is not None:
if not os.path.isdir(cert_loc):
pool_kwargs["ca_certs"] = cert_loc
else:
pool_kwargs["ca_cert_dir"] = verify
pool_kwargs["ca_cert_dir"] = cert_loc

pool_kwargs["cert_reqs"] = cert_reqs
if client_cert is not None:
if isinstance(client_cert, tuple) and len(client_cert) == 2:
Expand Down Expand Up @@ -316,10 +334,8 @@ def cert_verify(self, conn, url, verify, cert):
if url.lower().startswith("https") and verify:
conn.cert_reqs = "CERT_REQUIRED"

# Only load the CA certificates if 'verify' is a string indicating the CA bundle to use.
# Otherwise, if verify is a boolean, we don't load anything since
# the connection will be using a context with the default certificates already loaded,
# and this avoids a call to the slow load_verify_locations()
# Only load the CA certificates if `verify` is a
# string indicating the CA bundle to use.
if verify is not True:
# `verify` must be a str with a path then
cert_loc = verify
Expand Down
Loading