Add Flag to Disable SSL/TLS Certificates Verification #111
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
blaise-muhirwa
commented
Oct 12, 2023
|
|
||
| from urllib3.exceptions import InsecureRequestWarning | ||
|
|
||
| warnings.simplefilter("ignore", InsecureRequestWarning) |
Contributor
Author
There was a problem hiding this comment.
This suppresses the following warning:
/home/ubuntu/.cache/pypoetry/virtualenvs/groundlight-edge-ZbDJXDWz-py3.10/lib/python3.10/site-packages/urllib3/connectionpool.py:1056: InsecureRequestWarning: Unverified HTTPS request is being made to host 'localhost'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/1.26.x/advanced-usage.html#ssl-warnings
warnings.warn(
blaise-muhirwa
changed the title
blaise-muhirwa
commented
Oct 12, 2023
robotrapta
reviewed
Oct 13, 2023
Member
robotrapta
left a comment
robotrapta
left a comment
There was a problem hiding this comment.
Nice. A couple changes suggested for precedence logic and picking the configuration.
Co-authored-by: robotrapta <79607467+robotrapta@users.noreply.github.com>
Co-authored-by: robotrapta <79607467+robotrapta@users.noreply.github.com>
…-tls-verification
robotrapta
reviewed
Oct 17, 2023
src/groundlight/client.py
Outdated
| ), | ||
| ) from e | ||
|
|
||
| should_disable_tls_verification = disable_tls_verification or bool( |
Member
There was a problem hiding this comment.
Still not right. We want the parameter's value in code, if set, to take precedence over the environment variable. Here, if the code sets the value to False but the environment variable is true (1) then the result will be true.
We should only look at the environment variable if the parameter is None.
…hon-sdk into disable-tls-verification
robotrapta
approved these changes
Jan 3, 2024
Member
robotrapta
left a comment
robotrapta
left a comment
There was a problem hiding this comment.
Nice! Sorry for the slow review.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a constructor parameter called
disable_tls_verificationand optionally allows users to disable SSL/TLS verification via an env var calledTLS_VERIFY.I tested this on the edge endpoint where I generated self-signed TLS certificates with OpenSSL and then used the SDK from this branch to check that sending requests to an HTTPS server running on local host still works.