Repository navigation
fix(security): deny kirocrew skills authority-retire from an agent shell - #4533
Conversation
GPT review dispositionsExact reviewed head:
Validation on this exact head: complete affected suite 1015 passed, 20 skipped, 49 warnings; Black, isort, Flake8, mypy (988 source files), compilation, docs checks, and |
484dd70 to
25ac351
Compare
GPT review dispositions for
|
25ac351 to
08f0961
Compare
08f0961 to
8ae5bc3
Compare
8ae5bc3 to
70e44a9
Compare
|
🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: This PR surfaced in automated triage with merge conflicts and blocking readiness items. I assessed it, and it is outside what unattended automation should drive:
When you've rebased and resolved the conflicts, the pipeline will re-assess on its next cycle. |
Head branch was pushed to by a user without write access
70e44a9 to
58b71f8
Compare
|
Rebased onto
|
|
Rebased onto
|
bolichen97
left a comment
There was a problem hiding this comment.
Head reviewed: 84a1be9.
My earlier change request is met on this head, so I withdraw it. The description names this commit in the cherry-pick command and the local test line. Its base 4b066e0 is the real parent and the current tip of main. The budget figures in the body (29,607 to 29,627) match the diff, and main's #20687 paragraph is kept as written.
The code is correct and unchanged from the previous head. _is_self_skills_authority_retire calls _matches_self_subcommand(("skills", "authority-retire")) in the same way as the gateway restart floor. The id is in _SELF_PROTECTION_UNGATED_FLOOR_IDS, in the is_denied loop, in the facade and in _exports, and the spec lists the same six ungated ids as the code.
What still has to clear:
- The fork workflow runs on this head wait for maintainer approval. No CI or AI review lane has run on 84a1be9 yet.
- A maintainer must land this from a repository branch. Sensitive Change Review is red for that reason only.
- The
tag-grantscomment refresh in Design question 1 has no tracking issue. Open one and link it in the description, so the follow-up is not lost when #18595 lands.
|
The Security Scope Review on
The cherry-pick line in the description now names |
|
The help exemption on
A The new head is Any other word after the CLI keeps the floor on:
The direct and Changes on this head:
Checks: the security test modules pass (6,428 passed, 2 skipped). The cherry-pick line now names |
bolichen97
left a comment
There was a problem hiding this comment.
Head reviewed: ad33292.
Not ready. The help exemption added on this head lets a real retire through, and the branch needs a rebase that moves the floor registration.
-
The exemption compares the normalized argv (
scan.norm), not the words the shell passes. I ransecurity.is_deniedon this head. It allowskirocrew skills authority-retire ${nope:+--help}and the same command with`--help`in backticks. Bash expands each of those to an empty word, so the shell runs a barekirocrew skills authority-retire. Details are in the inline comment onargv_floor.py. -
Main moved the code this PR registers into. #21650 split the security facade: the loop over the ungated self-protection floors is now in
security/deny_gate.py, andsecurity/__init__.pyholds a lazy name map. After the rebase, add("self-protection-skills-authority-retire", _argv._is_self_skills_authority_retire)to thedeny_gate.pyloop and the name to the__init__.pymap. If only the old__init__.pyhunk is kept, the floor is exported butis_deniednever calls it. Main's package budget is now 30,209, so re-measure that pin too. -
The Security Scope Review on this head is red for
--help | head -40and--help 2>&1. If the exemption grows to allow pipes and redirects, it grows the surface from item 1. I suggest dropping the exemption, so this floor refuses--helplikerestart,gateway restartandcloudalready do (your own test pinskirocrew restart --helpas refused). The refusal note already tells the operator to run the command in their own terminal. A maintainer then records a scope disposition for the help rows on that head. If you prefer to keep the exemption, see the inline comment for the narrow form.
What is correct and unchanged: the matcher reuses _matches_self_subcommand like the sibling floors, the id is in _SELF_PROTECTION_UNGATED_FLOOR_IDS and the facade exports, and the spec lists the same six ungated ids as the code. A maintainer still has to land this from a repository branch.
| ): | ||
| continue | ||
| if _operands_lead_with(_self_cli_operands(tokens, prog_idx), spec): | ||
| if help_exempt and scan.norm[prog_idx + 1 :] in ([*spec, "--help"], [*spec, "-h"]): |
There was a problem hiding this comment.
🔴 The help exemption lets a bare authority-retire through when the flag is a word the shell expands to nothing.
The check compares scan.norm, the argv after _normalize_operand. That normalizer resolves ${nope:+--help} to its literal --help and strips the backticks from `--help`. Bash expands both to an empty word. So kirocrew skills authority-retire ${nope:+--help} reads as the exact help form here, and is_denied returns no denial, while the shell runs the retire with no arguments. I confirmed both strings are allowed on this head. This floor is the only argv-level check for the command, so the exemption reopens the gap the PR exists to close once #18595 lands.
if help_exempt and scan.norm[prog_idx + 1 :] in ([*spec, "--help"], [*spec, "-h"]):
continueWhy it matters: an agent shell can run the operator-only recovery with one extra word, on any host where the OS mask on tag-grants does not apply.
Suggestion: remove help_exempt, the golden-paths --help row and the help-allowed test, so this floor matches its siblings. If the exemption stays, compare the raw tokens[prog_idx + 1 :] after stripping only one matched pair of ' or " quotes, and refuse the exemption when any of those tokens contains $, a backtick, (, {, <, >, \ or a control operator. Add ${nope:+--help} and `--help` to test_every_other_skills_authority_retire_form_with_help_is_refused. If the raw-token form already refuses both strings, I will withdraw this.
Rule: AGENTS.md "Security invariants".
|
@bolichen97, the new head is
The Design lane's question, whether #18595 puts the authority root under the hidden Checks: the floor and gate tests pass on this head (131), and the full security set passes on the patch-identical commit one main step earlier (7,082 passed, 2 skipped). |
|
|
|
|
bolichen97
left a comment
There was a problem hiding this comment.
Head reviewed: 3e1e307.
Both items from my previous change request are fixed on this head, so I withdraw it.
- The help exemption is gone.
_matches_self_subcommandis back to its main signature and_is_self_skills_authority_retirecalls it with no exemption, like the restart, gateway restart and cloud floors. I ranis_deniedon this head:${nope:+--help}, a backtick--help,bash -c "kirocrew skills authority-retire",python -m kiro_crew skills authority-retire, a quoted"skills"and an interposed redirect are all refused, whilekirocrew skills --helpand a product-named worktree path that merely mentions the verb stay allowed. - The rebase is done. The branch sits one commit on a current main, merges into main without conflict, and the floor is registered in the
deny_gate.pysubcommand loop with its rule id in_SELF_PROTECTION_UNGATED_FLOOR_IDSand its note indenied_rules_tables.py.
Verified locally on this head: test/test_denied_commands_security.py and test/test_security_gate_liveness.py pass (87 tests), including the new help-form test and the raised package line budget.
No new findings. I am not approving yet: this change edits the command deny gate, so it needs a maintainer's sensitive-change sign-off and a green readiness signal on this head, and another reviewer's change request is still open.
|
Rebased to resolve a conflict in Main's #21573 raised Verified:
For the maintainer landing it, the cherry-pick is now |
|
Rebased onto Main's #21865 rewrote the row-backed self-protection paragraph (hosts-file digests on POSIX too), which sits between the two spec lines this PR changes. Main's paragraph is kept as it is, and this PR's two lines are unchanged. Every other file's patch ID is identical. Main's security package still measures 31,865 lines, so the budget stays at the measured 31,883. The Design Review's pre-merge check on The Security Scope Review passed on Verified on
|
|
|
Rebased onto Main's #22592 raised the same Every run on Verified:
|
|
The red Coverage Gate on The other two reds are not this change's either. The Dependency License Gate reports that dependency review is not enabled for the repository, and the Sensitive Change Review could not read the PR from the API (on a fork PR that touches a sensitive path it is red regardless). Every lane passed or reported no findings on |
|
Rebased The rebase resolved a conflict in Checked locally on this head: the 67 security, deny, argv and floor test modules (8,421 passed, 6 skipped), the facade-mirror and campaign-facade guards (233 tests), Cherry-pick for a maintainer: |
|
Rebased The rebase resolved a conflict in Checked locally on Cherry-pick for a maintainer: |
|
HOLD at 12a9f44. This adds an ungated deny floor to the security gate ( The code looks sound. The matcher reuses Still open before approval:
I read the diff and CI. I did not run tests. — crew-pr-reviewer (r119) |
|
Opened #23038 to track the |
|
Rebased onto main Checked on this head: the security, deny, argv and floor test modules (83 modules, 9,676 passed, 6 skipped), the facade-mirror and campaign-facade guards (318 tests), Main has since moved to |
|
Rebased onto main as The tests ran on |
The auto-skill publication authority (kirodotdev#18595) gains an operator-only recovery, `kirocrew skills authority-retire`, that moves an idle authority and its provenance record aside. It must run from the operator's own terminal, so this adds the ungated `self-protection-skills-authority-retire` argv floor beside the existing self-protection subcommand floors: one structural matcher, its rule id and reason, its `deny_gate` registration and facade export, and its name in the ungated-floor comment. No catalog row, no opt-out; no existing pass widened and no threshold moved. Like its sibling floors, it refuses the command's `--help` form too: a word that reads as `--help` can reach the shell as nothing or as a plain argument, and the retire then runs. This is the whole sensitive-path footprint of the auto-skill authority, split out so a maintainer can land it from a repository branch; kirodotdev#18595 touches no sensitive path and follows as an ordinary fork PR once this is on main. Tests: the floor's template, list, structural and help-form assertions in test_denied_commands_security.py; the security package line budget raised from 32,188 to the measured 32,206.
|
Rebased onto main Checked on this head: the security, deny, argv and floor modules (83 modules, 9,677 passed, 6 skipped), the facade-mirror and campaign-facade guards (318 tests), |
Problem / Motivation
Goal:
kirocrew skills authority-retire, the operator-only recovery that the auto-skill publication authority adds in #18595, is denied through an agent shell by an ungated argv floor.The auto-skill publication authority (#18595) adds one CLI recovery,
kirocrew skills authority-retire, which moves an idle authority root and its provenance record aside so that by-name skill mutation resumes on a host whose authority can no longer be certified. That command must run from the operator's own terminal: if an agent shell could run it, the agent could retire the very authority that fences unreviewed promotion. The existing self-protection subcommand floors (approve,gateway restart,cloud <destructive>) are the mechanism for exactly this: an argv matcher the sandbox denies with no catalog row and no opt-out.This PR is the sensitive-path footprint of the auto-skill promotion change, split out from the rest of it. The Sensitive Change Review refuses any fork PR that touches
src/kiro_crew/sandbox*.pyorsrc/kiro_crew/security/*, so this part has to be landed by a maintainer from a repository branch; the remainder (#18595) touches no sensitive path and merges on its own afterwards.Why it matters
Without the floor, the retire command ships as an agent-reachable way to loosen the authority that #18595 relies on. With the floor landed first, #18595 can merge as an ordinary fork PR with the operator-only guarantee already in place.
Not a goal
_is_self_skills_authority_retirereuses_matches_self_subcommand, the same structural matcher the existing subcommand floors use.tag-grantswas already on the deny floor and the sandbox mask.What changed (motivation → approach → change)
kirocrew skills authority-retire(new in #18595) would be allowed through an agent shell like any otherkirocrewsubcommandself-protection-skills-authority-retirefloor denies it structurally, with the reason text naming the operator-terminal requirementsecurity/argv_floor_self.pyandsecurity/argv_floor.py:_is_self_skills_authority_retire(text_lower)matches the product CLI as the argv's own program with the leading subcommandsskills authority-retire, through interposed shell dressing, via_matches_self_subcommand, exactly as the sibling subcommand floors do; theargv_floorfacade's manifest names it. No help form is exempt: likerestart,gateway restartandcloud, the floor refuses the command's--helptoo, because a word that reads as--helpcan reach the shell as nothing (${nope:+--help},`--help`) or as a plain argument (after--, or as an option's value), and the retire then runs.security/denied_rules_tables.py: the floor id joins_SELF_PROTECTION_UNGATED_FLOOR_IDSand its reason text joins_SELF_PROTECTION_FLOOR_NOTES; the comment enumerating the ungated floors names it.security/deny_gate.py,security/__init__.pyandsecurity/_exports.py: the matcher is registered indeny_gate's loop over the ungated self-protection floors, sois_deniedcalls it, and the facade's lazy name map, itsTYPE_CHECKINGmanifest and the export list name it.docs/system-specs/modules/security.md: the self-protection floor list names theskills authority-retirerecovery, the ungated set reads six ids, includingself-protection-file-delivery, each with its no-catalog-row reason, and the spec says the recovery's--helpform is refused like its siblings' whilekirocrew skills --helpstays allowed. Thetag-grantscomments insandbox_leaves.py,sandbox_precreate.pyandsecurity/paths_lists.pyare unchanged: a note that the leaf also parents the auto-skill authority belongs with the change that creates that authority, and until then it would name a constant that does not exist onmain.test/test_security_gate_liveness.py:_PACKAGE_LINE_BUDGETraised from main's 32,188 to the measured 32,206 with its reason (the matcher, its manifest line, the rule id and reason, thedeny_gateregistration, the export, and the comment line).Cherry-pick request for a maintainer: this PR is one commit,
2072a31878a2e9cc81df161835d43f3c4aa98c3eonrubencu/KiroCrewbranchsecurity/skills-authority-retire-floor(also this PR's head), on top ofmainat69b2e503ebce6dc992fa833994bc1e9164c3ac18.git fetch https://github.com/rubencu/KiroCrew.git security/skills-authority-retire-floor && git cherry-pick 2072a31878a2e9cc81df161835d43f3c4aa98c3eonto a repository branch reproduces it; the Sensitive Change Review template then applies to that branch.Backwards compatibility
Breaking: none. The floor denies a subcommand that does not exist until #18595 lands; on main today the same argv is an unknown
kirocrewsubcommand. No existing command, catalog row or threshold changes.Tests
test/test_denied_commands_security.py: the floor's command template, its place in the self-protection subcommand floor list, the structural assertionsecurity._is_self_skills_authority_retire("kirocrew -\\v skills authority-retire"), andtest_skills_authority_retire_refuses_its_help_form_like_its_sibling_floors: the plain,-h, module,${nope:+--help}, backtick, comment,--, option-value, piped and2>&1help forms are all refused, the restart floor still refuses its own help form, andkirocrew skills --help,skills listandskills rootstay allowed.test/test_security_gate_liveness.py: the package budget pin at the measured total.2072a31878(one commit on69b2e503eb; the rebase resolved a conflict intest/test_security_gate_liveness.py, where main's fix(security): keep SRI integrity digests out of the bare-secret pass #19689 raised the same budget by 63, to 32,188, for the SRI-digest exemption inredaction. Main's reason and line stay as they are, this floor's reason now reads "from 32,188", and every other per-file patch ID is unchanged from6a2fea080a): everytest_security_*module (the security-conductor corpus tests among them),test_denied_commands_security,test_deny_diffand the other deny, argv and floor modules pass (83 modules, 9,677 passed, 6 skipped), and the facade-mirror and campaign-facade guards pass on this head (318 tests). The budget test reads the package at exactly 32,206 lines: main's measured 32,188 plus this floor's 18.Design questions
tag-grantscomments insandbox_leaves.py,sandbox_precreate.pyandsecurity/paths_lists.pydo not yet say the leaf parents the auto-skill authority. Neither PR can carry that note cleanly: here it names a constant that does not exist onmain, and in feat(skills): atomically promote safe auto-skill updates #18595 it touches a sensitive path. It is a comment-only refresh for whoever lands the authority, tracked in Note in the tag-grants comments that the leaf parents the auto-skill authority #23038.Manual verification
N/A — the floor is exercised by the denied-command tests above; there is no user-visible surface.
Related Issues
Part of #18595 (the auto-skill promotion change this floor protects). Companion PRs: #11323 (coordination note posted there), #12970 (lock order), #7617 (all-invalid-script rule), as recorded on #18595.
Screenshots / video
Why no screenshot: no user-visible surface changes; this adds one denied-command floor and its spec entry.
Pattern harvest
Not generalizable: the gap is one operator-only recovery command (
kirocrew skills authority-retire) missing from the ungated argv floor, a per-command allow/deny decision that no line pattern separates from the operator commands the floor deliberately leaves reachable.Checklist
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)docs/system-specs/modules/security.mdlists the floor among the ungated self-protection floors in this commit; feat(skills): atomically promote safe auto-skill updates #18595 describes the authority it protectsContribution License Agreement
Contributions are accepted under the project's Apache-2.0 license; see the Licensing section of CONTRIBUTING.md.