From 34b8eaf35119193a5efd22577d56de800b5118f1 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sat, 12 Sep 2026 23:10:24 +0300 Subject: [PATCH 01/18] 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 02/18] 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 03/18] 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 04/18] 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 05/18] 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 From d6c368d525b846a2bd87b8e00df818bec6e69786 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sat, 12 Sep 2026 23:33:03 +0300 Subject: [PATCH 06/18] feat: let an MDM agent complete device enrollments an admin planned Enrolling three hundred phones through an MDM needs something to approve them, and the proof of concept did it by holding an admin certificate that could create and revoke anyone. This is the narrow version of that capability. An admin plans a device by calling /enrollment/init with mdm=true. The row is an ordinary pending enrollment carrying a marker, so it lists, locks and audits like every other one, and no new storage appears. No JWT is issued for it: that token is a live credential for the callsign, the device will never use it, and issuing it also overwrote the planning admin's own session cookie. The device's SCEP agent then calls /enrollment/accept with the CSR the device generated. It authenticates with its own client certificate from the deployment CA, and RM_MDM_AGENT_CNS says which CN may do this. That list is deliberately not the kraftwerk product CNs: those skip role checks, can have any CSR signed and cannot be revoked at the edge, which is far more than this needs. Empty by default, so the path does not exist until a deployment configures it. Approving a planned device by hand is refused with 409. It would generate the keypair here and the callsign would be spent, leaving the phone unable to ever have it. Three things that would otherwise be silent: - The claim is one conditional UPDATE, so two devices racing for a callsign produce one winner, and the row is re-read before approve() because approve() reads the CSR off the instance and would otherwise generate a keypair here. - Issuing is a network call to the CA. If it fails the claim is released so the next attempt can have the callsign back. - A repeat of a request we already answered returns the same certificate rather than 'callsign already taken', because the reply can be lost on the way back to the MDM. If the Person exists but has no certificate the callsign was spent by a failed issue and the agent is told so. Refusals all carry one detail so the agent cannot enumerate callsigns; the real reason goes to the audit log. Needs the verify_csr fix in the parent commit: without it the CSR subject is matched by substring, which is the only subject gate on this path. --- src/rasenmaeher_api/db/enrollments.py | 77 ++++++- src/rasenmaeher_api/rmsettings.py | 12 ++ .../web/api/enrollment/schema.py | 40 +++- .../web/api/enrollment/views.py | 203 ++++++++++++++++-- tests/test_enrollment.py | 172 +++++++++++++++ 5 files changed, 483 insertions(+), 21 deletions(-) diff --git a/src/rasenmaeher_api/db/enrollments.py b/src/rasenmaeher_api/db/enrollments.py index a07b02c4..600c3bc1 100644 --- a/src/rasenmaeher_api/db/enrollments.py +++ b/src/rasenmaeher_api/db/enrollments.py @@ -8,11 +8,12 @@ import uuid import warnings from collections.abc import AsyncGenerator -from typing import Any +from typing import Any, cast from sqlalchemy.dialects.postgresql import JSONB -from sqlalchemy.sql import func -from sqlmodel import Field, select +from sqlalchemy.engine import CursorResult +from sqlalchemy.sql import func, update +from sqlmodel import Field, col, select from ..rmsettings import RMSettings from ..web.api.utils.csr_utils import verify_csr @@ -157,7 +158,7 @@ class Enrollment(ORMBaseModel, table=True): approvecode: str = Field(nullable=False, index=True, unique=True) callsign: str = Field(nullable=False, index=True, unique=True) decided_on: datetime.datetime = Field(nullable=True, default=None) - decided_by: uuid.UUID = Field( + decided_by: uuid.UUID | None = Field( foreign_key=f"{ORMBaseModel.__table_args__['schema']}.users.pk", nullable=True, default=None ) person: uuid.UUID = Field( @@ -178,12 +179,74 @@ async def by_pk_or_callsign(cls, inval: str | uuid.UUID) -> "Enrollment": except ValueError: return await cls.by_callsign(str(inval)) - async def approve(self, approver: Person) -> Person: - """Creates the person record, their certs etc""" + @property + def planned_for_mdm(self) -> bool: + """Was this enrollment planned by an admin for a device that enrols through an MDM + + Set by POST /enrollment/init with mdm=true. Such a row waits for the device to present its + own CSR through an MDM agent; approving it by hand would generate a keypair on the server + and spend the callsign, so /enrollment/accept refuses that. + """ + return bool(self.extra) and self.extra.get("mdm") is True + + async def claim_with_csr(self, csrpem: str) -> bool: + """Attach a device CSR to this pending enrollment, once + + Returns False if somebody got there first. The check and the write are one statement on + purpose: two devices racing for the same planned callsign must not both proceed, and the + loser has to find out before any certificate is issued. + + NOTE: refresh (or re-read) the instance before calling approve(); approve() reads self.csr + off the in-memory object and would otherwise take the "no CSR" branch and generate a + keypair here on the server. + """ + with EngineWrapper.get_session() as session: + statement = ( + update(Enrollment) + .where( + col(Enrollment.pk) == self.pk, + col(Enrollment.csr).is_(None), + col(Enrollment.state) == EnrollmentState.PENDING, + col(Enrollment.decided_by).is_(None), + ) + .values(csr=csrpem) + ) + result = cast(CursorResult[Any], session.execute(statement)) + session.commit() + if result.rowcount != 1: + LOGGER.warning(f"Enrollment {self.pk} was already claimed") + return False + self.csr = csrpem + return True + + async def release_claim(self) -> None: + """Undo claim_with_csr + + Issuing can fail after the claim (the CA is a network call), and a claimed row with no + certificate is a callsign nobody can ever use. Releasing lets the device's next attempt + run. It does not help if a Person row was already committed -- see the caller. + """ + with EngineWrapper.get_session() as session: + statement = ( + update(Enrollment) + .where(col(Enrollment.pk) == self.pk, col(Enrollment.state) == EnrollmentState.PENDING) + .values(csr=None) + ) + session.execute(statement) + session.commit() + self.csr = None + + async def approve(self, approver: Person | None) -> Person: + """Creates the person record, their certs etc + + approver is None when an MDM agent completed a device enrolment the admin had already + planned: the agent is a service identity, not a Person, and decided_by is nullable. The + audit trail carries the agent's CN instead. + """ with EngineWrapper.get_session() as session: person = await Person.create_with_cert(self.callsign, extra=self.extra, csrpem=self.csr) self.state = EnrollmentState.APPROVED - self.decided_by = approver.pk + self.decided_by = approver.pk if approver else None self.decided_on = datetime.datetime.now(datetime.UTC) self.person = person.pk session.add(self) diff --git a/src/rasenmaeher_api/rmsettings.py b/src/rasenmaeher_api/rmsettings.py index 4a907141..ec4887a9 100644 --- a/src/rasenmaeher_api/rmsettings.py +++ b/src/rasenmaeher_api/rmsettings.py @@ -118,6 +118,13 @@ def log_level_int(self) -> int: # (suitable for in-cluster-only Service exposure during local dev). callsign_validity_secret: str | None = None + # Service identities allowed to complete device enrolments an admin planned with mdm=true, + # comma separated. Empty by default: until a deployment names its MDM agent here, the agent + # path in POST /enrollment/accept does not exist. Deliberately NOT the kraftwerk product CNs: + # those are exempt from role checks, can have any CSR signed and cannot be revoked at the + # edge, which is far more than completing a planned enrolment needs. + mdm_agent_cns: str = "" + persistent_data_dir: str = "/data/persistent" # mtls @@ -182,6 +189,11 @@ def deployment_name(self) -> str: LOGGER.warning("DNS name not defined") return "undefined" + @property + def mdm_agent_cn_set(self) -> set[str]: + """CNs allowed to act as an MDM enrolment agent""" + return {cnname.strip() for cnname in self.mdm_agent_cns.split(",") if cnname.strip()} + @property def valid_product_cns(self) -> list[str]: """Get valid CNs for productapi certs""" diff --git a/src/rasenmaeher_api/web/api/enrollment/schema.py b/src/rasenmaeher_api/web/api/enrollment/schema.py index b1fd2d91..7b35b212 100644 --- a/src/rasenmaeher_api/web/api/enrollment/schema.py +++ b/src/rasenmaeher_api/web/api/enrollment/schema.py @@ -2,6 +2,7 @@ from typing import Any +from libpvarki.schemas.generic import OperationResultResponse from pydantic import BaseModel, ConfigDict, Field @@ -117,6 +118,13 @@ class EnrollmentInitIn(BaseModel): callsign: str = Field(description="Callsign to create enrollment for") csr: str | None = Field(description="CSR for mTLS key in PEM format", default=None) + mdm: bool = Field( + description=( + "Mark this as a device enrolment planned for an MDM. The device supplies its own CSR later and an " + "MDM agent completes the enrolment; no JWT is issued to the caller and approving it by hand is refused." + ), + default=False, + ) class EnrollmentInitOut(BaseModel): @@ -136,7 +144,10 @@ class EnrollmentInitOut(BaseModel): callsign: str = Field(description="Callsign for which the enrollment got initialized") approvecode: str = Field(description="Code used to approve the enrollment, must be delivered to an admin") jwt: str = Field( - description="JWT that allows client to check enrollment approval status and fetc mTLS certs when approved" + description=( + "JWT that allows client to check enrollment approval status and fetc mTLS certs when approved. " + "Empty for enrolments planned for an MDM: nobody should be holding a device's credential." + ) ) @@ -191,7 +202,18 @@ class EnrollmentAcceptIn(BaseModel): callsign: str = Field(description="Callsign to approve") approvecode: str = Field( - description="Approval code for the callsign, this must have been delivered by the person to be enrolled" + description=( + "Approval code for the callsign, this must have been delivered by the person to be enrolled. " + "Not used when an MDM agent completes a planned device enrolment: there is no human to deliver it, " + "and the agent has authenticated with its own certificate." + ), + default="", + ) + csr: str | None = Field( + description=( + "CSR the device generated, PEM. Only for an MDM agent completing an enrolment planned with mdm=true." + ), + default=None, ) @@ -210,6 +232,20 @@ class EnrollmentAcceptOut(BaseModel): callsign: str +class EnrollmentAcceptResultOut(OperationResultResponse): + """Result of accepting an enrollment + + Additive superset of the plain result: an MDM agent gets the issued certificate back here + rather than fetching it afterwards. It has no user credential to fetch it with, and in a + meshed deployment its own service identity would win over any bearer token anyway. + """ + + certificate: str | None = Field( + description="Issued certificate, PEM. Only set when an MDM agent completed the enrolment.", + default=None, + ) + + class EnrollmentPromoteIn(BaseModel): """Enrollment promote in schema""" diff --git a/src/rasenmaeher_api/web/api/enrollment/views.py b/src/rasenmaeher_api/web/api/enrollment/views.py index 4117b4a2..9b735635 100644 --- a/src/rasenmaeher_api/web/api/enrollment/views.py +++ b/src/rasenmaeher_api/web/api/enrollment/views.py @@ -4,19 +4,24 @@ import uuid from typing import Any +from cryptography import x509 +from cryptography.hazmat.primitives import serialization from fastapi import APIRouter, Body, Depends, HTTPException, Request, Response from libpvarki.schemas.generic import OperationResultResponse from multikeyjwt import Issuer from multikeyjwt import config as jwtconfig -from ....db import Enrollment, EnrollmentPool, Person -from ....db.errors import NotFound +from ....db import Enrollment, EnrollmentPool, EnrollmentState, Person +from ....db.errors import Deleted, NotFound from ....rmsettings import RMSettings +from ..middleware.datatypes import MTLSorJWTPayloadType from ..middleware.mtls import MTLSorJWT from ..middleware.user import ValidUser from ..utils.auditcontext import build_audit_extra +from ..utils.csr_utils import verify_csr from .schema import ( EnrollmentAcceptIn, + EnrollmentAcceptResultOut, EnrollmentDemoteIn, EnrollmentGenVerifiOut, EnrollmentHaveIBeenAcceptedOut, @@ -219,10 +224,13 @@ async def request_enrollment_init( # TODO ADD POOL NAME CHECK - new_enrollment = await Enrollment.create_for_callsign(callsign=callsign, pool=None, extra={}, csr=request_in.csr) - # Create JWT token for user - claims = {"sub": callsign} - new_jwt = issue_enrollment_jwt(response, claims) + extra: dict[str, Any] = {"mdm": True} if request_in.mdm else {} + new_enrollment = await Enrollment.create_for_callsign(callsign=callsign, pool=None, extra=extra, csr=request_in.csr) + # Create JWT token for user. Never for a device planned for MDM enrolment: that token is a live + # credential for the callsign, the device will never use it (it receives a certificate through + # the agent), and issuing it here also overwrites the calling admin's own session cookie, which + # is unhelpful when planning three hundred of them. + new_jwt = "" if request_in.mdm else issue_enrollment_jwt(response, {"sub": callsign}) LOGGER.audit( # type: ignore[attr-defined] "Enrollment initiated by admin", @@ -231,6 +239,7 @@ async def request_enrollment_init( outcome="success", target=callsign, request=request, + mdm=request_in.mdm, ), ) @@ -397,23 +406,72 @@ async def request_enrollment_lock( return OperationResultResponse(success=True, extra="Lock task done") +# One detail for every MDM refusal. The agent sees only "no", so a caller holding the agent +# certificate cannot use the reply to learn which callsigns exist or which are planned. The real +# reason goes to the audit log. +MDM_REFUSED = "Not a device enrolment this agent may complete" + + +def mdm_agent_cn(request: Request) -> str | None: + """The caller's CN if it is a configured MDM enrolment agent, otherwise None + + The agent authenticates with its own client certificate from the deployment CA -- in a meshed + deployment, with its mesh identity, which reaches us through the same header. It is not a + Person and holds no role. The single thing this CN may do is complete a device enrolment an + admin has already planned. + """ + payload = getattr(request.state, "mtls_or_jwt", None) + if not payload or payload.type != MTLSorJWTPayloadType.MTLS or not payload.userid: + return None + if payload.userid not in RMSettings.singleton().mdm_agent_cn_set: + return None + return str(payload.userid) + + +def same_public_key(csrpem: str, certpem: str) -> bool: + """Does this CSR carry the key that was certified""" + csr = x509.load_pem_x509_csr(csrpem.encode("utf-8")) + cert = x509.load_pem_x509_certificate(certpem.encode("utf-8")) + encoding, fmt = serialization.Encoding.DER, serialization.PublicFormat.SubjectPublicKeyInfo + return csr.public_key().public_bytes(encoding, fmt) == cert.public_key().public_bytes(encoding, fmt) + + @ENROLLMENT_ROUTER.post( "/accept", - response_model=OperationResultResponse, - dependencies=[Depends(ValidUser(auto_error=True, require_roles=["admin"]))], + response_model=EnrollmentAcceptResultOut, ) async def post_enrollment_accept( request: Request, request_in: EnrollmentAcceptIn = Body(), -) -> OperationResultResponse: +) -> EnrollmentAcceptResultOut: """ Accept callsign_hash (callsign/enrollment) + + Two kinds of caller. An admin approving a person by hand, which is unchanged. Or the MDM + enrolment agent completing a device enrolment an admin planned, supplying the CSR the device + generated. Authorisation for the second is the agent's own certificate, checked here rather + than inferred from where the request came from. """ + agent_cn = mdm_agent_cn(request) + if agent_cn: + return await accept_as_mdm_agent(request, request_in, agent_cn) + admin_user = await ValidUser(auto_error=True, require_roles=["admin"])(request) + if not admin_user: + # ValidUser returns None without a Person for product certificates; they have no business + # approving enrollments and used to fall through to a confusing 404. + raise HTTPException(status_code=403, detail="Not authenticated") + return await accept_as_admin(request, request_in, admin_user) + + +async def accept_as_admin( + request: Request, + request_in: EnrollmentAcceptIn, + admin_user: Person, +) -> EnrollmentAcceptResultOut: + """An admin approves an enrollment by hand, with the code the enrollee gave them""" target_callsign = request_in.callsign - actor = request.state.mtls_or_jwt.userid try: - admin_user = await Person.by_callsign(callsign=actor) pending_enrollment = await Enrollment.by_callsign(callsign=target_callsign) except NotFound as exc: LOGGER.audit( # type: ignore[attr-defined] @@ -441,6 +499,25 @@ async def post_enrollment_accept( ) raise HTTPException(status_code=403, detail="Invalid approval code for this enrollment") + if pending_enrollment.planned_for_mdm and not pending_enrollment.csr: + # Approving it here would have rasenmaeher generate the keypair, and the device could then + # never have this callsign: callsigns are unique and are never released. + LOGGER.audit( # type: ignore[attr-defined] + "Enrollment approval refused - planned for MDM", + extra=build_audit_extra( + action="enrollment_approve", + outcome="failure", + actor=admin_user.callsign, + target=target_callsign, + request=request, + error_code="PLANNED_FOR_MDM", + ), + ) + raise HTTPException( + status_code=409, + detail="This device enrolls itself through the MDM, approving it by hand would spend the callsign", + ) + new_approved_user = await pending_enrollment.approve(approver=admin_user) LOGGER.audit( # type: ignore[attr-defined] @@ -453,7 +530,109 @@ async def post_enrollment_accept( ), ) - return OperationResultResponse(success=True, extra=f"Approved {new_approved_user.callsign}") + return EnrollmentAcceptResultOut(success=True, extra=f"Approved {new_approved_user.callsign}") + + +async def accept_as_mdm_agent( # pylint: disable=too-many-return-statements + request: Request, + request_in: EnrollmentAcceptIn, + agent_cn: str, +) -> EnrollmentAcceptResultOut: + """The MDM agent completes a device enrollment an admin planned + + Everything is checked before the enrollment is claimed, so a request we are going to refuse + never consumes the callsign. + """ + target_callsign = request_in.callsign + + def refuse(status_code: int, error_code: str, detail: str = MDM_REFUSED) -> HTTPException: + LOGGER.audit( # type: ignore[attr-defined] + "MDM enrollment refused", + extra=build_audit_extra( + action="enrollment_mdm_accept", + outcome="failure", + actor=agent_cn, + target=target_callsign, + request=request, + error_code=error_code, + ), + ) + return HTTPException(status_code=status_code, detail=detail) + + csrpem = request_in.csr + if not csrpem: + raise refuse(400, "CSR_MISSING", "The device CSR is required") + + try: + planned = await Enrollment.by_callsign(callsign=target_callsign) + except NotFound as exc: + raise refuse(403, "NOT_PLANNED") from exc + if not planned.planned_for_mdm: + raise refuse(403, "NOT_PLANNED_FOR_MDM") + if not verify_csr(csrpem, planned.callsign): + raise refuse(403, "CSR_SUBJECT") + + if planned.state != EnrollmentState.PENDING or planned.csr: + return await mdm_repeat(request, csrpem, planned, refuse) + + if not await planned.claim_with_csr(csrpem): + raise refuse(403, "ALREADY_CLAIMED") + # Re-read: approve() takes the CSR off the instance, and the claim was a separate statement. + claimed = await Enrollment.by_callsign(callsign=target_callsign) + try: + person = await claimed.approve(approver=None) + except Exception: + # Issuing is a network call to the CA. Let the device's next attempt have the callsign + # back -- unless a Person row was already committed, which mdm_repeat then reports. + await claimed.release_claim() + raise + + LOGGER.audit( # type: ignore[attr-defined] + "Enrollment completed by MDM agent", + extra=build_audit_extra( + action="enrollment_mdm_accept", + outcome="success", + actor=agent_cn, + target=target_callsign, + request=request, + ), + ) + return EnrollmentAcceptResultOut( + success=True, + extra=f"Approved {person.callsign}", + certificate=person.certfile.read_text(encoding="utf-8"), + ) + + +async def mdm_repeat( + request: Request, + csrpem: str, + planned: Enrollment, + refuse: Any, +) -> EnrollmentAcceptResultOut: + """The agent asked again for a callsign that is no longer waiting + + Almost always the reply was lost on the way back and the MDM repeated the identical request, + so hand the same certificate over again -- it is public, and the alternative is a device + stranded behind "callsign already taken". + """ + _ = request + try: + person = await Person.by_callsign(planned.callsign) + except (NotFound, Deleted) as exc: + raise refuse(403, "ALREADY_DECIDED") from exc + if not person.certfile.exists(): + # The Person row is committed before the CA is called and that commit cannot be rolled + # back, so a CA failure at just the wrong moment leaves the callsign unusable for good. + raise refuse(409, "CALLSIGN_SPENT", "That callsign was spent by a failed issue, plan another") + certpem = person.certfile.read_text(encoding="utf-8") + if not same_public_key(csrpem, certpem): + raise refuse(403, "DIFFERENT_KEY") + return EnrollmentAcceptResultOut( + success=True, + extra=f"Approved {person.callsign}", + certificate=certpem, + ) @ENROLLMENT_ROUTER.post( diff --git a/tests/test_enrollment.py b/tests/test_enrollment.py index f9ae9df7..c78acc62 100644 --- a/tests/test_enrollment.py +++ b/tests/test_enrollment.py @@ -3,17 +3,22 @@ import logging import secrets import uuid +from collections.abc import AsyncGenerator from pathlib import Path from typing import Any import cryptography.hazmat.primitives.serialization.pkcs12 import pytest +import pytest_asyncio from async_asgi_testclient import TestClient # type: ignore[import-untyped] from cryptography import x509 +from cryptography.hazmat.primitives import serialization +from fastapi import FastAPI from libpvarki.mtlshelp.csr import async_create_client_csr, async_create_keypair from rasenmaeher_api.db import ( EngineWrapper, + Enrollment, EnrollmentPool, Person, ) @@ -730,3 +735,170 @@ async def test_enrollmentpools_revoked_creator(dbinit_func, tilauspalvelu_jwt_ad found = True break assert found + + +MDM_AGENT_CN = "rmscep-test" + + +@pytest_asyncio.fixture(scope="function") +async def mdm_agent_client(app_instance: FastAPI) -> AsyncGenerator[TestClient, None]: + """Client authenticating as the MDM enrollment agent + + The agent holds its own client certificate from the deployment CA; in a real deployment the + front proxy puts its DN in this header exactly like it does for a browser. + """ + settings = RMSettings.singleton() + previous = settings.mdm_agent_cns + settings.mdm_agent_cns = MDM_AGENT_CN + async with TestClient(app_instance) as instance: + instance.headers.update({"X-ClientCert-DN": f"CN={MDM_AGENT_CN},O=N/A"}) + yield instance + settings.mdm_agent_cns = previous + + +async def _device_csr(tempdir: Path, callsign: str, name: str = "device") -> str: + """A CSR the way a device would send one: its own key, its callsign as the CN""" + privkeyfile = Path(tempdir) / f"{name}.key" + pubkeyfile = Path(tempdir) / f"{name}.pub" + csrfile = Path(tempdir) / f"{name}.csr" + ckp = await async_create_keypair(privkeyfile, pubkeyfile) + return str(await async_create_client_csr(ckp, csrfile, {"CN": callsign})) + + +def _public_key_der(pem: str) -> bytes: + """SubjectPublicKeyInfo of whatever this PEM is, certificate or request""" + encoding, fmt = serialization.Encoding.DER, serialization.PublicFormat.SubjectPublicKeyInfo + if "CERTIFICATE REQUEST" in pem: + return x509.load_pem_x509_csr(pem.encode("utf-8")).public_key().public_bytes(encoding, fmt) + return x509.load_pem_x509_certificate(pem.encode("utf-8")).public_key().public_bytes(encoding, fmt) + + +@pytest.mark.asyncio(loop_scope="session") +async def test_mdm_planned_enrollment_is_not_approvable_by_hand( + tilauspalvelu_jwt_admin_client: TestClient, +) -> None: + """An admin planning a device must not be able to approve it and spend the callsign""" + callsign = f"mdmplanned_{secrets.token_hex(4)}" + resp = await tilauspalvelu_jwt_admin_client.post( + "/api/v1/enrollment/init", json={"callsign": callsign, "mdm": True} + ) + assert resp.status_code == 200 + planned = resp.json() + assert planned["callsign"] == callsign + # No credential for the device is handed to the admin planning it + assert planned["jwt"] == "" + + resp = await tilauspalvelu_jwt_admin_client.post( + "/api/v1/enrollment/accept", json={"callsign": callsign, "approvecode": planned["approvecode"]} + ) + assert resp.status_code == 409 + # and the enrollment is untouched, so the device can still take it + still_pending = await Enrollment.by_callsign(callsign) + assert still_pending.state == 0 + assert still_pending.csr is None + + +@pytest.mark.asyncio(loop_scope="session") +async def test_mdm_agent_completes_planned_enrollment( + tilauspalvelu_jwt_admin_client: TestClient, mdm_agent_client: TestClient, nice_tmpdir: str +) -> None: + """The whole agent path: plan, complete with the device CSR, and survive a repeat""" + tempdir = Path(nice_tmpdir) + callsign = f"mdmdevice_{secrets.token_hex(4)}" + resp = await tilauspalvelu_jwt_admin_client.post( + "/api/v1/enrollment/init", json={"callsign": callsign, "mdm": True} + ) + assert resp.status_code == 200 + + csrpem = await _device_csr(tempdir, callsign) + resp = await mdm_agent_client.post("/api/v1/enrollment/accept", json={"callsign": callsign, "csr": csrpem}) + assert resp.status_code == 200 + body = resp.json() + assert body["success"] is True + certpem = body["certificate"] + assert certpem + + # The certificate must be for the key the device holds. If this ever regresses, rasenmaeher + # generated a keypair of its own and the device received a certificate it cannot use. + assert _public_key_der(certpem) == _public_key_der(csrpem) + assert [attr.value for attr in x509.load_pem_x509_certificate(certpem.encode("utf-8")).subject] == [callsign] + + # The reply can be lost on the way back to the MDM, which then repeats the request verbatim + resp = await mdm_agent_client.post("/api/v1/enrollment/accept", json={"callsign": callsign, "csr": csrpem}) + assert resp.status_code == 200 + assert resp.json()["certificate"] == certpem + + # ...but a different key for the same callsign is somebody else + other_csr = await _device_csr(tempdir, callsign, name="imposter") + resp = await mdm_agent_client.post("/api/v1/enrollment/accept", json={"callsign": callsign, "csr": other_csr}) + assert resp.status_code == 403 + + +@pytest.mark.asyncio(loop_scope="session") +async def test_mdm_agent_refusals( + tilauspalvelu_jwt_admin_client: TestClient, + mdm_agent_client: TestClient, + unauth_client_session: TestClient, + nice_tmpdir: str, +) -> None: + """What the agent may not do""" + tempdir = Path(nice_tmpdir) + + # A callsign nobody planned + unplanned = f"mdmunplanned_{secrets.token_hex(4)}" + csrpem = await _device_csr(tempdir, unplanned, name="unplanned") + resp = await mdm_agent_client.post("/api/v1/enrollment/accept", json={"callsign": unplanned, "csr": csrpem}) + assert resp.status_code == 403 + with pytest.raises(Exception): # noqa: B017 -- NotFound, and nothing was written + await Enrollment.by_callsign(unplanned) + + # A CSR for a different callsign than the one being completed + planned = f"mdmmismatch_{secrets.token_hex(4)}" + resp = await tilauspalvelu_jwt_admin_client.post("/api/v1/enrollment/init", json={"callsign": planned, "mdm": True}) + assert resp.status_code == 200 + wrong_csr = await _device_csr(tempdir, f"{planned}X", name="mismatch") + resp = await mdm_agent_client.post("/api/v1/enrollment/accept", json={"callsign": planned, "csr": wrong_csr}) + assert resp.status_code == 403 + assert (await Enrollment.by_callsign(planned)).csr is None + + # No CSR at all + resp = await mdm_agent_client.post("/api/v1/enrollment/accept", json={"callsign": planned}) + assert resp.status_code == 400 + + # And without the agent certificate the route is what it always was + resp = await unauth_client_session.post("/api/v1/enrollment/accept", json={"callsign": planned, "csr": wrong_csr}) + assert resp.status_code == 403 + + # An enrollment a human started: not the agent's business. + # A plain init hands the caller the new callsign's JWT as a session cookie, and JWTBearer + # prefers that cookie over the Authorization header, so this client would stop being an admin. + # Clean up after it here; not issuing it at all is exactly what the mdm form is for. + human = f"mdmhuman_{secrets.token_hex(4)}" + resp = await tilauspalvelu_jwt_admin_client.post("/api/v1/enrollment/init", json={"callsign": human}) + assert resp.status_code == 200 + if tilauspalvelu_jwt_admin_client.cookie_jar is not None: + tilauspalvelu_jwt_admin_client.cookie_jar.clear() + human_csr = await _device_csr(tempdir, human, name="human") + resp = await mdm_agent_client.post("/api/v1/enrollment/accept", json={"callsign": human, "csr": human_csr}) + assert resp.status_code == 403 + assert (await Enrollment.by_callsign(human)).csr is None + + +@pytest.mark.asyncio(loop_scope="session") +async def test_planning_many_devices_keeps_the_admin_session( + tilauspalvelu_jwt_admin_client: TestClient, +) -> None: + """Planning a fleet is one call per device and must not log the admin out on the first one + + A plain /enrollment/init issues the new callsign's JWT and sets it as the session cookie of + whoever called it, and JWTBearer prefers that cookie over the Authorization header -- so the + next call would arrive as a freshly created non-admin user. Devices planned for MDM get no JWT + at all, nobody should be holding a device's credential anyway, so this stays an admin. + """ + for _ in range(3): + callsign = f"mdmfleet_{secrets.token_hex(4)}" + resp = await tilauspalvelu_jwt_admin_client.post( + "/api/v1/enrollment/init", json={"callsign": callsign, "mdm": True} + ) + assert resp.status_code == 200 + assert resp.json()["jwt"] == "" From 099cc62f37110860652379c6bb67866bed29869a Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sat, 12 Sep 2026 23:33:03 +0300 Subject: [PATCH 07/18] 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 0eeaed75..f358bed6 100644 --- a/.bumpversion.toml +++ b/.bumpversion.toml @@ -1,5 +1,5 @@ [tool.bumpversion] -current_version = "1.19.3+260912" +current_version = "1.20.0+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 7a43f556..454b1c48 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "rasenmaeher_api" -version = "1.19.3+260912" +version = "1.20.0+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 d033b6f0..89c55d91 100644 --- a/src/rasenmaeher_api/__init__.py +++ b/src/rasenmaeher_api/__init__.py @@ -1,3 +1,3 @@ """python-rasenmaeher-api""" -__version__ = "1.19.3+260912" # NOTE Use `bump-my-version` to bump versions correctly +__version__ = "1.20.0+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 5c2d9e3d..7770a060 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.3+260912" + assert __version__ == "1.20.0+260912" @pytest.mark.asyncio(loop_scope="session") diff --git a/uv.lock b/uv.lock index ce3268f0..58b77306 100644 --- a/uv.lock +++ b/uv.lock @@ -2110,7 +2110,7 @@ wheels = [ [[package]] name = "rasenmaeher-api" -version = "1.19.3+260912" +version = "1.20.0+260912" source = { editable = "." } dependencies = [ { name = "aiodns" }, From ebd05aa4ac4c2e7266a4dd2d70699aea8a5f7b6a Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sun, 13 Sep 2026 04:18:36 +0300 Subject: [PATCH 08/18] fix: reserve MDM agent CNs against being taken as callsigns A certificate carrying the agent CN is accepted as the agent on sight, with no Person, role or product check, so anyone issued one could complete every planned device enrolment. Product CNs were already reserved here; these were not, and the callsign is caller-chosen on the unauthenticated invite-code route. --- src/rasenmaeher_api/db/enrollments.py | 5 ++++- src/rasenmaeher_api/db/people.py | 4 ++++ tests/test_enrollment.py | 22 ++++++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/rasenmaeher_api/db/enrollments.py b/src/rasenmaeher_api/db/enrollments.py index 600c3bc1..dd9d6110 100644 --- a/src/rasenmaeher_api/db/enrollments.py +++ b/src/rasenmaeher_api/db/enrollments.py @@ -344,8 +344,11 @@ async def create_for_callsign( csr: str | None = None, ) -> "Enrollment": """Create a new one with random code for the callsign""" - if callsign in RMSettings.singleton().valid_product_cns: + cnf = RMSettings.singleton() + if callsign in cnf.valid_product_cns: raise CallsignReserved("Using product CNs as callsigns is forbidden") + if callsign.lower() in {cnname.lower() for cnname in cnf.mdm_agent_cn_set}: + raise CallsignReserved("Using MDM agent CNs as callsigns is forbidden") if csr and not verify_csr(csr, callsign): raise CallsignReserved("CSR CN must match callsign") with EngineWrapper.get_session() as session: diff --git a/src/rasenmaeher_api/db/people.py b/src/rasenmaeher_api/db/people.py index 086c3ffc..557a4d4d 100644 --- a/src/rasenmaeher_api/db/people.py +++ b/src/rasenmaeher_api/db/people.py @@ -103,6 +103,10 @@ async def create_with_cert( raise CallsignReserved("Using product CNs as callsigns is forbidden") if callsign == cnf.mtls_client_cert_cn: raise CallsignReserved("Using the rasenmaeher service CN as a callsign is forbidden") + if callsign.lower() in {cnname.lower() for cnname in cnf.mdm_agent_cn_set}: + # A certificate with this CN is accepted as the MDM enrolment agent on sight, so + # issuing one to a person would hand them every planned device's identity. + raise CallsignReserved("Using MDM agent CNs as callsigns is forbidden") try: await Person.by_callsign(callsign) raise CallsignReserved() diff --git a/tests/test_enrollment.py b/tests/test_enrollment.py index c78acc62..449e5850 100644 --- a/tests/test_enrollment.py +++ b/tests/test_enrollment.py @@ -22,6 +22,7 @@ EnrollmentPool, Person, ) +from rasenmaeher_api.db.errors import CallsignReserved from rasenmaeher_api.rmsettings import RMSettings LOGGER = logging.getLogger(__name__) @@ -902,3 +903,24 @@ async def test_planning_many_devices_keeps_the_admin_session( ) assert resp.status_code == 200 assert resp.json()["jwt"] == "" + + +@pytest.mark.asyncio(loop_scope="session") +async def test_agent_cn_cannot_be_taken_as_a_callsign(monkeypatch: pytest.MonkeyPatch) -> None: + """The agent CN is a service identity, not a name anyone may enroll under + + A certificate carrying it is accepted as the MDM agent on sight, with no Person, role or + product check, so issuing one to a person would hand them every planned device's identity. + Both creation paths have to refuse it, and case cannot be used to slip past: callsign lookups + fold case, so a near-miss would be a confusing collision rather than a second identity. + + No database here on purpose -- the guard is meant to fire before anything is written. + """ + settings = RMSettings.singleton() + monkeypatch.setattr(settings, "mdm_agent_cns", f"{MDM_AGENT_CN},second-agent") + + for taken in (MDM_AGENT_CN, MDM_AGENT_CN.upper(), "second-agent"): + with pytest.raises(CallsignReserved): + await Enrollment.create_for_callsign(callsign=taken) + with pytest.raises(CallsignReserved): + await Person.create_with_cert(callsign=taken) From 8908ab698e984e05fb25e8b0abab69fd85316497 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sun, 13 Sep 2026 16:37:21 +0300 Subject: [PATCH 09/18] feat: mark devices planned for MDM in the enrollment list Both a planned device and a person waiting for approval sit in the list as PENDING, but the device is waiting for its own CSR through the agent and approving it by hand is refused, so without the marker the approve-users queue fills with rows nobody may act on. --- .../web/api/enrollment/views.py | 20 ++++++++++++++-- tests/test_enrollment.py | 23 +++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/rasenmaeher_api/web/api/enrollment/views.py b/src/rasenmaeher_api/web/api/enrollment/views.py index 9b735635..ff9bb516 100644 --- a/src/rasenmaeher_api/web/api/enrollment/views.py +++ b/src/rasenmaeher_api/web/api/enrollment/views.py @@ -181,12 +181,28 @@ async def request_enrollment_list(code: str | None = None) -> EnrollmentListOut: if code: try: enrollment = await Enrollment.by_approvecode(code) - result_list.append({"callsign": enrollment.callsign, "approvecode": code, "state": enrollment.state}) + result_list.append( + { + "callsign": enrollment.callsign, + "approvecode": code, + "state": enrollment.state, + "mdm": enrollment.planned_for_mdm, + } + ) except NotFound: pass return EnrollmentListOut(callsign_list=result_list) async for enrollment in Enrollment.list(): - result_list.append({"callsign": enrollment.callsign, "approvecode": "", "state": enrollment.state}) + # mdm marks a device an admin planned for MDM enrolment. It is waiting for the device's own + # CSR through the agent, not for a human to approve it, and approving it by hand is refused. + result_list.append( + { + "callsign": enrollment.callsign, + "approvecode": "", + "state": enrollment.state, + "mdm": enrollment.planned_for_mdm, + } + ) return EnrollmentListOut(callsign_list=result_list) diff --git a/tests/test_enrollment.py b/tests/test_enrollment.py index 449e5850..c8f13829 100644 --- a/tests/test_enrollment.py +++ b/tests/test_enrollment.py @@ -905,6 +905,29 @@ async def test_planning_many_devices_keeps_the_admin_session( assert resp.json()["jwt"] == "" +@pytest.mark.asyncio(loop_scope="session") +async def test_list_marks_devices_planned_for_mdm(tilauspalvelu_jwt_admin_client: TestClient) -> None: + """An admin has to be able to tell a planned device from a person waiting for approval + + Both sit in the list in state PENDING, but a device is waiting for its own CSR through the + agent and approving it by hand is refused, so without the marker the approve-users queue fills + with rows nobody may act on. + """ + device = f"mdmlisted_{secrets.token_hex(4)}" + human = f"humanlisted_{secrets.token_hex(4)}" + resp = await tilauspalvelu_jwt_admin_client.post("/api/v1/enrollment/init", json={"callsign": device, "mdm": True}) + assert resp.status_code == 200 + resp = await tilauspalvelu_jwt_admin_client.post("/api/v1/enrollment/init", json={"callsign": human}) + assert resp.status_code == 200 + # A plain init hands back the new callsign's JWT as a cookie, which would unseat the admin + if tilauspalvelu_jwt_admin_client.cookie_jar is not None: + tilauspalvelu_jwt_admin_client.cookie_jar.clear() + + listed = (await tilauspalvelu_jwt_admin_client.get("/api/v1/enrollment/list")).json()["callsign_list"] + marks = {row["callsign"]: row["mdm"] for row in listed if row["callsign"] in (device, human)} + assert marks == {device: True, human: False} + + @pytest.mark.asyncio(loop_scope="session") async def test_agent_cn_cannot_be_taken_as_a_callsign(monkeypatch: pytest.MonkeyPatch) -> None: """The agent CN is a service identity, not a name anyone may enroll under From e79d633f7b34db00c2a06208af21970053165803 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sun, 13 Sep 2026 20:10:33 +0300 Subject: [PATCH 10/18] test: two devices cannot both claim one planned callsign The clause the MVP plan gates on. A callsign is spent the moment it is used and is never released, so if both devices got past the claim one would hold an identity the other also has, and the loser could never be enrolled under any name it was promised. --- tests/test_enrollment.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/test_enrollment.py b/tests/test_enrollment.py index c8f13829..835179e1 100644 --- a/tests/test_enrollment.py +++ b/tests/test_enrollment.py @@ -1,5 +1,6 @@ """Test enrollment endpoint""" +import asyncio import logging import secrets import uuid @@ -20,6 +21,7 @@ EngineWrapper, Enrollment, EnrollmentPool, + EnrollmentState, Person, ) from rasenmaeher_api.db.errors import CallsignReserved @@ -928,6 +930,31 @@ async def test_list_marks_devices_planned_for_mdm(tilauspalvelu_jwt_admin_client assert marks == {device: True, human: False} +@pytest.mark.asyncio(loop_scope="session") +async def test_two_devices_cannot_both_claim_one_planned_callsign() -> None: + """The clause the MVP plan will not sign off without + + A callsign is spent the moment it is used and can never be released, so if two devices both + got past the claim one of them would receive a certificate for an identity the other also + holds, and the loser could never be enrolled under any name it had been promised. The check + and the write are one UPDATE for exactly this reason, so the guarantee is the database's and + not a check-then-act in Python. + """ + callsign = f"mdmrace_{secrets.token_hex(4)}" + planned = await Enrollment.create_for_callsign(callsign=callsign, extra={"mdm": True}) + + first, second = await asyncio.gather( + (await Enrollment.by_callsign(callsign)).claim_with_csr("csr-from-device-one"), + (await Enrollment.by_callsign(callsign)).claim_with_csr("csr-from-device-two"), + ) + assert sorted([first, second]) == [False, True], "exactly one device may claim a planned callsign" + + settled = await Enrollment.by_callsign(callsign) + assert settled.csr in ("csr-from-device-one", "csr-from-device-two") + assert settled.state == EnrollmentState.PENDING, "claiming must not decide the enrollment" + _ = planned + + @pytest.mark.asyncio(loop_scope="session") async def test_agent_cn_cannot_be_taken_as_a_callsign(monkeypatch: pytest.MonkeyPatch) -> None: """The agent CN is a service identity, not a name anyone may enroll under From f0b009f2716daf74c87ab5ec7489b26e3988ce66 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Mon, 14 Sep 2026 20:40:14 +0300 Subject: [PATCH 11/18] fix: a device request may not choose what it is issued The signer picks its profile from the request's own extended key usage, and nothing replaces the names it carries, so a request asking for server authentication was issued one by the deployment CA. On this path the request arrived from the public SCEP endpoint and its contents are entirely the caller's choice. A genuine MDM device request asks for none of this. --- .../web/api/enrollment/views.py | 39 +++++++++++ tests/test_enrollment.py | 67 ++++++++++++++++++- 2 files changed, 105 insertions(+), 1 deletion(-) diff --git a/src/rasenmaeher_api/web/api/enrollment/views.py b/src/rasenmaeher_api/web/api/enrollment/views.py index ff9bb516..eb9c2c51 100644 --- a/src/rasenmaeher_api/web/api/enrollment/views.py +++ b/src/rasenmaeher_api/web/api/enrollment/views.py @@ -6,6 +6,7 @@ from cryptography import x509 from cryptography.hazmat.primitives import serialization +from cryptography.x509.oid import ExtendedKeyUsageOID from fastapi import APIRouter, Body, Depends, HTTPException, Request, Response from libpvarki.schemas.generic import OperationResultResponse from multikeyjwt import Issuer @@ -444,6 +445,41 @@ def mdm_agent_cn(request: Request) -> str | None: return str(payload.userid) +def device_csr_overreach(csrpem: str) -> str | None: + """Is this request asking for more than a device identity, or None if it is not + + The signer picks its profile from what the request asks for, so a request that asks for server + authentication is issued one, and nothing replaces the names it carries. On this path the + request arrived from the public SCEP endpoint and its contents are entirely the caller's + choice, so the shape of what gets issued cannot be one of the things they choose. + + A genuine device request from an MDM asks for none of this: it carries a subject and a public + key and nothing else. + """ + try: + csr = x509.load_pem_x509_csr(csrpem.encode("utf-8")) + except ValueError as exc: + return f"the request does not parse: {exc}" + try: + usages = csr.extensions.get_extension_for_class(x509.ExtendedKeyUsage).value + if [usage for usage in usages if usage != ExtendedKeyUsageOID.CLIENT_AUTH]: + return "the request asks for a key usage beyond client authentication" + except x509.ExtensionNotFound: + pass + try: + csr.extensions.get_extension_for_class(x509.SubjectAlternativeName) + return "the request carries subject alternative names" + except x509.ExtensionNotFound: + pass + try: + basic = csr.extensions.get_extension_for_class(x509.BasicConstraints).value + if basic.ca: + return "the request asks to be a certificate authority" + except x509.ExtensionNotFound: + pass + return None + + def same_public_key(csrpem: str, certpem: str) -> bool: """Does this CSR carry the key that was certified""" csr = x509.load_pem_x509_csr(csrpem.encode("utf-8")) @@ -587,6 +623,9 @@ def refuse(status_code: int, error_code: str, detail: str = MDM_REFUSED) -> HTTP raise refuse(403, "NOT_PLANNED_FOR_MDM") if not verify_csr(csrpem, planned.callsign): raise refuse(403, "CSR_SUBJECT") + overreach = device_csr_overreach(csrpem) + if overreach: + raise refuse(403, "CSR_OVERREACH") if planned.state != EnrollmentState.PENDING or planned.csr: return await mdm_repeat(request, csrpem, planned, refuse) diff --git a/tests/test_enrollment.py b/tests/test_enrollment.py index 835179e1..ec7abe7e 100644 --- a/tests/test_enrollment.py +++ b/tests/test_enrollment.py @@ -13,7 +13,9 @@ import pytest_asyncio from async_asgi_testclient import TestClient # type: ignore[import-untyped] from cryptography import x509 -from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives import hashes, serialization +from cryptography.hazmat.primitives.asymmetric import rsa +from cryptography.x509.oid import ExtendedKeyUsageOID, NameOID from fastapi import FastAPI from libpvarki.mtlshelp.csr import async_create_client_csr, async_create_keypair @@ -974,3 +976,66 @@ async def test_agent_cn_cannot_be_taken_as_a_callsign(monkeypatch: pytest.Monkey await Enrollment.create_for_callsign(callsign=taken) with pytest.raises(CallsignReserved): await Person.create_with_cert(callsign=taken) + + +def _csr_asking_for(callsign: str, **extensions: Any) -> str: + """A device request that asks for more than a device identity""" + key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + builder = x509.CertificateSigningRequestBuilder().subject_name( + x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, callsign)]) + ) + if extensions.get("server_auth"): + builder = builder.add_extension( + x509.ExtendedKeyUsage([ExtendedKeyUsageOID.SERVER_AUTH, ExtendedKeyUsageOID.CLIENT_AUTH]), critical=False + ) + if extensions.get("san"): + builder = builder.add_extension( + x509.SubjectAlternativeName([x509.DNSName(str(extensions["san"]))]), critical=False + ) + if extensions.get("ca"): + builder = builder.add_extension(x509.BasicConstraints(ca=True, path_length=None), critical=True) + return builder.sign(key, hashes.SHA256()).public_bytes(serialization.Encoding.PEM).decode("utf-8") + + +@pytest.mark.asyncio(loop_scope="session") +async def test_a_device_request_may_not_choose_what_it_is_issued( + tilauspalvelu_jwt_admin_client: TestClient, mdm_agent_client: TestClient +) -> None: + """The signer picks its profile from what the request asks for + + So a request asking for server authentication is issued one, and nothing replaces the names it + carries. On this path the request arrived from the public SCEP endpoint, where its contents are + entirely the caller's choice, and every device in the deployment trusts this CA. A genuine MDM + device request asks for none of it. + """ + for label, kwargs in ( + ("server_auth", {"server_auth": True}), + ("san", {"san": "evil.example"}), + ("ca", {"ca": True}), + ): + callsign = f"mdmoverreach_{secrets.token_hex(4)}" + resp = await tilauspalvelu_jwt_admin_client.post( + "/api/v1/enrollment/init", json={"callsign": callsign, "mdm": True} + ) + assert resp.status_code == 200 + resp = await mdm_agent_client.post( + "/api/v1/enrollment/accept", + json={"callsign": callsign, "csr": _csr_asking_for(callsign, **kwargs)}, + ) + assert resp.status_code == 403, f"{label} must be refused" + assert (await Enrollment.by_callsign(callsign)).csr is None, f"{label} must not claim the callsign" + + +@pytest.mark.asyncio(loop_scope="session") +async def test_an_ordinary_device_request_is_still_accepted( + tilauspalvelu_jwt_admin_client: TestClient, mdm_agent_client: TestClient, nice_tmpdir: str +) -> None: + """The guard must not refuse what an MDM actually sends""" + callsign = f"mdmplain_{secrets.token_hex(4)}" + resp = await tilauspalvelu_jwt_admin_client.post( + "/api/v1/enrollment/init", json={"callsign": callsign, "mdm": True} + ) + assert resp.status_code == 200 + csrpem = await _device_csr(Path(nice_tmpdir), callsign, name="plain") + resp = await mdm_agent_client.post("/api/v1/enrollment/accept", json={"callsign": callsign, "csr": csrpem}) + assert resp.status_code == 200, resp.text From 19727eb2677ee54104b0ac7dff7818f1a9ae63ac Mon Sep 17 00:00:00 2001 From: benjamsf Date: Tue, 15 Sep 2026 12:25:46 +0300 Subject: [PATCH 12/18] fix: require the per-enrolment code, not just the callsign The SCEP endpoint is public and its challenge is not a secret, so the last gate was a callsign: a short human-chosen string anyone can guess. The MDM now carries @ in the one per-host field it can substitute into a subject, and the code is what says this device was assigned this callsign. Refused before anything is claimed, so guessing cannot spend a callsign either. --- .../web/api/enrollment/views.py | 8 +++- .../web/api/utils/csr_utils.py | 32 ++++++++++++++ tests/test_csr_utils.py | 42 ++++++++++++++++++- 3 files changed, 80 insertions(+), 2 deletions(-) diff --git a/src/rasenmaeher_api/web/api/enrollment/views.py b/src/rasenmaeher_api/web/api/enrollment/views.py index eb9c2c51..113c1620 100644 --- a/src/rasenmaeher_api/web/api/enrollment/views.py +++ b/src/rasenmaeher_api/web/api/enrollment/views.py @@ -19,7 +19,7 @@ from ..middleware.mtls import MTLSorJWT from ..middleware.user import ValidUser from ..utils.auditcontext import build_audit_extra -from ..utils.csr_utils import verify_csr +from ..utils.csr_utils import csr_carries_enrolment_proof, verify_csr from .schema import ( EnrollmentAcceptIn, EnrollmentAcceptResultOut, @@ -626,6 +626,12 @@ def refuse(status_code: int, error_code: str, detail: str = MDM_REFUSED) -> HTTP overreach = device_csr_overreach(csrpem) if overreach: raise refuse(403, "CSR_OVERREACH") + if not csr_carries_enrolment_proof(csrpem, planned.callsign, planned.approvecode): + # The callsign alone is not proof: it is a short human-chosen string, and the SCEP + # challenge in front of it is not a secret. The per-enrolment code is what says the MDM + # assigned THIS device THIS callsign. Refused before anything is claimed, so guessing + # cannot spend a callsign either. + raise refuse(403, "NO_ENROLMENT_PROOF") if planned.state != EnrollmentState.PENDING or planned.csr: return await mdm_repeat(request, csrpem, planned, refuse) diff --git a/src/rasenmaeher_api/web/api/utils/csr_utils.py b/src/rasenmaeher_api/web/api/utils/csr_utils.py index 184b8015..07cad9d7 100644 --- a/src/rasenmaeher_api/web/api/utils/csr_utils.py +++ b/src/rasenmaeher_api/web/api/utils/csr_utils.py @@ -1,6 +1,7 @@ """Utils for checking CSR:s etc""" import logging +import secrets from cryptography import x509 from cryptography.x509.oid import NameOID @@ -44,3 +45,34 @@ def verify_csr(csrpem: str, callsign: str) -> bool: # crypto.X509Extension(b"keyUsage", True, b"digitalSignature,nonRepudiation,keyEncipherment"), # crypto.X509Extension(b"extendedKeyUsage", True, b"clientAuth"), return True + + +def csr_carries_enrolment_proof(csrpem: str, callsign: str, approvecode: str) -> bool: + """Does this request carry the per-enrolment code, proving the MDM assigned this device + + The public SCEP endpoint is reachable by anyone, and the SCEP challenge is not a secret: it + sits in the MDM's configuration and travels in every device's request. So without this the + last gate is a callsign, which is a short human-chosen string an attacker can guess. + + The MDM carries the proof by putting ``@`` in the one per-host field it + can substitute into a subject, rendered into an attribute beside the common name. Every + attribute other than the common name is searched rather than one named position, because which + attribute the MDM renders it into is the MDM's business and not something to hard-code. + """ + wanted = f"{callsign}@{approvecode}" + if not approvecode or not wanted.isascii(): + return False + try: + csr = x509.load_pem_x509_csr(ensure_utf8(csrpem)) + except ValueError: + return False + for attribute in csr.subject: + if attribute.oid == NameOID.COMMON_NAME: + continue + value = attribute.value + if not isinstance(value, str) or not value.isascii(): + # compare_digest raises on non-ascii str, and nothing we issue for is non-ascii + continue + if secrets.compare_digest(value, wanted): + return True + return False diff --git a/tests/test_csr_utils.py b/tests/test_csr_utils.py index 30cc9c6a..2321292d 100644 --- a/tests/test_csr_utils.py +++ b/tests/test_csr_utils.py @@ -5,6 +5,7 @@ """ import logging +from typing import Any import pytest from cryptography import x509 @@ -12,7 +13,7 @@ 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 +from rasenmaeher_api.web.api.utils.csr_utils import csr_carries_enrolment_proof, verify_csr LOGGER = logging.getLogger(__name__) @@ -94,3 +95,42 @@ def test_unsigned_csr_is_refused() -> None: 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") + + +def _csr_with_subject(attrs: list[tuple[Any, str]]) -> str: + key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + name = x509.Name([x509.NameAttribute(oid, value) for oid, value in attrs]) + csr = x509.CertificateSigningRequestBuilder().subject_name(name).sign(key, hashes.SHA256()) + return csr.public_bytes(serialization.Encoding.PEM).decode("utf-8") + + +def test_the_enrolment_proof_is_required_and_exact() -> None: + """The callsign alone is a short guessable string, so it cannot be the last gate""" + good = _csr_with_subject([(NameOID.COMMON_NAME, "OTTER1"), (NameOID.ORGANIZATIONAL_UNIT_NAME, "OTTER1@7F3A9C2B")]) + assert csr_carries_enrolment_proof(good, "OTTER1", "7F3A9C2B") + assert not csr_carries_enrolment_proof(good, "OTTER1", "WRONGONE") + assert not csr_carries_enrolment_proof(good, "OTTER2", "7F3A9C2B"), "a code is bound to its callsign" + + bare = _csr_with_subject([(NameOID.COMMON_NAME, "OTTER1")]) + assert not csr_carries_enrolment_proof(bare, "OTTER1", "7F3A9C2B"), "what a guesser would send" + + +def test_the_proof_is_found_wherever_the_mdm_renders_it() -> None: + """Which attribute carries it is the MDM's business, so do not hard-code one""" + for oid in (NameOID.ORGANIZATIONAL_UNIT_NAME, NameOID.ORGANIZATION_NAME, NameOID.STATE_OR_PROVINCE_NAME): + csrpem = _csr_with_subject([(NameOID.COMMON_NAME, "OTTER1"), (oid, "OTTER1@7F3A9C2B")]) + assert csr_carries_enrolment_proof(csrpem, "OTTER1", "7F3A9C2B") + + +def test_the_common_name_cannot_be_the_proof() -> None: + """Otherwise a caller who guessed the callsign could satisfy the check with it""" + csrpem = _csr_with_subject([(NameOID.COMMON_NAME, "OTTER1@7F3A9C2B")]) + assert not csr_carries_enrolment_proof(csrpem, "OTTER1@7F3A9C2B", "7F3A9C2B") + + +def test_odd_input_is_refused_rather_than_raising() -> None: + """This runs on bytes that reached us from a public endpoint""" + assert not csr_carries_enrolment_proof("not a csr", "OTTER1", "7F3A9C2B") + assert not csr_carries_enrolment_proof(_csr_with_subject([(NameOID.COMMON_NAME, "OTTER1")]), "OTTER1", "") + unicode_ou = _csr_with_subject([(NameOID.COMMON_NAME, "OTTER1"), (NameOID.ORGANIZATIONAL_UNIT_NAME, "ÄÄÄ")]) + assert not csr_carries_enrolment_proof(unicode_ou, "OTTER1", "7F3A9C2B"), "must not raise on non-ascii" From d6d0df8df2ea6e0b287e54a7e72f2082caf21cf4 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sat, 19 Sep 2026 00:27:17 +0300 Subject: [PATCH 13/18] feat: list the approval code for devices, not for people For a person the code is a secret they tell an admin, and listing it would let any admin approve anybody without being told anything. For a device it is the opposite: the admin has to type it into the MDM for the device to enrol at all, and nobody else ever learns it. Withholding it there meant an admin who plans a hundred devices could only read the codes once, at the moment of creation. Co-Authored-By: Claude Opus 5 (1M context) --- .../web/api/enrollment/views.py | 8 +++++- tests/test_enrollment.py | 27 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/rasenmaeher_api/web/api/enrollment/views.py b/src/rasenmaeher_api/web/api/enrollment/views.py index 113c1620..dab0c448 100644 --- a/src/rasenmaeher_api/web/api/enrollment/views.py +++ b/src/rasenmaeher_api/web/api/enrollment/views.py @@ -196,10 +196,16 @@ async def request_enrollment_list(code: str | None = None) -> EnrollmentListOut: async for enrollment in Enrollment.list(): # mdm marks a device an admin planned for MDM enrolment. It is waiting for the device's own # CSR through the agent, not for a human to approve it, and approving it by hand is refused. + # + # The code is withheld for people: there it is a secret the person tells the admin, and + # listing it would let any admin approve anybody without being told anything. For a device + # it is the opposite -- the admin has to type it into the MDM for the device to enrol at + # all, and nobody else ever learns it. Withholding it there would mean an admin who plans a + # hundred devices can only ever read the codes once, at the moment of creation. result_list.append( { "callsign": enrollment.callsign, - "approvecode": "", + "approvecode": enrollment.approvecode if enrollment.planned_for_mdm else "", "state": enrollment.state, "mdm": enrollment.planned_for_mdm, } diff --git a/tests/test_enrollment.py b/tests/test_enrollment.py index ec7abe7e..7af205a7 100644 --- a/tests/test_enrollment.py +++ b/tests/test_enrollment.py @@ -932,6 +932,33 @@ async def test_list_marks_devices_planned_for_mdm(tilauspalvelu_jwt_admin_client assert marks == {device: True, human: False} +@pytest.mark.asyncio(loop_scope="session") +async def test_the_list_carries_the_code_for_devices_but_not_for_people( + tilauspalvelu_jwt_admin_client: TestClient, +) -> None: + """A device's code is configuration an admin must type into the MDM; a person's is a secret + + Withholding it for devices would mean an admin who plans a hundred of them can only read the + codes once, at the moment of creation. Exposing it for people would let any admin approve + anybody without being told anything. + """ + device = f"mdmcode_{secrets.token_hex(4)}" + human = f"humancode_{secrets.token_hex(4)}" + made = await tilauspalvelu_jwt_admin_client.post( + "/api/v1/enrollment/init", json={"callsign": device, "mdm": True} + ) + assert made.status_code == 200 + expected = made.json()["approvecode"] + assert await tilauspalvelu_jwt_admin_client.post("/api/v1/enrollment/init", json={"callsign": human}) + if tilauspalvelu_jwt_admin_client.cookie_jar is not None: + tilauspalvelu_jwt_admin_client.cookie_jar.clear() + + listed = (await tilauspalvelu_jwt_admin_client.get("/api/v1/enrollment/list")).json()["callsign_list"] + codes = {row["callsign"]: row["approvecode"] for row in listed if row["callsign"] in (device, human)} + assert codes[device] == expected, "a device's code has to be readable again after creation" + assert codes[human] == "", "a person's code is theirs to tell the admin" + + @pytest.mark.asyncio(loop_scope="session") async def test_two_devices_cannot_both_claim_one_planned_callsign() -> None: """The clause the MVP plan will not sign off without From 8ece58a9e855ac9b3a4b28f1248289339f0910e9 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sat, 19 Sep 2026 22:06:19 +0300 Subject: [PATCH 14/18] feat: tell the admin UI what to configure the MDM with The challenge is not a secret and is no longer treated as one. It sits in the MDM's own configuration and the MDM hands it to every device; what decides whether a device may have a callsign is the per-enrolment code in its certificate request. Admin only all the same, because nobody else has a reason to read it. Co-Authored-By: Claude Opus 5 (1M context) --- src/rasenmaeher_api/rmsettings.py | 7 ++++++ .../web/api/enrollment/schema.py | 9 +++++++ .../web/api/enrollment/views.py | 24 ++++++++++++++++++ tests/test_enrollment.py | 25 +++++++++++++++++++ 4 files changed, 65 insertions(+) diff --git a/src/rasenmaeher_api/rmsettings.py b/src/rasenmaeher_api/rmsettings.py index ec4887a9..559afd0d 100644 --- a/src/rasenmaeher_api/rmsettings.py +++ b/src/rasenmaeher_api/rmsettings.py @@ -125,6 +125,13 @@ def log_level_int(self) -> int: # edge, which is far more than completing a planned enrolment needs. mdm_agent_cns: str = "" + # The SCEP challenge the MDM is configured with, so the admin UI can show what to type there. + # Deliberately not a secret: it sits in the MDM's own configuration, the MDM hands it to every + # device, and it is a nuisance filter and nothing more. What actually decides whether a device + # may have a callsign is the per-enrolment code in its certificate request. Empty unless the + # deployment passes it in, and then the UI says where to look instead. + mdm_scep_challenge: str = "" + persistent_data_dir: str = "/data/persistent" # mtls diff --git a/src/rasenmaeher_api/web/api/enrollment/schema.py b/src/rasenmaeher_api/web/api/enrollment/schema.py index 7b35b212..62b0e0ff 100644 --- a/src/rasenmaeher_api/web/api/enrollment/schema.py +++ b/src/rasenmaeher_api/web/api/enrollment/schema.py @@ -469,3 +469,12 @@ class EnrollmentInviteCodeDeleteIn(BaseModel): ) invite_code: str + + +class MdmSettingsOut(BaseModel): + """What an operator has to type into whatever MDM their unit runs""" + + #: Where the MDM sends each device's certificate request + scep_url: str + #: The SCEP challenge, or empty if this deployment has not passed it through + challenge: str diff --git a/src/rasenmaeher_api/web/api/enrollment/views.py b/src/rasenmaeher_api/web/api/enrollment/views.py index dab0c448..8fb14905 100644 --- a/src/rasenmaeher_api/web/api/enrollment/views.py +++ b/src/rasenmaeher_api/web/api/enrollment/views.py @@ -43,6 +43,7 @@ EnrollmentShowVerificationCodeOut, EnrollmentStatusIn, EnrollmentStatusOut, + MdmSettingsOut, ) LOGGER = logging.getLogger(__name__) @@ -214,6 +215,29 @@ async def request_enrollment_list(code: str | None = None) -> EnrollmentListOut: return EnrollmentListOut(callsign_list=result_list) +@ENROLLMENT_ROUTER.get( + "/mdm-settings", + response_model=MdmSettingsOut, + dependencies=[Depends(ValidUser(auto_error=True, require_roles=["admin"]))], +) +async def request_mdm_settings(request: Request) -> MdmSettingsOut: + """What to configure the MDM with + + The challenge is not a secret and is not treated as one: it sits in the MDM's own + configuration, the MDM hands it to every device, and it is a nuisance filter against whatever + scans the internet. What decides whether a device may have a callsign is the per-enrolment code + in its certificate request, which is per device and never leaves the admin and the MDM. + + Admin only all the same, because there is no reason for anybody else to read it. + """ + cnf = RMSettings.singleton() + host = (request.url.hostname or "").removeprefix("mtls.") + return MdmSettingsOut( + scep_url=f"https://{host}/scep" if host else "", + challenge=cnf.mdm_scep_challenge, + ) + + def issue_enrollment_jwt(response: Response, claims: dict[str, Any]) -> str: """Issue longer lived JWT and set persistent cookie""" enroll_issuer = Issuer() diff --git a/tests/test_enrollment.py b/tests/test_enrollment.py index 7af205a7..527d02aa 100644 --- a/tests/test_enrollment.py +++ b/tests/test_enrollment.py @@ -1066,3 +1066,28 @@ async def test_an_ordinary_device_request_is_still_accepted( csrpem = await _device_csr(Path(nice_tmpdir), callsign, name="plain") resp = await mdm_agent_client.post("/api/v1/enrollment/accept", json={"callsign": callsign, "csr": csrpem}) assert resp.status_code == 200, resp.text + + +@pytest.mark.asyncio(loop_scope="session") +async def test_mdm_settings_says_what_to_configure( + tilauspalvelu_jwt_admin_client: TestClient, monkeypatch: pytest.MonkeyPatch +) -> None: + """The challenge is not a secret, so an admin reads it here rather than off the server + + It sits in the MDM's own configuration and the MDM hands it to every device. What decides + whether a device may have a callsign is the per-enrolment code, not this. + """ + monkeypatch.setattr(RMSettings.singleton(), "mdm_scep_challenge", "a-challenge-not-a-secret") + resp = await tilauspalvelu_jwt_admin_client.get("/api/v1/enrollment/mdm-settings") + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["challenge"] == "a-challenge-not-a-secret" + assert body["scep_url"].endswith("/scep") + assert not body["scep_url"].startswith("https://mtls."), "the responder is on the plain host" + + +@pytest.mark.asyncio(loop_scope="session") +async def test_mdm_settings_is_admin_only(tilauspalvelu_jwt_user_client: TestClient) -> None: + """No reason for anybody but an admin to read it, secret or not""" + resp = await tilauspalvelu_jwt_user_client.get("/api/v1/enrollment/mdm-settings") + assert resp.status_code == 403, resp.text From 4646a5e005aa377b73102b9cea3deeb320a23ee1 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Fri, 9 Oct 2026 21:17:38 +0300 Subject: [PATCH 15/18] style: let ruff format decide the wrapping --- tests/test_enrollment.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_enrollment.py b/tests/test_enrollment.py index 527d02aa..8472d20e 100644 --- a/tests/test_enrollment.py +++ b/tests/test_enrollment.py @@ -944,9 +944,7 @@ async def test_the_list_carries_the_code_for_devices_but_not_for_people( """ device = f"mdmcode_{secrets.token_hex(4)}" human = f"humancode_{secrets.token_hex(4)}" - made = await tilauspalvelu_jwt_admin_client.post( - "/api/v1/enrollment/init", json={"callsign": device, "mdm": True} - ) + made = await tilauspalvelu_jwt_admin_client.post("/api/v1/enrollment/init", json={"callsign": device, "mdm": True}) assert made.status_code == 200 expected = made.json()["approvecode"] assert await tilauspalvelu_jwt_admin_client.post("/api/v1/enrollment/init", json={"callsign": human}) From 8b7a92790eee4ef9dae7d6f49ccf7cd0bd627a93 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sat, 10 Oct 2026 02:28:46 +0300 Subject: [PATCH 16/18] fix: install the locked versions in the production image --find-links is additive, so pip re-resolved against PyPI and took a newer sqlmodel that widens its SQLAlchemy range. SQLAlchemy 2.1 points postgresql:// at psycopg3, which is not installed, so the image crashed on startup with ModuleNotFoundError while the lock said 2.0.52. --- Dockerfile | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index c4cf27de..4f0adb95 100644 --- a/Dockerfile +++ b/Dockerfile @@ -133,7 +133,10 @@ RUN --mount=type=ssh apt-get update && apt-get install -y \ && rm -rf /var/lib/apt/lists/* \ && chmod a+x /docker-entrypoint.sh \ && WHEELFILE=`echo /tmp/wheelhouse/rasenmaeher_api-*.whl` \ - && pip3 install --find-links=/tmp/wheelhouse/ "$WHEELFILE"[all] \ + # --no-index, or --find-links is merely additive and pip re-resolves against PyPI: the + # wheelhouse is built from `uv export --frozen`, but pip would happily pick a newer sqlmodel + # that widens its SQLAlchemy range, and the image then ships versions the lock never saw. + && pip3 install --no-index --find-links=/tmp/wheelhouse/ "$WHEELFILE"[all] \ && rm -rf /tmp/wheelhouse/ \ # Do whatever else you need to && true From f502565dab37d19bcd0146e944f54c276900f188 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sat, 10 Oct 2026 02:28:51 +0300 Subject: [PATCH 17/18] test: carry the enrolment proof the way an MDM does Requiring the per-enrolment code changed what a device has to send, but only the csr_utils tests were updated -- the two enrollment tests that expect a success still sent a bare CN and were refused. The code riding into the issued certificate is pinned too, since the signer copies the request's subject. Also stops test_list_as_adm asserting on whichever row came back first: a device planned for MDM carries its code on purpose, so that was a coin flip on what earlier tests had left behind. --- tests/test_enrollment.py | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/tests/test_enrollment.py b/tests/test_enrollment.py index 8472d20e..997bcac2 100644 --- a/tests/test_enrollment.py +++ b/tests/test_enrollment.py @@ -233,7 +233,11 @@ async def test_list_as_adm(tilauspalvelu_jwt_admin_client: TestClient) -> None: LOGGER.debug(resp_dict) assert resp_dict["callsign_list"] is not None assert resp_dict["callsign_list"][0]["callsign"] - assert not resp_dict["callsign_list"][0]["approvecode"] + # Every person's code, not just whichever row the database happened to return first: a device + # planned for MDM carries its code on purpose, so an index-0 check is a coin flip on what + # earlier tests left behind. Which kind may show its code is pinned in + # test_the_list_carries_the_code_for_devices_but_not_for_people, on rows it created itself. + assert all(not row["approvecode"] for row in resp_dict["callsign_list"] if not row["mdm"]) assert resp.status_code == 200 @@ -761,13 +765,21 @@ async def mdm_agent_client(app_instance: FastAPI) -> AsyncGenerator[TestClient, settings.mdm_agent_cns = previous -async def _device_csr(tempdir: Path, callsign: str, name: str = "device") -> str: - """A CSR the way a device would send one: its own key, its callsign as the CN""" +async def _device_csr(tempdir: Path, callsign: str, name: str = "device", code: str = "") -> str: + """A CSR the way a device would send one: its own key, its callsign as the CN + + With a code, also the way an MDM sends one: ``@`` rendered into the one + per-host subject field an MDM can substitute, which is what proves the MDM assigned this + device this callsign. Without it, what a caller who only guessed the callsign would send. + """ privkeyfile = Path(tempdir) / f"{name}.key" pubkeyfile = Path(tempdir) / f"{name}.pub" csrfile = Path(tempdir) / f"{name}.csr" + reqdn = {"CN": callsign} + if code: + reqdn["OU"] = f"{callsign}@{code}" ckp = await async_create_keypair(privkeyfile, pubkeyfile) - return str(await async_create_client_csr(ckp, csrfile, {"CN": callsign})) + return str(await async_create_client_csr(ckp, csrfile, reqdn)) def _public_key_der(pem: str) -> bytes: @@ -814,8 +826,9 @@ async def test_mdm_agent_completes_planned_enrollment( "/api/v1/enrollment/init", json={"callsign": callsign, "mdm": True} ) assert resp.status_code == 200 + code = resp.json()["approvecode"] - csrpem = await _device_csr(tempdir, callsign) + csrpem = await _device_csr(tempdir, callsign, code=code) resp = await mdm_agent_client.post("/api/v1/enrollment/accept", json={"callsign": callsign, "csr": csrpem}) assert resp.status_code == 200 body = resp.json() @@ -826,7 +839,13 @@ async def test_mdm_agent_completes_planned_enrollment( # The certificate must be for the key the device holds. If this ever regresses, rasenmaeher # generated a keypair of its own and the device received a certificate it cannot use. assert _public_key_der(certpem) == _public_key_der(csrpem) - assert [attr.value for attr in x509.load_pem_x509_certificate(certpem.encode("utf-8")).subject] == [callsign] + # The mTLS edge authorises on the CN it reads off the certificate, so exactly one and it is + # the callsign. The signer copies the rest of the request's subject verbatim, which is why the + # enrolment code rides along -- spent by the time it is in there, but worth noticing if the + # signer ever starts naming the certificate itself instead. + issued = x509.load_pem_x509_certificate(certpem.encode("utf-8")) + assert [attr.value for attr in issued.subject.get_attributes_for_oid(NameOID.COMMON_NAME)] == [callsign] + assert [attr.value for attr in issued.subject] == [f"{callsign}@{code}", callsign] # The reply can be lost on the way back to the MDM, which then repeats the request verbatim resp = await mdm_agent_client.post("/api/v1/enrollment/accept", json={"callsign": callsign, "csr": csrpem}) @@ -834,7 +853,7 @@ async def test_mdm_agent_completes_planned_enrollment( assert resp.json()["certificate"] == certpem # ...but a different key for the same callsign is somebody else - other_csr = await _device_csr(tempdir, callsign, name="imposter") + other_csr = await _device_csr(tempdir, callsign, name="imposter", code=code) resp = await mdm_agent_client.post("/api/v1/enrollment/accept", json={"callsign": callsign, "csr": other_csr}) assert resp.status_code == 403 @@ -1061,7 +1080,8 @@ async def test_an_ordinary_device_request_is_still_accepted( "/api/v1/enrollment/init", json={"callsign": callsign, "mdm": True} ) assert resp.status_code == 200 - csrpem = await _device_csr(Path(nice_tmpdir), callsign, name="plain") + code = resp.json()["approvecode"] + csrpem = await _device_csr(Path(nice_tmpdir), callsign, name="plain", code=code) resp = await mdm_agent_client.post("/api/v1/enrollment/accept", json={"callsign": callsign, "csr": csrpem}) assert resp.status_code == 200, resp.text From e420c3bf413eea79783a4c4993bb59bffb3c7999 Mon Sep 17 00:00:00 2001 From: benjamsf Date: Sat, 10 Oct 2026 02:28:54 +0300 Subject: [PATCH 18/18] test: give the CLI subprocess tests a realistic timeout 15s covers booting the whole app on a developer machine, not on a loaded shared runner. --- tests/test_console.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/test_console.py b/tests/test_console.py index a82b2c7c..023182b9 100644 --- a/tests/test_console.py +++ b/tests/test_console.py @@ -8,6 +8,11 @@ from rasenmaeher_api import __version__ +# Each case boots the whole app in a subprocess, which on a loaded shared runner is nowhere near +# the ~2s it takes on a developer machine. Generous on purpose: the bound is here to stop a hung +# subprocess holding the suite, not to assert anything about startup time. +CLI_TIMEOUT = 60 + @pytest.mark.asyncio(loop_scope="session") async def test_version_cli() -> None: @@ -18,7 +23,7 @@ async def test_version_cli() -> None: stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, ) - out = await asyncio.wait_for(process.communicate(), 15) + out = await asyncio.wait_for(process.communicate(), CLI_TIMEOUT) # Demand clean exit assert process.returncode == 0 # Check output @@ -35,7 +40,7 @@ async def test_addcode_cli() -> None: stderr=asyncio.subprocess.PIPE, env=os.environ, ) - out = await asyncio.wait_for(process.communicate(), 15) + out = await asyncio.wait_for(process.communicate(), CLI_TIMEOUT) # Demand clean exit assert process.returncode == 0 # Check output