Support a servername parameter on HTTPSConnections #1397

JackOfMostTrades · 2018-06-15T01:26:18Z

The servername parameter allows callers to override the hostname used for SNI. If specified this hostname will also be used as the value for hostname verification instead of the actual hostname (unless assert_hostname is set).

This is the same option and semantics that node.js provides via the "servername" attribute, or that golang provides via the "ServerName" option.

theacodes · 2018-06-15T16:20:31Z

urllib3/connection.py

@@ -242,14 +242,15 @@ class HTTPSConnection(HTTPConnection):

    def __init__(self, host, port=None, key_file=None, cert_file=None,
                 strict=None, timeout=socket._GLOBAL_DEFAULT_TIMEOUT,
-                 ssl_context=None, **kw):
+                 ssl_context=None, servername=None, **kw):


Can we use the same argument name as ssl_wrap_socket?

Yep, can do. Changed it to server_hostname everywhere.

codecov-io · 2018-06-15T17:53:47Z

Codecov Report

Merging #1397 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1397   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          21      21           
  Lines        1788    1794    +6     
======================================
+ Hits         1788    1794    +6

Impacted Files	Coverage Δ
urllib3/connection.py	`100% <100%> (ø)`	⬆️
urllib3/util/wait.py	`100% <0%> (ø)`	⬆️

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 6ef3c06...1957cf7. Read the comment docs.

theacodes

This looks pretty good, can you add an entry in the changelog for this?

theacodes · 2018-06-15T18:00:36Z

Also we'll need to make sure coverage stays at 100%.

JackOfMostTrades · 2018-06-15T19:56:05Z

Sure thing; I've updated the PR with a changelog update and added a test which asserts that server_hostname gets passed through to the wrapped socket and gets the code coverage back up to 100%.

JackOfMostTrades · 2018-06-15T19:58:27Z

And by the way, the docs in this project for helping a new developer get tests running are fantastic. That's always been such a struggle with other projects and it was incredibly easy this time around. I appreciate the time spent writing that and keeping it up to date! :)

theacodes · 2018-06-15T19:58:56Z

And by the way, the docs in this project for helping a new developer get tests running are fantastic. That's always been such a struggle with other projects and it was incredibly easy this time around. I appreciate the time spent writing that and keeping it up to date! :)

That's great to hear!

sethmlarson · 2018-06-18T03:48:39Z

Thanks for submitting this patch. Can you help me understand the use-case for this parameter? I'm unsure of a situation where you'd want to override the hostname sent via SNI compared to the one you're connecting to.

JackOfMostTrades · 2018-06-18T18:37:38Z

Can you help me understand the use-case for this parameter?

Sure. This doesn't really apply on the public internet, but it's something I've run into quite a bit in internal infrastructure. The most common case where I've run into this where you're connecting by IP instead of hostname (but you still want a particular virtual host served back). There's a few reasons you might be connecting by IP:

You're not using DNS for host resolution; e.g. Netflix uses Eureka which has built-in load-balancing and failover logic.
You're on an internal network and the DNS name would resolve to a public IP address which doesn't route correctly from the internal network.
You're running integration tests, e.g. against localhost. The service might not actually get added to the DNS hostname until after the integration tests have run and verified that the virtualhost configuration is working as expected.

sethmlarson · 2018-06-19T01:22:46Z

Thanks for explaining it! Sounds good to me. 👍

sethmlarson · 2018-07-11T13:05:54Z

Looks like a merge conflict that needs resolving for urllib3/connection.py. Could you address this and resubmit? :)

pquentin · 2018-07-11T13:11:10Z

urllib3/connection.py moved to src/urllib3/connection.py, so that should not be too hard to fix. :) The good news is that there is a small chance that a rebase will also fix the test_client_no_intermediate error on Windows.

…e name used for SNI/hostname verification.

JackOfMostTrades · 2018-07-11T17:28:56Z

Rebased. The windows test cleared up, although now a random macOS run of test_client_no_intermediate failed.

sethmlarson · 2018-07-11T17:33:30Z

Uh oh, @pquentin?

pquentin · 2018-07-12T03:56:35Z

@SethMichaelLarson Yes, this confirms that the serial number length issue is only a macOS 10.13+ issue or at least was not the only problem to fix. That at least makes sense, but unfortunately that also means that the macOS 10.12 flakiness on CI is not solved yet. At least now on every failing build we can see 1/ the actual exception 2/ where it got thrown.

pquentin · 2018-07-12T03:57:31Z

(However, the problem is unrelated to this pull request.)

sethmlarson · 2018-07-12T04:05:48Z

Merged! Thanks @JackOfMostTrades! 🎉

JackOfMostTrades · 2018-07-12T04:26:22Z

Yay! Thanks for everyone's time getting this cleaned up and merged in. I appreciate it!

