From b7bfd5297b650050916e1febdd4a5118763e864b Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 27 Oct 2026 18:04:38 -0500 Subject: [PATCH 1/3] One ODBC quoting and TLS tail helper for the store and the DATABASE connector messagefoundry/odbc_dsn.py now holds the brace quoting and the Encrypt/TrustServerCertificate tail that store/sqlserver.py connection_string and transports/database.py _build_dsn each carried. A leaf module, since transports/ may not import store/. It refuses a ServerCertificate pin beside a weakened posture, a shape no current caller sends. Both builders emit the same bytes as before: 1548 shapes compared, and tests/test_odbc_dsn.py pins golden strings taken from the old code. A scan fails if an engine module writes the escape or a TLS keyword part again. It also carries the store file's pin and floor comment copies (check_pinned_certificate, connection_string, purge_search_presets), which now link to the ssl_root_cert row of docs/CONFIGURATION.md and to SYSTEM-REQUIREMENTS.md. BACKLOG #3714 item 12 and slice C leftovers. --- changelog.d/3714-odbc.changed.md | 6 + messagefoundry/odbc_dsn.py | 61 +++++ messagefoundry/store/sqlserver.py | 58 ++--- messagefoundry/transports/database.py | 35 ++- scripts/security/crypto_inventory_check.py | 14 +- scripts/security/crypto_operations.py | 7 +- tests/test_odbc_dsn.py | 259 +++++++++++++++++++++ 7 files changed, 375 insertions(+), 65 deletions(-) create mode 100644 changelog.d/3714-odbc.changed.md create mode 100644 messagefoundry/odbc_dsn.py create mode 100644 tests/test_odbc_dsn.py diff --git a/changelog.d/3714-odbc.changed.md b/changelog.d/3714-odbc.changed.md new file mode 100644 index 0000000000..0fa8a84cb5 --- /dev/null +++ b/changelog.d/3714-odbc.changed.md @@ -0,0 +1,6 @@ +- **The SQL Server store and the DATABASE connector build their TLS keywords in one place.** Each + used to carry its own brace quoting and its own `Encrypt` and `TrustServerCertificate` tail. Both + now call `messagefoundry/odbc_dsn.py`, and both build the same strings as before. Only the store + sends a `ServerCertificate` pin. The shared code refuses a pin beside `encrypt = false` or + `trust_server_certificate = true`, a shape no current caller can send, so a pin the connector + gains later cannot go out on a hop that verifies nothing. (`BACKLOG #3714`) diff --git a/messagefoundry/odbc_dsn.py b/messagefoundry/odbc_dsn.py new file mode 100644 index 0000000000..b3e9759dc9 --- /dev/null +++ b/messagefoundry/odbc_dsn.py @@ -0,0 +1,61 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Foundation, LLC and contributors +"""Brace quoting and the TLS tail of a SQL Server ODBC Driver 18 connection string (BACKLOG #3714). + +The SQL Server store (:func:`messagefoundry.store.sqlserver.connection_string`) and the DATABASE +connector (:func:`messagefoundry.transports.database._build_dsn`) both build their strings here. +``transports/`` may not import ``store/`` (``tests/test_dependency_boundaries.py``), so this is a +leaf module that imports nothing from the engine. + +Two rules hold for every string built here: + +* :func:`odbc_brace` quotes a free-text value, so a value holding ``;``, ``{``, ``}`` or ``=`` (a + password, say) cannot add keywords (STORE-5). +* :func:`odbc_connection_string` puts the TLS keywords **last**. ODBC takes the last value of a + repeated keyword, so nothing earlier in the string can weaken TLS. + +What the pin does under ``Encrypt=strict``, and how far that was measured, is stated once, in the +``ssl_root_cert`` row of ``docs/CONFIGURATION.md``. +""" + +from __future__ import annotations + +from collections.abc import Sequence + +__all__ = ["odbc_brace", "odbc_connection_string"] + + +def odbc_brace(value: str) -> str: + """ODBC-quote ``value`` in braces, doubling any ``}`` inside it. + + Inside braces the driver reads ``;``, ``{`` and ``=`` as text, and ``}}`` as one ``}``, so an + attacker-influenced value cannot end the value early and add a keyword.""" + return "{" + value.replace("}", "}}") + "}" + + +def odbc_connection_string( + head: Sequence[str], + *, + encrypt: bool, + trust_server_certificate: bool, + pin: str | None = None, +) -> str: + """Join ``head`` and the TLS tail into one ``;``-terminated ODBC connection string. + + ``head`` is every ``KEY=value`` part but the TLS ones, already quoted. The tail follows it: + ``ServerCertificate`` when ``pin`` is set, then ``Encrypt`` and ``TrustServerCertificate``. + ``Encrypt`` is ``strict`` when a pin is sent, and otherwise ``yes`` or ``no`` from ``encrypt``. + + A pin beside ``encrypt=False`` or ``trust_server_certificate=True`` raises ``ValueError``: the + driver verifies nothing there, so the pin would look like a control and be none. No current + caller passes that shape. The pin file's own checks are the caller's job, as + :func:`messagefoundry.store.sqlserver.check_pinned_certificate` is the store's.""" + if pin and (not encrypt or trust_server_certificate): + raise ValueError( + "an ODBC ServerCertificate pin needs encrypt on and trust_server_certificate off; " + "the driver verifies nothing on a weakened posture, so the engine sends no pin there" + ) + tail = [f"ServerCertificate={odbc_brace(pin)}"] if pin else [] + tail.append(f"Encrypt={'strict' if pin else ('yes' if encrypt else 'no')}") + tail.append(f"TrustServerCertificate={'yes' if trust_server_certificate else 'no'}") + return ";".join([*head, *tail]) + ";" diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index e072d9732a..eb225d6c29 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -74,6 +74,7 @@ weakened_tls_escape_permitted, ) from messagefoundry.config.tls_policy import HopPosture, current_hop_posture +from messagefoundry.odbc_dsn import odbc_brace, odbc_connection_string from messagefoundry.odbc_env import disable_driver_manager_pooling from messagefoundry.parsing.binary import strip_documents as _strip_documents from messagefoundry.redaction import codec_safe_str, safe_text @@ -2711,20 +2712,15 @@ def _schema_hash() -> str: return hashlib.sha256("\n".join(_SCHEMA).encode()).hexdigest() -def _odbc_brace(value: str) -> str: - """ODBC-quote a value in braces, doubling any internal ``}`` — neutralizes ``; { } =`` inside it - so an attacker-influenced value (e.g. a password) can't inject extra connection keywords.""" - return "{" + value.replace("}", "}}") + "}" - - def check_pinned_certificate( settings: StoreSettings, *, posture: HopPosture | None = None, now: float | None = None ) -> None: """Refuse a ``[store].ssl_root_cert`` pin the engine must not send. Raises ``ValueError`` naming the setting. A no-op when the store sends no pin. - Vault BACKLOG #2223: under ``Encrypt=strict`` a pin match stands in for the driver's own chain, - host-name and expiry checks, and the pinned file is the connection's only trust anchor. So: + Vault BACKLOG #2223: the pinned file is the connection's only trust anchor, because of what a + pin match does under ``Encrypt=strict``. The ``ssl_root_cert`` row of ``docs/CONFIGURATION.md`` + states that once, with the build and platform it was tested on. So: * it takes the trust-anchor ACL and path checks with their own rules and messages (:func:`~messagefoundry.auth.trust_anchors.enforced_anchor`): a file or folder a broad @@ -2802,10 +2798,8 @@ def check_pinned_certificate( def connection_string(settings: StoreSettings, *, posture: HopPosture | None = None) -> str: """Build an ODBC connection string for the Microsoft ODBC Driver 18 from store settings. - Free-text values are brace-quoted to prevent connection-string injection (STORE-5), and the - ``Encrypt``/``TrustServerCertificate`` security flags are emitted **last** so — ODBC being - last-wins on duplicate keywords — nothing earlier can downgrade TLS. Identity fields are also - validated up front (see ``StoreSettings._no_odbc_injection``). + Quoting and the TLS tail: :mod:`messagefoundry.odbc_dsn` (STORE-5, BACKLOG #3714). Identity + fields are also validated up front (see ``StoreSettings._no_odbc_injection``). ``posture`` is the deriving instance's :class:`HopPosture` (threaded from ``open_store`` by the serve/engine caller). ``None`` reads the ambient posture, and with none the escape is NOT @@ -2834,42 +2828,37 @@ def connection_string(settings: StoreSettings, *, posture: HopPosture | None = N parts = [ "DRIVER={ODBC Driver 18 for SQL Server}", f"SERVER={settings.server},{settings.port}", # server validated; port is an int - f"DATABASE={_odbc_brace(settings.database or '')}", + f"DATABASE={odbc_brace(settings.database or '')}", # No login-timeout keyword here, on purpose (BACKLOG #1626). `Connection Timeout=` is an # ADO.NET keyword that ODBC Driver 18 silently ignores: measured against a black-hole address, # a DSN carrying `Connection Timeout=2` still waited 15.1 s, the same as no timeout at all. # The driver's login timeout is SQL_ATTR_LOGIN_TIMEOUT, which pyodbc sets from its `timeout=` # argument, so every connect site passes `timeout=settings.connect_timeout` instead. - f"APP={_odbc_brace(settings.application_name)}", + f"APP={odbc_brace(settings.application_name)}", ] if settings.auth is SqlAuth.SQL: - parts.append(f"UID={_odbc_brace(settings.username or '')}") - parts.append(f"PWD={_odbc_brace(settings.password or '')}") + parts.append(f"UID={odbc_brace(settings.username or '')}") + parts.append(f"PWD={odbc_brace(settings.password or '')}") elif settings.auth is SqlAuth.INTEGRATED: parts.append("Trusted_Connection=yes") elif settings.auth is SqlAuth.ENTRA: parts.append("Authentication=ActiveDirectoryDefault") # AOAG multi-subnet fast failover (BACKLOG #100). Not a TLS keyword, so it goes before the - # Encrypt/TrustServerCertificate tail and cannot disturb the last-wins TLS posture. + # TLS tail and cannot disturb the last-wins TLS posture. if settings.multi_subnet_failover: parts.append("MultiSubnetFailover=Yes") # Pin the DB server's certificate by file WITHOUT a machine-wide trust import (#45), via the ODBC - # Driver 18.1+ `ServerCertificate` keyword — only on the SECURE posture (encrypt on, verify on; the - # weakened branch above already refused/escaped). Brace-quoted (STORE-5). The pin goes out with - # Encrypt=strict: on the Driver 18.7 build tested on Windows, the pin was enforced with strict and - # NOT with Encrypt=yes, where a pin of the wrong certificate still connected (vault BACKLOG #2223, - # measured under #3009). Strict is TDS 8.0, so a server older than SQL Server 2022 refuses the - # connection; there is no fall back to an unenforced pin. Microsoft documents the exact match as - # done instead of the expiry, host-name and chain checks, so the engine checks the pinned - # certificate itself at store open (check_pinned_certificate). Not a revocation control. Both - # keywords stay inside the last-wins tail. - pin = settings.sqlserver_pin_path() - if pin: - parts.append(f"ServerCertificate={_odbc_brace(pin)}") - encrypt = "strict" if pin else ("yes" if settings.encrypt else "no") - parts.append(f"Encrypt={encrypt}") - parts.append(f"TrustServerCertificate={'yes' if settings.trust_server_certificate else 'no'}") - return ";".join(parts) + ";" + # Driver 18.1+ `ServerCertificate` keyword, only on the SECURE posture (encrypt on, verify on; the + # weakened branch above already refused or escaped). The pin goes out with Encrypt=strict, and the + # engine checks the pinned certificate itself at store open (check_pinned_certificate). Why strict, + # what it needs of the server, and what it was tested on: the `ssl_root_cert` row of + # docs/CONFIGURATION.md (vault BACKLOG #2223). Not a revocation control. + return odbc_connection_string( + parts, + encrypt=settings.encrypt, + trust_server_certificate=settings.trust_server_certificate, + pin=settings.sqlserver_pin_path(), + ) def _build_pool_executor(settings: StoreSettings, maxsize: int | None = None) -> ThreadPoolExecutor: @@ -8953,7 +8942,8 @@ async def purge_search_presets(self, *, older_than: float, now: float | None = N # ASVS 14.2.7. Whole-row DELETE — the criteria IS the payload and the row backs no count. # `updated_at`/`last_used_at` are FLOAT epochs here (as on SQLite/PG), so the comparison needs no # date handling. #306: key on the LATER of last-edited and last-used. T-SQL's GREATEST is SQL - # Server 2022+ and this backend's floor is 2016 SP1 (see _SCHEMA's CREATE OR ALTER guard), so the + # Server 2022+, newer than the 2016 SP1 that _SCHEMA's CREATE OR ALTER guard checks for, so this + # statement does not use it (the supported floor is docs/SYSTEM-REQUIREMENTS.md's). The # null-safe greatest-of-two is spelled as the equivalent PREDICATE pair — `MAX(a,b) < cutoff` is # exactly `a < cutoff AND (b IS NULL OR b < cutoff)`, and the NULL arm is what keeps a pre-#306 # row (never recall-stamped) purging on `updated_at` alone. `older_than` is bound TWICE. diff --git a/messagefoundry/transports/database.py b/messagefoundry/transports/database.py index a450385bf2..51330d003e 100644 --- a/messagefoundry/transports/database.py +++ b/messagefoundry/transports/database.py @@ -87,6 +87,7 @@ from messagefoundry.connection_names import inbound_record_name from messagefoundry.controlchars import has_control_char from messagefoundry.keywrap import refuse_weak_key_file +from messagefoundry.odbc_dsn import odbc_brace, odbc_connection_string from messagefoundry.odbc_env import disable_driver_manager_pooling from messagefoundry.redaction import safe_exc from messagefoundry.transports.base import ( @@ -122,13 +123,6 @@ _TRANSIENT_STATES = frozenset({"HYT00", "HYT01"}) -def _odbc_brace(value: str) -> str: - """ODBC-quote a value in braces, doubling any internal ``}`` — neutralizes ``; { } =`` inside it so - an attacker-influenced value (e.g. a password) can't inject extra connection keywords (mirrors the - store's ``connection_string`` hardening).""" - return "{" + value.replace("}", "}}") + "}" - - def _weakened_tls_permitted(*, attested: bool, posture: HopPosture | None) -> bool: """Whether a weakened (verify-off) customer-DB TLS posture may be used, routed through the ONE shared hop authority (#200, ADR 0092). @@ -192,12 +186,11 @@ def _build_dsn( ) -> str: """Build the ODBC connection string for SQL Server from the connection settings. - Free-text values are brace-quoted (injection guard) and the ``Encrypt``/``TrustServerCertificate`` - flags are emitted **last** (ODBC is last-wins, so nothing earlier can downgrade TLS). A weakened - TLS posture is **refused** via the shared posture-keyed authority (:func:`_weakened_tls_permitted`, - #200) — a STRICT verify-off cell that stays refused for staging AND prod PHI, with the global escape - clamped so it can never relax a production hop; ``attested`` (the per-connection ``tls_hop_attested``) - is the surgical, audited per-hop opt-in. + Quoting and the TLS tail: :mod:`messagefoundry.odbc_dsn` (BACKLOG #3714). It sends no + ``ServerCertificate`` pin. A weakened TLS posture is **refused** via the shared posture-keyed + authority (:func:`_weakened_tls_permitted`, #200) — a STRICT verify-off cell that stays refused + for staging AND prod PHI, with the global escape clamped so it can never relax a production hop; + ``attested`` (the per-connection ``tls_hop_attested``) is the surgical, audited per-hop opt-in. ``read_only`` (only the db_lookup pool sets it; destination/source omit it, keeping their DSN byte-identical) appends ``ApplicationIntent=ReadOnly`` so the connection advertises read-only intent @@ -237,15 +230,15 @@ def _build_dsn( "DATABASE server must not contain ';', '{', '}', '=', or newlines (ODBC injection risk)" ) parts = [ - f"DRIVER={_odbc_brace(str(s.get('odbc_driver', 'ODBC Driver 18 for SQL Server')))}", + f"DRIVER={odbc_brace(str(s.get('odbc_driver', 'ODBC Driver 18 for SQL Server')))}", f"SERVER={server},{int(s.get('port', 1433))}", - f"DATABASE={_odbc_brace(str(s['database']))}", + f"DATABASE={odbc_brace(str(s['database']))}", # No login-timeout keyword here, on purpose: see :func:`_login_timeout` (BACKLOG #2089). - f"APP={_odbc_brace(str(s.get('app_name', 'messagefoundry')))}", + f"APP={odbc_brace(str(s.get('app_name', 'messagefoundry')))}", ] if auth == "sql": - parts.append(f"UID={_odbc_brace(str(s.get('username') or ''))}") - parts.append(f"PWD={_odbc_brace(str(s.get('password') or ''))}") + parts.append(f"UID={odbc_brace(str(s.get('username') or ''))}") + parts.append(f"PWD={odbc_brace(str(s.get('password') or ''))}") elif auth == "integrated": parts.append("Trusted_Connection=yes") else: # entra @@ -254,9 +247,7 @@ def _build_dsn( # Advertise read-only intent for the db_lookup pool (ADR 0010). Emitted before the TLS flags so # the last-wins Encrypt/TrustServerCertificate posture is unchanged. parts.append("ApplicationIntent=ReadOnly") - parts.append(f"Encrypt={'yes' if encrypt else 'no'}") - parts.append(f"TrustServerCertificate={'yes' if trust else 'no'}") - return ";".join(parts) + ";" + return odbc_connection_string(parts, encrypt=encrypt, trust_server_certificate=trust) # A valid ODBC connection-string keyword: a letter then letters/digits/spaces/underscores. Rejecting the @@ -504,7 +495,7 @@ def _odbc_literal(value: object, *, what: str, connection: str | None, refused: f"{hop_name_prefix(connection)}DATABASE {what} must hold only printable ASCII characters" + (f", and none of {listed}" if refused else "") ) - return _odbc_brace(text) + return odbc_brace(text) def _refuse_remote_path(value: object, *, name: str, connection: str | None) -> None: diff --git a/scripts/security/crypto_inventory_check.py b/scripts/security/crypto_inventory_check.py index 091c0e1298..32216783cd 100644 --- a/scripts/security/crypto_inventory_check.py +++ b/scripts/security/crypto_inventory_check.py @@ -1547,9 +1547,10 @@ "compare:via messagefoundry.store.store", "hash:hashlib.sha256", "hash:via messagefoundry.store.store", - # Vault BACKLOG #2223: connection_string parses the [store].ssl_root_cert pin (public - # certificate only) through pki.load_pinned_server_certificate to refuse one outside its - # validity period, since Encrypt=strict leaves that check to the engine. + # Vault BACKLOG #2223: check_pinned_certificate parses the [store].ssl_root_cert pin + # (public certificate only) through pki.pinned_server_certificate to refuse one outside + # its validity period. Why the engine makes that check: the ssl_root_cert row of + # docs/CONFIGURATION.md. "key_cert:via messagefoundry.pki", # Vault BACKLOG #2223: the same pin takes the trust-anchor preflight (enforced_anchor), which # fingerprints the public file (SHA-256). No pin is configured for it, so no compare runs. @@ -1579,9 +1580,10 @@ ), # BACKLOG #1352 / #1171: the generic dialect checks a driver client key's wrap (libpq sslkey) # before the DSN reaches the driver. Before this it was IMPORT_ONLY; that entry's point still - # stands and is kept here: it appends Encrypt= and TrustServerCertificate= to a DSN string and - # refuses an insecure hop, a first-party TLS posture decision with no crypto-shaped expression - # for any pattern instrument to match (an INSTRUMENT LIMIT, named in BACKLOG #1164). + # stands and is kept here: it refuses an insecure hop and has messagefoundry/odbc_dsn.py append + # Encrypt= and TrustServerCertificate= to a DSN string (BACKLOG #3714), a first-party TLS posture + # decision with no crypto-shaped expression for any pattern instrument to match (an INSTRUMENT + # LIMIT, named in BACKLOG #1164). "messagefoundry/transports/database.py": frozenset({"key_cert:via messagefoundry.keywrap"}), "messagefoundry/transports/dicom.py": frozenset( { diff --git a/scripts/security/crypto_operations.py b/scripts/security/crypto_operations.py index d269474b14..4eb9d340b7 100644 --- a/scripts/security/crypto_operations.py +++ b/scripts/security/crypto_operations.py @@ -44,9 +44,10 @@ ``asyncio.to_thread``. The reload preflight reaches ``anchor_cadata`` that way (BACKLOG #2025). ``Engine.fingerprint_bundle_blocking`` calls its fold directly instead, so that one is seen (vault BACKLOG #2839, #2782); -* a crypto DECISION that is not a call or a TLS-attribute assignment. ``transports/database.py`` - appends ``Encrypt=`` and ``TrustServerCertificate=`` to a DSN string: a first-party TLS posture - decision with no crypto-shaped expression for any pattern instrument to match; +* a crypto DECISION that is not a call or a TLS-attribute assignment. ``odbc_dsn.py`` appends + ``Encrypt=`` and ``TrustServerCertificate=`` to a DSN string for ``transports/database.py`` and + ``store/sqlserver.py``: a first-party TLS posture decision with no crypto-shaped expression for + any pattern instrument to match; * crypto inside a third-party library the engine calls. The call is seen; the library's own operations are not; * for the non-Python arms, which are PATTERN instruments: an alias that renames the API, an idiom diff --git a/tests/test_odbc_dsn.py b/tests/test_odbc_dsn.py new file mode 100644 index 0000000000..d2cbae3415 --- /dev/null +++ b/tests/test_odbc_dsn.py @@ -0,0 +1,259 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Foundation, LLC and contributors +"""The shared ODBC quoting and TLS tail (``messagefoundry.odbc_dsn``, BACKLOG #3714). + +The SQL Server store and the DATABASE connector each used to build their own brace quoting and TLS +tail. Both now call one helper. These tests pin three things: + +* the helper's own output, shape by shape; +* both builders' strings, byte for byte, as the code before the move built them. The expected + strings were taken from that code; +* that no engine module writes the ``}}`` escape or the TLS keywords itself again. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +from messagefoundry import odbc_dsn +from messagefoundry.config.settings import ( + INSECURE_TLS_ESCAPE_ENV, + SqlAuth, + StoreBackend, + StoreSettings, +) +from messagefoundry.config.tls_policy import HopPosture +from messagefoundry.odbc_dsn import odbc_brace, odbc_connection_string +from messagefoundry.store.sqlserver import connection_string +from messagefoundry.transports.database import _build_dsn + +_WARN = HopPosture(enforcing=False) +_ENGINE = Path(__file__).resolve().parents[1] / "messagefoundry" + + +# --- the helper --------------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("value", "quoted"), + [ + pytest.param("plain", "{plain}", id="plain"), + pytest.param("", "{}", id="empty"), + pytest.param("p};DROP", "{p}};DROP}", id="close-brace-then-keyword"), + pytest.param("a;b=c{d", "{a;b=c{d}", id="semicolon-equals-open-brace-kept"), + pytest.param("}}x}", "{}}}}x}}}", id="every-close-brace-doubled"), + ], +) +def test_odbc_brace(value: str, quoted: str) -> None: + assert odbc_brace(value) == quoted + + +@pytest.mark.parametrize( + ("encrypt", "trust", "tail"), + [ + (True, False, "Encrypt=yes;TrustServerCertificate=no;"), + (True, True, "Encrypt=yes;TrustServerCertificate=yes;"), + (False, False, "Encrypt=no;TrustServerCertificate=no;"), + (False, True, "Encrypt=no;TrustServerCertificate=yes;"), + ], +) +def test_the_tail_without_a_pin(encrypt: bool, trust: bool, tail: str) -> None: + built = odbc_connection_string( + ["A=1", "B={2}"], encrypt=encrypt, trust_server_certificate=trust + ) + assert built == "A=1;B={2};" + tail + + +def test_a_pin_goes_out_quoted_with_strict_at_the_end() -> None: + built = odbc_connection_string( + ["A=1"], encrypt=True, trust_server_certificate=False, pin="C:\\p}x;y=z{.pem" + ) + assert built == ( + "A=1;ServerCertificate={C:\\p}}x;y=z{.pem};Encrypt=strict;TrustServerCertificate=no;" + ) + + +def test_an_empty_pin_sends_no_server_certificate() -> None: + built = odbc_connection_string(["A=1"], encrypt=True, trust_server_certificate=False, pin="") + assert built == "A=1;Encrypt=yes;TrustServerCertificate=no;" + + +@pytest.mark.parametrize( + ("encrypt", "trust"), + [ + pytest.param(False, False, id="plaintext"), + pytest.param(True, True, id="verify-off"), + pytest.param(False, True, id="both"), + ], +) +def test_a_pin_beside_a_weakened_posture_is_refused(encrypt: bool, trust: bool) -> None: + with pytest.raises(ValueError, match="ServerCertificate pin needs encrypt on"): + odbc_connection_string( + ["A=1"], encrypt=encrypt, trust_server_certificate=trust, pin="C:\\p.pem" + ) + + +# --- the store's string, byte for byte as before ------------------------------------------------- + + +def _store(**update: object) -> StoreSettings: + """SQL Server store settings. ``model_copy`` skips validation, so a pin path need not exist: + ``connection_string`` reads no file.""" + base = StoreSettings( + backend=StoreBackend.SQLSERVER, server="db.example", database="mefor", username="mefor" + ) + return base.model_copy(update={"application_name": "app}x", **update}) + + +@pytest.mark.parametrize( + ("update", "expected"), + [ + pytest.param( + {"password": "a;b=c{d}", "database": "d}}b;x"}, + "DRIVER={ODBC Driver 18 for SQL Server};SERVER=db.example,1433;DATABASE={d}}}}b;x};" + "APP={app}}x};UID={mefor};PWD={a;b=c{d}}};Encrypt=yes;TrustServerCertificate=no;", + id="secure-no-pin-braces", + ), + pytest.param( + {"password": "p};DROP", "ssl_root_cert": "C:\\pins\\ca}x;y=z{.pem"}, + "DRIVER={ODBC Driver 18 for SQL Server};SERVER=db.example,1433;DATABASE={mefor};" + "APP={app}}x};UID={mefor};PWD={p}};DROP};ServerCertificate={C:\\pins\\ca}}x;y=z{.pem};" + "Encrypt=strict;TrustServerCertificate=no;", + id="pin-strict", + ), + pytest.param( + { + "auth": SqlAuth.INTEGRATED, + "multi_subnet_failover": True, + "trust_server_certificate": True, + "ssl_root_cert": "C:\\pins\\db.pem", + }, + "DRIVER={ODBC Driver 18 for SQL Server};SERVER=db.example,1433;DATABASE={mefor};" + "APP={app}}x};Trusted_Connection=yes;MultiSubnetFailover=Yes;Encrypt=yes;" + "TrustServerCertificate=yes;", + id="escaped-verify-off-drops-pin", + ), + pytest.param( + {"auth": SqlAuth.ENTRA, "encrypt": False, "ssl_root_cert": "C:\\pins\\db.pem"}, + "DRIVER={ODBC Driver 18 for SQL Server};SERVER=db.example,1433;DATABASE={mefor};" + "APP={app}}x};Authentication=ActiveDirectoryDefault;Encrypt=no;" + "TrustServerCertificate=no;", + id="escaped-plaintext-drops-pin", + ), + ], +) +def test_the_store_string_is_unchanged( + monkeypatch: pytest.MonkeyPatch, update: dict[str, object], expected: str +) -> None: + monkeypatch.setenv(INSECURE_TLS_ESCAPE_ENV, "1") + assert connection_string(_store(**update), posture=_WARN) == expected + + +# --- the DATABASE connector's string, byte for byte as before ------------------------------------ + +_DB: dict[str, object] = { + "server": "db.example", + "port": 14330, + "database": "d}b;x", + "username": "u}", + "app_name": "a;p}p", +} + + +@pytest.mark.parametrize( + ("update", "read_only", "expected"), + [ + pytest.param( + {"password": "p};DROP", "auth": "sql", "encrypt": True}, + False, + "DRIVER={ODBC Driver 18 for SQL Server};SERVER=db.example,14330;DATABASE={d}}b;x};" + "APP={a;p}}p};UID={u}}};PWD={p}};DROP};Encrypt=yes;TrustServerCertificate=no;", + id="secure-sql-braces", + ), + pytest.param( + {"auth": "integrated", "encrypt": True, "odbc_driver": "x}y"}, + True, + "DRIVER={x}}y};SERVER=db.example,14330;DATABASE={d}}b;x};APP={a;p}}p};" + "Trusted_Connection=yes;ApplicationIntent=ReadOnly;Encrypt=yes;" + "TrustServerCertificate=no;", + id="read-only-before-the-tail", + ), + pytest.param( + {"auth": "entra", "encrypt": False}, + False, + "DRIVER={ODBC Driver 18 for SQL Server};SERVER=db.example,14330;DATABASE={d}}b;x};" + "APP={a;p}}p};Authentication=ActiveDirectoryDefault;Encrypt=no;" + "TrustServerCertificate=no;", + id="attested-plaintext", + ), + pytest.param( + { + "auth": "SQL", + "encrypt": "yes", + "trust_server_certificate": True, + "password": "}}x}", + "odbc_driver": "ODBC Driver 17 for SQL Server", + }, + False, + "DRIVER={ODBC Driver 17 for SQL Server};SERVER=db.example,14330;DATABASE={d}}b;x};" + "APP={a;p}}p};UID={u}}};PWD={}}}}x}}};Encrypt=yes;TrustServerCertificate=yes;", + id="attested-verify-off", + ), + ], +) +def test_the_connector_string_is_unchanged( + update: dict[str, object], read_only: bool, expected: str +) -> None: + settings = {**_DB, **update} + assert _build_dsn(settings, read_only=read_only, attested=True) == expected + + +# --- no second copy --------------------------------------------------------------------------------- + + +#: A string that STARTS with one of these is a connection-string part. Prose that names a keyword +#: mid-sentence, such as a refusal message, is not. +_TLS_KEYWORDS = ("Encrypt=", "ServerCertificate=", "TrustServerCertificate=") + + +def _copies(path: Path) -> list[str]: + """Where ``path`` writes the ``}}`` escape or a TLS keyword part itself. + + Only a file whose text holds ``}}`` or a keyword name is parsed, which keeps the scan of the + whole package fast. A copy spelled with escapes or split literals would pass unseen.""" + text = path.read_text(encoding="utf-8") + if "}}" not in text and "Encrypt" not in text and "ServerCertificate" not in text: + return [] + found: list[str] = [] + tree = ast.parse(text) + for node in ast.walk(tree): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "replace" + and [a.value for a in node.args if isinstance(a, ast.Constant)] == ["}", "}}"] + ): + found.append(f"{path.name}:{node.lineno} the }}}} escape") + if ( + isinstance(node, ast.Constant) + and isinstance(node.value, str) + and node.value.startswith(_TLS_KEYWORDS) + ): + found.append(f"{path.name}:{node.lineno} {node.value!r}") + return found + + +def test_the_helper_is_the_only_engine_module_that_writes_them() -> None: + helper = Path(odbc_dsn.__file__).resolve() + # POSITIVE CONTROL: the scan finds the helper's own escape and keywords, so a clean verdict + # elsewhere is not a scan that matches nothing. + assert len(_copies(helper)) >= 4 + paths = [p for p in sorted(_ENGINE.rglob("*.py")) if p.resolve() != helper] + # The walk reaches the package, at least both callers among the files it reads. + assert len(paths) >= 100 + assert {"sqlserver.py", "database.py"} <= {p.name for p in paths} + others = [hit for path in paths for hit in _copies(path)] + assert others == [] From 5b86c1807eb5527c80b046d075118dd7a9f4bb1f Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 27 Oct 2026 18:04:59 -0500 Subject: [PATCH 2/3] Pin fact copies in settings, serve and store tests link to CONFIGURATION.md The [store].ssl_root_cert comment in config/settings.py (which had no Windows qualifier), the serve cert-monitor comment in __main__.py and two texts in tests/test_store_ssl.py restated the strict-pin measurement. Each now points at the one statement, the ssl_root_cert row of docs/CONFIGURATION.md, which keeps "tested on Windows". BACKLOG #3714 slice D (item 12 and slice C's leftover copies). Proposed PR title: One ODBC TLS tail helper for the SQL Server store and the DATABASE connector, and the last pin-fact copies linked (BACKLOG #3714) Proposed ledger banner: PARTIAL -- slice D: item 12 built (one leaf helper, messagefoundry/odbc_dsn.py, for brace quoting and the TLS tail; both builders byte-identical, golden tests and a no-copy scan). Slice C's leftover copies of the pin fact and the 2016 SP1 floor wording now link to docs/CONFIGURATION.md and SYSTEM-REQUIREMENTS.md. Open as before: items 2 and 11 (owner ruling 2026-10-26; item 11 is #4325), 5, 6 and 8 (CI, PR 2579), and the slice A and C open findings. --- messagefoundry/__main__.py | 4 ++-- messagefoundry/config/settings.py | 6 +++--- tests/test_store_ssl.py | 11 +++++------ 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/messagefoundry/__main__.py b/messagefoundry/__main__.py index d32c420630..d17beae525 100644 --- a/messagefoundry/__main__.py +++ b/messagefoundry/__main__.py @@ -4762,8 +4762,8 @@ def registry_filter(reg: Registry) -> Registry: # noqa: F811 (local shard-bound api_tls_client_cert_files=settings.api.tls_client_cert_files, # BACKLOG #299: the settings-level CRL files ([tls], [logging] forward, [auth] OIDC, [store], # [api] client), so a CRL going stale on an OUTBOUND hop alerts before it fails a handshake. - # Vault BACKLOG #2223: plus the SQL Server store pin, whose expiry the driver does not check - # under Encrypt=strict. + # Vault BACKLOG #2223: plus the SQL Server store pin, whose expiry the engine watches itself + # (why: the `ssl_root_cert` row of docs/CONFIGURATION.md). settings_crls=[*crls_from_settings(settings), *certs_from_settings(settings)], # Reserve the engine's own API listener so no inbound can be wired onto it (it would collide # with uvicorn at bind); surfaced as a clear PortConflictError at check/start instead. diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index 22401092f2..abb4c596ef 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -957,9 +957,9 @@ class StoreSettings(_Section): # CA-bundle pin (chain + hostname still verified). # * SQL SERVER — the ODBC Driver 18.1+ `ServerCertificate` keyword pins the server's OWN certificate # by file (an exact-cert match, brace-quoted STORE-5-safe), so a CA file matches nothing. It goes - # out with `Encrypt=strict`: on the Driver 18.7 build tested, the pin was enforced with strict and - # not with `Encrypt=yes` (vault BACKLOG #2223). Strict is TDS 8.0, so it needs SQL Server 2022 or - # later, the supported floor; an older server refuses the connection. Not a revocation control. + # out with `Encrypt=strict` (vault BACKLOG #2223). Why strict, the server version it needs, and the + # build and platform it was tested on: the `ssl_root_cert` row of docs/CONFIGURATION.md. Not a + # revocation control. # REJECTED for SQLite (no TLS at all). A path, not a secret — it may live in the config file / # connections.toml. Empty = use the system trust store (the secure default). Existence is checked at load # (a missing file fails loud here, not confusingly at connect). diff --git a/tests/test_store_ssl.py b/tests/test_store_ssl.py index efa3dd833f..c50dd1025d 100644 --- a/tests/test_store_ssl.py +++ b/tests/test_store_ssl.py @@ -340,9 +340,8 @@ async def test_pool_connect_hook_is_absent_on_the_weakened_escapes( def test_connection_string_emits_server_certificate_on_secure_posture(tmp_path: object) -> None: - """The pin travels with ``Encrypt=strict``: Driver 18 enforced ``ServerCertificate`` with strict - and did not with ``Encrypt=yes``, where a pin naming the wrong certificate still connected (vault - BACKLOG #2223; measured under #3009).""" + """The pin travels with ``Encrypt=strict`` (vault BACKLOG #2223). Why, and the build and platform + that was measured on (#3009): the ``ssl_root_cert`` row of ``docs/CONFIGURATION.md``.""" pin = _leaf_file(tmp_path) dsn = connection_string(_ss(ssl_root_cert=pin)) # The ODBC Driver 18.1+ ServerCertificate keyword pins the cert by file, brace-quoted (STORE-5). @@ -415,9 +414,9 @@ def test_connection_string_server_certificate_brace_neutralizes_injection(tmp_pa # --- vault BACKLOG #2223: the engine checks the SQL Server pin itself -------------------------- # -# Under Encrypt=strict a pin match stands in for the driver's own chain, host-name and expiry checks, -# so the engine refuses a pin outside its validity period, and refuses at load a file that can never -# match the server's certificate. +# Under Encrypt=strict the engine checks the pin itself (why, and what it was tested on: the +# `ssl_root_cert` row of docs/CONFIGURATION.md). So it refuses a pin outside its validity period, +# and refuses at load a file that can never match the server's certificate. _DAY = 86_400.0 From 33a83a7b40117facf41723d2eff7061dd893b5c7 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 27 Oct 2026 18:25:55 -0500 Subject: [PATCH 3/3] ODBC helper review round 1: harness folded in, honest tail and refusal text From the code-review round on this branch: - harness/drivers/_database.py drops its own brace quoting and TLS tail and calls messagefoundry.odbc_dsn; its string is unchanged (216 inputs compared, plus a golden test). - The helper no longer claims ODBC keeps the last repeated keyword. Microsoft's SQLDriverConnect reference says the first, and the driver is not measured. The quoting is the guard, so say that. Same for three comments that leaned on "last-wins". - The pin-beside-weakened refusal gives the real reason: a pin sends Encrypt=strict, which would override the weakened posture asked for. - head is list[str], so a joined string cannot type-check. - The no-copy scan ignores case, sees a keyword after ';' and any "}}" constant, walks harness/ too, and has a test of its own shapes. BACKLOG #3714 slice D (item 12 and slice C's leftover copies). Proposed PR title: One ODBC TLS tail helper for the SQL Server store, the DATABASE connector and the harness, and the last pin-fact copies linked (BACKLOG #3714) Proposed ledger banner: PARTIAL -- slice D: item 12 built (one leaf helper, messagefoundry/odbc_dsn.py, for brace quoting and the TLS tail, used by the store, the DATABASE connector and the harness; each string byte-identical, golden tests and a no-copy scan). Slice C's leftover copies of the pin fact and the 2016 SP1 floor wording now link to docs/CONFIGURATION.md and SYSTEM-REQUIREMENTS.md. Open as before: items 2 and 11 (owner ruling 2026-10-26; item 11 is #4325), 5, 6 and 8 (CI, PR 2579), and the slice A and C open findings. --- changelog.d/3714-odbc.changed.md | 13 ++--- harness/drivers/_database.py | 33 +++++------- messagefoundry/odbc_dsn.py | 34 +++++++----- messagefoundry/store/sqlserver.py | 4 +- messagefoundry/transports/database.py | 4 +- tests/test_odbc_dsn.py | 77 ++++++++++++++++++--------- tests/test_store_file_hardening.py | 5 +- tests/test_store_ssl.py | 2 +- 8 files changed, 99 insertions(+), 73 deletions(-) diff --git a/changelog.d/3714-odbc.changed.md b/changelog.d/3714-odbc.changed.md index 0fa8a84cb5..ae9c7d3bca 100644 --- a/changelog.d/3714-odbc.changed.md +++ b/changelog.d/3714-odbc.changed.md @@ -1,6 +1,7 @@ -- **The SQL Server store and the DATABASE connector build their TLS keywords in one place.** Each - used to carry its own brace quoting and its own `Encrypt` and `TrustServerCertificate` tail. Both - now call `messagefoundry/odbc_dsn.py`, and both build the same strings as before. Only the store - sends a `ServerCertificate` pin. The shared code refuses a pin beside `encrypt = false` or - `trust_server_certificate = true`, a shape no current caller can send, so a pin the connector - gains later cannot go out on a hop that verifies nothing. (`BACKLOG #3714`) +- **The SQL Server store, the DATABASE connector and the test harness build their TLS keywords in + one place.** Each used to carry its own brace quoting and its own `Encrypt` and + `TrustServerCertificate` tail. All three now call `messagefoundry/odbc_dsn.py`, and each builds + the same strings as before. Only the store sends a `ServerCertificate` pin. The shared code + refuses a pin beside `encrypt = false` or `trust_server_certificate = true`, a shape no current + caller sends: a pin goes out with `Encrypt=strict`, which would quietly override the weakened + posture the caller asked for. (`BACKLOG #3714`) diff --git a/harness/drivers/_database.py b/harness/drivers/_database.py index 082c53602a..945ebaa507 100644 --- a/harness/drivers/_database.py +++ b/harness/drivers/_database.py @@ -28,6 +28,7 @@ from harness.endpoints import ENV_PREFIX, Endpoints from harness.endpoints.database import ENVIRONMENT_ONLY +from messagefoundry.odbc_dsn import odbc_brace, odbc_connection_string from messagefoundry.parsing.peek import DEFAULT_MAX_MESSAGE_BYTES #: The ODBC driver the engine's SQL Server preset names, and so the one the harness dials with. @@ -139,31 +140,21 @@ def target(endpoints: Endpoints, environ: Mapping[str, str] | None = None) -> Ta ) -def _brace(value: str) -> str: - """ODBC-quote a value, doubling any ``}``, so a value cannot add a connection keyword.""" - return "{" + value.replace("}", "}}") + "}" - - def build_dsn(t: Target) -> str: """The ODBC connection string for ``t``. TLS is always on and always verified: the harness - never weakens a hop the engine itself would refuse to weaken.""" + never weakens a hop the engine itself would refuse to weaken. The quoting and the TLS tail are + the engine's own (:mod:`messagefoundry.odbc_dsn`, BACKLOG #3714).""" if any(ch in _ODBC_METACHARACTERS for ch in t.server): raise ValueError("database_server must not contain ';', '{', '}', '=' or a newline") - return ( - ";".join( - [ - f"DRIVER={_brace(ODBC_DRIVER)}", - f"SERVER={t.server},{t.port}", - f"DATABASE={_brace(t.database)}", - "APP={messagefoundry-harness}", - f"UID={_brace(t.username)}", - f"PWD={_brace(t.password)}", - "Encrypt=yes", - "TrustServerCertificate=no", - ] - ) - + ";" - ) + head = [ + f"DRIVER={odbc_brace(ODBC_DRIVER)}", + f"SERVER={t.server},{t.port}", + f"DATABASE={odbc_brace(t.database)}", + "APP={messagefoundry-harness}", + f"UID={odbc_brace(t.username)}", + f"PWD={odbc_brace(t.password)}", + ] + return odbc_connection_string(head, encrypt=True, trust_server_certificate=False) def unavailable(endpoints: Endpoints, environ: Mapping[str, str] | None = None) -> str: diff --git a/messagefoundry/odbc_dsn.py b/messagefoundry/odbc_dsn.py index b3e9759dc9..79e1d3933b 100644 --- a/messagefoundry/odbc_dsn.py +++ b/messagefoundry/odbc_dsn.py @@ -10,9 +10,14 @@ Two rules hold for every string built here: * :func:`odbc_brace` quotes a free-text value, so a value holding ``;``, ``{``, ``}`` or ``=`` (a - password, say) cannot add keywords (STORE-5). -* :func:`odbc_connection_string` puts the TLS keywords **last**. ODBC takes the last value of a - repeated keyword, so nothing earlier in the string can weaken TLS. + password, say) cannot add keywords (STORE-5). It does not refuse a control character. +* :func:`odbc_connection_string` puts the TLS keywords **last**, after every other part. + +The two builders used to say the tail wins because ODBC keeps the last value of a repeated keyword. +Microsoft's ODBC reference for ``SQLDriverConnect`` says a driver uses the FIRST occurrence, and +Microsoft's SQL Server driver has not been measured here either way. So the position is not the +guard against a second TLS keyword. The guard is that every head part is either a fixed keyword or +a quoted or validated value, so no head part can carry one. What the pin does under ``Encrypt=strict``, and how far that was measured, is stated once, in the ``ssl_root_cert`` row of ``docs/CONFIGURATION.md``. @@ -20,8 +25,6 @@ from __future__ import annotations -from collections.abc import Sequence - __all__ = ["odbc_brace", "odbc_connection_string"] @@ -34,7 +37,7 @@ def odbc_brace(value: str) -> str: def odbc_connection_string( - head: Sequence[str], + head: list[str], *, encrypt: bool, trust_server_certificate: bool, @@ -42,18 +45,21 @@ def odbc_connection_string( ) -> str: """Join ``head`` and the TLS tail into one ``;``-terminated ODBC connection string. - ``head`` is every ``KEY=value`` part but the TLS ones, already quoted. The tail follows it: - ``ServerCertificate`` when ``pin`` is set, then ``Encrypt`` and ``TrustServerCertificate``. - ``Encrypt`` is ``strict`` when a pin is sent, and otherwise ``yes`` or ``no`` from ``encrypt``. + ``head`` is every ``KEY=value`` part but the TLS ones, already quoted. A list, so a joined + string cannot type-check here. The tail follows it: ``ServerCertificate`` when ``pin`` is set, + then ``Encrypt`` and ``TrustServerCertificate``. ``Encrypt`` is ``strict`` when a pin is sent, + and otherwise ``yes`` or ``no`` from ``encrypt``. - A pin beside ``encrypt=False`` or ``trust_server_certificate=True`` raises ``ValueError``: the - driver verifies nothing there, so the pin would look like a control and be none. No current - caller passes that shape. The pin file's own checks are the caller's job, as + A pin beside ``encrypt=False`` or ``trust_server_certificate=True`` raises ``ValueError``. The + caller asked for a weakened hop, and a pin sends ``Encrypt=strict``, which Microsoft documents + as ignoring ``TrustServerCertificate``. So the string would quietly turn on encryption or + verification the caller turned off, and the two settings would disagree. No current caller + passes that shape. The pin file's own checks are the caller's job, as :func:`messagefoundry.store.sqlserver.check_pinned_certificate` is the store's.""" if pin and (not encrypt or trust_server_certificate): raise ValueError( - "an ODBC ServerCertificate pin needs encrypt on and trust_server_certificate off; " - "the driver verifies nothing on a weakened posture, so the engine sends no pin there" + "an ODBC ServerCertificate pin needs encrypt on and trust_server_certificate off: " + "a pin sends Encrypt=strict, which would override the weakened posture asked for" ) tail = [f"ServerCertificate={odbc_brace(pin)}"] if pin else [] tail.append(f"Encrypt={'strict' if pin else ('yes' if encrypt else 'no')}") diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index eb225d6c29..e6403367c9 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -2843,8 +2843,8 @@ def connection_string(settings: StoreSettings, *, posture: HopPosture | None = N parts.append("Trusted_Connection=yes") elif settings.auth is SqlAuth.ENTRA: parts.append("Authentication=ActiveDirectoryDefault") - # AOAG multi-subnet fast failover (BACKLOG #100). Not a TLS keyword, so it goes before the - # TLS tail and cannot disturb the last-wins TLS posture. + # AOAG multi-subnet fast failover (BACKLOG #100). Not a TLS keyword, so it goes in the head, + # before the TLS tail (messagefoundry.odbc_dsn). if settings.multi_subnet_failover: parts.append("MultiSubnetFailover=Yes") # Pin the DB server's certificate by file WITHOUT a machine-wide trust import (#45), via the ODBC diff --git a/messagefoundry/transports/database.py b/messagefoundry/transports/database.py index 51330d003e..6688b6da2d 100644 --- a/messagefoundry/transports/database.py +++ b/messagefoundry/transports/database.py @@ -244,8 +244,8 @@ def _build_dsn( else: # entra parts.append("Authentication=ActiveDirectoryDefault") if read_only: - # Advertise read-only intent for the db_lookup pool (ADR 0010). Emitted before the TLS flags so - # the last-wins Encrypt/TrustServerCertificate posture is unchanged. + # Advertise read-only intent for the db_lookup pool (ADR 0010). Not a TLS keyword, so it goes + # in the head, before the TLS tail (messagefoundry.odbc_dsn). parts.append("ApplicationIntent=ReadOnly") return odbc_connection_string(parts, encrypt=encrypt, trust_server_certificate=trust) diff --git a/tests/test_odbc_dsn.py b/tests/test_odbc_dsn.py index d2cbae3415..ab3961d84b 100644 --- a/tests/test_odbc_dsn.py +++ b/tests/test_odbc_dsn.py @@ -8,12 +8,13 @@ * the helper's own output, shape by shape; * both builders' strings, byte for byte, as the code before the move built them. The expected strings were taken from that code; -* that no engine module writes the ``}}`` escape or the TLS keywords itself again. +* that no engine or harness module writes the ``}}`` escape or the TLS keywords itself again. """ from __future__ import annotations import ast +import re from pathlib import Path import pytest @@ -31,7 +32,7 @@ from messagefoundry.transports.database import _build_dsn _WARN = HopPosture(enforcing=False) -_ENGINE = Path(__file__).resolve().parents[1] / "messagefoundry" +_REPO = Path(__file__).resolve().parents[1] # --- the helper --------------------------------------------------------------------------------- @@ -211,49 +212,75 @@ def test_the_connector_string_is_unchanged( assert _build_dsn(settings, read_only=read_only, attested=True) == expected +def test_the_harness_string_is_unchanged() -> None: + from harness.drivers import _database + + t = _database.Target("db.example", 1433, "har}ness", "u}x", "p;w}d=1") + assert _database.build_dsn(t) == ( + "DRIVER={ODBC Driver 18 for SQL Server};SERVER=db.example,1433;DATABASE={har}}ness};" + "APP={messagefoundry-harness};UID={u}}x};PWD={p;w}}d=1};Encrypt=yes;" + "TrustServerCertificate=no;" + ) + + # --- no second copy --------------------------------------------------------------------------------- -#: A string that STARTS with one of these is a connection-string part. Prose that names a keyword -#: mid-sentence, such as a refusal message, is not. -_TLS_KEYWORDS = ("Encrypt=", "ServerCertificate=", "TrustServerCertificate=") +#: A string constant holding a TLS keyword at its start or right after a ``;`` is a connection-string +#: part. ODBC keywords ignore case, so this does too. Prose that names a keyword mid-sentence, such +#: as a refusal message, does not match. +_TLS_PART = re.compile(r"(?:^|;)\s*(?:trustservercertificate|servercertificate|encrypt)\s*=", re.I) def _copies(path: Path) -> list[str]: """Where ``path`` writes the ``}}`` escape or a TLS keyword part itself. - Only a file whose text holds ``}}`` or a keyword name is parsed, which keeps the scan of the - whole package fast. A copy spelled with escapes or split literals would pass unseen.""" + Only a file whose text holds ``}}`` or a keyword name is parsed, which keeps the scan fast. A + copy built from escapes or split literals would pass unseen.""" text = path.read_text(encoding="utf-8") - if "}}" not in text and "Encrypt" not in text and "ServerCertificate" not in text: + lowered = text.lower() + if "}}" not in text and "encrypt" not in lowered and "servercertificate" not in lowered: return [] found: list[str] = [] - tree = ast.parse(text) - for node in ast.walk(tree): - if ( - isinstance(node, ast.Call) - and isinstance(node.func, ast.Attribute) - and node.func.attr == "replace" - and [a.value for a in node.args if isinstance(a, ast.Constant)] == ["}", "}}"] - ): + for node in ast.walk(ast.parse(text)): + if not (isinstance(node, ast.Constant) and isinstance(node.value, str)): + continue + if node.value == "}}": found.append(f"{path.name}:{node.lineno} the }}}} escape") - if ( - isinstance(node, ast.Constant) - and isinstance(node.value, str) - and node.value.startswith(_TLS_KEYWORDS) - ): + elif _TLS_PART.search(node.value): found.append(f"{path.name}:{node.lineno} {node.value!r}") return found -def test_the_helper_is_the_only_engine_module_that_writes_them() -> None: +def test_the_scan_sees_each_shape_of_copy(tmp_path: Path) -> None: + module = tmp_path / "copy.py" + module.write_text( + 'a = v.replace("}", "}}")\n' + 'b = re.sub("}", "}}", v)\n' + 'c = "encrypt=no"\n' + 'd = f"APP={x};Encrypt={e}"\n' + 'e = "TrustServerCertificate = yes"\n' + 'f = "a pin sends Encrypt=strict"\n', # prose: not a part + encoding="utf-8", + ) + lines = sorted(int(hit.split()[0].split(":")[1]) for hit in _copies(module)) + assert lines == [1, 2, 3, 4, 5] + + +def test_the_helper_is_the_only_module_that_writes_them() -> None: + """The engine and the harness, which may import this leaf module, build no copy of their own.""" helper = Path(odbc_dsn.__file__).resolve() # POSITIVE CONTROL: the scan finds the helper's own escape and keywords, so a clean verdict # elsewhere is not a scan that matches nothing. assert len(_copies(helper)) >= 4 - paths = [p for p in sorted(_ENGINE.rglob("*.py")) if p.resolve() != helper] - # The walk reaches the package, at least both callers among the files it reads. + paths = [ + p + for tree in ("messagefoundry", "harness") + for p in sorted((_REPO / tree).rglob("*.py")) + if p.resolve() != helper + ] + # The walk reaches both trees, and every caller among the files it reads. assert len(paths) >= 100 - assert {"sqlserver.py", "database.py"} <= {p.name for p in paths} + assert {"sqlserver.py", "database.py", "_database.py"} <= {p.name for p in paths} others = [hit for path in paths for hit in _copies(path)] assert others == [] diff --git a/tests/test_store_file_hardening.py b/tests/test_store_file_hardening.py index 156ad28003..9663dbf9ab 100644 --- a/tests/test_store_file_hardening.py +++ b/tests/test_store_file_hardening.py @@ -51,7 +51,8 @@ def test_connection_string_neutralizes_password_injection() -> None: ) dsn = connection_string(s) assert "PWD={p}}a;Encrypt=no}" in dsn # brace-quoted, internal } doubled — no breakout - # The real security flags are emitted last (ODBC last-wins), so the injected one can't win. + # The real security flags are emitted last. The quoting above is what keeps the injected one out: + # which repeated keyword the driver keeps is not measured (messagefoundry/odbc_dsn.py). assert dsn.rstrip(";").endswith("TrustServerCertificate=no") assert "Encrypt=yes" in dsn @@ -80,7 +81,7 @@ def test_multi_subnet_failover_default_off() -> None: def test_multi_subnet_failover_emitted_when_enabled() -> None: dsn = connection_string(_sqlserver_settings(multi_subnet_failover=True)) assert "MultiSubnetFailover=Yes" in dsn - # It must sit BEFORE the TLS tail so ODBC last-wins keeps the secure Encrypt/Trust flags final. + # It sits in the head, BEFORE the TLS tail, so the Encrypt/Trust flags stay the final keywords. assert dsn.index("MultiSubnetFailover=Yes") < dsn.index("Encrypt=yes") assert dsn.rstrip(";").endswith("TrustServerCertificate=no") diff --git a/tests/test_store_ssl.py b/tests/test_store_ssl.py index c50dd1025d..4966c3e785 100644 --- a/tests/test_store_ssl.py +++ b/tests/test_store_ssl.py @@ -347,7 +347,7 @@ def test_connection_string_emits_server_certificate_on_secure_posture(tmp_path: # The ODBC Driver 18.1+ ServerCertificate keyword pins the cert by file, brace-quoted (STORE-5). # Match the standalone keyword (a leading ';') so it isn't confused with TrustServerCertificate. assert f";ServerCertificate={{{pin}}}" in dsn - # The TLS keywords stay the last-wins tail, and Encrypt=yes appears nowhere in the string. + # The TLS keywords stay the tail, and Encrypt=yes appears nowhere in the string. assert dsn.rstrip(";").endswith("Encrypt=strict;TrustServerCertificate=no") assert "Encrypt=yes" not in dsn assert dsn.index(";ServerCertificate=") < dsn.index(";Encrypt=strict")