-
Notifications
You must be signed in to change notification settings - Fork 169
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
Support self-signed SSL certificates #36
base: master
Are you sure you want to change the base?
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla. |
@@ -85,7 +85,7 @@ def set_http_session(self, http_session): | |||
delegate=self._delegate, | |||
service=self._service_name, | |||
) | |||
if self._ca_bundle: | |||
if self._ca_bundle is not None: |
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.
Is your intention to have _ca_bundle=False here so that http_session.verify
will be set to False
?
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.
Yes. Exactly.
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.
I am not sure this is an intuitive API
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.
What do you propose in return? Will it add a new parameter that will be mutually exclusive with ca_bundle?
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.
Does something like how PostgreSQL (and others) construct their URLs make sense? i.e. ?ssl=true&sslmode=verify&sslcert=cert.crt
ssl
(http/https) and sslcert
(ca bundle) are already there.
sslmode
can be introduced as a new parameter.
This is definitely interesting! Let's start a new issue about this and we will exchange thoughts there. |
@mik-laj @dongwoo1005 how this relates to #31 ? |
I think this is fine for me, but the confusing thing is that you can configure SSL validation in two ways:
|
Indeed, this might be confusing, but there might be a reason for that. @electrum can you comment? |
Hello,
I would like to add support for self-signed SSL certificates. Requests can ignore verifying the SSL certificate if we set verify to False: Unfortunately, in this library, it is not possible due to one incorrect condition in if-statement.
https://requests.readthedocs.io/en/master/user/advanced/#ssl-cert-verification
This is what I need to work on integration testing for Kerberos + Presto. I currently have a prepared environment.
https://github.com/mik-laj/presto-kerberos-docker
In the next step, I would like to integrate it with Apache Airflow.
Question for the project maintainer:
Would you like to use this environment in your CI/CD as well to test authorizations with Kerberos? Currently, they are very easy to configure. Just call
start.sh
and a full environment in Docker will be set up.Best regards,
Kamil