Creating `SSLContext` at import time makes mocking "impossible"

[mindflayer]

Is your feature request related to a problem?

Hi there, I am the author of Mocket.

Mocket is probably the only tool around able to mock non-blocking sockets, and it's also listed under https://docs.aiohttp.org/en/stable/third_party.html.

With aiohttp==3.10.6 you moved the creation of SSLContext at import time. This makes mocking HTTPS calls impossible if applying an agnostic approach which works for every client (mocking socket and ssl). To fix that on my side I'd have to change Mocket for mocking aiohttp internals: _SSL_CONTEXT_VERIFIED and _SSL_CONTEXT_UNVERIFIED.

Describe the solution you'd like

Just an hint to understand my point. I leave to you folks the decision about the possible change.

Change _make_ssl_context (in connector.py) to make it fail (e.g. add 1 / 0 as its first line).

>>> import aiohttp
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/tmp/aiohttp/__init__.py", line 6, in <module>
    from .client import (
  File "/tmp/aiohttp/client.py", line 85, in <module>
    from .connector import (
  File "/tmp/aiohttp/connector.py", line 757, in <module>
    _SSL_CONTEXT_VERIFIED = _make_ssl_context(True)
                            ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/employee/repos/python-mocket/aiohttp/connector.py", line 737, in _make_ssl_context
    1 / 0
    ~~^~~
ZeroDivisionError: division by zero

Describe alternatives you've considered

I don't like the idea of stopping supporting aiohttp, but I don't want Mocket to adapt to clients' internals, because it's exactly the opposite approach (mocking socket/ssl VS mocking clients).

Related component

Client

Additional context

No response

Code of Conduct

• [X] I agree to follow the aio-libs Code of Conduct

[Dreamsorcerer]

I'm not overly clear what the issue is. Are you saying you don't mock aiohttp, but mock the ssl calls? So, in this case you're trying to mock the creation of the SSLContext?

In that case, I'm not sure this worked correctly on previous releases, as the previous function used a cache decorator. Therefore, I'm assuming if you created a ClientSession before mocket, then even if you created another ClientSession with mocket it would still get the previous (unmocked) context. Likewise, if you created one with mocket first, then created one outside of the mock scope, the second one would still have the mocked context.

As the context will always be the same, and it's a blocking, expensive call, we wouldn't want to recreate the context every time.

[mindflayer]

Why not simply making them into @property? Something like:

class TCPConnector(BaseConnector):
    _SSL_CONTEXT_VERIFIED = None
    ...

    @property
    def ssl_context_verified(self):
        if not self._SSL_CONTEXT_VERIFIED:
            self._SSL_CONTEXT_VERIFIED = ...
        return self._SSL_CONTEXT_VERIFIED
...

In this case you only create them if you actually need to do it, making importing the client faster as a bonus.

[mindflayer]

I'm not overly clear what the issue is. Are you saying you don't mock aiohttp, but mock the ssl calls? So, in this case you're trying to mock the creation of the SSLContext?

Correct, I both mock socket and ssl modules and that's enough for mocking "any" client.

[Dreamsorcerer]

That wouldn't be cached, it'd be per connector. Again, the previous implementation used a cached function to create them. We moved them to import time to avoid blocking calls at runtime, but the more important thing here is that the contexts are reused across sessions/connectors. Homeassistant can use a lot of sessions, so that's one particular area helping to shape these decisions. @bdraco can provide some additional background.

If we simply revert the change, then I think the behaviour of your library would still be incorrect..

[mindflayer]

If we simply revert the change, then I think the behaviour of your library would still be incorrect..

Before v .6 everything worked perfectly. Mocket is currently used for mocking aiohttp calls.

[Dreamsorcerer]

But, did you actually test the scenarios I described?

Because if they work, then I don't understand how...

[mindflayer]

Mocket is normally used in situations like:

import aiohttp
import pytest
from mocket import mocketize

@mocketize
def test_a():
    ...

@mocketize
def test_b():
    ...

That's the use-case I am trying to fix, and it worked before. To make those tests work after the change, I have to move import aiohttp under the tests using it.

[Dreamsorcerer]

Yes, I understand that, but each of those decorators is meant to be a context, right?

So, doesn't it break (on previous releases) if we do this?

def test_a():
    """Should not be mocked."""
    async with ClientSession() as sess:
        ....

@mocketize
def test_b():
    """Should be mocked."""
    async with ClientSession() as sess:
        ...

My expectation is that both of those tests will use the same context, the mocked one if test_b runs first or the real one if test_a runs first.

[mindflayer]

Even when in control, Mocket lets the real traffic pass, so nothing breaks. In your non-decorated test, you'd have real socket/context instances doing their normal job.

[Dreamsorcerer]

OK, so not a real problem if test_b runs first. What if test_a runs first, won't the mocked test fail?

[mindflayer]

Let me write down a real example mimicking what you have in mind and come back to you.

[mindflayer]

You were right, and now that I read again the previous implementation I understand why. It was simply something that we never noticed. So, previously that use-case was broken (normally during tests you don't want to hit the network), v .6 broke any attempt to mock calls.

[bdraco]

We have the following high level requirements with creating the SSLContexts (probably a few more but these are the top ones):

1. We need to avoid all blocking I/O in the event loop to load ssl certificates from disk.
2. We need to support multiple different event loops: https://github.com/aio-libs/aiohttp/issues/9202
3. We need to create the SSLContexts only once
4. We should not incur additional overhead to check if the SSLContext needs to be created

Currently, the only place I'm aware of where we don't have to worry about blocking I/O or if multiple event loops are running is at import time. If you have a better solution that meets the above high level requirements, we would certainly be open to implementing it or accepting a PR.

[Dreamsorcerer]

I think if you wanted to mock out the method in aiohttp, in order to keep this working in your library, then we could consider a regression test here to make sure we don't break it by accident. But, not sure there's a good way to make this work otherwise.

[mindflayer]

As a monkey-patch, people could pass a fake SSLContext when performing HTTP calls. Something like:

    async with aiohttp.ClientSession(
        timeout=aiohttp.ClientTimeout(total=3)
    ) as session, session.get(url, ssl=FakeSSLContext()) as response:
        response = await response.json()
        assert response == data

To make it work, I had to apply a couple patches to aiohttp code:

diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py
index 602d6da5..9d247935 100644
--- a/aiohttp/client_reqrep.py
+++ b/aiohttp/client_reqrep.py
@@ -147,7 +147,8 @@ class Fingerprint:
 
 
 if ssl is not None:
-    SSL_ALLOWED_TYPES = (ssl.SSLContext, bool, Fingerprint)
+    from mocket import FakeSSLContext
+    SSL_ALLOWED_TYPES = (ssl.SSLContext, bool, Fingerprint, FakeSSLContext)
 else:  # pragma: no cover
     SSL_ALLOWED_TYPES = (bool,)
 
diff --git a/aiohttp/connector.py b/aiohttp/connector.py
index e00ad196..61614cb4 100644
--- a/aiohttp/connector.py
+++ b/aiohttp/connector.py
@@ -1034,7 +1034,7 @@ class TCPConnector(BaseConnector):
         if ssl is None:  # pragma: no cover
             raise RuntimeError("SSL is not supported.")
         sslcontext = req.ssl
-        if isinstance(sslcontext, ssl.SSLContext):
+        if hasattr(sslcontext, "wrap_socket"):
             return sslcontext
         if sslcontext is not True:
             # not verified or fingerprinted

For the first change, it'd be great if there was a way to inject new types (or maybe just a mix of types and duck-typing?), while the second is probably an harmless change (duck-typing vs class-based check).

[mindflayer]

An alternative I don't particularly like - client logic inside Mocket - would be mocking TCPConnector._get_ssl_context, but the function would have to be at module level to make it easier. The previous approach would imply devs need to know what they are doing and make their code behave differently when testing - adding the fake context to every HTTPS request -, while the second one would be totally transparent to them, at the cost of mocking a bit of aiohttp internals.

[mindflayer]

In the end I opted for the third approach, implementing a Mocket TcpConnector to be used with ClientSession when running tests.

class MocketTCPConnector(TCPConnector):
    """
    `aiohttp` reuses SSLContext instances created at import-time,
    making it more difficult for Mocket to do its job.
    This is an attempt to make things smoother, at the cost of
    slightly patching the `ClientSession` while testing.
    """
    def _get_ssl_context(self, req: ClientRequest) -> FakeSSLContext:
        return FakeSSLContext()

[mindflayer]

Mocket 3.13.2 introduces the above connector. Here is a working test showing there is no need to import aiohttp when Mocket is in control: https://github.com/mindflayer/python-mocket/blob/main/tests/test_asyncio.py#L49

Thanks for helping.

[Dreamsorcerer]

Seems like a decent solution. Feel free to open a PR with a regression test, so we don't break it by accident (e.g. removing/renaming that method during a refactor).

[cjavad]

A bit of a side track, but still directly related to the static initialization of the SSL contexts so i didn't want to open an entirely new issue here, just to add some more context on the effect of the change.

This change also broke a lot of code that depended on changing environment variables such as SSL_CERT_FILE to certifi.where() at runtime (typically in __main__) that affects the parsing of the openssl get and set _default_verify_paths.

The changelog only reflected a bug fix so this side effect went unnoticed into production, but it was easily fixed by making those changes at import time as well.

But i fully respect and understand the necessity of the change but it also felt important to note this perhaps not so niche use-case for future references.

Edit

Actually looking it up it seems to be quite a common pattern, see https://grep.app/search?q=%5B%22SSL_CERT_FILE%22%5D%20%3D&filter[lang][0]=Python for some examples.