Skip to content

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) - #2587

Merged
wshallwshall merged 3 commits into
mainfrom
b342-3714d
Oct 28, 2026
Merged

wshallwshall merged 3 commits into
mainfrom
b342-3714d

Conversation

@wshallwshall

@wshallwshall wshallwshall commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

Merge danger

Reversible: yes -- a revert of this change on main undoes all of it.

Blast radius: a new leaf module, messagefoundry/odbc_dsn.py (odbc_brace, odbc_connection_string), now builds the ODBC TLS tail (ServerCertificate only with a pin, Encrypt, TrustServerCertificate) and the brace quoting for three callers: the SQL Server store's connection_string (store/sqlserver.py), the DATABASE connector's _build_dsn and _odbc_literal (transports/database.py), and the harness build_dsn (harness/drivers/_database.py). Their local _odbc_brace / _brace copies are gone. Output is byte-identical (Evidence). One new refusal no current caller reaches: a pin beside encrypt=false or trust_server_certificate=true raises ValueError (sqlserver_pin_path() already returns None on a weakened posture). Comments and docstrings in config/settings.py, store/sqlserver.py, __main__.py, tests/test_store_ssl.py, scripts/security/crypto_inventory_check.py and crypto_operations.py now link to the one statement of the pin fact. 12 files.

Evidence

  • Identical output: a matrix script run on old main and on this branch over 1548 shapes gives equal SHA-256 (store: encrypt, trust, pin None/plain/with } ; =, all three auth modes, MultiSubnetFailover, passwords };DROP, a;b=c{d}, }}x}, a database name with }}, the escaped weakened posture; connector: encrypt as bool, None, "false", "yes", trust, auth sql/integrated/entra/SQL, read_only, three driver names; _odbc_literal with both refused sets); the harness, 216 inputs identical against origin/main's harness/drivers/_database.py.
  • Tests: tests/test_odbc_dsn.py (25 tests) pins golden strings taken from the old code for all three callers, and scans messagefoundry/ and harness/ for any module that writes the }} escape or a TLS keyword part (case-insensitive, with a positive control and a walk-size guard). Sabotage turned red every time: no } doubling (13 failed), no strict (2), no refusal (3), dropped head (15), tail not last (15), a regrown brace copy (1), a regrown tail copy (5).

BACKLOG #3714 slice D -- batch 342

Source branch b342-3714d, head 33a83a7b40117facf41723d2eff7061dd893b5c7, base origin/main 83ad747d61. Commits b7bfd52 (the helper), 5b86c18 (the copies), 33a83a7 (review round 1). The head is the reviewed head: review round 2 read it, and nothing was committed after.

Items

  • Item 12, built: "The DATABASE connector builds its own TLS tail and its own _odbc_brace" -- now one helper for the store, the connector and the harness. It sits at top level so transports/ never imports store/.
  • Slice C's leftover copies (engine PR 2562's "Copies left in other PRs' files", now that PR 2533 merged): config/settings.py ~959, store/sqlserver.py (check_pinned_certificate, the connection_string block, purge_search_presets now a feature fact pointing at SYSTEM-REQUIREMENTS.md for the floor), __main__.py ~4765, tests/test_store_ssl.py ~340 and ~413, crypto_inventory_check.py ~1550 (also stale: it named functions that no longer hold the check) all link to the CONFIGURATION.md ssl_root_cert row. Left: sqlserver.py's operator refusal text (Microsoft's documentation, not the measurement); tests/test_sqlserver_store.py and ADR 0173 (open PR 2579 edits both). Nothing here claims PR 2579's Linux measurement.
  • A withdrawn premise: an earlier comment called the ODBC tail's position the guard ("last-wins"). The reviewer cites Microsoft's SQLDriverConnect page as first-occurrence (not checked against the driver). The helper now says position is not the guard; the quoting and validation are. Three comments that leaned on last-wins were changed (sqlserver.py, database.py, tests/test_store_file_hardening.py).

