feat(security): operator keystone extension for OAuth consent-endpoint allowlist (#1341) - #2402
Conversation
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've examined the diff exhaustively and falsified my candidates:
No findings. [OPUS-REVIEWED] 0a2652f Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — 🟡 CONCERNSAdvisory design-level review of I have what I need for the design review. The diff is well-scoped (security.py extension + loader path + docs + tests), and I've verified the key claim: Design-Verdict: CONCERNS The extension lands on a gate with no production caller, so the operator harm it targets isn't relieved until an unfiled gate-consolidation follow-up ships. Watch
Suggestions
[DESIGN-REVIEWED] 0a2652f |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
…t allowlist (#1341) The security module's OAuth banner-safety contract exempts standard front-channel params from the exfiltration heuristics only at an exact-match endpoint in the code-owned _OAUTH_AUTHORIZATION_ENDPOINTS set. Any identity provider outside that launch set (Okta orgs, Auth0, self-hosted OIDC, tenant-scoped Entra paths) fails the gate closed with no remedy short of a code release. Add an operator-owned, keystone-fenced extension: - <config_dir>/oauth_endpoints.json holds {additional_authorization_endpoints: [{host, path}]}; the operator hand-edits it out-of-band (no dashboard writer). - security._load_operator_oauth_endpoints() fails soft to the EMPTY set on a missing/unreadable/corrupt/non-object file and strictly validates every entry (exact lowercase DNS host with letter TLD, exact case-sensitive path; no wildcards, schemes, ports, userinfo, percent-escapes, IP literals, whitespace, backslashes, or dot-dot). The entry list is truncated at 50 BEFORE validation so a mangled file cannot amplify, and the parsed set is memoized on the file's stat so a hand-edit takes effect on the next check without a restart. - Both checkpoints (_exfil_url_warning under allow_oauth_entropy and oauth_url_contains_credential) consult the union at check time via _approved_oauth_authorization_endpoint(). HTTPS-only, no explicit port, and every unconditional heuristic are unchanged and not relaxable via the file. - An approval that came from an operator entry emits a best-effort oauth_endpoint_extension_used SEL event, deduped per process per endpoint. - oauth_endpoints.json joins _CREW_SECRET_LEAVES so the agent can neither read nor write its own trust widening, under every crew home prefix. The dashboard's live MCP OAuth banner currently validates with a separate, more permissive per-param gate local to chat_runner.py that consults no endpoint allowlist; the extension governs the security- module contract gate and its consumers. Consolidating the two gates is tracked as a follow-up. The security.md keystone bullet states this enforcement-point caveat explicitly. Adds OPERATOR_EXTENSION_OAUTH_URLS to the corpus (the generic long-state IdP shape plus Okta org and tenant-scoped Entra) asserted rejected by default config and passing only with the operator file present. Closes #1341
8f3a35c to
0a2652f
Compare
|
CodeQL alert resolved ( |
|
Re: Design Review CONCERNS (sequencing inversion / dead-surface risk) — answering each point:
Holding vs. merging is the maintainers' call; the pieces are now cleanly separable either way. |
#4007) The MCP OAuth rejection banner names the symptom but not the remedy, so a legitimate consent URL at an unlisted identity provider reads as an unfixable failure. Two different causes reach this branch. A genuinely bogus URL embedding a credential is one. The other is a legitimate OAuth 2.1 + PKCE consent URL whose authorization endpoint is simply not in _OAUTH_AUTHORIZATION_ENDPOINTS: the entropy carve-out in oauth_url_contains_credential applies only at an approved (host, path), so an unlisted self-hosted IdP has its 43-byte code_challenge scanned as a bare secret and fails closed. That second case already has a remedy - the operator keystone extension oauth_endpoints.json added in #2402 (closing #1341) - but it is on _CREW_SECRET_LEAVES with deliberately no dashboard writer, and is documented only in docs/system-specs/modules/security.md. Nothing the user can see names it. In #3310 two people independently root-caused this from source instead of finding the one-line config fix, which is the migration story #2403 asked for and that was never shipped. Name the file in the banner and add a 'remedy' meta key. The rejection itself is unchanged: still failed/rejected_url, oauth_url still withheld, the URL still never rendered. HTTPS-only, no-explicit-port and exact-match remain enforced by the gate and are not relaxable via the file, so this widens nothing - it only tells the operator where the sanctioned lever is. Tests: test_rejection_banner_names_the_operator_remedy pins that the remedy is named and that the hint does not soften the rejection. 76 passed in test_mcp_oauth_banner.py, 234 in test_chat_runner_coverage.py + test_display_time_redaction.py; flake8 and isort clean. Co-authored-by: William Laws <jeeshofone@users.noreply.github.com>
…t allowlist (kirodotdev#1341) (kirodotdev#2402) The security module's OAuth banner-safety contract exempts standard front-channel params from the exfiltration heuristics only at an exact-match endpoint in the code-owned _OAUTH_AUTHORIZATION_ENDPOINTS set. Any identity provider outside that launch set (Okta orgs, Auth0, self-hosted OIDC, tenant-scoped Entra paths) fails the gate closed with no remedy short of a code release. Add an operator-owned, keystone-fenced extension: - <config_dir>/oauth_endpoints.json holds {additional_authorization_endpoints: [{host, path}]}; the operator hand-edits it out-of-band (no dashboard writer). - security._load_operator_oauth_endpoints() fails soft to the EMPTY set on a missing/unreadable/corrupt/non-object file and strictly validates every entry (exact lowercase DNS host with letter TLD, exact case-sensitive path; no wildcards, schemes, ports, userinfo, percent-escapes, IP literals, whitespace, backslashes, or dot-dot). The entry list is truncated at 50 BEFORE validation so a mangled file cannot amplify, and the parsed set is memoized on the file's stat so a hand-edit takes effect on the next check without a restart. - Both checkpoints (_exfil_url_warning under allow_oauth_entropy and oauth_url_contains_credential) consult the union at check time via _approved_oauth_authorization_endpoint(). HTTPS-only, no explicit port, and every unconditional heuristic are unchanged and not relaxable via the file. - An approval that came from an operator entry emits a best-effort oauth_endpoint_extension_used SEL event, deduped per process per endpoint. - oauth_endpoints.json joins _CREW_SECRET_LEAVES so the agent can neither read nor write its own trust widening, under every crew home prefix. The dashboard's live MCP OAuth banner currently validates with a separate, more permissive per-param gate local to chat_runner.py that consults no endpoint allowlist; the extension governs the security- module contract gate and its consumers. Consolidating the two gates is tracked as a follow-up. The security.md keystone bullet states this enforcement-point caveat explicitly. Adds OPERATOR_EXTENSION_OAUTH_URLS to the corpus (the generic long-state IdP shape plus Okta org and tenant-scoped Entra) asserted rejected by default config and passing only with the operator file present. Closes kirodotdev#1341 Co-authored-by: bolichen97 <bolichen97@users.noreply.github.com>
kirodotdev#4007) The MCP OAuth rejection banner names the symptom but not the remedy, so a legitimate consent URL at an unlisted identity provider reads as an unfixable failure. Two different causes reach this branch. A genuinely bogus URL embedding a credential is one. The other is a legitimate OAuth 2.1 + PKCE consent URL whose authorization endpoint is simply not in _OAUTH_AUTHORIZATION_ENDPOINTS: the entropy carve-out in oauth_url_contains_credential applies only at an approved (host, path), so an unlisted self-hosted IdP has its 43-byte code_challenge scanned as a bare secret and fails closed. That second case already has a remedy - the operator keystone extension oauth_endpoints.json added in kirodotdev#2402 (closing kirodotdev#1341) - but it is on _CREW_SECRET_LEAVES with deliberately no dashboard writer, and is documented only in docs/system-specs/modules/security.md. Nothing the user can see names it. In kirodotdev#3310 two people independently root-caused this from source instead of finding the one-line config fix, which is the migration story kirodotdev#2403 asked for and that was never shipped. Name the file in the banner and add a 'remedy' meta key. The rejection itself is unchanged: still failed/rejected_url, oauth_url still withheld, the URL still never rendered. HTTPS-only, no-explicit-port and exact-match remain enforced by the gate and are not relaxable via the file, so this widens nothing - it only tells the operator where the sanctioned lever is. Tests: test_rejection_banner_names_the_operator_remedy pins that the remedy is named and that the hint does not soften the rejection. 76 passed in test_mcp_oauth_banner.py, 234 in test_chat_runner_coverage.py + test_display_time_redaction.py; flake8 and isort clean. Co-authored-by: William Laws <jeeshofone@users.noreply.github.com>
Summary
Adds an operator-owned, keystone-fenced extension to the OAuth consent-endpoint allowlist (
_OAUTH_AUTHORIZATION_ENDPOINTS), so an identity provider outside the code-owned launch set (Okta orgs, Auth0, self-hosted OIDC, tenant-scoped Entra/{tenant}/oauth2/v2.0/authorize) can be approved by the operator without a Kiro Crew code release — while keeping the ceiling un-widenable by the agent.<config_dir>/oauth_endpoints.json—{"additional_authorization_endpoints": [{"host": "acme.okta.com", "path": "/oauth2/v1/authorize"}]}. Hand-edited out-of-band; deliberately no dashboard writer (minimal surface)._load_operator_oauth_endpoints): missing / unreadable / corrupt / non-object file → EMPTY set (mirrorscomputer_use.enable_state.load_state). A mangled file never widens trust.%, whitespace, backslash, IP literals, leading/trailing dots), exact case-sensitive path (rejects; ? # % \, whitespace,..); list truncated at 50 before validation so a mangled file cannot amplify._approved_oauth_authorization_endpoint(), memoized on the file's stat (mtime_ns,size) — a hand-edit takes effect on the next check, no restart; an unchanged file costs onestat._OAUTH_QUERY_PARAMSare skipped. Fixed-credential patterns, heavy percent-encoding, userinfo, fragments, backslashes, unknown-param heuristics, HTTPS-only, and no-explicit-port remain unconditional and are NOT relaxable via the file.oauth_endpoint_extension_usedevent (deduped per process per endpoint).oauth_endpoints.jsonjoins_CREW_SECRET_LEAVES— agent reads AND writes blocked on the tool path and shell forms, under both crew home prefixes.OPERATOR_EXTENSION_OAUTH_URLSlist restores the genericid.example-idp.comlong-statecase (plus Okta org and tenant-scoped Entra shapes) under the operator-extension contract — asserted rejected with default config and passing only with the operator file (deliberately NOT inLEGIT_OAUTH_URLS, which pins default-config behavior).Enforcement-point caveat (verified, documented)
security.oauth_url_contains_credential— the module's tested ACP banner-safety contract and the only gate that consults the endpoint allowlist — has no production caller on current main: the dashboard's live MCP OAuth banner uses a separate, more permissive per-param gate local tochat_runner.py(no allowlist, no query-length heuristic). Per the task scoping, this PR lands the extension at the allowlist's gate (the tested contract) and does not rewire the banner path: swapping the strict gate intochat_runnerwould newly reject currently-working IdPs and is a behavioral change needing its own review. The newsecurity.mdkeystone bullet states this caveat explicitly. Follow-up issues: gate consolidation is #2403; the pre-existing keystone bash-fence gap is #2404.Pre-push review fleet
Two model-pinned read-only reviewers (GPT 5.6 Sol + Opus 5) ran before push:
security.mdand the commit message.is_sensitive_write_pathpinned, distinct-endpoint SEL emit after dedupe, autouse reset of process-global memo/dedupe state.cdbash-fence gap — verified pre-existing for every_CREW_SECRET_LEAVEStrust root (denied_commands.json,security_policy.jsonbehave identically), so it needs a systemic matcher hardening covering all keystone leaves, not a bespoke rule here — tracked in Keystone bash fence does not catch relative-path access after cd into the crew home #2404.Testing
TestOperatorOAuthEndpointExtension(fail-soft ×5, hostile hosts ×16, hostile paths ×9, malformed entries ×5, cap bounds acceptance AND iteration, exact-match negatives ×5 incl. lookalike suffix, credentials-still-rejected ×2, general redactors unaffected, SEL emit/no-emit/dedupe/fail-open ×4, keystone fence read+write under both home prefixes, corpus contract both directions ×6, re-read contract).isort --check-only,flake8,mypy(856 files),python -m pytest(39,568 passed; remaining failures are this host's pre-existing environment baseline — verified identical failure families on unmodified main, all pass standalone), brand gate, docs-lint.Closes #1341