D062 — close ACL propagation and descriptor lifecycle gaps - #96
D062 — close ACL propagation and descriptor lifecycle gaps#96LogicDuke wants to merge 6 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (15)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe control channel now uses a V2 ACL snapshot protocol, a separately provenanced native descriptor creator, exclusive descriptor creation, and post-creation ACL verification before channel startup. ChangesControl descriptor security
Priority: ➖ Normal Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant startControlChannel
participant descriptorCreator
participant descriptorFile
participant descriptorVerifier
participant controlPipe
startControlChannel->>descriptorCreator: Remove stale file and create descriptor
descriptorCreator->>descriptorFile: Write descriptor with protected ACL
startControlChannel->>descriptorVerifier: Verify descriptor ACL
descriptorVerifier-->>startControlChannel: Return verification result
startControlChannel->>controlPipe: Build and listen after success
Merge Risk: ⚪ Minimal · up to The descriptor lifecycle and ACL hardening are consistently implemented across creation, verification, startup, provenance, and tests; no merge-blocking issue is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. A rabbit reads each line, Comment |
Windows chooses a newly created file's OWNER from the creating token's DEFAULT owner and its DACL from the parent's inheritable ACEs, so a `writeFileSync` descriptor is owned by BUILTIN\Administrators under an elevated token and carries an unprotected, inherited DACL. The exact-owner descriptor gate correctly rejected it, leaving the control channel unavailable on a valid anchor. Add a second build-provenanced native artifact with create-only authority, agentbridge-win-descriptor-create.exe, which creates exactly <anchor>\runtime-descriptor.json with CREATE_NEW and an explicit security descriptor: owner = its own process TokenUser SID, and a PROTECTED DACL whose principals are exactly that operator plus SYSTEM. It derives the operator itself, accepts no owner or filename input, receives the secret descriptor bytes only on stdin, and is SHA-256 verified against its own generated provenance before execution. The creator and the read-only owner helper have separate filenames, separate provenance modules, and separate exported bindings, so neither artifact's trust root can satisfy the other's. Creation is never trusted on its own word: the existing READ-ONLY helper then inspects the file that actually exists, and evaluateDescriptorSnapshot must accept it before any pipe is created. That evaluator now also requires a PROTECTED DACL and rejects any INHERITED_ACE, so a descriptor that could later widen through parent inheritance is refused at verification time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PxNRavn8EqdoiimAKQrcnT
The D062 createDescriptorFileNative writeFile-seam test hardcoded a Windows separator when forming the expected descriptor path, while production `descriptorPathFor()` uses the platform-aware `join()` from `node:path`. On ubuntu-latest the expectation demanded `C:\Anchor\runtime-descriptor.json` while production correctly produced `C:\Anchor/runtime-descriptor.json`, failing the Ubuntu verify job. Build the expectation with the same `join()` the production path helper uses, so expected and actual are separator-identical by construction on every platform. Test-only: production path semantics are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016SnRhGj1dW4KiYsviKo9KL
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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: 0be6e5661a
ℹ️ 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 (before === null) { | ||
| // Already provisioned with VALID pairs: do not recompile on every launch. The | ||
| // runtime still hashes each binary against its generated provenance before use. | ||
| note('valid helper/provenance pairs already present; skipping build.'); | ||
| process.exit(0); |
There was a problem hiding this comment.
When dist survives a rollback or mixed-cache restore, a canonical V1 owner-helper pair can coexist with the new canonical creator pair, causing firstInvalidPair() to return null here. The runtime now accepts only AGENTBRIDGE-ACL-V2, so the old helper's output is rejected as SNAPSHOT_MALFORMED, while every subsequent control:provision invocation reports success and skips the rebuild. Include a helper protocol/source version in pair validity, or retain parser compatibility, so provisioning can repair this state.
Useful? React with 👍 / 👎.
| descriptorAcl = await verifyDescriptor(descriptorPathFor(anchorPath), { env }); | ||
| } catch { | ||
| log('AgentBridge control channel: disabled (descriptor write failed).'); | ||
| removeDescriptorFile(anchorPath, deps.descriptorDeps); |
There was a problem hiding this comment.
When two runtimes overlap, a successor can remove and recreate the fixed descriptor while the first runtime's ACL query is failing; this unconditional unlink can then delete the successor's newly created descriptor and make its live channel undiscoverable. The later listen-failure and close paths already avoid this by calling removeOwnDescriptorFile with the minted pipe name, and both descriptor-verification failure paths need the same ownership check.
Useful? React with 👍 / 👎.
| if (!snapshot.daclProtected) { | ||
| return { ok: false, reason: CONTROL_ANCHOR_REJECTION.DACL_UNPROTECTED }; |
There was a problem hiding this comment.
This newly rejects an unprotected anchor DACL (and below also rejects every ACE lacking OBJECT_INHERIT_ACE), but docs/architecture/D3-cockpit-dashboard-host.md still tells operators that an operator+SYSTEM, present, non-inherited DACL is sufficient. Because the repository deliberately has no anchor provisioner, an operator can follow the only documented setup procedure and still have control startup fail closed; document the protected and file-inheritable requirements alongside the external prerequisite.
Useful? React with 👍 / 👎.
Three CURRENT/P2 findings on the exact PR #96 head, repaired coherently. F1 — provisioning could skip a stale helper forever. Lifecycle pair validity tested only self-consistency: on-disk provenance == encode(sha256(binary)). A canonical-but-older owner-helper surviving a rollback or mixed-cache restore therefore satisfied the gate, which reported success on every run, while the runtime rejected that helper's output as SNAPSHOT_MALFORMED because the parser requires AGENTBRIDGE-ACL-V2. Bind the reviewed build source into the canonical identity: each provenance module now names both the binary's SHA-256 and sourceId, the SHA-256 of the exact C source the build compiled. The accepted identity is the pair (binary, source), so lifecycle validity implies compatibility with the runtime's current snapshot protocol — whose only definition is that source. Nothing is read out of the on-disk provenance to decide this; the source digest is recomputed from the repository, so a stale helper never votes on its own currency. No regex, no version-text parser, no execution of the on-disk binary. provenance-format.mjs stays pure (sourceId is a parameter); helper-pair.mjs owns the one filesystem read and the one artifact->source mapping; build.mjs imports that same function, so producer and acceptor cannot drift. The runtime reads only filename and sha256 and ignores other fields, so this adds no runtime authority and leaves hash-before-exec unchanged. Termination is one rebuild: the builder publishes exactly what the gate recomputes. F2 — a successor runtime's descriptor could be deleted. Both descriptor-ACL verification failure paths unlinked the fixed pathname unconditionally, so a predecessor whose verification failed removed a successor's freshly created descriptor and made its live channel undiscoverable. Both paths now use the ownership-checked removeOwnDescriptorFile with the minted pipeName, exactly as the listen-failure and close() paths already did. Startup still fails closed. F3 — the documented anchor contract understated the code. The D3 host document described the DACL as operator + SYSTEM, present and non-inherited, but the evaluator also rejects an unprotected DACL and any ACE lacking OBJECT_INHERIT_ACE, so an operator could follow the only documented procedure and still fail closed. Replace that sentence with the full requirement/rejection table, verified against evaluateAnchorSnapshot, including that SYSTEM is permitted rather than required and that allow/deny type and access mask do not gate authorization. Reviewed C sources, msvc-toolchain.mjs and control-store.ts are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016SnRhGj1dW4KiYsviKo9KL
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b73b4a6fd
ℹ️ 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".
| let descriptorAcl: DescriptorAclVerification; | ||
| try { | ||
| writeDescriptorFile(anchorPath, descriptor, deps.descriptorDeps); | ||
| descriptorAcl = await verifyDescriptor(descriptorPathFor(anchorPath), { env }); |
There was a problem hiding this comment.
When runtimes overlap, a successor can replace the fixed descriptor while this asynchronous verification is in flight; the helper then validates the successor's descriptor and returns ok, but the predecessor proceeds to listen on its own pipe and reports successful startup even though its token and pipe are no longer discoverable. If the successor subsequently closes or fails to listen, no descriptor remains for the predecessor's still-running channel. After a successful ACL result, re-read the descriptor and fail startup unless it still contains this runtime's minted pipeName.
Useful? React with 👍 / 👎.
Descriptor ACL verification is asynchronous and path-based: the helper re-opens whatever file sits at the fixed descriptor path when it runs. A successor runtime removes that pathname unconditionally before creating its own, so during the await the helper could validate the SUCCESSOR's descriptor and return ok. The predecessor then listened on its own pipe and returned a handle reporting a successful startup, while the stored descriptor named the successor: the channel was live but undiscoverable, and once the successor closed, no descriptor remained for it at all. Prove ownership before listening. After a successful ACL result the runtime re-reads the current descriptor and proceeds only while it still carries this instance's minted pipeName; otherwise startup fails closed with no handle and no token. The ownership test is factored out of removeOwnDescriptorFile into a single descriptorIsOurs predicate, so the cleanup paths and this pre-listen proof decide through exactly one function and cannot drift. A missing, malformed or unreadable descriptor is not ours: ownership is proven, never assumed. Nothing is removed on the new path, deliberately. It is reachable only when the descriptor is absent, unprovable, or a successor's, and deleting any of those is precisely the successor-clobbering this ownership rule exists to prevent; a later runtime's stale-descriptor removal reclaims an orphaned pathname. This is distinct from the failure-path repair already on this branch: that governed cleanup after verification failed, this governs proceeding after it succeeded. Removing only the new check fails six of the eight added tests, which cover the successor rotation, a missing, malformed, unreadable or foreign descriptor, successor preservation, and the absence of any token in the failure log. No locks, retries, polling or global state; the descriptor store is unchanged and fail-closed startup is not weakened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016SnRhGj1dW4KiYsviKo9KL
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64845dedb8
ℹ️ 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 (!descriptorIsOurs(anchorPath, descriptor.pipeName, deps.descriptorDeps)) { | ||
| log('AgentBridge control channel: disabled (descriptor no longer identifies this runtime).'); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
When a successor rotates the descriptor after this check but before the asynchronous server.listen callback resolves, this runtime still returns a successful handle even though the fixed descriptor already names the successor, leaving this channel undiscoverable. The fresh evidence beyond the prior review is that the new ownership check remains before the separate listen await at lines 212–222; recheck after listening and close/fail startup if ownership was lost.
Useful? React with 👍 / 👎.
1 participant
Repairs the exact CURRENT Windows ACL/descriptor findings identified on PR #84.
Base / protected parent:
Repair head:
Findings closed:
Terminal invariants:
Validated patch:
Validation:
Authority:
This PR is intentionally Draft for exact-head CI and independent review.
Do not merge automatically.
Human merge authority remains required.
Summary by CodeRabbit
Security Enhancements
Reliability
Validation