pip icon indicating copy to clipboard operation
pip copied to clipboard

Truststore by default

Open sethmlarson opened this issue 2 years ago • 18 comments

Follow up to https://github.com/pypa/pip/pull/11082 which makes truststore w/ certifi the default behavior on Python 3.10+. This behavior can be opted-out of in favor of old behavior with --use-feature=no-truststore.

TODO:

  • [x] Add a NEWS entry
  • [x] Update documentation with the new situation and feature
  • [ ] Add more tests? Unsure how I can test that truststore isn't used when no-truststore is specified.

cc @davisagli @pradyunsg @uranusjr

sethmlarson avatar Dec 08 '22 04:12 sethmlarson

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

pypa-bot avatar Apr 01 '23 18:04 pypa-bot

@uranusjr Thanks for the review, just to double-check, I didn't add any new tests but because this is the new default behavior I figured this would be okay? Let me know if there is any work to be done outside this PR to prepare for this change (I'll likely write a blog post, but happy to help elsewhere too!)

sethmlarson avatar Apr 07 '23 19:04 sethmlarson

I imagine it may be difficult to test this in any case since the feature relies on system configuration by nature. I’m tentitively adding this to 23.1 but the RM has a final say.

uranusjr avatar Apr 10 '23 08:04 uranusjr

This doesn't seem to be following our normal transition policy. In particular, our documentation notes that legacy features enabled by a --use-legacy flag "should always include a warning that indicates when the feature is scheduled to be removed". And there isn't one here. (Also, as this is only enabled when Python 3.10 is available, "legacy certificates" won't get removed until we desupport Python 3.9, so it's hardly "legacy" in the sense we normally use the term...)

Furthermore, users who will see a change in behaviour when this gets enabled will have seen no advance warning that it's coming, and will have had no indication that they may need to test or make changes to their certificate handling. My impression is that this is intended to simply add extra ways that a certificate could be treated as valid, so it shouldn't break anything that works right now, but how sure are we of that? People do crazy things...

I suspect very few people will have tried the new feature yet, as it requires installing a support module and enabling an opt-in flag. So I'd consider the new functionality mostly "untried" at this point. I know it's hard to get people to test new stuff, but can we not at least do something to publicise the change in advance?

Sorry, but I'm going to reject this for 23.1, on the basis that the transition process needs a bit more work, and I don't want that to be rushed just to get the feature into 23.1.

I'm open to arguments to reconsider, but they'll need to be based around explanations of how the current approach is safer than I think it is, not around how we can improve the PR (which will be a fine approach, but not for 23.1)

pfmoore avatar Apr 10 '23 10:04 pfmoore

Thanks for your review and detailed thought process @pfmoore! I agree on your reasoning for not including this in 23.1. I had a few questions on how best to proceed:

I suspect very few people will have tried the new feature yet, as it requires installing a support module and enabling an opt-in flag. So I'd consider the new functionality mostly "untried" at this point. I know it's hard to get people to test new stuff, but can we not at least do something to publicise the change in advance?

Agreed, I'm thinking for 23.1 we could:

  • Vendor truststore 0.7.0 as we've proposed here to avoid the install
  • Start emitting a warning asking for opt-in for Python 3.10+?

Then we can start getting a lot more feedback and hopefully be more confident with this change being purely additive. Do you agree with this approach, and if so I can rapidly create a PR to meet 23.1?

Also, as this is only enabled when Python 3.10 is available, "legacy certificates" won't get removed until we desupport Python 3.9, so it's hardly "legacy" in the sense we normally use the term...

Since this change should be additive for Python 3.10+ compared to Python 3.9 and earlier, do you see it being necessary to have Python 3.9 support removed before Python 3.10+ would always have this new behavior? In my mind, Python 3.9 and earlier wouldn't be dependent on this change moving forward.

sethmlarson avatar Apr 10 '23 15:04 sethmlarson

I'm not particularly familiar with the use cases involved here, so I'm not the best person to comment - hopefully @uranusjr can help out.

One question I do have - is truststore now considered stable? In other words, are we safe to vendor it on the assumption that any issues (which have the potential to be security issues, I guess) will be handled via our usual "please report to the vendored project, we'll wait for them to issue a fix and then pick that up in our routine revendoring at our next scheduled release"? I ask because #11082 deliberately didn't vendor for this reason, and the release cadence still seems pretty fast to me.

I don't like the idea of an "always on" warning nagging people to use a new feature that may have no value to them. At least I assume that for a lot of people it will have no value - I've personally never had a problem because pip uses certifi rather than the system store. What are the target users for this feature, in fact? Presumably it's groups with custom indexes whose certificate is in the system store (managed corporately somehow) who are currently using a dedicated certificate file and who could drop it if they used the system store? So they have a bit of management overhead that they'd be glad to get rid of?

Conversely, what are the potential failure cases here? How could switching to use the system store cause something to fail that is currently working? Presumably one case would be "certificate expires and corporate are slow updating the system store"? If it's not obvious, I'm not a security specialist, so if these are dumb scenarios, feel free to ignore them and tell me what would be good ones. My concern here is people screaming "you broke my build pipeline" because there was a behaviour change that we hadn't told them about. Those people are typically the same ones who claim that adding an opt-out flag to all of their jobs isn't practical[^1].

Agreed, I'm thinking for 23.1 we could:

  • Vendor truststore 0.7.0 as we've proposed here to avoid the install
  • Start emitting a warning asking for opt-in for Python 3.10+?

I'll be honest, given that 23.1 is due at the end of this week, this still feels too much like trying to work out a solution under time pressure to me. I'm still inclined to say let's do nothing in 23.1 and get a proper plan in place after 23.1, so that it's ready to go in good time for 23.2 (and 23.3, if it needs a phased approach, as I feel it probably does).

[^1]: I'm not sure I believe the situation is as dire as it's often painted, but I'm not willing to fight that battle over this change in particular...

pfmoore avatar Apr 10 '23 16:04 pfmoore

As there has been no further comments on this, I'm moving it to the 23.2 milestone.

pfmoore avatar Apr 15 '23 09:04 pfmoore

There's been nothing further on this, and 23.2 is probably under a month away. @sethmlarson do you have any thoughts on how to proceed, or should we consider moving this to 23.3? I'm not (yet...) the RM for 23.3, but I'd be concerned about having this land at the last minute, just before a release. I know we don't do formal betas, but I still think it's better to get large features in earlier in the release cycle rather than later.

pfmoore avatar Jun 25 '23 12:06 pfmoore

@pfmoore Hey Paul, I agree that trying to get this PR in in it's current state wouldn't be a good idea. Apologies for not getting back to you sooner on this one.

One question I do have - is truststore now considered stable? In other words, are we safe to vendor it on the assumption that any issues (which have the potential to be security issues, I guess) will be handled via our usual "please report to the vendored project, we'll wait for them to issue a fix and then pick that up in our routine revendoring at our next scheduled release"? I ask because https://github.com/pypa/pip/pull/11082 deliberately didn't vendor for this reason, and the release cadence still seems pretty fast to me.

I consider it so, the core functionality (and everything that pip would need) is working and we've received no complaints so far with modest usage. I can throw together a PR that only vendors the project and removes the installation instruction from the documentation if you're still okay with this approach.

Conversely, what are the potential failure cases here? How could switching to use the system store cause something to fail that is currently working?

In the general case switching from "certifi is trust store" to "system trust store with certifi supplementing" shouldn't have an impact on folks since the same root to leaf chains should be . I'm sure there are ways that users could get in trouble, like if the system trust store could be configured to avoid supplemental CAs or something like that but I'm not familiar with all the configuration options available on Windows and macOS to know what's possible here.

What are the target users for this feature, in fact?

In addition to all the benefits of an OS trust store over a static trust store, this change is part of an effort to put Python more in line with other ecosystems where the system trust store is used instead of a trust store that's distributed+updated+managed "out-of-band" compared to all other software on the system.

From the PEP 543 rationale on "why system trust stores":

