Skip to content

[security] - Classify gh and apt functional egress, site the macOS and keyring helpers, drop the sandbox punkt fetch - #1646

Merged
cgfixit merged 2 commits into
mainfrom
claude/otel-hardening-round11-2026-10-13
Oct 13, 2026
Merged

cgfixit merged 2 commits into
mainfrom
claude/otel-hardening-round11-2026-10-13

Conversation

@cgfixit

@cgfixit cgfixit commented Oct 13, 2026 •

Copy link
Copy Markdown
Owner

Proposed changes

Follow-up to #1637, which the owner's auto-merge landed at 08:21 ET on 2026-10-13 while Codex's eleventh review pass was still running against its last head. That pass posted four P2 findings on the merged PR; this PR carries the one commit that answers them, cherry-picked onto fresh main (the merged branch is left as it was: no history rewrite). All four findings were verified against the source and confirmed, and a token-level sweep of every in-boundary shell surface found more of the same shape beside them, so the fix is the complete set rather than the four instances.

gh's GitHub traffic is functional egress, not only suppressed telemetry. The setup script's gh auth status / gh auth login --with-token and the agentic gh pr view / gh pr create argv were sited only under the category-1 GH_TELEMETRY row, so the inventory reported the auth flow and the PR operations purely as telemetry that is switched off. New category-3 component gh-api with its own row and sites, beside the telemetry row, the git / git-remote precedent. SECURITY.md's category-3 sentence names it. verify.sh scenario 48 removes both gh auth login branches and expects the WARN on the new row.

The generated Task Scheduler rows pinned a filename and a which(). utils/win_schtasks.py's cmd.exe row matched only the .cmd filename construction and the powershell row only shutil.which("powershell"), so replacing either generated child command left strict T13 green. cmd.exe now pins command=os.environ.get("COMSPEC", "cmd.exe") and the /c <launcher> argument; powershell pins the powershell/pwsh resolver, the ps = powershell or powershell_executable() selection and the -File wrapper_path argv. COMSPEC is the OS-owned variable naming where the same interpreter lives, the UV_BIN shape the owner ruled on in #1637, so it stays a fixed component rather than a dynamic row. Scenario 49 swaps the construction for a literal and expects the WARN.

