diff --git a/changelog.d/3714-odbc.changed.md b/changelog.d/3714-odbc.changed.md new file mode 100644 index 0000000000..ae9c7d3bca --- /dev/null +++ b/changelog.d/3714-odbc.changed.md @@ -0,0 +1,7 @@ +- **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/__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/messagefoundry/odbc_dsn.py b/messagefoundry/odbc_dsn.py new file mode 100644 index 0000000000..79e1d3933b --- /dev/null +++ b/messagefoundry/odbc_dsn.py @@ -0,0 +1,67 @@ +# 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). 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``. +""" + +from __future__ import annotations + +__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: list[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. 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 + 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: " + "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')}") + 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..e6403367c9 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. + # 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 - # 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..6688b6da2d 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,26 +230,24 @@ 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 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") - 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..ab3961d84b --- /dev/null +++ b/tests/test_odbc_dsn.py @@ -0,0 +1,286 @@ +# 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 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 + +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) +_REPO = Path(__file__).resolve().parents[1] + + +# --- 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 + + +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 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 fast. A + copy built from escapes or split literals would pass unseen.""" + text = path.read_text(encoding="utf-8") + lowered = text.lower() + if "}}" not in text and "encrypt" not in lowered and "servercertificate" not in lowered: + return [] + found: list[str] = [] + 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") + elif _TLS_PART.search(node.value): + found.append(f"{path.name}:{node.lineno} {node.value!r}") + return found + + +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 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", "_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 efa3dd833f..4966c3e785 100644 --- a/tests/test_store_ssl.py +++ b/tests/test_store_ssl.py @@ -340,15 +340,14 @@ 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). # 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") @@ -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