close
Skip to content

fix: accept the user's own gh/glab CLI install - #630

Merged
iamwhatever merged 1 commit into
mainfrom
fix/gh-cli-user-owned-trust
Jul 28, 2026
Merged

fix: accept the user's own gh/glab CLI install#630
iamwhatever merged 1 commit into
mainfrom
fix/gh-cli-user-owned-trust

Conversation

@dwu96

@dwu96 dwu96 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Problem

The Sidebar PR/MR panel and Issue Radar refuse any gh/glab whose binary and every parent directory is not root-owned and unwritable by the gateway user (_validate_provider_executable). A stock Homebrew install can never satisfy that:

/opt/homebrew/bin/gh -> ../Cellar/gh/2.96.0/bin/gh   # symlink → "not canonical"
/opt/homebrew                       uid=501          # user-owned → "not root-owned"

So a user who has gh installed and gh auth login-ed still got a wall of sudo instructions — copy the binary into a privileged libexec dir, chown root, and repeat after every brew upgrade. In practice most people just could not use the feature.

What Codex does

Investigated openai/codex before changing policy:

  • No executable-location allowlist anywhere. Shell tool calls become zsh -lc "<cmd>" (codex-rs/core/src/shell.rs) and gh is resolved by the shell from the user's PATH. Its own probes call bare gh (codex-rs/tui/src/branch_summary.rs).
  • No ownership/writability validation of tool binaries. Trust is scoped to directories (TrustLevel is per project dir) and command shapes (is_safe_command), never to binary provenance.
  • Containment carries the boundary instead: Seatbelt/bwrap+seccomp (which explicitly (allow process-exec)), network off by default, and host-bound credential brokering that replaces the real GitHub credential with a shaped dummy in the child env.
  • The one anti-tampering check that exists — bwrap discovery — rejects a candidate only when it resolves inside the workspace (codex-rs/sandboxing/src/bwrap.rs), i.e. it defends against a malicious repo shadowing the helper, not against user-installed tooling.

Change

Same model, minus the parts we cannot rely on:

Resolution — override (KIROCREW_GH_BIN / KIROCREW_GLAB_BIN / KIROCREW_ISSUE_RADAR_GH) → well-known install dirs → ambient PATH. A managed root-owned copy still wins when one exists; otherwise the install the user already runs from their terminal is found (Homebrew, Linuxbrew, asdf/mise, ~/.local/bin).

Validation — accept the gateway user's own install, including symlinked layouts. Refuse only provenance the user did not choose:

