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

certifi runtime hook interferes with ssl.load_default_certs() #332

Closed
LincolnPuzey opened this issue Oct 13, 2021 · 8 comments
Closed

certifi runtime hook interferes with ssl.load_default_certs() #332

LincolnPuzey opened this issue Oct 13, 2021 · 8 comments
Labels
bug Something isn't working

Comments

@LincolnPuzey
Copy link
Contributor

LincolnPuzey commented Oct 13, 2021

Describe the bug
The certifi runtime hook which was added in pyinstaller/pyinstaller#3952 sets the SSL_CERT_FILE environment variable to the path to certifi's cacert.pem.

This causes all the certificates in cacert.pem to be loaded by ssl.load_default_certs() and therefore to be included in the SSLContext created by ssl.create_default_context()

To Reproduce

My script print_certs.py

import ssl

import requests
import certifi

print(certifi.where())
context = ssl.create_default_context()
print(context.cert_store_stats())

output when running from source in venv with requests/certifi installed

PS C:\Users\Lincoln\cert_test> .\.venv\Scripts\python .\print_certs.py
C:\Users\Lincoln\cert_test\.venv\lib\site-packages\certifi\cacert.pem
{'x509': 27, 'crl': 0, 'x509_ca': 25}

Output when bundled with pyinstaller (all default options)

PS C:\Users\Lincoln\cert_test> .\dist\print_certs\print_certs.exe
C:\Users\Lincoln\cert_test\dist\print_certs\certifi\cacert.pem
{'x509': 148, 'crl': 0, 'x509_ca': 146}

Expected behavior
I would expect the output of cert_store_stats() for bundled to be the same as non-bundled

Desktop:

  • OS: Windows
  • Python Version: 3.7.9
  • Version of pyinstaller-hooks-contrib: 2021.3
  • Version of PyInstaller 4.5.1

Proposed Solution
I think the certifi runtime hook should be removed.

@LincolnPuzey LincolnPuzey added the state:triage We're still figuring out how severe this issue is label Oct 13, 2021
@bwoodsend
Copy link
Member

bwoodsend commented Oct 13, 2021

I don't know anything about certifi so I can't say much other than that I would also expect PyInstaller's behaviour to mirror the unfrozen behaviour. I'd really like to know what the rational behind pyinstaller/pyinstaller#3952 was though before I start deleting hooks. @ktong Are you still on Github? Can you comment?

@LincolnPuzey
Copy link
Contributor Author

certifi provides root certificates in the form of the cacert.pem file. Quoting the certifi README;

Certifi provides Mozilla's carefully curated collection of Root Certificates

The key word there is provides. It is up to the rest of you application to choose to use certifi's certificates or not.

For example, the requests library explicitly chooses to use certifi's certificates.


Running my script from source shows the correct behavior, which is that even though certifi is installed, ssl.create_default_context() returns a SSLContext only with the certificates from my operating system (Windows)

Running from bundled shows different/incorrect behavior - the SSLContext has many more loaded certificates because the ones from cacert.pem are being included.


htgoebel's first comment on the original PR pyinstaller/pyinstaller#3952 (comment) was correct.

@rokm
Copy link
Member

rokm commented Oct 14, 2021

https://github.com/pyinstaller/pyinstaller/wiki/Recipe-OpenSSL-Certificate also has a mention of the behavior change that pyinstaller/pyinstaller#3952 introduced.

But I agree that this should be opt-in, not opt-out, to keep behavior consistent with unfrozen version.

@bwoodsend bwoodsend added bug Something isn't working and removed state:triage We're still figuring out how severe this issue is labels Oct 18, 2021
@bwoodsend
Copy link
Member

Ok, looks like we're not going to get an answer from pyinstaller/pyinstaller#3952's author. @LincolnPuzey Go ahead and delete that runtime hook then.

@LincolnPuzey
Copy link
Contributor Author

I think the hook should be removed mainly because it gives inconsistent behavior between frozen/unfrozen.

However removing the hook will cause frozen applications to behave inconsistently across different computers when the computers have different certificates available from the Operating System.

Personally, my frozen application is distributed across Windows machines I have no control of and no knowledge of what certificates are available from Windows, so I will be adding code to my application to add back this behavior since it is essential for reliably verifying the domain my application connects to.

So to be clear, removing this hook is a breaking change, if your application relies on the certificates in certifi being included in:

  • an SSLContext which has had SSLContext.set_default_verify_paths() called on it. (I'm pretty sure this is the point that actually looks at the SSL_CERT_FILE environment variable.)
  • an SSLContext which has had SSLContext.load_default_certs() called on it. (This method calls set_default_verify_paths())
  • an SSLContext created from ssl.create_default_context(). (This function calls load_default_certs())

Unfortunately, it is likely that there are developers that don't know their application is currently relying on this behavior.

A trap developers might fall into is if they do the following

  1. update pyinstaller-hooks-contrib to get this change
  2. non-frozen unit tests pass (because this change won't effect non-frozen code)
  3. frozen application works fine on dev machine - because dev machine happens to have the required certs in the OS
  4. frozen application is distributed - starts to fail on machines that don't have the required certs in the OS

LincolnPuzey added a commit to LincolnPuzey/pyinstaller-hooks-contrib that referenced this issue Oct 19, 2021
This hook is being deleted because it is not required for certifi to function and it introduces inconsistent behavior between frozen/unfrozen code.
@bwoodsend
Copy link
Member

Surely though, the correct way to give ssl the certificates from certifi (without PyInstaller) is to give it the certificates file directly?

context = ssl.create_default_context(cafile=certifi.where())

In which case, relying on our runtime hook's meddling is really broken code so removing this hook is only breaking in the sense that its not bug for bug backwards compatible.

@LincolnPuzey
Copy link
Contributor Author

I will be adding code to my application to add back this behavior

Yes, I meant I'll be doing something like what you suggested (I won't be setting SSL_CERT_FILE)

@LincolnPuzey
Copy link
Contributor Author

For reference here are the OpenSSL docs describing the SSL_CERT_FILE environment variable:

LincolnPuzey added a commit to LincolnPuzey/pyinstaller-hooks-contrib that referenced this issue Oct 25, 2021
This hook is being deleted because it is not required for certifi to function and it introduces inconsistent behavior between frozen/unfrozen code.
bwoodsend pushed a commit that referenced this issue Oct 25, 2021
This hook is being deleted because it is not required for certifi to function and it introduces inconsistent behavior between frozen/unfrozen code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants