feat(certifi): optionally use certifi certificates (#394)

* Use certifi certificates if available

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.

* Add a flag to use certifi certificates

Defaults to not using certifi

* fix(certifi): report a missing certifi instead of silent fallback

--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.

* feat(certifi): allow with_certifi to be set from a config file

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.

* docs(certifi): document the --with-certifi option

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.

* chore(setup): add the certifi extra

---------

Co-authored-by: mvdbeek <m.vandenbeek@gmail.com>

Author: ekalinin

CHANGES

@@ -5,6 +5,8 @@ Version [unreleased]
 --------------------
 
 - Added check for how `activate` is called.
+- Added `--with-certifi` to download packages with the certifi certificate
+  bundle `#388 <https://github.com/ekalinin/nodeenv/pull/388>`_
 
 Version 1.3.1
 -------------

README.rst

@@ -330,6 +330,16 @@ Other options
 ``--ignore_ssl_certs``
     Ignore SSL certificates for package downloads. **UNSAFE - use at your own risk**.
 
+``--with-certifi``
+    Use the `certifi <https://pypi.org/project/certifi/>`_ certificate bundle for
+    package downloads instead of the system certificate store. Useful when the
+    system store is missing or outdated. If certifi is not installed, a warning is
+    printed and the system store is used. Ignored when ``--ignore_ssl_certs`` is
+    given. The same result can be achieved without this option by pointing
+    ``SSL_CERT_FILE`` at the bundle::
+
+        $ SSL_CERT_FILE=$(python -c 'import certifi; print(certifi.where())') nodeenv env
+
 ``--version``
     Show program version and exit.
 
@@ -351,6 +361,7 @@ These are the available options and their defaults::
     make = 'make'
     prebuilt = True
     ignore_ssl_certs = False
+    with_certifi = False
     mirror = None
 
 Alternatives

nodeenv.py

@@ -59,6 +59,9 @@
 is_CYGWIN = platform.system().startswith(('CYGWIN', 'MSYS'))
 
 ignore_ssl_certs = False
+# SSL context backed by the certifi bundle, built once by main()
+# when --with-certifi is given and certifi is importable
+certifi_context = None
 
 # ---------------------------------------------------------
 # Utils
@@ -101,6 +104,7 @@ class Config(object):
     make = 'make'
     prebuilt = True
     ignore_ssl_certs = False
+    with_certifi = False
     mirror = None
 
     @classmethod
@@ -368,6 +372,12 @@ def make_parser():
         action='store_true', default=Config.ignore_ssl_certs,
         help='Ignore certificates for package downloads. - UNSAFE -')
 
+    parser.add_argument(
+        '--with-certifi', dest='with_certifi',
+        action='store_true', default=Config.with_certifi,
+        help='Use the certifi certificate bundle for package downloads, '
+        'if certifi is installed. Ignored with --ignore_ssl_certs.')
+
     parser.add_argument(
         metavar='DEST_DIR', dest='env_dir', nargs='?',
         help='Destination directory')
@@ -644,6 +654,24 @@ def download_node_src(node_url, src_dir, args):
             archive.extractall(src_dir, extract_list)
 
 
+def make_certifi_context():
+    """
+    Build an SSL context backed by the certifi bundle.
+
+    Returns None if certifi is not installed, so that downloads keep
+    using the system certificate store.
+    """
+    try:
+        import certifi
+    except ImportError:
+        logger.warning(
+            'certifi is not installed, --with-certifi is ignored: '
+            'falling back to the system certificate store')
+        return None
+
+    return ssl.create_default_context(cafile=certifi.where())
+
+
 def urlopen(url):
     home_url = "https://github.com/ekalinin/nodeenv/"
     headers = {'User-Agent': 'nodeenv/%s (%s)' % (nodeenv_version, home_url)}
@@ -654,6 +682,11 @@ def urlopen(url):
         context = ssl.SSLContext(ssl.PROTOCOL_TLS)
         context.verify_mode = ssl.CERT_NONE
         return urllib2.urlopen(req, context=context)
