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

Provide flag for using "load default certs" from Python's standard library for the SSL Context #11038

Closed
1 task done
notatallshaw opened this issue Apr 14, 2022 · 10 comments
Closed
1 task done
Labels
type: feature request Request for a new feature

Comments

@notatallshaw
Copy link
Member

notatallshaw commented Apr 14, 2022

What's the problem this feature will solve?

As discussed in #10777 and #10961 .

In practice it will let Pip use the platform's certificate store. There is at least one edge case on Windows where this might not work as simply as that but idea is flag will defer entirely to the documentation in the standard library: https://docs.python.org/3/library/ssl.html#ssl.SSLContext.load_default_certs

Describe the solution you'd like

Optional flag to allow load default certs for Pip's SSLContext.

Alternative Solutions

Don't provide

Additional context

Allows some users to use this instead of "trusted_host" over HTTP, or trying to manually extract their certificate store in to a file (which is not always possible on Windows).

Code of Conduct

@notatallshaw notatallshaw added S: needs triage Issues/PRs that need to be triaged type: feature request Request for a new feature labels Apr 14, 2022
@uranusjr uranusjr added state: awaiting PR Feature discussed, PR is needed and removed S: needs triage Issues/PRs that need to be triaged labels Apr 15, 2022
@pfmoore
Copy link
Member

pfmoore commented Apr 15, 2022

Given this comment which clarifies the background (at least to me!) this seems reasonable. However, I worry that people won't understand the subtle difference between "use system certs" and "load default store". I don't think it's reasonable to expect pip users to understand the details of the stdlib SSL module (I certainly don't!) and even if they did, I see no mention in the stdlib docs of the possible cases mentioned in the linked thread where this flag wouldn't work as expected.

All of which means, I'd like the docs for this feature to be very clear on what it does and doesn't promise. And that's not just for users, it's for future maintainers trying to understand whether a claimed issue is or is not a bug.

@notatallshaw
Copy link
Member Author

notatallshaw commented Apr 15, 2022

However, I worry that people won't understand the subtle difference between "use system certs" and "load default store". I don't think it's reasonable to expect pip users to understand the details of the stdlib SSL module (I certainly don't!) and even if they did,

I don't disagree this is nuanced, but such is the case when dealing with SSL options. Even in requests exposed arguments there is a lot of nuance around verify and cert but they still need to be exposed to let users who have those needs perform those actions.

I see no mention in the stdlib docs of the possible cases mentioned in the linked thread where this flag wouldn't work as expected.

This is the relevant quote in the stdlib docs:

On Windows it loads CA certs from the CA and ROOT system stores.

And to your final point this is essentially all it is promising, that what ever happens to be in the CA and ROOT system stores (at least on Windows) is what is used to verify the HTTPS request to the Python index.

I think the main point the docs in Pip would need to explain is it's only https requests Pip is making (just like the other SSL options pip provides), and the purpose is to "Load a set of default CA certificates from default locations" as per the stdlib docs: https://docs.python.org/3/library/ssl.html#ssl.SSLContext.load_default_certs

Finally this would help support any user on Windows where their organization stores their own CA locally and all HTTPS connections are validated against that CA. Which my understanding is quite a lot of organizations do this these days but it's hard to get a grasp on numbers.

It does not help users where the CA being used to validate the HTTPS call is only used by Pip. This would be the edge case that is not supported by this standard library method. And users wishing this must develop a workaround where the CA is loaded in to the system store first.

p.s. I am currently negotiating with my new employer the remit of my ability to submit PRs to open source projects, once I get a chance I will be happy to submit a PR. But don't let that stop anyone else sufficiently motivated 🙂

@sethmlarson
Copy link
Contributor

@davisagli and I created truststore so that Python can verify certificates using native operating system APIs. This library could be used as a potential solution for this issue?

We use OpenSSL for the handshake but then pass the peer certificate chain to OS cert verification APIs like Security framework on macOS and CryptoAPI on Windows. The peer certificate chain is grabbed from experimental APIs in the ssl module but after speaking to @tiran the APIs will be in their same state in Python 3.11.

The library currently only supports Python 3.10+ so would need to be optional for now. The library also supports loading additional CA certificates into the SSLContext. To ensure that no certificate chains that previously verified using only certifi would stop verifying (although I believe this situation would be rare) the SSLContext could be loaded with certifi certs in addition to using the system store, if desired.

@notatallshaw
Copy link
Member Author

notatallshaw commented May 2, 2022

@sethmlarson this looks excellent! And is actually more akin to #10961 than the current issue (subtly the difference between the two issue is that was asking for the system certificate store but there is an edge case to using load_default_certs that on Windows missing intermediate certificates are not downloaded automatically using that method where as this issue says just provide an option to use load_default_certs and let the users handle the behavior with no guarantees from Pip) .

Though with that said my main concern is that Pip actually isn't the right place to adopt this. It would mean vendoring the library truststore and for Pip it increases the number of libraries it has to trust are secure.

Wouldn't it make more sense to have popular HTTP backends like urllib3 and httpcore adopt this?

Prior art already exists of using oscryptos Secure Transport for urllib3: https://urllib3.readthedocs.io/en/latest/reference/contrib/securetransport.html . Perhaps similar or even tighter integration with truststore could happen?

@uranusjr
Copy link
Member

uranusjr commented May 2, 2022

Wouldn't it make more sense to have popular HTTP backends like urllib3 and httpcore adopt this?

Seth maintains urllib3 so I assume at least one of these will happen at some point :p

I believe the intention of bringing this into pip before any popular library is that pip is the perfect real-world testing ground. With --use-feature we are at the position to make big-corp-network people actually use this and weed out potential issues. This should lead to a smoother transition when OSS maintainers (who are generally less experienced with this sort of setups) start to adopt truststore.

@notatallshaw
Copy link
Member Author

Oh super! I didn't realize, sorry for the comment with not enough understanding of who was providing this info.

@sethmlarson
Copy link
Contributor

sethmlarson commented May 2, 2022

@uranusjr Yes, exactly! Introducing into an HTTP library would either be an abrupt change if made required or likely will be unused if hidden behind a flag. Best to start in a place that already has a concept of feature flags and has a group of users with issues that truststore solves.

To give a bit of status where we are with the project: the basic functionality works but will likely have some edge cases that our relatively small test suite hasn't covered. We're working to test how the library behaves in more situations and implement+document how each operating system API interacts with corresponding OpenSSL-esque ssl.SSLContext features.

@uranusjr
Copy link
Member

Should we keep this open, or does supporting truststore cover the usage?

@uranusjr uranusjr added S: awaiting response Waiting for a response/more information and removed state: awaiting PR Feature discussed, PR is needed labels May 30, 2022
@notatallshaw
Copy link
Member Author

As I understand it from the blog post supporting truststore covers this.

The only issue is that you need to boostrap truststore in to the Python environment first (may require using trusted host arg to install). Hopefully this will eventually not be required.

@uranusjr
Copy link
Member

Yeah we’re going to vendor truststore at some point, when it’s more stable.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 30, 2022
@pradyunsg pradyunsg removed the S: awaiting response Waiting for a response/more information label Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: feature request Request for a new feature
Projects
None yet
Development

No branches or pull requests

5 participants