Skip to content

HTTPSConnection: Prefer proxy verify policy for the proxy connection - #5093

Merged
illia-v merged 9 commits into
urllib3:mainfrom
shazow:untangle-certreqs
Jul 29, 2026
Merged

illia-v merged 9 commits into
urllib3:mainfrom
shazow:untangle-certreqs

Conversation

@shazow

@shazow shazow commented Jun 26, 2026 •

Copy link
Copy Markdown
Member

Fixed HTTPSConnection overriding the proxy_config's cert policy with the target request's SSL context.

Including a proxy_config will now properly inherit verify_mode for the proxy connection and values for ca_* ssl_* are ignored.

@shazow
shazow force-pushed the untangle-certreqs branch 3 times, most recently from 3d8be4b to afdd21c Compare June 26, 2026 22:47
Comment thread src/urllib3/connection.py Outdated
with proxy_from_url(
proxy_url,
ca_certs=proxy.ca_certs,
ca_certs=server.ca_certs,

@shazow shazow Jun 28, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these test fixes make sense but I'm not 100% sure, please double check?

Before we were overriding proxy certs with server target certs anyway, so it passed. Now we don't do that anymore, so we pass proxy policy via proxy_ctx and ca_certs becomes the target server cert policy.

Feels like this was a footgun that we're fixing, but might be breaking in some edge cases? Let me know if I should expand about this in the changelog.

@shazow
shazow marked this pull request as ready for review June 28, 2026 17:04
@shazow
shazow requested review from a team and illia-v June 28, 2026 17:15

@illia-v illia-v left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall 👍🏻

Comment thread src/urllib3/connection.py Outdated
Comment on lines +810 to +814
ssl_context = self.proxy_config.ssl_context
self.sock = sock = self._connect_tls_proxy(self.host, sock)
sock_and_verified = _WrappedAndVerifiedSocket(
socket=sock,
is_verified=self.proxy_is_verified is True,
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make the private self._connect_tls_proxy return sock_and_verified instead of sock_and_verified.socket to avoid creating _WrappedAndVerifiedSocket again here?

Alternatively, we can make sock_and_verified unneeded in this method by saving local variables and using them instead of sock_and_verified.is_verified and sock_and_verified.socket.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried both, the former version felt like it was warping the intent of _WrappedAndVerifiedSocket too much, so went with the latter.

Comment thread changelog/5093.bugfix.rst Outdated
Comment on lines +4 to +8
``HTTPSConnection._connect_tls_proxy()`` now uses
``ProxyConfig.ssl_context``, ``ProxyConfig.assert_hostname``, and
``ProxyConfig.assert_fingerprint`` for the proxy handshake, while retaining the
existing fallback to ``HTTPSConnection.ssl_context``, ``cert_reqs``, ``ca_*``,
and ``ssl_*`` when ``ProxyConfig.ssl_context`` is ``None``.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid documenting details of the private method?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeaaa I was too tunnelvisioned about documenting literal changes, removed that one.

I looked at several changelog examples to try and find the appropriate tone we want there, but maybe I should rephrase to be more consumer-centric?

@DPS0340

DPS0340 commented Jul 26, 2026

Copy link
Copy Markdown

Drive-by: the lint failure here is two mypy errors, and both are one-line annotations. Posting the diff rather than a suggestion box since they are in two different files.

src/urllib3/connection.py:901: error: Incompatible types in assignment
    (expression has type "SSLContext | None", variable has type "SSLContext")  [assignment]
test/with_dummyserver/test_proxy_poolmanager.py:833: error: Item "None" of
    "ProxyConfig | None" has no attribute "ssl_context"  [union-attr]

First one — in _connect_tls_proxy, ssl_context is inferred as SSLContext from the if branch, then the else branch can assign None:

ssl_context = self.ssl_context if self.proxy_is_forwarding else None

cert_reqs right below it already declares its union up front, so the fix matches the surrounding style:

+        ssl_context: ssl.SSLContext | None
         cert_reqs: int | str | None
         if proxy_ssl_context is not None:

Second one — proxy_config is ProxyConfig | None, and proxy_from_url(...) always sets it, so narrowing is enough:

         assert proxy.proxy_ssl_context is ssl_context
+        assert proxy.proxy_config is not None
         assert proxy.proxy_config.ssl_context is None

With both applied on your branch (f1ea8b14):

$ mypy -p urllib3    # connection.py: no errors
$ mypy -p test       # test_proxy_poolmanager.py: no errors

Two things I checked so this is not misleading:

  • The coverage failure is separate and I have not looked at it; this only addresses lint.
  • black --check also flags connection.py, but the diff is at lines 270/312 ((version, code, message) → version, code, message), which is pre-existing and unrelated to this PR — my local black is simply a different version than CI's. I left it alone.

Happy to open this as a PR against your branch if that is easier than applying it by hand.

@shazow
shazow force-pushed the untangle-certreqs branch from f1ea8b1 to 2486c54 Compare July 27, 2026 15:09
@shazow
shazow force-pushed the untangle-certreqs branch from 1113753 to b5f9bba Compare July 27, 2026 15:46
@shazow
shazow requested a review from illia-v July 27, 2026 15:55

@illia-v illia-v left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks for the great work, @shazow!

@illia-v
illia-v merged commit 07408ce into urllib3:main Jul 29, 2026
38 checks passed
@illia-v illia-v linked an issue Jul 29, 2026 that may be closed by this pull request
@shazow
shazow deleted the untangle-certreqs branch July 30, 2026 14:45
Ousret added a commit to jawah/urllib3.future that referenced this pull request Sep 22, 2026
2.25.900 (2026-09-22)
=====================

- Added cohabitation wheels with a ``+isolation`` version suffix,
distributed through
the GitHub Pages isolation index with SLSA attestations. These install
only
  ``urllib3_future`` and omit the ``.pth`` startup hook.
- Fixed gzip responses with trailing garbage raising an error on Python
versions
with stricter ``zlib.Decompress.flush()`` error handling, in both sync
and async.
  See urllib3#5268
- Added opt-in response body caching for retry-delay hooks with
``Retry(cache_response_body=True)``, plus
``Retry.async_get_retry_after()`` for async
body inspection. Caching defaults to off in both sync and async. (#346)
- Backported HTTPS proxy TLS configuration isolation from upstream
urllib3#3786 and
urllib3#5093
An explicit ``proxy_ssl_context`` now retains its own trust and
certificate policy for
forwarding and CONNECT proxies, in both sync and async. Destination SNI,
identity assertions,
and client-certificate parameters no longer configure forwarding-proxy
TLS. (GHSA-8988-9cw3-xx77)
- Backported Deflate end-of-stream handling from upstream

GHSA-gh4c-6fx4-qh6g
Finished Deflate streams with trailing bytes no longer raise a spurious
``DecodeError``
  during sync or async streaming.
- Added concurrent WebSocket reads and writes over HTTP/1.1, in both
sync and async.
A waiting ``next_payload()`` releases the connection while waiting for
data, allowing
another thread or task to call ``send_payload()`` or ``ping()``.
Concurrent readers are
serialized and read timeouts remain in effect. An already-entered
blocking TLS receive
  can still delay a concurrent writer. (#400)
- Added an alternative WebSocket backend based on the ``websockets``
Sans-I/O engine.
Install ``urllib3-future[ws-fast]`` and select ``ws+fast://`` or
``wss+fast://`` for sync
and async WebSocket over HTTP/1.1. Requires Python 3.9+ and
``websockets>=15.0.1,<18``.
Plain ``ws://`` and ``wss://`` prefer ``wsproto`` when installed and
otherwise use this backend.
- Reduced webextension read/write overhead and avoided redundant
buffering of single
  protocol chunks in ``DirectStreamAccess`` and its async counterpart.
- Fixed connection allocation and pool eviction races under thread and
task contention,
including missed wakeups and incorrect ownership transfers. Available
capacity can now
be used while other connections are held, within the configured pool
limits.
- Fixed short HTTP/3 upgrade timeouts being applied to UDP socket
creation and resolver
work instead of preserving the configured connect timeout, in both sync
and async. (#416)
- Reset Linux ``UDP_SEGMENT`` after probing GSO support, preventing
unintended segmentation
  when falling back to ordinary datagram sends.
- Backported stricter URL host validation and percent normalization from
upstream
  urllib3#5095
Rejects raw spaces and control characters, encoded controls, malformed
host escapes,
and trailing newlines after ports. Validation also covers WebSocket,
SOCKS, and resolver schemes.
- Backported removal of fragments from absolute request targets from
upstream
  urllib3#5079
URL fragments are no longer sent to servers or forwarding proxies, in
both sync and async.
- Backported clearing request body framing and rewind state on HTTP 303
redirects from upstream
  urllib3#5161
Applies to sync and async, including deferred responses in multiplexed
mode.
- Backported draining unread response bodies in 64 KiB chunks from
upstream
  urllib3#5019
Reduces peak memory use in sync and async ``drain_conn()`` while
preserving upgraded
  webextension streams.
- Backported raising ``UnrewindableBodyError`` when a request body has
no ``seek()`` method from upstream
  urllib3#3780
  Applies to both synchronous and asynchronous body rewinding.
- Backported raising ``SSLError`` for non-hexadecimal certificate
fingerprints from upstream
  urllib3#5212
- Backported percent-decoding SOCKS proxy credentials from upstream
  urllib3#5031
Applies to sync and async ``python-socks`` and the ``PySocks`` fallback,
preserving explicit
credential overrides and decoding each username and password only once.
- Backported faster URL percent encoding from upstream
  urllib3#5221
Adds a fast path for valid ASCII components and reduces allocations in
the general encoder.
- Backported ``Url.auth_decoded`` and ``Url.auth_decoded_joined`` from
upstream
  urllib3#5055
These expose UTF-8-decoded credentials while keeping ``Url.auth``
percent-encoded.
- Backported ``basic_auth_encoding`` and ``proxy_basic_auth_encoding``
options in ``make_headers()`` from upstream
  urllib3#5092
Origin and proxy credentials can use independent encodings; both retain
the Latin-1 default.
- Fixed an async TLS connection cleanup race that could cause new
connections to
fail with "File descriptor ... is used by transport" when a socket
descriptor
  was immediately reused. (#417)
@openshift-merge-robot

Copy link
Copy Markdown

Fix included in release 5.1.0-0.nightly-2026-10-23-010359

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

proxy_ssl_context is mutated when target uses cert_reqs='CERT_NONE' Cannot use proxy_ssl_context with CERT_VERIFIED and ssl_context with CERT_NONE

4 participants