Skip to content
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

feat: Support OS truststore for the TLS certificate verification #9685

Closed
wants to merge 4 commits into from

Conversation

Ousret
Copy link

@Ousret Ousret commented Sep 12, 2024

This commit add support for loading the OS truststore root certificates in addition to the "legacy" certifi bundle. It will come in handy for every user behind a company proxy or just using a private PyPI warehouse.

The underlying library is called "wassima" and works without touching any Python internals (aka. SSLContext monkeypatch) and is compatible from Python 3.7 and greater. It's about a year old, and count approx 100k pull per month (very modest).

Its main strength is to be extremely simple. I remain available to answer any concerns.

Pull Request Check List

Resolves: #9249

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

@Ousret Ousret force-pushed the feat-truststore branch 2 times, most recently from 7fece0d to 13ec497 Compare September 12, 2024 18:41
@dimbleby
Copy link
Contributor

for comparison

  • pdm uses truststore and requires it explicitly to be injected as pdm self add truststore
  • pip uses truststore but only if you provide the command line flag asking for it
  • uv also requires a flag to use the system trust store (asserting that on macOS, reading the system trust store "incurs a significant delay")

notable that wassima is wrapping compiled code which makes it less portable - though I see that there is a good range of wheels available - and perhaps also a little harder to trust.

I would think that the barriers to merging this would be:

  • (nothing personal! but it needs asking for this sort of thing) do we trust wassima?
  • what platforms does wassima not support, is that going to be a problem?
  • should this be always on?

perhaps it is something that could be delivered as a plugin, so the user experience would be somewhat as it is in pdm: users would be expected to install the plugin in their poetry environment if they want to use it.

that way this project could duck those difficult questions for a little longer

@Secrus
Copy link
Member

Secrus commented Sep 12, 2024

To be honest, I don't like this. Seems like the library is quite fresh and wasn't battle-tested before. I don't feel like Poetry is a project that wants to be a testing ground for libraries. I would much rather see us use truststore, enabling it basing on the underlying Python version (truststore only supports Python 3.10+). Also, I don't think we need a library that wraps a compiled code unless there is no other viable option (in this case, there is truststore).
So, from my side, the question is: @Ousret, are you willing to work on this feature, but basing it on truststore instead of your own library?

@Ousret
Copy link
Author

Ousret commented Sep 13, 2024

Hello,

@dimbleby

what platforms does wassima not support,

It support Windows, MacOS, Linux, and FreeBSD. Wheels are available for every platform but FreeBSD due to restriction by the PyPI (FreeBSD does not have a concept of manylinux). Soon to be supported in Android. iOS is unlikely to be supported. So, beside what is listed, it won't work. But it cover easily 98% of the traffic (according to the public dataset of "downloads")

is that going to be a problem?

Not at all. It was developed with the main idea that it will never require a compilation toolchain and rather fallback on certifi if installed. And in "poetry" case, it is always there. so definitely no.

should this be always on?

Absolutely, yes. There's no reason not to trust the root CA carefully handled by the operating system. It's just wierd to issue a command that basically says:

$ poetry --please-trust-my-trusted-os-store

finally, uv, is based on what we use. without the bridge, of course.


@Secrus

To be honest, I don't like this. Seems like the library is quite fresh and wasn't battle-tested before.

Not at all. The core logic is battle tested, and actually, more battle tested than its counterpart. rustls-native-certs is five years old + 250k pull per day and truststore has 2 years + 60k pull per day. To date.

So, the question is, can we, as engineer look at fifty-ich lines of code and assess if it's a okay solution to start on? Do we need "other" people to "run it" so that we gain confidence? Do "other" people say the same thing, therefor locked in circular loop?

rustls-native-certs is trusted. "wassima" is rather new, okay, but I am not new myself, and can see what projects I maintain.

Also, I don't think we need a library that wraps a compiled code unless there is no other viable option (in this case, there is truststore).

truststore isn't as viable as you think it is. if I started "wassima" it was because of issues caused by it. The monkeypatching of the ssl standard library is causing weird side effect depending on numerous factors, it also have an unconsistent behavior depending on the platform (e.g. MacOS, and Windows, they use the native OS certificate validation (they don't extract the root CA, but rather send the validation process to the OS, which is fine but I was looking for same behavior across OSes), whereas Linux still is based on Python native capabilities).

are you willing to work on this feature, but basing it on truststore instead of your own library?

unfortunately no. you'll understand, for sure.

Also, I don't think we need a library that wraps a compiled code unless there is no other viable option

There's some library in the deps tree that are "wraps" around compiled code and those have an alternative. e.g. msgpack for instance.
Down the rabbit hole, isn't everything wrapped code in the end with CPython?


