Check OpenSSL environment variables before defaulting to certifi (#196)

* Check OpenSSL environment variables before defaulting to certifi

Signed-off-by: Roger Aiudi <[email protected]>

* Fix formatting

Signed-off-by: Roger Aiudi <[email protected]>

* Moved CA_CERTS to the base Connection module

Signed-off-by: Roger Aiudi <[email protected]>

* Updated requests Connection to use common CA_CERTS by default

Signed-off-by: Roger Aiudi <[email protected]>

* Pass nox linting

Signed-off-by: Roger Aiudi <[email protected]>

* Update CHANGELOG.md and USER_GUIDE.md

Signed-off-by: Roger Aiudi <[email protected]>

* Updated AIOHttpConnection to only load CA_CERTS if verify_certs is True

Signed-off-by: Roger Aiudi <[email protected]>

* Added test cases for CA_CERTS handling in each Connection implementation

Signed-off-by: Roger Aiudi <[email protected]>

* Move CA cert handling to Connection.default_ca_certs()
Add test cases for the different CA cert configurations

Signed-off-by: Roger Aiudi <[email protected]>

* Update actions to test unsupported Python versions on ubuntu-20.04

Signed-off-by: Roger Aiudi <[email protected]>

* Fix Python versions being interpreted as floats

Signed-off-by: Roger Aiudi <[email protected]>

* Workaround Monkeypatch not available on old pytest versions

Signed-off-by: Roger Aiudi <[email protected]>

Signed-off-by: Roger Aiudi <[email protected]>
Co-authored-by: Harsha Vamsi Kalluri <[email protected]>

.github/workflows/ci.yml

@@ -39,13 +39,21 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        python-version: [2.7, 3.5, 3.6, 3.7, 3.8, 3.9]
+        python-version: ['3.7', '3.8', '3.9', '3.10']
+        os: [ubuntu-latest]
         experimental: [false]
         include:
-          - python-version: 3.10.0-beta.2
+          - python-version: '2.7'
-            experimental: true
+            os: ubuntu-20.04
+            experimental: false
+          - python-version: '3.5'
+            os: ubuntu-20.04
+            experimental: false
+          - python-version: '3.6'
+            os: ubuntu-20.04
+            experimental: false
 
-    runs-on: ubuntu-latest
+    runs-on: ${{ matrix.os }}
     name: test-${{ matrix.python-version }}
     continue-on-error: ${{ matrix.experimental }}
     steps:

CHANGELOG.md

@@ -7,6 +7,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Github workflow for changelog verification ([#218](https://github.com/opensearch-project/opensearch-py/pull/218))
 ### Changed
 - Updated getting started to user guide ([#233](https://github.com/opensearch-project/opensearch-py/pull/233))
+- Updated CA certificate handling to check OpenSSL environment variables before defaulting to certifi ([#196](https://github.com/opensearch-project/opensearch-py/pull/196))
 ### Deprecated
 
 ### Removed

USER_GUIDE.md

@@ -53,7 +53,14 @@ from opensearchpy import OpenSearch
 host = 'localhost'
 port = 9200
 auth = ('admin', 'admin') # For testing only. Don't store credentials in code.
-ca_certs_path = '/full/path/to/root-ca.pem' # Provide a CA bundle if you use intermediate CAs with your root CA.
+
+# Provide a CA bundle if you use intermediate CAs with your root CA.
+# If this is not given, the CA bundle is is discovered from the first available
+# following options:
+# - OpenSSL environment variables SSL_CERT_FILE and SSL_CERT_DIR
+# - certifi bundle (https://pypi.org/project/certifi/)
+# - default behavior of the connection backend (most likely system certs)
+ca_certs_path = '/full/path/to/root-ca.pem'
 
 # Optional client certificates if you don't want to use HTTP basic authentication.
 # client_cert_path = '/full/path/to/client.pem'

opensearchpy/_async/client/__init__.pyi

@@ -60,7 +60,7 @@ class AsyncOpenSearch(object):
         self,
         hosts: Any = ...,
         transport_class: Type[AsyncTransport] = ...,
-        **kwargs: Any
+        **kwargs: Any,
     ) -> None: ...
     def __repr__(self) -> str: ...
     async def __aenter__(self) -> "AsyncOpenSearch": ...

opensearchpy/_async/http_aiohttp.py

@@ -49,15 +49,6 @@ from .compat import get_running_loop
 VERIFY_CERTS_DEFAULT = object()
 SSL_SHOW_WARN_DEFAULT = object()
 
-CA_CERTS = None
-
-try:
-    import certifi
-
-    CA_CERTS = certifi.where()
-except ImportError:
-    pass
-
 
 class AsyncConnection(Connection):
     """Base class for Async HTTP connection implementations"""
@@ -185,7 +176,7 @@ class AIOHttpConnection(AsyncConnection):
                 ssl_context.check_hostname = False
                 ssl_context.verify_mode = ssl.CERT_NONE
 
-            ca_certs = CA_CERTS if ca_certs is None else ca_certs
+            ca_certs = self.default_ca_certs() if ca_certs is None else ca_certs
             if verify_certs:
                 if not ca_certs:
                     raise ImproperlyConfigured(
@@ -193,6 +184,12 @@ class AIOHttpConnection(AsyncConnection):
                         "validation. Either pass them in using the ca_certs parameter or "
                         "install certifi to use it automatically."
                     )
+                if os.path.isfile(ca_certs):
+                    ssl_context.load_verify_locations(cafile=ca_certs)
+                elif os.path.isdir(ca_certs):
+                    ssl_context.load_verify_locations(capath=ca_certs)
+                else:
+                    raise ImproperlyConfigured("ca_certs parameter is not a path")
             else:
                 if ssl_show_warn:
                     warnings.warn(
@@ -200,13 +197,6 @@ class AIOHttpConnection(AsyncConnection):
                         % self.host
                     )
 
-            if os.path.isfile(ca_certs):
-                ssl_context.load_verify_locations(cafile=ca_certs)
-            elif os.path.isdir(ca_certs):
-                ssl_context.load_verify_locations(capath=ca_certs)
-            else:
-                raise ImproperlyConfigured("ca_certs parameter is not a path")
-
             # Use client_cert and client_key variables for SSL certificate configuration.
             if client_cert and not os.path.isfile(client_cert):
                 raise ImproperlyConfigured("client_cert is not a path to a file")

opensearchpy/connection/base.py

@@ -304,3 +304,22 @@ class Connection(object):
 
     def _get_default_user_agent(self):
         return "opensearch-py/%s (Python %s)" % (__versionstr__, python_version())
+
+    @staticmethod
+    def default_ca_certs():
+        """
+        Get the default CA certificate bundle, preferring those configured in
+        the standard OpenSSL environment variables before those provided by
+        certifi (if available)
+        """
+        ca_certs = os.environ.get("SSL_CERT_FILE") or os.environ.get("SSL_CERT_DIR")
+
+        if ca_certs:
+            return ca_certs
+
+        try:
+            import certifi
+        except ImportError:
+            pass
+        else:
+            return certifi.where()

opensearchpy/connection/base.pyi

@@ -114,3 +114,5 @@ class Connection(object):
         self, status_code: int, raw_data: str, content_type: Optional[str]
     ) -> NoReturn: ...
     def _get_default_user_agent(self) -> str: ...
+    @staticmethod
+    def default_ca_certs() -> Optional[str]: ...

opensearchpy/connection/http_requests.py

@@ -54,8 +54,10 @@ class RequestsHttpConnection(Connection):
     :arg use_ssl: use ssl for the connection if `True`
     :arg verify_certs: whether to verify SSL certificates
     :arg ssl_show_warn: show warning when verify certs is disabled
-    :arg ca_certs: optional path to CA bundle. By default standard requests'
+    :arg ca_certs: optional path to CA bundle. Defaults to configured OpenSSL
-        bundle will be used.
+        bundles from environment variables and then certifi before falling
+        back to the standard requests bundle to improve consistency with
+        other Connection implementations
     :arg client_cert: path to the file containing the private key and the
         certificate, or cert only if using client_key
     :arg client_key: path to the file containing the private key if using
@@ -129,6 +131,10 @@ class RequestsHttpConnection(Connection):
                     "You cannot pass CA certificates when verify SSL is off."
                 )
             self.session.verify = ca_certs
+        elif verify_certs:
+            ca_certs = self.default_ca_certs()
+            if ca_certs:
+                self.session.verify = ca_certs
 
         if not ssl_show_warn:
             requests.packages.urllib3.disable_warnings()

opensearchpy/connection/http_urllib3.py

@@ -24,7 +24,6 @@
 #  specific language governing permissions and limitations
 #  under the License.
 
-
 import ssl
 import time
 import warnings
@@ -49,15 +48,6 @@ from .base import Connection
 VERIFY_CERTS_DEFAULT = object()
 SSL_SHOW_WARN_DEFAULT = object()
 
-CA_CERTS = None
-
-try:
-    import certifi
-
-    CA_CERTS = certifi.where()
-except ImportError:
-    pass
-
 
 def create_ssl_context(**kwargs):
     """
@@ -186,7 +176,7 @@ class Urllib3HttpConnection(Connection):
             if ssl_show_warn is SSL_SHOW_WARN_DEFAULT:
                 ssl_show_warn = True
 
-            ca_certs = CA_CERTS if ca_certs is None else ca_certs
+            ca_certs = self.default_ca_certs() if ca_certs is None else ca_certs
             if verify_certs:
                 if not ca_certs:
                     raise ImproperlyConfigured(

opensearchpy/helpers/test.pyi

@@ -30,7 +30,6 @@ from unittest import TestCase
 from ..client import OpenSearch
 
 OPENSEARCH_URL: str
-CA_CERTS: str
 
 def get_test_client(nowait: bool = ..., **kwargs: Any) -> OpenSearch: ...
 def _get_version(version_string: str) -> Tuple[int, ...]: ...

test_opensearchpy/test_async/test_connection.py

@@ -40,6 +40,7 @@ from multidict import CIMultiDict
 
 from opensearchpy import AIOHttpConnection, __versionstr__
 from opensearchpy.compat import reraise_exceptions
+from opensearchpy.connection import Connection
 from opensearchpy.exceptions import ConnectionError
 
 pytestmark = pytest.mark.asyncio
@@ -246,6 +247,25 @@ class TestAIOHttpConnection:
                     == str(w[0].message)
                 )
 
+    @patch("ssl.SSLContext.load_verify_locations")
+    def test_uses_given_ca_certs(self, load_verify_locations, tmp_path):
+        path = tmp_path / "ca_certs.pem"
+        path.touch()
+        AIOHttpConnection(use_ssl=True, ca_certs=str(path))
+        load_verify_locations.assert_called_once_with(cafile=str(path))
+
+    @patch("ssl.SSLContext.load_verify_locations")
+    def test_uses_default_ca_certs(self, load_verify_locations):
+        AIOHttpConnection(use_ssl=True)
+        load_verify_locations.assert_called_once_with(
+            cafile=Connection.default_ca_certs()
+        )
+
+    @patch("ssl.SSLContext.load_verify_locations")
+    def test_uses_no_ca_certs(self, load_verify_locations):
+        AIOHttpConnection(use_ssl=True, verify_certs=False)
+        load_verify_locations.assert_not_called()
+
     @patch("opensearchpy.connection.base.logger")
     async def test_uncompressed_body_logged(self, logger):
         con = await self._get_mock_connection(connection_params={"http_compress": True})

test_opensearchpy/test_connection.py

@@ -62,6 +62,11 @@ from opensearchpy.exceptions import (
 
 from .test_cases import SkipTest, TestCase
 
+try:
+    from pytest import MonkeyPatch
+except ImportError:  # Old version of pytest for 2.7 and 3.5
+    from _pytest.monkeypatch import MonkeyPatch
+
 
 def gzip_decompress(data):
     buf = gzip.GzipFile(fileobj=io.BytesIO(data), mode="rb")
@@ -155,6 +160,28 @@ class TestBaseConnection(TestCase):
         finally:
             os.environ.pop("ELASTIC_CLIENT_APIVERSIONING")
 
+    def test_ca_certs_ssl_cert_file(self):
+        cert = "/path/to/clientcert.pem"
+        with MonkeyPatch().context() as monkeypatch:
+            monkeypatch.setenv("SSL_CERT_FILE", cert)
+            assert Connection.default_ca_certs() == cert
+
+    def test_ca_certs_ssl_cert_dir(self):
+        cert = "/path/to/clientcert/dir"
+        with MonkeyPatch().context() as monkeypatch:
+            monkeypatch.setenv("SSL_CERT_DIR", cert)
+            assert Connection.default_ca_certs() == cert
+
+    def test_ca_certs_certifi(self):
+        import certifi
+
+        assert Connection.default_ca_certs() == certifi.where()
+
+    def test_no_ca_certs(self):
+        with MonkeyPatch().context() as monkeypatch:
+            monkeypatch.setitem(sys.modules, "certifi", None)
+            assert Connection.default_ca_certs() is None
+
 
 class TestUrllib3Connection(TestCase):
     def _get_mock_connection(self, connection_params={}, response_body=b"{}"):
@@ -394,6 +421,19 @@ class TestUrllib3Connection(TestCase):
                     str(w[0].message),
                 )
 
+    def test_uses_given_ca_certs(self):
+        path = "/path/to/my/ca_certs.pem"
+        c = Urllib3HttpConnection(use_ssl=True, ca_certs=path)
+        self.assertEqual(path, c.pool.ca_certs)
+
+    def test_uses_default_ca_certs(self):
+        c = Urllib3HttpConnection(use_ssl=True)
+        self.assertEqual(Connection.default_ca_certs(), c.pool.ca_certs)
+
+    def test_uses_no_ca_certs(self):
+        c = Urllib3HttpConnection(use_ssl=True, verify_certs=False)
+        self.assertIsNone(c.pool.ca_certs)
+
     @patch("opensearchpy.connection.base.logger")
     def test_uncompressed_body_logged(self, logger):
         con = self._get_mock_connection(connection_params={"http_compress": True})
@@ -526,6 +566,19 @@ class TestRequestsConnection(TestCase):
         self.assertEqual("GET", request.method)
         self.assertEqual(None, request.body)
 
+    def test_uses_given_ca_certs(self):
+        path = "/path/to/my/ca_certs.pem"
+        c = RequestsHttpConnection(ca_certs=path)
+        self.assertEqual(path, c.session.verify)
+
+    def test_uses_default_ca_certs(self):
+        c = RequestsHttpConnection()
+        self.assertEqual(Connection.default_ca_certs(), c.session.verify)
+
+    def test_uses_no_ca_certs(self):
+        c = RequestsHttpConnection(verify_certs=False)
+        self.assertFalse(c.session.verify)
+
     def test_nowarn_when_uses_https_if_verify_certs_is_off(self):
         with warnings.catch_warnings(record=True) as w:
             con = self._get_mock_connection(