The macOS setup helpers and the Linux keyring CLI had no rows. Codex named /usr/bin/expect, osascript, pbcopy/pbpaste and the CYCLAW_SECRET_TOOL path. Rather than site those four, I swept the first word of every command position (after if, !, command -v, pipes, &&, $( and leading VAR=value assignments) in every in-boundary .sh, the Dockerfile's RUN lines and the compose file, minus builtins, coreutils and each file's own functions. That also surfaced lsof, sw_vers, and the Dockerfile's apt-get, groupadd and useradd. The local tools join the category-4 local-OS-tooling row with invocation-literal sites (expect on the heredoc and its spawn -noecho /usr/bin/security add-generic-password; osascript on both AppleScript runs; pbcopy/pbpaste on the copy, the readback and the clear; lsof on the listener lookup in setup-keys and uninstall; sw_vers -productVersion in three scripts; secret-tool on command -v, lookup, store and clear). apt-get is a new category-3 row apt-install (Debian mirror access at image-build time, version-pinned, nothing at runtime). CYCLAW_SECRET_TOOL is a path override of the same binary, so secret-tool is fixed, not dynamic. Scenarios 50 and 51 remove the expect invocation and the keyring lookup. SKILL.md step 5 records the token-level method and the path-override rule so the next sweep starts from the full list.

The sandbox audit fetched NLTK punkt on every run. .claude/skills/CyClaw-Sandbox/verify.sh (inside the swept boundary) ran nltk.download('punkt') and nltk.download('punkt_tab') unconditionally, which reaches the NLTK data host on a cold cache. The stemmer uses PorterStemmer only, SECURITY.md lists any nltk.download() as a security regression, and the core-libs row's evidence says punkt is avoided by design, so the step was removed rather than classified; the row records the removal. The .codex/skills/Cyclaw-Sandbox/verify.sh mirror has the same line; it is agent tooling outside the inventory boundary the owner drew in #1637 and was not touched here.

Invariant / Governance Impact: none of the six invariants or I6 are touched. The diff is the checker's inventory data (two new rows, nineteen new site rows, eleven new component names), its self-test (four scenarios), the skill text, one sentence of SECURITY.md's egress classification, and one obsolete line removed from the sandbox audit script. No production map, kill value or scrub name changed; CONTRACT_DIGEST is unaffected; no core file moved.

Agent-instruction surface (the pre-commit security gate flagged this, correctly): the skill files steer later agent sessions. Every row's text is my own paraphrase of what the cited file does; no web, issue or tool text was copied in. The gate's secure-by-design question about a new config input points at the COMSPEC and CYCLAW_SECRET_TOOL mentions in the checker and self-test: both are documented in rows as path overrides read by existing scripts, and the checker itself reads neither.


Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Security (auth, privacy, injection, egress, redaction, or secrets)
  • Invariant / Governance refinement (use this for changes that strengthen or evolve the 6 invariants, I6 isolation, or harness phases)
  • Dependencies (added, removed, or bumped)
  • CI / infrastructure (workflows, hooks, templates, or checkers)
  • Refactor or tests (behavior-preserving, no new capability)
  • Documentation update
  • Other (none of the boxes above fit)

Scope note: Infrastructure / CI (the verify-skills checker's egress inventory and its self-test), one sentence of SECURITY.md, and one obsolete download line removed from the sandbox audit script; no server runtime path, no operator script changed.


Benefits / why

  • The classification is now honest about kind for gh: an operator reading the inventory sees that the setup script talks to github.com on purpose and that the agentic layer can open pull requests when enabled, next to the separate fact that gh's telemetry and update checks are switched off.
  • Every helper binary the macOS setup and uninstall scripts run, and the Linux keyring CLI, can now go stale visibly: a renamed or removed invocation trips the fixed-site WARN. The token-level sweep that found them is written into the skill so the next run does not depend on a reviewer naming binaries one at a time.
  • The container build's package install is filed as the functional egress it is, consistent with the pip and uv rows from [security] - Classify the Keychain CLI and uv in the telemetry egress inventory #1637.
  • The sandbox audit no longer makes an unnecessary network fetch that the security assessment explicitly calls a regression.
  • The Task Scheduler rows pin what actually launches the generated jobs, so a change to the job's command or interpreter selection is caught.

Risks to monitor

  • No server runtime risk: nothing under utils/ (only the checker's view of utils/win_schtasks.py changed), gate*.py, graph.py, or the launchers the server depends on changed. A regression in the checker is a reporting regression, not a telemetry leak.
  • Marker brittleness grows again. Nineteen more fixed-site rows, several in the macOS setup scripts that are edited by hand. An innocent reflow of a lsof, pbcopy or osascript line trips a T13 WARN that must be re-pointed. Signal: verify-skills (otel-hardening) going yellow on an unrelated PR.
  • The sandbox audit lost a step. If anything in the sandbox suite did depend on punkt, its next run fails at that point rather than silently downloading. The runtime stemmer does not, and the suite's own tests mock retrieval, so I expect no effect; CI's verify-skills (CyClaw-Sandbox) leg is the check.
  • Two verify.sh copies now differ. .codex/skills/Cyclaw-Sandbox/verify.sh still has the punkt line; it is outside the boundary by the owner's decision, and this PR says so rather than editing a Codex-agent file. If the owner wants the mirror cleaned too, that is a one-line follow-up.
  • verify-skills is continue-on-error in ci.yml, so a red check_otel.py leg would not block a merge. Pre-existing; the green reported below is the only gate this PR actually has.

Checklist

  • I have read docs/THREAT_MODEL.md, INVARIANTS.md and SECURITY.md (and any relevant Phase docs)
  • This change preserves all 6 security invariants and I6 module isolation (explicit evidence or invariant matrix included for core changes)
  • Full sandbox validation has been run (GROK_API_KEY=dummy pytest tests/ -q --tb=short, and bash .claude/skills/CyClaw-Sandbox/verify.sh for core RAG/agentic paths) and passes with no regressions — deliberately not run, per CLAUDE.md §5's verification policy; CI runs the suites. The checks that exercise this diff were run directly and are listed under Further comments.
  • No new external network dependencies or mandatory online LLM assumptions were introduced without explicit justification + offline fallback path
  • For any agentic/fsconnect change: two-phase audit, quota enforcement, governed delete/trash, and write guards have been verified — n/a
  • Relevant architecture docs or threat model notes have been updated if core behavior or topology changed — n/a for core; the skill's SKILL.md and SECURITY.md's egress classification updated
  • Commit messages follow the title prefix convention above
  • For large or complex changes: before/after invariant matrix + sandbox evidence is included in "Further comments" or linked — not large

Further comments

Checks run directly on the pushed head (all green):

  • python3 .claude/skills/otel-hardening/check_otel.py --strict --as-of 2026-10-13 → 0 failures, 0 warnings; 44 inventory rows, 95 declared components resolve, 89 fixed launcher sites (39 binaries) and 4 dynamic sites carry their invocation markers.
  • bash .claude/skills/otel-hardening/verify.sh → ALL PASS (55 mutation scenarios; 48–51 are new). Run on the identical file set before the cherry-pick onto main, whose only additional change since the merged head is two documentation files.
  • python3 .claude/skills/doc-sync/doc_sync.py → 0 drift items, re-run on the new branch.
  • ruff check --select E,F,I,B,C4,UP,S --ignore E501 on the checker → clean; bash -n on both verify.sh files; bash scripts/check-pr-template.sh on this body → OK.
  • The token-level sweep described above, over macos/*.sh, scripts/*.sh, windows/, the Dockerfile, docker-compose.yml, cyclaw-gotchas/driver.sh and CyClaw-Sandbox/*.sh, plus a curated grep for forty common macOS/Linux tool names over the same surfaces and the PowerShell and Python files beside them.

Only CI has run: the pytest suites and the 80% coverage gate (this diff adds no Python under coverage sources), and the verify-skills (CyClaw-Sandbox) leg that exercises the sandbox script the punkt line was removed from.

Not run, and why: the four telemetry pytest files the skill's step 4 names; they pin utils/telemetry_kill.py's maps, the Dockerfile/compose delivery and the ONNX seam, none of which this PR touches. No operator script changed, so none was executed.

Review threads: the four Codex threads this PR answers live on the merged #1637; each has a reply pointing here and is resolved there.

Body layout note: pr-template-check.yml requires the Last updated stamp to be the body's last non-blank line, so the Claude Code attribution block sits just above it rather than after it.


Suggested merge order of open PRs

Trial merge verified: the branch is one commit on top of main @ b3771da (cherry-picked clean on 2026-10-13); nothing else open touches these files.

This PR · Safe ✅
Impact: Checker inventory and self-test, skill text, one SECURITY.md sentence, one line removed from the sandbox audit script; no server runtime path. Independent, only open PR.
Action: Merge when CI is green.

ELI5

The big guest-list PR (#1637) merged while the reviewer bot was still looking at it, and its last look found four more things. Instead of opening the merged PR back up, this small one carries the answer.

First, gh logging into GitHub and opening pull requests is real GitHub traffic the project does on purpose, so it now has its own "on purpose" entry next to the existing "we switched its telemetry off" entry, the same way git clone got its own entry last time. Second, the entries for the generated Windows scheduled tasks pointed at the file name and a lookup rather than the line that actually builds the command, so they have been re-pointed. Third, the Mac setup scripts run a handful of small helper programs (one that types a password into the Keychain prompt, one that fills the browser's key box, the clipboard, the port-checker, the version reader) and the Linux build uses a keyring tool; none of them were on the list. Rather than add only the ones the bot named, I listed every command word in every setup script and found a few more the same way, including the container build's package installer. Fourth, the sandbox test script was still downloading a language-toolkit file the project stopped needing long ago, which the security notes call a regression; that line is gone.

What could still go wrong: more entries means more chances an innocent edit to a setup script makes the checker grumble until someone re-points a line. The sandbox script lost a step it should not have needed; if it turns out it did, the sandbox CI leg says so. Nothing in the running server changed.

🤖 Generated with Claude Code

https://claude.ai/code/session_012semJHbJjTtdTy3zrhUnR3

Last updated: 2026-10-13 05:00 ET

…d keyring helpers, drop the sandbox punkt fetch

Codex round eleven on #1637, all four findings confirmed, plus what a
token-level sweep of every in-boundary shell surface turned up beside them:

- gh's GitHub traffic is functional egress, not only suppressed telemetry:
  new category-3 component `gh-api` sited on the setup script's
  `gh auth status` / `gh auth login --with-token` and the agentic
  `gh pr view` / `gh pr create` argv, beside the category-1 telemetry row
  (the git / git-remote precedent). SECURITY.md's category-3 sentence names
  it. Scenario 48 removes both auth-login branches and expects the WARN.
- The generated Task Scheduler rows pinned a filename and a which():
  cmd.exe now pins the COMSPEC command construction and the `/c <launcher>`
  argument, powershell the executable resolver and the `-File wrapper`
  argv. COMSPEC is a path override of the same interpreter (the UV_BIN
  shape), so it stays a fixed component. Scenario 49 swaps the COMSPEC
  construction and expects the WARN.
- macOS setup/uninstall helpers and the Linux keyring CLI had no rows:
  expect, osascript, pbcopy, pbpaste, lsof, sw_vers and secret-tool join the
  local OS tooling row with invocation sites; CYCLAW_SECRET_TOOL is a path
  override, not a caller-chosen command. The same sweep found the
  Dockerfile's apt-get (new category-3 row `apt-install`) and its
  groupadd/useradd (local tooling). Scenarios 50 and 51 remove the expect
  invocation and the secret-tool lookup.
- The sandbox audit's `nltk.download('punkt')` step was obsolete (the
  stemmer uses PorterStemmer only, no punkt, per SECURITY.md) and would
  have reached the NLTK data host on a cold cache: removed from
  .claude/skills/CyClaw-Sandbox/verify.sh; the core-libs row says so.
- SKILL.md step 5 records the token-level sweep method and the
  path-override rule so the next round starts from a complete list.

Checks: strict checker 0/0 (44 rows, 95 components, 89 sites / 39
binaries, 4 dynamic sites), verify.sh ALL PASS (55), doc-sync 0 drift,
ruff clean, bash -n on both verify scripts.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012semJHbJjTtdTy3zrhUnR3
@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

PR is a complete fill of .github/PULL_REQUEST_TEMPLATE.md (every section, ticked boxes, merge order, ELI5 last, last-updated stamp, title, branch).

@cgfixit
cgfixit marked this pull request as ready for review October 13, 2026 12:41
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-13T12:47:10.184297Z 0cf3fce Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cgfixit
cgfixit merged commit 5a5e8a4 into main Oct 13, 2026
48 of 49 checks passed
@cgfixit
cgfixit deleted the claude/otel-hardening-round11-2026-10-13 branch October 13, 2026 12:41

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0cf3fced6f

ℹ️ 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".

Comment on lines +171 to +175
The shell sweep is token-level, not name-level: take the first word of
every command position (after `if`, `!`, `command -v`, `|`, `&&`, `$(`
and leading `VAR=value` assignments) in every in-boundary `.sh`, the
Dockerfile's `RUN` lines and the compose file, drop shell builtins,
coreutils and the file's own functions, and classify what is left.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Implement the claimed token-level shell sweep

When an in-boundary shell script starts a new executable, T13 never parses that script as described here; it only checks manifests, the hard-coded KNOWN_EXTERNAL_COMPONENTS, and markers for already-declared sites. In an isolated checkout, adding wget https://example.invalid/probe to scripts/ensure-githooks.sh still made check_otel.py --strict exit 0 with zero warnings, so new unclassified network egress can silently pass the advertised strict classification check. Add the token-level discovery step (and an add-new-binary mutation test), or remove this coverage claim.

AGENTS.md reference: AGENTS.md:L49-L49

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants