Rely on urllib3 hostname matching for HTTPS proxy validation.

[jalopezsilva]

This PR aims to fix issue #2400. On that issue we discovered that if you attempt to do an HTTPS proxy connection to fetch an HTTPS website with a proxy that uses an IP address we won't validate its hostname correctly and we'll throw an error when we call wrap_socket.

The issue is caused by the fact that 1) HTTPS proxy validation relies on SSLContext.check_hostname for its hostname validation. 2) IP addresses can't be used as an SNI hint so we don't pass them along with server_hostname. The end result is that SSLContext throws on the wrap_socket call as it requires a server_hostname to be set if check_hostname is True.

To address that, I'm switching over HTTPS proxy hostname validation to use urllib3's default validation.

Couple of items worth discussing:

1. I'm sending this PR directly to 1.26.x to make it easier. Once merged, I'll create an equivalent PR to merge the changes into main too.
2. I didn't add assert_proxy_fingerprint or assert_proxy_hostname options. We can discuss if we think they are needed.

[codecov]

Codecov Report

Merging #2407 (83506c3) into 1.26.x (13603ec) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 83506c3 differs from pull request most recent head fc9a230. Consider uploading reports for the commit fc9a230 to get more accurate results

@@            Coverage Diff            @@
##            1.26.x     #2407   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         2313      2322    +9     
=========================================
+ Hits          2313      2322    +9     

Impacted Files	Coverage Δ
src/urllib3/util/proxy.py	100.00% <ø> (ø)
src/urllib3/connection.py	100.00% <100.00%> (ø)
src/urllib3/connectionpool.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13603ec...fc9a230. Read the comment docs.

[pquentin]

@sethmlarson The 1.26.x required checks appear to be messed up again. :/

I'm sending this PR directly to 1.26.x to make it easier. Once merged, I'll create an equivalent PR to merge the changes into main too.

We haven't ported the CVE-2021-28363 fix to the main branch yet. It's harder than on 1.26.x because we want to lean on SSLContext to perform hostname matching. This is tracked in #2190. (If you want to work on it, it would be nice, but you certainly don't have to!)

I didn't add assert_proxy_fingerprint or assert_proxy_hostname options. We can discuss if we think they are needed.

I don't think they are needed, and in any case we shouldn't worry about them in this PR which is about fixing the IP use case.

[pquentin]

This looks great, thanks! Can you please remove your TODO and update CHANGES.rst?

[pquentin]

Closing/reopening to see if the Windows 2.7 failure was transient or not. Edit: it was!

[pquentin]

Out of curiosity, how easy would it be to have something like dummyserver/https_proxy.py in order to test this? After all, this works in tests.

[achapkowski]

@pquentin is there anything I can do to help get this in? I also have the exact problem that is being fixed here.

[pquentin]

@achapkowski Nothing specific, I expect this to land soon, then we'll see if it warrants a release or not. This definitely impacts a few users!

[sethmlarson]

Thanks for putting this together @jalopezsilva, here are some review comments for you:

[jalopezsilva]

Out of curiosity, how easy would it be to have something like dummyserver/https_proxy.py in order to test this? After all, this works in tests.

This should be super easy and a good way for everyone to be able to tests changes with HTTPS proxies. I'll create a separate PR for this :)

[jalopezsilva]

I addressed most of the review comments.

@sethmlarson, I did run into a bug while verifying the cert with the IPv6 proxy address. It's the bug that #2241 is attempting to fix essentially. Should we merge this first and then tackle that one separately?

[sethmlarson]

LGTM, thank you for this!! Let's merge this first then tackle IPv6 separately.

[pquentin]

Thanks!