Account for PKCS#7 being unordered

lkubb · lkubb · commit 8cabca17251b · 2026-07-28T16:16:47.000+02:00
diff --git a/changelog/69893.fixed.md b/changelog/69893.fixed.md
@@ -0,0 +1 @@
+Fixed stateful management of PKCS#7 certificates with appended chain using `x509_v2.certificate_managed`. Also fixed loading of PKCS#7-encoded certificate bundles with `salt.utils.x509.load_cert`.
diff --git a/salt/states/x509_v2.py b/salt/states/x509_v2.py
@@ -187,8 +187,11 @@
 import os.path
 from datetime import datetime, timedelta, timezone
 
+import salt.utils.dictupdate
 import salt.utils.files
 import salt.utils.platform
+import salt.utils.stringutils
+import salt.utils.versions
 from salt.exceptions import CommandExecutionError, SaltInvocationError
 from salt.state import STATE_INTERNAL_KEYWORDS as _STATE_INTERNAL_KEYWORDS
 
@@ -505,7 +508,9 @@ def certificate_managed(
 
                 current_chain = current_chain or []
                 ca_chain = [x509util.load_cert(x) for x in append_certs]
-                if not _compare_ca_chain(current_chain, ca_chain):
+                if not _compare_ca_chain(
+                    current_chain, ca_chain, unordered="pkcs7" in current_encoding
+                ):
                     changes["additional_certs"] = True
 
                 (
@@ -1747,9 +1752,13 @@ def getextname(ext):
     return {"added": added, "changed": changed, "removed": removed}
 
 
-def _compare_ca_chain(current, new):
-    if not len(current) == len(new):
+def _compare_ca_chain(current, new, unordered=False):
+    if len(current) != len(new):
         return False
+    if unordered:
+        return {cert.fingerprint(hashes.SHA256()) for cert in new} == {
+            cert.fingerprint(hashes.SHA256()) for cert in current
+        }
     for i, new_cert in enumerate(new):
         if new_cert.fingerprint(hashes.SHA256()) != current[i].fingerprint(
             hashes.SHA256()
diff --git a/salt/utils/x509.py b/salt/utils/x509.py
@@ -869,6 +869,98 @@ def load_pubkey(pk, get_encoding=False):
         raise PubDeserializationError("Could not load DER-encoded public key.") from err
 
 
+def order_certs_naively(bundle, allow_orphans=True, require_leaf=True):
+    """
+    Deterministically order certificates in a bundle using a naive algorithm.
+    This is not a chain building algorithm! It just selects the longest chain
+    of direct certification, preferring leaves by default, and appends all
+    orphans ordered by their fingerprints, if orphans are allowed.
+
+    bundle
+        A set of cryptography.x509.Certificate objects to order.
+
+    allow_orphans
+        Do not require all certificates to build a single chain. Defaults to true.
+
+    require_leaf
+        Require that a path begins with a certificate that itself has not
+        been used to issue another certificate in the bundle. Defaults to true.
+    """
+    if len(bundle) < 2:
+        return list(bundle)
+
+    def _directly_issued_by(subject, issuer):
+        if subject.issuer != issuer.subject:
+            return False
+        try:
+            subject.verify_directly_issued_by(issuer)
+        except (InvalidSignature, TypeError, ValueError):
+            return False
+        return True
+
+    def _fp(cert):
+        return cert.fingerprint(hashes.SHA256())
+
+    ordered_bundle = tuple(sorted(bundle, key=_fp))
+    issuers = {
+        cert: [
+            candidate
+            for candidate in ordered_bundle
+            if _directly_issued_by(cert, candidate)
+        ]
+        for cert in ordered_bundle
+    }
+    if require_leaf:
+        # ensure we treat self-signed root certificates that have not issued another certificate in this bundle as a leaf
+        cert_issuers = {
+            issuer
+            for subject, candidates in issuers.items()
+            for issuer in candidates
+            if issuer != subject
+        }
+        leaves = {cert for cert in ordered_bundle if cert not in cert_issuers}
+        if not leaves:
+            # This would be unusual, but possible when e.g. two certificates signed each other
+            raise ValueError(
+                "Certificate bundle did not contain a single leaf certificate"
+            )
+    else:
+        leaves = {}
+
+    def _paths_from(
+        cert,
+        seen,
+    ):
+        candidates = [issuer for issuer in issuers[cert] if issuer not in seen]
+        if not candidates:
+            return [[cert]]
+        return [
+            [cert, *tail]
+            for issuer in candidates
+            for tail in _paths_from(issuer, seen | {issuer})
+        ]
+
+    paths = [
+        path for cert in ordered_bundle for path in _paths_from(cert, frozenset({cert}))
+    ]
+
+    # Longest path first; fingerprints provide a stable tie-breaker.
+    selected = min(
+        paths,
+        key=lambda path: (
+            -int(path[0] in leaves),
+            -len(path),
+            tuple(_fp(cert) for cert in path),
+        ),
+    )
+    orphans = [cert for cert in ordered_bundle if cert not in selected]
+    if not allow_orphans and orphans:
+        raise ValueError(
+            "Certificate bundle did not contain a singular chain comprising all certificates"
+        )
+    return [*selected, *orphans]
+
+
 def load_cert(cert, passphrase=None, load_chain=False, get_encoding=False):
     """
     Return a certificate instance from
@@ -910,12 +1002,13 @@ def load_cert(cert, passphrase=None, load_chain=False, get_encoding=False):
                 ) from err
         else:
             try:
-                loaded = pkcs7.load_pem_pkcs7_certificates(pems[0])
+                chain = order_certs_naively(pkcs7.load_pem_pkcs7_certificates(pems[0]))
+                loaded = chain.pop(0)  # the first cert is sure to be a leaf
                 if load_chain:
-                    return loaded.pop(0), loaded
+                    return loaded, chain
                 if get_encoding:
-                    return loaded.pop(0), "pkcs7_pem", loaded, None
-                return loaded.pop(0)
+                    return loaded, "pkcs7_pem", chain, None
+                return loaded
             except ValueError as err:
                 raise CertDeserializationError(
                     "Could not load PEM-encoded PKCS#7 blob"
@@ -952,14 +1045,20 @@ def load_cert(cert, passphrase=None, load_chain=False, get_encoding=False):
     # PKCS7
     try:
         # v37+
-        loaded = pkcs7.load_der_pkcs7_certificates(cert)
-        if load_chain:
-            return loaded.pop(0), loaded
-        if get_encoding:
-            return loaded.pop(0), "pkcs7_der", loaded, None
-        return loaded[0]
+        bundle = pkcs7.load_der_pkcs7_certificates(cert)
     except ValueError:
         pass
+    else:
+        try:
+            chain = order_certs_naively(bundle)
+        except ValueError as err:
+            raise CertDeserializationError(str(err)) from err
+        loaded = chain.pop(0)  # the first cert is sure to be a leaf
+        if load_chain:
+            return loaded, chain
+        if get_encoding:
+            return loaded, "pkcs7_der", chain, None
+        return loaded
     # nothing worked
     raise CertDeserializationError(
         "Could not deserialize binary data, neither as DER nor PKCS#7, PKCS#12."
diff --git a/tests/pytests/functional/utils/test_x509.py b/tests/pytests/functional/utils/test_x509.py
@@ -777,3 +777,58 @@ def test_load_cert_pkcs7_orders_chain_with_multiple_valid_paths(
     assert chain[1].subject.rfc4514_string() == chain[0].issuer.rfc4514_string()
     orphan_subjects = {orphan.subject.rfc4514_string() for orphan in chain[2:]}
     assert orphan_subjects == {"CN=Root A", "CN=Intermediate"}
+
+
+@pytest.mark.parametrize(
+    "certs,order",
+    (
+        (["leaf"], ["leaf"]),
+        (["ca_A"], ["ca_A"]),
+        (["leaf", "ca_AI", "ca_A"], ["leaf", "ca_AI", "ca_A"]),
+        (["leaf", "ca_BI", "ca_B"], ["leaf", "ca_BI", "ca_B"]),
+        (
+            ["ca_A", "ca_BI", "ca_B", "ca_AI", "leaf"],
+            ["leaf", "ca_BI", "ca_B", "ca_A", "ca_AI"],
+        ),
+        (["ca_A", "ca_B", "ca_C"], ["ca_C", "ca_B", "ca_A"]),
+        (
+            ["ca_A", "ca_B", "ca_C", "leaf", "ca_AI", "ca_BI", "ca_CI"],
+            ["leaf", "ca_BI", "ca_B", "ca_C", "ca_CI", "ca_A", "ca_AI"],
+        ),
+    ),
+)
+def test_order_certs_naively_works(certs, order, request):
+    bundle = [x509.load_cert(request.getfixturevalue(cert)) for cert in certs]
+    ordered_bundle = [x509.load_cert(request.getfixturevalue(cert)) for cert in order]
+    res = x509.order_certs_naively(bundle)
+    assert res == ordered_bundle
+
+
+@pytest.mark.parametrize(
+    "certs,expected",
+    (
+        (["leaf"], ["leaf"]),
+        (["ca_A"], ["ca_A"]),
+        (["leaf", "ca_AI", "ca_A"], ["leaf", "ca_AI", "ca_A"]),
+        (["leaf", "ca_BI", "ca_B"], ["leaf", "ca_BI", "ca_B"]),
+        (["leaf", "ca_BI", "ca_B", "ca_AI"], False),
+        (
+            ["ca_A", "ca_BI", "ca_B", "ca_AI", "leaf"],
+            False,
+        ),
+        (["ca_A", "ca_B", "ca_C"], False),
+    ),
+)
+def test_order_certs_naively_no_allow_orphans(certs, expected, request):
+    if expected is False:
+        ctx = pytest.raises(ValueError, match=".*did not contain a singular chain.*")
+        ordered_bundle = []
+    else:
+        ordered_bundle = [
+            x509.load_cert(request.getfixturevalue(cert)) for cert in expected
+        ]
+        ctx = contextlib.nullcontext()
+    bundle = [x509.load_cert(request.getfixturevalue(cert)) for cert in certs]
+    with ctx:
+        res = x509.order_certs_naively(bundle, allow_orphans=False)
+        assert res == ordered_bundle

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+Fixed stateful management of PKCS#7 certificates with appended chain using `x509_v2.certificate_managed`. Also fixed loading of PKCS#7-encoded certificate bundles with `salt.utils.x509.load_cert`.