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