feat(api): the engine always serves TLS, minting on first run (BACKLOG #1276 part A, ADR 0172) - #575
Open
wshallwshall wants to merge 14 commits into
Open
feat(api): the engine always serves TLS, minting on first run (BACKLOG #1276 part A, ADR 0172)#575wshallwshall wants to merge 14 commits into
wshallwshall wants to merge 14 commits into
Conversation
… diagnostic limb) #1245's AVAILABILITY defect is already closed -- _unclaimed_bootstrap is now the single predicate both readers call, and the confirming experiment exists. What the item keeps open is the DIAGNOSTIC limb, and it fences it explicitly: "DO NOT CLOSE THIS ON A GREEN RE-RUN. A green re-run is consistent with an intermittent, not evidence against one." THE RECORDED FAILURE IS `must_change_password` FALSE AFTER admin_reset_password -- NOT the retirement assertion below it. One investigation pass was already spent on the wrong assertion; the item says so in as many words, so the comment says it here where the next reader is standing. A bare assert cannot distinguish three causes, and all three produce an identical red: (a) the write never landed (b) it landed and something reset it (c) the read path is wrong A PAIRED READ AT THE SAME INSTANT -- the store record AND the raw row -- splits them. Uses the same `store._db` seam four other tests in this file already use, rather than introducing a second idiom for reading a row. TWO LINES, AND IT DOES NOT REQUIRE REPRODUCING THE FAILURE FIRST. That is what makes it worth adding before anyone tries to, and it is the item's own argument. VERIFIED BY FORCING IT, because a diagnostic whose failure path never runs is decoration. Injected the (a/b) condition -- cleared the column before the paired read -- and the assertion produced: #1245 intermittent: store record must_change_password=False, raw row must_change_password=0. DISAGREE -> the read path is wrong (c). BOTH falsey -> the write never landed or was reset (a/b), and the raw value tells you which by whether the row exists at all. Both values render and a reader lands on (a/b) without re-deriving anything. Injection reverted, file byte-identical by sha256. 139 passed across three auth modules; ruff format and check clean. NOT CLOSING THE ITEM. The item forbids closing on a green re-run, and this commit adds instrumentation rather than finding the mechanism. Six candidates are already refuted by reading in the item body; a seventh is deliberately not offered as a story.
…G #1276 part A) Owner ruling 2026-08-22, option 3, quoted by the liaison: "engine always serves TLS, supersede 0143 in the same PR". Before this, tls_enabled was literally bool(tls_cert_file) with both keys defaulting None, so an unconfigured engine opened a CLEARTEXT socket. An operator certificate still always wins -- this is a fallback BENEATH [api].tls_cert_file, never a replacement. UNCONDITIONAL ON PURPOSE. A conditional scheme is what let the tray, the harness and the DAST target each decide the scheme their own way; that divergence is the defect the item exists to remove, and you cannot disagree about a scheme that has no conditional. THE EXISTING TEST CAUGHT A REAL BREAK AND I FIXED THE CODE, NOT THE TEST. `tls_terminated_upstream` (+ trusted_proxies) declares a reverse proxy terminating TLS in FRONT of the engine and speaking plaintext to it. My first cut minted there too, which does not harden that deployment -- it BREAKS the proxy's own hop. So "always serves TLS" means the engine never leaves a hop unprotected, NOT that it terminates TLS in every topology. The helper returns None there and the serve path skips the wiring, exactly as before. ONE TEST IS DELIBERATELY INVERTED, and it is renamed so the diff cannot be misread: test_serve_loopback_without_tls_passes_no_ssl_factory asserted "plaintext loopback: no TLS wiring", pinning the behaviour the ruling removes. It now asserts the factory IS wired and the pair IS on disk. That case -- loopback -- is precisely ADR 0143's premise, which is why the ruling pairs this with superseding it. DECISIONS I MADE AND THE ALTERNATIVES REJECTED, both delegated to me explicitly: - PATH: beside the store database. That directory is already the engine's own writable state, already operator-controlled via --db / [store].path, and is NOT operator-authored config -- which keeps dead option 1 (engine edits operator TOML) dead. REJECTED: a new [api].tls_generated_dir knob, a setting for a question with one sensible answer. - LIFETIME: 365 days, inheriting the `cert self-signed` CLI default rather than inventing a second lifetime for the same primitive. MINT-ONCE COMES FREE: _write_private_key already uses O_EXCL + 0o600 + the Windows DACL tighten and REFUSES to overwrite, so a second start loads rather than rotating. FIVE TESTS: operator cert wins and nothing is minted (the control -- it also asserts the state dir stays EMPTY); a first run mints a loadable pair; the CN names the bind host, or every verifying client fails against the cert minted for it; a second start reuses byte-identical material; and the pair survives build_api_ssl_context, which is what serve actually calls. 327 passed across six modules; ruff format, ruff check clean; mypy strict clean on all 267 source files. REMAINING ON #1276: part B (the five DEV ONLY prohibitions + pki.py's "every shipped first-party client defaults to http" prose), part C (eight http:// client defaults, one a shipped surface), the ADR 0143 supersession, and the ROTATION shape -- which is an open decision with the dispatcher: nothing re-mints an expired pair today, so on day 366 this serves a certificate every client rejects.
…erves TLS (BACKLOG #1276) The lander caught that the 1276a branch changed 41 files and touched ADR 0143 in ZERO of them. They were right, and the reason is stronger than "the ruling said to". ADR 0143 DID NOT MERELY ASSUME NO AUTO-TLS. It CONSIDERED terminating TLS on the loopback bind, named it "a full fix", and DECLINED it -- its Accepted decision rests on that decline. #1276 part A does precisely the declined thing. Landing it with 0143 untouched would put an ADR on main that reads Accepted on a premise the code has deleted: trusted, wrong, and nothing anywhere reports it. WHY SUPERSEDE RATHER THAN AMEND, and it is a measurement not a preference. 0143 declined on sizing: "an XL: it means moving the whole API to https by default and migrating every client (harness, apiclient, tray, IDE) in lockstep". Measured on origin/main, that is false: tray INFERS -- service_toml_uses_tls, ONE caller apiclient DOES NOT -- zero tls_cert_file references; it is GIVEN a URL IDE DOES NOT -- its hits are MLLP CONNECTOR schema, same name, other setting harness DOES NOT INFER, it ASSUMES -- hardcoded http://127.0.0.1:8765 One inference site plus one-line default flips. The XL that justified the decline does not exist, so the decision falls with its premise. 0143's ANALYSIS SURVIVES AND IS WHY THIS IS BETTER: its diagnosis that a secure cookie over cleartext http is dropped by Chrome and Safari and BREAKS LOGIN is correct. Its app.state.loopback mechanism goes vestigial where the engine terminates TLS -- but NOT in the tls_terminated_upstream topology, which still reaches the engine over plaintext. Recorded so nobody deletes it as dead. CARRIES BOTH OWNER RULINGS, since three halves belong to one change: always-TLS (22:19Z), 0143 superseded in the same PR, and the re-mint AUDITED not silent (22:50Z). NUMBER ALLOCATED FROM THIS WORKTREE and the index row is in this same commit, per the ledger gate's ownership rule. NOTE ON THE GATES, CORRECTED BY MEASUREMENT AFTER THE FIRST WRITING OF THIS MESSAGE. The original text claimed committing from a fresh worktree "would have failed the ledger gate, which pins 0172 to THIS worktree". THAT IS FALSE and I tested it rather than keep asserting it: this commit WAS made from a second worktree, and the ledger gate PASSES -- ledger_check.py exits 0 there, and re-running the hooks shows "ledger gate (ADR/BACKLOG number reuse) ... Passed". So ADR ownership does NOT require the committing worktree to be the allocating one; the registry lives beside the SHARED object store and every worktree of the repo sees the same allocations. Only ONE gate was ever in play here, not two. I ALSO NEARLY MISREAD THAT TEST. The cherry-pick showed no hook output, and I started to conclude the hooks had not run -- but I had piped it through `tail -12`, which cut the hook lines off the top. My instrument truncated the evidence, not the gate. The same shape cost a peer a false "backup missing" an hour ago with `tail -3`.
The required backlog-hygiene gate on PR 575 failed: a PR that claims BACKLOG #N and changes engine code must also touch the backlog item namespace. The gate's condition accepts any edit to docs/BACKLOG.md or docs/archive/backlog/. The banner also asserted "Filed 2026-08-16 - not started", which part A makes false, so the edit was owed on its own merits rather than only to satisfy a gate. One blockquote line added. The status glyph is untouched and the item stays OPEN, because part B is not built: nothing re-mints an expired generated pair, build_api_ssl_context performs no expiry check, and the rotation shape is undecided. Controls, run against HEAD with parse_items rather than a hand-rolled scan: rows 363 before, 363 after open rows 247 before, 247 after status changes 0 rows added or removed 0 glyph counts unchanged lines added 1 Written by the LANDER. PR 575's author deliberately did not touch this banner and said so; this is gate satisfaction plus a factual correction, not a banner rewrite, and the note says as much inline so they can correct it freely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
wshallwshall
enabled auto-merge (squash)
August 25, 2026 01:23
wshallwshall
disabled auto-merge
August 25, 2026 01:23
…verify WIP toward PR 575's CI fix, verified partial: the health-check stage now succeeds (confirmed locally -- GET /health 200 -- against test_connscale_ smoke.py, which previously timed out before ever reaching the engine). EngineNode.url (failover.py, base class also used by ShardCertNode) was hardcoded http://; the engine always serves TLS now and mints a self-signed placeholder with no chain of trust. Fixed to https://, and every ad-hoc httpx.AsyncClient health-check construction that talks to it (failover.py, connscale/runner.py, shardcert.py's _await_health, estate/runner.py) gets verify=False -- reasoning per Builder 1: the harness's trust anchor is the PID it spawned and holds the handle to, not a certificate that same PID just minted. Cosmetic engine_url report fields updated to https:// too. NOT FIXED, and this is the reason CI is still red after this commit: EnginePoller (harness/load/enginepoll.py) does its polling through messagefoundry.apiclient.EngineClient, a different, more sophisticated shared client (also used by the web console) that verifies certs properly and has no verify=False escape hatch -- only `cacert`, to pin trust to a specific PEM. That is a real design decision, not a mechanical one: EnginePoller is constructed with multi-node URL lists at several call sites (multishard.py, shardcert.py x6, harness/load/runner.py), each node mints its OWN self-signed cert, and correctly threading per-node cert trust through a class the console also depends on needs the PR owner's judgement, not a one-off client instance escape hatch. Confirmed test_connscale_smoke.py now gets past node startup and fails later, inside _await_inbound_rows -> EnginePoller, exactly where this stops. Handed back to Builder 1 with this branch as a starting point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The engine now always serves TLS and mints a self-signed placeholder when no
operator certificate is configured. Every harness driver hardcoded http://, so
each one spoke cleartext to a TLS socket and no node ever became healthy.
The harness supplies the certificate rather than chasing the one the engine
mints. ensure_api_tls_material returns operator-supplied material on its first
branch and never mints over it, so handing each node MEFOR_API_TLS_CERT_FILE
makes the generated path unreachable. Two properties follow, and both are why
this shape was chosen over reading the minted cert off the node's state dir:
- No race. The pair is on disk before any engine spawns, so no client waits
for a file to appear. Waiting was the alternative, and its timeout would
surface as "nodes did not become healthy" -- indistinguishable from the bug
being fixed. A fix whose failure mode renders identically to the defect is
a second place for the same red to come from.
- One anchor, not N. Minting per node would mean threading a different CA
through every multi-node EnginePoller URL list. Minting once collapses all
eleven call sites to a single constant, resolved in EnginePoller itself, so
no caller signature changes.
EngineNode is the one seat all five drivers pass through (ShardCertNode
subclasses it), so the env pair and the https scheme are set there once.
Two things a site-by-site sweep would not have reached:
- batchbox spawns a CHILD harness process to poll engines the PARENT started.
A per-process mint would hand that child a different certificate from the
one those engines were given, so every poll would fail verification. The
anchor is published into the environment and inherited; no single-process
test can observe this, so it has one that spans a real subprocess.
- The four ad-hoc httpx clients pin rather than skipping verification. The
harness minted the cert, so verifying costs nothing and keeps them on the
same posture as EngineClient, which offers pinning and no way to switch
verification off.
ingress_probe is deliberately untouched: it runs uvicorn in-process with no
ssl_context_factory, so it genuinely serves cleartext and its http:// is
correct. A blanket http->https sweep would have broken a working path.
shardcert's two-box URLs stay http and now say why. That engine runs on another
host this process never spawned, so the run's anchor cannot cover it and
_cacert_for declines to pin a non-loopback URL. Converting them to https would
fail verification against a certificate this box has never seen. Whether the
two boxes share one certificate or the rig ships the engine box's cert is a
provisioning decision, and no CI workflow exercises that path.
Verified: test_connscale_smoke 9 passed against real engine subprocesses;
1130 passed / 41 skipped across the connscale, estate, failover, enginepoll and
monitor selection; new anchor tests prove the pinned context completes a
handshake a default context rejects.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
|
Harness fix landed (fast-forwarded onto this branch, 6 commits total) -- verified locally before pushing: 17/17 in tests/test_harness_tls_anchor.py + tests/test_connscale_smoke.py, including all 8 new anchor tests proving the pinned context completes a handshake a default context rejects. Flagging one thing raised in fleet coordination so it doesn't quietly disappear as "harness fixed, mission accomplished": |
wshallwshall
enabled auto-merge (squash)
August 25, 2026 20:27
CI's crypto-inventory gate (ASVS 11.1.3) correctly flagged the new harness/load/tlsmat.py as undocumented crypto use -- it imports ssl to build the pinned client context for the harness's own minted TLS anchor. Registered in three places, each with its own guard: - scripts/security/crypto_inventory_check.py's machine-readable INVENTORY - docs/ASVS-L2-PHASE0-CHANGES.md section 4's human-readable table, as a new "Load-test harness -> spawned-engine TLS" row - tests/test_key_usage_scope_inventory.py's completeness check classifies the new row as NOT key material: its certificate is minted via the same self-signed dev-cert path (pki.make_self_signed) the existing Cert tooling row already scopes, so there is nothing new to scope here. Verified: crypto_inventory_check.py reports 76 documented sites, no drift; test_crypto_inventory_doc.py + test_key_usage_scope_inventory.py both 14/14; ruff check + format clean on both changed Python files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…TLS fix Not the CodeQL false positive -- three separate, real issues, all confirmed across every failing leg before fixing (same three everywhere, no new ones): 1. tests/test_security_static.py::test_crypto_roots_carry_no_unrecorded_call_site -- this file keeps its own hand-maintained duplicate of the non-messagefoundry slice of crypto_inventory_check.py's INVENTORY, as a second, independently verified copy (same shape as #1301/#1338's banner guards). Added the new harness/load/tlsmat.py entry here too. While fixing it, corrected the docstring's stale claim that the gate's own CLI "still defaults to" messagefoundry/ alone -- WALK_ROOTS already covers harness/tee/scripts/ messagefoundry_webconsole too, confirmed by checking every existing entry in this set already has a matching one in the gate's INVENTORY. 2. tests/test_bench_batch_two_box.py::test_build_remote_argv_shape -- a real, deliberate behavior change in build_remote_argv (harness/load/connscale/ batchbox.py), not a regression: the connscale-remote child process it spawns inherits the harness's minted TLS anchor via environment (per tlsmat.py's cross-process design), so it now needs to reach the engine over https like everything else. Updated the test's expectation and a same-file fake node's .url for consistency (unasserted, but now realistic rather than stale). 3. docs/adr/0172-...md's own "Related" line cited "ADR 0002 (0002-tls- everywhere.md)" -- that file was never tracked under that name. The actual ADR 0002 is 0002-phase2-transport-security-and-strong-auth.md (confirmed by content: transport security + strong auth, matching what a "TLS everywhere" reference means here). Fixed the link. Verified locally: 81/81 across all three affected test files, ruff check + format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Handed to this seat by Builder 1. Landed as a branch that was already on origin; this PR makes it landable.
What it does
[api].tls_cert_fileandtls_key_fileboth shippedNoneandtls_enabledwas literallybool(self.tls_cert_file), so an unconfigured engine opened a cleartext socket. This mints a self-signed pair on first run beneath any operator-supplied material.Severity is conditional per CLAUDE.md section 0: zero deployments, so nothing is served in the clear today. This is what a deploying site would hit.
Three commits:
eeabc6batest(auth): split three causes of one intermittent red (BACKLOG #1245 diagnostic limb)48b98465feat(api): the engine always serves TLS, minting on first run (BACKLOG #1276 part A)25bb59addocs(adr): ADR 0172 supersedes 0143's decision (BACKLOG #1276)Note the first commit carries a #1245 diagnostic limb rather than #1276 work.
What this seat verified
25bb59ad39a0e02dmerge-treerc=0docs/adr/README.mdrowdocs/BACKLOG.mdmake_self_signedputs an IP literal in asiPAddress, so127.0.0.1verifies (BACKLOG #1179)Verification caveat, carried from the author and NOT re-run here
ruff check,ruff formatandmypyreported clean on both changed modules, and 118 tests pass intest_api_tls.pyandtest_auth_service.py. This is NOT a full-suite green: extras-gated modules were deselected at collection. Do not read it as suite-green. The 13 required contexts on this PR are the authoritative check.Known gap, filed rather than forgotten
Nothing re-mints an expired generated pair.
build_api_ssl_contextperforms no expiry check, so a site running past day 365 would serve an expired certificate. The rotation shape is an open decision on #1276; until it lands,CertExpiryRunneralarms on this path like any other served cert. The code documents this at the point it matters.Flagged, not acted on
BACKLOG #1276 is claimed by worktree
repo-security-review-afbae3, which no longer exists.claim.ps1offers-Forceto release it. Not this seat's to release.Auto-merge deliberately NOT armed, given the verification caveat above.