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

[feature] Add pip-system-certs to python dependencies #3123

Open
1 task done
daltonv opened this issue Mar 22, 2023 · 13 comments
Open
1 task done

[feature] Add pip-system-certs to python dependencies #3123

daltonv opened this issue Mar 22, 2023 · 13 comments
Assignees
Milestone

Comments

@daltonv
Copy link

daltonv commented Mar 22, 2023

What is your suggestion?

As conan seems to be widely used in corporate networks it seems like it would be a good idea to add pip-system-certs to the dependencies.

This should help anybody who has corporate self-signed certificates due to ssl inspection policies, as it forces python requests to use the system certificate store as well as the certifi store. The self signed cert aught to be on the system store, so no extra work is needed by the dev to get conan install to work.

Have you read the CONTRIBUTING guide?

  • I've read the CONTRIBUTING guide
@memsharded
Copy link
Member

Hi @daltonv

Thanks for the suggestion.
However, I am not fully sure about the problem that is being addressed here. The Conan conf for adding certificates seems to have been working reliably for users, and also, on the other hand, it seems that https://gitlab.com/alelec/pip-system-certs has only 11 stars on gitlab. This is not an absolute stopper, but we typically look for way more popular and mature libraries to use them as Conan dependencies, because we have sufferered that in the past. So the pain has to be larger and the library more mature to consider this addition.

@daltonv
Copy link
Author

daltonv commented Mar 22, 2023

Hi @memsharded,

Thanks for the quick response.

Is the conan conf for adding certs setting the core.net.http:cacert_path? Does that just replace the cacert_path and I need to be responsible for bundling common certs like from certifi in my chosen path?

My intention is that ideally you don't have to do anything extra if you have some self-signed cert in your system store, although I definitely appreciate not wanting to adding a less mature dependency. So maybe this dep isn't the ideal solution, but it would be nice to have a seamless solution. Maybe that's just a larger problem with how Python itself handles certs.

@memsharded
Copy link
Member

We are basically passing that cacert_path conf to requests, to the verify arg:

You can pass verify the path to a CA_BUNDLE file or directory with certificates of trusted CAs:

requests.get('https://github.com', verify='/path/to/certfile')

So yes, it seems the common practice is to have a directory with the desired certs, and point the cacert_path to it. In these cases we always do what the dependency does, and requests is massively used, if that is the way to define it, Conan will typically just follow it.

@daltonv
Copy link
Author

daltonv commented Mar 22, 2023

I also see the requests themselves seem to be considering this option as well. psf/requests#2966

Pip is also experimenting with it https://pip.pypa.io/en/stable/topics/https-certificates/#using-system-certificate-stores

So maybe I really want a deeper solution, and requests aught to change and conan should just follow them.

I would suggest conan should at least have some documentation specifically talking about self-signed certs. Dealing with them is not intuitive if you don't deal with certs everyday. Just a simple FAQ about getting self-signed cert issues when calling conan install and how to use conan config to deal with it.

@memsharded
Copy link
Member

Sounds reasonable, lets move this ticket to docs repo, and lets watch what python and requests will be doing with it.

@memsharded memsharded transferred this issue from conan-io/conan Mar 22, 2023
@memsharded memsharded added this to the 2 milestone Mar 22, 2023
@puetzk
Copy link
Contributor

puetzk commented Jan 5, 2024

Looks like pip settled on implementing a --use-feature=truststore starting from 22.2.

So maybe conan could/should grow a core.net.http:truststore=true conf option, or perhaps special-case core.net.http:cacert_path=truststore as a "magic" path for referring to the OS one, or something like that.

@puetzk
Copy link
Contributor

puetzk commented Jan 5, 2024

Also some interesting discussion (from the author of pip-system-certs), about how truststore.inject_into_ssl compares to his current implementation at sethmlarson/truststore#75 (comment)

definitely sounds like he thinks they did it better, and the remaining difference is just whether it's done by the app or via a .pth hook for the whole python installation/venv. Which is moot if it was going to be something controlled by the app (e.g. pip's feature flag or a similar conf for conan).

@memsharded
Copy link
Member

This seems interesting.
psf/requests#2966 (comment) and truststore itself describe how it can be used to inject into ssl, so requests doesn't really need any modification to use it.

There could be some problems, like for example the library only works with Python 3.10 and certain system apis, which means it can be almost impossible to make it available by default in Conan, so there should be some kind of opt-in.

@puetzk
Copy link
Contributor

puetzk commented Jan 8, 2024

Yeah, you'd just make it a conditional requirement in requirements.txt : truststore >= 0.6; python_version >= '3.10'

Then gate the import truststore + truststore.inject_into_ssl(), behind something in global.conf. Maybe duplicate the version check to report a nicer "truststure requires python 3.10" error message if someone tries to turn it on but lacks the prerequisites (or just let the import fail...)

@puetzk
Copy link
Contributor

puetzk commented Jan 8, 2024

Nevermind, truststore itself already does that with a custom ImportError: https://github.com/sethmlarson/truststore/blob/178fea29c3fb5b14a62deeb112ed3e3d81e95157/src/truststore/__init__.py#L5-L6

@puetzk
Copy link
Contributor

puetzk commented Jan 8, 2024

The dependency on >= 3.10 is (unfortunately) quite real: sethmlarson/truststore#107 (comment), that's when the ssl module was extended with the APIs truststore can use to hook itself in.

@memsharded
Copy link
Member

Yeah, you'd just make it a conditional requirement in requirements.txt : truststore >= 0.6; python_version >= '3.10'

I am still a bit concerned about adding this kind of dependency to Conan for every Conan users with Python 3.10, that could have issues with system requirements for example, so I'd be more inclined to make it opt-in initially protecting the import in Conan codebase only when enabled or something like that. It is still early stage, the repo has just 125 stars, still depends too much on a single person, etc, and we have had painful experiences with this kind of dependency in the past.

@puetzk
Copy link
Contributor

puetzk commented Jan 8, 2024

Fair. I was looking at adoption within cpython and pypa, more so than github stars. pip >= 23.3 has it always-installed (though not automatically imported/used). Though pip of course has vendored it rather than listed in requirements.txt, since pip itself can't rely on getting things via pip. And this version of pip which bundles a vendored truststore is what's being built for the now in the cpython 3.11 and 3.12 branches. Though this has only happened in the last few months, of course, but "truststore being included in ensurepip and thus in the default installation of cpython" seems like a pretty firm vote of "what python and requests will be doing".

Pip hasn't make --use-feature=truststore the default yet, though I think that's coming eventually; see pypa/pip#11647. My guess is that won't happen until after python 3.9 goes out of support next year, since a pip default that didn't work on all supported versions of python like a thing they'd avoid.

So I was leaning toward that same practice - installed by default (to avoid the chicken-and-egg problem that you can't install it because your SSL settings don't work without without it), but keeping actual use opt-in for now.

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

No branches or pull requests

3 participants