Checks (Builder's report)

  • Ran: /simplify; ruff check, ruff format --check; mypy in all four section 7 legs; changelog_fragments.py check (changelog.d/3714-odbc.changed.md); pytest -n 6 over 42 files: 3495 passed, 254 skipped (every brief file plus the harness database and scenario tests, the vacuous-absence lint, the crypto inventory scanner, the start-up import budget, and every test naming connection_string or _build_dsn).
  • Skipped: the full suite.
  • Hosted-only legs to read: CI gate, and the live SQL Server legs (test_sqlserver_store, test_database_*_integration), which build real strings through the helper.

Known open findings (shipped open; two rounds, cap reached; none changes behaviour or weakens a control in this branch)

  1. For the Lander's triage first: odbc_brace does not refuse NUL or control characters. A NUL in the head could cut the TLS tail at the driver, and with Driver 17 the default would be Encrypt=no. Unmeasured; predates this branch; fixing it is a control change.
  2. The helper does not refuse a head part keyed Encrypt, TrustServerCertificate or ServerCertificate.
  3. _build_dsn reads trust = bool(s.get(...)), so "false" counts as true (predates the branch).
  4. The SERVER metacharacter check is written three times (database.py, settings._no_odbc_injection, the harness).
  5. Nothing tests that odbc_dsn.py stays a leaf (unlike mllpcodec and tls_sigalgs).
  6. ADR 0173 ~1174 still says "last-wins tail (code)", which this branch withdraws (PR 2579 edits that ADR).
  7. pipeline/cert_expiry.py ~238 writes out the pin predicate instead of calling sqlserver_pin_path().
  8. The scan misses brace quoting with no doubling (f"PWD={{{p}}}") and flags any "}}" constant.
  9. The module and test docstrings say "both" builders; there are three.
  10. The pin-on-weakened refusal is a third copy of that rule.
  11. The purge_search_presets comment can read as two floors.

Proposed ledger banner (for the Lander)

PARTIAL, slice D -- item 12 built (one ODBC TLS tail helper for the store, the connector and the harness, byte-identical output); slice C's leftover copies linked. Items 6 and 8 are PR 2579 (measured); item 11 is #4325; open as before: items 2, 5 and the slice A and C findings.

Landing order

None required. Clean against main and against open PRs 2564, 2567, 2573, 2576, 2579, 2586 (each merged onto current main first).

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge; no new actionable issue was found.

Summary

Moves ODBC quoting and TLS keywords into one helper for the SQL Server store, DATABASE connector, and harness.

  • The SQL Server store, DATABASE connector, and test harness share one ODBC string builder.
  • Security notes point to one source for ODBC pin facts.

No new actionable issues found.

Acknowledgments supplied by wshallwshall, not repeated as findings:

  • NUL and control characters remain accepted; this predates the change and needs a separate control change.
  • TLS keys in head remain unchecked.
  • The connector still treats "false" as true for trust_server_certificate; this predates the change.
  • Server checks remain repeated in three places.
  • No test requires the helper to remain a leaf module.
  • ADR 0173’s “last-wins” wording is deferred to PR CI: the SQL Server certificate pin test runs on the 2025 leg against a strict server, with the certificate untrusted and trusted (BACKLOG #3714 items 6, 8) #2579.
  • pipeline/cert_expiry.py still repeats the pin predicate.
  • The copy scan misses quoting without doubling and can flag unrelated "}}" constants.
  • Two docstrings still describe “both” builders.
  • The pin refusal repeats an existing rule.
  • The purge_search_presets comment can suggest two server-version floors.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Store["SQL Server store"] --> Helper["odbc_dsn"]
  Connector["DATABASE connector"] --> Helper
  Harness["Test harness"] --> Helper
  Helper --> DSN["Quoted values and TLS keywords"]
Loading

Reviews (1) · Last reviewed commit: "ODBC helper review round 1: harness fold..." · Reviewed by Greptile

wshallwshall added 3 commits October 27, 2026 18:04
…onnector

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.
…ION.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.
…l 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.
@wshallwshall

Copy link
Copy Markdown
Collaborator Author

QA -- korus roles/BUILDER.md step 11
Level: xhigh, from the brief. Tag: xhigh effort → 10 inline angles → dedup (no verify) → sweep → ≤15 findings (forks a33e1e01498647ab1 round-6893ce442e, aabea055096911edf round-8ba5da20b4).
Rounds: 2. Findings: round 1, 8 found: 5 fixed (tail premise, refusal reason, scan blind spots, harness copy, str head), 1 not changed (crypto inventory: no crypto-shaped call, residual prose updated), 2 open (NUL, trust truthiness); round 2, 12 found, 0 fixed, all open.

@wshallwshall wshallwshall added the qa Builder QA record posted; not a merge gate label Oct 27, 2026
@wshallwshall

Copy link
Copy Markdown
Collaborator Author

Lander (session 8a04c9f3), 2026-10-28. Enqueuing at pinned head 33a83a7b40117facf41723d2eff7061dd893b5c7 under the owner's ruling of 2026-10-28 in the Lander's chat: the Lander may name each finding on the PR, merge, and file its row afterwards.

Review proof: round 2 read this head and made no fixes. All nine required contexts are a settled success on it.

Open findings. All are pre-existing or cosmetic, and none changes behaviour or weakens a control through this merge. Rows follow in a Lander ledger pass:

  1. odbc_brace does not refuse NUL or control characters (pre-existing)
  2. A head part keyed Encrypt, TrustServerCertificate or ServerCertificate is not refused
  3. _build_dsn reads the string "false" as true (pre-existing)
  4. The SERVER metacharacter check is written three times
  5. No test keeps odbc_dsn a leaf module
  6. ADR 0173 still says "last-wins"
  7. cert_expiry writes out the pin predicate instead of calling sqlserver_pin_path()
  8. The scan misses brace quoting with no doubling
  9. Docstrings say "both" builders where there are three
  10. The pin-on-weakened refusal is a third copy of that rule
  11. The purge_search_presets comment can read as two floors

@wshallwshall
wshallwshall added this pull request to the merge queue Oct 28, 2026
Merged via the queue into main with commit df6086d Oct 28, 2026
56 of 57 checks passed
@wshallwshall
wshallwshall deleted the b342-3714d branch October 28, 2026 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

qa Builder QA record posted; not a merge gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant