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

System SSL certificates aren't used #1025

Closed
Kuba314 opened this issue Sep 3, 2022 · 13 comments · Fixed by #1029
Closed

System SSL certificates aren't used #1025

Kuba314 opened this issue Sep 3, 2022 · 13 comments · Fixed by #1029

Comments

@Kuba314
Copy link

Kuba314 commented Sep 3, 2022

My company uses its own certificate, which I have installed on my system. Cloning a repository from our company gitlab instance works with git, but not with dulwich.

Log of the following script is here.

from dulwich import porcelain

porcelain.clone("https://company.gitlab.instance.com/namespace/project.git", "test-repo")

Note: I've replaced the url, project name and project namespace with placeholder values.

I've found this issue through poetry. Here's the original issue: python-poetry/poetry#6340

@springheeledjack0
Copy link
Contributor

Off the top of my head there are two things you can try:

@jelmer
Copy link
Owner

jelmer commented Sep 3, 2022

where is the company's certificate installed? do you have custom config for it it in ~/.gitconfig ?

by default, dulwich just lets urllib3 call out to certifi to find system certificates

@Kuba314
Copy link
Author

Kuba314 commented Sep 3, 2022

@springheeledjack0

Install certifi

I have certifi installed with pip. Issue still persists.

Create your own pool manager

Are you saying that in order to use system certificates with dulwich, I have to modify the code? My use-case is to use this in poetry so that I can load a git dependency from a company repository. This solution doesn't work for me.

@jelmer In folder /etc/pki/tls/certs as a .pem file. It is also inside /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem. This is all pointed to by /etc/ssl/certs/ and /etc/ssl/cert.pem symlinks respectively.

do you have custom config for it it

I don't. Should I? I'm really not experienced with certificates and how to use them.

I don't have anything like http.sslCAinfo and http.sslVerify set. I set it and saw that both work as intended in git with curl:

GIT_TRACE_CURL=1 git clone https://company.gitlab.instance.com/namespace/project.git

And although I don't have anything set, curl assumes a default path of /etc/pki/tls/certs/ca-bundle.crt. certifi however contains only official CA certificates and I can't modify that store. How should I tell dulwich where to search for certificates? It doesn't appear to read ~/.gitconfig.

@jelmer
Copy link
Owner

jelmer commented Sep 3, 2022

Dulwich should work in situations where C Git works without further changes. It does support reading ~/.gitconfig (and I believe poetry actually triggers a read of config nowadays - see 879a144c32d61595928c9bd5a915c757a7592067) and supports both http.sslCAinfo and http.sslVerify. And it relies on urllib3 and certifi together to properly find system certificates where the user hasn't provided configuration.

Does python3 -c 'import certifi; print(certifi.where())' print the location you expect?

Do python3 -c "import urllib3; http = urllib3.PoolManager(); http.request('GET', 'https://www.company.com/')" and python3 -c "from dulwich.client import default_urllib3_manager; from dulwich.config import StackedConfig; http = default_urllib3_manager(StackedConfig.default()); http.request('GET', 'https://www.company.com/')" work?

@jelmer
Copy link
Owner

jelmer commented Sep 3, 2022

Ah, just discovered that "dulwich clone" doesn't actually load ~/.gitconfig; fix landing shortly in master.

@Kuba314
Copy link
Author

Kuba314 commented Sep 3, 2022

python3 -c 'import certifi; print(certifi.where())' prints a location of official certificates, not my custom one. I can't change this.

The first command works. The second one doesn't (CERTIFICATE_VERIFY_FAILED)

Ah, just discovered that "dulwich clone" doesn't actually load ~/.gitconfig; fix landing shortly in master.

Awesome, this should hopefully fix the issue.

@Kuba314
Copy link
Author

Kuba314 commented Sep 3, 2022

@jelmer The issue isn't just with clone() I think. It's probably in fetch() as well, and maybe in other APIs too.

@jelmer
Copy link
Owner

jelmer commented Sep 3, 2022

python3 -c 'import certifi; print(certifi.where())' prints a location of official certificates, not my custom one. I can't change this.

The first command works. The second one doesn't (CERTIFICATE_VERIFY_FAILED)

That unfortunately means that the core issue is caused by something deeper, in certifi. I'm reluctant to reimplement system certificate loading in Dulwich, that's really urllib3/certifi's job. :-/

Ah, just discovered that "dulwich clone" doesn't actually load ~/.gitconfig; fix landing shortly in master.

Awesome, this should hopefully fix the issue.

This fix doesn't directly help with this bug, it just makes testing easier. It means you can set http.sslCAInfo and have "dulwich clone" honor that setting. poetry doesn't use the "dulwich clone" porcelain (but lower level functions) and already passes in the config (as of 2.0.0b3).

@jelmer
Copy link
Owner

jelmer commented Sep 3, 2022

@jelmer The issue isn't just with clone() I think. It's probably in fetch() as well, and maybe in other APIs too.

dulwich.porcelain.fetch as well as other APIs like dulwich.porcelain.push already pass in the config.

@dimbleby
Copy link
Contributor

dimbleby commented Sep 4, 2022

Sounds like, at least in this case, dulwich using certifi has been counter-productive. The reporter seems to say that

  • urllib3 is successfully finding their certificate if left alone
  • certifi.where() does not find their certificate

so dulwich forcing urllib3 to use the certificates returned by certifi.where() is actually hurting rather than helping.

Difficult to know what else would break of course, but perhaps the fix is simply to remove this block and let urllib3 figure it out?

I'm reluctant to reimplement system certificate loading in Dulwich

removing that code seems like a step in the right direction so far as that goes, anyway!

@dimbleby
Copy link
Contributor

dimbleby commented Sep 4, 2022

Looking over your git history it seems that at the time the certifi stuff was introduced, urllib3 did not verify by default. But since 1.25 it does.

So maybe it actually is reasonable just to remove certifi from dulwich as a direct dependency, and instead install urllib3[secure] >= 1.25

@jelmer
Copy link
Owner

jelmer commented Sep 4, 2022

Sounds like, at least in this case, dulwich using certifi has been counter-productive. The reporter seems to say that

* `urllib3` is successfully finding their certificate if left alone

* `certifi.where()` does not find their certificate

so dulwich forcing urllib3 to use the certificates returned by certifi.where() is actually hurting rather than helping.

I had a look at the background to this, since I'd paged most of that out. We were using certifi because that's what upstream recommended:

48e2ef8
https://urllib3.readthedocs.io/en/latest/user-guide.html#certificate-verification

(otherwise, on older urllib3 versions you don't get any verification at all)

Things have moved on now though, and I agree with your suggestion - we can now just drop the call to certifi here and instead raise the minimum version of urllib3 to 1.25.

@dimbleby
Copy link
Contributor

dimbleby commented Sep 4, 2022

Don't want the [secure] extra apparently, it's deprecated - urllib3/urllib3#2680

jelmer added a commit that referenced this issue Sep 4, 2022
This seems to be having adverse affects where we specify an incorrect path in some cases. Fixes #1025

Increment minimum urllib3 to >= 1.25 just to be sure; not strictly necessary -
we're already trying to be explicit about whether we want verification.

This was originally introduced in 48e2ef8, but
urllib3 now verifies certificates by default.
See also https://urllib3.readthedocs.io/en/latest/user-guide.html#certificate-verification

thanks to David Hotham for help with the archeology on this one :)
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 a pull request may close this issue.

4 participants