Repository navigation
fix(security): keep AWS access key IDs visible and redact signed-link signatures - #22757
bolichen97 wants to merge 4 commits into
Conversation
Tests that use an AWS access key ID as their stand-in for "a credential" now use a fake token the redactor treats as a secret (a GitLab- or Slack-shaped fake, or a labelled fake secret key). No product code changes; every test passes on main before and after.
Tests that use an AWS access key ID as their stand-in for "a credential" now use a fake token the redactor treats as a secret (a GitLab- or Slack-shaped fake, or a labelled fake secret key). No product code changes; every test passes on main before and after.
Built-in app tests that use an AWS access key ID as their stand-in for "a credential" now use a GitLab-shaped fake token the redactor treats as a secret. No product code changes; every test passes on main before and after.
7e25ceb to
e3c5f32
Compare
Sensitive Change ReviewSensitive files changed:
|
UX Review (Opus 5.5) — ✅ PASSUX-level review of UX-Verdict: PASS Nothing to check: no control, label or layout is added. The only visible effect is which substrings the redactors mask in text that already renders. Notes
[UX-REVIEWED] 289c3b6 Description read[DESCRIPTION-READ] 11feeb405250a56b33ec83b76d344b47c8c16ad170957ddca9c5b534fdb91557 That sha256 names everything this verdict read: the PR description, plus the 3 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
First Principles Review (Opus 5.5) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Nothing blocks: the maintainer's tier decision in Notes
What this change shipsInventory (10 items) — 6 justifiedIntent: let investigators see AWS key IDs on every surface while a signed link never exposes a working signature. This is a FIX.
[FIRST-PRINCIPLES-REVIEWED] 289c3b6 Description read[DESCRIPTION-READ] e283239348fce7310d324f0ab74fea9402d736c2a617f5f817923ae78414ad5f That sha256 names the PR description this verdict read. Recompute it with |
Design Review (Opus 5.5) — ✅ PASSDesign-level review of Design-Verdict: PASS Nothing blocks: the maintainer decision Notes
[DESIGN-REVIEWED] 289c3b6 Description read[DESCRIPTION-READ] 54efd2cdd2d5fd2dc6fd3d418944f9e68b0f5375fa86f005131ef126019a5835 That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
Opus 5.5 Review — 🔴 changes requested (blocking)Reviewed BLOCKING: the deploy scan now refuses, with no override, pages that only mention the AWS signing header or BLOCKING — src/kiro_crew/deploy/scan.py:184 [BLOCK-MERGE] 289c3b6 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Security Scope Review -- 🔴 could not complete (fails closed)No scope verdict for |
GPT 6.1 Review — 🔴 changes requested (blocking)GPT 6.1 found at least one blocking issue that must be resolved before merging This comment is updated in place on each push. BLOCKING -- src/kiro_crew/security/redaction_entropy.py:643 -- Encoded signing names bypass base64 credential detection
BLOCKING -- src/kiro_crew/security/redaction.py:518 -- HTML character references leave signed links intact
[BLOCK-MERGE] 289c3b6 Adjudication (Opus 5.5) — is blocking on each finding proportionate?F1 (fenced): Credential-exposure class, but only for input that real software doesn't produce. The decoded-chunk check at F2 (fenced): Same class. The value class 🏷️ Fenced finding(s) machine-flagged as likely edge caseThe security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override. The annotate-only pass judged the condition combination each one requires extreme, and pre-drafted the override rationale below. A repository writer must independently verify a rationale before posting it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop.
False positive or not applicable? A repository writer can comment: |
e3c5f32 to
db42ef2
Compare
Sensitive change review
|
db42ef2 to
acabdcf
Compare
Sensitive change review
|
acabdcf to
0759670
Compare
Sensitive change review
|
0759670 to
bcde9f0
Compare
Sensitive change review
|
… signatures An AWS access key ID (AKIA / ASIA) is Identifier tier: it names a key and cannot sign a request, so no surface redacts it or blocks on it. The secret access key and session token beside it stay Secret tier. Presigned and signed links are Bearer link tier keyed on the signing part: the signature and session-token parameter values are removed, and a SigV4 request signature is removed wherever it appears, while the key ID inside the link stays visible.
bcde9f0 to
289c3b6
Compare
Sensitive change review
|
Problem / Motivation
Goal: An AWS access key ID stays visible on every surface, while a presigned or signed link never shows a working signature.
An access key ID (
AKIA…/ASIA…) names a key and cannot sign a request. Kiro Crew redacts it anyway, on every surface: chat, logs, the audit log, metrics, channel messages, blocked links, the decisions gate and the frontend. Investigators who join logs on the key ID see a placeholder instead, and the decisions gate refuses a request that merely names one.Why it matters
The key ID is the join key for incident investigation (CloudTrail rows,
aws sts get-caller-identity, credential reports). Hiding it makes those logs unusable while protecting nothing, because the key ID cannot sign. The design in #22552 (section 3, question 1) moves it to the Identifier tier.Removing the key ID from redaction would, by itself, also make presigned links whole again: today the redacted key ID inside
X-Amz-Credentialis what breaks such a link. So this change redacts the signing part of the link at the same time.Not a goal
ABIA/ACCAids: they stay redacted in the log filter until their tier is confirmed.What changed (motivation → approach → change)
Every rule decided on its own that a key ID was a credential. This change removes the key-ID rule, bare and labelled, from each surface that had one, so the key ID is treated the same everywhere.
credential_patterns.AWS_KEY_IDstays as a structural anchor for the presigned-URL validators. The rule tier table carries no identifier rule, because an identifier is never redacted.AKIA…/ASIA…X-Amz-Signature=,Signature=(URL)X-Amz-Security-Token=(URL)Signature=<64 hex>(header)Signed links are Bearer link tier keyed on the signing part. The token-parameter pass now also removes the values of
X-Amz-Signature,SignatureandX-Amz-Security-Token. Its name matching (case, HTML entities, percent escapes, stream hold-back) is shared and built as a letter trie, so the extra names cost little. The credential pass also removes a SigV4 request signature afterSignature=and keeps the label. The decisions gate still refuses a secret or a signed link, and the deploy scan reports a key ID as an overridable warning, like an account id.The surfaces changed are
security/(redaction, exfil URL rules),decisions/gate.py,log_redaction.py,sel.py,metrics/schema.py,deploy/scan.py, the built-in apps (pptx_maker,aws_controlpackaging scan,ops_mission_controlcomments),security_posture.py, and the frontendwebsite/src/utils/sanitize.ts,website/src/utils/errorReport.tsand the mochi app (website/src/apps/mochi). The tier table lives incredential_patterns.py. Comments that named a key ID as a credential example are corrected inplatform/defaults.py,platform/interfaces.py,computer_use/tools.pyanddashboard/handlers/artifacts_parts/record_serialization.py.Depends on #22592 (its commit is included until it merges) and on the test-only fixture PRs #22736, #22737 and #22750, which move tests that used a key ID as "some credential" to a secret-shaped fake. Their commits are included too; this PR's own change is the top commit. The included commits also touch
acp/runtime.py,aws_consent.py,channel.py,dashboard/chat_runner.py,discord/transport_dispatch_commands.py,knowledge/ingestion.py,mcp_discovery_config.py,mcp_tools/knowledge.py,snapshot_redact.py(the composed pass of #22592) andtesting/fake_computer_use.py(the fixture swap).Backwards compatibility
Compatible: nothing that is accepted today is refused; key IDs are refused and redacted on fewer surfaces, and signed-link signatures on more.
Reader:
security/exfil_url_rules.py:_HARD_CREDENTIAL_RE-- chat, cron, subagent -- still matches labelled secrets, SSH and PEM markers and Slack tokens;test_redaction_key_id_identifier.py.Reader:
decisions/gate.py:scrub_reason-- decision points -- still refuses secrets and signed links;test_decisions_gate.py::TestScrub::test_a_secret_or_signed_link_beside_a_key_id_is_still_refused.Reader:
apps/builtins/aws_control/crew/packaging/pipeline/scan.py:_HARD_PATTERNS-- app bundle export -- the labelled secret, vendor token and canonical detector rows still refuse;test_labelled_secret_scan.py.Reader:
apps/builtins/pptx_maker/backend/routes.py:_scanned_bitmap_bytes-- app -- the decoded-bytes scan and URI terminator check still refuse a secret in an image;test_pptx_maker_routes.py.Reader:
sel.py:_redact_text-- audit log -- runs the shared redactor;test_sel_redaction.py.Known residuals:
ABIA/ACCAids stay redacted in the log filter, pending a tier decision (Redaction: classify by what a value can do (Secret / Bearer link / Identifier) #22552, question 1).Tests
test/test_redaction_key_id_identifier.py(new): a key ID alone stays on every redactor (bare, CloudTrail row, env, credentials file, STS JSON, in a URL, streamed under its label). A key ID beside its secret keeps the ID and loses the secret and token. Presigned S3, SigV2, CloudFront and lower-case links, and a SigV4 header, lose their signing values. A stream split inside a presigned URL emits no signing value. The log filter and metrics keep a key ID. All 21 fail on the base and pass here.test_decisions_gate.py: a state naming only a key ID is not refused; a key ID beside a secret, a presigned link and a signed link are.test_credential_patterns.py,test_credential_prefilter.py,test_redaction_tiers.py: the shared spellings, the branch samples (the new signature branch and its prefilter anchor) and the tier table follow the new rules.test_log_redaction.py,test_sel_redaction.py,test_deploy_round3_fixes.py,test_deploy_round4_fixes.py,test_security.py,test_security_posture.py, the packaging scan tests and the frontenderrorToAgent.test.tsxandsanitizeExfiltrationBase64Padding.test.ts: key IDs stay visible, and secrets are still caught.Run locally: every test file that mentions a key ID, a signature or the changed modules (about 33,800 tests), and the 154 related frontend test files. The only reds also fail on the base on this host:
test_design_tweak_backend.py,test_design_tweak_devproc.py,test_prompt_override_encoding.pyandMarkdownRendererCoverage.test.tsx. No test here waits, sleeps or depends on order; each new test is a pure function of fixed inputs.Manual verification
N/A - every surface changed is covered by unit tests on the redactors themselves.
Why no screenshot: the frontend change is in sanitizer and error-report helpers that render nothing new; a key ID simply stops being replaced.
Related Issues
Part of #22552
Pattern harvest
Rule candidate: review-prompt
Pattern: a value's sensitivity is decided per rule, so one identifier was redacted by 14 separate copies; new rules must name a tier from
credential_patterns.RULE_TIERS.Checklist
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)