If the final answer is no, then I'll invite you to close this PR.

Thanks for the (fast) response.

regards.

@Ousret Ousret force-pushed the feat-truststore branch 2 times, most recently from 1a73cd6 to 34c00b4 Compare September 13, 2024 05:32
@dimbleby
Copy link
Contributor

should this be always on?
Absolutely, yes

Yet all the other projects disagree, for varying reasons... are uv's performance concerns real?

I am less allergic than Secrus to compiled code: if it's a good solution then it's a good solution.

I notice you have suppressed type warnings. Prefer instead to add annotations.

@Ousret
Copy link
Author

Ousret commented Sep 13, 2024

Yet all the other projects disagree

Not at all. for example, recently, pip changed to truststore+certifi by default as I am proposing here but with "wassima"+certifi.

are uv's performance concerns real?

In our era where an energetic crisis is happening, along with global warming, I think speed and cpu time saving are vital.

I notice you have suppressed type warnings. Prefer instead to add annotations.

Yes, but Requests is untyped, and the typeshed does not have type for this method. I am uncomfortable setting types, but if you want to, I can, for sure.

regards,

@dimbleby
Copy link
Contributor

In our era where an energetic crisis is happening, along with global warming, I think speed and cpu time saving are vital.

a curious response, but insofar as it addresses the question I think you are agreeing that there is a performance problem?

@dimbleby
Copy link
Contributor

typeshed does not have type for this method

if it makes you happier - I'd encourage you to submit a pull request there too, then!

@Ousret
Copy link
Author

Ousret commented Sep 13, 2024

I think you are agreeing that there is a performance problem?

There is, but the debate is way larger than this and I am not the expert on the broader field.

if it makes you happier - I'd encourage you to submit a pull request there too, then!

does this mean you are considering this PR for merge?

regards,

@Ousret Ousret force-pushed the feat-truststore branch 3 times, most recently from c678a20 to fa6fe8b Compare September 13, 2024 09:54
@dimbleby
Copy link
Contributor

I don't have the power to merge or reject this PR, I am just a humble contributor

Improving typeshed is desirable regardless of whether this is merged, if you've chanced across a gap that it would be helpful to fill then I'd encourage you to do that either way

When I ask about the performance impact I think you know that I am not looking for a debate about global warming. I am asking whether poetry users will notice a meaningful difference. uv's user-facing documentation says that this is something they think is significant, at least on macos - are they right? is it also significant in the poetry context?

(uv is faster than poetry in general, it's plausible that a performance penalty that is important to them is not important to poetry)

@Ousret
Copy link
Author

Ousret commented Sep 13, 2024

Improving typeshed is desirable regardless of whether this is merged

It's debatable, because requests maintainers don't approve the typing established in typeshed, for various valid reasons.

When I ask about the performance impact I think you know that I am not looking for a debate about global warming.

I understand, clearly, but myself was surprised with the question as this PR isn't concerned with that topic (perf).

I am asking whether poetry users will notice a meaningful difference.

None. As long as we still load certifi, which is the case for that PR, the performance will be the same.
If we were to stop from loading certifi, and solely use the in-memory store from the OS, a noticeable performance bump (expressed in hundred of ms) will be noticed. Mostly UX / feel reactive kind of things.

uv's user-facing documentation says that this is something they think is significant, at least on macos - are they right?

yes. I don't have the data in-hand, but my experience with Python natively accessing truststore through ext. func calls are slow. idk the factor.

uv is faster than poetry in general, it's plausible that a performance penalty that is important to them is not important to poetry

I don't know how to answer this last point.


Finally, let's wait for guidance / dismissal / approval from a maintainer with access before doing any further effort.

regards,

@Ousret Ousret force-pushed the feat-truststore branch 2 times, most recently from 4402c77 to fb3e6ac Compare October 16, 2024 07:23
This commit add support for loading the OS truststore root certificates in addition to the "legacy" certifi bundle. It will come in handy for every user behind a company proxy or just using a private PyPI warehouse.

Close #9249
@Ousret
Copy link
Author

Ousret commented Oct 16, 2024

gentle ping @Secrus

If this PR can't be accepted, feel free to close it.

Side note: I've added / injected the OS truststore (if available) in the dulwich client (if http transport of course).

@Ousret
Copy link
Author

Ousret commented Oct 28, 2024

Well. #9805
So I guess this means that this pr is dead.
Seems going smooth. Long live monkeypatching.

@Ousret Ousret closed this Oct 28, 2024
@Ousret Ousret deleted the feat-truststore branch October 30, 2024 15:51
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.

Add truststore support to use system certificate store
3 participants