Skip to content

Use server_hostname parameter in call to ssl_wrap_socket#1270

Merged
bdarnell merged 3 commits intotornadoweb:masterfrom
djvis:master
Jan 12, 2015
Merged

Use server_hostname parameter in call to ssl_wrap_socket#1270
bdarnell merged 3 commits intotornadoweb:masterfrom
djvis:master

Conversation

@djvis
Copy link
Copy Markdown
Contributor

@djvis djvis commented Dec 11, 2014

No description provided.

@bdarnell
Copy link
Copy Markdown
Member

Can you add a new test to TestIOStreamStartTLS that covers this error?

@djvis
Copy link
Copy Markdown
Contributor Author

djvis commented Dec 12, 2014

Sure. But not for a few days.

@djvis
Copy link
Copy Markdown
Contributor Author

djvis commented Dec 23, 2014

I've added a test test_check_hostname which attempts to call start_tls on a stream with the default SSL context and a server hostname of 127.0.0.1. This will catch the bug if it reappears in the future. However it does result in an SSL error due to verification failure (as far as I can tell the server isn't reporting a hostname).

Also, here is a gist showing how the error can be triggered: https://gist.github.com/djvis/2298e024ddda4e829808

@djvis
Copy link
Copy Markdown
Contributor Author

djvis commented Dec 23, 2014

Just noticed that the call to ssl.create_default_context is failing on several python versions. Will look for a solution now.

@djvis
Copy link
Copy Markdown
Contributor Author

djvis commented Dec 23, 2014

Turns out check_hostname functionality is only available in python 2.7 and 3.4 anyway. Have added a decorator to skip the test if the ssl module is missing the create_default_context function - which is a nice proxy for checking if python is of the required version.

bdarnell added a commit that referenced this pull request Jan 12, 2015
Use server_hostname parameter in call to ssl_wrap_socket
@bdarnell bdarnell merged commit 1ebdf8c into tornadoweb:master Jan 12, 2015
wking added a commit to wking/dockerfile that referenced this pull request Aug 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants