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

Don't pass --cert to build subprocesses unless also given on CLI #13195

Merged
merged 1 commit into from
Feb 1, 2025

Conversation

ichard26
Copy link
Member

This fixes a regression introduced by commit 34fc0e2. After that patch, --cert would always be given to the nested pip call, either pointing to pip's CA bundle, or to whatever the user had set on the CLI. This means truststore is always disabled... which is bad.

We used to have to do some shenanigans to pass the CA bundle to the subprocess as certifi doesn't (didn't?) really play nice when in a zipfile. Regardless, we stopped packing pip into a zipfile to provision the build environment a while ago, so we can simply do the normal thing and pass --cert when it's actually given. Otherwise, the subprocess will find its CA bundle without fuss.

There apparently aren't any truststore tests (as testing system CAs is probably a pain), so I didn't add one here either. At some point, we should, though.

Fixes #13186.

This fixes a regression introduced by commit 34fc0e2. After
that patch, --cert would always be given to the nested pip call, either
pointing to pip's CA bundle, or to whatever the user had set on the CLI.
This means truststore is always disabled... which is bad.

We used to have to do some shenanigans to pass the CA bundle to the
subprocess as certifi doesn't (didn't?) really play nice when in a
zipfile. Regardless, we stopped packing pip into a zipfile to provision
the build environment a while ago, so we can simply do the normal thing
and pass --cert when it's actually given. Otherwise, the subprocess will
find its CA bundle without fuss.

There apparently aren't any truststore tests (as testing system CAs is
probably a pain), so I didn't add one here either. At some point, we
should, though.
@ichard26
Copy link
Member Author

I'll note that we used to not pass any certificates to the subprocess1 before we used zipped pip. That's obviously not great if the user had specified --cert, but it does seem to confirm this is sound.

Footnotes

  1. This is one of the commits right before https://github.com/pypa/pip/pull/9689 was landed.

@notatallshaw
Copy link
Member

Thanks for the history of this.

@sbidoul sbidoul merged commit 028c087 into pypa:main Feb 1, 2025
31 checks passed
@ichard26 ichard26 deleted the truststore-hotfix branch February 1, 2025 15:56
@sbidoul
Copy link
Member

sbidoul commented Feb 2, 2025

I briefly tried to reproduce with 25.0 by mostly emptying the certifi cert file and attempting a no binary install but could not succeed.

So this fix looks good to me but I don't quite understand it fully, and that worries me a bit.

@notatallshaw
Copy link
Member

notatallshaw commented Feb 2, 2025

I realized that my work Windows environment relies on truststore, an environment I don't often use, so I tried to reproduce myself.

I also can't reproduce, so I was looking carefully at #11647, and I find some things confusing:

  • I can only get a network error if I pass in --use-deprecated=legacy-certs, truststore certificates get loaded unconditionally otherwise
  • I find the line ctx.load_verify_locations(certifi.where()) confusing, shouldn't it be doing what @ichard26 did in passing the build certificates and be doing finder.custom_cert or where()? If a user has custom certificates they need to pass in that aren't in certifi or truststore, how do they get loaded without using --use-deprecated=legacy-certs?

In general this is making me think that:

  1. In general truststore will still work in the build environment in 25.0, which is why we've had so few reports
  2. There are possibly some very specific edge cases the current code doesn't take into account

I'm going to go back to #13186 and ask the user to run some specific commands

@ichard26
Copy link
Member Author

ichard26 commented Feb 2, 2025

@sbidoul I'm pretty sure requests doesn't use the SSLContext's CA bundle when session.verify is set to a string (which is the path to the custom bundle from --cert). Otherwise, it defaults to True, using whatever SSLContext is attached. With truststore enabled, that is the truststore context. If truststore is disabled, then requests' own SSLContext is used (with only certifi loaded).

We set session.verify here:

# Handle custom ca-bundles from the user
if options.cert:
session.verify = options.cert

@notatallshaw
Copy link
Member

notatallshaw commented Feb 6, 2025

Based on the discussion the updates in #13186 I think there is an issue (but not regression) here that extends outside the build steps.

Specifically there is some edge case to passing in session.verify = {cert_path} when those certs fail to verify but truststore succeeds in verifying. I'm not sure exactly what the edge case is though as I can't reproduce it (perhaps the same certificate authiority exists in both locations but the one in the path is expired?).

My guess is the broader solution is:

  1. When passed a certificate path truststore should extend the SSL context from that not from certifi
  2. When truststore is used pip should set session.verify = True not session.verify = options.cert (or nothing, I assume True is default?)

@sethmlarson do you have any input here?

@sethmlarson
Copy link
Contributor

@notatallshaw Thanks for the tag-in! I'm in agreement that when passed a certificate path then Truststore should use that path and not use certifi, that matches pip's behavior prior to "Truststore by default" where --cert would replace certifi completely.

I think that always using system certificates and then swapping out whether certifi or /something else/ is used as "additional" certificates the way to go. Longer-term phasing out certifi as a default set of additional certificates would be nice, too. Then there's no need to treat certifi security issues as pip issues.

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

Successfully merging this pull request may close these issues.

Pip 25.0 regression: truststore is not used for installing build dependencies
4 participants