Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support a servername parameter on HTTPSConnections #1397

Merged

Conversation

@JackOfMostTrades
Copy link
Contributor

@JackOfMostTrades JackOfMostTrades commented Jun 15, 2018

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.

@@ -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):

This comment has been minimized.

@theacodes

theacodes Jun 15, 2018
Member

Can we use the same argument name as ssl_wrap_socket?

This comment has been minimized.

@JackOfMostTrades

JackOfMostTrades Jun 15, 2018
Author Contributor

Yep, can do. Changed it to server_hostname everywhere.

@JackOfMostTrades JackOfMostTrades force-pushed the JackOfMostTrades:enable-servername-parameter branch from f365329 to 8a4faeb Jun 15, 2018
@codecov-io
Copy link

@codecov-io codecov-io commented Jun 15, 2018

Codecov Report

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

Impacted file tree graph

@@          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.

Copy link
Member

@theacodes theacodes left a comment

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

@theacodes
Copy link
Member

@theacodes theacodes commented Jun 15, 2018

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

@JackOfMostTrades JackOfMostTrades force-pushed the JackOfMostTrades:enable-servername-parameter branch from 8a4faeb to dd1809c Jun 15, 2018
@JackOfMostTrades
Copy link
Contributor Author

@JackOfMostTrades JackOfMostTrades commented Jun 15, 2018

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
Copy link
Contributor Author

@JackOfMostTrades JackOfMostTrades commented Jun 15, 2018

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
Copy link
Member

@theacodes theacodes commented Jun 15, 2018

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!

@JackOfMostTrades JackOfMostTrades force-pushed the JackOfMostTrades:enable-servername-parameter branch from dd1809c to f3ff97b Jun 15, 2018
@sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Jun 18, 2018

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
Copy link
Contributor Author

@JackOfMostTrades JackOfMostTrades commented Jun 18, 2018

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
Copy link
Member

@sethmlarson sethmlarson commented Jun 19, 2018

Thanks for explaining it! Sounds good to me. 👍

@sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Jul 11, 2018

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

@pquentin
Copy link
Member

@pquentin pquentin commented Jul 11, 2018

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 JackOfMostTrades force-pushed the JackOfMostTrades:enable-servername-parameter branch from f3ff97b to 1957cf7 Jul 11, 2018
@JackOfMostTrades
Copy link
Contributor Author

@JackOfMostTrades JackOfMostTrades commented Jul 11, 2018

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

@sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Jul 11, 2018

Uh oh, @pquentin?

@pquentin
Copy link
Member

@pquentin pquentin commented Jul 12, 2018

@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
Copy link
Member

@pquentin pquentin commented Jul 12, 2018

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

@sethmlarson sethmlarson merged commit 3f2267a into urllib3:master Jul 12, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Jul 12, 2018

Merged! Thanks @JackOfMostTrades! 🎉

@JackOfMostTrades
Copy link
Contributor Author

@JackOfMostTrades JackOfMostTrades commented Jul 12, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.