PYTHON-5309 Async client no longer works on Atlas mongodb Cloud #2286

sleepyStick · 2025-04-15T22:12:13Z

jira: https://jira.mongodb.org/browse/PYTHON-5309
problem: if a user had pyopenssl installed on their machine, the async client wouldnt configure ssl properly
brief description of my changes:

ssl_context will try to load both stdlib ssl and pyopenssl
the driver will now dynamically determine which ssl to use (if the user has both) depending on async vs sync context
to get this to work, AutoEncryptionOpts delays the resolution of _kms_ssl_contexts

…ure if this is correct or not, we'll seeeeee)

ShaneHarvey · 2025-04-17T17:58:48Z

CONTRIBUTING.md

@@ -420,3 +420,21 @@ partially-converted asynchronous version of the same name to the `test/asynchron
 Use this generated file as a starting point for the completed conversion.

 The script is used like so: `python tools/convert_test_to_async.py [test_file.py]`
+
+## Running PyMongo with SSL


What's the motivation for adding this section?

Oh uh, I was struggling to get it working and asked Noah a bunch of questions, so he suggested I add a section here.

ShaneHarvey · 2025-04-17T17:59:33Z

pymongo/encryption_options.py

@@ -58,6 +58,7 @@ def __init__(
        bypass_query_analysis: bool = False,
        encrypted_fields_map: Optional[Mapping[str, Any]] = None,
        key_expiration_ms: Optional[int] = None,
+        is_sync: bool = True,


This is a public api so we can't add "is_sync" here. What's the motivation for this?

Oh, I added is_sync as a param to a few functions because in ssl_support.get_ssl_context(), we'll need to know which version of ssl should be used,, is there another way to go about this that is preferred?

Thanks, we still can not add "is_sync" here so we need to find another way.

One way would be to lazily init the async SSLContext in kms_request().

I think I did it? Not sure if this is what you meant, but what I have now would leave the params to AutoEncryptOpts unchanged.
I basically delayed the parse_kms_tls_options because that's where is_sync was needed. I believe i appropriately delayed the definition of kms_tls_options but let me know if I missed something. I'm not super familiar with how encryption in the driver works >.<

.evergreen/generated_configs/variants.yml

NoahStapp · 2025-04-21T18:00:56Z

Can you schedule a run of all the pyopenssl variants/tasks?

sleepyStick · 2025-04-21T18:09:45Z

Can you schedule a run of all the pyopenssl variants/tasks?

oops yeah, sorry i'll add it to the latest run with the updated evergreen patch changes from steve's PR

.evergreen/generated_configs/variants.yml

pymongo/network_layer.py

NoahStapp · 2025-04-22T15:39:34Z

pymongo/network_layer.py

-        BLOCKING_IO_WRITE_ERROR,
-        _sslConn,
-    )
+    from pymongo.pyopenssl_context import _sslConn as _pysslConn


What's the motivation behind the renaming to _pysslConn here?

When i was making these changes, I started interpreting py* to mean it would refer to pyopenssl (if it existed, and stdlib ssl if it did not) whereas ssl related vars without py in the front meant it was strictly referring to stdlib ssl. I'm not strongly attached to the renaming and wouldn't mind undoing it, but it helped my brain read the code.

I'd prefer the old name (_sslConn). A larger refactor could be a good idea but I'd prefer this PR be as minimal as possible to avoid regressions because it's going into a bugfix release.

NoahStapp · 2025-04-22T15:41:48Z

pymongo/ssl_support.py

@@ -57,6 +60,20 @@
    BLOCKING_IO_WRITE_ERROR = _ssl.BLOCKING_IO_WRITE_ERROR
    BLOCKING_IO_LOOKUP_ERROR = BLOCKING_IO_READ_ERROR

+    if HAVE_PYSSL:


Can we use Union types here to remove the need for two separate exports of everything? Something like:

BLOCKING_IO_ERRORS = _ssl.BLOCKING_IO_ERRORS | _pyssl.BLOCKING_IO_ERRORS

Are there any situations where we specifically care if an exception type is from PyOpenSSL or stdlib SSL?

ooo good call out. I don't think we particularly care if its pyopenssl vs stdlib ssl error.

I think python3.9 didn't love the union types? so i just made them tuples. But its a similar idea.
I couldn't apply this to SSLError though because sometimes we'd raise SSLError so it needed to be one specific type.

test/asynchronous/test_encryption.py

ShaneHarvey · 2025-04-22T20:07:44Z

pymongo/ssl_support.py

    IPADDR_SAFE = True
+
+    if HAVE_PYSSL:
+        HAS_SNI = _pyssl.HAS_SNI | _ssl.HAS_SNI


This logic for HAS_SNI is problematic since this happens at import time but the actual ssl impl at runtime (stdlib vs pyopenssl) may or may not support SNI. I believe we need something like:

# We have to pass hostname / ip address to wrap_socket # to use SSLContext.check_hostname. if ssl_context.has_sni: ...

Hmm I don't think I understand the problem? HAS_SNI was previously defined in the same spot. Did my changes introduce this problem? Or did the previous code also have the same problem?

The new logic is incorrect. If pyopenssl is install and _pyssl.HAS_SNI=False and _ssl.HAS_SNI=True, the sync code will incorrectly think that pyopenssl supports SNI.

Ah, okay i understand how the logic is incorrect now (I previously thought that the two would be the same value regardless of pyopenssl vs stdlibssl)
would you mind elaborating on your suggested fix though? I'm not sure I understand how that would resolve pyopenssl's has_sni and stdlib ssl's has_sni being two different values and how to navigate having those two potentially different values in the rest of the code?

Where we actually use HAS_SNI in pool_shared.py, we need to delegate to the appropriate _pyssl.HAS_SNI or _ssl.HAS_SNI property. Perhaps via a new helper function.

Not sure if I did it the way you envisioned it, but I created a helper function, has_sni(is_sync) -> bool, and call that everywhere HAS_SNI was being used.

ShaneHarvey · 2025-04-22T20:12:52Z

test/asynchronous/test_encryption.py

-            {}, "keyvault.datakeys", mongocryptd_spawn_args=["--idleShutdownTimeoutSecs=88"]
+            {},
+            "keyvault.datakeys",
+            mongocryptd_spawn_args=["--idleShutdownTimeoutSecs=88"],


Could we revert these format-only changes?

ShaneHarvey · 2025-04-22T20:14:03Z

pymongo/encryption_options.py

        self._bypass_query_analysis = bypass_query_analysis
        self._key_expiration_ms = key_expiration_ms

+    def _parse_kms_tls_options(self, is_sync: bool) -> None:
+        self._kms_ssl_contexts = _parse_kms_tls_options(self._kms_tls_options, is_sync)


This is problematic because a single AutoEncryptionOpts can be shared between multiple clients (even sync + async) at the same time. Currently the _kms_ssl_contexts will clash.

Another problem is that _parse_kms_tls_options is expensive and should only be called at most once for sync and async.

What if we do this at construction time?:

# Maps KMS provider name to a SSLContext. self._kms_ssl_contexts = _parse_kms_tls_options(kms_tls_options, True) if pyopenssl_enabled: self._async_kms_ssl_contexts = _parse_kms_tls_options(kms_tls_options, False) else: self._async_kms_ssl_contexts = self._kms_ssl_contexts

Hmm actually, I thought of a better idea. _EncryptionIO can parse and store the contexts:

class _EncryptionIO(AsyncMongoCryptCallback): def __init__( self, client: Optional[AsyncMongoClient[_DocumentTypeArg]], key_vault_coll: AsyncCollection[_DocumentTypeArg], mongocryptd_client: Optional[AsyncMongoClient[_DocumentTypeArg]], opts: AutoEncryptionOpts, ): ... self.opts = opts self._kms_ssl_contexts = _parse_kms_tls_options(opts._kms_tls_options, IS_SYNC) ...

ShaneHarvey · 2025-04-22T20:15:04Z

pymongo/ssl_support.py

+        if is_sync and HAVE_PYSSL:
+            ssl_in_use: types.ModuleType = _pyssl
+        else:
+            ssl_in_use = _ssl


Can we rename ssl_in_use -> _ssl that way there's less code churn.

I don't think so? because _ssl is referring to stdlib ssl and i don't think we'd want it to be replaced by pyopenssl just because pyopenssl is installed?

It's a local variable assignment so what's the issue?

if is_sync and HAVE_PYSSL: _ssl: types.ModuleType = _pyssl else: _ssl = _ssl

Or even just:

if is_sync and HAVE_PYSSL: _ssl: types.ModuleType = _pyssl

Ah, no you're right -- I've been conditioned to think that shadowing a global var with a local var is generally bad for code readability(?) so my brain forgot that it was something we could do.

Er, am i doing it incorrectly? I'm seeing the error "UnboundLocalError: local variable '_ssl' referenced before assignment"

Uh, I swore I've done this before. I guess not. The only way around that is like this:

if is_sync and HAVE_PYSSL: _ssl: types.ModuleType = _pyssl else: _ssl = globals()["_ssl"]

Or by renaming the global "_ssl".

I actually kinda wanted to rename the global _ssl except i think there were some other modules that imported _ssl from ssl_support and then i wasn't sure if i was allowed to change it...

You know what, I think it's simpler to go back to the the local ssl_in_use var or just rename ssl_in_use to ssl? Apologies for the back and forth.

All good, went with ssl because its shorter.

NoahStapp · 2025-04-23T14:23:03Z

doc/changelog.rst

@@ -12,6 +12,7 @@ Version 4.12.1 is a bug fix release.
 - Fixed a bug that caused direct use of ``pymongo.uri_parser`` to raise an ``AttributeError``.
 - Removed Eventlet testing against Python versions newer than 3.9 since
  Eventlet is actively being sunset by its maintainers and has compatibility issues with PyMongo's dnspython dependency.
+- Fixed a bug that would try to use PyOpenSSL with AsyncMongoClient, causing the client to fail.


Make this more specific to the exact error reported, something like:

Fixed a bug that would cause AsyncMongoClient to attempt to use PyOpenSSL when available, resulting in errors such as "pymongo.errors.ServerSelectionTimeoutError: 'SSLContext' object has no attribute 'wrap_bio'"

NoahStapp · 2025-04-23T14:34:31Z

pymongo/pool_shared.py

@@ -138,7 +138,7 @@ def _raise_connection_failure(
        msg += format_timeout_details(timeout_details)
    if isinstance(error, socket.timeout):
        raise NetworkTimeout(msg) from error
-    elif isinstance(error, SSLError) and "timed out" in str(error):
+    elif isinstance(error, (SSLError, PYSSLError)) and "timed out" in str(error):


Do these need to be separate SSLError and PYSSLError types, or can we export a shared one from ssl_support to reduce code churn?

Err, I tried to do something like SSLError = (_ssl.SSLError, _pyssl.SSLError) in ssl_support but i think it was in network_layer where we do a raise SSLError() so it needed to be a specific type. Is there another way to accomplish what you're describing that would work?

The combined type could be here in pool_shared.py, as that's where it would get used.

We only explicitly raise SSLError in network_layer for our socket-based Windows SSL I/O. Those will go away if we do PYTHON-5215.

Ahh i see, makes sense!

blink1073 · 2025-04-23T20:26:56Z

drivers-pr-bot please backport to v4.12

ShaneHarvey

LGTM! Could you schedule the all the mac, windows, PyOpenSSL, OCSP, and encryption tasks to check for regressions before merging?

sleepyStick added 9 commits April 15, 2025 13:42

add is_sync param

efe494d

update contributing

b29d1ba

add vars for pyopenssl and test

d9dfb99

update evergreen config to run this pyopenssl on async as well (not s…

c847f25

…ure if this is correct or not, we'll seeeeee)

fix typing

ae8ecc4

fix tests

03f4ba1

fix test pt2

5349164

edit evergreen config

67100fc

fix test

88ae345

ShaneHarvey requested changes Apr 17, 2025

View reviewed changes

.evergreen/generated_configs/variants.yml Show resolved Hide resolved

sleepyStick and others added 10 commits April 18, 2025 10:27

fix test errors

e451ceb

fix typo...

0312acb

fix typing

4e85024

Merge branch 'master' into PYTHON-5309

dccd96a

maybe this works?

c86a85f

fix typing

12ef993

fix tests

bc76aae

fix typing again

a9c63c8

fix tests pt 2?

67c6738

fix test pt3

3ea4de7

sleepyStick marked this pull request as ready for review April 21, 2025 17:51

sleepyStick requested a review from NoahStapp April 21, 2025 17:51

Merge branch 'master' into PYTHON-5309

38ad677

add test_name

c57aed2

NoahStapp requested changes Apr 22, 2025

View reviewed changes

sleepyStick added 2 commits April 22, 2025 11:26

address review

2591169

update changelog

06a710d

sleepyStick added 5 commits April 22, 2025 11:29

undo whitespace changes

ef4111e

fix failures

0b3c6bb

fix typing

9336f58

fix typing?

23b7cbe

fix error?

760fa97

sleepyStick requested review from NoahStapp and ShaneHarvey April 22, 2025 19:52

ShaneHarvey requested changes Apr 22, 2025

View reviewed changes

sleepyStick added 6 commits April 22, 2025 15:01

undo whitespace changes

4b8a4ed

more whitespace changes

5807ba1

move kms_ssl_contexts

683ba33

fix import

d007c5f

fix test failures

05c061a

fix test failure pt2

350f103

NoahStapp requested changes Apr 23, 2025

View reviewed changes

sleepyStick and others added 9 commits April 23, 2025 09:57

change changelog line ft noah's suggestion

5fa117f

_ssl -> _stdssl and ssl_in_use -> _ssl

56c9662

make combined error type

a7324e5

jk cant do _stdssl

af83d81

_pysslConn back to _sslConn

f6b17dd

fix _ssl and has_sni

74ca8be

fix test

536f189

_ssl -> ssl

24354b4

Merge branch 'master' into PYTHON-5309

4178fcc

ShaneHarvey previously approved these changes Apr 23, 2025

View reviewed changes

Merge branch 'master' into PYTHON-5309

b2324e3

sleepyStick dismissed ShaneHarvey’s stale review via b2324e3 April 23, 2025 20:40

sleepyStick requested a review from NoahStapp April 23, 2025 20:43

PYTHON-5309 Async client no longer works on Atlas mongodb Cloud #2286

Are you sure you want to change the base?

PYTHON-5309 Async client no longer works on Atlas mongodb Cloud #2286

Conversation

sleepyStick commented Apr 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp commented Apr 21, 2025

sleepyStick commented Apr 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey Apr 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blink1073 commented Apr 23, 2025

ShaneHarvey left a comment

Choose a reason for hiding this comment

sleepyStick commented Apr 15, 2025 •

edited

Loading

ShaneHarvey Apr 22, 2025 •

edited

Loading

ShaneHarvey Apr 23, 2025 •

edited

Loading