…e name used for SNI/hostname verification. (urllib3#1397)

This add the missing support to set the `server_hostname` setting when creating TCP connection, when the underlying connection is authenticated using TLS. See the documentation for the 2 stdlib functions: * https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.create_connection * https://docs.python.org/3/library/asyncio-eventloop.html#opening-network-connections The implemention is similar to what was done in urllib3 in urllib3/urllib3#1397 This would be needed to support features in clients using aiohttp, such as tomplus/kubernetes_asyncio#267 Closes: aio-libs#7114

…7541) ## What do these changes do? This adds the missing support to set the `server_hostname` setting when creating TCP connection, when the underlying connection is authenticated using TLS. See the documentation for the 2 stdlib functions: * https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.create_connection * https://docs.python.org/3/library/asyncio-eventloop.html#opening-network-connections This would be needed to support features in clients using aiohttp, such as tomplus/kubernetes_asyncio#267 ## Are there changes in behavior for the user? The default behavior should not change, but this would allow on a per-connection basis to specify a custom server name to check the certificate name against. ## Related issue number Closes: #7114 (for reference, similar implementation in urllib3: urllib3/urllib3#1397) ## Checklist - [x] I think the code is well written - [x] Unit tests for the changes exist - [x] Documentation reflects the changes - [x] If you provide code modification, please add yourself to `CONTRIBUTORS.txt` * The format is <Name> <Surname>. * Please keep alphabetical order, the file is sorted by names. - [x] Add a new news fragment into the `CHANGES` folder * name it `<issue_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Sam Bull <aa6bs0@sambull.org>

…io-libs#7541) This adds the missing support to set the `server_hostname` setting when creating TCP connection, when the underlying connection is authenticated using TLS. See the documentation for the 2 stdlib functions: * https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.create_connection * https://docs.python.org/3/library/asyncio-eventloop.html#opening-network-connections This would be needed to support features in clients using aiohttp, such as tomplus/kubernetes_asyncio#267 The default behavior should not change, but this would allow on a per-connection basis to specify a custom server name to check the certificate name against. Closes: aio-libs#7114 (for reference, similar implementation in urllib3: urllib3/urllib3#1397) - [x] I think the code is well written - [x] Unit tests for the changes exist - [x] Documentation reflects the changes - [x] If you provide code modification, please add yourself to `CONTRIBUTORS.txt` * The format is <Name> <Surname>. * Please keep alphabetical order, the file is sorted by names. - [x] Add a new news fragment into the `CHANGES` folder * name it `<issue_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Sam Bull <aa6bs0@sambull.org> (cherry picked from commit ac29dea)

theacodes reviewed Jun 15, 2018

View reviewed changes

JackOfMostTrades force-pushed the enable-servername-parameter branch from f365329 to 8a4faeb Compare June 15, 2018 17:28

theacodes reviewed Jun 15, 2018

View reviewed changes

JackOfMostTrades force-pushed the enable-servername-parameter branch from 8a4faeb to dd1809c Compare June 15, 2018 19:53

theacodes approved these changes Jun 15, 2018

View reviewed changes

JackOfMostTrades force-pushed the enable-servername-parameter branch from dd1809c to f3ff97b Compare June 15, 2018 20:28

Support a servername parameter on HTTPSConnections which overrides th…

1957cf7

…e name used for SNI/hostname verification.

JackOfMostTrades force-pushed the enable-servername-parameter branch from f3ff97b to 1957cf7 Compare July 11, 2018 17:01

sethmlarson merged commit 3f2267a into urllib3:master Jul 12, 2018

Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022

Support a servername parameter on HTTPSConnections which overrides th…

5325a46

…e name used for SNI/hostname verification. (urllib3#1397)

multani mentioned this pull request Aug 19, 2023

Configure SNI multani/aiohttp#1

Closed

5 tasks

multani mentioned this pull request Aug 19, 2023

Support passing a custom server name parameter on HTTPS connection aio-libs/aiohttp#7541

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support a servername parameter on HTTPSConnections #1397

Support a servername parameter on HTTPSConnections #1397

JackOfMostTrades commented Jun 15, 2018

theacodes Jun 15, 2018

JackOfMostTrades Jun 15, 2018

codecov-io commented Jun 15, 2018 •

edited

theacodes left a comment

theacodes commented Jun 15, 2018

JackOfMostTrades commented Jun 15, 2018

JackOfMostTrades commented Jun 15, 2018

theacodes commented Jun 15, 2018

sethmlarson commented Jun 18, 2018

JackOfMostTrades commented Jun 18, 2018

sethmlarson commented Jun 19, 2018

sethmlarson commented Jul 11, 2018

pquentin commented Jul 11, 2018

JackOfMostTrades commented Jul 11, 2018

sethmlarson commented Jul 11, 2018

pquentin commented Jul 12, 2018

pquentin commented Jul 12, 2018

sethmlarson commented Jul 12, 2018

JackOfMostTrades commented Jul 12, 2018

Support a servername parameter on HTTPSConnections #1397

Support a servername parameter on HTTPSConnections #1397

Conversation

JackOfMostTrades commented Jun 15, 2018

theacodes Jun 15, 2018

Choose a reason for hiding this comment

JackOfMostTrades Jun 15, 2018

Choose a reason for hiding this comment

codecov-io commented Jun 15, 2018 • edited

Codecov Report

theacodes left a comment

Choose a reason for hiding this comment

theacodes commented Jun 15, 2018

JackOfMostTrades commented Jun 15, 2018

JackOfMostTrades commented Jun 15, 2018

theacodes commented Jun 15, 2018

sethmlarson commented Jun 18, 2018

JackOfMostTrades commented Jun 18, 2018

sethmlarson commented Jun 19, 2018

sethmlarson commented Jul 11, 2018

pquentin commented Jul 11, 2018

JackOfMostTrades commented Jul 11, 2018

sethmlarson commented Jul 11, 2018

pquentin commented Jul 12, 2018

pquentin commented Jul 12, 2018

sethmlarson commented Jul 12, 2018

JackOfMostTrades commented Jul 12, 2018

codecov-io commented Jun 15, 2018 •

edited