requests icon indicating copy to clipboard operation
requests copied to clipboard

Address certificate loading regression

Open nateprewitt opened this issue 1 year ago • 8 comments
trafficstars

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 https://github.com/psf/requests/pull/6710#issuecomment-2137802782 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.

nateprewitt avatar May 31 '24 19:05 nateprewitt

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!

Heraldk avatar Jul 02 '24 10:07 Heraldk

The regression is there for some time and the PR is still in draft. Is there some way to move this forward?

stratakis avatar Sep 04 '24 12:09 stratakis

Maintainers, is there any update here? The latest version of HTTPie is mostly non-functional due to this issue, and we will need to drop it and migrate to other tools if this cannot be addressed.

There does not seem to be any blocking feedback on this PR - can any clarity be provided about what is currently preventing a merge? Is additional contribution required from the community?

ghost avatar Sep 16 '24 15:09 ghost

A friendly reminder that this issue has been stagnant for 4 months already.

kenan-altaki avatar Sep 23 '24 12:09 kenan-altaki

Hi @sigmavirus24 you're listed as reviewer for this. Would you like to review this or assign someone else to do so?

agm-eratosth avatar Oct 04 '24 16:10 agm-eratosth

Could we please move this forward somehow? How can the community help?

This makes our lives harder because we don't know whether to wait for this fix or to adapt to the new behavior caused by the security fix.

frenzymadness avatar Dec 17 '24 10:12 frenzymadness

I found this pull request and one of the associated issues (https://github.com/psf/requests/issues/6730) via the information given in https://github.com/IBM/ibm-cos-sdk-python-core/issues/30. As this seems to be a topic for multiple people, I wanted to ask if it is planned to release this change/fix in some way in the near future. Thank you for your time and for your reply.

adbuerger avatar May 02 '25 15:05 adbuerger

@nateprewitt @Pjrich1313 @sigmavirus24 I hope that I am tagging the right people for this topic since it is unclear to me from the discussion above what aspects are left unresolved regarding this PR. I understand that @sigmavirus24 left comments on the implementations of @nateprewitt and @Pjrich1313 already approved the changes. So are there any aspects that still prevent these changes from being merged? I am not familiar with the details of this topic so I might not understand the remaining issues. Thank you very much for your consideration and for your work on this project.

adbuerger avatar Jun 02 '25 13:06 adbuerger

Resolving in favor of #6767.

nateprewitt avatar Jun 13 '25 16:06 nateprewitt