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

Send SNI data for SSL connections on Python 2.7.9+ #13

Merged
merged 3 commits into from
Jun 17, 2016
Merged

Send SNI data for SSL connections on Python 2.7.9+ #13

merged 3 commits into from
Jun 17, 2016

Conversation

ntninja
Copy link
Contributor

@ntninja ntninja commented Jun 16, 2016

Make use of the backported ssl.SSLContext that is available since Python 2.7.9 to send the hostname as part of the SSL connection to the server.

@stinky2nine
Copy link
Contributor

Can you also add 'context.check_hostname = True' if not disable_validation?

In addition, can you also add a new test case?

@ntninja
Copy link
Contributor Author

ntninja commented Jun 16, 2016

Can you also add 'context.check_hostname = True' if not disable_validation?

Done.

In addition, can you also add a new test case?

How exactly do you think such a test case would look like?

@stinky2nine
Copy link
Contributor

Can you revert line 82? No need to line it up to the next line.

I just want to make sure there is test coverage for any new code addition. You can probably just test the returned ssl wrap socket.

>>> dir(ssl_sock.context)
['__class__', '__delattr__', '__doc__', '__format__', '__getattribute__', '__hash__', '__init__', '__module__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__slots__', '__str__', '__subclasshook__', '__weakref__', '_load_windows_store_certs', '_set_alpn_protocols', '_set_npn_protocols', '_windows_cert_stores', '_wrap_socket', 'cert_store_stats', 'check_hostname', 'get_ca_certs', 'load_cert_chain', 'load_default_certs', 'load_dh_params', 'load_verify_locations', 'options', 'protocol', 'session_stats', 'set_alpn_protocols', 'set_ciphers', 'set_default_verify_paths', 'set_ecdh_curve', 'set_npn_protocols', 'set_servername_callback', 'verify_flags', 'verify_mode', 'wrap_socket']

@stinky2nine
Copy link
Contributor

Also I think line 83 validation is wrong.

check_hostname needs a SSL context with either CERT_OPTIONAL or CERT_REQUIRED

@ntninja
Copy link
Contributor Author

ntninja commented Jun 17, 2016

The statement on line 83 will only enable hostname validation if certificate validation is enabled. (ssl.CERT_NONE => 0 therefor bool(ssl.CERT_NONE) => False)
I have updated the code for clarity through.

@ntninja
Copy link
Contributor Author

ntninja commented Jun 17, 2016

I've also added the requested unit test

@stinky2nine
Copy link
Contributor

The statement on line 83 will only enable hostname validation if certificate validation is enabled. (ssl.CERT_NONE => 0 therefor bool(ssl.CERT_NONE) => False)

I wouldn't do it that way because SSL.CERT_REQUIRED is 2 so I would do explicit checking. I see that you've already updated the code. Thanks.

Can you add comments to the top of the file on python2/httplib2/test/server.key|server.pem? It's a self-signed certificate?

For the new py test file (thanks by the way), can you follow PEP 8 style guide (https://www.python.org/dev/peps/pep-0008/) and Google Python style guide (https://google.github.io/styleguide/pyguide.html)? It makes code easier to read and less line wrapping.

I would also add code to validate conn.sock.verify_mode, check_hostname|, and protocol.

@ntninja
Copy link
Contributor Author

ntninja commented Jun 17, 2016

Added more checks for verify_mode, check_hostname and protocol to the unit test, converted tabs to 4 spaces in unit test and added a comment (and certtool text annotations) to the certificate files.

cert_reqs=cert_reqs, ca_certs=ca_certs,
ssl_version=ssl_version)

if hasattr(ssl, 'SSLContext'): # Python 2.7.9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two spaces before #.

@stinky2nine
Copy link
Contributor

Actually I will just approve for now and clean up styling later.

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

Successfully merging this pull request may close these issues.

None yet

2 participants