From 34b8eaf35119193a5efd22577d56de800b5118f1 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sat, 12 Sep 2026 23:10:24 +0300 Subject: [PATCH 1/5] fix: verify_csr compared a rendered string, so a crafted subject slipped through The check was a substring test on the RFC4514 form of the CSR subject and did not look at the signature at all. Both matter more than they look. The subject test accepted a CSR whose CN merely started with the callsign (CN=OTTER1X for OTTER1), and accepted one that carried the callsign in some other attribute while the real CN was something else entirely (OU=CN=OTTER1,CN=ADMIN). The CA signs the request as presented and the mTLS edge authorises on the CN it reads from the certificate, so the second case issues a working identity for a callsign the requester was never granted. Compare the parsed CN attributes instead, and require exactly one equal to the callsign. Other attribute types are still allowed: an MDM filling a subject template may add O or OU and nothing downstream reads them. The missing signature check meant we had no proof the requester held the private key. Worse, an unverifiable request was still passed to the CA, which rejects it only after Person.create_with_cert has committed the Person row, spending that callsign for good. Adds tests/test_csr_utils.py. Every negative case in it was accepted before. --- .../web/api/utils/csr_utils.py | 31 +++++- tests/test_csr_utils.py | 97 +++++++++++++++++++ 2 files changed, 123 insertions(+), 5 deletions(-) create mode 100644 tests/test_csr_utils.py diff --git a/src/rasenmaeher_api/web/api/utils/csr_utils.py b/src/rasenmaeher_api/web/api/utils/csr_utils.py index 836ff504..184b8015 100644 --- a/src/rasenmaeher_api/web/api/utils/csr_utils.py +++ b/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"), diff --git a/tests/test_csr_utils.py b/tests/test_csr_utils.py new file mode 100644 index 00000000..418cb2ae --- /dev/null +++ b/tests/test_csr_utils.py @@ -0,0 +1,97 @@ +"""Unit tests for CSR verification + +No database, no containers: these build certificate requests in memory and check what +``verify_csr`` accepts. Every negative case here was accepted by the previous substring +implementation, which is why they are pinned. +""" + +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") From 641c1369a6ec2329f8745df15f55bf4b63539730 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sat, 12 Sep 2026 23:10:35 +0300 Subject: [PATCH 2/5] chore: bump version --- .bumpversion.toml | 2 +- pyproject.toml | 2 +- src/rasenmaeher_api/__init__.py | 2 +- tests/test_rasenmaeher_api.py | 2 +- uv.lock | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.bumpversion.toml b/.bumpversion.toml index c65cef51..0eeaed75 100644 --- a/.bumpversion.toml +++ b/.bumpversion.toml @@ -1,5 +1,5 @@ [tool.bumpversion] -current_version = "1.19.2+260912" +current_version = "1.19.3+260912" parse = "(?P\\d+)\\.(?P\\d+)\\.(?P\\d+)\\+(?P\\d+)" serialize = ["{major}.{minor}.{patch}+{build}"] search = "{current_version}" diff --git a/pyproject.toml b/pyproject.toml index 4e73826e..7a43f556 100644 --- a/pyproject.toml +++ b/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" }, diff --git a/src/rasenmaeher_api/__init__.py b/src/rasenmaeher_api/__init__.py index 86358cd7..d033b6f0 100644 --- a/src/rasenmaeher_api/__init__.py +++ b/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 diff --git a/tests/test_rasenmaeher_api.py b/tests/test_rasenmaeher_api.py index e674b9ad..5c2d9e3d 100644 --- a/tests/test_rasenmaeher_api.py +++ b/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") diff --git a/uv.lock b/uv.lock index 1822903c..ce3268f0 100644 --- a/uv.lock +++ b/uv.lock @@ -2110,7 +2110,7 @@ wheels = [ [[package]] name = "rasenmaeher-api" -version = "1.19.2+260912" +version = "1.19.3+260912" source = { editable = "." } dependencies = [ { name = "aiodns" }, From 92bcd8a6ae172e08639f2f5c00d089f6476ca16f Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sun, 13 Sep 2026 02:31:41 +0300 Subject: [PATCH 3/5] fix: verify_csr matched the callsign as a substring CN=OTTER1X passed for OTTER1, and OU=CN=OTTER1,CN=ADMIN passed while the cert the CA issued said ADMIN. Now compares parsed CN attributes, and checks the signature so an unverifiable request never reaches the CA. --- .../web/api/utils/csr_utils.py | 31 +++++- tests/test_csr_utils.py | 97 +++++++++++++++++++ 2 files changed, 123 insertions(+), 5 deletions(-) create mode 100644 tests/test_csr_utils.py diff --git a/src/rasenmaeher_api/web/api/utils/csr_utils.py b/src/rasenmaeher_api/web/api/utils/csr_utils.py index 836ff504..184b8015 100644 --- a/src/rasenmaeher_api/web/api/utils/csr_utils.py +++ b/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"), diff --git a/tests/test_csr_utils.py b/tests/test_csr_utils.py new file mode 100644 index 00000000..418cb2ae --- /dev/null +++ b/tests/test_csr_utils.py @@ -0,0 +1,97 @@ +"""Unit tests for CSR verification + +No database, no containers: these build certificate requests in memory and check what +``verify_csr`` accepts. Every negative case here was accepted by the previous substring +implementation, which is why they are pinned. +""" + +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") From 78def9a485d470f695e7a9dc36f0c4227a2ba2a8 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sun, 13 Sep 2026 02:31:41 +0300 Subject: [PATCH 4/5] chore: bump version --- .bumpversion.toml | 2 +- pyproject.toml | 2 +- src/rasenmaeher_api/__init__.py | 2 +- tests/test_rasenmaeher_api.py | 2 +- uv.lock | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.bumpversion.toml b/.bumpversion.toml index c65cef51..0eeaed75 100644 --- a/.bumpversion.toml +++ b/.bumpversion.toml @@ -1,5 +1,5 @@ [tool.bumpversion] -current_version = "1.19.2+260912" +current_version = "1.19.3+260912" parse = "(?P\\d+)\\.(?P\\d+)\\.(?P\\d+)\\+(?P\\d+)" serialize = ["{major}.{minor}.{patch}+{build}"] search = "{current_version}" diff --git a/pyproject.toml b/pyproject.toml index 4e73826e..7a43f556 100644 --- a/pyproject.toml +++ b/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" }, diff --git a/src/rasenmaeher_api/__init__.py b/src/rasenmaeher_api/__init__.py index 86358cd7..d033b6f0 100644 --- a/src/rasenmaeher_api/__init__.py +++ b/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 diff --git a/tests/test_rasenmaeher_api.py b/tests/test_rasenmaeher_api.py index e674b9ad..5c2d9e3d 100644 --- a/tests/test_rasenmaeher_api.py +++ b/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") diff --git a/uv.lock b/uv.lock index 1822903c..ce3268f0 100644 --- a/uv.lock +++ b/uv.lock @@ -2110,7 +2110,7 @@ wheels = [ [[package]] name = "rasenmaeher-api" -version = "1.19.2+260912" +version = "1.19.3+260912" source = { editable = "." } dependencies = [ { name = "aiodns" }, From 9eb21b6259d3b032c518e262485a0db66eaff17f Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sun, 13 Sep 2026 02:33:56 +0300 Subject: [PATCH 5/5] chore: remove useless comments --- tests/test_csr_utils.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/test_csr_utils.py b/tests/test_csr_utils.py index 418cb2ae..30cc9c6a 100644 --- a/tests/test_csr_utils.py +++ b/tests/test_csr_utils.py @@ -1,8 +1,7 @@ """Unit tests for CSR verification -No database, no containers: these build certificate requests in memory and check what -``verify_csr`` accepts. Every negative case here was accepted by the previous substring -implementation, which is why they are pinned. +Build certificate requests in memory and check what +``verify_csr`` accepts. """ import logging