Change error on empty subjectAltNames (#67)

* More Napoleon * Change error on empty subjectAltNames * Add PR # * Add integration tests for cryptography * Update tests/test_hazmat.py Co-authored-by: Adi Roiban <[email protected]> * Black --------- Co-authored-by: Adi Roiban <[email protected]>
pyca · Jan 14, 2024 · 0c81d1d · 0c81d1d
1 parent f93a617
commit 0c81d1d
Show file tree

Hide file tree

Showing 8 changed files with 139 additions and 45 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,12 @@ You can find out backwards-compatibility policy [here](https://github.com/pyca/s
 
 ## [Unreleased](https://github.com/pyca/service-identity/compare/23.1.0...HEAD)
 
+### Changed
+
+- If a certificate doesn't contain any `subjectAltName`s, we now raise `service_identity.exceptions.CertificateError` instead of `service_identity.exceptions.VerificationError` to make the problem easier to debug.
+  [#67](https://github.com/pyca/service-identity/pull/67)
+
+
 ## [23.1.0](https://github.com/pyca/service-identity/compare/21.1.0...23.1.0) - 2023-06-14
 
 ### Removed

diff --git a/src/service_identity/cryptography.py b/src/service_identity/cryptography.py
@@ -40,22 +40,31 @@
 def verify_certificate_hostname(
     certificate: Certificate, hostname: str
 ) -> None:
-    """
+    r"""
     Verify whether *certificate* is valid for *hostname*.
 
-    .. note:: Nothing is verified about the *authority* of the certificate;
-       the caller must verify that the certificate chains to an appropriate
-       trust root themselves.
+    .. note::
+        Nothing is verified about the *authority* of the certificate;
+        the caller must verify that the certificate chains to an appropriate
+        trust root themselves.
+
+    Args:
+        certificate: A *cryptography* X509 certificate object.
+
+        hostname: The hostname that *certificate* should be valid for.
 
-    :param certificate: A *cryptography* X509 certificate object.
-    :param hostname: The hostname that *certificate* should be valid for.
+    Raises:
+        service_identity.VerificationError:
+            If *certificate* is not valid for *hostname*.
 
-    :raises service_identity.VerificationError: If *certificate* is not valid
-        for *hostname*.
-    :raises service_identity.CertificateError: If *certificate* contains
-        invalid / unexpected data.
+        service_identity.CertificateError:
+            If *certificate* contains invalid / unexpected data. This includes
+            the case where the certificate contains no `subjectAltName`\ s.
 
-    :returns: ``None``
+    .. versionchanged:: 24.1.0
+        :exc:`~service_identity.CertificateError` is raised if the certificate
+        contains no ``subjectAltName``\ s instead of
+        :exc:`~service_identity.VerificationError`.
     """
     verify_service_identity(
         cert_patterns=extract_patterns(certificate),
@@ -67,25 +76,35 @@ def verify_certificate_hostname(
 def verify_certificate_ip_address(
     certificate: Certificate, ip_address: str
 ) -> None:
-    """
+    r"""
     Verify whether *certificate* is valid for *ip_address*.
 
-    .. note:: Nothing is verified about the *authority* of the certificate;
-       the caller must verify that the certificate chains to an appropriate
-       trust root themselves.
+    .. note::
+        Nothing is verified about the *authority* of the certificate;
+        the caller must verify that the certificate chains to an appropriate
+        trust root themselves.
+
+    Args:
+        certificate: A *cryptography* X509 certificate object.
 
-    :param certificate: A *cryptography* X509 certificate object.
-    :param ip_address: The IP address that *connection* should be valid
-        for.  Can be an IPv4 or IPv6 address.
+        ip_address:
+            The IP address that *connection* should be valid for.  Can be an
+            IPv4 or IPv6 address.
 
-    :raises service_identity.VerificationError: If *certificate* is not valid
-        for *ip_address*.
-    :raises service_identity.CertificateError: If *certificate* contains
-        invalid / unexpected data.
+    Raises:
+        service_identity.VerificationError:
+            If *certificate* is not valid for *ip_address*.
 
-    :returns: ``None``
+        service_identity.CertificateError:
+            If *certificate* contains invalid / unexpected data. This includes
+            the case where the certificate contains no ``subjectAltName``\ s.
 
     .. versionadded:: 18.1.0
+
+    .. versionchanged:: 24.1.0
+        :exc:`~service_identity.CertificateError` is raised if the certificate
+        contains no ``subjectAltName``\ s instead of
+        :exc:`~service_identity.VerificationError`.
     """
     verify_service_identity(
         cert_patterns=extract_patterns(certificate),

diff --git a/src/service_identity/hazmat.py b/src/service_identity/hazmat.py
@@ -50,6 +50,11 @@ def verify_service_identity(
     *obligatory_ids* must be both present and match.  *optional_ids* must match
     if a pattern of the respective type is present.
     """
+    if not cert_patterns:
+        raise CertificateError(
+            "Certificate does not contain any `subjectAltName`s."
+        )
+
     errors = []
     matches = _find_matches(cert_patterns, obligatory_ids) + _find_matches(
         cert_patterns, optional_ids

diff --git a/src/service_identity/pyopenssl.py b/src/service_identity/pyopenssl.py
@@ -37,19 +37,28 @@
 
 
 def verify_hostname(connection: Connection, hostname: str) -> None:
-    """
+    r"""
     Verify whether the certificate of *connection* is valid for *hostname*.
 
-    :param connection: A pyOpenSSL connection object.
-    :param hostname: The hostname that *connection* should be connected to.
+    Args:
+        connection: A pyOpenSSL connection object.
+
+        hostname: The hostname that *connection* should be connected to.
+
+    Raises:
+        service_identity.VerificationError:
+            If *connection* does not provide a certificate that is valid for
+            *hostname*.
 
-    :raises service_identity.VerificationError: If *connection* does not
-        provide a certificate that is valid for *hostname*.
-    :raises service_identity.CertificateError: If the certificate chain of
-        *connection* contains a certificate that contains invalid/unexpected
-        data.
+        service_identity.CertificateError:
+            If certificate provided by *connection* contains invalid /
+            unexpected data. This includes the case where the certificate
+            contains no ``subjectAltName``\ s.
 
-    :returns: ``None``
+    .. versionchanged:: 24.1.0
+        :exc:`~service_identity.CertificateError` is raised if the certificate
+        contains no ``subjectAltName``\ s instead of
+        :exc:`~service_identity.VerificationError`.
     """
     verify_service_identity(
         cert_patterns=extract_patterns(
@@ -61,22 +70,31 @@ def verify_hostname(connection: Connection, hostname: str) -> None:
 
 
 def verify_ip_address(connection: Connection, ip_address: str) -> None:
-    """
+    r"""
     Verify whether the certificate of *connection* is valid for *ip_address*.
 
-    :param connection: A pyOpenSSL connection object.
-    :param ip_address: The IP address that *connection* should be connected to.
-        Can be an IPv4 or IPv6 address.
+    Args:
+        connection: A pyOpenSSL connection object.
+
+        ip_address:
+            The IP address that *connection* should be connected to. Can be an
+            IPv4 or IPv6 address.
 
-    :raises service_identity.VerificationError: If *connection* does not
-        provide a certificate that is valid for *ip_address*.
-    :raises service_identity.CertificateError: If the certificate chain of
-        *connection* contains a certificate that contains invalid/unexpected
-        data.
+    Raises:
+        service_identity.VerificationError:
+            If *connection* does not provide a certificate that is valid for
+            *ip_address*.
 
-    :returns: ``None``
+        service_identity.CertificateError:
+            If the certificate chain of *connection* contains a certificate
+            that contains invalid/unexpected data.
 
     .. versionadded:: 18.1.0
+
+    .. versionchanged:: 24.1.0
+        :exc:`~service_identity.CertificateError` is raised if the certificate
+        contains no ``subjectAltName``\ s instead of
+        :exc:`~service_identity.VerificationError`.
     """
     verify_service_identity(
         cert_patterns=extract_patterns(

diff --git a/tests/util.py → tests/certificates.py b/tests/util.py → tests/certificates.py
diff --git a/tests/test_cryptography.py b/tests/test_cryptography.py
@@ -12,6 +12,7 @@
     verify_certificate_ip_address,
 )
 from service_identity.exceptions import (
+    CertificateError,
     DNSMismatch,
     IPAddressMismatch,
     VerificationError,
@@ -24,7 +25,12 @@
     URIPattern,
 )
 
-from .util import PEM_CN_ONLY, PEM_DNS_ONLY, PEM_EVERYTHING, PEM_OTHER_NAME
+from .certificates import (
+    PEM_CN_ONLY,
+    PEM_DNS_ONLY,
+    PEM_EVERYTHING,
+    PEM_OTHER_NAME,
+)
 
 
 backend = default_backend()
@@ -35,6 +41,29 @@
 
 
 class TestPublicAPI:
+    def test_no_cert_patterns_hostname(self):
+        """
+        A certificate without subjectAltNames raises a helpful
+        CertificateError.
+        """
+        with pytest.raises(
+            CertificateError,
+            match="Certificate does not contain any `subjectAltName`s.",
+        ):
+            verify_certificate_hostname(X509_CN_ONLY, "example.com")
+
+    @pytest.mark.parametrize("ip", ["203.0.113.0", "2001:db8::"])
+    def test_no_cert_patterns_ip_address(self, ip):
+        """
+        A certificate without subjectAltNames raises a helpful
+        CertificateError.
+        """
+        with pytest.raises(
+            CertificateError,
+            match="Certificate does not contain any `subjectAltName`s.",
+        ):
+            verify_certificate_ip_address(X509_CN_ONLY, ip)
+
     def test_certificate_verify_hostname_ok(self):
         """
         verify_certificate_hostname succeeds if the hostnames match.

diff --git a/tests/test_hazmat.py b/tests/test_hazmat.py
@@ -30,8 +30,8 @@
     verify_service_identity,
 )
 
+from .certificates import DNS_IDS
 from .test_cryptography import CERT_EVERYTHING
-from .util import DNS_IDS
 
 
 try:
@@ -45,6 +45,18 @@ class TestVerifyServiceIdentity:
     Simple integration tests for verify_service_identity.
     """
 
+    def test_no_cert_patterns(self):
+        """
+        Empty cert patterns raise a helpful CertificateError.
+        """
+        with pytest.raises(
+            CertificateError,
+            match="Certificate does not contain any `subjectAltName`s.",
+        ):
+            verify_service_identity(
+                cert_patterns=[], obligatory_ids=[], optional_ids=[]
+            )
+
     def test_dns_id_success(self):
         """
         Return pairs of certificate ids and service ids on matches.

diff --git a/tests/test_pyopenssl.py b/tests/test_pyopenssl.py
@@ -21,7 +21,12 @@
     verify_ip_address,
 )
 
-from .util import PEM_CN_ONLY, PEM_DNS_ONLY, PEM_EVERYTHING, PEM_OTHER_NAME
+from .certificates import (
+    PEM_CN_ONLY,
+    PEM_DNS_ONLY,
+    PEM_EVERYTHING,
+    PEM_OTHER_NAME,
+)
 
 
 if pytest.importorskip("OpenSSL"):