Fix/verify csr exact subject

.bumpversion.toml

@@ -1,5 +1,5 @@
 [tool.bumpversion]
-current_version = "1.19.2+260912"
+current_version = "1.19.3+260912"
 parse = "(?P<major>\\d+)\\.(?P<minor>\\d+)\\.(?P<patch>\\d+)\\+(?P<build>\\d+)"
 serialize = ["{major}.{minor}.{patch}+{build}"]
 search = "{current_version}"

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "rasenmaeher_api"
-version = "1.19.2+260912"
+version = "1.19.3+260912"
 description = "python-rasenmaeher-api"
 authors = [
     { name = "Aciid", email = "703382+Aciid@users.noreply.github.com" },

src/rasenmaeher_api/__init__.py

@@ -1,3 +1,3 @@
 """python-rasenmaeher-api"""
 
-__version__ = "1.19.2+260912"  # NOTE Use `bump-my-version` to bump versions correctly
+__version__ = "1.19.3+260912"  # NOTE Use `bump-my-version` to bump versions correctly

src/rasenmaeher_api/web/api/utils/csr_utils.py

@@ -3,6 +3,7 @@
 import logging
 
 from cryptography import x509
+from cryptography.x509.oid import NameOID
 from libadvian.binpackers import ensure_utf8
 
 LOGGER = logging.getLogger(__name__)
@@ -12,12 +13,32 @@
 
 
 def verify_csr(csrpem: str, callsign: str) -> bool:
-    """Verify CSR matches our rules for CN/DN for the given callsign"""
+    """Verify CSR matches our rules for CN/DN for the given callsign
+
+    Two properties are checked, and both are load-bearing:
+
+    1. The CSR is signed by the key it carries. Without this we have no proof that whoever
+       submitted the request holds the private key, and a request we cannot vouch for would be
+       passed to the CA anyway -- which answers with an error *after* the Person row has been
+       committed, permanently spending the callsign.
+    2. The subject carries exactly one CN and it equals the callsign. This used to be a substring
+       test on the rendered RFC4514 string, which accepted a great deal more than it looks like:
+       ``CN=OTTER1X`` matched callsign ``OTTER1``, and ``OU=CN=OTTER1,CN=ADMIN`` matched it too
+       while the certificate the CA then issued carried ``CN=ADMIN``. The mTLS edge authorises on
+       the CN it reads from the certificate, so that is an identity mismatch, not a cosmetic one.
+
+    Attributes other than CN are left alone on purpose: an MDM filling the subject template of a
+    device may add O, OU or ST, nothing downstream reads them, and refusing them would break
+    enrolment for no gain.
+    """
     csr = x509.load_pem_x509_csr(ensure_utf8(csrpem))
-    dn = csr.subject.rfc4514_string()
-    LOGGER.debug(f"DN={dn} callsign={callsign}")
-    if f"CN={callsign}" not in dn:
-        LOGGER.warning(f"Callsign does not match CSR subject. DN={dn} callsign={callsign}")
+    if not csr.is_signature_valid:
+        LOGGER.warning(f"CSR signature does not verify. callsign={callsign}")
+        return False
+    common_names = [attr.value for attr in csr.subject.get_attributes_for_oid(NameOID.COMMON_NAME)]
+    LOGGER.debug(f"CNs={common_names} callsign={callsign}")
+    if common_names != [callsign]:
+        LOGGER.warning(f"Callsign does not match CSR subject. CNs={common_names} callsign={callsign}")
         return False
     # TODO: check that keyusages in the CSR are fine
     #       crypto.X509Extension(b"keyUsage", True, b"digitalSignature,nonRepudiation,keyEncipherment"),

tests/test_csr_utils.py

@@ -0,0 +1,96 @@
+"""Unit tests for CSR verification
+
+Build certificate requests in memory and check what
+``verify_csr`` accepts.
+"""
+
+import logging
+
+import pytest
+from cryptography import x509
+from cryptography.hazmat.primitives import hashes, serialization
+from cryptography.hazmat.primitives.asymmetric import ec, rsa
+from cryptography.x509.oid import NameOID
+
+from rasenmaeher_api.web.api.utils.csr_utils import verify_csr
+
+LOGGER = logging.getLogger(__name__)
+
+
+def _csr(subject: x509.Name, key: object | None = None) -> str:
+    """A signed CSR for the given subject, EC by default"""
+    signing_key = key or ec.generate_private_key(ec.SECP256R1())
+    csr = x509.CertificateSigningRequestBuilder().subject_name(subject).sign(signing_key, hashes.SHA256())
+    return csr.public_bytes(serialization.Encoding.PEM).decode("utf-8")
+
+
+def _cn(value: str) -> x509.NameAttribute:
+    return x509.NameAttribute(NameOID.COMMON_NAME, value)
+
+
+def test_exact_common_name_passes() -> None:
+    """The normal case"""
+    assert verify_csr(_csr(x509.Name([_cn("OTTER1")])), "OTTER1")
+
+
+def test_rsa_key_passes() -> None:
+    """Key type is not what is being checked -- RSA is still what SCEP devices mostly send"""
+    key = rsa.generate_private_key(public_exponent=65537, key_size=2048)
+    assert verify_csr(_csr(x509.Name([_cn("OTTER1")]), key), "OTTER1")
+
+
+def test_extra_non_cn_attributes_pass() -> None:
+    """An MDM may fill O/OU/ST from its own template and nothing downstream reads them"""
+    subject = x509.Name(
+        [
+            x509.NameAttribute(NameOID.ORGANIZATION_NAME, "Example"),
+            x509.NameAttribute(NameOID.ORGANIZATIONAL_UNIT_NAME, "Devices"),
+            _cn("OTTER1"),
+        ]
+    )
+    assert verify_csr(_csr(subject), "OTTER1")
+
+
+def test_longer_common_name_is_refused() -> None:
+    """``CN=OTTER1X`` contains ``CN=OTTER1`` as a substring, which used to be enough"""
+    assert not verify_csr(_csr(x509.Name([_cn("OTTER1X")])), "OTTER1")
+
+
+def test_second_common_name_is_refused() -> None:
+    """The escalation: the issued certificate would carry ADMIN, the Person would be OTTER1"""
+    subject = x509.Name([_cn("ADMIN"), _cn("OTTER1")])
+    assert not verify_csr(_csr(subject), "OTTER1")
+
+
+def test_callsign_smuggled_into_another_attribute_is_refused() -> None:
+    """``OU=CN=OTTER1,CN=ADMIN`` renders a DN containing ``CN=OTTER1`` while the CN is ADMIN"""
+    subject = x509.Name(
+        [
+            x509.NameAttribute(NameOID.ORGANIZATIONAL_UNIT_NAME, "CN=OTTER1"),
+            _cn("ADMIN"),
+        ]
+    )
+    rendered = x509.load_pem_x509_csr(_csr(subject).encode("utf-8")).subject.rfc4514_string()
+    assert "CN=OTTER1" in rendered, "precondition: the old substring test would have accepted this"
+    assert not verify_csr(_csr(subject), "OTTER1")
+
+
+def test_subject_without_common_name_is_refused() -> None:
+    """An empty callsign used to match ``CN=`` in almost anything"""
+    subject = x509.Name([x509.NameAttribute(NameOID.ORGANIZATION_NAME, "Example")])
+    assert not verify_csr(_csr(subject), "")
+
+
+def test_unsigned_csr_is_refused() -> None:
+    """Proof of possession: the request must be signed by the key it carries"""
+    pem = _csr(x509.Name([_cn("OTTER1")]))
+    csr = x509.load_pem_x509_csr(pem.encode("utf-8"))
+    der = bytearray(csr.public_bytes(serialization.Encoding.DER))
+    der[-1] ^= 0xFF  # break the signature, keep the structure parseable
+    try:
+        broken = x509.load_der_x509_csr(bytes(der))
+    except ValueError:
+        pytest.skip("mangled DER no longer parses, cannot exercise the signature path this way")
+    broken_pem = broken.public_bytes(serialization.Encoding.PEM).decode("utf-8")
+    assert not broken.is_signature_valid, "precondition: the signature must actually be invalid"
+    assert not verify_csr(broken_pem, "OTTER1")

tests/test_rasenmaeher_api.py

@@ -15,7 +15,7 @@
 
 def test_version() -> None:
     """Make sure version matches expected"""
-    assert __version__ == "1.19.2+260912"
+    assert __version__ == "1.19.3+260912"
 
 
 @pytest.mark.asyncio(loop_scope="session")