Striped square brackets from host IP

[derekhiggins]

When comparing host IP to a cert subjectAltName->"IP Address"
ensure were strip square brackets from IPv6 addresses.

Fixes: #2240

[derekhiggins]

I also looked into stripping out the brackets further up the stack
in PoolManager.connection_from_host , but this causes the CONNECT to the proxy to break
1621610373.972 3 fd00:1101::1 TCP_TUNNEL/200 1050 CONNECT [fd00:1101::7d50:2883:3df9:68ce]:9999 - HIER_DIRECT/fd00:1101::7d50:2883:3df9:68ce -
vs.
1621606788.209 257 fd00:1101::1 TAG_NONE/503 0 CONNECT fd00:1101 - HIER_NONE/- -

[codecov]

Codecov Report

Merging #2241 (1d64d79) into main (384feec) will decrease coverage by 0.72%.
The diff coverage is 100.00%.

❗ Current head 1d64d79 differs from pull request most recent head 42deee0. Consider uploading reports for the commit 42deee0 to get more accurate results

@@             Coverage Diff             @@
##              main    #2241      +/-   ##
===========================================
- Coverage   100.00%   99.27%   -0.73%     
===========================================
  Files           25       25              
  Lines         2478     2478              
===========================================
- Hits          2478     2460      -18     
- Misses           0       18      +18     

Impacted Files	Coverage Δ
src/urllib3/util/ssl_.py	96.39% <100.00%> (-3.61%)	⬇️
src/urllib3/util/ssl_match_hostname.py	100.00% <100.00%> (ø)
src/urllib3/contrib/socks.py	81.08% <0.00%> (-18.92%)	⬇️
src/urllib3/util/wait.py	91.11% <0.00%> (-8.89%)	⬇️
src/urllib3/response.py	99.26% <0.00%> (-0.74%)	⬇️

---

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 384feec...42deee0. Read the comment docs.

[hramezani]

I am not sure about the right place to stripping out the brackets would be match_hostname function, but the implementation and tests look good to me.
Also, I think we need to backport it to 1.26.x

[sethmlarson]

I wonder if there's a better place to do this within the proxy flow?

[derekhiggins]

I wonder if there's a better place to do this within the proxy flow?

The alternative I tried was to strip them out in PoolManager.connection_from_host but this failed.

Looking a bit deeper this appears to be because HTTPSConnection.set_tunnel requires the square brackets to be in place. Which I guess is why they were left in place in the first place. I could strip them out in connection_from_host and then put them
back in for the call to set_tunnel but I'm not sure if there are other places where I'd need to put them back.

[derekhiggins]

Rebased

[sethmlarson]

This much more localized implementation looks good to me, thank you!

Would love an integration test too, especially one that tests proxies too (cc @jalopezsilva).

The integration test on 1.26.x will probably reveal a slightly more complex route for 1.26.x because iirc we don't unconditionally use our implementation of ssl_match_hostname() there. Will have to take a peek.

[sethmlarson]

Applied type hint updates and made mypy happy with the branch. One of the changes was switching from Dict -> Mapping for the cert type, mypy was complaining for variance/covariance.

Since I made changes myself I'll wait for someone else to review as well.

[jalopezsilva]

Looks good to me!

[jalopezsilva]

@sethmlarson, should I add the proxy integration directly here or wait until we merge it?

[sethmlarson]

@jalopezsilva Let's merge this then you can add the proxy integration after. I think this one may be non-trivial to backport, maybe you can look at this a bit too?