Skip to content

Add support for system truststore#9805

Draft
Secrus wants to merge 1 commit intopython-poetry:mainfrom
Secrus:truststore
Draft

Add support for system truststore#9805
Secrus wants to merge 1 commit intopython-poetry:mainfrom
Secrus:truststore

Conversation

@Secrus
Copy link
Member

@Secrus Secrus commented Oct 28, 2024

Pull Request Check List

Resolves: #TBD

  • Added tests for changed code.
  • Updated documentation for changed code.

Copy link
Member

@abn abn left a 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

Comment on lines +352 to +354
import truststore

truststore.inject_into_ssl()
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

@Secrus Secrus force-pushed the truststore branch 4 times, most recently from a0d2e18 to 32e5ff6 Compare January 15, 2025 14:30
@ikappaki
Copy link

ikappaki commented Feb 20, 2025

HI @Secrus, Is there much left to address in this PR or is there anything an external observer can do to help? I just wanted to give a gentle nudge since it's fundamental for expanding Poetry's support to a wider range of environments.

thanks

@Secrus
Copy link
Member Author

Secrus commented Feb 20, 2025

@ikappaki what is left is the testing. There is no test to check which truststore is used and I didn't have time to come up with one.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants