feat(certifi): optionally use certifi certificates - #394
Merged
Conversation
We're using nodeenv to set up node on various HPC systems, and we already use certifi. Tools that use urllib3 work out of the box with certifi, but for urllib2 this is needed for certifi certificates to be picked up.
Defaults to not using certifi
--with-certifi asks for a specific trust store, so silently falling back to the system one leaves the user with an opaque SSL error instead of "certifi is not installed". Resolve certifi once in main() and warn there. Also narrows the try/except to the import only: it used to wrap the network call as well, so an ImportError raised from inside urlopen would silently repeat the request without certifi. Building the SSL context once instead of per request drops the repeated parsing of the CA bundle.
Every other persistent option is a Config attribute used as the argparse default, which makes it settable in ~/.nodeenvrc, tox.ini or setup.cfg and lists it in Config._dump(). with_certifi was hardcoded to False and so was reachable only from the command line, while a config default is exactly what a shared machine needs.
Adds the option to the "Other options" section and with_certifi to the configuration defaults block, which mirrors Config._dump(). Mentions the SSL_CERT_FILE alternative, which reaches the same result without the option.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on #388 by @mvdbeek: the two original commits are kept as-is and rebased on current
master, with review fixes on top.Why not just merge #388
--with-certififell back to the system certificate store without a word when certifi was missing. The flag asks for a specific trust store, so the user ended up with an opaqueCERTIFICATE_VERIFY_FAILEDinstead of "certifi is not installed" - and a broken system store is exactly the situation the flag exists for.tryblock wrapped the network call as well as the import, so anImportErrorraised from insideurlopenwould silently repeat the request without certifi.with_certifiwas not aConfigattribute, so unlike every other persistent option it could not be set in~/.nodeenvrc,tox.iniorsetup.cfg, and it was missing fromConfig._dump().What changed on top
fix(certifi)- certifi is resolved once inmain()via a newmake_certifi_context(), which imports certifi, warns and returnsNoneif it is absent, and otherwise returns a context built a single time. Thetrynow covers the import only. The globaluse_certifiis replaced bycertifi_context- the context has to be stored anyway, so a separate boolean was duplicate state, and this also settles the--with-certifi/args.with_certifi/use_certifinaming split. Precedence over--ignore_ssl_certsis now explicit rather than an artifact of thereturnorder.feat(certifi)-with_certifi = FalseonConfiganddefault=Config.with_certifiin the parser, so the option behaves like all the others.docs(certifi)- README option entry and the configuration defaults block, plus aCHANGESentry. The README also mentions theSSL_CERT_FILEalternative, which reaches the same result with no code at all:chore(setup)-extras_require={'certifi': ['certifi']}forpip install nodeenv[certifi].install_requiresstays empty.Tests -
TestCertificovers the threeurlopenbranches,make_certifi_contextwith and without certifi, and that the option reachesConfig.Verification
flake8 --extend-ignore=E127 nodeenv.py tests setup.py- cleanpytest tests/nodeenv_test.py- 88 passed (the one warning is the pre-existingssl.PROTOCOL_TLSdeprecation in theignore_ssl_certspath)nodeenv --with-certifi -lwith certifi installed - workswith_certifi = Truevia-C configfile- enables the optionCloses #388