fix: use the correct SSL config if cert verification is disabled #187
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
Signed-off-by: Norbert Biczo <[email protected]>
Signed-off-by: Norbert Biczo <[email protected]>
pyrooka
force-pushed
the
nb/fix-disable-ssl
branch
from
0c0e0c0 to
7fa94d2
Compare
padamstx
reviewed
Jan 23, 2024
There was a problem hiding this comment.
LGTM, but I have a couple of questions/comments:
- Perhaps we should add a negative test that shows that an exception occurs (bad certificate) if SSL verification is NOT disabled.
- Perhaps it would be good to document how you created the certificate and key files used during the test. Maybe the test_no_ssl_verification.py file would be a good place for this?
Signed-off-by: Norbert Biczo <[email protected]>
Signed-off-by: Norbert Biczo <[email protected]>
Signed-off-by: Norbert Biczo <[email protected]>
Signed-off-by: Norbert Biczo <[email protected]>
Signed-off-by: Norbert Biczo <[email protected]>
pyrooka
force-pushed
the
nb/fix-disable-ssl
branch
from
a8330c2 to
af12ac3
Compare
Signed-off-by: Norbert Biczo <[email protected]>
pyrooka
commented
Jan 24, 2024
Signed-off-by: Norbert Biczo <[email protected]>
ibm-devx-sdk
pushed a commit
that referenced
this pull request
Jan 24, 2024
## [3.19.1](v3.19.0...v3.19.1) (2024-01-24) ### Bug Fixes * use the correct SSL config if cert verification is disabled ([#187](#187)) ([7fc172a](7fc172a))
|
🎉 This PR is included in version 3.19.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
It turned out, disabling the SSL certificate verification didn't work as expected.
We passed the right arguments to the
requestspackage, but since we usea custom SSL context, it couldn't turn off the verification entirely, because the
default SSL context has the
check_hostnameset toTrueand the documentationsays the following: "
verify_modeis now automatically changed toCERT_REQUIREDwhen hostname checking is enabled and
verify_modeisCERT_NONE."This commit adds an addition step which disables the hostname checking in our
custom SSL context.
Ref: #186