search

sethmlarson / truststore

Verify certificates using OS trust stores
https://truststore.readthedocs.io
MIT License
151 stars 18 forks source link

RecursionError when SSLContext imported from urllib3 #121

Closed nateprewitt closed 10 months ago

nateprewitt commented 10 months ago

We recently encountered an interesting interaction with boto3 and arcgis due to arcgis' adoption of truststore (https://github.com/boto/boto3/issues/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 usingtruststore 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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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