-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-5309 Ensure AsyncMongoClient doesn't use PyOpenSSL #2286
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
Changes from 9 commits
efe494d
b29d1ba
d9dfb99
c847f25
ae8ecc4
03f4ba1
5349164
67100fc
88ae345
e451ceb
0312acb
4e85024
dccd96a
c86a85f
12ef993
bc76aae
a9c63c8
67c6738
3ea4de7
38ad677
c57aed2
2591169
06a710d
ef4111e
0b3c6bb
9336f58
23b7cbe
760fa97
4b8a4ed
5807ba1
683ba33
d007c5f
05c061a
350f103
5fa117f
56c9662
a7324e5
af83d81
f6b17dd
74ca8be
536f189
24354b4
4178fcc
b2324e3
17cf61d
257f8fe
74f98c5
6752a67
750a9aa
e7e36b4
8af8f09
16d3cc3
6971fed
4d12c59
a0fe2e5
7b4ae9c
f02a791
4ed055e
981a046
c20623f
bdaf87a
c2b2cc3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the motivation for adding this section? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh uh, I was struggling to get it working and asked Noah a bunch of questions, so he suggested I add a section here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
Note that `AsyncMongoClient` does not support PyOpenSSL. | ||
Assuming all required packages are installed, set the `tls` and `tlsAllowInvalidCertificates` flags in the URI to enable | ||
the driver to connect with SSL, like so: | ||
```python | ||
from pymongo import MongoClient | ||
|
||
client = MongoClient( | ||
"mongodb://localhost:27017?tls=true&tlsAllowInvalidCertificates=true" | ||
) | ||
``` | ||
Another way of doing this would be to pass these options in as parameters to the MongoClient, like so: | ||
```python | ||
client = MongoClient( | ||
"mongodb://localhost:27017", tls=True, tlsAllowInvalidCertificates=True | ||
) | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a public api so we can't add "is_sync" here. What's the motivation for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I added There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, we still can not add "is_sync" here so we need to find another way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One way would be to lazily init the async SSLContext in kms_request(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
) -> None: | ||
"""Options to configure automatic client-side field level encryption. | ||
|
||
|
@@ -236,7 +237,7 @@ def __init__( | |
if not any("idleShutdownTimeoutSecs" in s for s in self._mongocryptd_spawn_args): | ||
self._mongocryptd_spawn_args.append("--idleShutdownTimeoutSecs=60") | ||
# Maps KMS provider name to a SSLContext. | ||
self._kms_ssl_contexts = _parse_kms_tls_options(kms_tls_options) | ||
self._kms_ssl_contexts = _parse_kms_tls_options(kms_tls_options, is_sync) | ||
self._bypass_query_analysis = bypass_query_analysis | ||
self._key_expiration_ms = key_expiration_ms | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,7 @@ | |
) | ||
from pymongo.network_layer import AsyncNetworkingInterface, NetworkingInterface, PyMongoProtocol | ||
from pymongo.pool_options import PoolOptions | ||
from pymongo.ssl_support import HAS_SNI, SSLError | ||
from pymongo.ssl_support import HAS_SNI, PYSSLError, SSLError | ||
|
||
if TYPE_CHECKING: | ||
from pymongo.pyopenssl_context import _sslConn | ||
|
@@ -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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do these need to be separate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Err, I tried to do something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The combined type could be here in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only explicitly raise There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh i see, makes sense! |
||
# Eventlet does not distinguish TLS network timeouts from other | ||
# SSLErrors (https://github.com/eventlet/eventlet/issues/692). | ||
# Luckily, we can work around this limitation because the phrase | ||
|
@@ -293,7 +293,7 @@ async def _async_configured_socket( | |
# Raise _CertificateError directly like we do after match_hostname | ||
# below. | ||
raise | ||
except (OSError, SSLError) as exc: | ||
except (OSError, SSLError, PYSSLError) as exc: | ||
sock.close() | ||
# We raise AutoReconnect for transient and permanent SSL handshake | ||
# failures alike. Permanent handshake failures, like protocol | ||
|
@@ -349,7 +349,7 @@ async def _configured_protocol_interface( | |
# Raise _CertificateError directly like we do after match_hostname | ||
# below. | ||
raise | ||
except (OSError, SSLError) as exc: | ||
except (OSError, SSLError, PYSSLError) as exc: | ||
# We raise AutoReconnect for transient and permanent SSL handshake | ||
# failures alike. Permanent handshake failures, like protocol | ||
# mismatch, will be turned into ServerSelectionTimeoutErrors later. | ||
|
@@ -467,7 +467,7 @@ def _configured_socket(address: _Address, options: PoolOptions) -> Union[socket. | |
# Raise _CertificateError directly like we do after match_hostname | ||
# below. | ||
raise | ||
except (OSError, SSLError) as exc: | ||
except (OSError, SSLError, PYSSLError) as exc: | ||
sock.close() | ||
# We raise AutoReconnect for transient and permanent SSL handshake | ||
# failures alike. Permanent handshake failures, like protocol | ||
|
@@ -516,7 +516,7 @@ def _configured_socket_interface(address: _Address, options: PoolOptions) -> Net | |
# Raise _CertificateError directly like we do after match_hostname | ||
# below. | ||
raise | ||
except (OSError, SSLError) as exc: | ||
except (OSError, SSLError, PYSSLError) as exc: | ||
sock.close() | ||
# We raise AutoReconnect for transient and permanent SSL handshake | ||
# failures alike. Permanent handshake failures, like protocol | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,16 +15,19 @@ | |
"""Support for SSL in PyMongo.""" | ||
from __future__ import annotations | ||
|
||
import types | ||
import warnings | ||
from typing import Optional | ||
from typing import Any, Optional, Union | ||
|
||
from pymongo.errors import ConfigurationError | ||
|
||
HAVE_SSL = True | ||
HAVE_PYSSL = True | ||
|
||
try: | ||
import pymongo.pyopenssl_context as _ssl | ||
import pymongo.pyopenssl_context as _pyssl | ||
except (ImportError, AttributeError) as exc: | ||
HAVE_PYSSL = False | ||
if isinstance(exc, AttributeError): | ||
warnings.warn( | ||
"Failed to use the installed version of PyOpenSSL. " | ||
|
@@ -35,10 +38,10 @@ | |
UserWarning, | ||
stacklevel=2, | ||
) | ||
try: | ||
import pymongo.ssl_context as _ssl # type: ignore[no-redef] | ||
except ImportError: | ||
HAVE_SSL = False | ||
try: | ||
import pymongo.ssl_context as _ssl | ||
except ImportError: | ||
HAVE_SSL = False | ||
|
||
|
||
if HAVE_SSL: | ||
|
@@ -57,6 +60,20 @@ | |
BLOCKING_IO_WRITE_ERROR = _ssl.BLOCKING_IO_WRITE_ERROR | ||
BLOCKING_IO_LOOKUP_ERROR = BLOCKING_IO_READ_ERROR | ||
|
||
if HAVE_PYSSL: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use
Are there any situations where we specifically care if an exception type is from PyOpenSSL or stdlib SSL? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooo good call out. I don't think we particularly care if its pyopenssl vs stdlib ssl error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think python3.9 didn't love the union types? so i just made them tuples. But its a similar idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you expand on why the union types didn't work? We use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm okay not sure what i did incorrectly last time, but it works now! sorry about that >.< There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait,, how am i supposed to do it then? I thought There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That error is from trying to use the pipe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, it works! thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, this is a runtime object, not a type hint. We should be using the same type as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
PYSSLError: Any = _pyssl.SSLError | ||
PYBLOCKING_IO_ERRORS: Any = _pyssl.BLOCKING_IO_ERRORS | ||
PYBLOCKING_IO_READ_ERROR: Any = _pyssl.BLOCKING_IO_READ_ERROR | ||
PYBLOCKING_IO_WRITE_ERROR: Any = _pyssl.BLOCKING_IO_WRITE_ERROR | ||
PYBLOCKING_IO_LOOKUP_ERROR: Any = BLOCKING_IO_READ_ERROR | ||
else: | ||
# just make them the same as SSL so imports won't error | ||
PYSSLError = _ssl.SSLError | ||
PYBLOCKING_IO_ERRORS = _ssl.BLOCKING_IO_ERRORS | ||
PYBLOCKING_IO_READ_ERROR = _ssl.BLOCKING_IO_READ_ERROR | ||
PYBLOCKING_IO_WRITE_ERROR = _ssl.BLOCKING_IO_WRITE_ERROR | ||
PYBLOCKING_IO_LOOKUP_ERROR = BLOCKING_IO_READ_ERROR | ||
|
||
def get_ssl_context( | ||
certfile: Optional[str], | ||
passphrase: Optional[str], | ||
|
@@ -65,10 +82,15 @@ def get_ssl_context( | |
allow_invalid_certificates: bool, | ||
allow_invalid_hostnames: bool, | ||
disable_ocsp_endpoint_check: bool, | ||
) -> _ssl.SSLContext: | ||
is_sync: bool, | ||
) -> Union[_pyssl.SSLContext, _ssl.SSLContext]: # type: ignore[name-defined] | ||
"""Create and return an SSLContext object.""" | ||
if is_sync and HAVE_PYSSL: | ||
ssl_in_use: types.ModuleType = _pyssl | ||
else: | ||
ssl_in_use = _ssl | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we rename There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Er, am i doing it incorrectly? I'm seeing the error "UnboundLocalError: local variable '_ssl' referenced before assignment" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually kinda wanted to rename the global There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You know what, I think it's simpler to go back to the the local There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All good, went with |
||
verify_mode = CERT_NONE if allow_invalid_certificates else CERT_REQUIRED | ||
ctx = _ssl.SSLContext(_ssl.PROTOCOL_SSLv23) | ||
ctx = ssl_in_use.SSLContext(ssl_in_use.PROTOCOL_SSLv23) | ||
if verify_mode != CERT_NONE: | ||
ctx.check_hostname = not allow_invalid_hostnames | ||
else: | ||
|
@@ -80,22 +102,20 @@ def get_ssl_context( | |
# up to date versions of MongoDB 2.4 and above already disable | ||
# SSLv2 and SSLv3, python disables SSLv2 by default in >= 2.7.7 | ||
# and >= 3.3.4 and SSLv3 in >= 3.4.3. | ||
ctx.options |= _ssl.OP_NO_SSLv2 | ||
ctx.options |= _ssl.OP_NO_SSLv3 | ||
ctx.options |= _ssl.OP_NO_COMPRESSION | ||
ctx.options |= _ssl.OP_NO_RENEGOTIATION | ||
ctx.options |= ssl_in_use.OP_NO_SSLv2 | ||
ctx.options |= ssl_in_use.OP_NO_SSLv3 | ||
ctx.options |= ssl_in_use.OP_NO_COMPRESSION | ||
ctx.options |= ssl_in_use.OP_NO_RENEGOTIATION | ||
if certfile is not None: | ||
try: | ||
ctx.load_cert_chain(certfile, None, passphrase) | ||
except _ssl.SSLError as exc: | ||
except ssl_in_use.SSLError as exc: | ||
raise ConfigurationError(f"Private key doesn't match certificate: {exc}") from None | ||
if crlfile is not None: | ||
if _ssl.IS_PYOPENSSL: | ||
if ssl_in_use.IS_PYOPENSSL: | ||
raise ConfigurationError("tlsCRLFile cannot be used with PyOpenSSL") | ||
# Match the server's behavior. | ||
ctx.verify_flags = getattr( # type:ignore[attr-defined] | ||
_ssl, "VERIFY_CRL_CHECK_LEAF", 0 | ||
) | ||
ctx.verify_flags = getattr(ssl_in_use, "VERIFY_CRL_CHECK_LEAF", 0) | ||
ctx.load_verify_locations(crlfile) | ||
if ca_certs is not None: | ||
ctx.load_verify_locations(ca_certs) | ||
|
Uh oh!
There was an error while loading. Please reload this page.