-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Add support for system truststore #9805
base: main
Are you sure you want to change the base?
Conversation
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.
You also might want to update the http
fixture in tests/conftest.py
to be something like this:
@pytest.fixture
def http(mocker: MockerFixture) -> Iterator[type[httpretty.httpretty]]:
mocker.patch("truststore.inject_into_ssl")
httpretty.reset()
with httpretty.enabled(allow_net_connect=False, verbose=True):
yield httpretty
import truststore | ||
|
||
truststore.inject_into_ssl() |
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.
shouldn't this be wrapped in try/catch for ImportError? We should not need the ssl_truststore
module.
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.
As you can see, there are several checks done in ssl_truststore
module. I wanted to isolate it and avoid cluttering the Application
with 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.
As I understood it those checks are redundant. If truststore cannot be used an import error will be raised.
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.
Simple ImportError
is too little in my taste. The _is_truststore_available()
function has various checks and logs a proper message.
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 might be, but it does not add anything in this context.
https://truststore.readthedocs.io/en/latest/
If Truststore can’t work for a given platform due to APIs not being available then at import time the exception ImportError will be raised with an informative message
The only case where it makes sense is if we need to programmatically check if truststore is available. If that is the case, we might not want inject_into_ssl
anyway.
Also note;
The call to truststore.inject_into_ssl() should be called as early as possible in your program as modules that have already imported ssl.SSLContext won’t be affected.
This means this might be too late as imports to repos and authenticator would have already happened.
0529b8e
to
a0d2e18
Compare
Pull Request Check List
Resolves: #TBD