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

Make it possible to ignore SSL certificate errors. #114

Closed
wants to merge 6 commits into from
Closed

Make it possible to ignore SSL certificate errors. #114

wants to merge 6 commits into from

Conversation

j3pic
Copy link

@j3pic j3pic commented Jan 18, 2018

Imply.io uses an SSL certificate that doesn't pass certificate security checks. This PR makes PyDruid compatible with this. Call set_ignore_certificate_errors on the PyDruid object to ignore certificate errors.

This is a fork of #80, because I need basic auth, too.

@gianm I wasn't able to access the CLA. I got this error: https://i.imgur.com/4Hwlf52.png

@gianm
Copy link
Member

gianm commented Jan 19, 2018

@j3pic - I just merged #80 so you should be able to base this one off master now. Will do a code review shortly.

Copy link
Member

@gianm gianm left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @j3pic. I made a couple of suggestions.

In addition - ignoring the cert validity checks makes TLS more or less useless for security, so it'd be nice to also add an option to add additional trusted root certificates rather than disable verification completely. Then the error goes away but security is preserved. In urllib it can be done through SSLContext.load_verify_locations (see https://docs.python.org/2/library/ssl.html).

@@ -386,11 +403,20 @@ class PyDruid(BaseDruidClient):
def __init__(self, url, endpoint):
super(PyDruid, self).__init__(url, endpoint)

def ssl_context(self):
Copy link
Member

Choose a reason for hiding this comment

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

This should at least be called something like insecure_ssl_context(). The current name is misleading, possibly dangerously so since a caller might not realize the context is insecure.

I think, though, that it'd be even better to have ssl_context() always get called by _post and for it to do something different based on the configuration. Then it would make sense to keep the current name.

def _post(self, query):
try:
headers, querystr, url = self._prepare_url_headers_and_body(query)
req = urllib.request.Request(url, querystr, headers)
res = urllib.request.urlopen(req)
if self.ignore_certificate_errors:
Copy link
Member

Choose a reason for hiding this comment

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

If you do the suggestion above, this would just be res = urllib.request.urlopen(req, context=self.ssl_context()) rather than having a conditional.

@gianm
Copy link
Member

gianm commented Jan 19, 2018

Fyi - since you said you're using Imply, in that case you can download the root certificate by following the docs at https://docs.imply.io/cloud/deploy/security#downloading-the-root-tls-certificate. If you configure your client to trust that then you should be good to go.

@j3pic
Copy link
Author

j3pic commented Jan 19, 2018

@gianm #115 replaces this PR.

@j3pic j3pic closed this Jan 19, 2018
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.

2 participants