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.

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

Address certificate loading regression #6731

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

Conversation

nateprewitt
Copy link
Member

Overview

This PR is intended to address two distinct issues introduced with the default cert optimizations originally introduced in 2.32.0. While we continue to refine the settings considered when opting into our optimized context, we'll no longer use the new default if any custom cert values are supplied. This addresses the concurrency issues raised in #6726.

The second piece of this will be ensuring that when opting out of the default SSLContext, we're still supplying to the default CA Cert bundle correctly. This addresses the problems noticed in #6710 (comment) and #6730.

Considerations

We're now duplicating a decent chunk of the logic from cert_verify inside _urllib3_request_context but without our validation exceptions. That's a potential vector for behavioral shifts in the future. We could consolidate some of this behavior in one place but it's going to require constructing a dict and using setattr on our conn in cert_verify while setting pool_kwargs in _urllib3_request_context. I started writing that up but it feels clunky. This is probably going to be a tradeoff of risking drift like we have with Session settings and binding the two behaviors together too tightly.

Testing

I'd like to codify the issues we've encountered through the whole 2.32.x saga in tests to hopefully avoid this in the future. Doing it cleanly without relying on external endpoints is proving to be a bit more involved than I'd like. I think we can harness some of the infrastructure added in #6662, but I haven't had a chance to really dig into that.

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.

@Heraldk
Copy link

Heraldk commented Jul 2, 2024

Is there an ETA for this change or a similar change? 2.32.3 doesn't work for us, as it broke our usage of requests_pkcs12. It looks like that package was updated with a temporary change here, so if a "proper" fix is not intended to be released anytime soon then we can try that. However I'd be keen to take a proper fix for both if that's expected soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants