Fix certificate issuer verification on new Twisted. (#7520)

pyproject.toml

@@ -267,6 +267,7 @@ markers = [
     "requires_botocore: marks tests that need botocore (but not boto3)",
     "requires_boto3: marks tests that need botocore and boto3",
     "requires_mitmproxy: marks tests that need mitmproxy",
+    "requires_internet: marks tests that need real Internet access",
 ]
 filterwarnings = [
     "ignore::DeprecationWarning:twisted.web.static",

scrapy/core/downloader/contextfactory.py

@@ -131,7 +131,6 @@ class _ScrapyClientContextFactory(BrowserLikePolicyForHTTPS):
                 return _ScrapyClientTLSOptions26(
                     self._get_cert_options()._makeTLSConnection,
                     hostname.decode("ascii"),
-                    verbose_logging=self.tls_verbose_logging,
                 )
             return _ScrapyClientTLSOptions(
                 hostname.decode("ascii"),  # type: ignore[arg-type]

scrapy/core/downloader/tls.py

@@ -107,34 +107,23 @@ class _ScrapyClientTLSOptions26(ClientTLSOptions):
     logging warnings.
 
     Instances of this class are returned from
-    :class:`.ScrapyClientContextFactory`.
+    :class:`._ScrapyClientContextFactory`.
 
     This class is used on Twisted 26.4.0 and newer.
     """
 
-    def __init__(
-        self,
-        createConnection: Callable[[TLSMemoryBIOProtocol], SSL.Connection],
-        hostname: str,
-        verbose_logging: bool = False,
-    ):
-        super().__init__(createConnection, hostname)
-        self.verbose_logging: bool = verbose_logging
-
     def clientConnectionForTLS(
         self, tlsProtocol: TLSMemoryBIOProtocol
     ) -> SSL.Connection:
         """This method is needed to override the verify callback."""
         conn = super().clientConnectionForTLS(tlsProtocol)
-        callback = self._verifyCB(
-            self._hostnameIsDnsName, self._hostnameASCII, self.verbose_logging
-        )
+        callback = self._verifyCB(self._hostnameIsDnsName, self._hostnameASCII)
         conn.set_verify(SSL.VERIFY_PEER | SSL.VERIFY_FAIL_IF_NO_PEER_CERT, callback)
         return conn
 
     @staticmethod
     def _verifyCB(
-        hostIsDNS: bool, hostnameASCII: str, verbose_logging: bool
+        hostIsDNS: bool, hostnameASCII: str
     ) -> Callable[[SSL.Connection, X509, int, int, int], bool]:
         svcid: ServiceID = (
             DNS_ID(hostnameASCII) if hostIsDNS else IPAddress_ID(hostnameASCII)
@@ -145,7 +134,7 @@ class _ScrapyClientTLSOptions26(ClientTLSOptions):
         ) -> bool:
             if depth != 0:
                 # We are only verifying the leaf certificate.
-                return bool(ok)
+                return True
 
             try:
                 verify_service_identity(extract_patterns(cert), [svcid], [])

tests/test_downloader_handler_httpx.py

@@ -18,6 +18,7 @@ from tests.test_downloader_handlers_http_base import (
     TestHttpsWrongHostnameBase,
     TestHttpWithCrawlerBase,
     TestMitmProxyBase,
+    TestRealWebsiteBase,
     TestSimpleHttpsBase,
 )
 from tests.utils.decorators import coroutine_test
@@ -145,3 +146,8 @@ class TestHttpsProxy(HttpxDownloadHandlerMixin, TestHttpProxyBase):
 @pytest.mark.requires_mitmproxy
 class TestMitmProxy(HttpxDownloadHandlerMixin, TestMitmProxyBase):
     pass
+
+
+@pytest.mark.requires_internet
+class TestRealWebsite(HttpxDownloadHandlerMixin, TestRealWebsiteBase):
+    pass

tests/test_downloader_handler_twisted_http11.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+import sys
 from typing import TYPE_CHECKING, Any
 
 import pytest
@@ -20,6 +21,7 @@ from tests.test_downloader_handlers_http_base import (
     TestHttpsWrongHostnameBase,
     TestHttpWithCrawlerBase,
     TestMitmProxyBase,
+    TestRealWebsiteBase,
     TestSimpleHttpsBase,
 )
 
@@ -103,3 +105,10 @@ class TestHttpsProxy(HTTP11DownloadHandlerMixin, TestHttpProxyBase):
 class TestMitmProxy(HTTP11DownloadHandlerMixin, TestMitmProxyBase):
     # not implemented
     handler_supports_tls_in_tls = False
+
+
+@pytest.mark.requires_internet
+class TestRealWebsite(HTTP11DownloadHandlerMixin, TestRealWebsiteBase):
+    @property
+    def platform_cert_store_works(self) -> bool:
+        return sys.platform != "win32"

tests/test_downloader_handler_twisted_http2.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+import sys
 from typing import TYPE_CHECKING, Any
 
 import pytest
@@ -21,6 +22,7 @@ from tests.test_downloader_handlers_http_base import (
     TestHttpsWrongHostnameBase,
     TestHttpWithCrawlerBase,
     TestMitmProxyBase,
+    TestRealWebsiteBase,
 )
 from tests.utils.decorators import coroutine_test
 
@@ -196,3 +198,10 @@ class TestHttp2Proxy(H2DownloadHandlerMixin, TestHttpProxyBase):
 @pytest.mark.requires_mitmproxy
 class TestMitmProxy(H2DownloadHandlerMixin, TestMitmProxyBase):
     pass
+
+
+@pytest.mark.requires_internet
+class TestRealWebsite(H2DownloadHandlerMixin, TestRealWebsiteBase):
+    @property
+    def platform_cert_store_works(self) -> bool:
+        return sys.platform != "win32"

tests/test_downloader_handlers_http_base.py

@@ -6,7 +6,6 @@ import gzip
 import json
 import logging
 import os
-import platform
 import re
 import sys
 from abc import ABC, abstractmethod
@@ -524,7 +523,7 @@ class TestHttpBase(ABC):
             await download_handler.download_request(request)
         assert "download_latency" in request.meta
         latency = request.meta["download_latency"]
-        if sys.version_info < (3, 13) and platform.system() == "Windows":
+        if sys.version_info < (3, 13) and sys.platform == "win32":
             # time.monotonic() resolution is too low here:
             # https://docs.python.org/3/whatsnew/3.13.html#time
             assert latency >= 0
@@ -1266,3 +1265,69 @@ class TestMitmProxyBase(ABC):
     @staticmethod
     def _assert_got_auth_exception(log: str) -> None:
         assert "Proxy Authentication Required" in log or "407" in log
+
+
+class TestRealWebsiteBase(ABC):
+    @property
+    @abstractmethod
+    def download_handler_cls(self) -> type[DownloadHandlerProtocol]:
+        raise NotImplementedError
+
+    @property
+    @abstractmethod
+    def settings_dict(self) -> dict[str, Any] | None:
+        raise NotImplementedError
+
+    @property
+    @abstractmethod
+    def platform_cert_store_works(self) -> bool:
+        """Whether valid certificates can be verified.
+
+        Twisted on Windows cannot do that out of the box, see e.g.
+        https://github.com/twisted/twisted/issues/6371.
+        """
+        return True
+
+    @asynccontextmanager
+    async def get_dh(
+        self, settings_dict: dict[str, Any] | None = None
+    ) -> AsyncGenerator[DownloadHandlerProtocol]:
+        crawler = get_crawler(DefaultSpider, settings_dict)
+        crawler.spider = crawler._create_spider()
+        dh = build_from_crawler(self.download_handler_cls, crawler)
+        try:
+            yield dh
+        finally:
+            await dh.close()
+
+    @coroutine_test
+    async def test_download(self) -> None:
+        request = Request("https://books.toscrape.com/")
+        async with self.get_dh() as download_handler:
+            response = await download_handler.download_request(request)
+        assert response.status == 200
+        assert "All products | Books to Scrape - Sandbox" in response.text
+
+    @coroutine_test
+    async def test_download_with_spider(self) -> None:
+        crawler = get_crawler(SingleRequestSpider, self.settings_dict)
+        await maybe_deferred_to_future(
+            crawler.crawl(seed=Request("https://books.toscrape.com/"))
+        )
+        assert isinstance(crawler.spider, SingleRequestSpider)
+        failure = crawler.spider.meta.get("failure")
+        assert failure is None
+        reason = crawler.spider.meta["close_reason"]
+        assert reason == "finished"
+
+    @coroutine_test
+    async def test_verify_certs(self) -> None:
+        if not self.platform_cert_store_works:
+            pytest.skip("Cannot verify certificates")
+        request = Request("https://books.toscrape.com/")
+        async with self.get_dh(
+            {"DOWNLOAD_VERIFY_CERTIFICATES": True}
+        ) as download_handler:
+            response = await download_handler.download_request(request)
+        assert response.status == 200
+        assert "All products | Books to Scrape - Sandbox" in response.text