✨ Restrict chain validation to server certificates only

Since these are used for server verification, we check the presence
of SAN extension/extract the DNS name to validate the chain against.

.gitignore

@@ -57,6 +57,7 @@ reports/
 local_settings.py
 db.sqlite3
 testapp/simple_certmanager.db
+testapp/private-media
 
 # Flask stuff:
 instance/

simple_certmanager/admin.py

@@ -5,7 +5,7 @@
 
 from .forms import CertificateAdminForm
 from .models import Certificate
-from .utils import suppress_crypto_errors
+from .utils import suppress_cryptography_errors
 
 
 @admin.register(Certificate)
@@ -33,7 +33,7 @@ def get_label(self, obj):
         return str(obj)
 
     @admin.display(description=_("serial number"))
-    @suppress_crypto_errors
+    @suppress_cryptography_errors
     def serial_number(self, obj: Certificate):
         # alias model property to catch errors
         try:
@@ -42,7 +42,7 @@ def serial_number(self, obj: Certificate):
             return _("file not found")
 
     @admin.display(description=_("expiry date"))
-    @suppress_crypto_errors
+    @suppress_cryptography_errors
     def expiry_date(self, obj: Certificate):
         # alias model property to catch errors
         try:
@@ -51,7 +51,7 @@ def expiry_date(self, obj: Certificate):
             return _("file not found")
 
     @admin.display(description=_("valid key pair"), boolean=True)
-    @suppress_crypto_errors
+    @suppress_cryptography_errors
     def is_valid_key_pair(self, obj: Certificate):
         # alias model property to catch errors
         try:
@@ -60,7 +60,7 @@ def is_valid_key_pair(self, obj: Certificate):
             return None
 
     @admin.display(description=_("valid chain"), boolean=True)
-    @suppress_crypto_errors
+    @suppress_cryptography_errors
     def has_valid_chain(self, obj: Certificate):
         try:
             return obj.has_valid_chain()

simple_certmanager/models.py

@@ -104,7 +104,16 @@ def is_valid_key_pair(self) -> None | bool:
     is_valid_key_pair.boolean = True  # type: ignore
 
     def has_valid_chain(self) -> None | bool:
+        # if we have a public cert / private key combination, then this is *most likely*
+        # used for client authentication in an mTLS context. A client cert does not have
+        # a DNSName, so validating the certificate chain does not make any sense. We
+        # only require server certificate validation. There is no reason why we would
+        # have the private key of a server we're talking to.
+        if self.type != CertificateTypes.cert_only:
+            return None
+
         with self.public_certificate.open(mode="rb") as f:
-            return check_pem(f.read())
+            cert_data: bytes = f.read()
+            return check_pem(cert_data)
 
     has_valid_chain.boolean = True  # type: ignore

simple_certmanager/utils.py

@@ -27,6 +27,26 @@ def pretty_print_certificate_components(x509name: x509.Name) -> str:
     return ", ".join(bits)
 
 
+def extract_dns_names(
+    certificate: x509.Certificate, allow_empty: bool = False
+) -> list[str]:
+    # SubjectAlternativeNames extension is required
+    try:
+        san_extension = certificate.extensions.get_extension_for_oid(
+            x509.OID_SUBJECT_ALTERNATIVE_NAME
+        ).value
+        assert isinstance(san_extension, x509.SubjectAlternativeName)
+    except x509.ExtensionNotFound as exc:
+        raise ValueError(
+            "Certificate is missing the SubjectAlternativeName extension."
+        ) from exc
+
+    dns_names = san_extension.get_values_for_type(x509.DNSName)
+    if not dns_names:
+        raise ValueError("Certificate does not have any DNSName entries.")
+    return dns_names
+
+
 def check_pem(pem: bytes, ca: str | Path = certifi.where()) -> bool:
     """Simple (possibly incomplete) sanity check on pem chain.
 
@@ -44,7 +64,8 @@ def check_pem(pem: bytes, ca: str | Path = certifi.where()) -> bool:
 
     .. todo: The default for ``ca`` should probably support a setting so that
        self_certifi paths/dirs can be taken into account, or maybe consider the envvar
-       ``REQUESTS_CA_BUNDLE``. This will make it possible to support the G1 Private root.
+       ``REQUESTS_CA_BUNDLE``. This will make it possible to support the G1 Private
+       root.
     """
     # normalize to Path
     if isinstance(ca, str):
@@ -59,27 +80,34 @@ def check_pem(pem: bytes, ca: str | Path = certifi.where()) -> bool:
     # extract the DNS name from the leaf certificate - we don't really care about the
     # exact host name for the chain validation since we don't know which hosts will be
     # connected to with this certificate - that only happens at runtime. So, we use the
-    # first entry.
-    # XXX: this can crash if the SAN extension is not present or no DNS names are
-    # specified - it's unclear if those are situations we need to support or not.
-    # Probably the validation of the (leaf) certificate itself should enforce the
-    # presence of this information.
-    san_extension = leaf.extensions.get_extension_for_oid(
-        x509.OID_SUBJECT_ALTERNATIVE_NAME
-    ).value
-    assert isinstance(san_extension, x509.SubjectAlternativeName)
-    dns_names = san_extension.get_values_for_type(x509.DNSName)
+    # first available entry.
+    try:
+        dns_names = extract_dns_names(leaf)
+    except ValueError as exc:
+        # ValueError: Certificate is missing the SubjectAlternativeName extension
+        logger.info(
+            "Could not extract DNS name from (leaf) certificate data (got error %r)",
+            exc,
+            exc_info=exc,
+        )
+        return False
+
     dns_name = x509.DNSName(dns_names[0])
     verifier = builder.build_server_verifier(dns_name)
 
     try:
         verifier.verify(leaf, intermediates)
         return True
-    except VerificationError:
+    except VerificationError as exc:
+        logger.info(
+            "Invalid certificate chain detected, verification error is: %r",
+            exc,
+            exc_info=exc,
+        )
         return False
 
 
-def suppress_crypto_errors(func):
+def suppress_cryptography_errors(func):
     """
     Decorator to suppress exceptions thrown while processing PKI data.
     """