PYTHON-5309 Ensure AsyncMongoClient doesn't use PyOpenSSL #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.

I'm concerned about people seeing this section and misinterpreting it to mean that they should be using tlsAllowInvalidCertificates. Can we remove it or make it very clear that this is only for local testing (using our test certificates) and not production?

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.

Can you expand on why the union types didn't work? We use Union elsewhere in our type hints.

hmm okay not sure what i did incorrectly last time, but it works now! sorry about that >.<
(I'm going to guess i handled BLOCKING_IO_ERRORS incorrectly cuz i believe that's already a tuple of types and the union of two different types of tuples was not good?)

Wait,, how am i supposed to do it then? I thought Union[x, y] was for type hints??

That error is from trying to use the pipe | operator, right? What happens when you use Union[x, y] instead?

Ah, it works! thanks!

Oh, this is a runtime object, not a type hint. We should be using the same type as ssl.BLOCKING_IO_ERRORS (most likely a tuple or a list)

yeah, it didn't immediately error (as a runtime object) so i thought it was just something I didn't know about how python works HAHA but okay, changing it back to a tuple now xD

test/asynchronous/test_encryption.py

NoahStapp

🥳

ShaneHarvey · 2025-04-24T19:07:29Z

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


I'm concerned about people seeing this section and misinterpreting it to mean that they should be using tlsAllowInvalidCertificates. Can we remove it or make it very clear that this is only for local testing (using our test certificates) and not production?

sleepyStick · 2025-04-24T19:13:47Z

That's fair, I've removed it and added it to our onboarding google doc for now. :)

NoahStapp · 2025-04-24T19:18:20Z

That's fair, I've removed it and added it to our onboarding google doc for now. :)

I would rather keep the section in contributing.md to help any external contributors. Having a warning that this is purely for local testing and should never be done in production seems sufficient to me.

ShaneHarvey · 2025-04-24T19:20:32Z

test/test_encryption.py

@@ -168,7 +169,9 @@ def test_init_spawn_args(self):
    def test_init_kms_tls_options(self):
        # Error cases:
        with self.assertRaisesRegex(TypeError, r'kms_tls_options\["kmip"\] must be a dict'):
-            AutoEncryptionOpts({}, "k.d", kms_tls_options={"kmip": 1})
+            opts = AutoEncryptionOpts({}, "k.d", kms_tls_options={"kmip": 1})
+            _parse_kms_tls_options(opts._kms_tls_options, _IS_SYNC)


These tests should use the real validation path. Something like:

# Error cases: opts = AutoEncryptionOpts({}, "k.d", kms_tls_options={"kmip": 1}) with self.assertRaisesRegex(TypeError, r'kms_tls_options\["kmip"\] must be a dict'): self.mongo_client(auto_encryption_opts=opts)

Alternatively, we can keep the old sync validation behavior in the AutoEncryptionOpts constructor and only re-parse the kms_tls_options in the async _EncryptionIO case.

I'm fine with either option.

ShaneHarvey · 2025-04-24T19:21:34Z

I would rather keep the section in contributing.md to help any external contributors. Having a warning that this is purely for local testing and should never be done in production seems sufficient to me.

That's fine but it's unrelated to this PR so should be done in a different ticket.

ShaneHarvey · 2025-04-24T20:41:34Z

@sleepyStick could you also update the PR title to describe the bug fix?

ShaneHarvey · 2025-04-24T21:13:39Z

test/test_encryption.py

        with self.assertRaisesRegex(TypeError, r'kms_tls_options\["kmip"\] must be a dict'):
-            AutoEncryptionOpts({}, "k.d", kms_tls_options={"kmip": 1})
+            client = self.rs_or_single_client(auto_encryption_opts=opts)
+            client.db.coll.insert_one({"encrypted": "test"})


These tests should raise from the AsyncMongoClient constructor, without even attempting to connect. We could even use AsyncMongoClient() directly:

with self.assertRaisesRegex(TypeError, r'kms_tls_options\["kmip"\] must be a dict'): AsyncMongoClient(auto_encryption_opts=opts)

Could you confirm?

Based on the test failure, the current approach is a little problematic because the validation error happens after we've created an internal MongoClient. When the validation fails, we leave the client open, leading to a resource warning.

Ahhh yeah, i see now. Thanks for popping over to further explain it, and for working thru the soln with me!

ShaneHarvey · 2025-04-24T22:53:25Z

pymongo/asynchronous/encryption.py

