Skip to content

Commit b644729

Browse files
dhoeppillia-v
andauthored
Fix proxy_ssl_context ignored for HTTPS forwarding proxies (#3786)
Co-authored-by: Illia Volochii <illia.volochii@gmail.com>
1 parent e8a81de commit b644729

4 files changed

Lines changed: 165 additions & 17 deletions

File tree

‎changelog/2577.bugfix.rst‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fixed usage of ``proxy_ssl_context`` with ``ProxyManager`` when ``use_forwarding_for_https=True``.
2+
Passing ``ssl_context`` instead of ``proxy_ssl_context`` for HTTPS proxies in this configuration now emits a ``FutureWarning`` and will raise an error in v3.0.

‎src/urllib3/connection.py‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -793,6 +793,20 @@ def connect(self) -> None:
793793
# Remove trailing '.' from fqdn hostnames to allow certificate validation
794794
server_hostname_rm_dot = server_hostname.rstrip(".")
795795

796+
# Forwarding proxies should use proxy SSL context for
797+
# wrapping since that's the connection being established,
798+
# whereas tunneling proxies should use the connection's SSL
799+
# context.
800+
# However, for backwards compatibility reasons, if the proxy
801+
# is forwarding but no proxy SSL context is provided, we
802+
# fall back to using the connection's SSL context until
803+
# urllib3 v3.0. Appropriate warning is emitted in
804+
# ``ProxyManager.__init__``.
805+
if self.proxy_is_forwarding and self.proxy_config is not None:
806+
ssl_context = self.proxy_config.ssl_context
807+
else:
808+
ssl_context = self.ssl_context
809+
796810
sock_and_verified = _ssl_wrap_socket_and_match_hostname(
797811
sock=sock,
798812
cert_reqs=self.cert_reqs,
@@ -806,7 +820,7 @@ def connect(self) -> None:
806820
key_file=self.key_file,
807821
key_password=self.key_password,
808822
server_hostname=server_hostname_rm_dot,
809-
ssl_context=self.ssl_context,
823+
ssl_context=ssl_context,
810824
tls_in_tls=tls_in_tls,
811825
assert_hostname=self.assert_hostname,
812826
assert_fingerprint=self.assert_fingerprint,

‎src/urllib3/poolmanager.py‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -581,6 +581,21 @@ def __init__(
581581
if proxy.scheme not in ("http", "https"):
582582
raise ProxySchemeUnknown(proxy.scheme)
583583

584+
if (
585+
use_forwarding_for_https
586+
and proxy.scheme == "https"
587+
and connection_pool_kw.get("ssl_context") is not None
588+
):
589+
warnings.warn(
590+
"Passing ssl_context when use_forwarding_for_https=True is deprecated "
591+
"and will raise an error in urllib3 v3.0. "
592+
"Use proxy_ssl_context to configure the TLS connection to the proxy.",
593+
FutureWarning,
594+
stacklevel=2,
595+
)
596+
if proxy_ssl_context is None:
597+
proxy_ssl_context = connection_pool_kw.get("ssl_context")
598+
584599
if not proxy.port:
585600
port = port_by_scheme.get(proxy.scheme, 80)
586601
proxy = proxy._replace(port=port)

‎test/with_dummyserver/test_proxy_poolmanager.py‎

Lines changed: 133 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import socket
1111
import ssl
1212
import tempfile
13+
import typing
1314
from test import LONG_TIMEOUT, SHORT_TIMEOUT, resolvesLocalhostFQDN, withPyOpenSSL
1415
from test.conftest import ServerConfig
1516

@@ -24,7 +25,7 @@
2425
)
2526
from urllib3 import HTTPResponse
2627
from urllib3._collections import HTTPHeaderDict
27-
from urllib3.connection import VerifiedHTTPSConnection
28+
from urllib3.connection import HTTPSConnection, VerifiedHTTPSConnection
2829
from urllib3.connectionpool import connection_from_url
2930
from urllib3.exceptions import (
3031
ConnectTimeoutError,
@@ -166,6 +167,43 @@ def test_https_proxy_forwarding_for_https(self) -> None:
166167
r = https.request("GET", f"{self.https_url}/")
167168
assert r.status == 200
168169

170+
@requires_network()
171+
@pytest.mark.parametrize(
172+
"proxy_ssl_context_kw",
173+
[
174+
("ssl_context",),
175+
("proxy_ssl_context",),
176+
("ssl_context", "proxy_ssl_context"),
177+
],
178+
)
179+
def test_https_proxy_forwarding_for_https_with_custom_context(
180+
self, proxy_ssl_context_kw: tuple[str, ...]
181+
) -> None:
182+
"""
183+
Test that an HTTP request succeeds when using a forwarding HTTPS
184+
proxy with an SSL context provided via either
185+
``proxy_ssl_context``, ``ssl_context`` (fallback), or both.
186+
"""
187+
188+
proxy_ctx = ssl.create_default_context()
189+
proxy_ctx.load_verify_locations(DEFAULT_CA)
190+
191+
warning_checker: contextlib.AbstractContextManager[typing.Any]
192+
if "ssl_context" in proxy_ssl_context_kw:
193+
warning_checker = pytest.warns(FutureWarning, match="ssl_context")
194+
else:
195+
warning_checker = contextlib.nullcontext()
196+
with warning_checker:
197+
proxy = proxy_from_url(
198+
self.https_proxy_url,
199+
**{kw: proxy_ctx for kw in proxy_ssl_context_kw},
200+
use_forwarding_for_https=True,
201+
)
202+
203+
with proxy:
204+
resp = proxy.request("GET", self.https_url)
205+
assert resp.status == 200
206+
169207
def test_nagle_proxy(self) -> None:
170208
"""Test that proxy connections do not have TCP_NODELAY turned on"""
171209
with ProxyManager(self.proxy_url) as http:
@@ -694,20 +732,8 @@ def test_https_proxy_tls_error(
694732
assert isinstance(e.value.reason.original_error, SSLError)
695733

696734
@requires_network()
697-
@pytest.mark.parametrize(
698-
["proxy_scheme", "use_forwarding_for_https"],
699-
[
700-
("http", False),
701-
("https", False),
702-
("https", True),
703-
],
704-
)
705-
def test_proxy_https_target_tls_error(
706-
self, proxy_scheme: str, use_forwarding_for_https: str
707-
) -> None:
708-
if proxy_scheme == "https" and use_forwarding_for_https:
709-
pytest.skip("Test is expected to fail due to urllib3/urllib3#2577")
710-
735+
@pytest.mark.parametrize("proxy_scheme", ["http", "https"])
736+
def test_proxy_https_target_tls_error(self, proxy_scheme: str) -> None:
711737
proxy_url = self.https_proxy_url if proxy_scheme == "https" else self.proxy_url
712738
proxy_ctx = ssl.create_default_context()
713739
proxy_ctx.load_verify_locations(DEFAULT_CA)
@@ -717,12 +743,103 @@ def test_proxy_https_target_tls_error(
717743
proxy_url,
718744
proxy_ssl_context=proxy_ctx,
719745
ssl_context=ctx,
720-
use_forwarding_for_https=use_forwarding_for_https,
721746
) as proxy:
722747
with pytest.raises(MaxRetryError) as e:
723748
proxy.request("GET", self.https_url)
724749
assert isinstance(e.value.reason, SSLError)
725750

751+
@requires_network()
752+
def test_forwarding_proxy_tls_error_with_fallback(self) -> None:
753+
"""
754+
Test that when only ``ssl_context`` is passed to a forwarding
755+
HTTPS proxy, a ``FutureWarning`` is emitted and if the TLS
756+
handshake with the proxy fails using that context, the error is
757+
a ``ProxyError`` wrapping the original TLS error.
758+
"""
759+
ctx = ssl.create_default_context()
760+
761+
with pytest.warns(FutureWarning, match="ssl_context"):
762+
proxy = proxy_from_url(
763+
self.https_proxy_url,
764+
ssl_context=ctx,
765+
use_forwarding_for_https=True,
766+
)
767+
768+
with proxy:
769+
with pytest.raises(MaxRetryError) as e:
770+
proxy.request("GET", self.https_url)
771+
assert isinstance(e.value.reason, ProxyError)
772+
assert isinstance(e.value.reason.original_error, SSLError)
773+
774+
@requires_network()
775+
def test_forwarding_proxy_ssl_context_precedence(self) -> None:
776+
"""
777+
Test that when both ``ssl_context`` and ``proxy_ssl_context``
778+
are passed to a forwarding HTTPS proxy, ``proxy_ssl_context`` is
779+
used for the proxy connection and ``ssl_context`` is ignored
780+
(with a warning).
781+
"""
782+
ssl_context = ssl.create_default_context(cafile=DEFAULT_CA)
783+
proxy_ssl_context = ssl.create_default_context(cafile=DEFAULT_CA)
784+
785+
with pytest.warns(FutureWarning, match="ssl_context"):
786+
proxy = proxy_from_url(
787+
self.https_proxy_url,
788+
proxy_ssl_context=proxy_ssl_context,
789+
ssl_context=ssl_context,
790+
use_forwarding_for_https=True,
791+
)
792+
793+
with proxy:
794+
pool = proxy.connection_from_url(self.https_url)
795+
with contextlib.closing(pool._new_conn()) as conn:
796+
conn = typing.cast(HTTPSConnection, conn)
797+
conn.connect()
798+
assert isinstance(conn.sock, ssl.SSLSocket)
799+
assert conn.sock.context is proxy_ssl_context
800+
801+
@requires_network()
802+
def test_forwarding_proxy_ssl_context_fallback(self) -> None:
803+
"""
804+
Test that when only ``ssl_context`` is passed to a forwarding
805+
HTTPS proxy, a ``FutureWarning`` is emitted and ``ssl_context``
806+
is used as the proxy TLS context (fallback).
807+
"""
808+
ssl_context = ssl.create_default_context(cafile=DEFAULT_CA)
809+
810+
with pytest.warns(FutureWarning, match="ssl_context"):
811+
proxy = proxy_from_url(
812+
self.https_proxy_url,
813+
ssl_context=ssl_context,
814+
use_forwarding_for_https=True,
815+
)
816+
817+
with proxy:
818+
pool = proxy.connection_from_url(self.https_url)
819+
with contextlib.closing(pool._new_conn()) as conn:
820+
conn = typing.cast(HTTPSConnection, conn)
821+
conn.connect()
822+
assert isinstance(conn.sock, ssl.SSLSocket)
823+
assert conn.sock.context is ssl_context
824+
825+
@requires_network()
826+
def test_forwarding_non_https_proxy_ssl_context_fallback(self) -> None:
827+
"""
828+
Test that when only ``ssl_context`` is passed to a forwarding
829+
non-HTTPS proxy, no warning or error is emitted.
830+
"""
831+
ssl_context = ssl.create_default_context(cafile=DEFAULT_CA)
832+
# No warning/error should be emitted even when ``ssl_context``
833+
# is provided to a non-HTTPS proxy for some reason.
834+
proxy = proxy_from_url(
835+
self.proxy_url,
836+
ssl_context=ssl_context,
837+
use_forwarding_for_https=True,
838+
)
839+
with proxy:
840+
resp = proxy.request("GET", self.https_url)
841+
assert resp.status == 200
842+
726843
def test_scheme_host_case_insensitive(self) -> None:
727844
"""Assert that upper-case schemes and hosts are normalized."""
728845
with proxy_from_url(self.proxy_url.upper(), ca_certs=DEFAULT_CA) as http:

0 commit comments

Comments
 (0)