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

Set the SSL_CERT_FILE environment variables via an activation script on Windows #21

Merged
merged 5 commits into from
Aug 23, 2023

Conversation

JeanChristopheMorinPerso
Copy link

@JeanChristopheMorinPerso JeanChristopheMorinPerso commented Aug 9, 2023

Set the SSL_CERT_FILE environment variable via an activation script on Windows to make sure Python can do HTTPS requests by default without requiring certifi.

Note that the activation scripts will only be bundled on Windows. Also note that we will only set SSL_CERT_FILE if not not yet set. If a customer has set its own SSL_CERT_FILE, we don't want to override it to something else.

For more context, when compiling OpenSSL, we can pass the --openssldir flag. The openssl dir is the directory where
the OpenSSL config can be found. The path given to --openssldir gets hardcoded in the libraries.

On Linux and macOS, we set it to $PREFIX/ssl (implicitly by setting --prefix) and when the package is installed in
an environment, conda takes care of replacing the the prefix in the DSOs with the path to the environment where the package is being installed. $PREFIX/ssl is populated with a file named cacert.pem which comes from the ca-certificates package. So when we use the openssl package, it uses this file.

But on Windows, conda doesn't do any DSO prefix replacement. So we hardcode the value to %CommonProgramFiles%\ssl
for security reasons (see

REM Configure step
REM
REM Conda currently does not perform prefix replacement on Windows, so
REM OPENSSLDIR cannot (reliably) be used to provide functionality such as a
REM default configuration and standard CA certificates on a per-environment
REM basis. Given that, we set OPENSSLDIR to a location with extremely limited
REM write permissions to limit the risk of non-privileged users exploiting
REM OpenSSL's engines feature to perform arbitrary code execution attacks
REM against applications that load the OpenSSL DLLs.

and https://anaconda.atlassian.net/browse/DSNC-2391). Because of that, openssl doesn't know anything about CA root certs and calling urllib.request.urlopen in python on Windows will result in an error like

ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED]  
certificate verify failed: unable to get local issuer certificate   
(_ssl.c:1129)

This is because Python doesn't use the Windows system truststore and since %CommonProgramFiles%\ssl likely doesn't exist, then it fails.

Setting $SSL_CERT_FILE avoids this problem. It makes it consistent across all platforms.

…on Windows to make sure Python can do HTTPS requests by default without requiring certifi.
@JeanChristopheMorinPerso JeanChristopheMorinPerso force-pushed the use_SSL_CERT_FILE_on_windows branch from 3bf0f8f to 2fa0139 Compare August 9, 2023 20:32
recipe/deactivate.sh Outdated Show resolved Hide resolved
…FIX and properly unset __CONDA_OPENSLL_CERT_FILE_SET
@JeanChristopheMorinPerso
Copy link
Author

@chenghlee do you know if I need to add a PowerShell activation script?

Copy link

@ryanskeith ryanskeith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look ok to me. I don't see anything that raises a red flag. We require ca_certificates so we should be ok to always find them as well.

Copy link

@M-Waszkiewicz-Anaconda M-Waszkiewicz-Anaconda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please tell us the reason of that PR why export SSL_CERT_FILE is better than the certifi ?

recipe/bld.bat Show resolved Hide resolved
@JeanChristopheMorinPerso
Copy link
Author

@M-Waszkiewicz-Anaconda I updated the PR description.

@cbouss
Copy link

cbouss commented Aug 10, 2023

@chenghlee do you know if I need to add a PowerShell activation script?

I think you do need to add them. From what I see:
conda sets this when on powershell: https://github.com/conda/conda/blob/main/conda/activate.py#L1111
And use it here to get the list of activate scripts: https://github.com/conda/conda/blob/main/conda/activate.py#L746

@M-Waszkiewicz-Anaconda
Copy link

@M-Waszkiewicz-Anaconda I updated the PR description.

thank you, so if I understand correctly the %CONDA_PREFIX%\Library\ssl\cacert.pem wasn't part of certifi or for some other reason it was not working

@JeanChristopheMorinPerso
Copy link
Author

JeanChristopheMorinPerso commented Aug 10, 2023

@M-Waszkiewicz-Anaconda I updated the PR description.

thank you, so if I understand correctly the %CONDA_PREFIX%\Library\ssl\cacert.pem wasn't part of certifi or for some other reason it was not working

No, certifi works, but requires one to hook it up (unless you use libraries like requests, I think). certifi ships its own cacert.pem. This PR just ensures that our packaging ecosystem works as it's supposed to without a special exception for Windows.

- python -c "import certifi; import ssl; import urllib.request as urlrq; urlrq.urlopen('https://pypi.org', context=ssl.create_default_context(cafile=certifi.where()))" # [win]
- if "%SSL_CERT_FILE%"=="" exit 1 # [win]
- if not exist "%SSL_CERT_FILE%" exit 1 # [win]
- python -c "import urllib.request; urllib.request.urlopen('https://pypi.org')"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be extra beneficial to also check in the test.commands section that cacert.pem are also delivered and present in the intended ${CONDA_PREFIX} path?

@bhuwan-agarwal
Copy link

As described in the PR description, this change standardize the way of setting certificate location regardless of the platform. Personally, I'm unsure of any unintended consequences on Windows. Otherwise this change seems fine to me.

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 this pull request may close these issues.

7 participants