-
Notifications
You must be signed in to change notification settings - Fork 0
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
Using SSL options in BucketFS connection #87
Conversation
|
||
sslopt = _extract_ssl_options(conf) | ||
verify = sslopt.get("cert_reqs") == ssl.CERT_REQUIRED | ||
verify = sslopt.get("ca_certs") or sslopt.get("ca_cert_path") or verify |
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 think, that doesn't do what you expect, I think, you want to assign either sslopt.get("ca_certs") or sslopt.get("ca_cert_path") or verify, however the or operation will force conversion to boolean
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.
sslopt is a temporary object. There is a function that creates it from the data in the secret store. It checks if the provided files and directories actually exist, so I thought it would be good to use it.
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.
sslopt is not the issue, I am not sure, if the "or" assignment works
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.
It doesn't convert to boolean. See the last test.
exasol/nb_connector/connections.py
Outdated
Currently, it's not possible to set any of the TLS/SSL parameters. If secured comm | ||
is selected it automatically sets the certificate validation on. |
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.
should this be removed?
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.
Ah, yes, thanks for spotting it.
closes #86