@@ -398,6 +398,7 @@ def __init__(self, client: AsyncMongoClient[_DocumentTypeArg], opts: AutoEncrypt
            encrypted_fields_map = _dict_to_bson(opts._encrypted_fields_map, False, _DATA_KEY_OPTS)
        self._bypass_auto_encryption = opts._bypass_auto_encryption
        self._internal_client = None
+        opts._kms_ssl_contexts(_IS_SYNC)


It's worth adding a comment here to explain that we call this here so that parsing errors can be raised before creating internal clients.

makes sense, added :)

ShaneHarvey · 2025-04-24T22:54:25Z

pymongo/encryption_options.py

-        self._kms_ssl_contexts = _parse_kms_tls_options(kms_tls_options)
+        self._kms_tls_options = kms_tls_options
+        self._sync_kms_ssl_contexts: dict[str, SSLContext] = None  # type:ignore[assignment]
+        self._async_kms_ssl_contexts: dict[str, SSLContext] = None  # type:ignore[assignment]


Please remove type:ignore[assignment]. Those are bug factories and should only be used in exceptional circumstances.

mongodb-drivers-pr-bot · 2025-04-24T23:21:30Z

Sorry, unable to cherry-pick to v4.12, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout -b cherry-pick-v4.12-c3e3373df2bf93c616dca0d53c7fbe3f7064d7ba v4.12

git fetch origin c3e3373df2bf93c616dca0d53c7fbe3f7064d7ba

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 c3e3373df2bf93c616dca0d53c7fbe3f7064d7ba

You will likely have some merge/cherry-pick conflicts here, fix them and commit:

git commit -am {message}

Push to a named branch:

git push origin cherry-pick-v4.12-c3e3373df2bf93c616dca0d53c7fbe3f7064d7ba

Create a PR against branch v4.12. I would have named this PR:

"PYTHON-5309 Ensure AsyncMongoClient doesn't use PyOpenSSL (#2286) [v4.12]"

Co-authored-by: Noah Stapp <noah@noahstapp.com>

…2319) Co-authored-by: Iris <58442094+sleepyStick@users.noreply.github.com>

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

NoahStapp previously approved these changes Apr 24, 2025

View reviewed changes

ShaneHarvey requested changes Apr 24, 2025

View reviewed changes

remove added section

4d12c59

sleepyStick dismissed NoahStapp’s stale review via 4d12c59 April 24, 2025 19:11

sleepyStick requested a review from ShaneHarvey April 24, 2025 19:13

ShaneHarvey requested changes Apr 24, 2025

View reviewed changes

modify encryption tests

a0fe2e5

sleepyStick changed the title ~~PYTHON-5309 Async client no longer works on Atlas mongodb Cloud~~ PYTHON-5309 Ensure AsyncMongoClient doesn't use PyOpenSSL Apr 24, 2025

fix test

7b4ae9c

ShaneHarvey reviewed Apr 24, 2025

View reviewed changes

sleepyStick added 5 commits April 24, 2025 14:16

change test

f02a791

close the client

4ed055e

fix the tests pt3

981a046

fix typing

c20623f

fix import

bdaf87a

sleepyStick requested a review from ShaneHarvey April 24, 2025 22:38

ShaneHarvey approved these changes Apr 24, 2025

View reviewed changes

ShaneHarvey requested changes Apr 24, 2025

View reviewed changes

fix typing and add comment in encryption

c2b2cc3

sleepyStick requested a review from ShaneHarvey April 24, 2025 23:09

ShaneHarvey approved these changes Apr 24, 2025

View reviewed changes

sleepyStick merged commit c3e3373 into mongodb:master Apr 24, 2025
31 of 32 checks passed

sleepyStick deleted the PYTHON-5309 branch April 24, 2025 23:20

NoahStapp added a commit to NoahStapp/mongo-python-driver that referenced this pull request Apr 28, 2025

PYTHON-5309 Ensure AsyncMongoClient doesn't use PyOpenSSL (mongodb#2286)

7e9fe16

Co-authored-by: Noah Stapp <noah@noahstapp.com>

NoahStapp added a commit that referenced this pull request Apr 28, 2025

PYTHON-5309: [v4.12] AsyncMongoClient doesn't use PyOpenSSL (#2286) (#…

dd99f80

…2319) Co-authored-by: Iris <58442094+sleepyStick@users.noreply.github.com>

PYTHON-5309 Ensure AsyncMongoClient doesn't use PyOpenSSL #2286

PYTHON-5309 Ensure AsyncMongoClient doesn't use PyOpenSSL #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

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

NoahStapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleepyStick commented Apr 24, 2025

NoahStapp commented Apr 24, 2025

Choose a reason for hiding this comment

ShaneHarvey commented Apr 24, 2025

ShaneHarvey commented Apr 24, 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

mongodb-drivers-pr-bot bot commented Apr 24, 2025

sleepyStick commented Apr 15, 2025 •

edited

Loading