+
+    # Use certifi certificates if they were requested and are available
+    if certifi_context is not None:
+        return urllib2.urlopen(req, context=certifi_context)
+
     return urllib2.urlopen(req)
 
 # ---------------------------------------------------------
@@ -1132,8 +1165,11 @@ def main():
 
     global src_base_url
     global ignore_ssl_certs
+    global certifi_context
 
     ignore_ssl_certs = args.ignore_ssl_certs
+    if args.with_certifi and not ignore_ssl_certs:
+        certifi_context = make_certifi_context()
 
     src_domain = None
     if args.mirror:

setup.py

@@ -37,6 +37,7 @@ def read_file(file_name):
     author='Eugene Kalinin',
     author_email='e.v.kalinin@gmail.com',
     install_requires=[],
+    extras_require={'certifi': ['certifi']},
     python_requires=(
         ">=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*,!=3.4.*,!=3.5.*,!=3.6.*"
     ),

tests/nodeenv_test.py

@@ -12,6 +12,7 @@
 import sys
 import sysconfig
 import platform
+import ssl
 import zipfile
 
 try:
@@ -1619,3 +1620,60 @@ def test_install_npm_win_zip_extraction(self):
 
             # Verify extraction
             mock_zip.extractall.assert_called_once_with(src_dir)
+
+
+class TestCertifi:
+    """Tests for the --with-certifi option"""
+
+    def test_urlopen_without_certifi(self):
+        """No SSL context is passed when certifi is not in use"""
+        with mock.patch.object(nodeenv, 'ignore_ssl_certs', False), \
+             mock.patch.object(nodeenv, 'certifi_context', None), \
+             mock.patch.object(nodeenv.urllib2, 'urlopen') as m_urlopen:
+            nodeenv.urlopen('https://nodejs.org/dist/index.json')
+
+        assert m_urlopen.call_args[1] == {}
+
+    def test_urlopen_with_certifi(self):
+        """The context built by main() is reused for every download"""
+        with mock.patch.object(nodeenv, 'ignore_ssl_certs', False), \
+             mock.patch.object(nodeenv, 'certifi_context',
+                               mock.sentinel.certifi_context), \
+             mock.patch.object(nodeenv.urllib2, 'urlopen') as m_urlopen:
+            nodeenv.urlopen('https://nodejs.org/dist/index.json')
+
+        context = m_urlopen.call_args[1]['context']
+        assert context is mock.sentinel.certifi_context
+
+    def test_urlopen_ignore_ssl_certs_wins(self):
+        """--ignore_ssl_certs takes precedence over --with-certifi"""
+        with mock.patch.object(nodeenv, 'ignore_ssl_certs', True), \
+             mock.patch.object(nodeenv, 'certifi_context',
+                               mock.sentinel.certifi_context), \
+             mock.patch.object(nodeenv.urllib2, 'urlopen') as m_urlopen:
+            nodeenv.urlopen('https://nodejs.org/dist/index.json')
+
+        assert m_urlopen.call_args[1]['context'].verify_mode == ssl.CERT_NONE
+
+    def test_make_certifi_context(self):
+        certifi = mock.Mock()
+        certifi.where.return_value = '/path/to/cacert.pem'
+
+        with mock.patch.dict(sys.modules, {'certifi': certifi}), \
+             mock.patch.object(nodeenv.ssl,
+                               'create_default_context') as m_context:
+            assert nodeenv.make_certifi_context() is m_context.return_value
+
+        m_context.assert_called_once_with(cafile='/path/to/cacert.pem')
+
+    def test_make_certifi_context_without_certifi(self):
+        """A missing certifi is reported instead of silently ignored"""
+        with mock.patch.dict(sys.modules, {'certifi': None}), \
+             mock.patch.object(nodeenv.logger, 'warning') as m_warning:
+            assert nodeenv.make_certifi_context() is None
+
+        assert 'certifi is not installed' in m_warning.call_args[0][0]
+
+    def test_with_certifi_is_configurable(self):
+        """with_certifi can be set from the config file, like other options"""
+        assert 'with_certifi' in nodeenv.Config._default