fix(security): lock remaining secret writers down before publish

[kyleseaman]

Problem / Motivation

The remaining #5346 writers still published a secret-bearing file and then
locked it down. On Windows chmod / chmod_safe is a no-op, so the payload
landed under the inherited parent DACL. On POSIX the file existed at the
published name with a wider mode until the chmod returned. After
#5493 converted the
denied-commands keystone, three sites were still on that shape:

• write_pod_config (write_text then os.chmod 0o600)
• snapshot merge restore of missing security files (shutil.copy2 then
  restrict_to_owner on the dest)
• SEL _append_lines_locked (os.chmod after append — same event-loop /
  icacls reason as #5228)

The denied-commands writer on this branch still drops a pre-write mkdir
that would follow a planted parent link, and adds a failed-lockdown test
#5493 did not.

Why it matters

These files carry provider tokens (pod config) and security-restore secrets
(telemetry_salt). A co-tenant who can read the inherited DACL can read a
restored secret. The lockdown-before-publish gate (#5348) was tracking the
debt in KNOWN_UNCONVERTED; leaving it there keeps the window.

What changed (motivation → approach → change)

Symptom was write-then-restrict at the published path. The established fix
in this tree is lock the temp down first, then publish.

• Pod config — create-only path now uses
  atomic_write(..., restrict_to_owner=True). That helper encodes UTF-8;
  the previous write_text call did not pass encoding=.
• Snapshot merge restore — _copy_locked locks the temp, writes the
  payload, then publishes with os.link (the create-only shape
  _get_telemetry_salt uses). os.replace / atomic_write would clobber a
  dest created in the window (--force restore racing a live gateway
  creating telemetry_salt). Any publish OSError (live dest, no hard
  links, restrict failure, short write) skips that file without aborting
  merge after earlier components were applied; dest stays missing and
  _get_telemetry_salt regenerates. Size is checked only when the source
  or dest is named telemetry_salt (32 bytes); a planted giant is skipped
  the same way. _copy_locked returns whether it published, so a lost
  os.link race does not print "restored".
• Denied commands — drop the mkdir that ran before
  atomic_write(..., restrict_to_owner=True). The helper already refuses a
  linked parent then mkdirs; a mkdir here would walk a planted link first.
  #5493 already routed the write through atomic_write.
• SEL append — not converted: restrict_to_owner shells out to icacls
  and this path can run on the asyncio loop. POSIX os.open(..., 0o600)
  already creates owner-only. Annotated
  # lockdown-ok: #5228 -- icacls would block the event loop.
• Gate — KNOWN_UNCONVERTED is empty (shrink-only). os.link is now a
  recognized publish call, so a lockdown after os.link(tmp, dest) is the
  same defect as a lockdown after os.replace.

Tests

• test/test_lockdown_before_publish.py — real-tree scan asserts no
  unclassified write-then-restrict and that KNOWN_UNCONVERTED is empty;
  a fixture asserts os.link then restrict-dest is reported, and
  lock-temp-then-link is not.
• test/test_pod.py::TestPodConfigWrite.test_a_failed_lockdown_publishes_no_config
  — a refused restrict leaves no published config.json.
• test/test_denied_commands_api.py::test_a_failed_lockdown_publishes_no_denied_commands
  — a refused restrict leaves no published denied_commands.json.
• test/test_snapshot.py::TestMergeRestoreLocksBeforePublish — merge restore
  of telemetry_salt locks the temp, not the dest; a failed restrict leaves
  the dest uncreated; an existing dest is not overwritten; a dest created
  before os.link is not clobbered; an oversized source is skipped; a
  hard-link OSError skips that file and does not abort merge of other
  components; a close-time writeback error does not retry the released
  fd and abort merge. The fake-home helper writes a 32-byte salt so
  merge restore of a normal snapshot still succeeds.

Local: python3 scripts/check_lockdown_before_publish.py passed; targeted
lockdown, pod, denied-commands, and snapshot restore-merge tests passed
after rebase onto origin/main.

Manual verification

N/A — unit coverage sufficient. The behaviour is the file-lockdown layer:
POSIX mode 0o600 is asserted in the tests; Windows DACL is the same
restrict_to_owner contract already exercised by other keystone writers.
Create-only publish is asserted by the os.link race test.

Screenshots / video

Why no screenshot: backend file-lockdown only; no rendered UI change.

Related Issues

Fixes #5346

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 52e11eaa88fa5c7856044d266229cb60510826dc and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 52e11ea

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 52e11eaa88fa5c7856044d266229cb60510826dc: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 52e11eaa88fa5c7856044d266229cb60510826dc — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered. Producing the review.

First-Principles-Verdict: CONCERNS

The restore path of telemetry_salt is now locked down, but its create path — the same secret, same DACL exposure — ships unfixed on Windows.

What this change ships

Intent: close the last write-then-restrict secret writers so a co-tenant can never read a token/salt through an inherited DACL — a FIX.

1. Pod config is owner-only before any token-bearing byte lands — justified
2. Pod config now written with explicit UTF-8 encoding — declared, rides along benignly
3. Merge restore locks the salt's temp and never clobbers a live salt — justified
4. A restore failure now skips the salt instead of aborting merge — declared, justified
5. A snapshot salt ≠ 32 bytes is silently skipped — one consumer, generalized
6. Denied-commands write no longer pre-creates parent dirs through a plantable link — justified
7. SEL chmod annotated as accepted debt (no behavior change) — justified (fix: lock credential writes to owner, de-corrupt Windows install cmd (#4987) #5228)
8. Gate recognizes os.link as a publish; KNOWN_UNCONVERTED emptied — justified, cause-level
9. "restored" printed only when the file actually landed — declared, justified

Watch

• "lock remaining secret writers down before publish" overstates completion. I grepped os.link publishers: 3 sites, 2 unfixed. _get_telemetry_salt (handlers_system.py:116) creates the same telemetry_salt via write → chmod_safe(tmp) → link; chmod_safe is a documented silent no-op on Windows, so the created salt stays under the inherited DACL forever — the exact harm the description names for the restored one. beacon.install_id (beacon.py:389-396) writes the payload before restrict_to_owner and publishes even when the restrict fails (suppress(OSError)). Both escape the gate because temps are exempt from write-order scanning. Accepted-and-deferred at minimum; the description should say the create path is left.
• _copy_locked is the third hand-rolled mkstemp+lock+link spelling (with handlers_system.py:111, beacon.py:387); three copies of this order-sensitive sequence will diverge, as the two older ones already have.

Subtractions

• Drop the src.name == "telemetry_salt" or dst.name == "telemetry_salt" conditional in _copy_locked (snapshot.py:1178) — its only caller (snapshot.py:1348) iterates CORE_FILES["security"], which has exactly one entry, telemetry_salt (snapshot.py:154); apply the 32-byte check unconditionally.
• Drop the second if dst.exists(): return False after read_bytes (snapshot.py:1183) — os.link already refuses an existing dest and the except OSError branch returns False; the recheck removes nothing.

[FIRST-PRINCIPLES-REVIEWED] 52e11ea

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 52e11eaa88fa5c7856044d266229cb60510826dc — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound continuation of the lock-temp-then-publish pattern, but _copy_locked trades the old fail-loud restore for a fully silent skip under a success banner.

Watch

• _copy_locked swallows every OSError (restrict failure, no-hardlink filesystem, oversized/unreadable source) and prints nothing, while _do_merge still prints ✅ security — "dest stays missing and _get_telemetry_salt regenerates" justifies the skip for telemetry_salt specifically, yet the helper is wired into the generic CORE_FILES["security"] loop. A future file added to that tuple inherits skip-silently semantics that are only safe for regenerable files, and today an operator restoring onto a no-hardlink mount loses salt continuity with no visible signal.

Suggestions

• Print a {f}: skipped (<reason>) line on the return False paths so a failed security restore is visible in the merge output, mirroring the existing restored (was missing) line.

[DESIGN-REVIEWED] 52e11ea

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 52e11eaa88fa5c7856044d266229cb60510826dc — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 52e11ea

Verdict parsed from the review's SHA-scoped output markers for commit 52e11eaa88fa5c7856044d266229cb60510826dc.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 52e11eaa88fa5c7856044d266229cb60510826dc: <one-sentence reason>

[kyleseaman]

• Raw replacement corrupts concurrent state and aborts on Windows — fixed at 3d82d8edb4522cb2bc26989f2a1c217db8070a83.

os.replace(tmp_path, dst)
--force restore + gateway salt creation -> live salt overwritten; Windows scanner hold -> uncaught PermissionError -> restore aborts.
Fix: Revert _copy_locked; publication needs a create-only, Windows-resilient design.

_copy_locked now uses atomic_write(..., restrict_to_owner=True) (Windows replace_with_retry) and returns if dest already exists, so a live telemetry_salt is left alone. Reverting the helper would put write-then-restrict back. A planted giant salt is also refused before read_bytes() (32-byte contract matching _get_telemetry_salt).

[kyleseaman]

• The chosen shape rests on a false premise / contract parity not delivered — fixed at 3d82d8edb4522cb2bc26989f2a1c217db8070a83.

The PR keeps _atomic_json_write because its replace_with_retry is load-bearing — but atomic_write already renames via replace_with_retry. _atomic_json_write lacks _refuse_linked_parent, and _write_denied_state still mkdirs first.

Denied-commands now calls atomic_write(..., restrict_to_owner=True) and no longer mkdir's first. The restrict_to_owner kwarg on _atomic_json_write was dropped.

• _copy_locked is a third hand-rolled writer — fixed at 3d82d8edb4522cb2bc26989f2a1c217db8070a83.

atomic_write(dst, src.read_bytes(), restrict_to_owner=True) covers it.

That is now the implementation, plus the create-only exists() guard and the 32-byte cap.

[kyleseaman]

• Two of the four conversions hand-build what atomic_write(..., restrict_to_owner=True) already is — fixed at 3d82d8edb4522cb2bc26989f2a1c217db8070a83.

atomic_write already supports restrict_to_owner=True with replace_with_retry. The new _atomic_json_write kwarg has exactly one True caller, and _copy_locked has one consumer.

Both remaining callers now go through atomic_write. _atomic_json_write is back to its original signature.

[kyleseaman]

• new os.replace(tmp_path, dst) in _copy_locked skips replace_with_retry — fixed at 3d82d8edb4522cb2bc26989f2a1c217db8070a83.

so a Windows AV/indexer holding the freshly-copied temp raises PermissionError (WinError 5/32/33) and aborts the merge restore

_copy_locked now publishes through atomic_write, whose rename is replace_with_retry.

[kyleseaman]

• Atomic replacement clobbers a concurrently created live salt — fixed at 3991140f8167c1b3ad23afe13417005a3529f852.

BLOCKING -- snapshot.py:1130 -- Atomic replacement clobbers a concurrently created live salt
atomic_write(dst, payload, restrict_to_owner=True)
Forced merge + concurrent _get_telemetry_salt() creation -> os.replace overwrites the live salt.
Fix: Publish the locked temp create-only and leave an existing destination untouched.

_copy_locked no longer uses atomic_write. It locks the temp, writes the payload, then publishes with os.link. FileExistsError leaves the live dest. A dest created in the window is covered by test_a_dest_created_before_link_is_not_clobbered. Same span as round 1 (snapshot.py _copy_locked); this is round 2.

A malformed telemetry_salt is skipped rather than raised, so merge restore of other components is not aborted mid-apply.

[kyleseaman]

• _copy_locked hardcodes the 32-byte salt invariant for every security file — fixed at 3991140f8167c1b3ad23afe13417005a3529f852.

_copy_locked hardcodes the 32-byte telemetry_salt invariant but is called generically for every entry in CORE_FILES["security"] — the next file added to that tuple fails restore with "expected 32 bytes". Key the size check on src.name == "telemetry_salt" (or move it to the caller).

Size is checked only when src.name or dst.name is telemetry_salt. A mismatch skips that file instead of aborting the merge.

[kyleseaman]

• The description narrates a different implementation than the diff — fixed at 3991140f8167c1b3ad23afe13417005a3529f852.

The diff fixes all four #5346 sites at cause level, but the description narrates a different implementation and two test files this diff does not contain.

The body now matches the diff: pod config and denied-commands use atomic_write(..., restrict_to_owner=True); merge restore publishes create-only with os.link; _atomic_json_write is unchanged; SEL stays lockdown-ok.

[kyleseaman]

• fixed — os.link on a filesystem without hard links aborted _do_merge after memory and config were already applied. _copy_locked now treats any publish OSError as skip: dest stays missing and _get_telemetry_salt regenerates. Landed at b1759c03364a116a441134b72d1c427fe6428be5.

  os.link on a filesystem without hard links raised into _do_merge after earlier components were applied.

• rebutted — swallowing non-FileExistsError OSErrors is not silent data loss. Merge restore already skip-and-continues a bad crons file and still prints ✅ security / ✅ Merge complete; the per-file restored line prints only when _copy_locked returns True. Raising those errors is the defect the previous GPT round required us to close (test_a_hardlink_failure_does_not_abort_merge). The live create path (_get_telemetry_salt) also uses os.link and falls back to an in-memory salt on OSError, so persist-without-hardlinks is not this PR's contract.

  Catch only FileExistsError; propagate other OSErrors.

[kyleseaman]

• The _TELEMETRY_SALT_BYTES cap is a symptom of read_bytes() — rebutted. A previous GPT round required the in-memory cap after _copy_locked started loading the source; chunk-streaming would drop a pin that already has a failing test (test_an_oversized_source_is_refused_before_publish) and is out of proportion to closing write-then-restrict. Other restore copies (shutil.copy2 of config/crons/memory) are pre-existing and not this PR's secret-writer sites.

  Streaming the copy in chunks removes the hazard and deletes the constant.

• Name conditional is always true / drop src.name == "telemetry_salt" — rebutted. The 32-byte constant is the salt's create-path contract (secrets.token_bytes(32)), not a generic security-file size. A prior GPT round forbade applying that cap to every restored security file; the name check is what keeps the invariant from firing on a later CORE_FILES["security"] member.

  Drop the name conditional at snapshot.py:1123.

• Second dst.exists() is a no-op TOCTOU — rebutted. It is a cheap skip of mkstemp when the dest already appeared; os.link's FileExistsError remains the publish authority. Deleting it does not change the race.

  Delete the second dst.exists() at snapshot.py:1128.

• Pod-config UTF-8 encoding rider undeclared — fixed in the PR body at b1759c03364a116a441134b72d1c427fe6428be5 (atomic_write encodes UTF-8; the previous write_text did not pass encoding=).

  The pod-config encoding change is real shipped behavior the description never states.

[kyleseaman]

• fixed — os.close(fd) then fd = -1 left the fd in finally when close raised, so a second os.close in finally could replace the skip with an escaping OSError and abort merge. Close now drops fd first; finally swallows a leftover close error. Landed at db2e1d89af5d86f97820d7e0b748e89fb7154671 (test_a_failed_close_does_not_abort_merge).

  Close-time writeback error -> caught OSError -> finally re-closes the released descriptor -> merge aborts.

[kyleseaman]

• Print a skipped line when _copy_locked returns False — rebutted. Merge restore already skip-and-continues a bad crons file and still prints ✅ security. A "skipped" line is logging polish, not a merge-abort or secret-window defect. The per-file restored line already prints only on success.

  print/log a "skipped" line in the except path so a persistent restrict_to_owner failure is visible.

[kyleseaman]

• Stream the copy and drop _TELEMETRY_SALT_BYTES — rebutted. A prior GPT round required the 32-byte cap after _copy_locked started loading the archive member; test_an_oversized_source_is_refused_before_publish pins it. Chunk-streaming is a different restore engine, out of proportion to closing write-then-restrict. Wrong-sized salt already self-heals at read; the restore cap is the OOM guard for a planted giant, not a substitute for that reader.

  stream the copy in chunks onto the locked temp fd instead.

• Three os.link siblings still write then chmod_safe — rebutted. Those sites lock the temp then publish (restrict/chmod before os.link); the gate correctly treats that as the fixed shape, which is why they were never in KNOWN_UNCONVERTED. #5346 was the remaining write-then-restrict-at-the-published-name list. install_receipt swallowing a restrict failure is a separate defect, not this PR's debt.

  The harm sentence still holds at 3 unfixed create-only siblings.

• Name-conditional size check is generality for one caller — rebutted. The 32-byte constant is the salt create-path contract. Applying it to every future CORE_FILES["security"] member was the defect a prior GPT round required us to stop.

  The name conditional has one caller ever.

[kyleseaman]

• Print an explicit skipped line when salt restore is skipped — rebutted. _do_merge already skip-and-continues a bad crons file and still prints ✅ security. The per-file restored line prints only when _copy_locked returns True. A skipped log line is operator-polish, not a secret-window or merge-abort defect.

  print an explicit skipped line for the file instead of silence.

[kyleseaman]

• test_a_failed_close_does_not_abort_merge teardown OSError — fixed at 52e11eaa88fa5c7856044d266229cb60510826dc. The test patched os.close (the same module object snapshot.py imports) and raised on every call, so pytest's tmp_path teardown hit the stub. The stub now fires once, which is enough to exercise the skip path.

  ERROR at teardown of test_a_failed_close_does_not_abort_merge: OSError: close: delayed writeback

[kyleseaman]

• The salt create path still uses chmod_safe before os.link — rebutted. #5346 was the remaining write-then-restrict sites in KNOWN_UNCONVERTED. _get_telemetry_salt already locks the temp then publishes; converting that create path is a follow-up, not this PR's leftover-writer list. Widening here would re-review the live mint path this restore was matching, not completing #5346.

  its create path — the same secret, same DACL exposure — ships unfixed on Windows.

• Name-conditional 32-byte skip is generality for one consumer — rebutted. The constant is the salt create-path contract. A prior GPT round forbade applying it to every restored security file.

  A snapshot salt ≠ 32 bytes is silently skipped — one consumer, generalized.

[kyleseaman]

• Silent skip under a success banner — rebutted. Merge restore already skip-and-continues a bad crons file and still prints ✅ security. The per-file restored line prints only when _copy_locked returns True. A skipped log line is operator-polish; it does not reopen a secret window or abort merge. CORE_FILES["security"] is currently only telemetry_salt, which regenerates.

  _copy_locked swallows every OSError and prints nothing, while _do_merge still prints ✅ security.