Revert sharing of the SSL context in _ScrapyClientContextFactory. (#7450)

scrapy/core/downloader/contextfactory.py

@@ -78,13 +78,6 @@ class _ScrapyClientContextFactory(BrowserLikePolicyForHTTPS):
             self.tls_ciphers = AcceptableCiphers.fromOpenSSLCipherString(tls_ciphers)
         else:
             self.tls_ciphers = DEFAULT_CIPHERS
-        with _filter_method_warning():
-            self._certificate_options = CertificateOptions(
-                method=self._ssl_method,
-                fixBrokenPeers=True,
-                acceptableCiphers=self.tls_ciphers,
-            )
-        self._ctx = self._get_context()
         self._verify_certificates = verify_certificates
 
     @classmethod
@@ -109,23 +102,38 @@ class _ScrapyClientContextFactory(BrowserLikePolicyForHTTPS):
             **kwargs,
         )
 
+    # should be removed together with ScrapyClientContextFactory
     def getCertificateOptions(self) -> CertificateOptions:  # pragma: no cover
-        return self._certificate_options
+        return self._get_cert_options()
+
+    def _get_cert_options(self) -> CertificateOptions:
+        with _filter_method_warning():
+            return CertificateOptions(
+                method=self._ssl_method,
+                fixBrokenPeers=True,
+                acceptableCiphers=self.tls_ciphers,
+            )
 
     # kept for old-style HTTP/1.0 downloader context twisted calls,
     # e.g. connectSSL()
+    # should be removed together with ScrapyClientContextFactory
     def getContext(self, hostname: Any = None, port: Any = None) -> SSL.Context:
-        return self._ctx
+        return self._get_context()
 
     def _get_context(self) -> SSL.Context:
-        ctx = self._certificate_options.getContext()
+        cert_options = self._get_cert_options()
+        ctx = cert_options.getContext()
         ctx.set_options(0x4)  # OP_LEGACY_SERVER_CONNECT
         return ctx
 
     def creatorForNetloc(self, hostname: bytes, port: int) -> ClientTLSOptions:
         if not self._verify_certificates:
-            return _ScrapyClientTLSOptions(hostname.decode("ascii"), self._ctx)  # type: ignore[no-untyped-call]
-        # Note that this doesn't use self._ctx
+            # _ScrapyClientTLSOptions is needed to skip verification errors
+            return _ScrapyClientTLSOptions(
+                hostname.decode("ascii"), self._get_context()
+            )  # type: ignore[no-untyped-call]
+        # Otherwise use the normal Twisted function.
+        # Note that this doesn't use self._get_context().
         with _filter_method_warning():
             return optionsForClientTLS(
                 hostname=hostname.decode("ascii"),

scrapy/utils/_deps_compat.py

@@ -6,5 +6,7 @@ from twisted.python.versions import Version as TxVersion
 TWISTED_FAILURE_HAS_STACK = TWISTED_VERSION < TxVersion("twisted", 24, 10, 0)
 
 PYOPENSSL_VERSION = Version(PYOPENSSL_VERSION_STRING)
-# SSL.Context.use_certificate wants an X509 object, SSL.Context.use_privatekey wants a PKey object
+# SSL.Context.use_certificate() wants an X509 object, SSL.Context.use_privatekey() wants a PKey object
 PYOPENSSL_WANTS_X509_PKEY = PYOPENSSL_VERSION < Version("24.3.0")
+# SSL.Context.set_cipher_list() creates a temporary connection, making the context immutable
+PYOPENSSL_SET_CIPHER_LIST_TMP_CONN = PYOPENSSL_VERSION < Version("25.2.0")

tests/test_core_downloader.py

@@ -17,6 +17,7 @@ from scrapy.core.downloader.contextfactory import (
 )
 from scrapy.core.downloader.handlers.http11 import _RequestBodyProducer
 from scrapy.exceptions import ScrapyDeprecationWarning
+from scrapy.utils._deps_compat import PYOPENSSL_SET_CIPHER_LIST_TMP_CONN
 from scrapy.utils.defer import maybe_deferred_to_future
 from scrapy.utils.misc import build_from_crawler
 from scrapy.utils.python import to_bytes
@@ -98,7 +99,7 @@ class TestContextFactoryBase:
 
 class TestContextFactory(TestContextFactoryBase):
     @coroutine_test
-    async def testPayload(self, server_url: str) -> None:
+    async def test_payload(self, server_url: str) -> None:
         s = "0123456789" * 10
         crawler = get_crawler()
         client_context_factory = _load_context_factory_from_settings(crawler)
@@ -107,6 +108,41 @@ class TestContextFactory(TestContextFactoryBase):
         )
         assert body == to_bytes(s)
 
+    def test_no_context_sharing(self) -> None:
+        """Every call to creatorForNetloc() should give a fresh context."""
+        crawler = get_crawler()
+        client_context_factory: _ScrapyClientContextFactory = (
+            _load_context_factory_from_settings(crawler)
+        )
+        creator1 = client_context_factory.creatorForNetloc(b"website1.tld", 443)
+        assert creator1._hostnameBytes == b"website1.tld"
+        creator2 = client_context_factory.creatorForNetloc(b"website2.tld", 443)
+        assert creator2._hostnameBytes == b"website2.tld"
+        assert creator1._ctx is not creator2._ctx
+
+    @pytest.mark.skipif(
+        PYOPENSSL_SET_CIPHER_LIST_TMP_CONN,
+        reason="Fails or doesn't make sense on this pyOpenSSL version",
+    )
+    def test_no_immutable_ctx_warning(self) -> None:
+        """There should be no pyOpenSSL context modification warning.
+
+        pyOpenSSL < 25.1.0 doesn't produce this warning, and on 25.1.0 it's
+        always produced due to
+        https://github.com/scrapy/scrapy/issues/6859#issuecomment-4294917851.
+        """
+        crawler = get_crawler()
+        client_context_factory: _ScrapyClientContextFactory = (
+            _load_context_factory_from_settings(crawler)
+        )
+        with warnings.catch_warnings():
+            warnings.filterwarnings(
+                "error",
+                category=DeprecationWarning,
+                message="Attempting to mutate a Context after a Connection was created",
+            )
+            client_context_factory.creatorForNetloc(b"website.tld", 443)
+
 
 class TestContextFactoryTLSMethod(TestContextFactoryBase):
     async def _assert_factory_works(