-
Notifications
You must be signed in to change notification settings - Fork 184
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
Conversation
Can you also add 'context.check_hostname = True' if not disable_validation? In addition, can you also add a new test case? |
Done.
How exactly do you think such a test case would look like? |
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'] |
Also I think line 83 validation is wrong.
|
The statement on line 83 will only enable hostname validation if certificate validation is enabled. ( |
I've also added the requested unit test |
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. |
Added more checks for |
cert_reqs=cert_reqs, ca_certs=ca_certs, | ||
ssl_version=ssl_version) | ||
|
||
if hasattr(ssl, 'SSLContext'): # Python 2.7.9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two spaces before #.
Actually I will just approve for now and clean up styling later. |
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.