D62 — Authenticated post-start Autoflow control channel - #84
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DBynXdqPS9ZzfXshWHTyaD
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b00c91c52
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| // 3. Anchor ACL (read-only). | ||
| const icacls = await runProcess(icaclsPath(systemRoot), [anchorPath]); |
There was a problem hiding this comment.
When the anchor is owned by another local account but its current DACL lists only the runtime operator and SYSTEM, this verification succeeds because the plain icacls <anchor> output is used only to inspect ACEs and the owner is never queried. A Windows object owner can change its DACL, so that account can subsequently grant itself access, read the descriptor token, and authenticate to the control channel; verify that the owner is also the runtime operator or SYSTEM before accepting the anchor.
Useful? React with 👍 / 👎.
| pipePath, | ||
| close: (): Promise<void> => | ||
| new Promise<void>((resolvePromise) => { | ||
| removeDescriptorFile(anchorPath, deps.descriptorDeps); |
There was a problem hiding this comment.
When two runtime instances overlap, the newer instance replaces the fixed descriptor file, but shutting down the older instance unconditionally removes that newer descriptor here. The newer pipe remains live yet the official CLI can no longer discover it; this also occurs during a normal start-before-stop restart. Associate cleanup with the descriptor this handle published and avoid removing the file if it now identifies another process/channel.
Useful? React with 👍 / 👎.
| if (/processed\s+\d+\s+files/i.test(line)) { | ||
| sawSummary = true; | ||
| continue; |
There was a problem hiding this comment.
On a non-English Windows installation, icacls localizes its success-summary text, so this English-only regex does not recognize the footer. The footer is then passed to parseAce, which returns null, causing every otherwise valid anchor to be rejected as ACL_UNREADABLE and disabling the control channel. Determine command success from the already-checked exit status or parse the output without requiring an English sentence.
Useful? React with 👍 / 👎.
Decision 062 Amendment A (PR #84 F1). The control-anchor gate verified the DACL but not the OWNER SID, so a foreign owner could rewrite the DACL, grant itself access, and read the descriptor token. verifyControlAnchor now also requires the anchor OWNER SID to equal the exact runtime operator SID (SYSTEM is allowed as a DACL principal, never as owner), fail-closed. The owner SID is read by a single source-in-repo native helper built from reviewed C by a trusted Windows build; its identity and SHA-256 are generated build metadata (a built JS artifact, not a committed literal or .sha256 sidecar) and the helper bytes are hash-verified before it is executed via the existing bounded, shell-free runner. This adds one read-only executable (whoami, icacls, owner helper) and no more. F2 (control-runtime.ts) and F3 (icacls parser) are unchanged. Full suite 2010/2010; typecheck/lint/build/diff-check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GC2pVoLmjUX5FPLq4urhJT
The real Windows owner-helper positive test assumed mkdtempSync(tmpdir()) yields an operator-owned directory (owner SID == whoami SID). That holds in a non-elevated context but not on GitHub's elevated Windows runner, where a freshly created directory is owned by Administrators (or SYSTEM) — a non-operator SID — so production correctly fails closed and the ok:true assertion breaks. The test now reads the temp directory's ACTUAL owner SID via the same real, build-provenanced helper binary, then asserts the corresponding deterministic gate behaviour: owner == operator must accept and echo that SID; a non-operator owner must fail closed with the exact reason (OWNER_IS_SYSTEM for SYSTEM, OWNER_MISMATCH otherwise). Meaningful in both elevated and non-elevated contexts, and never treats a foreign owner as success. Test-only. Production (ownerSid === operatorSid gate) unchanged; F1/F2/F3 and the six protected files byte-identical. Full suite 2010/2010; typecheck/lint/build/diff-check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PPAMmQpdQbunE8kGbezgih
…context test(control): make owner helper integration context-aware
The control runtime unlinked the fixed runtime-descriptor.json unconditionally in close() and in the listen-failure cleanup. Under an overlapped start (runtime B rotates and publishes its descriptor before runtime A closes), A's cleanup deleted B's descriptor, leaving B's live pipe undiscoverable by the official CLI. Availability/correctness only; no authority or token exposure. Cleanup is now bound to the runtime instance identity: the descriptor is read and validated through the existing trusted parser and unlinked only on an exact pipeName match (the per-process 128-bit-random identity; a pid can be reused, a pipeName cannot). A missing, malformed, unreadable, or successor-owned descriptor is left untouched, so cleanup fails safe for the successor in both lifecycle locations. The startup rotation of a stale crash descriptor is intentionally unchanged. Read-compare-unlink is not atomic; the residual race narrows from the successor's whole lifetime to the sub-millisecond match-to-unlink gap and is not closable with JS fs primitives. Adds deterministic adversarial tests: A/B overlap survival, missing, malformed, and unreadable descriptors, same-pid foreign descriptor, and listen-failure protection of a foreign descriptor alongside own-cleanup. F1/F3 and the protected files byte-identical. tests/control 126/126 (real owner-helper binary included), full suite 2017/2017; typecheck/lint/build/diff-check clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ZN6c4sZ6k4RSNigL5CWxG
…ship fix(control): preserve successor runtime descriptor
Decision 062 Amendment B (PR #85 F3): replace the localized icacls DACL read with a single build-provenanced native OWNER + DACL security-descriptor snapshot emitted as canonical SIDs only, so control-anchor authorization is identical on any Windows locale. SYSTEM is recognized solely as S-1-5-18 and the operator by exact canonical SID; a display name can never enter the decision. F1 owner-only mode and its provenance/hash gate are preserved; F2 is untouched. The deferred P3 (operator effective-rights type/mask semantics) is intentionally out of scope and not repaired here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KE5bkGWp2jPzaJaLvFhPPu
fix(control): make anchor ACL verification locale-independent
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wunzfbkt5fnNjTMRKtr9Jo
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H47bxbCApT9gJMxkTMkBhf
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H47bxbCApT9gJMxkTMkBhf
…prerequisite PR93 P2 — document hardened control anchor prerequisite
…-provision PR92 P2 — add explicit control provisioning step
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013y7H4Sgp9MxM9WQG4yTBH8
…tection PR92 P2 — detect required MSVC workload
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013y7H4Sgp9MxM9WQG4yTBH8
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0147CEMLP35K39BzWMhPoEgg
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JqXwUv8Aw55oJMKYgBHLJ
…fecycle D062 — coherent control launch lifecycle reconstruction
…ion) into feature/d062-post-start-control-channel
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30fc85e195
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ace.inherited) { | ||
| return { ok: false, reason: CONTROL_ANCHOR_REJECTION.INHERITED_PRINCIPAL }; | ||
| } |
There was a problem hiding this comment.
When the anchor has direct operator/SYSTEM ACEs without OBJECT_INHERIT_ACE, this check accepts it, but the newly created descriptor does not inherit those restrictions and instead receives the process token's default DACL; mode: 0o600 does not establish an equivalent Windows DACL. If that default DACL includes another local principal, that account can read the token and authenticate to the control pipe. Capture the ACE propagation flags and require suitable file inheritance, or explicitly create and verify a restricted descriptor DACL.
Useful? React with 👍 / 👎.
| /** A canonical OWNER + DACL snapshot as parsed from the native `--acl` helper. */ | ||
| export interface AclSnapshot { | ||
| readonly ownerSid: string; | ||
| /** `false` iff the object has a NULL DACL (grants everyone — fail closed). */ | ||
| readonly daclPresent: boolean; | ||
| readonly aces: readonly AclSnapshotAce[]; |
There was a problem hiding this comment.
The snapshot records whether the DACL is present and which ACEs it currently contains, but not the SE_DACL_PROTECTED control bit. An anchor with direct operator/SYSTEM ACEs but inheritance still enabled therefore passes; if an account able to modify an unverified parent later adds an inheritable ACE, Windows can propagate it into the anchor after startup, allowing that account to read the descriptor token while the server continues using it. Include and validate the protected-DACL state before accepting the anchor.
Useful? React with 👍 / 👎.
1 participant
Decision 062 implementation.
Authority:
Security boundary:
Validation before commit:
Exact validated candidate:
Deployment remains separately gated.
No ACL provisioning or runtime cutover is part of this PR.