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. #115

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

j3pic
Copy link

@j3pic j3pic commented Jan 19, 2018

This is a remake of #114. I had to delete the entire repo, re-fork it, and manually copy the changed file over because of the way that PR #80 was merged.

I have changed the way ssl_context() works so that the configuration check happens there instead of in _post().

@gianm
Copy link
Member

gianm commented Jan 19, 2018

@j3pic Thanks for re-raising it.

On the git stuff: we typically squash before merging to keep the history in master cleaner. So that can make future merges and rebases fail, but you should be able to get away with rebasing on master and skipping the commits from #80, or alternatively creating a new branch from master and then cherry-picking the commit you wanted from #114.

The patch looks good to me. I just checked with the powers that be and it looks like the CLA form is working once again. So could you please give that a shot, and then we can accept this patch. Thanks!

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.

LGTM after CLA is sorted.

@jachen-alch
Copy link

ping.
status on this patch?

@j3pic
Copy link
Author

j3pic commented Oct 23, 2018

I never heard back about that corporate CLA.

@vikramarsid
Copy link

@gianm Any update on this PR, else I can open a new PR crediting @j3pic.

@jachen-alch
Copy link

@gianm Ping, any update on this?

@mistercrunch
Copy link
Member

Rebase pls!

Copy link

@PythonAlchemist PythonAlchemist left a comment

Choose a reason for hiding this comment

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

Can we get these merged in, changes look great.

@didip
Copy link

didip commented Jul 1, 2021

Is there a reason why this PR is still unmerged? Not being able to talk to Druid via HTTPS is a pretty serious problem.

@j3pic
Copy link
Author

j3pic commented Jul 6, 2021

@didip My former employer, Instacart, has just ignored the request for the "CLA" document (which IIRC has something to do with copyright). I was never able to get it signed by someone high enough in the company. Someone probably needs to re-fix this problem from scratch, since I don't work there anymore.

@valer-cara
Copy link

valer-cara commented Oct 12, 2021

Indeed pretty sad that this PR hasn't yet been merged. I've used stunnel for to proxy plain http to remote TLS with a config file like below. Just set your pydruid host to http://127.0.0.1:8989 and stunnel will ship it upstream

foreground = yes # optional, yes, outside of [foobar]

[foobar]
client = yes
accept = 127.0.0.1:8989
verifyChain = no # the point of all this, ignore invalid certs
connect = some-upstream-host
sni =  some-upstream-host # optional, helps if remote host validates SNI

Thanks for creating this PR in any case! Hope it'll get merged :)

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.

8 participants