[Recovery] Surface self-hosted runner bootstrap tooling - #3261
Chris0Jeky wants to merge 5 commits into
Conversation
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Fresh-context security review: FIX-FIRST at 6027ef33192c3a220e01ca487dfc22e4f37fd069.
Blocking: the Windows cleanup hook does not enforce the documented nested-reparse contract
README.md promises that the cleanup hooks reject any symlink, junction, or reparse point in a governed root or ancestor. cleanup-windows.ps1 validates the governed root/ancestors, then Clear-Children inspects only each immediate child for ReparsePoint before calling Remove-Item -Recurse -Force on an ordinary child directory. A nested junction/symlink below that first directory level is therefore neither detected nor rejected by the hook.
The static contract test only checks that the source contains the text child_reparse; it does not execute a filesystem fixture and cannot prove recursive deletion is non-following across the supported PowerShell/Windows version. This is a safety-boundary claim, so relying on undocumented or version-sensitive deletion behavior is not adequate.
Before porting this stale branch onto current main:
- enumerate the complete descendant tree without following links and fail closed if any descendant carries
FileAttributes.ReparsePoint, immediately before deletion; - add a real Windows regression with an ordinary top-level directory containing a nested junction/symlink to an external sentinel directory; require a nonzero fail-closed result and prove the sentinel is untouched;
- either enforce the same stated nested-link contract on Linux with an executable fixture, or narrow the documentation to the exact boundary actually guaranteed;
- rebase/port the corrected slice onto current
mainand run the current Smart-CI/control-plane contracts plus Windows PowerShell qualification.
The branch also changes R4 control paths, so even after correction it remains behind the unresolved ADR-0066 J.3/J.4 control-plane review gate. Do not merge this stale branch wholesale.
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: 6027ef3319
ℹ️ 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".
6027ef3 to
6079ed5
Compare
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review Please review exact current head 3dc8c2c against base e09f10f. The fix rejects nested and root reparse points before cleanup traversal, rejects runner ChangePermissions/TakeOwnership ACLs in both Windows bootstrap and cleanup validation, and extends the contract assertions. Local runner contract proof is 14/14; PowerShell parsing passes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dc8c2c431
ℹ️ 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".
| } | ||
| catch { | ||
| Stop-Contract 'account_group_unverifiable' |
There was a problem hiding this comment.
When the pre-created runner account has a direct user-right assignment such as SeTakeOwnershipPrivilege or SeRestorePrivilege, it can enable that privilege and take ownership of or rewrite the protected policy and hook even though its only local-group membership is Builtin Users. Fresh evidence at this head is that validation still stops at the group-membership check, so rejecting TakeOwnership and ChangePermissions ACL bits does not close this privilege path; validate and reject dangerous user-right assignments before accepting the account.
Useful? React with 👍 / 👎.
| if [[ "$DRY_RUN" -eq 0 && "$count" -gt 0 ]]; then | ||
| find "$candidate" -mindepth 1 -maxdepth 1 -exec rm --one-file-system -rf -- {} + \ | ||
| >/dev/null 2>&1 || fail 'clear_failed' |
There was a problem hiding this comment.
When a job leaves a FUSE/bind/host mount as an immediate child of a cleanup root, this passes that mountpoint as its own rm operand and can delete the mounted filesystem's contents before failing to remove the busy mountpoint. rm --help defines --one-file-system relative to each corresponding command-line argument, so it does not protect a mounted child that is itself an argument; inspect the root for submounts and fail before deleting anything, or traverse from the validated root with a filesystem boundary.
Useful? React with 👍 / 👎.
|
Estate ops disposition (eng supervisor bot) — 2026-09-23 Park / do not deepen Recovery. Own FIX-FIRST review already blocks merge: Windows cleanup only inspects immediate children for reparse points, then recursively deletes — nested junction can escape the governed root; regex contract does not prove an external sentinel is safe. Guidance
No merge from this bot. No cloud agents. |
This branch has not been deployed
1 participant
Why this draft exists
The stale-branch audit found
issue-2328/runner-bootstrapwith no pull request. It contains cross-platform bootstrap and cleanup tooling plus runner documentation, but it is now far behind the active CI architecture.Recovered scope
Review status — FIX-FIRST
Fresh-context security review at head
6027ef33192c3a220e01ca487dfc22e4f37fd069found a blocking contract gap:Before this work is ported, the Windows hook must reject nested reparse descendants immediately before deletion and gain a real Windows regression with an external sentinel. Linux must either prove the same documented contract or the documentation must be narrowed to the boundary actually enforced.
Disposition
Do not merge this stale branch wholesale. Port a corrected, minimal slice onto current
main, run current Linux and Windows contract qualification, and keep the successor parked behind the ADR-0066 J.3/J.4 R4 control-plane review gate.Refs #2328
Related audit: #3256