Skip to content

Fix #173: HTTPS request doesn't work when disable_ssl_certificate_validation=True #288

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.

Sign up for GitHub

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

Closed
r14c wants to merge 2 commits into from
Closed

Fix #173: HTTPS request doesn't work when disable_ssl_certificate_validation=True #288

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af0d43f
Fix #173
Nov 25, 2014
015b332
Changes based on comments on pull #173
Nov 25, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions python3/httplib2/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,18 +827,19 @@ def __init__(self, host, port=None, key_file=None, cert_file=None,
timeout=None, proxy_info=None,
ca_certs=None, disable_ssl_certificate_validation=False):
self.proxy_info = proxy_info
context = None
if ca_certs is None:
ca_certs = CA_CERTS
if (cert_file or ca_certs) and not disable_ssl_certificate_validation:
if not hasattr(ssl, 'SSLContext'):
raise CertificateValidationUnsupportedInPython31()
context = ssl.SSLContext(ssl.PROTOCOL_TLSv1)
if not hasattr(ssl, 'SSLContext'):
raise CertificateValidationUnsupportedInPython31()

@jayvdb jayvdb Nov 25, 2014

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm pretty sure this causes Python 3.1 to always raise an exception, even when certification validation is explicitly not disabled, - i.e. when disable_ssl_certificate_validation is enabled.

fwiw, there is another pending patch for this issue; pull #269 , which has bitrotted.

@r14c r14c Nov 25, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@jayvdb you are totally right, I'll update the patch.

context = ssl.SSLContext(ssl.PROTOCOL_TLSv1)
if disable_ssl_certificate_validation:
context.verify_mode = ssl.CERT_OPTIONAL

@jayvdb jayvdb Nov 25, 2014

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What is the benefit of using ssl.CERT_OPTIONAL?
This looks like another bit of the patch which will break on Python 3.1.
While it isnt documented at https://docs.python.org/3/library/ssl.html#ssl.SSLContext.verify_mode as being 3.2 specific, verify_mode doesnt appear in https://docs.python.org/3.1/library/ssl.html

@r14c r14c Nov 25, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/carlos/Public/zulu/oss/httplib2/python3/httplib2/__init__.py", line 851, in __init__
    check_hostname=True)
  File "/usr/local/Cellar/python3/3.4.2_1/Frameworks/Python.framework/Versions/3.4/lib/python3.4/http/client.py", line 1211, in __init__
    raise ValueError("check_hostname needs a SSL context with "
ValueError: check_hostname needs a SSL context with either CERT_OPTIONAL or CERT_REQUIRED

@jayvdb jayvdb Nov 26, 2014

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

But what is wrong with the very small patch that everyone has been using for three years?
i.e. pull #289 (but it has been submitted many times previously).

I've tested it on py3.3 and 3.4 (to confirm it fixes the unittests at https://github.com/jayvdb/pywikibot-core/blob/master/tests/http_tests.py), and it has also been tested on py3.2 Windows by @fijiaaron.

@r14c r14c Nov 26, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I believe my patch fixes the issue without avoiding code paths that are intended to improve security.

context = ssl.SSLContext(ssl.PROTOCOL_TLSv1)

Newer versions of Python 3 include SSLContext objects in your requests. Among other things, these allow you to specify which protocol to use. That means we get a reasonable amount of security even when we don't care that the certificate in use is signed by a CA.

@jayvdb jayvdb Nov 26, 2014

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Enabling TLS and ssl protocol version (with or without disable_ssl_certificate_validation=True) is .. IMO another issue. An issue was created recently about that #287 , and ive found an earlier one, even a patch.

Regarding the certificate verification aspects of your patch, ...

ssl.CERT_OPTIONAL does not seem like a good fit for "disable_ssl_certificate_validation", as "In this mode no certificates will be required from the other side of the socket connection; but if they are provided, validation will be attempted and an SSLError will be raised on failure."
< https://docs.python.org/3.2/library/ssl.html#constants ; 3.3 and 3.4 says the same >

That means SSL exceptions are going to occur if the server sends the wrong certificate...?

And your patch leaves check_hostname=True.

"If context is specified and has a verify_mode of either CERT_OPTIONAL or CERT_REQUIRED, then by default host is matched against the host name(s) allowed by the server’s certificate."

Again, this means SSL certificate errors will occur, which seems to be against the intention of "disable_ssl_certificate_validation".
IMO disable_ssl_certificate_validation is an explicit "I cant be bothered with security", which is OK in some situations, but the real solution is to provide a custom ca_certs/cert_file, or allow known good scenarios like 'allowed self-signed certificate'.

else:
context.verify_mode = ssl.CERT_REQUIRED
if cert_file:
context.load_cert_chain(cert_file, key_file)
if ca_certs:
context.load_verify_locations(ca_certs)
if cert_file:
context.load_cert_chain(cert_file, key_file)
if ca_certs:
context.load_verify_locations(ca_certs)
http.client.HTTPSConnection.__init__(
self, host, port=port, key_file=key_file,
cert_file=cert_file, timeout=timeout, context=context,
Expand Down