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

Support SSL_CERT_FILE and SSL_CERT_DIR env vars #2903

Closed
wants to merge 1 commit into from

Conversation

thoger
Copy link

@thoger thoger commented Nov 26, 2015

A proposed fix for issue #2899.

Python PEP 476 (Enabling certificate verification by default for stdlib http
clients) recommends the use of SSL_CERT_FILE and SSL_CERT_DIR environment
variables to point the OpenSSL library used by Python to use specific
non-default bundle of trusted CA certificates.

https://www.python.org/dev/peps/pep-0476/#trust-database

These variables could not have been used to point scripts using requests to a
different CA bundle.  A different variable, REQUESTS_CA_BUNDLE, is read by
requests.  CURL_CA_BUNDLE is also used for compatibility with cURL.

This commit makes requests also look at SSL_CERT_FILE and SSL_CERT_DIR.  They
are handled as equivalent to REQUESTS_CA_BUNDLE.  As REQUESTS_CA_BUNDLE can
point to either certificate file or certificate directory, SSL_CERT_* can also
point to a file or directory.  There's no attempt to ensure SSL_CERT_FILE can
only point to a file and SSL_CERT_DIR to a directory.  This is similar to how
CURL_CA_BUNDLE is handled - requests allows it to specify certificate
directory, while cURL only allows it to specify certificate file.

Fixes requests issue #2899:

https://github.com/kennethreitz/requests/issues/2899
@Lukasa
Copy link
Member

Lukasa commented Nov 27, 2015

At a certain point this will get clearer if we use a for loop, but for now I think we're still ok. Thanks @thoger!

@standaloneSA
Copy link

Hey, this would be really helpful for me. Can you merge this, please?

@Lukasa
Copy link
Member

Lukasa commented Feb 26, 2016

@standaloneSA This has been tagged for the 3.0.0 release. =)

@standaloneSA
Copy link

All the +1s. You rock @Lukasa thanks!

@kennethreitz
Copy link
Contributor

@Lukasa why is this being delayed until 3.x? It doesn't appear to be break any backwards compatibility to me.

@kennethreitz
Copy link
Contributor

I'm hitting merge in a few hours unless I hear a good reason not to.

@Lukasa
Copy link
Member

Lukasa commented Apr 6, 2016

Because it can unexpectedly break previously working code: code that previously ignored these environment variables now pays attention to them, which can cause logic that used to work fine to now fail.

Generally speaking my view on environment variables is that if they aren't namespaced for us we should assume that some systems already have these set, and that they're set in a way that does not behave well for us.

@kennethreitz
Copy link
Contributor

Ah, I see. I do think that's a bit overzealous, and tend to consider a "breaking change" as removing/modifying existing behavior, not adding new behavior (e.g. considering this a "new feature").

But, I see your reasoning—especially given the implicitness of environment variables. That being said, I think this is being overly-precautious and is not something I'd do personally, but I'm fine with doing so if you feel strongly about it.

@Lukasa
Copy link
Member

Lukasa commented Apr 7, 2016

I think I'm just as borderline about it as you are: while the previous post portrayed confidence, it was mostly me playing devils advocate.

However, I don't think it hurts us at all to be a bit more cautious at this stage. We're downloaded more than 6 million times per month from PyPI alone, we underpin some of the most important software infrastructure in the world, and we run on a pretty severe deficit of maintainer time. My inclination is that when we're adding a new feature, if it's borderline whether it should be semver-major or semver-minor, we should play it safe and call it semver-major. Especially when there's a major release on the horizon (I am going to push for us to release 3.0.0 at PyCon).

@kennethreitz
Copy link
Contributor

@Lukasa ah, think that'll be during the sprints? If so, I might try to stay for them then (was planning on not attending sprints this year).

@Lukasa
Copy link
Member

Lukasa commented Apr 7, 2016

Should be do-able in one day of the sprints I reckon, so you don't need to hang around much. I'll also be there during tutorial-time if that works better for you.

@kennethreitz
Copy link
Contributor

It would be great to be able to walk around all PyCon telling people about v3.0.0.

Maybe I could finally do stickers this year. I've always wanted to do that.

@kennethreitz
Copy link
Contributor

idk, I feel like stickers are going out of style. I'm probably wrong, I just haven't gone to many conferences lately (and I haven't accepted stickers handed to me in years).

@kennethreitz
Copy link
Contributor

I've also wanted to do t-shirts in years past, but that always felt like too much overhead. I love the idea of a company sponsoring a drop-shipped t-shirt to every contributor.

Perhaps not a tshirt, but some other object. Branded quartz crystal shards!

If I had all the time in the world :)

@jakirkham
Copy link

So, I think the ssl module does a lot of the work for you. Take this example where I have set SSL_CERT_FILE and SSL_CERT_DIR to some dummy values to demonstrate the effect.

$ SSL_CERT_FILE=/bin/bash SSL_CERT_DIR=/bin ipython
Python 2.7.11 |Continuum Analytics, Inc.| (default, Dec  6 2015, 18:57:58) 
Type "copyright", "credits" or "license" for more information.

IPython 4.2.0 -- An enhanced Interactive Python.
?         -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help      -> Python's own help system.
object?   -> Details about 'object', use 'object??' for extra details.

In [1]: import ssl

In [2]: ssl.get_default_verify_paths()
Out[2]: DefaultVerifyPaths(cafile='/bin/bash', capath='/bin', openssl_cafile_env='SSL_CERT_FILE', openssl_cafile='/zopt/conda2/envs/nanshenv/ssl/cert.pem', openssl_capath_env='SSL_CERT_DIR', openssl_capath='/zopt/conda2/envs/nanshenv/ssl/certs')

@Lukasa
Copy link
Member

Lukasa commented Jun 26, 2016

@jakirkham The problem is that that call can also accidentally have some unexpected results from there. Ideally we'd come up with something a bit more comprehensive. Especially as that, for example, does not work on Python 2.6 (no such method), older Python 2.7s (pre 2.7.9), or Python 3.3.

@kennethreitz
Copy link
Contributor

Let's merge this into the 3 branch.

@kennethreitz
Copy link
Contributor

merged into proposed/3.0.0 branch!

@kennethreitz
Copy link
Contributor

✨🍰✨

@NOYB
Copy link

NOYB commented Dec 25, 2017

The problem with this solution is that it only looks in ONE of those locations for a matching cert. What is really needed is to look in A, then B, then C,... For instance look in a CA bundle, if match not found then continue by looking in a hash dir.

@thoger
Copy link
Author

thoger commented Jan 2, 2018

The problem with this solution is that it only looks in ONE of those locations for a matching cert. What is really needed is to look in A, then B, then C,... For instance look in a CA bundle, if match not found then continue by looking in a hash dir.

You can also consider the ability to avoid the use of system CA bundle when specifying SSL_CERT_FILE or SSL_CERT_DIR to be a feature - you often do not need to trust all the CAs in the system bundle for a specific use case and only trust one or few that you know you really need to trust.

Additionally, having SSL_CERT_* environment variables have different meaning in requests than they have for OpenSSL or Python stdlib would be error prone.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants