diff --git a/.gitignore b/.gitignore index 6e78dc2..df6305c 100644 --- a/.gitignore +++ b/.gitignore @@ -57,6 +57,7 @@ reports/ local_settings.py db.sqlite3 testapp/simple_certmanager.db +testapp/private-media # Flask stuff: instance/ diff --git a/simple_certmanager/admin.py b/simple_certmanager/admin.py index a4e3888..09511f4 100644 --- a/simple_certmanager/admin.py +++ b/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() diff --git a/simple_certmanager/models.py b/simple_certmanager/models.py index b7129a7..beec8de 100644 --- a/simple_certmanager/models.py +++ b/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 diff --git a/simple_certmanager/utils.py b/simple_certmanager/utils.py index 743ea1f..bcd7869 100644 --- a/simple_certmanager/utils.py +++ b/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. """