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

Support self-signed SSL certificates #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mik-laj
Copy link
Member

@mik-laj mik-laj commented Aug 17, 2020

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

@cla-bot
Copy link

cla-bot bot commented Aug 17, 2020

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:
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Exactly.

Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

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.

@findepi
Copy link
Member

findepi commented Aug 20, 2020

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.

This is definitely interesting!

Let's start a new issue about this and we will exchange thoughts there.

@findepi
Copy link
Member

findepi commented Aug 21, 2020

@mik-laj @dongwoo1005 how this relates to #31 ?

@mik-laj
Copy link
Member Author

mik-laj commented Aug 21, 2020

@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:

  • using ca_bundle: in auth
  • using verify in client.

@findepi findepi requested a review from electrum August 21, 2020 21:43
@findepi
Copy link
Member

findepi commented Aug 25, 2020

Indeed, this might be confusing, but there might be a reason for that. @electrum can you comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants