From c8bb16388bed970ef75bf25982e8f8d1c2bfe462 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 10 Aug 2026 18:16:42 -0500 Subject: [PATCH] feat(sftp): assert an algorithm floor on the SSH hop, mapped from the TLS one (#178) The SSH hop was the one transport hop with no cipher assertion. Measured 2026-08-10: harden_cipher_suites is called at 7 sites across 5 files (the API/UI listener, MLLP in both directions, both DICOM contexts, FTPS, and the SMTP alert sink), while disabled_algorithms appeared 0 times anywhere in the package. 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. config/ssh_policy.py is the missing assertion, and it is the TLS floor re-expressed rather than a second opinion. The shipped TLS contexts resolve to 17 suites whose measured properties are Kx in {ECDH, DH, any}, Enc in {AES, AESGCM, CHACHA20} and Mac in {AEAD, SHA256, SHA384}; each cell maps to one SSH rule (forward-secret key exchange, no 64-bit-block or broken cipher, no MD5/SHA-1 MAC). Two of those three are properties OpenSSL's default cipher string gives the TLS side for free and paramiko's defaults do not, which is why the SSH side has to name them. Two operations, deliberately separated. BELOW-floor names are disabled -- SSH negotiates from the offer, so pruning the offer IS the enforcement, unlike TLS where the context already resolves forward-secret and harden_cipher_suites only has to check it. Everything surviving is asserted recognisable, so an unclassified algorithm raises at connect naming itself rather than being quietly dropped and narrowing reachability with no signal. Against paramiko 5.0.0 the floor prunes 5 of 24 offered names: 3des-cbc, hmac-md5, hmac-md5-96, hmac-sha1, hmac-sha1-96. Every algorithm a current OpenSSH negotiates by default survives, AES-CBC included -- dropping it would re-introduce on this hop exactly the interop regression harden_cipher_suites measured and declined to take on TLS. Fails closed without failing silently: a partner offering nothing above the floor is refused with a permanent error naming the category, the algorithms refused, why they are refused, and what to enable instead -- not a timeout and not a bare "Incompatible ssh server (no acceptable macs)", which reads like a partner defect. paramiko stays lazily imported behind the [sftp] extra: ssh_policy imports it never, the caller passes the offer in, and an install without paramiko behaves exactly as it did. No operator override, deliberately, so nothing is added to security_loosenings. Nothing plausible needs loosening from this floor, and a switch nobody asked for is a second posture by the back door. If a partner ever forces the question it belongs there as a REQUIRED parameter, the way the TLS deviations do. The operator-configurable cipher/KEX/MAC allow-list that is #178's other half stays DEMAND-GATED and unbuilt -- this module has no configuration surface at all. Verified against a real key exchange rather than by attribute, because a floor asserted only on a constructed object passes identically when disabled_algorithms never reaches the connect call. tests/test_ssh_algorithm_floor.py stands up a live paramiko SSH server on loopback: the three algorithms that connected before are each refused after, with the message asserted, and a modern partner plus a paramiko-defaults partner both still negotiate. The pure-policy half imports no SSH library and runs without the extra. --- docs/CONNECTIONS.md | 8 + messagefoundry/config/ssh_policy.py | 318 +++++++++++++++++ messagefoundry/transports/remotefile.py | 75 +++- tests/test_remotefile_transport.py | 14 + tests/test_ssh_algorithm_floor.py | 444 ++++++++++++++++++++++++ 5 files changed, 856 insertions(+), 3 deletions(-) create mode 100644 messagefoundry/config/ssh_policy.py create mode 100644 tests/test_ssh_algorithm_floor.py 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"]