Refused Why
binary or ancestor owned by another unprivileged account shared hosts
anything world-writable /tmp-style planting
anything inside the project checkout / workspace root the one substitution vector the model itself controls (codex's bwrap rule)

Unchanged — provider children still receive only the minimal provider-scoped environment (no AWS/Slack/gateway secrets, no inherited PATH), host pinning, subprocess/payload bounds, and the SEL audit event per spawn.

Escape hatchKIROCREW_PROVIDER_BIN_STRICT=1 restores the previous policy exactly (canonical, symlink-free, root-owned through every ancestor, PATH never consulted) and its setup error keeps the privileged-copy instructions.

Dev Fleet has a separate resolver (_trusted_bin) whose dir list was /usr/local/bin:/usr/bin:/bin — no Homebrew prefix at all, so a brew gh was invisible there too. It gains /opt/homebrew/bin and /home/linuxbrew/.linuxbrew/bin; its existing resolved-target checks (not writable by us, never under $HOME) already accept a brew binary (-r-xr-xr-x) and reject a planted shim.

The tradeoff is explicit: a shim the user (or the agent) plants in a user-owned PATH dir outside the project/workspace tree — e.g. ~/.local/bin — is now accepted, exactly as it is when the user types gh in their terminal. That is the requirement this PR implements.

Testing

  • test/test_source_providers.py — rewritten provider-executable suite: accepts user-owned and symlinked (Homebrew-layout) installs; rejects foreign-uid, world-writable file, world-writable parent, and agent-writable-tree shims; PATH hits appended after the well-known dirs; PATH ignored in strict mode; install-guidance vs strict sudo-guidance error copy.
  • test/test_issue_radar_gh_bin.py (new) — Issue Radar's _gh_bin: PATH-found user-owned install accepted, agent-tree shim refused, missing-gh guidance, strict mode still root-owned-only, override failure → GhSetupError(reason="not_installed"), cache behaviour.
  • test/test_dev_fleet_app.py — trusted-dir list now asserts the Homebrew/Linuxbrew prefixes are present in both _TRUSTED_BIN_DIRS and the pinned _TRUSTED_PATH.
  • Verified against the real host: a Homebrew gh (user-owned symlink into Cellar/) now resolves; strict mode rejects it; a shim inside the workspace tree and a 0777 /tmp shim are rejected.
  • flake8 clean, mypy src/kiro_crew clean (496 files), full suite 18959 passed — the 4 remaining failures (test_mcp_apps_call_endpoint AF_UNIX path-length, test_module_loader reload) reproduce unchanged on pristine main in this environment.

Docs: docs/system-specs/modules/issue-radar.md, docs/system-specs/modules/learn-cron-dashboard.md, CHANGELOG entry under Unreleased → Fixes.

@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Jul 28, 2026
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @dwu96 overrides the GPT 5.6 finding for ea3f95b57d1e23cbf44546d1dd68fc8d0afedcf1; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

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

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Jul 28, 2026
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Arbiter — ✅ no blocking findings

Arbiter found no unresolved long-term items that require action before merging ea3f95b57d1e23cbf44546d1dd68fc8d0afedcf1.

Second-order review for ea3f95b57d1e23cbf44546d1dd68fc8d0afedcf1; this comment is updated in place on each push.

Review details

Both files read. The line-level reviewers raised nothing (Opus 5: no findings; GPT 5.6: single finding overridden by an authorized human), and the design review passed with two explicitly follow-up-scoped suggestions. Neither suggestion is a one-way door (an env-var knob and a duplicated trust policy are both freely changeable in a later PR — the governance SCOPE_CATALOG is explicitly data-extensible) nor a concrete harm this diff triggers. Verdict below.

Arbiter-Verdict: PASS

No sub-threshold finding meets the long-term-impact bar.

Suggested follow-ups (open as issues — non-blocking)

  • Unify Dev Fleet's _trusted_bin with the shared provider resolver — design reviewer suggestion. Dev Fleet (apps/builtins/dev_fleet/server.py) keeps a second, independently-maintained trust policy for the same gh/git binaries; this PR had to patch both sides in parallel, which is exactly the drift risk. Safe to wait: both policies are currently consistent and either can be refactored onto provider_executable_candidates + _validate_provider_executable in a later change with no data/API impact. Fix in dev_fleet/server.py by delegating to source_providers.
  • Expose KIROCREW_PROVIDER_BIN_STRICT as a governance scope — design reviewer suggestion. The repo's stated home for un-weakenable policy is the governance ceiling (security_policy.json / SCOPE_CATALOG), and an env var cannot be enterprise force-pinned. Safe to wait: adding a scope is a SCOPE_CATALOG data change by design (never an evaluator edit), so it composes cleanly later; the env var remains a working operator control in the meantime. Fix in platform/governance_profiles.py (SCOPE_CATALOG) plus a read at the provider-validation site.

[ARBITER-REVIEWED] ea3f95b

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

For a broader accepted-risk deferral, apply defer-longterm and explain why.

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of ea3f95b57d1e23cbf44546d1dd68fc8d0afedcf1 — updated in place on each push; does not block merge.

Design-Verdict: PASS

A real adoption-killing policy is replaced with a proportionate, well-benchmarked trust model; the weakened guarantee is explicitly documented and reversible via strict mode.

The security delta is genuinely near-zero for this deployment shape: the agent can already run the user's gh directly through its bash tool, so refusing user-owned binaries in this one path defended nothing the rest of the system doesn't already grant — the old rule was the design bug, and this fixes the cause, not a symptom.

Suggestions

  • Dev Fleet's _trusted_bin remains a second, divergent trust policy for the same binaries (this PR had to patch both sides separately); a follow-up unifying it on provider_executable_candidates + _validate_provider_executable would prevent the drift the diff itself demonstrates.
  • KIROCREW_PROVIDER_BIN_STRICT is an env-var security knob on a repo whose stated home for un-weakenable policy is the governance ceiling (security_policy.json / SCOPE_CATALOG); exposing strict mode as a governance scope would let enterprise deployments force-pin it — worth a follow-up, not this PR.

[DESIGN-REVIEWED] ea3f95b

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Opus 5 Review — ✅ no blocking findings

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

No findings.

Verdict recorded via the action's structured output for commit ea3f95b57d1e23cbf44546d1dd68fc8d0afedcf1.

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

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Jul 28, 2026
@dwu96

dwu96 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 23ba58cf addressing the review round on f047cc44.

Accepted and fixed

  • Arbiter / Design Review — root-gateway refusal moved behind strict mode. Correct catch, and unintended: on a root gateway every spawned process (the agent's own shell included) is root too, so the ownership and agent-writable-tree checks that carry the relaxed policy are vacuous. The refusal is unconditional again in both modes, documented in the spec and the CHANGELOG entry. Non-root users — the whole population this PR unblocks — lose nothing.
  • GPT 5.6 refactor: rename project KiroClaw to KiroCrew #2 (dev_fleet/server.py) — "agent replaces the user-owned symlink after validation". The concrete race was real, though not the prefix itself: _trusted_bin cached the bin-dir entry it searched, so a repointed symlink would have been followed at spawn time. It now caches the resolved target it actually vetted (resolved = str(real)), which is what gets spawned. source_providers._validate_provider_executable already returned the resolved real path, so the PR panel and Issue Radar were never exposed to that race.
  • Backend test failures (Linux shards 2/4). My ancestor rule rejected any world-writable directory, which on Linux includes /tmp (1777) — so every pytest tmp_path fixture, and with it the whole accept path, failed on Linux while passing on macOS (/private/var/folders, 0700). A world-writable directory is now tolerated when it carries the sticky bit: only the owner may replace an entry there, so the "owned by another user" check still decides. World-writable files are still refused. Added an explicit test for the sticky case so this cannot silently regress. Coverage Gate failed only because it fails closed when the test jobs do.

Not taking: GPT 5.6 #1 — "restore rejection of gateway-writable executables and ancestors"

That is the exact behaviour this PR exists to remove, so restoring it would revert the PR rather than fix it. The requirement driving the change is: if gh works in the user's terminal, it must work here. A user-owned ~/.local/bin/gh, an asdf/mise shim, and a Homebrew bin/gh -> ../Cellar/... symlink are all "gateway-writable" by construction, and the previous rule refused all of them — which is why users with an installed, authenticated gh could not use the PR panel or Issue Radar at all without a sudo cp ritual repeated after every brew upgrade.

The comparison point is deliberate: Codex applies no binary-provenance check whatsoever — shell tool calls become zsh -lc "…" and gh is resolved from the user's PATH (codex-rs/core/src/shell.rs); its own probes call bare gh. Trust there is scoped to project directories and command shapes, with containment (Seatbelt / bwrap+seccomp, network off by default, host-bound credential brokering) carrying the boundary. Its single anti-tampering check rejects a candidate only when it resolves inside the workspace — which is precisely the rule this PR adopts via _agent_writable_roots().

So the residual risk is stated rather than eliminated: an executable the agent plants in a user-owned PATH directory outside the project checkout and workspace root is accepted, exactly as it would be if the user typed gh in their own shell. What still bounds it: the project/workspace tree (where the agent overwhelmingly writes) is refused; foreign-owned and world-writable binaries are refused; a root gateway is refused; the child gets only a minimal provider-scoped environment — no AWS/Slack/gateway secrets, no inherited PATH; and every spawn emits an SEL audit event. KIROCREW_PROVIDER_BIN_STRICT=1 restores the old root-owned-only policy verbatim for shared or multi-tenant hosts.

Happy to take a narrower hardening if reviewers prefer one — e.g. adding a deny rule so the agent cannot write a file named gh/glab into any user bin directory, which would close the plant vector without refusing installs the user made themselves.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Jul 28, 2026
@dwu96
dwu96 force-pushed the fix/gh-cli-user-owned-trust branch from 23ba58c to b2740cc Compare July 28, 2026 05:49
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Jul 28, 2026
The PR/MR panel and Issue Radar refused any provider CLI whose binary or
parent directories were not root-owned and unwritable by the gateway user.
A stock `brew install gh` can never satisfy that: /opt/homebrew is owned by
the user and bin/gh is a symlink into Cellar/. Users who had gh installed
and authenticated were told to sudo-copy it into a privileged libexec dir
and re-copy it after every CLI upgrade, so most simply could not use the
feature.

Codex, for comparison, applies no provenance check at all: shell tool calls
resolve through the user's PATH and trust is carried by the OS sandbox,
approval gates, and host-bound credential brokering. Its one anti-tampering
check rejects a binary found *inside the workspace*.

Adopt that model. Resolution is now override -> well-known install dirs ->
ambient PATH, and validation accepts the gateway user's own install
(symlinked Homebrew/Linuxbrew/asdf layouts included) while refusing
provenance the user did not choose: a binary or ancestor owned by another
unprivileged account, anything world-writable (a world-writable directory
is tolerated only when sticky, where the owner check still decides), and
anything inside the agent-writable project checkout or workspace root.
A root gateway stays refused outright in both modes: every process it
spawns is root too, which would make those checks vacuous. Containment
carries the boundary instead -- provider children still get only a minimal
provider-scoped env and every spawn stays SEL-audited.

Dev Fleet's separate git/gh resolver gains the Homebrew and Linuxbrew
prefixes for the same reason, and now pins the RESOLVED target rather than
the bin-dir entry it searched, so a user-writable symlink cannot be
repointed between validation and execution.

KIROCREW_PROVIDER_BIN_STRICT=1 restores the previous root-owned,
symlink-free policy for shared or multi-tenant hosts, and its setup error
keeps the privileged-copy instructions.
@dwu96
dwu96 force-pushed the fix/gh-cli-user-owned-trust branch from b2740cc to ea3f95b Compare July 28, 2026 06:15
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Jul 28, 2026
@dwu96

dwu96 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Pushed ea3f95b5.

Fixed — the launcher regression was real and my fault. Opus 5, the Arbiter and GPT 5.6 #2 all flagged the same hunk: packaging/build-desktop.sh had lost the while [ -h "$SOURCE" ] chain-walk from #188 (a4e2a307). Cause: squashing with git reset --soft origin/main after origin/main had advanced past my base, so my older copy of that file became a diff that reverted the fix. The file is restored to origin/main verbatim and is no longer in this PR's diff at all — the PR now touches only the provider-CLI resolution, its tests, and the docs. Nothing else in the diff depends on it.

Also in this revision (from the previous round): unconditional root-gateway refusal restored, Dev Fleet now caches the resolved target rather than the bin-dir symlink, and a sticky world-writable ancestor (/tmp, 1777) is tolerated while world-writable files stay refused — that last one is what was failing Backend Tests on the Linux shards, since every pytest tmp_path lives under /tmp there.

Not taking — GPT 5.6's remaining item ("restore Issue Radar's root-owned candidate resolution"). That is the change this PR exists to make, so implementing it would revert the PR rather than fix it. The requirement: if gh works in the user's terminal, it must work here. A Homebrew bin/gh -> ../Cellar/..., an asdf/mise shim and ~/.local/bin/gh are all "gateway-writable" by construction, and the old rule refused all three — which is why users with an installed, authenticated gh could not use Issue Radar or the PR panel without a sudo cp repeated after every upgrade.

The Arbiter reached the same conclusion and explicitly declined to escalate it: "this is the PR's deliberate, documented policy change, not an accident … reversible by flipping an env default later; it is not a one-way door."

Residual risk is stated rather than removed: an executable planted in a user-owned PATH directory outside the project checkout and workspace root is accepted, exactly as it would be if the user typed gh in their own shell — which is what Codex does unconditionally (no provenance check anywhere; zsh -lc + PATH). What still bounds it: the project/workspace tree is refused (_agent_writable_roots(), the same rule Codex applies to its sandbox helper), foreign-owned and world-writable binaries are refused, a root gateway is refused outright, the child gets only a minimal provider-scoped environment (no AWS/Slack/gateway secrets, no inherited PATH), and every spawn emits an SEL audit event. KIROCREW_PROVIDER_BIN_STRICT=1 restores the old root-owned-only policy verbatim.

Gates: flake8 clean, mypy src/kiro_crew clean, backend/frontend suites green on the previous push; this revision only drops a file from the diff.

Follow-up accepted as a separate issue, not this PR: converge Dev Fleet's _trusted_bin onto the shared resolver so one policy governs all credential-bearing CLI resolution, and fix the now-inaccurate "never drift" comment in source_providers.py.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Jul 28, 2026
@dwu96

dwu96 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

/ai-review override gpt ea3f95b: Accepting a user-owned gh/glab is this PR's deliberate, documented trust-model change (a Homebrew/asdf install is gateway-writable by construction, so restoring the root-owned rule would revert the PR) — provenance is replaced by containment the PR keeps: agent project/workspace-tree refusal, foreign-owner and world-writable refusal, unconditional root-gateway refusal, a minimal provider-scoped child env with no inherited PATH, per-spawn SEL audit, and KIROCREW_PROVIDER_BIN_STRICT=1 to restore the old policy; the Arbiter reviewed the same finding and explicitly declined to escalate it as "the PR's deliberate, documented policy change … not a one-way door".

@github-actions

Copy link
Copy Markdown
Contributor

AI-review override not recorded: keep the reason to 500 characters or fewer.

@dwu96

dwu96 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

/ai-review override gpt ea3f95b: Accepting a user-owned gh is this PR's deliberate documented policy change (a Homebrew/asdf install is gateway-writable by construction, so restoring the root-owned rule would revert the PR); containment is kept — agent-tree, foreign-owner, world-writable and root-gateway refusals, minimal provider-scoped env, SEL audit, plus KIROCREW_PROVIDER_BIN_STRICT=1 to restore the old policy — and the Arbiter reviewed this same finding and declined to escalate it.

@github-actions

Copy link
Copy Markdown
Contributor

Human judgment recorded

@dwu96 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for ea3f95b57d1e23cbf44546d1dd68fc8d0afedcf1.

Accepting a user-owned gh is this PR's deliberate documented policy change (a Homebrew/asdf install is gateway-writable by construction, so restoring the root-owned rule would revert the PR); containment is kept — agent-tree, foreign-owner, world-writable and root-gateway refusals, minimal provider-scoped env, SEL audit, plus KIROCREW_PROVIDER_BIN_STRICT=1 to restore the old policy — and the Arbiter reviewed this same finding and declined to escalate it.

This decision applies only to this commit. A new push requires a new judgment.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Jul 28, 2026
@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Jul 28, 2026
@dwu96

dwu96 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

This change is based on offline discussions with @CrysisDeu to offer better UX when users using GitHub gh CLI integrations in Kiro Crew.

Human reviewer please check PR description, AI review comments and overriding information.

@iamwhatever
iamwhatever merged commit 43f88a6 into main Jul 28, 2026
41 of 42 checks passed
@iamwhatever
iamwhatever deleted the fix/gh-cli-user-owned-trust branch July 28, 2026 06:59
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Jul 28, 2026
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
The PR/MR panel and Issue Radar refused any provider CLI whose binary or
parent directories were not root-owned and unwritable by the gateway user.
A stock `brew install gh` can never satisfy that: /opt/homebrew is owned by
the user and bin/gh is a symlink into Cellar/. Users who had gh installed
and authenticated were told to sudo-copy it into a privileged libexec dir
and re-copy it after every CLI upgrade, so most simply could not use the
feature.

Codex, for comparison, applies no provenance check at all: shell tool calls
resolve through the user's PATH and trust is carried by the OS sandbox,
approval gates, and host-bound credential brokering. Its one anti-tampering
check rejects a binary found *inside the workspace*.

Adopt that model. Resolution is now override -> well-known install dirs ->
ambient PATH, and validation accepts the gateway user's own install
(symlinked Homebrew/Linuxbrew/asdf layouts included) while refusing
provenance the user did not choose: a binary or ancestor owned by another
unprivileged account, anything world-writable (a world-writable directory
is tolerated only when sticky, where the owner check still decides), and
anything inside the agent-writable project checkout or workspace root.
A root gateway stays refused outright in both modes: every process it
spawns is root too, which would make those checks vacuous. Containment
carries the boundary instead -- provider children still get only a minimal
provider-scoped env and every spawn stays SEL-audited.

Dev Fleet's separate git/gh resolver gains the Homebrew and Linuxbrew
prefixes for the same reason, and now pins the RESOLVED target rather than
the bin-dir entry it searched, so a user-writable symlink cannot be
repointed between validation and execution.

KIROCREW_PROVIDER_BIN_STRICT=1 restores the previous root-owned,
symlink-free policy for shared or multi-tenant hosts, and its setup error
keeps the privileged-copy instructions.
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