diff --git a/docs/CONNECTIONS.md b/docs/CONNECTIONS.md index 7d15bd3d..0245178b 100644 --- a/docs/CONNECTIONS.md +++ b/docs/CONNECTIONS.md @@ -845,6 +845,14 @@ poll/write shape against a remote server, selected by an internal `protocol` set document, so on a production-PHI enforcing instance the escape is inert and an unknown host key stays refused (`RejectPolicy`) even with the variable set; it takes effect only on a non-enforcing / non-PHI instance. + **Since #178 the negotiated key exchange, cipher and MAC are held to an algorithm floor** mapped from + the one the TLS hops assert on their contexts (`messagefoundry/config/ssh_policy.py` — forward-secret + key exchange, no 64-bit-block cipher, no MD5/SHA-1 MAC). It prunes `3des-cbc` and the MD5/SHA-1 MACs + from paramiko's offer and leaves everything a current OpenSSH server negotiates by default, so an + ordinary partner is unaffected. A partner that offers nothing above the floor is **refused with a + message naming the algorithms refused and what to enable instead** — not a timeout or a bare + `Incompatible ssh server`. There is **no configuration surface and no override**: the operator-tunable + allow-list is the separate, still-deferred half of #178. - **`Ftp(...)`** — stdlib `ftplib`, **no extra**: `tls=False` is plain FTP, `tls=True` is **FTPS** (explicit TLS + `PROT P`, encrypting the control *and* data channels). FTPS **verifies the server certificate and hostname by default** (a verifying `SSLContext`, not ftplib's no-verify fallback). diff --git a/messagefoundry/config/ssh_policy.py b/messagefoundry/config/ssh_policy.py new file mode 100644 index 00000000..155c4c1b --- /dev/null +++ b/messagefoundry/config/ssh_policy.py @@ -0,0 +1,318 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Shared SSH (SFTP) algorithm floor -- the SSH sibling of ``tls_policy.harden_cipher_suites``. + +**The asymmetry this closes.** The engine asserts a cipher floor wherever it builds a TLS context -- +measured 2026-08-10, seven :func:`~messagefoundry.config.tls_policy.harden_cipher_suites` call sites +across five files (the API/UI listener, MLLP in both directions, both DICOM contexts, FTPS, and the +SMTP alert sink). The SSH hop had no equivalent: ``disabled_algorithms`` appeared zero times anywhere +in the package on that date, so whatever paramiko happened to offer was whatever the engine would +negotiate. Driven against a live paramiko server, the SFTP client accepted ``hmac-md5``, +``hmac-sha1`` and ``3des-cbc`` without complaint -- a partner, or an on-path attacker steering the +negotiation, could pick any of them and nothing would say so. This module is the missing assertion. + +**Where the floor comes from -- it is the TLS floor, re-expressed, not a second opinion.** The +constants below are derived cell by cell from what the shipped TLS contexts *actually negotiate*, so +the two hops enforce one policy. Measured on CPython 3.14 / OpenSSL 3.5.7, both the client- and +server-default contexts resolve to 17 suites whose properties are: + +========= ================================================== ============================ +TLS cell Measured effective set SSH floor it maps to +========= ================================================== ============================ +``Kx=`` ``ECDH``, ``DH``, ``any`` (TLS 1.3, always ECDHE) forward-secret kex only +``Enc=`` ``AES(128/256)``, ``AESGCM(128/256)``, ``CHACHA20`` no 64-bit-block, no broken +``Mac=`` ``AEAD``, ``SHA256``, ``SHA384`` no MD5, no SHA-1, no 64-bit +========= ================================================== ============================ + +The ``Kx`` row is the property ``harden_cipher_suites`` asserts explicitly. The other two rows are +properties OpenSSL's default cipher string already guarantees for free on the TLS side -- there is no +3DES and no MD5/SHA-1 MAC in that 17-suite set -- but which paramiko's defaults do *not* guarantee, +which is why the SSH side has to name them. Mapping them across is what makes the floor the same +policy rather than an independently-invented one. + +**Two operations, deliberately separated** (each mirrors a property of the TLS side): + +* ``BELOW``-floor names are *disabled* (paramiko ``disabled_algorithms``). SSH negotiates from the + offer, so pruning the offer IS the enforcement -- unlike TLS, where the context already resolves to + a forward-secret set and ``harden_cipher_suites`` therefore only has to check it. +* Everything that survives is *asserted* to be recognisably above the floor. An unrecognised name + raises rather than being trusted, exactly as ``_is_forward_secret`` treats an unknown ``Kx``. + +The split is why an unknown name is never silently pruned: a paramiko release that adds a new modern +algorithm the allow-side does not yet know would fail LOUD at connect, naming the algorithm, instead +of being quietly dropped from the offer and narrowing reachability with no signal. + +**There is deliberately NO operator override.** The floor prunes 3DES and the MD5/SHA-1 MACs, all of +which every SSH server still under support has offered alternatives to for well over a decade, so +nothing plausible needs loosening; and a loosening switch nobody asked for is a second posture by the +back door. If a partner ever does force the question, the override belongs in +``config.settings.security_loosenings`` as a REQUIRED parameter, the way the TLS deviations do -- an +optional one there is a detector that silently fails to fire. Note the difference from BACKLOG #178's +other half (an operator-configurable cipher/KEX/MAC allow-list), which stays DEMAND-GATED: this +module has no configuration surface at all. + +Pure and stdlib-only: this module never imports paramiko. The caller passes the offer in, so the +``[sftp]`` extra stays optional and lazily imported (an install without paramiko behaves exactly as +it does today), and the policy stays unit-testable with no SSH library present. +""" + +from __future__ import annotations + +import enum +from collections.abc import Mapping, Sequence + +__all__ = [ + "SSH_ALGORITHM_CATEGORIES", + "FloorVerdict", + "ssh_algorithm_verdict", + "ssh_disabled_algorithms", + "ssh_floor_refusal", +] + +#: The paramiko ``disabled_algorithms`` keys this floor governs, in the order an operator reads them. +#: ``keys``/``pubkeys`` (host-key and public-key *authentication* algorithms) are deliberately absent: +#: they are a signature-algorithm question, not a confidentiality one, and paramiko 5.0 already dropped +#: SHA-1 RSA from both (the ``[sftp]`` extra floors paramiko at >=5.0 for exactly that reason). +SSH_ALGORITHM_CATEGORIES = ("kex", "ciphers", "macs") + +#: The substring paramiko puts in an ``IncompatiblePeer`` message for each category, so a negotiation +#: failure can be attributed to the right half of the floor. Written out rather than derived from the +#: category name: the mapping is not mechanical (``kex`` -> "kex algorithm", ``ciphers`` -> "ciphers"), +#: and a clever derivation would be one more thing to get silently wrong on a paramiko reword. +_PEER_MESSAGE_TOKENS = { + "kex": "no acceptable kex", + "ciphers": "no acceptable ciphers", + "macs": "no acceptable macs", +} + +#: How each category is named to an operator. The paramiko key is a wire-protocol abbreviation; the +#: refusal an operator has to act on should read like the SSH server config they will go and edit. +_CATEGORY_LABELS = {"kex": "key-exchange algorithm", "ciphers": "cipher", "macs": "MAC"} + + +class FloorVerdict(enum.Enum): + """Where one SSH algorithm name sits relative to the floor. + + ``ABOVE`` -- recognised and at or above the mapped TLS floor; offer it. ``BELOW`` -- recognised and + beneath it; disable it. ``UNKNOWN`` -- not recognised, so the floor cannot vouch for it; it is left + in the offer and the assertion raises on it, which is the loud half of the fail-closed posture.""" + + ABOVE = "above" + BELOW = "below" + UNKNOWN = "unknown" + + +# --- kex: the forward-secrecy cell (TLS ``Kx=ECDH``/``DH``) ------------------------------------- +# +# Forward-secret SSH key-exchange families. Each performs an ephemeral Diffie-Hellman (classical, +# elliptic-curve, or a PQ hybrid that carries an X25519/NIST-curve ECDH alongside the KEM), so a later +# compromise of the host key cannot decrypt recorded traffic -- the same property the TLS side asserts. +_FORWARD_SECRET_KEX_PREFIXES = ( + "curve25519-", + "curve448-", + "ecdh-sha2-", + "diffie-hellman-group", + "sntrup761x25519-", + "mlkem768x25519-", + "mlkem1024nistp384-", + "mlkem768nistp256-", + "gss-group", + "gss-curve25519-", + "gss-nistp", +) +#: RFC 4432 RSA key TRANSPORT. The client encrypts the session secret to a transient RSA key, with no +#: Diffie-Hellman anywhere -- the SSH analogue of a static-RSA TLS suite, and the one kex family that is +#: below the forward-secrecy floor outright. paramiko 5.0 implements neither; naming them keeps the +#: floor a statement about the policy rather than about one library version. +_RSA_TRANSPORT_KEX_PREFIXES = ("rsa1024-", "rsa2048-") + + +# --- ciphers: the bulk-encryption cell (TLS ``Enc=AES``/``AESGCM``/``CHACHA20``) ------------------ +# +# AES-CBC survives deliberately. ``harden_cipher_suites`` measured and REJECTED a narrowing that would +# have dropped the CBC-SHA2 TLS suites real hospital peers still speak; dropping SSH's AES-CBC here +# would re-introduce on the SSH hop exactly the interop regression the TLS side declined to take. +_ALLOWED_CIPHER_PREFIXES = ("aes", "chacha20-poly1305") +#: Below-floor bulk ciphers: 64-bit block sizes (Sweet32-class birthday collisions on a long-lived +#: session), broken stream ciphers, and the null cipher. ``aes`` names are screened separately below so +#: a hypothetical weak AES mode cannot ride in on the prefix. +_BELOW_FLOOR_CIPHERS = frozenset( + { + "3des-cbc", + "3des-ctr", + "des-cbc", + "des", + "blowfish-cbc", + "blowfish-ctr", + "cast128-cbc", + "cast128-ctr", + "arcfour", + "arcfour128", + "arcfour256", + "idea-cbc", + "serpent128-cbc", + "twofish128-cbc", + "none", + } +) + + +# --- macs: the integrity cell (TLS ``Mac=AEAD``/``SHA256``/``SHA384``) --------------------------- +# +# SHA-2 HMACs and umac-128 only. hmac-sha1 is included in the refusal even though it is not yet +# practically forgeable, because the TLS side's effective set contains no SHA-1 MAC either and the +# point of this module is that the two hops agree. +_ALLOWED_MAC_PREFIXES = ("hmac-sha2-256", "hmac-sha2-512", "umac-128") +#: Matched as SUBSTRINGS, not whole names: ``hmac-md5-96`` and a hypothetical ``hmac-none`` are as +#: much below the floor as ``hmac-md5`` and ``none``, and a whole-name set would have to enumerate +#: every truncation and vendor spelling to say so. +_BELOW_FLOOR_MAC_TOKENS = ("md5", "sha1", "ripemd", "umac-64", "none") + + +def ssh_algorithm_verdict(category: str, name: str) -> FloorVerdict: + """Classify one SSH algorithm ``name`` in ``category`` against the floor. + + ``category`` is one of :data:`SSH_ALGORITHM_CATEGORIES`. Names are compared with their + ``@domain`` vendor suffix (``@openssh.com``, ``@libssh.org``) stripped, since that suffix names the + originating implementation and never changes the cryptography. An unrecognised ``category`` is + itself :attr:`FloorVerdict.UNKNOWN` -- a new paramiko algorithm class must be classified here + deliberately, not admitted by falling off the end of a chain of ``if``\\ s.""" + bare = name.split("@", 1)[0].strip().lower() + if category == "kex": + return _kex_verdict(bare) + if category == "ciphers": + return _cipher_verdict(bare) + if category == "macs": + return _mac_verdict(bare) + return FloorVerdict.UNKNOWN + + +def _kex_verdict(bare: str) -> FloorVerdict: + if bare.startswith(_RSA_TRANSPORT_KEX_PREFIXES): + return FloorVerdict.BELOW # RSA key transport: no ephemeral DH, so no forward secrecy + if not bare.startswith(_FORWARD_SECRET_KEX_PREFIXES): + return FloorVerdict.UNKNOWN + # Forward-secret family, but the exchange hash must clear the MAC cell too: the TLS side's 17 + # suites hash with SHA-256 or better, so a SHA-1 exchange hash is below the same floor. + if bare.endswith(("-sha1", "-md5")): + return FloorVerdict.BELOW + return FloorVerdict.ABOVE + + +def _cipher_verdict(bare: str) -> FloorVerdict: + if bare in _BELOW_FLOOR_CIPHERS: + return FloorVerdict.BELOW + if not bare.startswith(_ALLOWED_CIPHER_PREFIXES): + return FloorVerdict.UNKNOWN + return FloorVerdict.ABOVE + + +def _mac_verdict(bare: str) -> FloorVerdict: + # Token test first: it must beat the allow-prefixes, so a name like ``hmac-sha2-256-md5`` cannot + # be admitted by its prefix. Order is load-bearing. + if any(token in bare for token in _BELOW_FLOOR_MAC_TOKENS): + return FloorVerdict.BELOW + if not bare.startswith(_ALLOWED_MAC_PREFIXES): + return FloorVerdict.UNKNOWN + return FloorVerdict.ABOVE + + +def ssh_disabled_algorithms( + offer: Mapping[str, Sequence[str]], *, connector: str +) -> dict[str, list[str]]: + """Derive paramiko's ``disabled_algorithms`` from ``offer``, and **assert** what survives. + + ``offer`` maps each :data:`SSH_ALGORITHM_CATEGORIES` key to the algorithm names the SSH library + would propose (paramiko's ``Transport._preferred_kex`` / ``_preferred_ciphers`` / + ``_preferred_macs``). Returns the mapping to hand to ``SSHClient.connect(disabled_algorithms=...)``: + every name this floor rates :attr:`FloorVerdict.BELOW`, so it can never be negotiated. + + Raises :class:`ValueError` -- the same class the sibling TLS hardening raises, so it surfaces + through the existing connector error handling rather than as a wire-time surprise -- when + + * a surviving name is :attr:`FloorVerdict.UNKNOWN`: the floor cannot vouch for it, and admitting + an algorithm nobody classified is how an inherited property stops being a checked one; or + * a category is missing from ``offer``, or the floor would empty it: a client that can offer no + key exchange at all would otherwise fail at the wire with a message about the peer, blaming the + partner for a floor that is ours. + + Measured against paramiko 5.0.0 (the version the lock resolves): 7 kex, all above the floor; + 9 ciphers, ``3des-cbc`` below; 8 macs, ``hmac-md5``, ``hmac-md5-96``, ``hmac-sha1`` and + ``hmac-sha1-96`` below. So the floor prunes 5 of 24 offered names, leaving intact at least the + AES-CTR/GCM/CBC ciphers and the SHA-2 MACs that current OpenSSH releases offer by default -- which + is the point: it asserts a floor rather than maximising strictness.""" + disabled: dict[str, list[str]] = {} + for category in SSH_ALGORITHM_CATEGORIES: + names = offer.get(category) + if names is None: + raise ValueError( + f"{connector}: the SSH library offered no {category!r} algorithm list, so the " + f"algorithm floor cannot be applied to it. Refusing to connect with an unchecked " + f"key exchange, cipher or MAC rather than inheriting whatever is negotiated." + ) + verdicts = [(n, ssh_algorithm_verdict(category, n)) for n in names] + below = [n for n, v in verdicts if v is FloorVerdict.BELOW] + unknown = [n for n, v in verdicts if v is FloorVerdict.UNKNOWN] + if unknown: + raise ValueError( + f"{connector}: the SSH library offers {category} algorithm(s) " + f"{', '.join(sorted(unknown))} that the MessageFoundry algorithm floor does not " + f"recognise, so it cannot confirm they meet the forward-secrecy / no-MD5 / no-SHA-1 " + f"floor asserted on every other transport hop. Classify them in " + f"messagefoundry/config/ssh_policy.py before this connection can be used." + ) + if len(below) == len(verdicts): + raise ValueError( + f"{connector}: every {category} algorithm the SSH library offers " + f"({', '.join(names)}) is below the MessageFoundry algorithm floor, so the client " + f"would offer none at all. This is a library/policy mismatch, not a partner fault." + ) + if below: + disabled[category] = below + return disabled + + +def ssh_floor_refusal( + peer_message: str, + *, + connector: str, + host: str, + port: int, + disabled: Mapping[str, Sequence[str]], + offered: Mapping[str, Sequence[str]], +) -> str | None: + """Explain an SSH negotiation failure that the floor is responsible for -- or return ``None``. + + A floor that fails closed must not fail *silently*: a partner this floor now refuses would + otherwise surface as paramiko's bare ``Incompatible ssh server (no acceptable macs)``, which names + neither the algorithms the engine refused nor the ones the partner could enable instead, and reads + like a partner defect. Given paramiko's own ``peer_message``, this returns an operator-actionable + replacement naming the category, what was refused and why, and what the server must offer. + + Returns ``None`` when ``peer_message`` names an incompatibility outside this floor's remit (a host + key, an SSH protocol version), so the caller keeps its existing generic message rather than + blaming a floor that had nothing to do with it. Some negotiation failures are genuinely the + partner's; only the ones the floor could have caused are re-described here.""" + lowered = peer_message.lower() + for category, token in _PEER_MESSAGE_TOKENS.items(): + # paramiko's phrasing is "Incompatible ssh peer (no acceptable kex algorithm)" and + # "Incompatible ssh server (no acceptable ciphers|macs)". The live negotiation tests drive + # real refusals through here, so a paramiko release that renames these fails loud rather + # than quietly degrading this explanation back to the generic message. + if token not in lowered: + continue + refused = list(disabled.get(category, ())) + if not refused: + return None # the floor pruned nothing here, so it cannot be the cause + pruned = set(refused) + surviving = [n for n in offered.get(category, ()) if n not in pruned] + label = _CATEGORY_LABELS[category] + return ( + f"{connector}: {host}:{port} offered no {label} this engine will accept " + f"({peer_message}). The MessageFoundry algorithm floor refuses {', '.join(refused)} on " + f"the SSH hop -- below the forward-secrecy / no-MD5 / no-SHA-1 floor asserted on every " + f"other transport hop, so a recorded session protected by one of them would be readable " + f"or forgeable on a later key compromise. Enable one of {', '.join(surviving)} on the " + f"server, or move the feed to a partner endpoint that supports them." + ) + return None diff --git a/messagefoundry/transports/remotefile.py b/messagefoundry/transports/remotefile.py index bf68157a..66555485 100644 --- a/messagefoundry/transports/remotefile.py +++ b/messagefoundry/transports/remotefile.py @@ -7,7 +7,9 @@ - ``sftp`` — SSH file transfer (paramiko, the ``[sftp]`` extra — lazily imported, so installs that never use SFTP skip it). **Host-key verification is ON by default**; an unknown key is refused unless the explicit dev escape ``MEFOR_ALLOW_INSECURE_TLS`` is set (and logged loudly when it is), - mirroring the SQL Server backend's weakened-TLS posture. + mirroring the SQL Server backend's weakened-TLS posture. The negotiated key exchange, cipher and + MAC are held to the same floor the TLS hops assert on their contexts — see + :mod:`messagefoundry.config.ssh_policy`, which is where that floor is defined and justified. - ``ftp`` — plain FTP (stdlib ``ftplib``). Cleartext: credentials over plain ``ftp`` are **refused** unless the escape is set (use ``ftps``/``sftp``), mirroring :func:`refuse_cleartext_credentials`. - ``ftps`` — FTP over explicit TLS (``ftplib.FTP_TLS`` + ``PROT P``), credentials encrypted. **The @@ -55,6 +57,10 @@ INSECURE_TLS_ESCAPE_ENV, weakened_tls_escape_permitted_here, ) +from messagefoundry.config.ssh_policy import ( + ssh_disabled_algorithms, + ssh_floor_refusal, +) from messagefoundry.config.tls_policy import ( TrustAnchorPolicy, build_verifying_client_context, @@ -384,8 +390,57 @@ def __init__(self, settings: dict[str, Any]) -> None: INSECURE_TLS_ESCAPE_ENV, ) + @property + def _connector(self) -> str: + """The label the algorithm-floor errors identify this hop by, matching the log-line style.""" + return f"REMOTEFILE sftp {self._host}" + + def _algorithm_offer(self, paramiko: Any) -> dict[str, list[str]]: + """The key-exchange / cipher / MAC names the SSH library would propose (#178). + + The only thing this reads from paramiko; the policy applied to it lives in + ``config/ssh_policy.py``, which is also where the floor is justified. Read through + ``getattr`` so a library that stops exposing one of the lists leaves that category ABSENT, + which makes :func:`ssh_disabled_algorithms` refuse rather than leaving it silently + unfloored. Recomputed per connect rather than cached: it is an attribute read next to a TCP + connect and a key exchange, and a cache keyed on nothing would outlive a test's fake + library.""" + transport = getattr(paramiko, "Transport", None) + raw = { + "kex": getattr(transport, "_preferred_kex", None), + "ciphers": getattr(transport, "_preferred_ciphers", None), + "macs": getattr(transport, "_preferred_macs", None), + } + return {category: list(names) for category, names in raw.items() if names is not None} + + def _floor_refusal(self, paramiko: Any, exc: Exception) -> str | None: + """The operator-actionable text for a negotiation failure the floor caused, else ``None``. + + Recomputes the floor rather than carrying it on the instance: this runs on an error path, and + state stashed during a connect would be stale (or absent) for a connect that never got that + far. A derivation failure here yields ``None`` so the caller reports the ORIGINAL negotiation + error -- an error path must not raise a second, unrelated exception over the first.""" + try: + offer = self._algorithm_offer(paramiko) + disabled = ssh_disabled_algorithms(offer, connector=self._connector) + except ValueError: + return None + return ssh_floor_refusal( + str(exc), + connector=self._connector, + host=self._host, + port=self._port, + disabled=disabled, + offered=offer, + ) + def _connect(self) -> Any: paramiko = _import_paramiko() + # Applied here rather than passed in, so there is no parameter a caller could omit: an + # algorithm floor that one code path can skip is not a floor. + disabled = ssh_disabled_algorithms( + self._algorithm_offer(paramiko), connector=self._connector + ) client = paramiko.SSHClient() client.load_system_host_keys() if self._known_hosts: @@ -405,6 +460,7 @@ def _connect(self) -> Any: timeout=self._timeout, allow_agent=False, look_for_keys=False, + disabled_algorithms=disabled, ) return client @@ -477,8 +533,21 @@ def _op(self, fn: Callable[[Any], _T]) -> _T: ) from exc except paramiko.SSHException as exc: # SSHException covers an unknown/rejected host key (RejectPolicy) — a security stop the - # operator must resolve, so it's permanent, not a retry. - raise _RemoteError(f"SFTP connection rejected: {exc}", permanent=True) from exc + # operator must resolve, so it's permanent, not a retry. It ALSO covers a peer that offers + # nothing above the algorithm floor; that refusal gets its own message naming what was + # refused and what the server could enable instead, because "SFTP connection rejected: + # Incompatible ssh server (no acceptable macs)" reads like a partner defect and gives an + # operator nothing to act on. `ssh_floor_refusal` returns None for every incompatibility + # the floor did not cause, so the generic message still covers those. + raise _RemoteError( + self._floor_refusal(paramiko, exc) or f"SFTP connection rejected: {exc}", + permanent=True, + ) from exc + except ValueError as exc: + # The floor derivation itself refused (an unclassified algorithm, or a library exposing no + # preferred list) — or a malformed private key. Both are operator-fixable configuration, + # never transient, so retrying the backlog against them would be pure noise. + raise _RemoteError(f"SFTP connect refused: {exc}", permanent=True) from exc except (OSError, EOFError) as exc: raise _RemoteError(f"SFTP connect failed: {exc}", permanent=False) from exc try: diff --git a/tests/test_remotefile_transport.py b/tests/test_remotefile_transport.py index a4c85740..bfa783a1 100644 --- a/tests/test_remotefile_transport.py +++ b/tests/test_remotefile_transport.py @@ -782,8 +782,22 @@ class _AuthException(Exception): pass +class _FakeTransport: + """Stands in for ``paramiko.Transport`` so the #178 algorithm floor has an offer to read. + + Deliberately a MINIMAL above-floor set rather than a copy of paramiko's real defaults: these + host-key tests must keep measuring the host-key policy, and a fake that drifted below the floor + would make them pass for the wrong reason (a floor refusal also raises permanently). If the floor + ever rejects these three, that is a genuine failure worth seeing here.""" + + _preferred_kex = ("curve25519-sha256@libssh.org",) + _preferred_ciphers = ("aes256-ctr",) + _preferred_macs = ("hmac-sha2-256",) + + class _FakeParamiko: SSHClient = _FakeSSHClient + Transport = _FakeTransport RejectPolicy = _RejectPolicy AutoAddPolicy = _AutoAddPolicy SSHException = _SSHException diff --git a/tests/test_ssh_algorithm_floor.py b/tests/test_ssh_algorithm_floor.py new file mode 100644 index 00000000..79450ed0 --- /dev/null +++ b/tests/test_ssh_algorithm_floor.py @@ -0,0 +1,444 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The SSH (SFTP) algorithm floor -- BACKLOG #178, policy half. + +**Why these tests drive a real key exchange.** The engine's other transport hops assert their cipher +floor on an ``SSLContext``, and the TLS suite's own master-test-plan entry records the weakness in +testing that by attribute: the floor is proven by construction, never observed. A floor tested that +way would pass identically if ``disabled_algorithms`` never reached the connect call, or if paramiko +ignored it. So the negotiation tests here stand up a live paramiko SSH server on loopback, restrict +what it will speak, and observe what the engine's own ``_SftpClient`` does about it. + +**Red-first evidence.** Measured 2026-08-10 against this same harness, on the code before the floor +(origin/main d5ff1804), the engine CONNECTED to a server offering only ``hmac-md5`` (negotiating +``mac=hmac-md5``), only ``hmac-sha1``, and only ``3des-cbc`` (negotiating ``cipher=3des-cbc``). Every +one of those is below what the shipped TLS contexts already negotiate. The three refusal cases below +are exactly those three; each connected before the change and is refused after it. + +**Skip posture.** The pure-policy tests import no SSH library and always run -- that is the same +property that keeps the ``[sftp]`` extra lazily importable. Only the live-negotiation tests need +paramiko, and they SKIP without it; a run reporting green with those skipped has not tested the +enforcement, only the policy. State which ones ran when reporting on this file. + +No PHI is involved: the server serves nothing, the client never opens a file, and the host key is +generated per run. +""" + +from __future__ import annotations + +import importlib.util +import logging +import socket +import threading +from collections.abc import Iterator, Sequence +from contextlib import contextmanager +from pathlib import Path +from typing import Any + +import pytest + +from messagefoundry.config.ssh_policy import ( + SSH_ALGORITHM_CATEGORIES, + FloorVerdict, + ssh_algorithm_verdict, + ssh_disabled_algorithms, + ssh_floor_refusal, +) +from messagefoundry.transports.remotefile import _RemoteError, _SftpClient + +_HAS_PARAMIKO = importlib.util.find_spec("paramiko") is not None +needs_paramiko = pytest.mark.skipif( + not _HAS_PARAMIKO, reason="the [sftp] extra is not installed, so no negotiation can be driven" +) + + +# === the pure policy: no SSH library required ================================ + + +@pytest.mark.parametrize( + ("category", "name", "expected"), + [ + # kex: the forward-secrecy cell (TLS Kx=ECDH/DH) + ("kex", "curve25519-sha256@libssh.org", FloorVerdict.ABOVE), + ("kex", "ecdh-sha2-nistp521", FloorVerdict.ABOVE), + ("kex", "diffie-hellman-group16-sha512", FloorVerdict.ABOVE), + ("kex", "sntrup761x25519-sha512@openssh.com", FloorVerdict.ABOVE), + # RFC 4432 RSA key TRANSPORT: no ephemeral DH at all, the SSH static-RSA analogue + ("kex", "rsa2048-sha256", FloorVerdict.BELOW), + ("kex", "rsa1024-sha1", FloorVerdict.BELOW), + # forward-secret family, SHA-1 exchange hash: below the same cell that bars a SHA-1 TLS MAC + ("kex", "diffie-hellman-group14-sha1", FloorVerdict.BELOW), + ("kex", "diffie-hellman-group1-sha1", FloorVerdict.BELOW), + ("kex", "some-future-kex-sha256", FloorVerdict.UNKNOWN), + # ciphers: the bulk-encryption cell (TLS Enc=AES/AESGCM/CHACHA20) + ("ciphers", "aes256-gcm@openssh.com", FloorVerdict.ABOVE), + ("ciphers", "aes128-cbc", FloorVerdict.ABOVE), + ("ciphers", "chacha20-poly1305@openssh.com", FloorVerdict.ABOVE), + ("ciphers", "3des-cbc", FloorVerdict.BELOW), + ("ciphers", "arcfour256", FloorVerdict.BELOW), + ("ciphers", "none", FloorVerdict.BELOW), + ("ciphers", "kuznyechik-ctr", FloorVerdict.UNKNOWN), + # macs: the integrity cell (TLS Mac=AEAD/SHA256/SHA384) + ("macs", "hmac-sha2-256", FloorVerdict.ABOVE), + ("macs", "hmac-sha2-512-etm@openssh.com", FloorVerdict.ABOVE), + ("macs", "umac-128-etm@openssh.com", FloorVerdict.ABOVE), + ("macs", "hmac-md5", FloorVerdict.BELOW), + ("macs", "hmac-md5-96", FloorVerdict.BELOW), + ("macs", "hmac-sha1", FloorVerdict.BELOW), + ("macs", "hmac-sha1-96", FloorVerdict.BELOW), + ("macs", "hmac-ripemd160", FloorVerdict.BELOW), + ("macs", "umac-64@openssh.com", FloorVerdict.BELOW), + ("macs", "hmac-blake3", FloorVerdict.UNKNOWN), + # an unclassified CATEGORY is unknown, not silently admitted + ("compression", "zlib", FloorVerdict.UNKNOWN), + ], +) +def test_algorithm_verdicts(category: str, name: str, expected: FloorVerdict) -> None: + assert ssh_algorithm_verdict(category, name) is expected + + +def test_a_below_floor_token_beats_an_above_floor_prefix() -> None: + """Order inside ``_mac_verdict`` is load-bearing and easy to invert while every other test stays + green: a name that starts ``hmac-sha2-256`` must still be refused if it carries an MD5 token.""" + assert ssh_algorithm_verdict("macs", "hmac-sha2-256-md5@example.com") is FloorVerdict.BELOW + + +def test_an_unrecognised_survivor_raises_rather_than_being_offered() -> None: + """The assertion half. An algorithm nobody classified is precisely the inherited-never-checked + state this item is about, so it must be loud -- and it must name the algorithm and the + connection, or an operator cannot act on it.""" + offer = { + "kex": ["curve25519-sha256@libssh.org", "quantum-widget-kex"], + "ciphers": ["aes256-ctr"], + "macs": ["hmac-sha2-256"], + } + with pytest.raises(ValueError, match="quantum-widget-kex") as ei: + ssh_disabled_algorithms(offer, connector="REMOTEFILE sftp p.example.org") + assert "REMOTEFILE sftp p.example.org" in str(ei.value) + + +def test_a_missing_category_raises_rather_than_going_unfloored() -> None: + """Fail closed on a library that stops exposing a list. Skipping the category would leave the hop + negotiating an unchecked cipher while every test still passed -- a control that silently stops + covering something is worse than one that was never built.""" + with pytest.raises(ValueError, match="macs"): + ssh_disabled_algorithms( + {"kex": ["curve25519-sha256@libssh.org"], "ciphers": ["aes256-ctr"]}, + connector="test", + ) + + +def test_a_category_the_floor_would_empty_raises_and_blames_the_policy_not_the_partner() -> None: + with pytest.raises(ValueError, match="library/policy mismatch") as ei: + ssh_disabled_algorithms( + {"kex": ["rsa2048-sha256"], "ciphers": ["aes256-ctr"], "macs": ["hmac-sha2-256"]}, + connector="test", + ) + assert "rsa2048-sha256" in str(ei.value) + + +def test_the_refusal_explains_only_failures_the_floor_could_have_caused() -> None: + """A floor that explained every negotiation failure as its own doing would send operators to + edit their MACs list over a host-key mismatch. ``None`` means keep the generic message.""" + offered = {"macs": ["hmac-sha2-256", "hmac-md5"]} + assert ( + ssh_floor_refusal( + "Incompatible ssh peer (no acceptable host key)", + connector="c", + host="h", + port=22, + disabled={"macs": ["hmac-md5"]}, + offered=offered, + ) + is None + ) + assert ( + ssh_floor_refusal( + "Incompatible ssh server (no acceptable macs)", + connector="c", + host="h", + port=22, + disabled={}, # the floor pruned no MAC, so it is not the cause + offered=offered, + ) + is None + ) + explained = ssh_floor_refusal( + "Incompatible ssh server (no acceptable macs)", + connector="REMOTEFILE sftp h", + host="h", + port=2222, + disabled={"macs": ["hmac-md5"]}, + offered=offered, + ) + assert explained is not None + assert "hmac-md5" in explained # what was refused + assert "hmac-sha2-256" in explained # what the partner could enable instead + assert "h:2222" in explained # which partner + + +def test_every_governed_category_has_an_operator_label() -> None: + """Liveness receipt for the refusal text: a category added to the floor with no label would raise + a KeyError inside an error path, replacing an actionable refusal with a crash.""" + from messagefoundry.config import ssh_policy + + assert set(ssh_policy._CATEGORY_LABELS) == set(SSH_ALGORITHM_CATEGORIES) + assert set(ssh_policy._PEER_MESSAGE_TOKENS) == set(SSH_ALGORITHM_CATEGORIES) + + +# === the floor's effect on the SHIPPED library =============================== + + +def _paramiko_offer() -> dict[str, list[str]]: + import paramiko + + return { + "kex": list(paramiko.Transport._preferred_kex), + "ciphers": list(paramiko.Transport._preferred_ciphers), + "macs": list(paramiko.Transport._preferred_macs), + } + + +@needs_paramiko +def test_the_default_offer_is_pruned_exactly_where_the_tls_floor_would_prune_it() -> None: + """The floor is the TLS floor re-expressed, so its effect on the shipped library must be the + measured set and no more. + + Pinned as an EQUALITY, not a membership: a floor that quietly grew to refuse ``aes128-cbc`` would + still satisfy "refuses 3des-cbc", and would break ordinary partners with nothing going red. + """ + disabled = ssh_disabled_algorithms(_paramiko_offer(), connector="test") + assert disabled.get("kex", []) == [], ( + "paramiko's default kex list is already forward-secret end to end; a non-empty prune here " + "means either paramiko regressed or the kex predicate did" + ) + assert set(disabled.get("ciphers", [])) == {"3des-cbc"} + assert set(disabled.get("macs", [])) == { + "hmac-sha1", + "hmac-sha1-96", + "hmac-md5", + "hmac-md5-96", + } + + +@needs_paramiko +def test_the_offer_left_standing_is_what_a_modern_ssh_server_speaks() -> None: + """Constraint check on the floor itself: it must not be set so high that ordinary partners fail. + + ``hmac-sha2-256`` and ``aes*-ctr``/``aes*-gcm`` are in the default MACs/Ciphers of every OpenSSH + release still supported, so an intersection containing them is the evidence that this asserts a + floor rather than maximising strictness. AES-CBC surviving is deliberate and mirrors the TLS + side's measured decision not to drop the CBC-SHA2 suites hospital peers still speak. + """ + offer = _paramiko_offer() + disabled = ssh_disabled_algorithms(offer, connector="test") + surviving = {c: [n for n in offer[c] if n not in disabled.get(c, [])] for c in offer} + assert "hmac-sha2-256" in surviving["macs"] + assert "hmac-sha2-512" in surviving["macs"] + assert "aes256-ctr" in surviving["ciphers"] + assert "aes256-gcm@openssh.com" in surviving["ciphers"] + assert "aes128-cbc" in surviving["ciphers"], ( + "AES-CBC must survive: dropping it re-introduces on the SSH hop the interop regression " + "harden_cipher_suites measured and declined to take on the TLS hop" + ) + assert "curve25519-sha256@libssh.org" in surviving["kex"] + + +# === live negotiation against a real paramiko SSH server ===================== + + +@pytest.fixture(scope="module") +def host_key() -> Any: + """A throwaway server host key. ECDSA rather than RSA purely for generation speed (measured + instant vs. hundreds of milliseconds); the key type is not what these tests are about.""" + paramiko = pytest.importorskip("paramiko") + return paramiko.ECDSAKey.generate() + + +@pytest.fixture(autouse=True) +def _quiet_paramiko() -> Iterator[None]: + """paramiko logs a full traceback from its transport thread on every negotiation failure. These + tests CAUSE such failures deliberately, so that noise is expected output, not a signal.""" + log = logging.getLogger("paramiko") + before = log.level + log.setLevel(logging.CRITICAL) + try: + yield + finally: + log.setLevel(before) + + +def _accept_any_password(paramiko: Any) -> Any: + """The narrowest server that gets a client through key exchange and authentication. It offers no + channels, so a test can observe the negotiated transport without an SFTP subsystem existing. + + Built inside a function so this module imports with no SSH library present.""" + + class _AcceptAnyPassword(paramiko.ServerInterface): # type: ignore[misc] + def check_auth_password(self, username: str, password: str) -> int: + return int(paramiko.AUTH_SUCCESSFUL) + + def get_allowed_auths(self, username: str) -> str: + return "password" + + return _AcceptAnyPassword() + + +def _serve_one(listener: socket.socket, host_key: Any, prefs: dict[str, Sequence[str]]) -> None: + """Accept ONE connection and speak SSH with the algorithm lists in ``prefs``. + + The ``_preferred_*`` overrides are set on the instance after construction, which is where + paramiko reads them during ``_parse_kex_init`` -- that is how a test stands up a partner which + speaks only a legacy algorithm.""" + import paramiko + + try: + conn, _addr = listener.accept() + except OSError: # pragma: no cover - listener closed before a client arrived + return + transport = paramiko.Transport(conn) + for category, names in prefs.items(): + setattr(transport, f"_preferred_{category}", tuple(names)) + transport.add_server_key(host_key) + try: + transport.start_server(server=_accept_any_password(paramiko)) + transport.join(timeout=10) + except paramiko.SSHException: + pass # the refusal under test, seen from the far end + finally: + transport.close() + + +@contextmanager +def _partner(tmp_path: Path, host_key: Any, **prefs: Sequence[str]) -> Iterator[tuple[Any, int]]: + """Start a one-shot SSH server on loopback and yield ``(_SftpClient, port)`` pointed at it. + + The server's host key is written into a ``known_hosts`` the client is configured with, so the + connector's default RejectPolicy is satisfied and host-key verification is NOT what these tests + accidentally end up measuring.""" + import paramiko + + listener = socket.socket() + listener.bind(("127.0.0.1", 0)) + listener.listen(1) + port = int(listener.getsockname()[1]) + thread = threading.Thread( + target=_serve_one, args=(listener, host_key, dict(prefs)), daemon=True + ) + thread.start() + + known_hosts = tmp_path / f"known_hosts_{port}" + keys = paramiko.HostKeys() + keys.add(f"[127.0.0.1]:{port}", host_key.get_name(), host_key) + keys.save(str(known_hosts)) + + client = _SftpClient( + { + "host": "127.0.0.1", + "port": port, + "username": "u", + "password": "p", + "known_hosts": str(known_hosts), + "connect_timeout": 15.0, + } + ) + try: + yield client, port + finally: + listener.close() + thread.join(timeout=10) + + +@needs_paramiko +@pytest.mark.parametrize( + ("label", "prefs", "refused"), + [ + # Each of these CONNECTED before the floor; measured 2026-08-10 on origin/main d5ff1804. + ("md5 mac", {"macs": ["hmac-md5"], "ciphers": ["aes128-ctr"]}, "hmac-md5"), + ("sha1 mac", {"macs": ["hmac-sha1"], "ciphers": ["aes128-ctr"]}, "hmac-sha1"), + ("3des", {"ciphers": ["3des-cbc"], "macs": ["hmac-sha2-256"]}, "3des-cbc"), + ], +) +def test_a_below_floor_partner_is_refused_with_an_actionable_message( + tmp_path: Path, host_key: Any, label: str, prefs: dict[str, Sequence[str]], refused: str +) -> None: + """Fail closed, but not silently. + + The assertion is on the MESSAGE, not merely on the raise: a timeout or a bare + ``Incompatible ssh server (no acceptable macs)`` would also "refuse", and would leave an operator + with a broken feed and no sign the engine did it deliberately. The refusal must name the + algorithm refused, say why, and name something the partner can enable instead. + """ + with ( + _partner(tmp_path, host_key, **prefs) as (client, port), + pytest.raises(_RemoteError) as ei, + ): + client.list_dir("/in") + message = str(ei.value) + assert refused in message, f"{label}: refusal does not name what was refused: {message}" + assert "algorithm floor" in message + assert f"127.0.0.1:{port}" in message # which partner + assert "Enable one of" in message # what to do about it + assert "hmac-sha2-256" in message or "aes256-ctr" in message # a concrete alternative + assert ei.value.permanent is True, "a floor refusal cannot be fixed by retrying" + + +@needs_paramiko +def test_an_ordinary_modern_partner_still_negotiates(tmp_path: Path, host_key: Any) -> None: + """The floor must not break reachability for a partner anyone actually runs. + + ``aes256-gcm`` + ``hmac-sha2-256`` is inside the default offer of every supported OpenSSH. + Asserted on the NEGOTIATED algorithm from the live transport, so this cannot pass by the connect + silently not happening.""" + with _partner( + tmp_path, host_key, ciphers=["aes256-gcm@openssh.com"], macs=["hmac-sha2-256"] + ) as (client, _port): + connected = client._connect() + try: + transport = connected.get_transport() + assert transport is not None and transport.is_active() + assert transport.local_cipher == "aes256-gcm@openssh.com" + finally: + connected.close() + + +@needs_paramiko +def test_a_partner_on_the_libraries_own_defaults_still_negotiates( + tmp_path: Path, host_key: Any +) -> None: + """The unrestricted case: a server speaking everything paramiko does must still connect, and must + land ABOVE the floor rather than on one of the algorithms the floor refuses.""" + with _partner(tmp_path, host_key) as (client, _port): + connected = client._connect() + try: + transport = connected.get_transport() + assert transport is not None and transport.is_active() + assert transport.local_cipher != "3des-cbc" + assert transport.local_mac not in {"hmac-md5", "hmac-md5-96", "hmac-sha1"} + finally: + connected.close() + + +@needs_paramiko +def test_the_connect_call_actually_carries_the_floor( + tmp_path: Path, host_key: Any, monkeypatch: pytest.MonkeyPatch +) -> None: + """Wiring receipt. The negotiation tests prove the floor takes effect; this proves WHERE, so a + refactor that moved the derivation somewhere the connect call no longer sees goes red here and + not only in the slower live tests.""" + import paramiko + + seen: dict[str, Any] = {} + real_connect = paramiko.SSHClient.connect + + def spy(self: Any, *args: Any, **kwargs: Any) -> Any: + seen.update(kwargs) + return real_connect(self, *args, **kwargs) + + monkeypatch.setattr(paramiko.SSHClient, "connect", spy) + with _partner(tmp_path, host_key) as (client, _port): + connected = client._connect() + connected.close() + assert "disabled_algorithms" in seen, "the connect call was made without the floor" + assert "3des-cbc" in seen["disabled_algorithms"]["ciphers"] + assert "hmac-md5" in seen["disabled_algorithms"]["macs"]