diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 053630c1..85fe84cf 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -8824,3 +8824,66 @@ larger change than this one. **Source:** found 2026-08-09 while probing for a second instance of the `#1106` class before building a generalised check, on the reasoning that a meta-check built from one instance is shaped like that instance. Two domains were probed; this one leaked. + +## 1206. `redacted_settings` served ODBC driver credentials sitting in `odbc_params` + +> 🔢 **Filed 2026-08-09 - FIXED IN THE SAME CHANGE, entry published WITH the fix.** Value **8/10** · Difficulty **3/10**. `redacted_settings` masks flat scalars and descended into `headers` alone, so a credential inside `odbc_params` was returned VERBATIM by `GET /connections/{name}/metadata` behind `MONITORING_READ` and printed by `graph --json` - on the SAME object whose top-level `password` masked correctly. + +**Cluster:** Security / secret disclosure. **Priority:** P1. **Verdict:** build (done). +**Severity:** on a first deployment, an ODBC driver password would be served to any monitoring reader +and written to stdout, a CI log and the IDE graph view. No PHI. + +**Measured, both serializers, before and after:** + +``` +BEFORE: odbc_params={"PWD": S, "sslpassword": S} -> both returned verbatim + password="p" on the same object -> '***' +AFTER : PWD, sslpassword, Password -> '***' + Encrypt, ApplicationIntent, + TrustServerCertificate, sslkey (a PATH) -> still readable +``` + +**IT IS NOT MERELY OPERATOR MISUSE, WHICH IS WHY IT MASKS RATHER THAN WARNS.** The docstring says +`odbc_params` "carries only static driver keywords", and the typed fields carry exactly ONE credential +(`username`/`password`, key names configurable via `odbc_user_key`/`odbc_password_key`). But +`_reject_envref_odbc_params` refuses `env()` there. So a connection needing a SECOND driver credential +- libpq `sslpassword` beside `PWD` - has no typed home and no `env()` form, and the inline literal is +the only expressible shape. **A refusal that removes the SAFE expression while leaving the UNSAFE one +is not a mitigation.** + +**THIS IS A DISPLAY FIX, NOT A STORAGE FIX** - stated because the difference matters and is easy to +lose. The credential remains an inline literal in the config file. Keeping it out of the file needs +`env()` to work here, which needs nested settings to be env-resolved. That changes the resolution path +and what the refusal above means, so it is the **route-onward** and is deliberately not folded in. + +**A THIRD PREDICATE, AND THE FIRST ATTEMPT PROVES WHY.** I reached for `_is_secret_setting` - and it +returns False for every one of `PWD`, `Password` and `sslpassword`, because it matches a fixed +frozenset of MessageFoundry SETTINGS names while these are ODBC DRIVER keywords with different +spellings and different case. **A fix shipped on that predicate would have masked nothing while reading +as a fix**, inside the change closing a defect whose whole shape is a control whose domain is narrower +than its surface. `_is_secret_odbc_key` is shape-based and case-insensitive; `pwd` is listed explicitly +because it is an abbreviation matching no substring rule. + +**THE GUARD WRITTEN AGAINST THIS CLASS WAS GREEN OVER IT, AND THAT IS THE REAL FINDING.** +`tests/test_connection_factory_redaction_domain.py` filtered its AST-derived domain through +`_decorator_style`, keeping **4 of 23** spec-returning functions and dropping every base constructor +including `Database`. Its docstring asserted "no shipped factory emits a nested container beyond those +declared below" and called the hole "THEORETICAL rather than live". **Both false.** That claim is +DELETED rather than softened - a number a test has not established has no business in the file defining +the test, and a hedged version keeps the authority while losing the falsifiability. + +The domain is now all 23, and `test_the_domain_covers_every_spec_returning_function` fails if any +discovered function is missing from it. Every other control in that file answers *is this instrument +working* - make it fail on purpose, confirm the injection landed, run a negative control, assert it +examined something. **None of them answers *is it pointed at the whole thing*.** The domain is a +separate claim and now carries its own evidence. + +**Found on the way, and worth more than the fix:** `Http` and `Soap` REFUSE an inline intake +credential outright and demand `env()`, so the value never resolves into settings and no serializer can +leak it. That is the stronger control `odbc_params` lacks, and it is now asserted by +`test_a_refusing_connector_actually_refuses_an_inline_credential` rather than left as folklore. + +**Source:** found 2026-08-09 by the `asvs-tracking-rework` session's independent assessment of ASVS +15.3.1, which I had recused from because I authored the two fixes bearing on that cell. Reproduced here +by execution before any code changed. This is the fourth instance of the class and the second time a +guard written after the previous instance picked a domain narrower than the surface. diff --git a/messagefoundry/config/wiring.py b/messagefoundry/config/wiring.py index 5d05c1fa..82605611 100644 --- a/messagefoundry/config/wiring.py +++ b/messagefoundry/config/wiring.py @@ -728,6 +728,41 @@ def _looks_like_a_credential_value(value: object) -> bool: return v.lower().startswith(_CREDENTIAL_VALUE_PREFIXES) or bool(_JWT_SHAPE.match(v)) +#: Credential-bearing ODBC/libpq driver keywords, by SHAPE and case-insensitively (BACKLOG #1206). +#: +#: A THIRD predicate rather than reuse, and the first attempt at this fix proves why. I reached for +#: :func:`_is_secret_setting` -- and it returned False for every one of ``PWD``, ``Password`` and +#: ``sslpassword``, because it matches a fixed frozenset of MessageFoundry SETTINGS names and these are +#: ODBC DRIVER keywords with different spellings and different case. A fix that shipped on that +#: predicate would have masked nothing while reading as a fix, in the change closing a defect whose +#: whole shape is a control whose domain is narrower than its surface. +#: +#: ``pwd`` is listed explicitly because it is an ABBREVIATION and matches no substring rule -- it is +#: also the single most common spelling in a SQL Server DSN. +_SECRET_ODBC_SUBSTRINGS = ( + "pwd", + "password", + "passwd", + "secret", + "token", + "credential", + "passphrase", +) + +#: ODBC keywords that carry a credential-ish substring and are PATHS, not material. Masking a path +#: hides configuration an operator needs to see and protects nothing: the file's contents never enter +#: settings. ``sslkey`` and ``sslcert`` are libpq file paths. +_NOT_SECRET_ODBC_KEYS = frozenset({"sslkey", "sslcert", "sslrootcert", "sslcrl"}) + + +def _is_secret_odbc_key(name: str) -> bool: + """Would printing this ODBC keyword's VALUE disclose a credential?""" + low = str(name).strip().lower() + if low in _NOT_SECRET_ODBC_KEYS or low.endswith(("_file", "_path")): + return False + return any(tok in low for tok in _SECRET_ODBC_SUBSTRINGS) + + def _is_secret_header(name: str, value: object = None) -> bool: """Would printing this header's VALUE disclose a credential? @@ -856,6 +891,31 @@ def redacted_settings(settings: Mapping[str, Any]) -> dict[str, Any]: out[name] = "***" elif name == "headers" and isinstance(value, dict): out[name] = {k: ("***" if _is_secret_header(k, v) else v) for k, v in value.items()} + elif name == "odbc_params" and isinstance(value, dict): + # BACKLOG #1206. This bag is documented as carrying "only static driver keywords", and the + # redactor honoured that by not descending -- so a credential inside it was served verbatim + # by /metadata behind MONITORING_READ and printed by graph --json, on the SAME object whose + # top-level `password` masked correctly. + # + # It is not merely operator misuse, which is why this masks rather than warns. The typed + # fields carry exactly ONE credential (`username`/`password`, key names configurable via + # `odbc_user_key`/`odbc_password_key`), and `_reject_envref_odbc_params` refuses `env()` + # here. So a connection needing a SECOND driver credential -- libpq `sslpassword` beside + # `PWD` -- has no typed home and no env() form, and the inline literal is the only + # expressible shape. A refusal that removes the SAFE expression while leaving the UNSAFE + # one is not a mitigation. + # + # Keys only, by the same predicate the rest of this function uses: real static driver + # keywords (`Encrypt`, `TrustServerCertificate`, `ApplicationIntent`) are not + # credential-shaped, and the ones that are -- `PWD`, `Password`, `sslpassword` -- are + # credentials. Values are NOT shape-tested here: a driver keyword's value is opaque and + # masking on its content would hide ordinary configuration with nothing to say so. + # + # THIS IS A DISPLAY FIX, NOT A STORAGE FIX. The credential remains an inline literal in + # the config file. Keeping it out of the file needs `env()` to work here, which needs + # nested settings to be env-resolved -- filed as #1206's route-onward, deliberately not + # folded in, because it changes the resolution path and what the refusal above means. + out[name] = {k: ("***" if _is_secret_odbc_key(k) else v) for k, v in value.items()} else: out[name] = value return out diff --git a/tests/test_connection_factory_redaction_domain.py b/tests/test_connection_factory_redaction_domain.py index 243a5b7f..dfcdb31b 100644 --- a/tests/test_connection_factory_redaction_domain.py +++ b/tests/test_connection_factory_redaction_domain.py @@ -2,8 +2,9 @@ # Copyright (C) 2026 MessageFoundry Organization and contributors """Every connection factory's EMITTED settings survive `redacted_settings` (BACKLOG #1106). -This is the third guard against one class, and the first two both failed the same way: **they chose a -domain narrower than the surface.** +This is the FOURTH guard against one class, and the first three ALL failed the same way: **they chose +a domain narrower than the surface.** The third was written by the author of this sentence, against +exactly that failure, and made it anyway. 1. The original exhaustive redaction test built its sentinels *from* ``_SECRET_SETTING_KEYS``, so a credential never added to that frozenset could not fail it. Four did exactly that -- ``ws_password``, @@ -20,9 +21,16 @@ secret survives.** No frozenset is consulted, no parameter name is inspected, and the sentinel values come from this test rather than from the thing under test. -**The domain is DERIVED, never listed.** It is every function in the package annotated to return a -``ConnectionSpec``, found by AST. A new factory joins the domain by existing; nobody has to remember to -add it here, which is the failure that produced defects 1 and 2 above. +3. This file's own first version DERIVED the domain by AST -- and then filtered it through + ``_decorator_style``, keeping 4 of 23. Every base constructor was dropped, so + ``Database.odbc_params`` served an ODBC password verbatim while the guard was green (BACKLOG + #1206). Deriving the domain is not enough if something narrows it afterwards. + +**The domain is DERIVED and its COVERAGE IS ASSERTED.** It is every function in the package annotated +to return a ``ConnectionSpec``, found by AST -- all 23, decorators and base constructors alike -- and +``test_the_domain_covers_every_spec_returning_function`` fails if any discovered function is missing +from it. That assertion is the lesson of defect 3: every other control here answers *is this instrument +working*; only that one answers *is it pointed at the whole thing*. """ from __future__ import annotations @@ -52,6 +60,16 @@ #: Keys that contain a credential-ish substring but are NOT secrets, each needing a stated reason. #: This is the only allowlist in the file and it is about KEYS, not about values. NOT_A_SECRET: dict[str, str] = { + "signing_key": ( + "a PATH to the sender's PEM/DER key, not the material -- wiring.py documents it as such and " + "transports/direct.py _read_file()s it. The engine classifies signing_key_password and NOT " + "signing_key deliberately; that asymmetry is correct, not an oversight" + ), + "odbc_password_key": ( + "the NAME of the ODBC keyword to put the password under (default 'PWD'), not the password. " + "Naming indirection, same class as sign_key_id" + ), + "odbc_user_key": "the NAME of the ODBC user keyword (default 'UID'), not the user", "sign_key_id": "a key IDENTIFIER (JWS kid) -- names which key, carries no key material", "key_id": "identifier, not material", "tls_key_file": "a PATH to key material, not the material; the file's contents never enter settings", @@ -112,53 +130,127 @@ def _factories_returning_a_spec() -> list[tuple[str, str]]: return found -def _decorator_style(mod: str, name: str) -> Any | None: - """Return the callable if it is a `(spec, *, ...) -> ConnectionSpec` decorator, else None. +def _resolve(mod: str, name: str) -> Any | None: + """Return the callable, or None if it cannot be imported. - Those are the ones that ADD settings to an existing spec, which is where a rename across the - parameter/setting boundary can happen. Base constructors are covered by their own connector tests. + NO STYLE FILTER. Its predecessor `_decorator_style` kept only `(spec, *, ...)` decorators, which + was 4 of 23 spec-returning functions -- every base connector constructor, including `Database`, + `Rest` and `MLLP`, was silently dropped. The `odbc_params` credential disclosure (BACKLOG #1206) + lives in `Database`, so the guard written against exactly this class could not see it. + + The filter was convenience presented as scope: decorators take an existing spec and base + constructors have to be built, which is more work. All 23 are constructible with naive arguments -- + measured, not assumed -- so there is no excuse left. If one ever stops being constructible, list it + explicitly as unreachable-by-this-test rather than letting a predicate drop it silently. """ fn = getattr(importlib.import_module(mod), name, None) - if fn is None or not callable(fn): - return None - try: - params = list(inspect.signature(fn).parameters.values()) - except (TypeError, ValueError): # pragma: no cover - return None - return fn if params and params[0].name == "spec" else None + return fn if callable(fn) else None + + +def _takes_a_spec(fn: Any) -> bool: + params = list(inspect.signature(fn).parameters.values()) + return bool(params) and params[0].name == "spec" def _call_with_sentinels(fn: Any) -> Any: - """Invoke the factory, passing SENTINEL for every credential-shaped keyword parameter.""" + """Invoke the factory with SENTINEL in every credential-shaped parameter. + + Handles BOTH shapes: a decorator gets a base spec as its first argument, a base constructor is + built outright. + """ + decorator = _takes_a_spec(fn) + params = list(inspect.signature(fn).parameters.values()) kwargs: dict[str, Any] = {} - for p in list(inspect.signature(fn).parameters.values())[1:]: + required: set[str] = set() + for p in params[1:] if decorator else params: if p.kind not in (p.KEYWORD_ONLY, p.POSITIONAL_OR_KEYWORD): continue - looks_secret = _is_credential_param(p.name) - if looks_secret: + ann = str(p.annotation) + if _is_credential_param(p.name): kwargs[p.name] = SENTINEL elif p.default is inspect.Parameter.empty: - # A required non-credential argument. Any plausible string keeps the call alive; the + required.add(p.name) + # A required non-credential argument. Any plausible value keeps the call alive; the # assertion is about credential values, not this one. - kwargs[p.name] = "https://example.invalid/token" if "url" in p.name else "x" + kwargs[p.name] = ( + "https://example.invalid/token" + if "url" in p.name.lower() + else 2575 + if ("int" in ann and "str" not in ann) + else "SELECT 1" + if "statement" in p.name + else "x" + ) elif _is_container_param(p): # OPTIONAL CONTAINER PARAMETERS ARE POPULATED DELIBERATELY. Passing only the REQUIRED - # arguments left every container empty, and the surface walk below skips empty containers - # -- so it examined nothing and passed vacuously. Measured: zero containers walked. A - # guard that passes because it looked at nothing is the exact defect this file exists for, - # and it very nearly shipped inside the generalisation of it. - kwargs[p.name] = {"X-Probe": SENTINEL} if _is_mapping_param(p) else ["X-Probe"] - base = messagefoundry.Rest( - url="https://example.invalid/endpoint", headers={"X-Probe-Auth": SENTINEL} - ) - return fn(base, **kwargs) + # arguments left every container empty, and the surface walk skips empty containers -- so + # it examined nothing and passed vacuously. A guard that passes because it looked at + # nothing is the defect this file exists for, and it nearly shipped inside the + # generalisation of it. + kwargs[p.name] = ( + {"PWD": SENTINEL, "Encrypt": "yes"} + if "odbc" in p.name + else {"X-Probe-Auth": SENTINEL} + if _is_mapping_param(p) + else ["X-Probe"] + ) + base = messagefoundry.Rest(url="https://example.invalid/endpoint") + + def build(kw: dict[str, Any]) -> Any: + return fn(base, **kw) if decorator else fn(**kw) + try: + return build(kwargs) + except Exception as exc: # noqa: BLE001 - the refusal message IS the signal here + if "env(" not in str(exc): + raise + # THE CONNECTOR REFUSES AN INLINE CREDENTIAL AND DEMANDS env(). That is the STRONGER control: + # the value never resolves into settings at all, so no serializer can leak it. `Http` and + # `Soap` do this for their intake credentials. `odbc_params` is the surface that does NOT + # (BACKLOG #1206), which is precisely why it leaked and they did not. + for k, v in list(kwargs.items()): + if v is SENTINEL: + kwargs[k] = messagefoundry.env(f"probe_{k}") + elif isinstance(v, dict) and any(x is SENTINEL for x in v.values()): + # SOAP body_secrets is {placeholder_token: env(secret_key)} BY CONTRACT -- the + # factory forbids an inline literal there, which is the env()-only treatment + # odbc_params lacks. Mirror the contract rather than skipping the connector. + kwargs[k] = { + ik: (messagefoundry.env(f"probe_{ik}") if iv is SENTINEL else iv) + for ik, iv in v.items() + } + try: + return build(kwargs) + except Exception: # noqa: BLE001 + # Still refused -- these connectors COUPLE their parameters (setting `intake_api_key` + # without `intake_auth` is itself rejected), and encoding each connector's coupling rules + # here would put connector-specific knowledge in a generic domain test. + # + # Drop the credential parameters and build the connector plainly. It stays IN the domain + # for the container walk, and `test_a_refusing_connector_actually_refuses` asserts the + # refusal separately. A connector excluded from the domain is a hole; a connector whose + # credential path is covered by a refusal instead of by redaction is a documented fact. + REFUSES_INLINE_CREDENTIALS.add(getattr(fn, "__name__", "?")) + # REQUIRED ARGUMENTS ONLY. Dropping just the credentials was not enough: these + # connectors also refuse an OPTIONAL control that is set while its gate is off + # (`intake_client_subjects` with `intake_auth='none'`), on the stated grounds that a + # control configured but never consulted reads as protection that does not exist. + # That refusal is correct and this test has no business working around it. + return build({k: v for k, v in kwargs.items() if k in required}) + + +def _is_envref(v: Any) -> bool: + return type(v).__name__ == "EnvRef" + + +#: Connectors that REFUSE an inline credential outright. Populated by `_call_with_sentinels` as a +#: side effect of discovering it, and asserted non-empty below -- if it empties, either every +#: connector stopped refusing (a real regression) or the detection broke. +REFUSES_INLINE_CREDENTIALS: set[str] = set() -FACTORIES = [ - (mod, name) - for mod, name in _factories_returning_a_spec() - if _decorator_style(mod, name) is not None -] + +#: THE DOMAIN: every spec-returning public function, decorators AND base constructors. 23 of them. +FACTORIES = [(mod, name) for mod, name in _factories_returning_a_spec() if _resolve(mod, name)] def test_the_domain_is_not_empty_and_is_wider_than_the_public_api() -> None: @@ -190,7 +282,7 @@ def test_no_factory_emits_a_credential_that_survives_redaction(mod: str, name: s reachable through `GET /connections/{name}/metadata` behind `MONITORING_READ` alone, and printed by `graph --json` to stdout, a CI log and the IDE. """ - fn = _decorator_style(mod, name) + fn = _resolve(mod, name) assert fn is not None spec = _call_with_sentinels(fn) redacted = redacted_settings(dict(spec.settings)) @@ -264,6 +356,15 @@ def test_a_public_header_is_not_redacted(header: str) -> None: #: `_is_secret_setting` and descends only into `headers` via `_is_secret_header`. HANDLED_CONTAINERS: dict[str, str] = { "headers": "per-key via _is_secret_header", + "odbc_params": "per-key via _is_secret_odbc_key (BACKLOG #1206)", + "calling_ae_allowlist": ( + "DICOM AE TITLES that may connect -- an allowlist of peer identifiers. Naming who may " + "call is not a credential, and masking it would hide the access-control decision from " + "the operator who has to audit it" + ), + "presentation_contexts": ( + "DICOM SOP class / transfer syntax negotiation -- protocol capability, public by nature" + ), "dynamic_headers": ( "values are HL7 field REFERENCES (e.g. 'MSH-4'), not credentials -- the secret arrives from " "the message at runtime and never enters settings" @@ -281,17 +382,19 @@ def test_no_unclassified_container_can_reach_a_serializer() -> None: does not survive staff turnover. This inverts it: instead of enumerating the leaks, enumerate the SURFACES, and fail on any surface nobody has classified. - Measured 2026-08-09: no shipped factory emits a nested container beyond those declared below, so - this currently guards a hole that is THEORETICAL rather than live -- stated plainly because a clean - result reported as a finding is the failure mode this whole line of work is about. Its value is - prospective: `redacted_settings` masks flat scalars and descends only into `headers`, so the first - connector to put a credential inside a new dict or list would leak it silently, exactly as - `sign_private_key` and `X-Auth-Token` did. This makes that a red test instead of an incident. + THE MEASUREMENT CLAIM THAT USED TO SIT HERE IS DELETED, NOT SOFTENED. It read "Measured + 2026-08-09: no shipped factory emits a nested container beyond those declared below, so this + currently guards a hole that is THEORETICAL rather than live". Both halves were FALSE. The walk + ran over 4 of 23 spec-returning functions -- every base constructor was dropped by a style filter + -- and `Database.odbc_params` was carrying a live credential disclosure the whole time (BACKLOG + #1206). A number a test has not established has no business in the file that defines the test, and + a softened version ("largely", "in the main") would be worse: it keeps the authority and loses the + falsifiability. The domain size is now ASSERTED below instead of described here. """ unclassified: list[str] = [] examined = 0 for mod, name in FACTORIES: - fn = _decorator_style(mod, name) + fn = _resolve(mod, name) assert fn is not None spec = _call_with_sentinels(fn) for key, value in spec.settings.items(): @@ -379,3 +482,46 @@ def test_a_sentinel_under_a_known_secret_key_is_actually_redacted() -> None: spec = messagefoundry.Rest(url="https://example.invalid/x") spec.settings["basic_password"] = SENTINEL assert SENTINEL not in str(redacted_settings(dict(spec.settings)).get("basic_password")) + + +def test_the_domain_covers_every_spec_returning_function() -> None: + """THE ASSERTION THAT WOULD HAVE CAUGHT #1206, and the one this file most needed. + + Its predecessor filtered the domain through `_decorator_style` and kept 4 of 23. Every base + connector constructor -- `Database`, `Rest`, `MLLP` -- was dropped, and `Database.odbc_params` was + serving an ODBC password verbatim the entire time the guard was green. + + Every control in this file answers *is this instrument working*: make it fail on purpose, confirm + the injection landed, run a negative control, assert it examined something. NONE of them answers + *is it pointed at the whole thing*. The domain is a separate claim and needs its own evidence, so + here it is. + """ + discovered = {n for _, n in _factories_returning_a_spec()} + covered = {n for _, n in FACTORIES} + assert discovered - covered == set(), ( + f"these spec-returning functions are NOT in the tested domain: {sorted(discovered - covered)}. " + "A connector missing from the domain is invisible to every other assertion in this file." + ) + assert len(covered) >= 23, ( + f"domain shrank to {len(covered)}; it was 23 when #1206 was fixed. A shrinking domain is how " + "this defect class recurs -- it has done so four times." + ) + + +def test_a_refusing_connector_actually_refuses_an_inline_credential() -> None: + """The connectors that demand `env()` have a STRONGER control than redaction, and it is asserted. + + `Http` and `Soap` refuse an inline intake credential outright, so the value never resolves into + settings and no serializer can leak it. That is the treatment `odbc_params` lacks -- there, `env()` + is refused and the inline literal is the only expressible form, which is the whole of #1206. + + `_call_with_sentinels` discovers these by trying an inline credential and reading the refusal. If + this set empties, either every connector stopped refusing (a real regression) or the discovery + broke -- and both must go red rather than pass quietly. + """ + for mod, name in FACTORIES: + _call_with_sentinels(_resolve(mod, name)) + assert REFUSES_INLINE_CREDENTIALS, ( + "no connector refused an inline credential. Either the env()-only refusals were removed, or " + "_call_with_sentinels stopped detecting them; a green here proves neither." + )