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

RecursionError when SSLContext imported from urllib3 #121

Closed
nateprewitt opened this issue Nov 1, 2023 · 5 comments · Fixed by #122
Closed

RecursionError when SSLContext imported from urllib3 #121

nateprewitt opened this issue Nov 1, 2023 · 5 comments · Fixed by #122

Comments

@nateprewitt
Copy link

We recently encountered an interesting interaction with boto3 and arcgis due to arcgis' adoption of truststore (boto/boto3#3912). Boto3 has existing usage of the SSLContext from urllib3.utils.ssl_ to work around historical issues with pyopenssl. Most of that no longer exists, but the imports/behaviors are left in place for backwards compatibility.

When truststore.inject_into_ssl() is called, it's currently patching the SSLContext referenced in urllib3 resulting in a RecusionError depending on the order these operations are performed. I've created a minimal repro with only Truststore and urllib3.

Minimal Reproduction

from urllib3.util.ssl_ import SSLContext, PROTOCOL_TLS_CLIENT

import truststore
truststore.inject_into_ssl()

s = SSLContext(PROTOCOL_TLS_CLIENT)
s.options |= 0  # Any arbitrary options int

This issue is avoided if the truststore injection is done first. This is fine for code you control, but becomes more complicated to root cause when a module using truststore is imported later in user code.

Environment details

Original report is for Windows and Python 3.11.5, I've also repro'd on macOS 13.5.2 with Python 3.11.5.

urllib3==2.0.7
truststore==0.8.0

@davisagli
Copy link
Collaborator

Hi @nateprewitt -- we've documented that inject_into_ssl() won't affect modules that have imported SSLContext prior to calling inject_into_ssl(): https://truststore.readthedocs.io/en/latest/#user-guide says "The call to truststore.inject_into_ssl() should be called as early as possible in your program as modules that have already imported ssl.SSLContext won’t be affected."

This means that it generally won't work well if a library tries to use inject_into_ssl(). It's meant for use in the initial entrypoint to an application.

I get that this is not particularly convenient. (And we should clearly have a look at your reproduction case to see if we can at least fail out in a better way than a recursion error.) Ideas welcome.

@nateprewitt
Copy link
Author

nateprewitt commented Nov 1, 2023

Hi @davisagli! Yeah, I spoke with Seth about this on Discord. I agree this behavior is already documented, I think the issue is coming in where 3p libraries are calling inject_into_ssl because the end user 1.) doesn't directly control the order it's happening and 2.) doesn't even know this is happening in most cases.

I'll let Seth expand on his thoughts, but it sounds like this may just be updating docs. It sounds like arcgis' use case may not be intended to be supported but it would be great if there's a clearer way to surface an error when we're in this failure state.

@sethmlarson
Copy link
Owner

Yeah this is an issue with arcgis' usage, we should be guiding library maintainers to not use inject_into_ssl() and instead use SSLContext() since library maintainers aren't in control of import order of their code. I'm creating a PR for that right now.

@sethmlarson
Copy link
Owner

This issue actually makes me wonder if we should deprecate inject_into_ssl() or at least raise a warning now that Truststore has begun being integrated into more libraries and tools using the SSLContext method?

@davisagli
Copy link
Collaborator

@sethmlarson Just spitballing here: show a warning if it's called from a module that is not named __main__?

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 a pull request may close this issue.

3 participants