Relying on certifi is less than ideal, as most system administrators do not expect to receive security-critical software updates from PyPI. Additionally, it is not easy to extend the certifi trust bundle to include custom roots, or to centrally manage trust using the certifi model.

Even in situations where the system certificate stores are made available to OpenSSL in some form, the experience is still sub-standard, as OpenSSL will perform different validation checks than the platform-native TLS implementation. This can lead to users experiencing different behaviour on their browsers or other platform-native tools than they experience in Python, with little or no recourse to resolve the problem.

sethmlarson avatar Jun 27 '23 02:06 sethmlarson

Created the new PR with the vendoring here: https://github.com/pypa/pip/pull/12107

sethmlarson avatar Jun 27 '23 03:06 sethmlarson

So with #12107, the vendoring can be ripped out of this PR, leaving it being just the implementation of the transition process for making truststore the default, correct? That seems like a good approach to me.

See my comment on the vendoring PR for discussion of the timing. But basically, I think we should first get this ready for release, with no target release date in mind, and then, once it's ready, decide which actual release(s) it will target. That way, we avoid the current situation of "someone reminds you that a release is coming up, flurry of (rushed) activity, we fail to make the release, everything goes quiet again, cycle repeats".

With that in mind, I'm removing the 23.2 milestone. It can always be added back if we change our minds.

pfmoore avatar Jun 27 '23 09:06 pfmoore

Checking in on this one. Since #12107 is now released I don't have to do crazy hacks to support our corporate environment, but this is the last piece to making pip "just work."

EmperorArthur avatar Apr 27 '24 19:04 EmperorArthur

Given the outstanding conflicts here and the fact that there's agreement that this PR's current state isn't sufficient to merge (based on https://github.com/pypa/pip/pull/11647#issuecomment-1608646075), I am not comfortable including this in 24.1.

pradyunsg avatar May 05 '24 00:05 pradyunsg

Given the outstanding conflicts here and the fact that there's agreement that this PR's current state isn't sufficient to merge (based on #11647 (comment)), I am not comfortable including this in 24.1.

I don't understand. If it's not stable and there are conflicts, then why was it vendored in 23.3? At this point, the only thing that should be remaining is a wording change and adjusting the feature to be enabled by default.

EmperorArthur avatar May 06 '24 16:05 EmperorArthur

@EmperorArthur There was a conflict during the upgrade from 0.8.0 to 0.9.0 that's unrelated to truststore's fitness (was caused by an inconsistency in the CPython certificate APIs in 3.13). It's still the goal to get this landed, but it requires fixing the found issue, upgrading pip, and then testing which might not fit the timeline for 24.1.

sethmlarson avatar May 06 '24 16:05 sethmlarson

Thanks for the reviews and all the assistance @uranusjr and @pfmoore! @davisagli and I are quite excited to see our work from two years ago land in pip :pray:

sethmlarson avatar May 20 '24 21:05 sethmlarson

Would it be worth it to still document the experimental/built-in-but-disabled support older versions of pip had? It has led to user confusion (https://github.com/pypa/pip/issues/12652) and this could contribute to it. I suppose flipping the default to on would eventually lessen this problem.

ichard26 avatar May 20 '24 21:05 ichard26

@sethmlarson the 24.2 development period has started. Probably good time to update this PR (at least to pull in the CI fixes).

ichard26 avatar Jun 27 '24 18:06 ichard26

(kicking CI... fingers crossed)

ichard26 avatar Jul 06 '24 03:07 ichard26

Windows CI is failing due to flaky tests I introduced earlier. They should be fixed once #12839 is merged. Sorry about the trouble!

ichard26 avatar Jul 12 '24 17:07 ichard26

Oh fun. It obviously doesn't sign commits made on the web UI for others.

pradyunsg avatar Jul 13 '24 21:07 pradyunsg

@pradyunsg Looks like the rebase has a passing test suite, ready to merge whenever :)

sethmlarson avatar Jul 15 '24 13:07 sethmlarson

Thank you for the patience, and congratulations on the merge @sethmlarson. :tada:

ichard26 avatar Jul 20 '24 02:07 ichard26