From c556f2f2d8cdb53c15f2756d7c87c8cac0c9f678 Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Sun, 4 Oct 2026 12:53:01 +0300 Subject: [PATCH 01/14] feat: add AWS account ID and EC2 instance ID scanner Reads the lines a pull request adds and reports unredacted AWS account IDs (twelve digits, no digit either side) and EC2 instance IDs (`i-` plus eight or seventeen hexadecimal characters, case-insensitive). Read-only: it runs `git diff` and nothing else, and its only write is appending to $GITHUB_STEP_SUMMARY. Added lines only. `main` already carries real-looking identifiers in committed eval results and example ARNs, so a whole-file scan would fail pull requests for content their authors never wrote. The diff is read with --find-renames, without which a pure rename reports every line as added and would flag identifiers a pull request only moved. A matched identifier has an optional trailing `_` stripped before the allowlist comparison, which is the skill evaluation tool's redaction suffix. A \b-anchored pattern would not match the suffixed form at all, since `_` is a word character, so the suffix is consumed by the pattern and normalized away. Four suppressors: the allowlist file's own added lines are skipped; an entry in .github/aws-identifier-allowlist.json, which must carry a reason; a twelve-digit run that is a bare JSON value rather than a quoted string, in a .json/.jsonl path, since AWS always writes an account ID as a string; and an `aws-id-ok: ` comment on the line, in the file types that have a comment syntax. The marker is checked after the patterns, so a line that merely names it and holds no identifier is left alone. The allowlist parser fails closed: bad JSON, a missing key, or a blank reason grants nothing and exits 2. Exit 0 clean (a pull request adding no lines included), 1 on findings, 2 on a broken allowlist or an unreadable diff. `--self-check` asserts every allowlist entry is suppressed bare and `_N`-suffixed. Verified against real history (origin/main~1...origin/main reports the four real-looking account IDs the last merge added to eval fixtures and nothing else) and against 15 diff fixtures held in /tmp: known-bad account and instance IDs fail; all 12 seed values pass bare and suffixed; `322122547200` as a bare JSON value passes while the same digits quoted fail; a marked line passes; removed lines, context lines, the `+++` header and a pure rename produce nothing; 60 findings print 50 and a remainder count; four malformed allowlists each exit 2. --- .github/aws-identifier-allowlist.json | 18 + .github/scripts/scan_aws_identifiers.py | 731 ++++++++++++++++++++++++ 2 files changed, 749 insertions(+) create mode 100644 .github/aws-identifier-allowlist.json create mode 100755 .github/scripts/scan_aws_identifiers.py diff --git a/.github/aws-identifier-allowlist.json b/.github/aws-identifier-allowlist.json new file mode 100644 index 00000000..df010edb --- /dev/null +++ b/.github/aws-identifier-allowlist.json @@ -0,0 +1,18 @@ +{ + "account_ids": { + "123456789012": "AWS documentation example account ID", + "111122223333": "AWS documentation example account ID", + "444455556666": "AWS documentation example account ID", + "123456789010": "Example account ID used throughout this repository", + "012345678901": "Skill evaluation tool redaction placeholder for account IDs", + "753240598075": "AWS-owned account that publishes the Lambda Web Adapter layer; referenced legitimately in SAM templates", + "602401143452": "AWS-owned Amazon EKS ECR registry account for us-east-1" + }, + "instance_ids": { + "i-1234567890abcdef0": "Skill evaluation tool redaction placeholder for EC2 instance IDs", + "i-0123456789abcdef0": "AWS documentation example instance ID", + "i-0aaaaaaaaaaaaaaaa": "Obvious placeholder, repeated character", + "i-0a1b2c3d4e5f67890": "Obvious placeholder, ascending digit pattern", + "i-0fedcba9876543210": "Obvious placeholder, descending hexadecimal pattern" + } +} diff --git a/.github/scripts/scan_aws_identifiers.py b/.github/scripts/scan_aws_identifiers.py new file mode 100755 index 00000000..62919650 --- /dev/null +++ b/.github/scripts/scan_aws_identifiers.py @@ -0,0 +1,731 @@ +#!/usr/bin/env python3 +"""Scan the lines a pull request adds for AWS account IDs and EC2 instance IDs. + +The point of the check is to keep unredacted identifiers out of the repository: +a twelve-digit AWS account ID or an EC2 instance ID committed in a skill, an +eval result, a workflow or a document names a real resource in a real account, +and once it is on ``main`` it is in the public history for good. + +**Scope: only the lines the pull request adds.** The diff is read with +``git diff --find-renames --unified=0 ...``; only the ``+`` +lines of that diff are scanned, and removed lines, context lines and the +``+++``/``---`` file headers are all ignored. Scanning whole files, or the whole +tree, was rejected deliberately: ``main`` already carries real-looking +identifiers — committed eval results, example ARNs in skill documentation, a +handful of values in MCP comments — so a whole-file scan would fail pull +requests for content their authors never wrote, and the only way to go green +would be to clean up someone else's lines. Cleaning those up is a separate +change; this check stops new ones arriving. + +``--find-renames`` is part of that scope and not a nicety. Without it a pure +file rename is reported as a deletion plus an addition of every line, so moving +a file that already contains identifiers would fail the check for a change that +added nothing. With it, a pure rename produces no added lines at all. + +Two patterns are matched: + +* **AWS account ID** — exactly twelve digits with no digit on either side, so + ``arn:aws:iam::123456789012:role/Example`` matches while a thirteen-digit + number does not. +* **EC2 instance ID** — ``i-`` followed by either eight hexadecimal characters + (the old form) or seventeen (the current form), case-insensitive, with no + other hexadecimal word character on either side. + +**Redaction suffixes.** The skill evaluation tool redacts identifiers by +replacing each distinct original with a placeholder plus an index, as in +``012345678901_2`` and ``i-1234567890abcdef0_2``, where the trailing number +tells two different originals apart. A matched identifier therefore has an +optional trailing ``_`` stripped from it before it is compared against +the allowlist, so the bare placeholder and every indexed form of it are treated +alike. The finding reports the token as written; only the comparison is +normalized. + +**Four ways a match is suppressed:** + +1. The file being scanned *is* the allowlist. Added lines in + ``.github/aws-identifier-allowlist.json`` are skipped, so adding an entry to + the allowlist does not trip the scanner on that very entry. +2. An entry in the allowlist file, which must carry a reason. +3. A twelve-digit run that is a bare JSON number rather than a quoted JSON + string, in a ``.json`` or ``.jsonl`` file. AWS always writes an account ID + as a string, so a bare number of that length is a byte count or a similar + measurement, never an account ID. +4. An ``aws-id-ok: `` marker on the line. Honored in file types that + have a comment syntax — ``.md``, ``.yaml``, ``.yml``, ``.py``, ``.sh``, + ``.ts``, ``.js``, ``.html``, ``.htm``, ``.xml``. JSON has no comment syntax, + which is why the allowlist is the route for JSON content. + +The allowlist-path skip comes first, and the marker is checked last, after the +patterns have run: a line naming the marker but holding no identifier suppresses +nothing, so it is left alone rather than treated as an opt-out. + +Exit codes: ``0`` when nothing was found, including a pull request that adds no +lines at all; ``1`` when there are findings; ``2`` when the scan could not be +trusted — a malformed allowlist, an unreadable diff file, or a failing ``git`` +command. The gate workflow fails the job on any non-zero code; the split exists +so a contributor can tell a leaked identifier from a broken allowlist. +""" + +from __future__ import annotations + +import argparse +import json +import os +import re +import subprocess +import sys +from dataclasses import dataclass +from pathlib import Path + + +# Twelve digits with no digit on either side. The lookarounds are what stop a +# longer run of digits from matching a twelve-digit window inside itself, so +# neither a thirteen-digit timestamp nor a long id is reported. +# +# The optional ``_`` group is the skill evaluation tool's redaction +# suffix. It is written as a lookahead-guarded optional group rather than with +# ``\b`` on purpose: ``_`` is a word character, so ``\b``-anchored patterns do +# not match the suffixed form at all, and a suffixed placeholder would slip +# past the allowlist comparison as if it were a different value. Here the group +# consumes the suffix, and the suffix is then stripped before the comparison. +ACCOUNT_RE = re.compile(r"(?` comment on the line itself. The allowlist " + "is the route for JSON files, which have no comment syntax. See the " + '"Scanning for AWS Identifiers" section of CONTRIBUTING.md.' +) + + +@dataclass +class Finding: + path: str + line: int + kind: str + # The token exactly as it appears on the line, redaction suffix included, so + # the message matches what the author will search for in the file. + value: str + + +def _repo_root() -> Path: + """Resolve the repository root from this script's location (.github/scripts/).""" + return Path(__file__).resolve().parents[2] + + +def _git(repo_root: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", *args], cwd=repo_root, capture_output=True, text=True + ) + + +def _warn(message: str) -> None: + """Report a non-fatal problem, as an annotation under Actions.""" + if os.environ.get("GITHUB_ACTIONS"): + print(f"::warning::{message.replace(chr(10), ' ')}") + else: + print(f"warning: {message}", file=sys.stderr) + + +def _error(message: str) -> None: + """Report a fatal problem, as an annotation under Actions.""" + if os.environ.get("GITHUB_ACTIONS"): + print(f"::error::{message.replace(chr(10), ' ')}") + else: + print(f"error: {message}", file=sys.stderr) + + +def read_diff(repo_root: Path, base_ref: str, head_ref: str) -> str | None: + """Return the unified diff of what ``head_ref`` adds, or None on failure. + + The flags each earn their place: + + * ``...`` — a three-dot diff, against the merge base, so + commits that landed on the base branch after the pull request branched are + not attributed to the pull request. + * ``--find-renames`` — without it a pure rename is reported as every line + being added, which would flag identifiers the pull request only moved. + * ``--unified=0`` — only added lines are scanned, so context lines are + output nobody reads. + * ``core.quotePath=false`` — a non-ASCII path is printed as itself in the + ``+++`` header rather than octal-escaped, which keeps the reported file + path usable. + + A failing three-dot diff is retried with two dots, which covers the case of + a shallow clone or an unrelated history where no merge base exists. When + both fail the caller exits with the configuration failure code: a scan that + cannot read the diff has to fail loudly rather than report "nothing found". + """ + flags = ( + "-c", + "core.quotePath=false", + "diff", + "--find-renames", + "--unified=0", + "--no-color", + ) + + result = _git(repo_root, *flags, f"{base_ref}...{head_ref}") + if result.returncode == 0: + return result.stdout + + _warn( + f"git diff {base_ref}...{head_ref} failed " + f"({result.stderr.strip()}); retrying without the merge base" + ) + result = _git(repo_root, *flags, base_ref, head_ref) + if result.returncode == 0: + return result.stdout + + _error( + f"git diff {base_ref} {head_ref} failed: {result.stderr.strip()}. " + "The scan cannot run without a diff." + ) + return None + + +def read_diff_file(path: str) -> str | None: + """Read a saved diff from ``path``, or from stdin when ``path`` is ``-``. + + This is the local-testing entry point: with a saved diff the scanner runs no + ``git`` command at all, which makes it easy to exercise against a fixture. + """ + if path == "-": + return sys.stdin.read() + try: + return Path(path).read_text(encoding="utf-8", errors="replace") + except OSError as exc: + _error(f"cannot read diff file `{path}`: {exc}") + return None + + +def added_lines(diff_text: str) -> list[tuple[str, int, str]]: + """Parse a unified diff into (path, line number in the new file, line text). + + Only added lines are returned, and the leading ``+`` is stripped from the + text. The ``+++``/``---`` headers are skipped rather than scanned, so a file + path that happens to contain a twelve-digit run is never reported as + content. + + The headers are recognised only outside a hunk. Unified diff is ambiguous + here: an added line whose own text starts with ``++ `` appears in the diff + as ``+++ ``, exactly like a file header. Tracking whether the parser is + inside a hunk resolves it, since a header never appears inside one. + + The line counter comes from the hunk header and advances on added and + context lines but not on removed ones, which is how a line number in the new + file is reached. Context lines are counted even though ``--unified=0`` emits + none, so that a fixture diff generated with default context parses correctly + too. + """ + hunk_re = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@") + + entries: list[tuple[str, int, str]] = [] + path: str | None = None + line_number = 0 + in_hunk = False + + for raw in diff_text.splitlines(): + if raw.startswith("diff --git "): + path, in_hunk = None, False + continue + if not in_hunk and raw.startswith("+++ "): + target = raw[4:].strip() + # /dev/null is a deleted file: its hunks hold only removed lines. + path = None if target == "/dev/null" else _strip_diff_prefix(target) + continue + if not in_hunk and raw.startswith("--- "): + continue + match = hunk_re.match(raw) + if match: + line_number = int(match.group(1)) + in_hunk = True + continue + if not in_hunk: + # index / mode / similarity / rename / binary header lines. + continue + if raw.startswith("+"): + if path is not None: + entries.append((path, line_number, raw[1:])) + line_number += 1 + elif raw.startswith("-"): + pass # removed line: not part of what the pull request adds + elif raw.startswith("\\"): + pass # "\ No newline at end of file" + else: + line_number += 1 # context line + + return entries + + +def _strip_diff_prefix(target: str) -> str: + """Drop the ``b/`` that ``git diff`` puts in front of the new file's path.""" + return target[2:] if target.startswith(("a/", "b/")) else target + + +def load_allowlist(path: Path) -> tuple[set[str], set[str], list[str]]: + """Read the allowlist, returning (account IDs, instance IDs, problems). + + The shape is two required objects, each mapping an identifier to the reason + it is safe to publish:: + + { + "account_ids": {"123456789012": "AWS documentation example"}, + "instance_ids": {"i-0123456789abcdef0": "AWS documentation example"} + } + + The reason is mandatory. An allowlist entry waives a check on published + content, and the reason is what a reviewer judges the waiver on. + + **The parser fails closed**: an unreadable file, invalid JSON, a top level + that is not an object, a missing key, a value that is not an object, a key + that does not look like the identifier it claims to be, or a reason that is + missing or blank — each is reported as a problem, and when there is any + problem the function grants no allowance at all. + + That is the opposite of how ``parse_label_names`` in validate_skill_evals.py + treats a mangled value, and deliberately so. The label that script reads can + only make its check stricter, so treating a mangled one as absent is safe. + An allowlist only ever makes this check *weaker*. Degrading a broken + allowlist to "allow nothing" would be noisy but safe, and degrading it to + "allow everything" would silently waive a real leak — so rather than guess, + a broken allowlist is itself an error, reported with the reason and exiting + with the configuration failure code. + """ + rel = path.name + try: + raw = json.loads(path.read_text(encoding="utf-8")) + except OSError as exc: + return set(), set(), [f"cannot read allowlist `{path}`: {exc}"] + except (json.JSONDecodeError, UnicodeDecodeError) as exc: + return set(), set(), [f"`{rel}` is not valid JSON ({exc})"] + + if not isinstance(raw, dict): + return ( + set(), + set(), + [f"`{rel}` must be a JSON object with `account_ids` and `instance_ids`"], + ) + + problems: list[str] = [] + collected: dict[str, set[str]] = {"account_ids": set(), "instance_ids": set()} + patterns = {"account_ids": ACCOUNT_RE, "instance_ids": INSTANCE_RE} + + for key, pattern in patterns.items(): + if key not in raw: + problems.append(f"`{rel}` is missing the `{key}` object") + continue + body = raw[key] + if not isinstance(body, dict): + problems.append( + f"`{rel}` key `{key}` must be an object mapping identifier to reason" + ) + continue + for identifier, reason in body.items(): + if not isinstance(identifier, str) or not pattern.fullmatch(identifier): + problems.append( + f"`{rel}` entry `{identifier}` under `{key}` is not a valid " + "identifier of that type" + ) + continue + if not isinstance(reason, str) or not reason.strip(): + problems.append( + f"`{rel}` entry `{identifier}` needs a non-empty reason " + "explaining why the value is safe to publish" + ) + continue + collected[key].add(_normalize(identifier)) + + if problems: + # Fail closed: no allowance at all while the file is broken. + return set(), set(), problems + + return collected["account_ids"], collected["instance_ids"], [] + + +def _normalize(value: str) -> str: + """Lowercase an identifier and drop any ``_`` redaction suffix. + + Comparisons against the allowlist use this form, so ``i-1234567890abcdef0``, + ``I-1234567890ABCDEF0`` and ``i-1234567890abcdef0_2`` all resolve to the one + allowlist entry. + """ + return re.sub(r"_[0-9]+$", "", value).lower() + + +def _inside_json_string(line: str, pos: int) -> bool: + """True when ``pos`` in ``line`` falls inside a double-quoted JSON string. + + Counts the double quotes before ``pos`` that are not escaped — a quote + preceded by an odd run of backslashes is escaped — and returns whether that + count is odd. + + Used only for account IDs in ``.json``/``.jsonl`` files: AWS always writes + an account ID as a JSON string, so a twelve-digit run appearing as a bare + JSON number is a byte count or a similar measurement rather than an account + ID. Instance IDs are never bare numbers, so the exclusion never applies to + them. + + The limits, stated plainly: this reads one line in isolation and is not a + JSON parse. A fragment with unbalanced quotes, a value split across lines, + or a string containing an unescaped-looking quote can be judged wrongly, and + the file type is taken from the extension rather than from the content. It + is a heuristic that trades a small chance of missing an identifier for not + failing every pull request that adds a twelve-digit byte count to an eval + result. + """ + quotes = 0 + index = 0 + while index < pos: + if line[index] == "\\": + index += 2 # skip the escaped character, whatever it is + continue + if line[index] == '"': + quotes += 1 + index += 1 + return quotes % 2 == 1 + + +def _excluded_paths(repo_root: Path, allowlist: Path) -> set[str]: + """Repository-relative paths whose added lines are never scanned. + + The allowlist file itself, so that adding ``753240598075`` to it does not + make the scanner report that very entry. Both the file in use and the + default are listed, so a local run against a temporary allowlist still + skips the committed one, which is the file a pull request would be editing. + """ + excluded = {DEFAULT_ALLOWLIST} + try: + excluded.add(allowlist.resolve().relative_to(repo_root).as_posix()) + except ValueError: + pass # a --allowlist outside the repository has no diff path to skip + return excluded + + +def scan_added_lines( + entries: list[tuple[str, int, str]], + accounts: set[str], + instances: set[str], + excluded: set[str], +) -> tuple[list[Finding], list[str]]: + """Find identifiers in added lines, returning (findings, warnings). + + The marker is applied *after* the patterns rather than before. A line that + mentions ``aws-id-ok`` but carries no identifier is suppressing nothing, so + it neither counts as an opt-out nor earns the missing-reason warning — + without which every line of documentation or source that names the marker, + this file's own ``MARKER`` constant included, would be warned about. + """ + findings: list[Finding] = [] + warnings: list[str] = [] + + for path, line_number, text in entries: + if path in excluded: + continue + + suffix = Path(path).suffix.lower() + is_json = suffix in JSON_SUFFIXES + + candidates: list[Finding] = [] + + for match in ACCOUNT_RE.finditer(text): + if _normalize(match.group(1)) in accounts: + continue + if is_json and not _inside_json_string(text, match.start()): + continue # a bare JSON number, so not an account ID + candidates.append( + Finding(path, line_number, ACCOUNT_KIND, match.group(0)) + ) + + for match in INSTANCE_RE.finditer(text): + if _normalize(match.group(1)) in instances: + continue + candidates.append( + Finding(path, line_number, INSTANCE_KIND, match.group(0)) + ) + + if not candidates: + continue + + if suffix in COMMENT_BEARING_SUFFIXES and MARKER in text: + if not MARKER_WITH_REASON_RE.search(text): + warnings.append( + f"{path}:{line_number} suppresses the AWS identifier scan with " + f"a bare `{MARKER}` marker; write `{MARKER}: ` so a " + "reviewer can see why the value is safe to publish" + ) + continue + + findings += candidates + + return findings, warnings + + +def _annotate(findings: list[Finding], warnings: list[str]) -> None: + """Emit GitHub Actions annotations, one per finding. + + Each finding is anchored with ``file=`` and ``line=`` so it appears inline on + the pull request's diff, next to the line that added the identifier. + Annotations are single-line, so any embedded newline is flattened. + """ + if not os.environ.get("GITHUB_ACTIONS"): + return + for finding in findings[:MAX_PRINTED_FINDINGS]: + message = ( + f"{finding.kind} `{finding.value}` was added on this line. {REMEDIATION}" + ) + print( + f"::error file={finding.path},line={finding.line}," + f"title=AWS identifier::{message.replace(chr(10), ' ')}" + ) + for warning in warnings: + print(f"::warning title=AWS identifier::{warning.replace(chr(10), ' ')}") + + +def _write_summary( + findings: list[Finding], warnings: list[str], problems: list[str] +) -> None: + summary_path = os.environ.get("GITHUB_STEP_SUMMARY") + if not summary_path: + return + + lines = ["## AWS identifier scan", ""] + + if problems: + lines.append("### Allowlist problems") + lines.append("") + lines += [f"- {problem}" for problem in problems] + lines += [ + "", + f"No allowance was granted while `{DEFAULT_ALLOWLIST}` is broken, so " + "findings below may include values the allowlist is meant to cover. " + "Fix the file and re-run.", + "", + ] + + if not findings: + lines.append( + "No AWS account IDs or EC2 instance IDs were added by this pull request." + ) + else: + lines.append(f"{len(findings)} finding(s) on lines this pull request adds:") + lines.append("") + lines.append("| File | Line | Kind | Value |") + lines.append("| --- | --- | --- | --- |") + for finding in findings[:MAX_PRINTED_FINDINGS]: + lines.append( + f"| `{finding.path}` | {finding.line} | {finding.kind} " + f"| `{finding.value}` |" + ) + remainder = len(findings) - MAX_PRINTED_FINDINGS + if remainder > 0: + lines.append(f"| … and {remainder} more | | | |") + lines += ["", REMEDIATION] + + if warnings: + lines += ["", "### Warnings", ""] + lines += [f"- {warning}" for warning in warnings] + + with open(summary_path, "a", encoding="utf-8") as fh: + fh.write("\n".join(lines) + "\n") + + +def self_check(accounts: set[str], instances: set[str]) -> list[str]: + """Confirm every allowlist entry is suppressed bare and with a ``_N`` suffix. + + The redaction suffix is the one part of the matching that is easy to get + silently wrong: a ``\\b``-anchored pattern does not match + ``i-1234567890abcdef0_3`` at all, so a regression there would stop + *reporting* suffixed placeholders rather than start reporting them, and no + pull request would fail to reveal it. This walks every entry in the + allowlist in use, builds a synthetic added line for the bare form and for a + ``_7``-suffixed form, scans it, and reports any that produced a finding. + + Run it with ``--self-check``. It reads nothing but the allowlist and runs no + ``git`` command. + """ + failures: list[str] = [] + for identifier in sorted(accounts | instances): + for value in (identifier, f"{identifier}_7"): + entries = [("docs/self-check.md", 1, f"value {value} on a line")] + findings, _ = scan_added_lines(entries, accounts, instances, set()) + if findings: + failures.append( + f"allowlisted `{identifier}` was still reported as " + f"`{findings[0].value}` when written as `{value}`" + ) + return failures + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--base-ref", + default="origin/main", + help="Base ref of the pull request (default: origin/main).", + ) + parser.add_argument( + "--head-ref", + default="HEAD", + help="Head ref of the pull request (default: HEAD).", + ) + parser.add_argument( + "--allowlist", + default=DEFAULT_ALLOWLIST, + metavar="PATH", + help=( + "Allowlist of identifiers that are safe to publish, each with a " + f"reason (default: {DEFAULT_ALLOWLIST}). A relative path is resolved " + "against the repository root." + ), + ) + parser.add_argument( + "--diff-file", + default=None, + metavar="PATH", + help=( + "Scan a saved unified diff instead of running git; `-` reads stdin. " + "Intended for testing the scanner against a fixture." + ), + ) + parser.add_argument( + "--self-check", + action="store_true", + help=( + "Scan nothing; instead confirm every allowlist entry is suppressed " + "both bare and with a `_N` redaction suffix, and exit non-zero if " + "any is not." + ), + ) + args = parser.parse_args(argv) + + repo_root = _repo_root() + + allowlist = Path(args.allowlist) + if not allowlist.is_absolute(): + allowlist = repo_root / allowlist + + accounts, instances, problems = load_allowlist(allowlist) + for problem in problems: + _error(problem) + + if args.self_check: + if problems: + return 2 + failures = self_check(accounts, instances) + for failure in failures: + _error(failure) + total = len(accounts) + len(instances) + print( + f"Self-check: {total} allowlist entr(ies), bare and `_N`-suffixed, " + f"{len(failures)} failure(s)." + ) + return 1 if failures else 0 + + if args.diff_file: + diff_text = read_diff_file(args.diff_file) + else: + diff_text = read_diff(repo_root, args.base_ref, args.head_ref) + if diff_text is None: + return 2 + + entries = added_lines(diff_text) + if not entries: + print("This pull request adds no lines; nothing to scan.") + _write_summary([], [], problems) + return 2 if problems else 0 + + findings, warnings = scan_added_lines( + entries, accounts, instances, _excluded_paths(repo_root, allowlist) + ) + + for finding in findings[:MAX_PRINTED_FINDINGS]: + print( + f"FOUND {finding.path}:{finding.line} {finding.kind} {finding.value}" + ) + remainder = len(findings) - MAX_PRINTED_FINDINGS + if remainder > 0: + print(f" … and {remainder} more finding(s) not printed.") + + for warning in warnings: + _warn(warning) + + _annotate(findings, warnings) + _write_summary(findings, warnings, problems) + + print( + f"\n{len(entries)} added line(s) scanned: {len(findings)} finding(s).", + file=sys.stderr, + ) + + if problems: + print( + f"\nThe allowlist `{DEFAULT_ALLOWLIST}` is broken, so no identifier was " + "allowed. Fix the problems reported above and re-run.", + file=sys.stderr, + ) + return 2 + if findings: + print(f"\n{REMEDIATION}", file=sys.stderr) + return 1 + + print("No AWS account IDs or EC2 instance IDs were added by this pull request.") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) From 945acffbaee3006270a58b2a0aa1329c9d65aeff Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Sun, 4 Oct 2026 12:53:34 +0300 Subject: [PATCH 02/14] feat: add the Scan for AWS identifiers pull request check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Runs the scanner on every pull request and reports a status. Strictly read-only: it checks out into the runner's throwaway workspace, runs `git fetch` and the scanner, and writes only into the artifact directory, $GITHUB_OUTPUT and $GITHUB_STEP_SUMMARY. No `git` write command, no pull request mutation, `permissions: contents: read` and nothing more. The job id and its display name are the identical string `scan-aws-identifiers`, because the display name is the status check context branch protection matches on and contexts are global across the repository. Three deliberate omissions, each commented in the file. No `paths:` filter: a required check that never runs reports pending forever and would permanently block pull requests touching nothing it names, so the job always runs and the scanner exits 0 when nothing was added. No `if:` guard on the job: a skipped job reports "skipped", which branch protection can treat as satisfied, so a guard keyed on anything a contributor controls would be a merge bypass. And `continue-on-error: true` comes before the artifact upload, with the job failed by a later step, because otherwise the artifact never uploads in the failing case — exactly the case that needs a label. `labeled` is in the trigger list so a maintainer can force a run on an already-open pull request; GitHub cannot filter `labeled` by label name. A label applied by GITHUB_TOKEN raises no `labeled` event, so the companion labeler cannot re-trigger this check. Every event-payload value goes through `env:` and is quoted on use; no `${{ }}` appears in a `run:` line. Verified by parsing the file with PyYAML: single job key equal to its `name:`, permissions read-only, no `paths:`, no `if:`. A red check does not block merging until an admin adds the context `scan-aws-identifiers` to the required status checks for `main`. --- .github/workflows/scan-aws-identifiers.yml | 137 +++++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 .github/workflows/scan-aws-identifiers.yml diff --git a/.github/workflows/scan-aws-identifiers.yml b/.github/workflows/scan-aws-identifiers.yml new file mode 100644 index 00000000..5781b19a --- /dev/null +++ b/.github/workflows/scan-aws-identifiers.yml @@ -0,0 +1,137 @@ +name: Scan for AWS identifiers + +# Gates pull requests on the lines they add carrying no unredacted AWS account +# IDs (twelve digits) or EC2 instance IDs (`i-` plus eight or seventeen +# hexadecimal characters). An identifier committed here names a real resource in +# a real account, and once it is on `main` it is in the public history for good. +# +# Only the lines the pull request *adds* are scanned (see +# .github/scripts/scan_aws_identifiers.py). `main` already carries real-looking +# identifiers in committed eval results and in example ARNs, so a whole-file +# scan would fail pull requests for content their authors never wrote, and the +# only way to go green would be to clean up someone else's lines. +# +# This job is strictly read-only. It checks the pull request out into the +# runner's throwaway workspace, reads the diff, and reports a status. It runs no +# `git` write command, pushes nothing, edits no pull request, and writes nothing +# into the repository tree — only into the artifact directory on the runner, +# $GITHUB_OUTPUT and $GITHUB_STEP_SUMMARY. +# +# Three ways to resolve a finding, all in the author's hands: +# +# 1. Redact the value. +# 2. Add it to .github/aws-identifier-allowlist.json with a reason explaining +# why it is safe to publish. This is the route for JSON files, which have +# no comment syntax. The allowlist fails closed: a malformed entry fails +# the check rather than silently waiving one. +# 3. Put an `aws-id-ok: ` comment on the line, in a file type that has +# a comment syntax. +# +# A companion workflow (label-aws-identifiers-result.yml) applies or removes the +# `needs-id-redact` label based on this job's outcome. It has to be a separate +# workflow_run job because GITHUB_TOKEN is read-only for pull_request events +# from forks, which is where most contributions here come from. +# +# NOTE: a red check does not block merging on its own. An admin must add the +# job's name ("scan-aws-identifiers") to the required status checks for `main` in +# branch protection. Until then this workflow is advisory. + +on: + pull_request: + # `labeled` is included so a maintainer can force a run on an already-open + # pull request by adding any label. GitHub cannot filter `labeled` by label + # name, so this fires for every label — which is the point: a pull request + # opened before this workflow existed reports no status at all and sits + # blocked without a log, and one label gets it a run. + # + # A label applied by GITHUB_TOKEN raises no `labeled` event, so the + # companion labeling workflow cannot re-trigger this one. + types: [opened, synchronize, reopened, ready_for_review, labeled] + # Deliberately NOT filtered with `paths:`. A required status check that never + # runs is reported as pending forever, so a path filter here would + # permanently block every pull request that happens to touch nothing the + # filter names. Instead the job always runs and the scanner exits 0 when the + # pull request adds no lines. + +# Read-only: the job inspects the diff and reports a status, so pull requests +# from forks work without any elevated permissions. Labeling happens in the +# companion workflow. +permissions: + contents: read + +concurrency: + group: scan-aws-identifiers-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + # The job id and its display name are kept identical, because the display name + # is the status check context that branch protection matches on. Contexts are + # global across all workflows in the repo, so it is spelled out rather than + # left as a generic "scan" another workflow could collide with. Renaming + # either one later means updating the required checks list too. + scan-aws-identifiers: + name: scan-aws-identifiers + runs-on: ubuntu-latest + # No `if:` guard here on purpose. Skipping this job would report the required + # check as "skipped", which branch protection can treat as satisfied — so a + # guard keyed on anything a contributor controls, a label most of all, would + # let anyone unblock a failing pull request. The job always runs and always + # reports a real status; the three suppression routes above are how an author + # resolves a finding, and each of them is visible in the diff under review. + steps: + - uses: actions/checkout@v4 + with: + # Full history so the merge base with the base branch can be computed. + # The scanner diffs with three dots, which needs it. + fetch-depth: 0 + + - uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Scan the added lines for AWS identifiers + id: scan + # Never fails the step: the outcome is recorded, published for the + # labeling workflow, and only then turned into a job failure. Without + # this the artifact would not be uploaded on failure — the case that + # needs a label. + continue-on-error: true + # Every value from the event payload is passed through the environment + # rather than interpolated into the command, and quoted on use. + env: + BASE_REF: ${{ github.event.pull_request.base.ref }} + run: | + # Make sure the base branch is available locally as a remote-tracking + # ref so the merge base can be computed. + git fetch --no-tags --quiet origin \ + "+refs/heads/${BASE_REF}:refs/remotes/origin/${BASE_REF}" + + set +e + python3 .github/scripts/scan_aws_identifiers.py \ + --base-ref "origin/${BASE_REF}" \ + --head-ref HEAD + code=$? + set -e + echo "code=${code}" >> "$GITHUB_OUTPUT" + exit "${code}" + + - name: Publish outcome for the labeling workflow + env: + PR_NUMBER: ${{ github.event.pull_request.number }} + OUTCOME: ${{ steps.scan.outputs.code == '0' && 'pass' || 'fail' }} + run: | + mkdir -p aws-id-check-result + printf '%s\n' "$PR_NUMBER" > aws-id-check-result/pr-number + printf '%s\n' "$OUTCOME" > aws-id-check-result/outcome + + - uses: actions/upload-artifact@v4 + with: + name: aws-id-check-result + path: aws-id-check-result/ + retention-days: 1 + + - name: Fail the job when the scan failed + if: steps.scan.outputs.code != '0' + run: | + echo "::error::AWS identifier scan failed — see the log above." + exit 1 From cee8aed30be26629f34b58803d923836ac0bed12 Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Sun, 4 Oct 2026 12:53:35 +0300 Subject: [PATCH 03/14] feat: add the needs-id-redact labeler for the AWS identifier check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies `needs-id-redact` when "Scan for AWS identifiers" fails and removes it once the check passes, so a pull request is never left carrying a stale label. A separate workflow_run workflow because GITHUB_TOKEN is read-only for pull_request events from forks whatever the `permissions:` block says, and this repository is overwhelmingly fork-driven — labeling from the check job itself would silently do nothing for most pull requests. The pull request number comes from the check's artifact, since `workflow_run.pull_requests` is empty for fork pull requests, which is what the `actions: read` permission is for. The repository's default workflow permission is read-only, which is the recommended baseline and must stay that way, so `pull-requests: write` is raised on this one job. It writes labels and nothing else: no checkout, no branch, no commit. A label written by GITHUB_TOKEN raises no `labeled` event, so this workflow cannot re-trigger the check it reports on. Removal is a checked lookup rather than `--remove-label || true`. Suppressing the absent-label error also suppresses every other error, including a missing-GH_REPO failure, and the step then reports success while doing nothing — the bug label-skill-evals-result.yml was fixed for. This file starts from the fix, so an absent label stays benign while a real API failure fails the job. This workflow cannot be smoke-tested on the pull request that introduces it: workflow_run only fires for a file already on the default branch. Verifying it is a post-merge step, noted in the file's header. Verified by parsing with PyYAML: `workflows:` is byte-equal to the gate's `name:`, the artifact name and both filenames match on both sides, and the job permissions are exactly {contents: read, actions: read, pull-requests: write}. --- .../label-aws-identifiers-result.yml | 143 ++++++++++++++++++ 1 file changed, 143 insertions(+) create mode 100644 .github/workflows/label-aws-identifiers-result.yml diff --git a/.github/workflows/label-aws-identifiers-result.yml b/.github/workflows/label-aws-identifiers-result.yml new file mode 100644 index 00000000..935f5c15 --- /dev/null +++ b/.github/workflows/label-aws-identifiers-result.yml @@ -0,0 +1,143 @@ +name: Label AWS identifier scan result + +# Applies the `needs-id-redact` label to a pull request whose "Scan for AWS +# identifiers" check failed, and removes it once the check passes. +# +# `needs-id-redact` is an *output* of the check and is owned by this workflow +# alone. It is not something to add or remove by hand: this workflow rewrites it +# on every run of the check, so a hand-applied one disappears on the next push +# and a hand-removed one comes straight back. +# +# Why a separate workflow: for pull_request events from a fork, GITHUB_TOKEN is +# read-only regardless of the `permissions:` block, so the scanning job cannot +# label its own pull request — and most contributions to this repo come from +# forks, which would make labeling from the check job silently do nothing for +# the majority of pull requests. A workflow_run job runs in the base +# repository's context with write access, and uses the workflow definition from +# the default branch rather than from the pull request. +# +# The pull request number is read from an artifact rather than from the +# workflow_run payload, because `workflow_run.pull_requests` is empty for fork +# pull requests. +# +# A label written by GITHUB_TOKEN raises no `labeled` event, so this workflow +# cannot re-trigger the check it reports on — the two cannot loop. +# +# NOTE: this workflow cannot be smoke-tested on the pull request that introduces +# it. A workflow_run trigger only fires for a workflow file that is already on +# the default branch, so the first real run happens after merge. Verifying it is +# a post-merge step: push a commit adding an unallowlisted identifier to a +# scratch pull request, confirm the label arrives, redact the value, confirm the +# label goes away. + +on: + workflow_run: + workflows: ["Scan for AWS identifiers"] + types: [completed] + +permissions: + contents: read + +concurrency: + group: label-aws-identifiers-${{ github.event.workflow_run.id }} + cancel-in-progress: false + +jobs: + label: + runs-on: ubuntu-latest + # The repository's default workflow permission is read-only, which is the + # recommended baseline and must stay that way at the repository level. + # Elevation therefore belongs here, on the one job that needs to write, and + # is scoped to the two things it writes. + permissions: + contents: read + actions: read # download the artifact from the triggering run + pull-requests: write # add / remove the label + # A cancelled or skipped scan run has nothing to report. + if: >- + github.event.workflow_run.conclusion == 'success' || + github.event.workflow_run.conclusion == 'failure' + steps: + - name: Download the check outcome + id: artifact + uses: actions/download-artifact@v4 + continue-on-error: true + with: + name: aws-id-check-result + path: result + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ github.token }} + + - name: Read outcome + id: outcome + run: | + if [ ! -f result/pr-number ] || [ ! -f result/outcome ]; then + echo "::notice::No outcome artifact found; nothing to label." + echo "found=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + + pr=$(tr -dc '0-9' < result/pr-number) + outcome=$(tr -dc 'a-z' < result/outcome) + + if [ -z "$pr" ] || { [ "$outcome" != "pass" ] && [ "$outcome" != "fail" ]; }; then + echo "::warning::Unexpected artifact contents (pr='$pr' outcome='$outcome')." + echo "found=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + + echo "found=true" >> "$GITHUB_OUTPUT" + echo "pr=$pr" >> "$GITHUB_OUTPUT" + echo "outcome=$outcome" >> "$GITHUB_OUTPUT" + + - name: Apply or remove the label + if: steps.outcome.outputs.found == 'true' + env: + GH_TOKEN: ${{ github.token }} + # Required. `gh` resolves the target repository from git remotes, and + # this job deliberately does no checkout, so without GH_REPO every + # `gh` call here fails with "fatal: not a git repository" before + # reaching the API. + GH_REPO: ${{ github.repository }} + PR: ${{ steps.outcome.outputs.pr }} + OUTCOME: ${{ steps.outcome.outputs.outcome }} + LABEL: needs-id-redact + run: | + if [ "$OUTCOME" = "fail" ]; then + # Idempotent: creating an existing label, or adding one a pull + # request already has, is a no-op and emits no `labeled` event, so + # this cannot loop. D93F0B matches the `needs-evals` label, the + # other check-output label in this repository. + gh label create "$LABEL" \ + --description "Unredacted AWS account IDs or EC2 instance IDs found in this pull request" \ + --color D93F0B 2>/dev/null || true + gh pr edit "$PR" --add-label "$LABEL" + echo "::notice::Labeled PR #$PR with $LABEL." + else + # Checked rather than attempted-and-ignored. `--remove-label` errors + # when the label is absent, so suppressing that one error with + # `|| true` would also suppress every other error, including a + # missing-GH_REPO failure — the step would report success while + # doing nothing. That exact bug was fixed in + # label-skill-evals-result.yml; this file starts from the fix. + # + # The lookup is a separate statement on purpose. Piping it straight + # into a test — `gh pr view ... | grep -qx true` — reproduces the + # same pathology: a failed lookup prints nothing, the test simply + # does not match, and the step reports "the label wasn't set" and + # exits 0. `set -e` does not catch it, because a command failing + # inside an `if` condition is exempt from errexit. As its own + # assignment, a failed lookup fails the job, while an absent label + # stays benign. + # + # LABEL is read from the environment by jq rather than interpolated + # into the jq program text; `gh --jq` has no `--arg`. + has_label=$(gh pr view "$PR" --json labels \ + --jq '[.labels[].name] | index(env.LABEL) != null') + if [ "$has_label" = "true" ]; then + gh pr edit "$PR" --remove-label "$LABEL" + echo "::notice::PR #$PR passed; removed $LABEL." + else + echo "::notice::PR #$PR passed; $LABEL was not set." + fi + fi From 26156403bf2540a0bbae766a0bce832e90788004 Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Sun, 4 Oct 2026 12:54:03 +0300 Subject: [PATCH 04/14] docs: document the AWS identifier scan and needs-id-redact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CONTRIBUTING.md gets a top-level "Scanning for AWS Identifiers" section, placed after Custom Agents and before Reporting Bugs so it sits alongside the skill-evals gate material without being nested under Skills — the check applies to every file in the repository, not only to skills. It covers the two patterns, the added-lines-only scope and why (`main` already carries real-looking identifiers, and a whole-file scan would fail contributors for content they did not write), the three suppression routes with an example of each, the bare-JSON-number exclusion, how to run the scanner and `--self-check` locally, what exit 1 and exit 2 mean, and the `needs-id-redact` label being applied and removed by automation alone. Every identifier in the examples is an allowlisted value, except the `322122547200` byte count, which carries an `aws-id-ok: ` marker so the section passes the check it describes. The same material goes into .kiro/steering/project-conventions.md as a short "AWS Identifiers" section after Disallowed Content, grouped with the other repository-wide content constraints. That file already covers the skill-evals gate at comparable depth, which is the condition for touching it, so this stays to six bullets. .claude/CLAUDE.md is a tracked near-duplicate of it and gets the same section with the shallower relative link, so the pair does not drift. The documentation also states that pre-existing identifiers on `main` are a separate follow-up change and not to be cleaned up opportunistically, since the check never reports them. Verified: the staged tree self-scans clean — 1113 added lines, 0 findings, 0 warnings. --- .claude/CLAUDE.md | 11 +++++ .kiro/steering/project-conventions.md | 11 +++++ CONTRIBUTING.md | 62 +++++++++++++++++++++++++++ 3 files changed, 84 insertions(+) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index b2b09993..c06b5c21 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -190,6 +190,17 @@ Only these extensions are permitted inside **skill** directories (enforced by `s - `.kiro/` directories should not be committed. - `.DS_Store` and other OS files should not be committed. +## AWS Identifiers + +A pull request check (`.github/workflows/scan-aws-identifiers.yml`, running `.github/scripts/scan_aws_identifiers.py`) reports unredacted AWS identifiers and applies to every file in the repository, not only to skills. See the "Scanning for AWS Identifiers" section of [CONTRIBUTING.md](../CONTRIBUTING.md). + +- Two patterns are matched: an AWS account ID as exactly twelve digits with no digit on either side, and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters. +- **Only the lines a pull request adds are scanned.** `main` already carries real-looking identifiers in committed eval results and example ARNs, so a whole-file scan would fail contributors for content they did not write. Renames are detected, so moving such a file reports nothing. +- Three ways to resolve a finding: redact the value; add it to `.github/aws-identifier-allowlist.json` with a mandatory reason; or put an `aws-id-ok: ` comment on the line, which works in the comment-bearing file types only. JSON has no comment syntax, so the allowlist is the route for JSON content. +- The allowlist parser fails closed: invalid JSON, a missing key, or a blank reason grants no allowance and fails the check, so a typo cannot silently waive a leak. The skill evaluation tool's redaction placeholders `012345678901` and `i-1234567890abcdef0` are allowlisted, suffixed forms included. +- The `needs-id-redact` label is applied by automation when the check fails and removed when it passes. Do not add or remove it by hand. +- Identifiers already on `main` are a separate follow-up change. Do not clean them up opportunistically in an unrelated pull request — the check never reports them, since it only reads added lines. + ## Adding a New Skill 1. Create a new directory under `skills/` with the skill name. diff --git a/.kiro/steering/project-conventions.md b/.kiro/steering/project-conventions.md index 443d1ece..64e568af 100644 --- a/.kiro/steering/project-conventions.md +++ b/.kiro/steering/project-conventions.md @@ -189,6 +189,17 @@ Only these extensions are permitted inside **skill** directories (enforced by `s - `.claude/` directories should not be committed (except CLAUDE.md). - `.DS_Store` and other OS files should not be committed. +## AWS Identifiers + +A pull request check (`.github/workflows/scan-aws-identifiers.yml`, running `.github/scripts/scan_aws_identifiers.py`) reports unredacted AWS identifiers and applies to every file in the repository, not only to skills. See the "Scanning for AWS Identifiers" section of [CONTRIBUTING.md](../../CONTRIBUTING.md). + +- Two patterns are matched: an AWS account ID as exactly twelve digits with no digit on either side, and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters. +- **Only the lines a pull request adds are scanned.** `main` already carries real-looking identifiers in committed eval results and example ARNs, so a whole-file scan would fail contributors for content they did not write. Renames are detected, so moving such a file reports nothing. +- Three ways to resolve a finding: redact the value; add it to `.github/aws-identifier-allowlist.json` with a mandatory reason; or put an `aws-id-ok: ` comment on the line, which works in the comment-bearing file types only. JSON has no comment syntax, so the allowlist is the route for JSON content. +- The allowlist parser fails closed: invalid JSON, a missing key, or a blank reason grants no allowance and fails the check, so a typo cannot silently waive a leak. The skill evaluation tool's redaction placeholders `012345678901` and `i-1234567890abcdef0` are allowlisted, suffixed forms included. +- The `needs-id-redact` label is applied by automation when the check fails and removed when it passes. Do not add or remove it by hand. +- Identifiers already on `main` are a separate follow-up change. Do not clean them up opportunistically in an unrelated pull request — the check never reports them, since it only reads added lines. + ## Adding a New Skill 1. Create a new directory under `skills/` with the skill name. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 24c6c84d..bc72d4fd 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -187,6 +187,68 @@ If an issue lands with you, verify the skill against the checklist in it. When i Test relevant scenarios with and without the custom agent, multiple times. Focus on quality and consistency of the output, compared to asking DevOps Agent the same question using chat. When checking consistency, the output doesn't have to be the same verbatim - focus on the substance +## Scanning for AWS Identifiers + +A pull request check ([`.github/workflows/scan-aws-identifiers.yml`](.github/workflows/scan-aws-identifiers.yml)) looks for unredacted AWS identifiers in what your pull request adds. An account ID or instance ID committed here names a real resource in a real account, and once it's on `main` it's in the public history for good. It applies to every file in the repository, not just to skills. + +Two things are matched: + +| Identifier | Shape | +| --- | --- | +| AWS account ID | exactly twelve digits with no digit on either side, so `arn:aws:iam::123456789012:role/Example` matches while a thirteen-digit number doesn't | +| EC2 instance ID | `i-` followed by either eight hexadecimal characters (the old form) or seventeen (the current one), case-insensitive | + +**Only the lines your pull request adds are scanned.** Removed lines, context lines, and lines you didn't touch are all ignored. That's deliberate: `main` already carries real-looking identifiers in committed eval results and in example ARNs, so a whole-file scan would fail you for content you never wrote, and the only way to go green would be to clean up someone else's lines. Renames are detected too, so moving a file that already contains identifiers adds no lines and reports nothing. + +The redaction placeholders the skill evaluation tool writes — `012345678901` for an account ID and `i-1234567890abcdef0` for an instance ID — are allowlisted already, with or without the `_N` suffix the tool adds to tell two different originals apart. + +### Resolving a finding + +Three routes, and the first one is usually right: + +1. **Redact the value.** Replace it with one of the placeholders above, or with an obvious stand-in. + +2. **Add it to the allowlist**, if the value is genuinely safe to publish — an AWS documentation example, or an AWS-owned account such as the one that publishes a public Lambda layer. Add it to [`.github/aws-identifier-allowlist.json`](.github/aws-identifier-allowlist.json) with a reason, which is mandatory and is what a reviewer judges the entry on: + + ```json + { + "account_ids": { + "123456789012": "AWS documentation example account ID" + }, + "instance_ids": { + "i-0123456789abcdef0": "AWS documentation example instance ID" + } + } + ``` + + The file fails closed: invalid JSON, a missing key, or a blank reason grants nothing at all and fails the check, rather than silently waiving an entry. This is also the only route for JSON files, which have no comment syntax. + +3. **Mark the line**, where the value belongs in place and an allowlist entry would be too broad — a value that only makes sense in the one line it's on. Put an `aws-id-ok: ` comment on it: + + ```yaml + AccountId: "123456789012" # aws-id-ok: placeholder in a template parameter default + ``` + + The marker is honored in `.md`, `.yaml`, `.yml`, `.py`, `.sh`, `.ts`, `.js`, `.html`, `.htm`, and `.xml` files. A bare `aws-id-ok` with no reason still suppresses the line, but the check reports a warning asking for one. + +One false positive is suppressed without any of that: a twelve-digit number appearing as a bare JSON value in a `.json` or `.jsonl` file, as in `"bytes": 322122547200` — a three-hundred-gigabyte volume size. AWS always writes an account ID as a string, so a bare number of that length is a byte count or a similar measurement. A twelve-digit run inside a quoted JSON string is still reported. + +### Running it locally + +```bash +python3 .github/scripts/scan_aws_identifiers.py --base-ref origin/main --head-ref HEAD +``` + +Findings are printed as `FOUND : `, and in the check's log they also appear as annotations on the pull request diff and as a summary table on the workflow run page. `--self-check` confirms the allowlist parses and that every entry in it is suppressed in both its bare and `_N`-suffixed forms. + +The exit code tells the two failure modes apart: `1` means identifiers were found, `2` means the scan couldn't be trusted — a malformed allowlist, or a diff it couldn't read. + +### The label + +A failing check puts a `needs-id-redact` label on the pull request, which is removed automatically once the check passes. That label is written only by automation and only reflects the last result — it isn't something to add or remove by hand. + +NOTE: a red check does not block merging on its own. An admin must add the job's name (`scan-aws-identifiers`) to the required status checks for `main` in branch protection. Until then this check is advisory. + ## Reporting Bugs/Feature Requests We welcome you to use the GitHub issue tracker to report bugs or suggest features. From bab8905a0a70169b524a5c3367dbd9f9c3a14546 Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Sun, 4 Oct 2026 13:21:51 +0300 Subject: [PATCH 05/14] fix: separate a scan that failed from a scan that found identifiers The scan step left `git fetch` to errexit, so an unreachable remote killed the step before it wrote its exit code. The empty value read downstream became OUTCOME=fail, and the labeling workflow then applied needs-id-redact to a pull request that contained no identifiers at all. Exit code 2, which the scanner uses for a malformed allowlist or a git failure, collapsed the same way, discarding the distinction the scanner takes care to maintain. The step now captures the fetch exit code instead of dying on it, and the published outcome carries three values rather than two: pass, fail, error. The labeling workflow leaves the label untouched on error, because needs-id-redact is a claim about a pull request's content and a broken scan is in no position to make it. The job still fails on every non-zero exit, so a scan that could not run can never be read as a clean one. The scanner's --self-check runs in the gate as well. It covers the `_N` redaction-suffix handling, which is the one part whose breakage is silent: a regression there makes identifiers stop being detected rather than start firing falsely, so a passing check would be the only symptom. --- .../label-aws-identifiers-result.yml | 27 ++++++++- .github/workflows/scan-aws-identifiers.yml | 60 +++++++++++++++---- 2 files changed, 74 insertions(+), 13 deletions(-) diff --git a/.github/workflows/label-aws-identifiers-result.yml b/.github/workflows/label-aws-identifiers-result.yml index 935f5c15..0ae62d30 100644 --- a/.github/workflows/label-aws-identifiers-result.yml +++ b/.github/workflows/label-aws-identifiers-result.yml @@ -80,8 +80,31 @@ jobs: pr=$(tr -dc '0-9' < result/pr-number) outcome=$(tr -dc 'a-z' < result/outcome) - if [ -z "$pr" ] || { [ "$outcome" != "pass" ] && [ "$outcome" != "fail" ]; }; then - echo "::warning::Unexpected artifact contents (pr='$pr' outcome='$outcome')." + case "$outcome" in + pass|fail) ;; + error) + # The check ran but could not reach a verdict: a malformed + # allowlist, a failed self-check, an unreachable remote. The + # scanning job has already failed, so the pull request is not + # going anywhere, and leaving the label untouched is the point — + # `needs-id-redact` says "this pull request contains identifiers", + # which is a claim nobody is in a position to make here. Applying + # it would send the author hunting for a value that does not + # exist; removing it would clear a true finding from an earlier + # run on the strength of an infrastructure failure. + echo "::notice::The scan could not complete, so the needs-id-redact label is left as it is." + echo "found=false" >> "$GITHUB_OUTPUT" + exit 0 + ;; + *) + echo "::warning::Unexpected artifact contents (pr='$pr' outcome='$outcome')." + echo "found=false" >> "$GITHUB_OUTPUT" + exit 0 + ;; + esac + + if [ -z "$pr" ]; then + echo "::warning::Artifact carries outcome '$outcome' but no pull request number." echo "found=false" >> "$GITHUB_OUTPUT" exit 0 fi diff --git a/.github/workflows/scan-aws-identifiers.yml b/.github/workflows/scan-aws-identifiers.yml index 5781b19a..6f6f4ef6 100644 --- a/.github/workflows/scan-aws-identifiers.yml +++ b/.github/workflows/scan-aws-identifiers.yml @@ -101,24 +101,50 @@ jobs: env: BASE_REF: ${{ github.event.pull_request.base.ref }} run: | - # Make sure the base branch is available locally as a remote-tracking - # ref so the merge base can be computed. + # The scanner's own regression check over every allowlist entry, in + # both its bare and its `_N`-suffixed form. It runs here because that + # is the one part of the scanner whose breakage is silent: a regression + # in the suffix handling makes identifiers stop being detected rather + # than start firing falsely, so a passing check would be the only + # symptom. Treated as a scan error rather than a finding, below. + set +e + python3 .github/scripts/scan_aws_identifiers.py --self-check + self_check=$? + + # The base branch has to exist locally as a remote-tracking ref before + # the merge base can be computed. Its exit code is captured instead of + # being left to errexit: a failing fetch would otherwise kill the step + # before `code` is written, and the empty value read downstream is + # indistinguishable from a real finding — which is how an unreachable + # remote ends up labeled as an unredacted identifier. git fetch --no-tags --quiet origin \ "+refs/heads/${BASE_REF}:refs/remotes/origin/${BASE_REF}" + fetched=$? - set +e - python3 .github/scripts/scan_aws_identifiers.py \ - --base-ref "origin/${BASE_REF}" \ - --head-ref HEAD - code=$? + if [ "$self_check" -ne 0 ] || [ "$fetched" -ne 0 ]; then + echo "::error::The AWS identifier scan could not run (self-check exit ${self_check}, base branch fetch exit ${fetched}). This is not a finding about the pull request." + code=2 + else + python3 .github/scripts/scan_aws_identifiers.py \ + --base-ref "origin/${BASE_REF}" \ + --head-ref HEAD + code=$? + fi set -e + echo "code=${code}" >> "$GITHUB_OUTPUT" exit "${code}" - name: Publish outcome for the labeling workflow env: PR_NUMBER: ${{ github.event.pull_request.number }} - OUTCOME: ${{ steps.scan.outputs.code == '0' && 'pass' || 'fail' }} + # Three outcomes, not two. The scanner separates "found identifiers" + # (exit 1) from "could not complete" (exit 2, and an empty value when + # the step died outright), and that separation has to survive to the + # labeling workflow: `needs-id-redact` is a claim about the pull + # request's content, so a configuration error or an unreachable remote + # must not apply it. The job fails either way; only the label differs. + OUTCOME: ${{ steps.scan.outputs.code == '0' && 'pass' || steps.scan.outputs.code == '1' && 'fail' || 'error' }} run: | mkdir -p aws-id-check-result printf '%s\n' "$PR_NUMBER" > aws-id-check-result/pr-number @@ -130,8 +156,20 @@ jobs: path: aws-id-check-result/ retention-days: 1 - - name: Fail the job when the scan failed - if: steps.scan.outputs.code != '0' + - name: Fail the job when identifiers were found + if: steps.scan.outputs.code == '1' + run: | + echo "::error::Unredacted AWS identifiers were found in the lines this pull request adds. The annotations above name each one, and the job summary lists them with the three ways to resolve a finding." + exit 1 + + # Fails the job too, so a broken scan can never be mistaken for a clean + # one, but says plainly that the pull request is not the problem. Catches + # the empty output value as well, which is what a step killed mid-run + # leaves behind. + - name: Fail the job when the scan could not complete + if: steps.scan.outputs.code != '0' && steps.scan.outputs.code != '1' + env: + CODE: ${{ steps.scan.outputs.code }} run: | - echo "::error::AWS identifier scan failed — see the log above." + echo "::error::The AWS identifier scan did not complete (exit code '${CODE}'). Nothing was reported about this pull request's content — see the log above for the scan or configuration error." exit 1 From 9a476a1b135249af3fb08526d40c766959bbc980 Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Sun, 4 Oct 2026 13:47:38 +0300 Subject: [PATCH 06/14] fix: stop reporting UUID tails and hex resource ids as account ids A UUID ends in twelve hexadecimal characters, so about one in 250 ends in twelve digits. The account-ID pattern required only that no digit sit on either side, and a hyphen is not a digit, so the final group of a UUID was reported as an account. Eleven UUIDs in committed eval output match that shape today. Measuring the fix over the whole tree turned up a second class the same check had been reporting: a twelve-digit run inside a longer hexadecimal token. A network interface id is built from a hex blob, and the twelve digits sitting in the middle of one name no account at all -- yet 24 lines of eval output were reported that way. Widening the boundary from "no digit" to "no letter or digit" covers it, along with every other resource id built the same way: subnet, vol, sg, snap, ami. Both guards only remove matches, verified by replaying the old and new patterns over every text file in the repository: 8694 matches before, 8645 after, and not one match that the new pattern finds and the old one missed. The 49 it drops are all UUID tails or hex resource ids. The UUID guard spells out the full 8-4-4-4 hex prefix rather than rejecting a preceding hyphen, so an account ID that merely follows a hyphen is still reported. Every element of it is a fixed repetition, which is what lets Python's re accept it as a lookbehind. Eleven boundary cases now run in --self-check, which the gate workflow executes on every pull request. They are checked against the pattern directly rather than through the allowlist, because a guard that grows too broad fails silently: identifiers stop being reported and a passing check is the only symptom. --- .claude/CLAUDE.md | 3 +- .github/scripts/scan_aws_identifiers.py | 144 ++++++++++++++++++++---- .kiro/steering/project-conventions.md | 3 +- CONTRIBUTING.md | 7 +- 4 files changed, 130 insertions(+), 27 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index c06b5c21..5ac9ecb9 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -194,7 +194,8 @@ Only these extensions are permitted inside **skill** directories (enforced by `s A pull request check (`.github/workflows/scan-aws-identifiers.yml`, running `.github/scripts/scan_aws_identifiers.py`) reports unredacted AWS identifiers and applies to every file in the repository, not only to skills. See the "Scanning for AWS Identifiers" section of [CONTRIBUTING.md](../CONTRIBUTING.md). -- Two patterns are matched: an AWS account ID as exactly twelve digits with no digit on either side, and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters. +- Two patterns are matched: an AWS account ID as exactly twelve digits with no letter or digit on either side, and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters. +- Two account-ID lookalikes are deliberately not reported, both present in committed eval output: the last group of a UUID (`a618bd73-f5dc-4e6b-b1f4-123412341234`, since about one UUID in 250 ends in twelve digits) and a twelve-digit run inside a longer hexadecimal token (`eni-097816109986f5e1d`, and likewise `subnet-`, `vol-`, `sg-`, `snap-`, `ami-`). Only a full `8-4-4-4-` hex prefix is skipped, so `stack-1234-111122223333` is still reported. Both boundaries are pinned by cases in the scanner's `--self-check`, which the check runs on every pull request. - **Only the lines a pull request adds are scanned.** `main` already carries real-looking identifiers in committed eval results and example ARNs, so a whole-file scan would fail contributors for content they did not write. Renames are detected, so moving such a file reports nothing. - Three ways to resolve a finding: redact the value; add it to `.github/aws-identifier-allowlist.json` with a mandatory reason; or put an `aws-id-ok: ` comment on the line, which works in the comment-bearing file types only. JSON has no comment syntax, so the allowlist is the route for JSON content. - The allowlist parser fails closed: invalid JSON, a missing key, or a blank reason grants no allowance and fails the check, so a typo cannot silently waive a leak. The skill evaluation tool's redaction placeholders `012345678901` and `i-1234567890abcdef0` are allowlisted, suffixed forms included. diff --git a/.github/scripts/scan_aws_identifiers.py b/.github/scripts/scan_aws_identifiers.py index 62919650..dff0fd19 100755 --- a/.github/scripts/scan_aws_identifiers.py +++ b/.github/scripts/scan_aws_identifiers.py @@ -24,9 +24,17 @@ Two patterns are matched: -* **AWS account ID** — exactly twelve digits with no digit on either side, so - ``arn:aws:iam::123456789012:role/Example`` matches while a thirteen-digit - number does not. +* **AWS account ID** — exactly twelve digits with no letter or digit on either + side, so ``arn:aws:iam::123456789012:role/Example`` matches while a + thirteen-digit number does not. Two lookalikes are excluded, both of which + occur in this repository's committed eval output: the last group of a UUID + (``a618bd73-f5dc-4e6b-b1f4-123412341234``, since a UUID ends in twelve + hexadecimal characters and about one in 250 of those is all digits), and a + twelve-digit run inside a longer hexadecimal token (``eni-097816109986f5e1d`` + names a network interface, and the twelve digits in the middle of it name no + account at all). Only a full ``8-4-4-4-`` hex prefix is skipped, so an + account ID that merely follows a hyphen — ``stack-1234-111122223333`` — is + still reported. * **EC2 instance ID** — ``i-`` followed by either eight hexadecimal characters (the old form) or seventeen (the current form), case-insensitive, with no other hexadecimal word character on either side. @@ -78,17 +86,44 @@ from pathlib import Path -# Twelve digits with no digit on either side. The lookarounds are what stop a -# longer run of digits from matching a twelve-digit window inside itself, so -# neither a thirteen-digit timestamp nor a long id is reported. -# -# The optional ``_`` group is the skill evaluation tool's redaction -# suffix. It is written as a lookahead-guarded optional group rather than with -# ``\b`` on purpose: ``_`` is a word character, so ``\b``-anchored patterns do -# not match the suffixed form at all, and a suffixed placeholder would slip -# past the allowlist comparison as if it were a different value. Here the group -# consumes the suffix, and the suffix is then stripped before the comparison. -ACCOUNT_RE = re.compile(r"(?`` group is the skill evaluation tool's redaction + # suffix. It is written as a boundary-guarded optional group rather than + # with ``\b`` on purpose: ``_`` is a word character, so ``\b``-anchored + # patterns do not match the suffixed form at all, and a suffixed + # placeholder would slip past the allowlist comparison as if it were a + # different value. Here the group consumes the suffix, and the suffix is + # then stripped before the comparison. + r"(?:_([0-9]+))?" + # Rejects a thirteenth digit, so a longer run of digits never matches a + # twelve-digit window inside itself and a millisecond timestamp stays + # unreported. + r"(?![0-9A-Za-z])" +) # ``i-`` plus either of the two legal lengths, seventeen hexadecimal characters # (current) or eight (the old form), with the seventeen-character alternative @@ -584,21 +619,81 @@ def _write_summary( fh.write("\n".join(lines) + "\n") +# Boundary cases for the account-ID pattern, checked against the pattern itself +# rather than through the allowlist, so each one states a property of the +# matching and nothing else. Every entry is a string to match and the list of +# account IDs the pattern is expected to pull out of it. +# +# These are here because both guards in ACCOUNT_RE suppress matches, and a +# suppressing guard that grows too broad fails silently: identifiers stop being +# reported, and a passing check is the only symptom. +# +# Every case that is expected to match uses an allowlisted documentation +# placeholder rather than an invented twelve-digit value. The cases exercise +# ACCOUNT_RE directly, and the allowlist is applied later, in +# ``scan_added_lines`` — so a placeholder proves the pattern matches just as +# well as a real-looking number would, and this file does not become the one +# place in the repository carrying identifiers that the check itself would +# report. Allowlist suppression is covered by the second group of cases in +# ``self_check``. +ACCOUNT_PATTERN_CASES: tuple[tuple[str, list[str]], ...] = ( + # The UUID guard. Nothing to report: the twelve digits are a UUID's final + # group. + ("a618bd73-f5dc-4e6b-b1f4-123412341234", []), + # Upper-case hex is a UUID too. + ("A618BD73-F5DC-4E6B-B1F4-123412341234", []), + # The guard must not reach further than a real 8-4-4-4 hex prefix. Here the + # first group is not hex, so this is not a UUID and the digits are reported. + ("zzzzzzzz-f5dc-4e6b-b1f4-123456789012", ["123456789012"]), + # A hyphen before the digits is not a UUID on its own. + ("stack-1234-111122223333", ["111122223333"]), + # The alphanumeric guard. A twelve-digit run inside a longer hex token + # belongs to the token, not to an account. + ("eni-097816109986f5e1d", []), + ("vol-0a1b2c3d123456789012", []), + # Still reported in the places an account ID actually appears. + ("arn:aws:iam::123456789012:role/Example", ["123456789012"]), + ('"accountId": "123456789012",', ["123456789012"]), + ("123456789012", ["123456789012"]), + # A thirteen-digit millisecond timestamp has no twelve-digit window to find. + ("1727000000000", []), + # The redaction suffix is consumed, and the identifier reported without it. + ("012345678901_7", ["012345678901"]), +) + + def self_check(accounts: set[str], instances: set[str]) -> list[str]: - """Confirm every allowlist entry is suppressed bare and with a ``_N`` suffix. + """Confirm the pattern boundaries hold and every allowlist entry is suppressed. - The redaction suffix is the one part of the matching that is easy to get - silently wrong: a ``\\b``-anchored pattern does not match - ``i-1234567890abcdef0_3`` at all, so a regression there would stop - *reporting* suffixed placeholders rather than start reporting them, and no - pull request would fail to reveal it. This walks every entry in the - allowlist in use, builds a synthetic added line for the bare form and for a - ``_7``-suffixed form, scans it, and reports any that produced a finding. + Two groups of cases, both covering failures that are silent — they stop + identifiers from being *reported* rather than start reporting things + falsely, so a passing check is the only symptom and no pull request reveals + them. + + The first group walks ACCOUNT_PATTERN_CASES, which pins the two guards in + ACCOUNT_RE: a UUID's final group is not an account ID, and neither is a + twelve-digit run inside a longer hex token, while an account ID in an ARN, + in JSON, or after a plain hyphen still is. + + The second group walks every entry in the allowlist in use, builds a + synthetic added line for the bare form and for a ``_7``-suffixed form, and + reports any that produced a finding. The redaction suffix is the part of the + matching that is easiest to get wrong: a ``\\b``-anchored pattern does not + match ``i-1234567890abcdef0_3`` at all. Run it with ``--self-check``. It reads nothing but the allowlist and runs no ``git`` command. """ failures: list[str] = [] + + for text, expected in ACCOUNT_PATTERN_CASES: + found = [match.group(1) for match in ACCOUNT_RE.finditer(text)] + if found != expected: + failures.append( + f"account pattern on `{text}` produced {found or 'no match'}, " + f"expected {expected or 'no match'}" + ) + for identifier in sorted(accounts | instances): for value in (identifier, f"{identifier}_7"): entries = [("docs/self-check.md", 1, f"value {value} on a line")] @@ -671,8 +766,9 @@ def main(argv: list[str] | None = None) -> int: _error(failure) total = len(accounts) + len(instances) print( - f"Self-check: {total} allowlist entr(ies), bare and `_N`-suffixed, " - f"{len(failures)} failure(s)." + f"Self-check: {len(ACCOUNT_PATTERN_CASES)} account pattern " + f"boundary case(s), {total} allowlist entr(ies) bare and " + f"`_N`-suffixed, {len(failures)} failure(s)." ) return 1 if failures else 0 diff --git a/.kiro/steering/project-conventions.md b/.kiro/steering/project-conventions.md index 64e568af..d08eab69 100644 --- a/.kiro/steering/project-conventions.md +++ b/.kiro/steering/project-conventions.md @@ -193,7 +193,8 @@ Only these extensions are permitted inside **skill** directories (enforced by `s A pull request check (`.github/workflows/scan-aws-identifiers.yml`, running `.github/scripts/scan_aws_identifiers.py`) reports unredacted AWS identifiers and applies to every file in the repository, not only to skills. See the "Scanning for AWS Identifiers" section of [CONTRIBUTING.md](../../CONTRIBUTING.md). -- Two patterns are matched: an AWS account ID as exactly twelve digits with no digit on either side, and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters. +- Two patterns are matched: an AWS account ID as exactly twelve digits with no letter or digit on either side, and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters. +- Two account-ID lookalikes are deliberately not reported, both present in committed eval output: the last group of a UUID (`a618bd73-f5dc-4e6b-b1f4-123412341234`, since about one UUID in 250 ends in twelve digits) and a twelve-digit run inside a longer hexadecimal token (`eni-097816109986f5e1d`, and likewise `subnet-`, `vol-`, `sg-`, `snap-`, `ami-`). Only a full `8-4-4-4-` hex prefix is skipped, so `stack-1234-111122223333` is still reported. Both boundaries are pinned by cases in the scanner's `--self-check`, which the check runs on every pull request. - **Only the lines a pull request adds are scanned.** `main` already carries real-looking identifiers in committed eval results and example ARNs, so a whole-file scan would fail contributors for content they did not write. Renames are detected, so moving such a file reports nothing. - Three ways to resolve a finding: redact the value; add it to `.github/aws-identifier-allowlist.json` with a mandatory reason; or put an `aws-id-ok: ` comment on the line, which works in the comment-bearing file types only. JSON has no comment syntax, so the allowlist is the route for JSON content. - The allowlist parser fails closed: invalid JSON, a missing key, or a blank reason grants no allowance and fails the check, so a typo cannot silently waive a leak. The skill evaluation tool's redaction placeholders `012345678901` and `i-1234567890abcdef0` are allowlisted, suffixed forms included. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bc72d4fd..f31acec6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -195,9 +195,14 @@ Two things are matched: | Identifier | Shape | | --- | --- | -| AWS account ID | exactly twelve digits with no digit on either side, so `arn:aws:iam::123456789012:role/Example` matches while a thirteen-digit number doesn't | +| AWS account ID | exactly twelve digits with no letter or digit on either side, so `arn:aws:iam::123456789012:role/Example` matches while a thirteen-digit number doesn't | | EC2 instance ID | `i-` followed by either eight hexadecimal characters (the old form) or seventeen (the current one), case-insensitive | +Two things that look like an account ID are deliberately not reported, because both occur in this repository's committed eval output and neither names an account: + +- **The last group of a UUID.** A UUID ends in twelve hexadecimal characters, so roughly one in 250 ends in twelve digits — `a618bd73-f5dc-4e6b-b1f4-123412341234`. Only a full `8-4-4-4-` hex prefix is skipped, so an account ID that merely follows a hyphen, as in `stack-1234-111122223333`, is still reported. +- **A twelve-digit run inside a longer hexadecimal token.** `eni-097816109986f5e1d` is a network interface, and the twelve digits sitting in the middle of it name no account. The same applies to the other resource IDs built from a hex blob: `subnet-`, `vol-`, `sg-`, `snap-`, `ami-`. + **Only the lines your pull request adds are scanned.** Removed lines, context lines, and lines you didn't touch are all ignored. That's deliberate: `main` already carries real-looking identifiers in committed eval results and in example ARNs, so a whole-file scan would fail you for content you never wrote, and the only way to go green would be to clean up someone else's lines. Renames are detected too, so moving a file that already contains identifiers adds no lines and reports nothing. The redaction placeholders the skill evaluation tool writes — `012345678901` for an account ID and `i-1234567890abcdef0` for an instance ID — are allowlisted already, with or without the `_N` suffix the tool adds to tell two different originals apart. From b7f4b099345b3f555b70de52c4338d443c270acb Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Sun, 4 Oct 2026 21:22:26 +0300 Subject: [PATCH 07/14] fix: report an account ID only where something proves it is one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The twelve-digit shape rule was wrong more often than right. Of 1867 twelve-digit runs on the open pull requests' added lines, 1008 (54%) name no account, in four shapes: a YYYYMMDDHHMM datestamp used as a resource-name suffix (916 hits), the fractional digits of a decimal (57), the integer part of a decimal (7), and a zero-padded counter (28). The bare-JSON-number heuristic could not catch the decimal ones, because the eval journals store serialized AWS API responses as strings inside the outer JSON, so those digits really are inside a JSON string. The scan now runs in two passes. Pass one gathers evidence and reports nothing, from three sources: the account field of an ARN (the fifth colon-separated field, per the AWS reference); a twelve-digit object key in the tool result of a tool_summary block, in a journal_records.json file; and the value of an aws_account_id field, in a journal_records.json file. The two journal sources are fields of the DevOps Agent journal schema, read by walking the record tree and parsing the JSON-inside-JSON as it goes, since the escaping depth varies from one record to the next. Pass one reads every changed file whole, so an ARN in a part of a file the pull request never touched still proves an account that an added line names bare. Pass two reports, on added lines only: an ARN whose account field holds a non-allowlisted twelve-digit value, reported as the whole ARN so the remedy is to remove the ARN rather than blank one field and leave the rest of it in place; every occurrence of an account pass one proved, in any file type; and EC2 instance IDs, unchanged. An ARN with no account field is not reported. Of 121 such ARNs on the open pull requests' added lines, none names a private resource — 97 are templates or explicit examples, 21 are placeholders or AWS-published service quota codes, 3 are truncated — and there is no way to tell a real bucket ARN from an example one, since anybody may own a bucket by any name. Removed with the shape rule: the UUID-tail guard, the alphanumeric boundary as a load-bearing guard, and the unquoted-JSON-number exclusion. The two boundary lookarounds stay on the occurrence search as cheap insurance and are commented as such. Verified: - --self-check exits 0: 13 evidence cases (the three ARN resource forms, a non-aws partition, an account-less ARN, the literal aws in the account field, the redaction suffix, both journal sources including the nested and escaped tool_summary shape, and a journal that will not parse), 16 reporting cases (the whole-ARN finding, prose occurrences, and every lookalike shape staying silent), and all 12 allowlist entries suppressed bare and _N-suffixed. - The branch passes its own scan against origin/main with no findings. - All 33 open pull requests were scanned. Five report findings and 28 are clean. Of the five, one is instance IDs alone; three carry ARNs naming real accounts; one is caught solely by a tool_summary object key in an eval journal, which the previous rule also caught but only by shape. None of the four removed shapes is reported anywhere. - One pull request stays clean that the old rule would have failed: its account appears only as prose in a benchmark file with no ARN, which is the documented gap this design accepts. - Whole main tree, every line of all 860 tracked files: 9016 findings against the old rule's 9102. Account findings fall from 8218 across 18 distinct values to 8132 across 3, each of the 3 proved by an ARN. Of the 15 values no longer reported, 4 are not accounts at all (two are divisors in a SQL snippet) and the rest are named only in prose or in a field this scan does not read. Instance findings are identical, 884 across 4 distinct ids; running the previous scanner over the same added lines of the two pull requests that carry instance IDs produced a byte-identical list. - Four local eval directories outside the repository were scanned, with gitignored paths skipped. The account each one carries in CloudFormation stack ARNs in _metadata.json is still found, now through the ARN rule. One account named only in agent prose and in an aws-iam-authenticator string is missed, which is the same accepted gap. - The scanner still writes nothing. Its subprocess calls are git diff and git show, both read-only, and its only file write appends to $GITHUB_STEP_SUMMARY. Neither workflow YAML nor the allowlist changed; this is a scanner-internal change. CONTRIBUTING.md, the Kiro steering file and .claude/CLAUDE.md are rewritten to describe the ARN-and-proven-value model, the documentation convention of writing an example ARN with an allowlisted placeholder account, and the known prose gap. --- .claude/CLAUDE.md | 13 +- .github/scripts/scan_aws_identifiers.py | 967 +++++++++++++++++++----- .kiro/steering/project-conventions.md | 13 +- CONTRIBUTING.md | 42 +- 4 files changed, 814 insertions(+), 221 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 5ac9ecb9..ae47ef69 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -194,11 +194,14 @@ Only these extensions are permitted inside **skill** directories (enforced by `s A pull request check (`.github/workflows/scan-aws-identifiers.yml`, running `.github/scripts/scan_aws_identifiers.py`) reports unredacted AWS identifiers and applies to every file in the repository, not only to skills. See the "Scanning for AWS Identifiers" section of [CONTRIBUTING.md](../CONTRIBUTING.md). -- Two patterns are matched: an AWS account ID as exactly twelve digits with no letter or digit on either side, and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters. -- Two account-ID lookalikes are deliberately not reported, both present in committed eval output: the last group of a UUID (`a618bd73-f5dc-4e6b-b1f4-123412341234`, since about one UUID in 250 ends in twelve digits) and a twelve-digit run inside a longer hexadecimal token (`eni-097816109986f5e1d`, and likewise `subnet-`, `vol-`, `sg-`, `snap-`, `ami-`). Only a full `8-4-4-4-` hex prefix is skipped, so `stack-1234-111122223333` is still reported. Both boundaries are pinned by cases in the scanner's `--self-check`, which the check runs on every pull request. -- **Only the lines a pull request adds are scanned.** `main` already carries real-looking identifiers in committed eval results and example ARNs, so a whole-file scan would fail contributors for content they did not write. Renames are detected, so moving such a file reports nothing. -- Three ways to resolve a finding: redact the value; add it to `.github/aws-identifier-allowlist.json` with a mandatory reason; or put an `aws-id-ok: ` comment on the line, which works in the comment-bearing file types only. JSON has no comment syntax, so the allowlist is the route for JSON content. -- The allowlist parser fails closed: invalid JSON, a missing key, or a blank reason grants no allowance and fails the check, so a typo cannot silently waive a leak. The skill evaluation tool's redaction placeholders `012345678901` and `i-1234567890abcdef0` are allowlisted, suffixed forms included. +- An account ID is reported only when something proves it is one; twelve digits on their own are not evidence. Measured across the open pull requests, 54% of the twelve-digit runs on their added lines named no account — a `YYYYMMDDHHMM` datestamp in a resource name, the fractional or integer part of a decimal, a zero-padded counter. The scan therefore runs in two passes. +- **Pass one gathers evidence and reports nothing**, from exactly three sources: the account field of an [ARN](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) (the fifth colon-separated field); an object key in the tool result of a `tool_summary` block, in a `journal_records.json` file, where DevOps Agent keys a per-account API result by account ID; and the value of an `aws_account_id` field, in a `journal_records.json` file. The last two are fields of the DevOps Agent journal schema, which is why they are read only in that file name. Other spellings such as `AccountId` and `accountId` are deliberately not read. Pass one reads every changed file whole, so an ARN in an untouched part of a file still proves an account ID an added line names bare. +- **Pass two reports, on added lines only:** an ARN whose account field holds a non-allowlisted twelve-digit value, reported as the **whole ARN** rather than the account segment (blanking one field would leave the rest of the ARN in place, and a surviving copy of the account ID elsewhere in the file would rebuild it); every occurrence of an account ID pass one proved, in any file type; and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters, which needs no evidence. +- An ARN with no account field is not reported — `arn:aws:s3:::my-internal-bucket`, `arn:aws:iam::aws:policy/...`. There is no reliable way to tell a real bucket from an example one, since anybody may own `arn:aws:s3:::example-bucket`, so reporting them would be noise; redacting a real one is the author's job and catching it is a human reviewer's. Write example ARNs with `123456789012` or `111122223333`, both allowlisted. +- The known gap: an account ID that appears only as prose in a file that is not a journal is proved by none of the three sources and is not reported. Both passes and the lookalikes that must stay silent are pinned by cases in the scanner's `--self-check`, which the check runs on every pull request. +- **Only the lines a pull request adds are reported on.** `main` already carries real identifiers in committed eval results and example ARNs, so reporting on whole files would fail contributors for content they did not write. Renames are detected, so moving such a file reports nothing. +- Three ways to resolve a finding: redact the value, removing the whole ARN for an ARN finding; add it to `.github/aws-identifier-allowlist.json` with a mandatory reason; or put an `aws-id-ok: ` comment on the line, which works in the comment-bearing file types only. JSON has no comment syntax, so the allowlist is the route for JSON content. +- The allowlist parser fails closed: invalid JSON, a missing key, or a blank reason grants no allowance and fails the check, so a typo cannot silently waive a leak. An allowlisted account ID is dropped from the proven set, which silences both bare occurrences of it and any ARN naming it. The skill evaluation tool's redaction placeholders `012345678901` and `i-1234567890abcdef0` are allowlisted, suffixed forms included. - The `needs-id-redact` label is applied by automation when the check fails and removed when it passes. Do not add or remove it by hand. - Identifiers already on `main` are a separate follow-up change. Do not clean them up opportunistically in an unrelated pull request — the check never reports them, since it only reads added lines. diff --git a/.github/scripts/scan_aws_identifiers.py b/.github/scripts/scan_aws_identifiers.py index dff0fd19..1982fdc1 100755 --- a/.github/scripts/scan_aws_identifiers.py +++ b/.github/scripts/scan_aws_identifiers.py @@ -2,17 +2,82 @@ """Scan the lines a pull request adds for AWS account IDs and EC2 instance IDs. The point of the check is to keep unredacted identifiers out of the repository: -a twelve-digit AWS account ID or an EC2 instance ID committed in a skill, an -eval result, a workflow or a document names a real resource in a real account, -and once it is on ``main`` it is in the public history for good. - -**Scope: only the lines the pull request adds.** The diff is read with -``git diff --find-renames --unified=0 ...``; only the ``+`` -lines of that diff are scanned, and removed lines, context lines and the -``+++``/``---`` file headers are all ignored. Scanning whole files, or the whole -tree, was rejected deliberately: ``main`` already carries real-looking -identifiers — committed eval results, example ARNs in skill documentation, a -handful of values in MCP comments — so a whole-file scan would fail pull +an AWS account ID or an EC2 instance ID committed in a skill, an eval result, a +workflow or a document names a real resource in a real account, and once it is +on ``main`` it is in the public history for good. + +**An account ID is reported only when something proves it is one.** Twelve +digits on their own are not evidence. Measured against the open pull requests in +this repository, 1008 of 1867 twelve-digit runs on their added lines — 54% — +name no account, in four shapes: a ``YYYYMMDDHHMM`` datestamp used as a +resource-name suffix (916 hits), the fractional digits of a decimal (57 hits), +the integer part of a decimal (7 hits), and a zero-padded counter (28 hits). +Synthetic examples of all four, and of the twelve-digit run inside a longer +hexadecimal resource id, are in ``REPORTING_CASES`` at the foot of this file. +So the scan works in two passes. + +**Pass one gathers evidence and reports nothing.** Three sources put a value in +a position that only an account ID occupies: + +1. The account field of an ARN. ``arn:partition:service:region:account-id:…`` + — when the fifth colon-separated field is exactly twelve digits, those digits + are an account ID. +2. An object key in the tool result of a ``tool_summary`` block, in a file named + ``journal_records.json``. The DevOps Agent journal records a per-account AWS + API result as a map keyed by account ID, as in + ``{"123456789012": {"DBInstances": []}}``. +3. The value of an ``aws_account_id`` field, in a file named + ``journal_records.json``. That field name is part of the DevOps Agent journal + schema, and it appears both in a recorded tool input and in agent prose. + +Only those three. Other spellings an AWS API response or a human might use — +``AccountId``, ``accountId``, ``Account`` — are deliberately not read, because +the two journal sources above are schema-defined fields whose meaning is fixed, +while a loose field-name match would reintroduce guesswork. + +Pass one reads **every file the pull request changes, whole**, not just the added +lines. Evidence costs nothing to collect, and the wider read means an account ID +written bare on an added line is still caught when the ARN that proves it sits +in a part of the file the pull request never touched. + +**Pass two reports, and only on the lines the pull request adds:** + +* Any ARN whose account field holds a twelve-digit value that is not + allowlisted. The finding is the **whole ARN**, not the account segment inside + it. Flagging only the segment would let an author blank that one field and + leave the region, the service and the resource name in place — and a surviving + copy of the account ID elsewhere in the file would be enough to rebuild the + full ARN. Making the ARN the finding makes the remedy "remove this ARN". +* Any occurrence of an account ID that pass one proved, anywhere on an added + line, in any file type. This is what catches the same account written bare in + prose or in a field this scan does not read. +* An EC2 instance ID: ``i-`` followed by either eight hexadecimal characters + (the old form) or seventeen (the current form). An instance ID is + self-evidencing — its prefix and length are the proof — so it needs no pass + one. + +**An ARN with no account field is not reported.** ``arn:aws:s3:::my-bucket`` and +``arn:aws:iam::aws:policy/ReadOnlyAccess`` name no account, and there is no +reliable way to tell a real bucket from an example one: anybody may own +``arn:aws:s3:::my-internal-bucket`` or ``arn:aws:s3:::example-bucket``, so +reporting them would be noise. Of 121 account-less ARNs on the open pull +requests' added lines, none named a private resource — 97 were templates or +explicit examples, 21 were placeholders or AWS-published service quota codes, 3 +were truncated. Redacting a real S3 bucket ARN is the author's job, and catching +one is a human reviewer's. + +**The known gap**, stated plainly so nobody mistakes a pass for a guarantee: an +account ID that appears only as prose in a file that is not a journal — "the +cluster in account 123456789012", with no ARN anywhere in that file — is proved +by none of the three sources and is not reported. + +**Scope: only the lines the pull request adds are reported on.** The diff is read +with ``git diff --find-renames --unified=0 ...``; only the +``+`` lines of that diff are scanned for findings, and removed lines, context +lines and the ``+++``/``---`` file headers are all ignored. Reporting on whole +files, or on the whole tree, was rejected deliberately: ``main`` already carries +real identifiers — committed eval results, example ARNs in skill documentation, +a handful of values in MCP comments — so a whole-file report would fail pull requests for content their authors never wrote, and the only way to go green would be to clean up someone else's lines. Cleaning those up is a separate change; this check stops new ones arriving. @@ -22,23 +87,6 @@ a file that already contains identifiers would fail the check for a change that added nothing. With it, a pure rename produces no added lines at all. -Two patterns are matched: - -* **AWS account ID** — exactly twelve digits with no letter or digit on either - side, so ``arn:aws:iam::123456789012:role/Example`` matches while a - thirteen-digit number does not. Two lookalikes are excluded, both of which - occur in this repository's committed eval output: the last group of a UUID - (``a618bd73-f5dc-4e6b-b1f4-123412341234``, since a UUID ends in twelve - hexadecimal characters and about one in 250 of those is all digits), and a - twelve-digit run inside a longer hexadecimal token (``eni-097816109986f5e1d`` - names a network interface, and the twelve digits in the middle of it name no - account at all). Only a full ``8-4-4-4-`` hex prefix is skipped, so an - account ID that merely follows a hyphen — ``stack-1234-111122223333`` — is - still reported. -* **EC2 instance ID** — ``i-`` followed by either eight hexadecimal characters - (the old form) or seventeen (the current form), case-insensitive, with no - other hexadecimal word character on either side. - **Redaction suffixes.** The skill evaluation tool redacts identifiers by replacing each distinct original with a placeholder plus an index, as in ``012345678901_2`` and ``i-1234567890abcdef0_2``, where the trailing number @@ -48,17 +96,15 @@ alike. The finding reports the token as written; only the comparison is normalized. -**Four ways a match is suppressed:** +**Three ways a match is suppressed:** 1. The file being scanned *is* the allowlist. Added lines in ``.github/aws-identifier-allowlist.json`` are skipped, so adding an entry to the allowlist does not trip the scanner on that very entry. -2. An entry in the allowlist file, which must carry a reason. -3. A twelve-digit run that is a bare JSON number rather than a quoted JSON - string, in a ``.json`` or ``.jsonl`` file. AWS always writes an account ID - as a string, so a bare number of that length is a byte count or a similar - measurement, never an account ID. -4. An ``aws-id-ok: `` marker on the line. Honored in file types that +2. An entry in the allowlist file, which must carry a reason. An allowlisted + account ID is dropped from the proven set, which suppresses both the bare + occurrences of it and any ARN that names it. +3. An ``aws-id-ok: `` marker on the line. Honored in file types that have a comment syntax — ``.md``, ``.yaml``, ``.yml``, ``.py``, ``.sh``, ``.ts``, ``.js``, ``.html``, ``.htm``, ``.xml``. JSON has no comment syntax, which is why the allowlist is the route for JSON content. @@ -86,45 +132,57 @@ from pathlib import Path -# Twelve digits, with two guards against things that merely look like an -# account ID. Both were measured against this repository rather than guessed: -# with neither guard, 35 of the 8660 twelve-digit runs in the tree are not -# account IDs at all. -ACCOUNT_RE = re.compile( - # No alphanumeric character on either side, rather than no digit. Digits - # alone let a twelve-digit run inside a longer hex token through, which is - # how the digits inside `eni-097816109986f5e1d` got reported as an account - # ID — 24 lines of committed eval output in this repository did that. The - # same guard covers every other AWS resource id built from a hex blob: - # subnet, vol, sg, snap, ami. The cost is a twelve-digit account ID written - # flush against a letter, which no AWS output produces and which did not - # occur anywhere in the tree. - r"(?`` group is the skill evaluation tool's redaction - # suffix. It is written as a boundary-guarded optional group rather than - # with ``\b`` on purpose: ``_`` is a word character, so ``\b``-anchored - # patterns do not match the suffixed form at all, and a suffixed - # placeholder would slip past the allowlist comparison as if it were a - # different value. Here the group consumes the suffix, and the suffix is - # then stripped before the comparison. - r"(?:_([0-9]+))?" - # Rejects a thirteenth digit, so a longer run of digits never matches a - # twelve-digit window inside itself and a millisecond timestamp stays - # unreported. - r"(?![0-9A-Za-z])" +# An AWS account ID as a complete value: exactly twelve digits, with the skill +# evaluation tool's optional ``_`` redaction suffix. Used with +# ``fullmatch`` on a value that some structure has already singled out — an ARN +# field, a journal object key, an ``aws_account_id`` value, an allowlist entry — +# never to go hunting for twelve digits in free text. +ACCOUNT_VALUE_RE = re.compile(r"([0-9]{12})(?:_([0-9]+))?") + +# An ARN, parsed by position. The AWS reference +# (https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) gives +# three forms: +# +# arn:partition:service:region:account-id:resource-id +# arn:partition:service:region:account-id:resource-type/resource-id +# arn:partition:service:region:account-id:resource-type:resource-id +# +# They differ only in what *follows* the account, so the parse does not branch: +# the account is always the fifth colon-separated field, and everything after it +# is the resource. +# +# The account field is matched loosely and judged in code rather than being +# pinned to twelve digits here, so that an ARN with an empty account +# (``arn:aws:s3:::my-bucket``), the literal ``aws`` +# (``arn:aws:iam::aws:policy/ReadOnlyAccess``) or a redaction suffix +# (``012345678901_1``) all still parse — and are then handled on their merits. +# An ARN whose account is a template expression such as ``${AWS::AccountId}`` +# does not parse at all, because the braces hold colons of their own; that costs +# nothing, since such an ARN names no account either. +# +# The partition is ``aws`` plus any number of lowercase suffixes, which covers +# the three in practice: ``aws``, ``aws-cn``, ``aws-us-gov``. The region may be +# empty. The resource may be empty too, so a truncated ARN that stops right +# after the account still yields its account. +ARN_RE = re.compile( + r"arn:" + r"(?Paws(?:-[a-z0-9]+)*):" + r"(?P[a-z0-9][a-z0-9-]*):" + r"(?P[a-z0-9-]*):" + r"(?P[0-9A-Za-z_-]*):" + # The resource ends where the surrounding text begins. Whitespace, a quote, + # a backtick, a backslash (an ARN inside an escaped JSON string ends at the + # escape), a comma and the closing brackets are all excluded, because each + # of them delimits an ARN far more often than it occurs inside one. + r"(?P[^\s\"'`\\,<>|)\]}]*)" ) +# Trailing characters stripped from a reported ARN. A sentence ending in an ARN +# leaves the full stop inside the match, and a truncated ARN leaves a dangling +# colon. Only the reported string is trimmed; the account was already read from +# its own field, so the trim cannot change what is detected. +ARN_TRAILING_PUNCTUATION = ".;:" + # ``i-`` plus either of the two legal lengths, seventeen hexadecimal characters # (current) or eight (the old form), with the seventeen-character alternative # first so the longer match wins. @@ -138,7 +196,38 @@ re.IGNORECASE, ) +# The only file name whose contents are read for the two journal sources of +# evidence. DevOps Agent writes its journal to this name, and the two field +# positions below are part of that journal's schema — so reading them out of +# some other JSON file would be reading a field that means something else. +JOURNAL_FILE_NAME = "journal_records.json" + +# The journal nests JSON inside JSON: a record's ``content`` is a string holding +# more JSON, which can itself hold a string holding more JSON. The walk parses +# each such string and recurses, and this caps how far down it goes, so a +# pathological or self-referential file cannot spin the scan. +JOURNAL_MAX_DEPTH = 16 + +# The journal block whose tool result is keyed by account ID. +TOOL_SUMMARY_BLOCK = "tool_summary" + +# The journal field whose value is an account ID. +JOURNAL_ACCOUNT_FIELD = "aws_account_id" + +# Fallback for the ``aws_account_id`` field when the walk above cannot reach it: +# a truncated journal that will not parse, or a record whose escaping is deeper +# than the walk unwinds. The separator class covers every form the field takes +# in a real journal — ``"aws_account_id": "123456789012"``, the same escaped as +# ``\"aws_account_id\": \"123456789012\"``, and agent prose writing +# ``aws_account_id `123456789012` `` — and is bounded so the match cannot run +# from the field name across unrelated text into some other number. The closing +# guard rejects a thirteenth digit. +JOURNAL_ACCOUNT_FIELD_RE = re.compile( + JOURNAL_ACCOUNT_FIELD + r"""[\s:=,"'`()\\]{0,16}([0-9]{12})(?:_([0-9]+))?(?![0-9A-Za-z])""" +) + ACCOUNT_KIND = "AWS account ID" +ARN_KIND = "ARN naming an AWS account" INSTANCE_KIND = "EC2 instance ID" # Per-line opt-out. A line carrying this marker is skipped entirely. @@ -165,11 +254,6 @@ ".xml", } -# File types where the bare-JSON-number exclusion applies. JSON has no comment -# syntax, so a line in one of these can never carry the marker above, and the -# allowlist is the only suppression route for them. -JSON_SUFFIXES = {".json", ".jsonl"} - # Repository-relative path of the allowlist the gate workflow uses. DEFAULT_ALLOWLIST = ".github/aws-identifier-allowlist.json" @@ -179,10 +263,12 @@ MAX_PRINTED_FINDINGS = 50 REMEDIATION = ( - "To resolve a finding: redact the value, or add it to " - f"`{DEFAULT_ALLOWLIST}` with a reason explaining why it is safe to publish, " - f"or put an `{MARKER}: ` comment on the line itself. The allowlist " - "is the route for JSON files, which have no comment syntax. See the " + "To resolve a finding: redact the value — for an ARN, remove the whole ARN, " + "since blanking its account field leaves the rest of it in place — or add " + f"the account ID or instance ID to `{DEFAULT_ALLOWLIST}` with a reason " + "explaining why it is safe to publish, or put an " + f"`{MARKER}: ` comment on the line itself. The allowlist is the " + "route for JSON files, which have no comment syntax. See the " '"Scanning for AWS Identifiers" section of CONTRIBUTING.md.' ) @@ -193,7 +279,8 @@ class Finding: line: int kind: str # The token exactly as it appears on the line, redaction suffix included, so - # the message matches what the author will search for in the file. + # the message matches what the author will search for in the file. For an + # ARN finding this is the whole ARN. value: str @@ -204,7 +291,11 @@ def _repo_root() -> Path: def _git(repo_root: Path, *args: str) -> subprocess.CompletedProcess[str]: return subprocess.run( - ["git", *args], cwd=repo_root, capture_output=True, text=True + ["git", *args], + cwd=repo_root, + capture_output=True, + text=True, + errors="replace", ) @@ -234,7 +325,7 @@ def read_diff(repo_root: Path, base_ref: str, head_ref: str) -> str | None: not attributed to the pull request. * ``--find-renames`` — without it a pure rename is reported as every line being added, which would flag identifiers the pull request only moved. - * ``--unified=0`` — only added lines are scanned, so context lines are + * ``--unified=0`` — only added lines are reported on, so context lines are output nobody reads. * ``core.quotePath=false`` — a non-ASCII path is printed as itself in the ``+++`` header rather than octal-escaped, which keeps the reported file @@ -347,6 +438,39 @@ def added_lines(diff_text: str) -> list[tuple[str, int, str]]: return entries +def changed_paths(diff_text: str) -> list[str]: + """Paths of every file the diff touches, in the order the diff names them. + + This is wider than the set of paths in ``added_lines``: a file the pull + request only deletes lines from still names an ARN in the lines it keeps, + and that ARN still proves an account ID an added line elsewhere mentions + bare. A deleted file is left out, since there is nothing to read at the head + ref. + + The ``+++`` header is recognised only outside a hunk, for the same reason + ``added_lines`` tracks that state: an added line whose own text starts with + ``++ `` appears in the diff looking exactly like a file header. + """ + hunk_re = re.compile(r"^@@ -\d+(?:,\d+)? \+\d+(?:,\d+)? @@") + + paths: list[str] = [] + in_hunk = False + for raw in diff_text.splitlines(): + if raw.startswith("diff --git "): + in_hunk = False + continue + if not in_hunk and raw.startswith("+++ "): + target = raw[4:].strip() + if target != "/dev/null": + path = _strip_diff_prefix(target) + if path not in paths: + paths.append(path) + continue + if hunk_re.match(raw): + in_hunk = True + return paths + + def _strip_diff_prefix(target: str) -> str: """Drop the ``b/`` that ``git diff`` puts in front of the new file's path.""" return target[2:] if target.startswith(("a/", "b/")) else target @@ -398,7 +522,7 @@ def load_allowlist(path: Path) -> tuple[set[str], set[str], list[str]]: problems: list[str] = [] collected: dict[str, set[str]] = {"account_ids": set(), "instance_ids": set()} - patterns = {"account_ids": ACCOUNT_RE, "instance_ids": INSTANCE_RE} + patterns = {"account_ids": ACCOUNT_VALUE_RE, "instance_ids": INSTANCE_RE} for key, pattern in patterns.items(): if key not in raw: @@ -435,50 +559,213 @@ def load_allowlist(path: Path) -> tuple[set[str], set[str], list[str]]: def _normalize(value: str) -> str: """Lowercase an identifier and drop any ``_`` redaction suffix. - Comparisons against the allowlist use this form, so ``i-1234567890abcdef0``, - ``I-1234567890ABCDEF0`` and ``i-1234567890abcdef0_2`` all resolve to the one - allowlist entry. + Comparisons against the allowlist and against the proven account IDs use + this form, so ``i-1234567890abcdef0``, ``I-1234567890ABCDEF0`` and + ``i-1234567890abcdef0_2`` all resolve to the one allowlist entry, and + ``123456789012_3`` on a line matches ``123456789012`` proven in an ARN. """ return re.sub(r"_[0-9]+$", "", value).lower() -def _inside_json_string(line: str, pos: int) -> bool: - """True when ``pos`` in ``line`` falls inside a double-quoted JSON string. +def changed_file_documents( + repo_root: Path, + head_ref: str | None, + paths: list[str], + entries: list[tuple[str, int, str]], +) -> list[tuple[str, str]]: + """Return (path, whole file text at ``head_ref``) for every changed file. + + This is the input to the evidence pass, and it is wider than what the + reporting pass reads: every file the diff touches, whole, rather than the + added lines. An ARN sitting in a part of the file the pull request never + touched still proves the account ID that an added line mentions bare, and + reading it costs one ``git show``. + + ``git show`` is read-only, as is every other ``git`` call this scanner makes. + Reading from the working tree instead was rejected because ``head_ref`` is + not always what is checked out — a local run against ``pr/112`` never checks + that branch out. + + When ``head_ref`` is None the caller is scanning a saved diff with + ``--diff-file`` and there is no ref to read from, so each file's added lines, + joined back together, stand in for its contents. The same stand-in covers a + file ``git show`` cannot produce as text, a binary asset most likely. Both + cases narrow the evidence pass to the added lines, which is the scope the + reporting pass uses anyway. + """ + added_by_path: dict[str, list[str]] = {} + for path, _line_number, text in entries: + added_by_path.setdefault(path, []).append(text) + + documents: list[tuple[str, str]] = [] + for path in paths: + whole = _head_file_text(repo_root, head_ref, path) if head_ref else None + if whole is None: + whole = "\n".join(added_by_path.get(path, [])) + if whole: + documents.append((path, whole)) + return documents + + +def _head_file_text(repo_root: Path, head_ref: str, path: str) -> str | None: + """Return the text of ``path`` at ``head_ref``, or None when git cannot. + + None covers a path that is absent at that ref — a file the pull request + deletes still has a diff entry — and anything git declines to print. + Undecodable bytes are replaced rather than raising, so a binary asset + yields harmless text instead of killing the scan. + """ + result = _git(repo_root, "show", f"{head_ref}:{path}") + return result.stdout if result.returncode == 0 else None - Counts the double quotes before ``pos`` that are not escaped — a quote - preceded by an odd run of backslashes is escaped — and returns whether that - count is odd. - Used only for account IDs in ``.json``/``.jsonl`` files: AWS always writes - an account ID as a JSON string, so a twelve-digit run appearing as a bare - JSON number is a byte count or a similar measurement rather than an account - ID. Instance IDs are never bare numbers, so the exclusion never applies to - them. +def arn_accounts(text: str) -> set[str]: + """Account IDs read out of the account field of every ARN in ``text``. - The limits, stated plainly: this reads one line in isolation and is not a - JSON parse. A fragment with unbalanced quotes, a value split across lines, - or a string containing an unescaped-looking quote can be judged wrongly, and - the file type is taken from the extension rather than from the content. It - is a heuristic that trades a small chance of missing an identifier for not - failing every pull request that adds a twelve-digit byte count to an eval - result. + The field is evidence because of where it sits, not because of how it looks: + an ARN's fifth colon-separated field holds an account ID or nothing at all. + An empty field (``arn:aws:s3:::my-bucket``), the literal ``aws`` + (``arn:aws:iam::aws:policy/ReadOnlyAccess``) and anything else that is not + twelve digits yield nothing. """ - quotes = 0 - index = 0 - while index < pos: - if line[index] == "\\": - index += 2 # skip the escaped character, whatever it is - continue - if line[index] == '"': - quotes += 1 - index += 1 - return quotes % 2 == 1 + found: set[str] = set() + for match in ARN_RE.finditer(text): + account = match.group("account") + if ACCOUNT_VALUE_RE.fullmatch(account): + found.add(_normalize(account)) + return found + + +def journal_accounts(text: str) -> set[str]: + """Account IDs read out of a DevOps Agent journal, from its two fields. + + The journal nests JSON inside JSON — a record's ``content`` is a string + holding more JSON, sometimes several levels down, and the same block can + appear both as parsed structure and as an escaped string — so the two fields + are read by walking the parsed tree and parsing every string that starts a + JSON object or array along the way. A regex over the raw escaped text cannot + be the primary mechanism, because the escaping varies in depth: ``\\"``, + ``\\\\"`` and deeper all occur in real journals, and a pattern pinned to one + depth silently misses the others. + + ``JOURNAL_ACCOUNT_FIELD_RE`` then runs over the raw text as a fallback, for + a journal that will not parse at all or whose escaping outruns the walk. + """ + found: set[str] = set() + + try: + tree = json.loads(text) + except (ValueError, RecursionError): + tree = None + if tree is not None: + _walk_journal(tree, False, 0, found) + + for match in JOURNAL_ACCOUNT_FIELD_RE.finditer(text): + found.add(match.group(1)) + + return found + + +def _walk_journal( + node: object, in_tool_summary: bool, depth: int, found: set[str] +) -> None: + """Collect account IDs from a journal subtree into ``found``. + + Two positions count, and both are part of the DevOps Agent journal schema: + + * An object key of exactly twelve digits anywhere below a block whose + ``block_type`` is ``tool_summary``. The journal stores a per-account AWS + API result as a map keyed by account ID, so the key *is* the account — + ``{"123456789012": {"DBInstances": []}}`` under + ``tool_summary > tool_result > text``. + * The value of an ``aws_account_id`` field, anywhere in the tree. It appears + in a recorded tool input and in agent prose alike. + + ``in_tool_summary`` is carried down rather than recomputed, because the key + sits several levels below the block that gives it its meaning. + """ + if depth > JOURNAL_MAX_DEPTH: + return + + if isinstance(node, str): + # A string that starts an object or an array is nested JSON; anything + # else is a leaf and json.loads would only raise on it. + stripped = node.lstrip() + if stripped[:1] in ("{", "["): + try: + nested = json.loads(stripped) + except (ValueError, RecursionError): + return + _walk_journal(nested, in_tool_summary, depth + 1, found) + return + + if isinstance(node, list): + for item in node: + _walk_journal(item, in_tool_summary, depth + 1, found) + return + + if isinstance(node, dict): + inside = in_tool_summary or node.get("block_type") == TOOL_SUMMARY_BLOCK + for key, value in node.items(): + if inside and isinstance(key, str) and ACCOUNT_VALUE_RE.fullmatch(key): + found.add(_normalize(key)) + if key == JOURNAL_ACCOUNT_FIELD and isinstance(value, (str, int)): + candidate = str(value) + if ACCOUNT_VALUE_RE.fullmatch(candidate): + found.add(_normalize(candidate)) + _walk_journal(value, inside, depth + 1, found) + + +def extract_accounts(documents: list[tuple[str, str]]) -> set[str]: + """Account IDs proved by the three sources, across every changed file. + + ARNs are read in every file, whatever its type. The two journal fields are + read only in a file named ``journal_records.json``, because they are fields + of the DevOps Agent journal schema and the same names in some other file + would mean something else. + """ + proven: set[str] = set() + for path, text in documents: + proven |= arn_accounts(text) + if Path(path).name == JOURNAL_FILE_NAME: + proven |= journal_accounts(text) + return proven + + +def accounts_to_flag( + documents: list[tuple[str, str]], allowed_accounts: set[str] +) -> set[str]: + """The proven account IDs, less the ones the allowlist covers.""" + return extract_accounts(documents) - allowed_accounts + + +def account_occurrence_re(accounts: set[str]) -> re.Pattern[str] | None: + """One pattern matching any of ``accounts`` as a whole token, or None. + + The two boundary lookarounds are cheap insurance rather than the thing that + keeps false positives out — that job now belongs to the evidence pass, which + only ever hands this function a value some structure proved to be an account + ID. They cover the remaining coincidence: a proven account whose digits also + sit inside a longer token, such as a hexadecimal resource id, where the + longer token is the real reference and the account is not being named at + all. The optional ``_`` group consumes the skill evaluation tool's + redaction suffix, which matters because ``_`` is a word character and a + ``\\b``-anchored pattern would not match the suffixed form at all. + + None when nothing was proven, which lets the caller skip the search. + """ + if not accounts: + return None + alternatives = "|".join(re.escape(account) for account in sorted(accounts)) + return re.compile( + rf"(? set[str]: - """Repository-relative paths whose added lines are never scanned. + """Repository-relative paths whose added lines are never reported on. - The allowlist file itself, so that adding ``753240598075`` to it does not + The allowlist file itself, so that adding ``123456789012`` to it does not make the scanner report that very entry. Both the file in use and the default are listed, so a local run against a temporary allowlist still skips the committed one, which is the file a pull request would be editing. @@ -493,11 +780,25 @@ def _excluded_paths(repo_root: Path, allowlist: Path) -> set[str]: def scan_added_lines( entries: list[tuple[str, int, str]], - accounts: set[str], - instances: set[str], + proven_accounts: set[str], + allowed_accounts: set[str], + allowed_instances: set[str], excluded: set[str], ) -> tuple[list[Finding], list[str]]: - """Find identifiers in added lines, returning (findings, warnings). + """Report identifiers on added lines, returning (findings, warnings). + + Three things are reported, in this order: + + 1. An ARN whose account field holds a twelve-digit value the allowlist does + not cover, reported as the whole ARN. ``proven_accounts`` is not + consulted for this: the ARN on the line is its own evidence. + 2. An occurrence of any account ID in ``proven_accounts``, reported as the + token on the line. The account field of an ARN already reported above is + skipped, so one ARN produces one finding rather than two overlapping + ones. Only that field is skipped: a *different* account ID appearing in + the ARN's resource name is still reported in its own right. + 3. An EC2 instance ID, which needs no evidence — its prefix and length are + the proof. The marker is applied *after* the patterns rather than before. A line that mentions ``aws-id-ok`` but carries no identifier is suppressing nothing, so @@ -507,27 +808,41 @@ def scan_added_lines( """ findings: list[Finding] = [] warnings: list[str] = [] + occurrence_re = account_occurrence_re(proven_accounts) for path, line_number, text in entries: if path in excluded: continue - suffix = Path(path).suffix.lower() - is_json = suffix in JSON_SUFFIXES - candidates: list[Finding] = [] - - for match in ACCOUNT_RE.finditer(text): - if _normalize(match.group(1)) in accounts: + # Character ranges of the account fields of the ARNs already reported on + # this line, so the occurrence search below does not report the same + # digits a second time. + reported_account_spans: list[tuple[int, int]] = [] + + for match in ARN_RE.finditer(text): + account = match.group("account") + if not ACCOUNT_VALUE_RE.fullmatch(account): + continue # no account field, the literal `aws`, or a template + if _normalize(account) in allowed_accounts: continue - if is_json and not _inside_json_string(text, match.start()): - continue # a bare JSON number, so not an account ID - candidates.append( - Finding(path, line_number, ACCOUNT_KIND, match.group(0)) - ) + arn = match.group(0).rstrip(ARN_TRAILING_PUNCTUATION) + reported_account_spans.append(match.span("account")) + candidates.append(Finding(path, line_number, ARN_KIND, arn)) + + if occurrence_re is not None: + for match in occurrence_re.finditer(text): + if any( + start <= match.start() < end + for start, end in reported_account_spans + ): + continue + candidates.append( + Finding(path, line_number, ACCOUNT_KIND, match.group(0)) + ) for match in INSTANCE_RE.finditer(text): - if _normalize(match.group(1)) in instances: + if _normalize(match.group(1)) in allowed_instances: continue candidates.append( Finding(path, line_number, INSTANCE_KIND, match.group(0)) @@ -536,6 +851,7 @@ def scan_added_lines( if not candidates: continue + suffix = Path(path).suffix.lower() if suffix in COMMENT_BEARING_SUFFIXES and MARKER in text: if not MARKER_WITH_REASON_RE.search(text): warnings.append( @@ -619,85 +935,317 @@ def _write_summary( fh.write("\n".join(lines) + "\n") -# Boundary cases for the account-ID pattern, checked against the pattern itself -# rather than through the allowlist, so each one states a property of the -# matching and nothing else. Every entry is a string to match and the list of -# account IDs the pattern is expected to pull out of it. +# --------------------------------------------------------------------------- +# Self-check cases # -# These are here because both guards in ACCOUNT_RE suppress matches, and a -# suppressing guard that grows too broad fails silently: identifiers stop being -# reported, and a passing check is the only symptom. +# Every case below uses a documentation placeholder — `123456789012`, +# `111122223333`, `012345678901`, `i-1234567890abcdef0` — and never an invented +# real-looking value. This file is scanned by its own check, and a twelve-digit +# value in an ARN here would be a finding against the pull request that edits the +# scanner. The reporting cases get around the allowlist by running with an empty +# allowlist, which is what lets a placeholder stand in for a real account: with +# nothing allowed, `123456789012` is reported exactly as a real account ID would +# be. The suppression cases run with the allowlist in use, and assert silence. +# --------------------------------------------------------------------------- + +# (label, file path, whole file text, account IDs the evidence pass should prove) # -# Every case that is expected to match uses an allowlisted documentation -# placeholder rather than an invented twelve-digit value. The cases exercise -# ACCOUNT_RE directly, and the allowlist is applied later, in -# ``scan_added_lines`` — so a placeholder proves the pattern matches just as -# well as a real-looking number would, and this file does not become the one -# place in the repository carrying identifiers that the check itself would -# report. Allowlist suppression is covered by the second group of cases in -# ``self_check``. -ACCOUNT_PATTERN_CASES: tuple[tuple[str, list[str]], ...] = ( - # The UUID guard. Nothing to report: the twelve digits are a UUID's final - # group. - ("a618bd73-f5dc-4e6b-b1f4-123412341234", []), - # Upper-case hex is a UUID too. - ("A618BD73-F5DC-4E6B-B1F4-123412341234", []), - # The guard must not reach further than a real 8-4-4-4 hex prefix. Here the - # first group is not hex, so this is not a UUID and the digits are reported. - ("zzzzzzzz-f5dc-4e6b-b1f4-123456789012", ["123456789012"]), - # A hyphen before the digits is not a UUID on its own. - ("stack-1234-111122223333", ["111122223333"]), - # The alphanumeric guard. A twelve-digit run inside a longer hex token - # belongs to the token, not to an account. - ("eni-097816109986f5e1d", []), - ("vol-0a1b2c3d123456789012", []), - # Still reported in the places an account ID actually appears. - ("arn:aws:iam::123456789012:role/Example", ["123456789012"]), - ('"accountId": "123456789012",', ["123456789012"]), - ("123456789012", ["123456789012"]), - # A thirteen-digit millisecond timestamp has no twelve-digit window to find. - ("1727000000000", []), - # The redaction suffix is consumed, and the identifier reported without it. - ("012345678901_7", ["012345678901"]), +# These pin the evidence pass, whose failures are silent: an account ID that +# stops being proved stops being reported, and a passing check is the only +# symptom. +EXTRACTION_CASES: tuple[tuple[str, str, str, list[str]], ...] = ( + ( + "resource-id form: account is the fifth field", + "docs/example.md", + "arn:aws:sns:us-east-1:123456789012:example-topic", + ["123456789012"], + ), + ( + "resource-type/resource-id form", + "docs/example.md", + "arn:aws:iam::123456789012:role/Example", + ["123456789012"], + ), + ( + "resource-type:resource-id form", + "docs/example.md", + "arn:aws:states:us-east-1:123456789012:execution:Example:run-1", + ["123456789012"], + ), + ( + "a partition other than aws parses the same way", + "docs/example.md", + "arn:aws-us-gov:ec2:us-gov-west-1:111122223333:volume/vol-0abcd", + ["111122223333"], + ), + ( + "an empty account field proves nothing", + "docs/example.md", + "arn:aws:s3:::my-internal-bucket and arn:aws:health:us-west-2::event/XYZ", + [], + ), + ( + "the literal aws in the account field proves nothing", + "docs/example.md", + "arn:aws:iam::aws:policy/ReadOnlyAccess", + [], + ), + ( + "a redaction suffix in the account field is stripped", + "docs/example.md", + "arn:aws:iam::012345678901_3:role/Example", + ["012345678901"], + ), + ( + "ARNs are read in any file type, journal fields are not", + "evals/benchmark.json", + '{"aws_account_id": "123456789012"}', + [], + ), + ( + "the aws_account_id field of a journal, in a parsed tool input", + "outputs/journal_records.json", + '[{"content": "{\\"input\\": {\\"aws_account_id\\": \\"123456789012\\"}}"}]', + ["123456789012"], + ), + ( + "the aws_account_id field of a journal, in agent prose", + "outputs/journal_records.json", + '[{"content": "I queried aws_account_id `111122223333` for events."}]', + ["111122223333"], + ), + ( + "a twelve-digit object key under a tool_summary block, nested twice", + "outputs/journal_records.json", + '[{"content": "{\\"block_type\\": \\"tool_summary\\", \\"content\\": ' + '[{\\"type\\": \\"tool_result\\", \\"content\\": [{\\"text\\": ' + '\\"{\\\\\\"123456789012\\\\\\": {\\\\\\"DBInstances\\\\\\": []}}\\"}]}]}"}]', + ["123456789012"], + ), + ( + "the same key outside a tool_summary block proves nothing", + "outputs/journal_records.json", + '[{"content": "{\\"block_type\\": \\"load_skill\\", \\"content\\": ' + '{\\"123456789012\\": {}}}"}]', + [], + ), + ( + "a journal that will not parse falls back to the field pattern", + "outputs/journal_records.json", + '[{"content": "{\\"aws_account_id\\": \\"111122223333\\"', + ["111122223333"], + ), +) + +# (label, file path, whole file text, added line, finding values expected) +# +# Run with an empty allowlist, so a placeholder behaves as a real account ID +# would. The file text is the evidence pass's input and the added line is the +# reporting pass's, which is how a case can prove an account in an untouched +# part of a file and then report it where the pull request names it. +REPORTING_CASES: tuple[tuple[str, str, str, str, list[str]], ...] = ( + ( + "an ARN on an added line is reported whole, not just its account", + "docs/report.md", + "", + "See arn:aws:iam::123456789012:role/Example for the trust policy.", + ["arn:aws:iam::123456789012:role/Example"], + ), + ( + "an account-less ARN is not reported", + "docs/report.md", + "", + "Grant access to arn:aws:s3:::my-internal-bucket/*", + [], + ), + ( + "an ARN naming the AWS-owned policy account is not reported", + "docs/report.md", + "", + "Attach arn:aws:iam::aws:policy/ReadOnlyAccess to the role.", + [], + ), + ( + "a second account inside an ARN's resource name is reported too", + "docs/report.md", + "arn:aws:eks:us-east-1:111122223333:cluster/example", + "arn:aws:iam::123456789012:role/audit-for-111122223333", + [ + "arn:aws:iam::123456789012:role/audit-for-111122223333", + "111122223333", + ], + ), + ( + "an account proved elsewhere in the file is reported bare in prose", + "docs/report.md", + "arn:aws:eks:us-east-1:111122223333:cluster/example", + "The cluster runs in account 111122223333.", + ["111122223333"], + ), + ( + "a proved account is reported with its redaction suffix too", + "outputs/journal_records.json", + '[{"content": "{\\"aws_account_id\\": \\"012345678901\\"}"}]', + '{"message": "events for 012345678901_2"}', + ["012345678901_2"], + ), + ( + "twelve digits nothing proved are not reported", + "docs/report.md", + "arn:aws:s3:::my-internal-bucket", + "A different number entirely: 123456789012.", + [], + ), + # The five shapes the old twelve-digit rule reported, plus a thirteen-digit + # epoch. Each one carries the placeholder's digits and no evidence at all, + # so what keeps it silent is the thing this change introduced: nothing + # proved those digits. Each illustration is synthetic — a UUID of repeated + # letters, a round datestamp, a counter of zeros — rather than an instance + # copied out of the repository's committed eval output, since a real + # resource name in this file would outlive the pull request that cited it. + ( + "the last group of a UUID is not an account ID", + "docs/report.md", + "", + "execution aaaaaaaa-bbbb-cccc-dddd-123456789012 finished", + [], + ), + ( + "the fractional digits of a decimal are not an account ID", + "outputs/metrics.json", + "", + '{"latency": 46526.123456789012}', + [], + ), + ( + "the integer part of a decimal is not an account ID", + "outputs/metrics.json", + "", + '{"bytes": 123456789012.7833}', + [], + ), + ( + "a YYYYMMDDHHMM datestamp in a resource name is not an account ID", + "docs/report.md", + "", + "job nightly-training-run-200001010000 completed", + [], + ), + ( + "a zero-padded counter is not an account ID", + "outputs/events.json", + "", + '{"destinationId": "destinationId-000000000001"}', + [], + ), + ( + "a thirteen-digit millisecond timestamp is not an account ID", + "docs/report.md", + "", + "recorded at 1234567890123", + [], + ), + # This one does exercise the boundary lookarounds: the file proves the + # account through an ARN, and the added line holds the same digits inside a + # longer hexadecimal resource id, where they belong to the id and name no + # account. The ENI id is built from the placeholder for the same reason as + # above. + ( + "a proved account's digits inside a longer hex id are not reported", + "docs/report.md", + "arn:aws:ec2:us-east-1:123456789012:instance/example", + "the interface eni-0123456789012abcd is attached", + [], + ), + ( + "an instance ID needs no evidence", + "docs/report.md", + "", + "Instance i-1234567890abcdef0 stopped responding.", + ["i-1234567890abcdef0"], + ), + ( + "an AMI id is not an instance id", + "docs/report.md", + "", + "Launched from ami-0abcdef1234567890 in the same subnet.", + [], + ), ) def self_check(accounts: set[str], instances: set[str]) -> list[str]: - """Confirm the pattern boundaries hold and every allowlist entry is suppressed. + """Confirm the two passes behave and every allowlist entry is suppressed. - Two groups of cases, both covering failures that are silent — they stop + Three groups of cases, all covering failures that are silent — they stop identifiers from being *reported* rather than start reporting things falsely, so a passing check is the only symptom and no pull request reveals them. - The first group walks ACCOUNT_PATTERN_CASES, which pins the two guards in - ACCOUNT_RE: a UUID's final group is not an account ID, and neither is a - twelve-digit run inside a longer hex token, while an account ID in an ARN, - in JSON, or after a plain hyphen still is. - - The second group walks every entry in the allowlist in use, builds a - synthetic added line for the bare form and for a ``_7``-suffixed form, and - reports any that produced a finding. The redaction suffix is the part of the - matching that is easiest to get wrong: a ``\\b``-anchored pattern does not - match ``i-1234567890abcdef0_3`` at all. + EXTRACTION_CASES pins the evidence pass: the three ARN resource forms, an + account-less ARN, the ``aws`` account field, a non-``aws`` partition, both + journal fields including the nested and escaped ``tool_summary`` shape, and + the redaction suffix. + + REPORTING_CASES pins the reporting pass end to end with an empty allowlist: + an ARN reported whole, a proved account found bare in prose and with a + suffix, an instance ID, and each of the five lookalike classes that used to + be reported by the old twelve-digit shape rule and must now stay silent. + Each case carries the file text the evidence pass reads and, separately, the + added line the reporting pass reads, which is how a case can prove an + account in a part of a file the pull request never touched and then see it + reported where the pull request names it. + + The third group walks every entry in the allowlist in use. For an account ID + it builds a file whose text is an ARN naming that entry, which is what makes + the entry proved, and then an added line mentioning it bare; nothing may be + reported, since the allowlist drops the entry from the proven set and + suppresses the ARN as well. For an instance ID the ARN is inert — an + instance ID in an account field is not twelve digits — and the added line is + what matters. Both the bare and the ``_7``-suffixed form are tried, because + the suffix is the part of the matching that is easiest to get wrong. Run it with ``--self-check``. It reads nothing but the allowlist and runs no ``git`` command. """ failures: list[str] = [] - for text, expected in ACCOUNT_PATTERN_CASES: - found = [match.group(1) for match in ACCOUNT_RE.finditer(text)] - if found != expected: + for label, path, text, expected_accounts in EXTRACTION_CASES: + proven = sorted(extract_accounts([(path, text)])) + if proven != sorted(expected_accounts): + failures.append( + f"evidence pass, {label}: proved {proven or 'nothing'}, " + f"expected {sorted(expected_accounts) or 'nothing'}" + ) + + for label, path, text, line, expected_values in REPORTING_CASES: + # An empty file text means the added line is the whole file, which is + # how a real run sees a one-line addition: the line itself is part of + # the evidence pass's input. + documents = [(path, text or line)] + proven = accounts_to_flag(documents, set()) + findings, _ = scan_added_lines( + [(path, 1, line)], proven, set(), set(), set() + ) + reported = sorted(finding.value for finding in findings) + if reported != sorted(expected_values): failures.append( - f"account pattern on `{text}` produced {found or 'no match'}, " - f"expected {expected or 'no match'}" + f"reporting pass, {label}: reported {reported or 'nothing'}, " + f"expected {sorted(expected_values) or 'nothing'}" ) for identifier in sorted(accounts | instances): for value in (identifier, f"{identifier}_7"): - entries = [("docs/self-check.md", 1, f"value {value} on a line")] - findings, _ = scan_added_lines(entries, accounts, instances, set()) + path = "docs/self-check.md" + documents = [(path, f"arn:aws:iam::{identifier}:role/Example")] + proven = accounts_to_flag(documents, accounts) + findings, _ = scan_added_lines( + [(path, 1, f"value {value} on a line")], + proven, + accounts, + instances, + set(), + ) if findings: failures.append( f"allowlisted `{identifier}` was still reported as " @@ -734,16 +1282,19 @@ def main(argv: list[str] | None = None) -> int: metavar="PATH", help=( "Scan a saved unified diff instead of running git; `-` reads stdin. " - "Intended for testing the scanner against a fixture." + "Intended for testing the scanner against a fixture. With a saved " + "diff there is no head ref to read whole files from, so the " + "evidence pass sees only the added lines." ), ) parser.add_argument( "--self-check", action="store_true", help=( - "Scan nothing; instead confirm every allowlist entry is suppressed " - "both bare and with a `_N` redaction suffix, and exit non-zero if " - "any is not." + "Scan nothing; instead run the scanner's own cases over the " + "evidence pass, the reporting pass, and every allowlist entry both " + "bare and with a `_N` redaction suffix, and exit non-zero if any " + "fails." ), ) args = parser.parse_args(argv) @@ -766,9 +1317,9 @@ def main(argv: list[str] | None = None) -> int: _error(failure) total = len(accounts) + len(instances) print( - f"Self-check: {len(ACCOUNT_PATTERN_CASES)} account pattern " - f"boundary case(s), {total} allowlist entr(ies) bare and " - f"`_N`-suffixed, {len(failures)} failure(s)." + f"Self-check: {len(EXTRACTION_CASES)} evidence case(s), " + f"{len(REPORTING_CASES)} reporting case(s), {total} allowlist " + f"entr(ies) bare and `_N`-suffixed, {len(failures)} failure(s)." ) return 1 if failures else 0 @@ -785,8 +1336,23 @@ def main(argv: list[str] | None = None) -> int: _write_summary([], [], problems) return 2 if problems else 0 + # Evidence first, over the whole of every changed file; then reporting, over + # the added lines alone. A saved diff has no ref to read files from, so the + # evidence pass narrows to the added lines there. + documents = changed_file_documents( + repo_root, + None if args.diff_file else args.head_ref, + changed_paths(diff_text), + entries, + ) + proven_accounts = accounts_to_flag(documents, accounts) + findings, warnings = scan_added_lines( - entries, accounts, instances, _excluded_paths(repo_root, allowlist) + entries, + proven_accounts, + accounts, + instances, + _excluded_paths(repo_root, allowlist), ) for finding in findings[:MAX_PRINTED_FINDINGS]: @@ -804,7 +1370,12 @@ def main(argv: list[str] | None = None) -> int: _write_summary(findings, warnings, problems) print( - f"\n{len(entries)} added line(s) scanned: {len(findings)} finding(s).", + f"\n{len(documents)} changed file(s) read for evidence: " + f"{len(proven_accounts)} account ID(s) proved and not allowlisted.", + file=sys.stderr, + ) + print( + f"{len(entries)} added line(s) scanned: {len(findings)} finding(s).", file=sys.stderr, ) diff --git a/.kiro/steering/project-conventions.md b/.kiro/steering/project-conventions.md index d08eab69..5069fd45 100644 --- a/.kiro/steering/project-conventions.md +++ b/.kiro/steering/project-conventions.md @@ -193,11 +193,14 @@ Only these extensions are permitted inside **skill** directories (enforced by `s A pull request check (`.github/workflows/scan-aws-identifiers.yml`, running `.github/scripts/scan_aws_identifiers.py`) reports unredacted AWS identifiers and applies to every file in the repository, not only to skills. See the "Scanning for AWS Identifiers" section of [CONTRIBUTING.md](../../CONTRIBUTING.md). -- Two patterns are matched: an AWS account ID as exactly twelve digits with no letter or digit on either side, and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters. -- Two account-ID lookalikes are deliberately not reported, both present in committed eval output: the last group of a UUID (`a618bd73-f5dc-4e6b-b1f4-123412341234`, since about one UUID in 250 ends in twelve digits) and a twelve-digit run inside a longer hexadecimal token (`eni-097816109986f5e1d`, and likewise `subnet-`, `vol-`, `sg-`, `snap-`, `ami-`). Only a full `8-4-4-4-` hex prefix is skipped, so `stack-1234-111122223333` is still reported. Both boundaries are pinned by cases in the scanner's `--self-check`, which the check runs on every pull request. -- **Only the lines a pull request adds are scanned.** `main` already carries real-looking identifiers in committed eval results and example ARNs, so a whole-file scan would fail contributors for content they did not write. Renames are detected, so moving such a file reports nothing. -- Three ways to resolve a finding: redact the value; add it to `.github/aws-identifier-allowlist.json` with a mandatory reason; or put an `aws-id-ok: ` comment on the line, which works in the comment-bearing file types only. JSON has no comment syntax, so the allowlist is the route for JSON content. -- The allowlist parser fails closed: invalid JSON, a missing key, or a blank reason grants no allowance and fails the check, so a typo cannot silently waive a leak. The skill evaluation tool's redaction placeholders `012345678901` and `i-1234567890abcdef0` are allowlisted, suffixed forms included. +- An account ID is reported only when something proves it is one; twelve digits on their own are not evidence. Measured across the open pull requests, 54% of the twelve-digit runs on their added lines named no account — a `YYYYMMDDHHMM` datestamp in a resource name, the fractional or integer part of a decimal, a zero-padded counter. The scan therefore runs in two passes. +- **Pass one gathers evidence and reports nothing**, from exactly three sources: the account field of an [ARN](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) (the fifth colon-separated field); an object key in the tool result of a `tool_summary` block, in a `journal_records.json` file, where DevOps Agent keys a per-account API result by account ID; and the value of an `aws_account_id` field, in a `journal_records.json` file. The last two are fields of the DevOps Agent journal schema, which is why they are read only in that file name. Other spellings such as `AccountId` and `accountId` are deliberately not read. Pass one reads every changed file whole, so an ARN in an untouched part of a file still proves an account ID an added line names bare. +- **Pass two reports, on added lines only:** an ARN whose account field holds a non-allowlisted twelve-digit value, reported as the **whole ARN** rather than the account segment (blanking one field would leave the rest of the ARN in place, and a surviving copy of the account ID elsewhere in the file would rebuild it); every occurrence of an account ID pass one proved, in any file type; and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters, which needs no evidence. +- An ARN with no account field is not reported — `arn:aws:s3:::my-internal-bucket`, `arn:aws:iam::aws:policy/...`. There is no reliable way to tell a real bucket from an example one, since anybody may own `arn:aws:s3:::example-bucket`, so reporting them would be noise; redacting a real one is the author's job and catching it is a human reviewer's. Write example ARNs with `123456789012` or `111122223333`, both allowlisted. +- The known gap: an account ID that appears only as prose in a file that is not a journal is proved by none of the three sources and is not reported. Both passes and the lookalikes that must stay silent are pinned by cases in the scanner's `--self-check`, which the check runs on every pull request. +- **Only the lines a pull request adds are reported on.** `main` already carries real identifiers in committed eval results and example ARNs, so reporting on whole files would fail contributors for content they did not write. Renames are detected, so moving such a file reports nothing. +- Three ways to resolve a finding: redact the value, removing the whole ARN for an ARN finding; add it to `.github/aws-identifier-allowlist.json` with a mandatory reason; or put an `aws-id-ok: ` comment on the line, which works in the comment-bearing file types only. JSON has no comment syntax, so the allowlist is the route for JSON content. +- The allowlist parser fails closed: invalid JSON, a missing key, or a blank reason grants no allowance and fails the check, so a typo cannot silently waive a leak. An allowlisted account ID is dropped from the proven set, which silences both bare occurrences of it and any ARN naming it. The skill evaluation tool's redaction placeholders `012345678901` and `i-1234567890abcdef0` are allowlisted, suffixed forms included. - The `needs-id-redact` label is applied by automation when the check fails and removed when it passes. Do not add or remove it by hand. - Identifiers already on `main` are a separate follow-up change. Do not clean them up opportunistically in an unrelated pull request — the check never reports them, since it only reads added lines. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f31acec6..5b306b49 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -191,19 +191,35 @@ Test relevant scenarios with and without the custom agent, multiple times. Focus A pull request check ([`.github/workflows/scan-aws-identifiers.yml`](.github/workflows/scan-aws-identifiers.yml)) looks for unredacted AWS identifiers in what your pull request adds. An account ID or instance ID committed here names a real resource in a real account, and once it's on `main` it's in the public history for good. It applies to every file in the repository, not just to skills. -Two things are matched: +**Twelve digits on their own are not an account ID.** Measured across the open pull requests in this repository, 54% of the twelve-digit runs on their added lines named no account at all, in four shapes: a `YYYYMMDDHHMM` datestamp used as a resource-name suffix, the fractional digits of a decimal, the integer part of a decimal, and a zero-padded counter. A twelve-digit run inside a longer hexadecimal resource ID, such as an ENI ID, is a fifth. So the check only reports an account ID when something proves it is one, and it works in two passes. -| Identifier | Shape | +**Pass one gathers evidence and reports nothing.** Three places put a value where only an account ID sits: + +| Source | What it looks like | | --- | --- | -| AWS account ID | exactly twelve digits with no letter or digit on either side, so `arn:aws:iam::123456789012:role/Example` matches while a thirteen-digit number doesn't | -| EC2 instance ID | `i-` followed by either eight hexadecimal characters (the old form) or seventeen (the current one), case-insensitive | +| The account field of an [ARN](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) | the fifth colon-separated field, as in `arn:aws:iam::123456789012:role/Example` | +| An object key in the tool result of a `tool_summary` block, in a `journal_records.json` file | DevOps Agent records a per-account AWS API result as a map keyed by account ID: `{"123456789012": {"DBInstances": []}}` | +| The value of an `aws_account_id` field, in a `journal_records.json` file | `"aws_account_id": "123456789012"`, in a recorded tool input or in agent prose | + +The two journal sources read fields of the DevOps Agent journal schema, which is why they're read only in a file named `journal_records.json` — the same names elsewhere mean something else. Other spellings an API response or a person might use, `AccountId` and `accountId` among them, are deliberately not read. + +Pass one reads every file your pull request changes, whole, not just the added lines. So an account ID you write bare on an added line is still caught when the ARN that proves it sits in a part of the file you never touched. -Two things that look like an account ID are deliberately not reported, because both occur in this repository's committed eval output and neither names an account: +**Pass two reports, and only on the lines your pull request adds:** + +| Reported | Detail | +| --- | --- | +| An ARN whose account field holds a twelve-digit value that isn't allowlisted | the finding is the **whole ARN**, not the account segment — blanking that one field would leave the region, the service and the resource name in place, and a surviving copy of the account ID elsewhere in the file would be enough to rebuild the ARN, so the remedy is to remove the ARN | +| Any occurrence of an account ID pass one proved | anywhere on an added line, in any file type, which is what catches the same account written bare in prose | +| An EC2 instance ID | `i-` followed by either eight hexadecimal characters (the old form) or seventeen (the current one), case-insensitive. An instance ID needs no evidence: its prefix and length are the proof | -- **The last group of a UUID.** A UUID ends in twelve hexadecimal characters, so roughly one in 250 ends in twelve digits — `a618bd73-f5dc-4e6b-b1f4-123412341234`. Only a full `8-4-4-4-` hex prefix is skipped, so an account ID that merely follows a hyphen, as in `stack-1234-111122223333`, is still reported. -- **A twelve-digit run inside a longer hexadecimal token.** `eni-097816109986f5e1d` is a network interface, and the twelve digits sitting in the middle of it name no account. The same applies to the other resource IDs built from a hex blob: `subnet-`, `vol-`, `sg-`, `snap-`, `ami-`. +**An ARN with no account field isn't reported.** `arn:aws:s3:::my-internal-bucket` and `arn:aws:iam::aws:policy/ReadOnlyAccess` name no account, and there's no reliable way to tell a real bucket from an example one — anybody may own `arn:aws:s3:::my-internal-bucket` or `arn:aws:s3:::example-bucket`, so reporting them would be noise. Redacting a real S3 bucket ARN is yours to do, and catching one is a human reviewer's. -**Only the lines your pull request adds are scanned.** Removed lines, context lines, and lines you didn't touch are all ignored. That's deliberate: `main` already carries real-looking identifiers in committed eval results and in example ARNs, so a whole-file scan would fail you for content you never wrote, and the only way to go green would be to clean up someone else's lines. Renames are detected too, so moving a file that already contains identifiers adds no lines and reports nothing. +**The gap, so a passing check isn't mistaken for a guarantee:** an account ID that appears only as prose in a file that isn't a journal — "the cluster in account 123456789012", with no ARN anywhere in that file — is proved by none of the three sources and isn't reported. If you know a value is real, redact it whether or not the check flags it. + +When you write an example ARN in documentation, use `123456789012` or `111122223333`. Both are allowlisted, so an ARN naming either is never reported. + +**Only the lines your pull request adds are reported on.** Removed lines, context lines, and lines you didn't touch are all ignored. That's deliberate: `main` already carries real identifiers in committed eval results and in example ARNs, so reporting on whole files would fail you for content you never wrote, and the only way to go green would be to clean up someone else's lines. Renames are detected too, so moving a file that already contains identifiers adds no lines and reports nothing. The redaction placeholders the skill evaluation tool writes — `012345678901` for an account ID and `i-1234567890abcdef0` for an instance ID — are allowlisted already, with or without the `_N` suffix the tool adds to tell two different originals apart. @@ -211,7 +227,7 @@ The redaction placeholders the skill evaluation tool writes — `012345678901` f Three routes, and the first one is usually right: -1. **Redact the value.** Replace it with one of the placeholders above, or with an obvious stand-in. +1. **Redact the value.** Replace it with one of the placeholders above, or with an obvious stand-in. For an ARN finding, remove the whole ARN rather than emptying its account field: the rest of the ARN still names a real resource, and the account ID is likely written somewhere else in the same file. 2. **Add it to the allowlist**, if the value is genuinely safe to publish — an AWS documentation example, or an AWS-owned account such as the one that publishes a public Lambda layer. Add it to [`.github/aws-identifier-allowlist.json`](.github/aws-identifier-allowlist.json) with a reason, which is mandatory and is what a reviewer judges the entry on: @@ -226,7 +242,7 @@ Three routes, and the first one is usually right: } ``` - The file fails closed: invalid JSON, a missing key, or a blank reason grants nothing at all and fails the check, rather than silently waiving an entry. This is also the only route for JSON files, which have no comment syntax. + The file fails closed: invalid JSON, a missing key, or a blank reason grants nothing at all and fails the check, rather than silently waiving an entry. This is also the only route for JSON files, which have no comment syntax. An allowlisted account ID is dropped from the proven set, which silences both the bare occurrences of it and any ARN that names it. 3. **Mark the line**, where the value belongs in place and an allowlist entry would be too broad — a value that only makes sense in the one line it's on. Put an `aws-id-ok: ` comment on it: @@ -236,15 +252,15 @@ Three routes, and the first one is usually right: The marker is honored in `.md`, `.yaml`, `.yml`, `.py`, `.sh`, `.ts`, `.js`, `.html`, `.htm`, and `.xml` files. A bare `aws-id-ok` with no reason still suppresses the line, but the check reports a warning asking for one. -One false positive is suppressed without any of that: a twelve-digit number appearing as a bare JSON value in a `.json` or `.jsonl` file, as in `"bytes": 322122547200` — a three-hundred-gigabyte volume size. AWS always writes an account ID as a string, so a bare number of that length is a byte count or a similar measurement. A twelve-digit run inside a quoted JSON string is still reported. - ### Running it locally ```bash python3 .github/scripts/scan_aws_identifiers.py --base-ref origin/main --head-ref HEAD ``` -Findings are printed as `FOUND : `, and in the check's log they also appear as annotations on the pull request diff and as a summary table on the workflow run page. `--self-check` confirms the allowlist parses and that every entry in it is suppressed in both its bare and `_N`-suffixed forms. +Findings are printed as `FOUND : `, where the kind is `ARN naming an AWS account`, `AWS account ID` or `EC2 instance ID`. In the check's log they also appear as annotations on the pull request diff and as a summary table on the workflow run page. The run also reports how many account IDs pass one proved, which is the quickest way to see why a value was or wasn't picked up. + +`--self-check` runs the scanner's own cases over both passes — ARN parsing, the two journal sources, the occurrence search, and the lookalikes that must stay silent — and confirms every allowlist entry is suppressed in both its bare and `_N`-suffixed forms. The exit code tells the two failure modes apart: `1` means identifiers were found, `2` means the scan couldn't be trusted — a malformed allowlist, or a diff it couldn't read. From 5fbf3744f277a678ede5b222138e3e259c02ebfb Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Sun, 4 Oct 2026 21:55:37 +0300 Subject: [PATCH 08/14] fix: read the account of an ARN whose region is a wildcard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the evidence-based account ID detection found two blocking gaps and two inaccurate comments. ARN_RE pinned the region to [a-z0-9-]* and the account to [0-9A-Za-z_-]*, so an ARN with * in either field failed to parse at all and its twelve-digit account field was never read — neither as evidence nor as a finding. That shape is how an IAM policy scopes a resource across regions, and this repository ships seven policy JSON files. A policy file naming a real account proved nothing and was reported as nothing. Every field before the resource now accepts *, so such an ARN parses and is judged on its account field: twelve digits are proved and reported whole, and a wildcard or a template expression stays silent because it names no account, not because the parse failed. .agents/ is now gitignored. The directory this change created holds working notes that quote real account IDs as prose, in files that are not journals and carry no ARN — the one gap this scan documents as uncatchable, so a commit of them would pass clean. Two comments are reworded: JOURNAL_ACCOUNT_FIELD_RE runs on every journal alongside the tree walk rather than only when the walk fails, and the extraction case claiming ARNs are read in any file type now carries an ARN in its fixture. CONTRIBUTING.md's ARN source row names the wildcard case, for the contributor writing a policy reference file. Verification, all re-run after the fix: - --self-check exits 0: 15 evidence cases, 18 reporting cases (two of each are new and cover the wildcard fields), 12 allowlist entries bare and _N-suffixed, 0 failures. - The branch passes its own scan against origin/main, 0 findings, exit 0; the working tree was scanned the same way through --diff-file before committing. - All 33 open pull requests: the same five fail and 28 pass as before the fix, finding for finding, so the wider parse costs no new findings on them. None of the four removed false-positive classes is reported anywhere. - The whole main tree: 9016 findings, unchanged, across the same three ARN-proven accounts and four instance IDs. Every wildcard-region ARN on main holds * or ${AWS::AccountId} in its account field, so none of them reports. - The four local eval directories report exactly what they reported before. - A fixture shaped like a skill's references/iam-policy.json, two statements with wildcard regions naming one account: 0 proved and 0 findings before the fix, 1 proved and 2 whole-ARN findings after. - The scanner still performs no repository write: git diff and git show only, and one append to $GITHUB_STEP_SUMMARY. --- .github/scripts/scan_aws_identifiers.py | 81 ++++++++++++++++++++----- .gitignore | 7 +++ CONTRIBUTING.md | 2 +- 3 files changed, 74 insertions(+), 16 deletions(-) diff --git a/.github/scripts/scan_aws_identifiers.py b/.github/scripts/scan_aws_identifiers.py index 1982fdc1..3789bc8c 100755 --- a/.github/scripts/scan_aws_identifiers.py +++ b/.github/scripts/scan_aws_identifiers.py @@ -164,12 +164,24 @@ # the three in practice: ``aws``, ``aws-cn``, ``aws-us-gov``. The region may be # empty. The resource may be empty too, so a truncated ARN that stops right # after the account still yields its account. +# +# Every field before the resource also accepts ``*``, because an IAM policy +# scopes a resource by wildcarding whichever fields it does not care about: +# ``arn:aws:logs:*:123456789012:log-group:/aws/lambda/x`` is the ordinary way to +# name a log group in every region, and this repository ships seven policy JSON +# files written that way. Without ``*`` in the region class the regex demands a +# ``:`` where the ``*`` sits, the whole match fails, and the account field is +# never read at all — so a policy file naming a real account would prove nothing +# and be reported as nothing. A wildcard in the partition or the account field +# should also be silent, but silent because the account field is not twelve +# digits, which is a judgement ``arn_accounts`` makes, rather than because the +# ARN failed to parse. ARN_RE = re.compile( r"arn:" - r"(?Paws(?:-[a-z0-9]+)*):" - r"(?P[a-z0-9][a-z0-9-]*):" - r"(?P[a-z0-9-]*):" - r"(?P[0-9A-Za-z_-]*):" + r"(?P\*|aws(?:-[a-z0-9*]+)*):" + r"(?P[a-z0-9*][a-z0-9*-]*):" + r"(?P[a-z0-9*-]*):" + r"(?P[0-9A-Za-z_*-]*):" # The resource ends where the surrounding text begins. Whitespace, a quote, # a backtick, a backslash (an ARN inside an escaped JSON string ends at the # escape), a comma and the closing brackets are all excluded, because each @@ -214,9 +226,14 @@ # The journal field whose value is an account ID. JOURNAL_ACCOUNT_FIELD = "aws_account_id" -# Fallback for the ``aws_account_id`` field when the walk above cannot reach it: -# a truncated journal that will not parse, or a record whose escaping is deeper -# than the walk unwinds. The separator class covers every form the field takes +# A second reading of the ``aws_account_id`` field, straight off the raw text. +# It runs on every journal, alongside the walk rather than instead of it, and it +# exists to cover the journals the walk cannot reach: a truncated journal that +# will not parse, or a record whose escaping is deeper than the walk unwinds. +# Reading the same field twice costs one pass over the text and can only add the +# value the walk would have added anyway. +# +# The separator class covers every form the field takes # in a real journal — ``"aws_account_id": "123456789012"``, the same escaped as # ``\"aws_account_id\": \"123456789012\"``, and agent prose writing # ``aws_account_id `123456789012` `` — and is bounded so the match cannot run @@ -648,8 +665,13 @@ def journal_accounts(text: str) -> set[str]: ``\\\\"`` and deeper all occur in real journals, and a pattern pinned to one depth silently misses the others. - ``JOURNAL_ACCOUNT_FIELD_RE`` then runs over the raw text as a fallback, for - a journal that will not parse at all or whose escaping outruns the walk. + ``JOURNAL_ACCOUNT_FIELD_RE`` then reads the same ``aws_account_id`` field a + second time, straight off the raw text. It runs on every journal, not only + on one the walk failed to parse, because deciding in advance whether the + walk reached every record would cost more than reading the text again. What + it is there for is the journal the walk cannot reach: one truncated + mid-record, or one whose escaping is deeper than the walk unwinds. It can + only add the value the walk would have added anyway. """ found: set[str] = set() @@ -996,11 +1018,24 @@ def _write_summary( "arn:aws:iam::012345678901_3:role/Example", ["012345678901"], ), + ( + "a wildcard region still yields the account, as an IAM policy writes it", + "skills/example/references/iam-policy.json", + '{"Resource": "arn:aws:logs:*:123456789012:log-group:/aws/lambda/x:*"}', + ["123456789012"], + ), + ( + "a wildcard in the partition or the account field yields nothing", + "skills/example/references/iam-policy.json", + '{"Resource": ["arn:*:s3:*:*:accesspoint/example", "arn:aws:iam::*:role/X"]}', + [], + ), ( "ARNs are read in any file type, journal fields are not", "evals/benchmark.json", - '{"aws_account_id": "123456789012"}', - [], + '{"cluster": "arn:aws:eks:us-east-1:111122223333:cluster/example",' + ' "aws_account_id": "123456789012"}', + ["111122223333"], ), ( "the aws_account_id field of a journal, in a parsed tool input", @@ -1051,6 +1086,13 @@ def _write_summary( "See arn:aws:iam::123456789012:role/Example for the trust policy.", ["arn:aws:iam::123456789012:role/Example"], ), + ( + "an IAM policy resource with a wildcard region is reported whole", + "skills/example/references/iam-policy.json", + "", + ' "Resource": "arn:aws:logs:*:123456789012:log-group:/aws/lambda/x:*"', + ["arn:aws:logs:*:123456789012:log-group:/aws/lambda/x:*"], + ), ( "an account-less ARN is not reported", "docs/report.md", @@ -1058,6 +1100,13 @@ def _write_summary( "Grant access to arn:aws:s3:::my-internal-bucket/*", [], ), + ( + "a wildcard account field is not reported", + "skills/example/references/iam-policy.json", + "", + ' "Resource": "arn:aws:iam::*:role/ExampleRole"', + [], + ), ( "an ARN naming the AWS-owned policy account is not reported", "docs/report.md", @@ -1183,12 +1232,14 @@ def self_check(accounts: set[str], instances: set[str]) -> list[str]: them. EXTRACTION_CASES pins the evidence pass: the three ARN resource forms, an - account-less ARN, the ``aws`` account field, a non-``aws`` partition, both - journal fields including the nested and escaped ``tool_summary`` shape, and - the redaction suffix. + account-less ARN, the ``aws`` account field, a non-``aws`` partition, the + wildcard fields an IAM policy resource is written with, both journal fields + including the nested and escaped ``tool_summary`` shape, and the redaction + suffix. REPORTING_CASES pins the reporting pass end to end with an empty allowlist: - an ARN reported whole, a proved account found bare in prose and with a + an ARN reported whole, an IAM policy resource whose region is a wildcard + reported whole as well, a proved account found bare in prose and with a suffix, an instance ID, and each of the five lookalike classes that used to be reported by the old twelve-digit shape rule and must now stay silent. Each case carries the file text the evidence pass reads and, separately, the diff --git a/.gitignore b/.gitignore index cfc85125..fc26e1a1 100644 --- a/.gitignore +++ b/.gitignore @@ -32,5 +32,12 @@ docs/mcp-servers/*.md .claude/* !.claude/CLAUDE.md +# Agent task notes and review write-ups. These are working notes for a single +# change, and they quote whatever the agent was looking at — including real AWS +# account IDs in prose, which the AWS identifier scan cannot prove and so cannot +# catch. Keeping the directory out of git is the enforcement the note's own +# "do not commit" header is not. +.agents/ + # Holmes .holmesignore diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5b306b49..ec60c6b3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -197,7 +197,7 @@ A pull request check ([`.github/workflows/scan-aws-identifiers.yml`](.github/wor | Source | What it looks like | | --- | --- | -| The account field of an [ARN](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) | the fifth colon-separated field, as in `arn:aws:iam::123456789012:role/Example` | +| The account field of an [ARN](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) | the fifth colon-separated field, as in `arn:aws:iam::123456789012:role/Example`. A `*` in the partition, service or region field is read the same way, so an IAM policy resource such as `arn:aws:logs:*:123456789012:log-group:/aws/lambda/x:*` still proves its account | | An object key in the tool result of a `tool_summary` block, in a `journal_records.json` file | DevOps Agent records a per-account AWS API result as a map keyed by account ID: `{"123456789012": {"DBInstances": []}}` | | The value of an `aws_account_id` field, in a `journal_records.json` file | `"aws_account_id": "123456789012"`, in a recorded tool input or in agent prose | From 8f9e86cf8b4618f4e04ee48a322234d9aab9b0aa Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Sun, 4 Oct 2026 22:25:05 +0300 Subject: [PATCH 09/14] fix: read the account of an ARN whose region is a template expression The ARN pattern lines up five colon-separated fields to find the account in the fifth. A CloudFormation template expression carries a colon pair of its own, so a region written as ${AWS::Region} left the fields unalignable, the whole match failed, and the account standing next to it was never read -- neither proved as an account nor reported as a finding. The field that matters there is not the templated one. A SAM template names a published Lambda layer as arn:aws:lambda:${AWS::Region}::layer:X, so the account beside the expression is a literal twelve digits, and a failed parse made it invisible. Every field before the resource now accepts a ${...} expression as one unit, braces and inner colons included, matched ahead of the plainer character classes so those classes never stop at a colon inside the braces. A template expression in the account field itself still yields nothing, but now because the field is not twelve digits rather than because the ARN did not parse -- which is the distinction the comment beside the pattern had claimed and did not have. That comment is corrected, and the placeholder syntaxes still not covered in a field before the resource are named there and in the contributor documentation: {Region}, and %REGION% all fail to parse, so an account beside one of those goes unreported. Four lines in this repository's SAM templates were affected, all naming the AWS-owned Lambda Web Adapter account, which is allowlisted -- so they proved nothing before and report nothing now, and no finding changes. Verified against every open pull request: the same five report and the same twenty-eight are clean, finding for finding. The self-check gains four cases, two proving the account beside a templated region is read and two proving a templated account field stays silent. --- .github/scripts/scan_aws_identifiers.py | 59 ++++++++++++++++++++++--- CONTRIBUTING.md | 2 +- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/.github/scripts/scan_aws_identifiers.py b/.github/scripts/scan_aws_identifiers.py index 3789bc8c..cd85ce08 100755 --- a/.github/scripts/scan_aws_identifiers.py +++ b/.github/scripts/scan_aws_identifiers.py @@ -156,9 +156,23 @@ # (``arn:aws:s3:::my-bucket``), the literal ``aws`` # (``arn:aws:iam::aws:policy/ReadOnlyAccess``) or a redaction suffix # (``012345678901_1``) all still parse — and are then handled on their merits. -# An ARN whose account is a template expression such as ``${AWS::AccountId}`` -# does not parse at all, because the braces hold colons of their own; that costs -# nothing, since such an ARN names no account either. +# Every field before the resource also accepts a CloudFormation template +# expression such as ``${AWS::Region}``, which is why ARN_TEMPLATE_FIELD below +# swallows the braces whole: ``${AWS::Region}`` carries a ``::`` of its own, so +# without it the fields cannot be lined up and the match fails outright. The +# field that matters there is not the templated one but the account beside it -- +# ``arn:aws:lambda:${AWS::Region}::layer:X`` is how a SAM +# template names a published layer, and a failed parse means that literal +# account is neither proved nor reported. A template expression in the account +# field itself stays silent, but silent because the field is not twelve digits, +# which is a judgement ``arn_accounts`` makes, rather than because the ARN +# failed to parse. +# +# Only the ``${...}`` form is covered, because that is the one this repository +# uses. An ARN written with another placeholder syntax in a field before the +# resource -- ``{Region}``, ````, ``%REGION%`` -- still fails to parse, +# and an account standing beside one of those would go unreported. See the +# documented gaps in CONTRIBUTING.md. # # The partition is ``aws`` plus any number of lowercase suffixes, which covers # the three in practice: ``aws``, ``aws-cn``, ``aws-us-gov``. The region may be @@ -176,12 +190,17 @@ # should also be silent, but silent because the account field is not twelve # digits, which is a judgement ``arn_accounts`` makes, rather than because the # ARN failed to parse. +# +# A ``${...}`` template expression, matched before the plainer classes so the +# braces are consumed as one unit rather than left to a class that would stop at +# the first ``:`` inside them. +ARN_TEMPLATE_FIELD = r"\$\{[^}]*\}" ARN_RE = re.compile( r"arn:" - r"(?P\*|aws(?:-[a-z0-9*]+)*):" - r"(?P[a-z0-9*][a-z0-9*-]*):" - r"(?P[a-z0-9*-]*):" - r"(?P[0-9A-Za-z_*-]*):" + r"(?P" + ARN_TEMPLATE_FIELD + r"|\*|aws(?:-[a-z0-9*]+)*):" + r"(?P" + ARN_TEMPLATE_FIELD + r"|[a-z0-9*][a-z0-9*-]*):" + r"(?P" + ARN_TEMPLATE_FIELD + r"|[a-z0-9*-]*):" + r"(?P" + ARN_TEMPLATE_FIELD + r"|[0-9A-Za-z_*-]*):" # The resource ends where the surrounding text begins. Whitespace, a quote, # a backtick, a backslash (an ARN inside an escaped JSON string ends at the # escape), a comma and the closing brackets are all excluded, because each @@ -1030,6 +1049,18 @@ def _write_summary( '{"Resource": ["arn:*:s3:*:*:accesspoint/example", "arn:aws:iam::*:role/X"]}', [], ), + ( + "a template expression in the region still yields the account beside it", + "mcp/example/template.yaml", + " - !Sub arn:aws:lambda:${AWS::Region}:123456789012:layer:Example:24", + ["123456789012"], + ), + ( + "a template expression in the account field itself yields nothing", + "mcp/example/template.yaml", + " - !Sub arn:aws:iam::${AWS::AccountId}:role/Example", + [], + ), ( "ARNs are read in any file type, journal fields are not", "evals/benchmark.json", @@ -1107,6 +1138,20 @@ def _write_summary( ' "Resource": "arn:aws:iam::*:role/ExampleRole"', [], ), + ( + "an ARN whose region is a template expression is reported whole", + "mcp/example/template.yaml", + "", + " - !Sub arn:aws:lambda:${AWS::Region}:123456789012:layer:Example:24", + ["arn:aws:lambda:${AWS::Region}:123456789012:layer:Example:24"], + ), + ( + "an ARN whose account is a template expression is not reported", + "mcp/example/template.yaml", + "", + " - !Sub arn:aws:iam::${AWS::AccountId}:role/Example", + [], + ), ( "an ARN naming the AWS-owned policy account is not reported", "docs/report.md", diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ec60c6b3..d5746864 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -215,7 +215,7 @@ Pass one reads every file your pull request changes, whole, not just the added l **An ARN with no account field isn't reported.** `arn:aws:s3:::my-internal-bucket` and `arn:aws:iam::aws:policy/ReadOnlyAccess` name no account, and there's no reliable way to tell a real bucket from an example one — anybody may own `arn:aws:s3:::my-internal-bucket` or `arn:aws:s3:::example-bucket`, so reporting them would be noise. Redacting a real S3 bucket ARN is yours to do, and catching one is a human reviewer's. -**The gap, so a passing check isn't mistaken for a guarantee:** an account ID that appears only as prose in a file that isn't a journal — "the cluster in account 123456789012", with no ARN anywhere in that file — is proved by none of the three sources and isn't reported. If you know a value is real, redact it whether or not the check flags it. +**Two gaps, so a passing check isn't mistaken for a guarantee.** An account ID that appears only as prose in a file that isn't a journal — "the cluster in account 123456789012", with no ARN anywhere in that file — is proved by none of the three sources and isn't reported. And an ARN is only read when the check can line its fields up, which it can do for a wildcard (`arn:aws:logs:*:123456789012:log-group:x`) and for a CloudFormation `${...}` expression (`arn:aws:lambda:${AWS::Region}:123456789012:layer:X`), but not for another placeholder syntax in a field before the resource — `{Region}`, ``, `%REGION%` — where the whole ARN fails to parse and an account standing beside one goes unreported. If you know a value is real, redact it whether or not the check flags it. When you write an example ARN in documentation, use `123456789012` or `111122223333`. Both are allowlisted, so an ARN naming either is never reported. From f37dfcbdbca83cc39cdccc647ce8c9e5e22a2423 Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Sun, 4 Oct 2026 22:34:13 +0300 Subject: [PATCH 10/14] chore: ignore eval run metadata and local git worktrees The skill evaluation tool writes _metadata.json at the root of each functional version directory. It records how the run was invoked -- region, iteration count, local skill directory -- and the CloudFormation stacks the tool created, each named by a full stack ARN. Those stack ARNs carry the account the evaluation ran in, which makes this the one file in an eval run that names a real account and keeps it. The tool's redaction pass replaces account IDs in the agent's own output, but _metadata.json comes from the harness rather than from the agent, so the redaction never reaches it. Of the two copies committed today, one names a real account and the other a documentation placeholder; every local run inspected names the account its evaluation used. Nothing reads the file after the run. The evals validation check lists it among the files it deliberately does not look at, so ignoring it changes no other check. The pattern is spelled out as **/evals/functional/*/_metadata.json rather than **/_metadata.json so the name stays committable elsewhere under skills/, matching how the sibling patterns for outputs/ are scoped. Verified that a fresh _metadata.json under a version directory is ignored while benchmark.json and evals.json beside it stay committable. Also ignores .worktrees/. An agent working on a branch in parallel with the checkout creates a worktree there, and each one is a full second copy of the tree, so committing one would nest the repository inside itself. Git keeps a worktree's administrative files under .git/worktrees instead, so ignoring the directory loses nothing. This does not untrack the two _metadata.json files already committed -- ignoring a path has no effect on a path git is already tracking. Removing them is a separate change. --- .gitignore | 7 +++++++ skills/.gitignore | 13 +++++++++++++ 2 files changed, 20 insertions(+) diff --git a/.gitignore b/.gitignore index fc26e1a1..481bb783 100644 --- a/.gitignore +++ b/.gitignore @@ -39,5 +39,12 @@ docs/mcp-servers/*.md # "do not commit" header is not. .agents/ +# Git worktrees. An agent working on a branch in parallel with the checkout +# creates one here, and each is a full second copy of the tree, so committing one +# would nest the repository inside itself. Git keeps the worktree's own +# administrative files elsewhere, under .git/worktrees, so ignoring this +# directory loses nothing. +.worktrees/ + # Holmes .holmesignore diff --git a/skills/.gitignore b/skills/.gitignore index 41ebaf08..bc4590ec 100644 --- a/skills/.gitignore +++ b/skills/.gitignore @@ -42,3 +42,16 @@ # outputs/ so these common filenames stay committable elsewhere under skills/. **/outputs/classified_output.json **/outputs/metadata.json + +# Run metadata the skill evaluation tool writes at the root of each functional +# version directory. It records how the run was invoked — the region, the +# iteration count, the local skill directory — and the CloudFormation stacks the +# tool created, each named by a full stack ARN. Those ARNs carry the account the +# evaluation ran in, which is the one place an eval run names a real account that +# no redaction pass touches, because the file comes from the harness rather than +# from the agent. Nothing reads it afterwards: the evals validation check lists +# it among the files it deliberately does not look at. +# +# The path is spelled out rather than matched as **/_metadata.json so the name +# stays committable elsewhere under skills/. +**/evals/functional/*/_metadata.json From 8c641b2e308c56fea5ba2a02a3e0abb1a95dbf0c Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Sun, 4 Oct 2026 23:04:41 +0300 Subject: [PATCH 11/14] docs: tell contributors to redact their own AWS identifiers Until now the three contributor documents described the pull request check and nothing else: what it reports, what it deliberately stays quiet about, and how to resolve a finding. Read together they implied the tooling is the control, so a contributor whose pull request goes green had no reason to look further. The tooling is not the control, and two of its limits are the reason. The skill evaluation tool's redaction pass rewrites the agent's own output and only that -- a hand-written evals.json prompt is never touched, nor is the _metadata.json the harness writes at the root of each functional version directory, and a third-party account inside a returned API payload has been seen passing through unredacted while the operating account in the same sentence was replaced. The check, by design, reports an account ID only where something proves it is one, so a green check means nothing was proved rather than that nothing is there. CONTRIBUTING.md gains a "Redacting AWS Identifiers" section, placed ahead of the one describing the check so the contributor's own duty is read first. It gives the replacement for each kind of identifier -- a documentation example account, the example instance ID, an ARN rewritten whole rather than blanked in one field, and a generic stand-in for any other real resource name -- then lists the four places real values arrive, and closes with why neither piece of tooling is enough. The Kiro steering file and .claude/CLAUDE.md carry the same instruction in the short form those two documents use. Agent output is named as three files per run, not one: journal_records.json, benchmark.json, and each scenario's functional-tests-results.json, which quotes the output again as the evidence for every assertion it judged. The same identifier usually appears in more than one, so fixing the journal and assuming the rest followed leaves the value behind. Of the eval result files committed today, instance IDs and account-bearing ARNs appear in functional-tests-results.json across two skills. Also corrects a stale line in both short documents, which still described a single known gap. There are two: an account ID written only as prose in a file that is not a journal, and an ARN whose fields cannot be lined up because a field before the resource uses a placeholder syntax the check does not parse. CONTRIBUTING.md was already updated when the second gap was found. No behaviour changes. Every example uses an allowlisted documentation placeholder, and the three files pass the check they describe. --- .claude/CLAUDE.md | 10 ++++++++- .kiro/steering/project-conventions.md | 10 ++++++++- CONTRIBUTING.md | 32 +++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index ae47ef69..8397778b 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -192,13 +192,21 @@ Only these extensions are permitted inside **skill** directories (enforced by `s ## AWS Identifiers +**Redact real AWS identifiers before committing, rather than leaving it to either piece of tooling.** The repository is public and a commit is permanent — removing a value in a later commit does not remove it from the history. Replace an account ID with `123456789012` or `111122223333`, an instance ID with `i-0123456789abcdef0`, and an ARN by rewriting it whole, account and region and resource name together, since blanking the account field leaves the rest of it standing. Do the same for any other real resource name: a cluster, bucket, database, stack or role name describes the environment to a reader and cannot be told apart from an example by automation. See the "Redacting AWS Identifiers" section of [CONTRIBUTING.md](../CONTRIBUTING.md). + +Neither piece of tooling is sufficient on its own: + +- The skill evaluation tool's redaction pass rewrites the agent's own output and only that. A hand-written `evals.json` prompt is never touched; neither is the `_metadata.json` the harness writes at the root of each functional version directory, whose stack ARNs name the account the run used (gitignored for that reason). A third-party account inside a returned API payload has been observed passing through unredacted while the operating account in the same sentence was replaced. +- Agent output lands in three files per run: `journal_records.json`, `benchmark.json`, and each scenario's `functional-tests-results.json`, which quotes the output again as the evidence for every assertion. The same identifier usually appears in more than one, so check each rather than fixing the journal and assuming the rest followed. +- The check reports an account ID only where something proves it is one, so a green check means nothing was proved, not that nothing is there. + A pull request check (`.github/workflows/scan-aws-identifiers.yml`, running `.github/scripts/scan_aws_identifiers.py`) reports unredacted AWS identifiers and applies to every file in the repository, not only to skills. See the "Scanning for AWS Identifiers" section of [CONTRIBUTING.md](../CONTRIBUTING.md). - An account ID is reported only when something proves it is one; twelve digits on their own are not evidence. Measured across the open pull requests, 54% of the twelve-digit runs on their added lines named no account — a `YYYYMMDDHHMM` datestamp in a resource name, the fractional or integer part of a decimal, a zero-padded counter. The scan therefore runs in two passes. - **Pass one gathers evidence and reports nothing**, from exactly three sources: the account field of an [ARN](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) (the fifth colon-separated field); an object key in the tool result of a `tool_summary` block, in a `journal_records.json` file, where DevOps Agent keys a per-account API result by account ID; and the value of an `aws_account_id` field, in a `journal_records.json` file. The last two are fields of the DevOps Agent journal schema, which is why they are read only in that file name. Other spellings such as `AccountId` and `accountId` are deliberately not read. Pass one reads every changed file whole, so an ARN in an untouched part of a file still proves an account ID an added line names bare. - **Pass two reports, on added lines only:** an ARN whose account field holds a non-allowlisted twelve-digit value, reported as the **whole ARN** rather than the account segment (blanking one field would leave the rest of the ARN in place, and a surviving copy of the account ID elsewhere in the file would rebuild it); every occurrence of an account ID pass one proved, in any file type; and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters, which needs no evidence. - An ARN with no account field is not reported — `arn:aws:s3:::my-internal-bucket`, `arn:aws:iam::aws:policy/...`. There is no reliable way to tell a real bucket from an example one, since anybody may own `arn:aws:s3:::example-bucket`, so reporting them would be noise; redacting a real one is the author's job and catching it is a human reviewer's. Write example ARNs with `123456789012` or `111122223333`, both allowlisted. -- The known gap: an account ID that appears only as prose in a file that is not a journal is proved by none of the three sources and is not reported. Both passes and the lookalikes that must stay silent are pinned by cases in the scanner's `--self-check`, which the check runs on every pull request. +- Two known gaps. An account ID appearing only as prose in a file that is not a journal is proved by none of the three sources and is not reported. And an ARN is read only where its fields can be lined up, which works for a `*` wildcard and for a CloudFormation `${...}` expression but not for another placeholder syntax in a field before the resource — `{Region}`, ``, `%REGION%` — where the whole ARN fails to parse and a literal account standing beside one goes unreported. Both passes and the lookalikes that must stay silent are pinned by cases in the scanner's `--self-check`, which the check runs on every pull request. - **Only the lines a pull request adds are reported on.** `main` already carries real identifiers in committed eval results and example ARNs, so reporting on whole files would fail contributors for content they did not write. Renames are detected, so moving such a file reports nothing. - Three ways to resolve a finding: redact the value, removing the whole ARN for an ARN finding; add it to `.github/aws-identifier-allowlist.json` with a mandatory reason; or put an `aws-id-ok: ` comment on the line, which works in the comment-bearing file types only. JSON has no comment syntax, so the allowlist is the route for JSON content. - The allowlist parser fails closed: invalid JSON, a missing key, or a blank reason grants no allowance and fails the check, so a typo cannot silently waive a leak. An allowlisted account ID is dropped from the proven set, which silences both bare occurrences of it and any ARN naming it. The skill evaluation tool's redaction placeholders `012345678901` and `i-1234567890abcdef0` are allowlisted, suffixed forms included. diff --git a/.kiro/steering/project-conventions.md b/.kiro/steering/project-conventions.md index 5069fd45..5675cb17 100644 --- a/.kiro/steering/project-conventions.md +++ b/.kiro/steering/project-conventions.md @@ -191,13 +191,21 @@ Only these extensions are permitted inside **skill** directories (enforced by `s ## AWS Identifiers +**Redact real AWS identifiers before committing, rather than leaving it to either piece of tooling.** The repository is public and a commit is permanent — removing a value in a later commit does not remove it from the history. Replace an account ID with `123456789012` or `111122223333`, an instance ID with `i-0123456789abcdef0`, and an ARN by rewriting it whole, account and region and resource name together, since blanking the account field leaves the rest of it standing. Do the same for any other real resource name: a cluster, bucket, database, stack or role name describes the environment to a reader and cannot be told apart from an example by automation. See the "Redacting AWS Identifiers" section of [CONTRIBUTING.md](../../CONTRIBUTING.md). + +Neither piece of tooling is sufficient on its own: + +- The skill evaluation tool's redaction pass rewrites the agent's own output and only that. A hand-written `evals.json` prompt is never touched; neither is the `_metadata.json` the harness writes at the root of each functional version directory, whose stack ARNs name the account the run used (gitignored for that reason). A third-party account inside a returned API payload has been observed passing through unredacted while the operating account in the same sentence was replaced. +- Agent output lands in three files per run: `journal_records.json`, `benchmark.json`, and each scenario's `functional-tests-results.json`, which quotes the output again as the evidence for every assertion. The same identifier usually appears in more than one, so check each rather than fixing the journal and assuming the rest followed. +- The check reports an account ID only where something proves it is one, so a green check means nothing was proved, not that nothing is there. + A pull request check (`.github/workflows/scan-aws-identifiers.yml`, running `.github/scripts/scan_aws_identifiers.py`) reports unredacted AWS identifiers and applies to every file in the repository, not only to skills. See the "Scanning for AWS Identifiers" section of [CONTRIBUTING.md](../../CONTRIBUTING.md). - An account ID is reported only when something proves it is one; twelve digits on their own are not evidence. Measured across the open pull requests, 54% of the twelve-digit runs on their added lines named no account — a `YYYYMMDDHHMM` datestamp in a resource name, the fractional or integer part of a decimal, a zero-padded counter. The scan therefore runs in two passes. - **Pass one gathers evidence and reports nothing**, from exactly three sources: the account field of an [ARN](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) (the fifth colon-separated field); an object key in the tool result of a `tool_summary` block, in a `journal_records.json` file, where DevOps Agent keys a per-account API result by account ID; and the value of an `aws_account_id` field, in a `journal_records.json` file. The last two are fields of the DevOps Agent journal schema, which is why they are read only in that file name. Other spellings such as `AccountId` and `accountId` are deliberately not read. Pass one reads every changed file whole, so an ARN in an untouched part of a file still proves an account ID an added line names bare. - **Pass two reports, on added lines only:** an ARN whose account field holds a non-allowlisted twelve-digit value, reported as the **whole ARN** rather than the account segment (blanking one field would leave the rest of the ARN in place, and a surviving copy of the account ID elsewhere in the file would rebuild it); every occurrence of an account ID pass one proved, in any file type; and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters, which needs no evidence. - An ARN with no account field is not reported — `arn:aws:s3:::my-internal-bucket`, `arn:aws:iam::aws:policy/...`. There is no reliable way to tell a real bucket from an example one, since anybody may own `arn:aws:s3:::example-bucket`, so reporting them would be noise; redacting a real one is the author's job and catching it is a human reviewer's. Write example ARNs with `123456789012` or `111122223333`, both allowlisted. -- The known gap: an account ID that appears only as prose in a file that is not a journal is proved by none of the three sources and is not reported. Both passes and the lookalikes that must stay silent are pinned by cases in the scanner's `--self-check`, which the check runs on every pull request. +- Two known gaps. An account ID appearing only as prose in a file that is not a journal is proved by none of the three sources and is not reported. And an ARN is read only where its fields can be lined up, which works for a `*` wildcard and for a CloudFormation `${...}` expression but not for another placeholder syntax in a field before the resource — `{Region}`, ``, `%REGION%` — where the whole ARN fails to parse and a literal account standing beside one goes unreported. Both passes and the lookalikes that must stay silent are pinned by cases in the scanner's `--self-check`, which the check runs on every pull request. - **Only the lines a pull request adds are reported on.** `main` already carries real identifiers in committed eval results and example ARNs, so reporting on whole files would fail contributors for content they did not write. Renames are detected, so moving such a file reports nothing. - Three ways to resolve a finding: redact the value, removing the whole ARN for an ARN finding; add it to `.github/aws-identifier-allowlist.json` with a mandatory reason; or put an `aws-id-ok: ` comment on the line, which works in the comment-bearing file types only. JSON has no comment syntax, so the allowlist is the route for JSON content. - The allowlist parser fails closed: invalid JSON, a missing key, or a blank reason grants no allowance and fails the check, so a typo cannot silently waive a leak. An allowlisted account ID is dropped from the proven set, which silences both bare occurrences of it and any ARN naming it. The skill evaluation tool's redaction placeholders `012345678901` and `i-1234567890abcdef0` are allowlisted, suffixed forms included. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d5746864..423df74a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -187,6 +187,38 @@ If an issue lands with you, verify the skill against the checklist in it. When i Test relevant scenarios with and without the custom agent, multiple times. Focus on quality and consistency of the output, compared to asking DevOps Agent the same question using chat. When checking consistency, the output doesn't have to be the same verbatim - focus on the substance +## Redacting AWS Identifiers + +**Redact your own AWS identifiers before you commit.** This repository is public, and a commit is permanent: once an account ID, an ARN or an instance ID is on `main` it is in the history for good, and deleting it in a later commit does not remove it. Redacting is yours to do. Two pieces of tooling help — the skill evaluation tool's redaction pass, and the pull request check described in the next section — and neither covers everything. + +Replace, in every file you add or change: + +| Identifier | Replace with | +| --- | --- | +| An AWS account ID | `123456789012` or `111122223333`, the AWS documentation example accounts | +| An ARN | the whole ARN, not only its account field. An ARN also carries a region, a service and a resource name, so blanking the account leaves most of it standing. Rewrite it with an example account and a generic resource name: `arn:aws:eks:us-east-1:123456789012:cluster/example-cluster` | +| An EC2 instance ID | `i-0123456789abcdef0` | +| Any other real resource name | something plainly generic. A cluster name, an S3 bucket name, a database identifier, a stack name and a role name each describe your environment to a reader, and none of them is something automation can tell apart from an example | + +Where real identifiers tend to arrive: + +1. **A hand-written eval prompt in `evals.json`.** You write the prompt, so no tool ever rewrites it. A prompt naming the cluster you tested against publishes that cluster's ARN. +2. **Agent output recorded by the skill evaluation tool**, in three files per run: `journal_records.json` holds the DevOps Agent journal, `benchmark.json` holds the scores and the output they were scored on, and each scenario's `functional-tests-results.json` quotes the output again as the evidence for every assertion it judged. All three are where the tool's redaction does most of its work, and where anything it misses ends up. The same identifier usually appears in more than one of them, so check each rather than fixing the journal and assuming the rest followed. +3. **Anything pasted from the console, the CLI or a support case** into a skill, a reference document, a README or a pull request description. +4. **Example IAM policies and CloudFormation snippets** in documentation, which carry ARNs by nature. + +### Why the tooling is not enough + +**The skill evaluation tool's redaction pass rewrites the agent's own output, and only that.** It replaces an account ID with `012345678901` and an instance ID with `i-1234567890abcdef0`, adding `_1`, `_2` and so on to tell two different originals apart. Three things it does not reach: + +- A file it did not write. Your hand-written `evals.json` prompt is untouched, and so is the run metadata the harness writes at the root of each functional version directory — `_metadata.json`, whose CloudFormation stack ARNs name the account the evaluation ran in. That file is gitignored for this reason, so a fresh run no longer carries it into the repository. +- An account that is not the one the run operated in. A third-party account appearing inside a returned API payload has been observed passing through unredacted while the operating account in the same sentence was replaced. +- Anything outside an eval run. + +**The check only reports an account ID when something proves it is one**, which is what keeps it quiet about the twelve-digit numbers that are not accounts. The cost is two gaps, both described in the next section: an account ID written only as prose in a file that is not a journal is proved by nothing and goes unreported, and an ARN using a placeholder syntax the check cannot parse takes the account beside it down with it. An ARN with no account field is not reported at all, deliberately. + +So a green check means nothing was proved, not that nothing is there. Read your own diff before you push, and look hardest at what you pasted rather than at what you wrote. + ## Scanning for AWS Identifiers A pull request check ([`.github/workflows/scan-aws-identifiers.yml`](.github/workflows/scan-aws-identifiers.yml)) looks for unredacted AWS identifiers in what your pull request adds. An account ID or instance ID committed here names a real resource in a real account, and once it's on `main` it's in the public history for good. It applies to every file in the repository, not just to skills. From ffef4d165ba623d6053bed68adcd406a6aba5872 Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Mon, 5 Oct 2026 10:05:51 +0300 Subject: [PATCH 12/14] docs: correct the reasons on three allowlisted identifiers Every allowlist entry carries a reason, and reviewing that reason is the only control on what the allowlist waives. Three of them described the wrong thing, or only half of it. i-0123456789abcdef0 was described as the AWS documentation example instance ID. It is not: AWS documents i-1234567890abcdef0, both on the EC2 resource ID page and in its API examples. The entry is still worth keeping, and its shape is reason enough -- a counting run through the hexadecimal digits. The reason now adds that the value is used as an example instance ID throughout this repository, which is an observation about where it already appears rather than a convention anyone settled on. i-1234567890abcdef0 and 012345678901 were each described only as the skill evaluation tool's redaction placeholder, which both are. Both are also values AWS documents, so an author copying an example from AWS lands on them, and a reader of either reason alone would think the entry narrower than it is. The account ID appears on the AWS account identifiers page and in the Account Management reference; the instance ID on the EC2 resource ID page. No value is added or removed, so nothing changes about what the check reports. Verified that the file still parses and the scanner's self-check passes, which it would not if a reason were left blank. --- .github/aws-identifier-allowlist.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/aws-identifier-allowlist.json b/.github/aws-identifier-allowlist.json index df010edb..a8880ebb 100644 --- a/.github/aws-identifier-allowlist.json +++ b/.github/aws-identifier-allowlist.json @@ -4,13 +4,13 @@ "111122223333": "AWS documentation example account ID", "444455556666": "AWS documentation example account ID", "123456789010": "Example account ID used throughout this repository", - "012345678901": "Skill evaluation tool redaction placeholder for account IDs", + "012345678901": "AWS documentation example account ID, and the skill evaluation tool's redaction placeholder for account IDs", "753240598075": "AWS-owned account that publishes the Lambda Web Adapter layer; referenced legitimately in SAM templates", "602401143452": "AWS-owned Amazon EKS ECR registry account for us-east-1" }, "instance_ids": { - "i-1234567890abcdef0": "Skill evaluation tool redaction placeholder for EC2 instance IDs", - "i-0123456789abcdef0": "AWS documentation example instance ID", + "i-1234567890abcdef0": "AWS documentation example instance ID, and the skill evaluation tool's redaction placeholder for EC2 instance IDs", + "i-0123456789abcdef0": "Obvious placeholder, a counting run through the hexadecimal digits; used as an example instance ID throughout this repository", "i-0aaaaaaaaaaaaaaaa": "Obvious placeholder, repeated character", "i-0a1b2c3d4e5f67890": "Obvious placeholder, ascending digit pattern", "i-0fedcba9876543210": "Obvious placeholder, descending hexadecimal pattern" From dec55ba2201f69ae7910c8f2f59db212b94d3840 Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Mon, 5 Oct 2026 10:10:41 +0300 Subject: [PATCH 13/14] chore: allowlist two placeholder instance IDs Scanning every open pull request for identifiers that are plainly fabricated but absent from the allowlist turned up exactly two, both in a hand-written eval fixture: i-0a1b2c3d4e5f60011 and i-0a1b2c3d4e5f60022. They are the ascending digits interleaved with letters that i-0a1b2c3d4e5f67890 already uses, ending in a repeated pair instead. Reporting them would fail a pull request over values that name nothing. That is the failure worth avoiding most while this check is new, because an author who meets it has nothing to redact and no way to tell the check is wrong rather than themselves. The account side needed no such additions. Every plainly fabricated account ID found in the tree or on an added line was already allowlisted, so these two are the whole gap. With them in, four open pull requests report and twenty-nine are clean, down from five reporting. The four that remain all name real identifiers. --- .github/aws-identifier-allowlist.json | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/aws-identifier-allowlist.json b/.github/aws-identifier-allowlist.json index a8880ebb..d4331225 100644 --- a/.github/aws-identifier-allowlist.json +++ b/.github/aws-identifier-allowlist.json @@ -13,6 +13,8 @@ "i-0123456789abcdef0": "Obvious placeholder, a counting run through the hexadecimal digits; used as an example instance ID throughout this repository", "i-0aaaaaaaaaaaaaaaa": "Obvious placeholder, repeated character", "i-0a1b2c3d4e5f67890": "Obvious placeholder, ascending digit pattern", + "i-0a1b2c3d4e5f60011": "Obvious placeholder, the same ascending digits interleaved with letters, ending in a repeated pair", + "i-0a1b2c3d4e5f60022": "Obvious placeholder, the same ascending digits interleaved with letters, ending in a repeated pair", "i-0fedcba9876543210": "Obvious placeholder, descending hexadecimal pattern" } } From f579b1e0f567790c7460ba2277adbf2ae7867d2f Mon Sep 17 00:00:00 2001 From: Udi Dahan Date: Mon, 5 Oct 2026 10:30:37 +0300 Subject: [PATCH 14/14] docs: name the text field the account ID is a key of The three contributor documents described the second evidence source as an object key in the tool result of a tool_summary block. That skips the step a reader needs. The tool result holds a text field, the text field holds JSON as a string, and the account ID is a key of that JSON -- which is the path the scanner's own docstring spells out as tool_summary > tool_result > text. Without naming the text field, the description reads as though the key sits directly in the tool result, so anyone checking the behaviour against a real journal looks in the wrong place. The example now shows the field holding its escaped string rather than the parsed object. Wording only. No change to what is read or reported. --- .claude/CLAUDE.md | 2 +- .kiro/steering/project-conventions.md | 2 +- CONTRIBUTING.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 8397778b..1ee341ab 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -203,7 +203,7 @@ Neither piece of tooling is sufficient on its own: A pull request check (`.github/workflows/scan-aws-identifiers.yml`, running `.github/scripts/scan_aws_identifiers.py`) reports unredacted AWS identifiers and applies to every file in the repository, not only to skills. See the "Scanning for AWS Identifiers" section of [CONTRIBUTING.md](../CONTRIBUTING.md). - An account ID is reported only when something proves it is one; twelve digits on their own are not evidence. Measured across the open pull requests, 54% of the twelve-digit runs on their added lines named no account — a `YYYYMMDDHHMM` datestamp in a resource name, the fractional or integer part of a decimal, a zero-padded counter. The scan therefore runs in two passes. -- **Pass one gathers evidence and reports nothing**, from exactly three sources: the account field of an [ARN](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) (the fifth colon-separated field); an object key in the tool result of a `tool_summary` block, in a `journal_records.json` file, where DevOps Agent keys a per-account API result by account ID; and the value of an `aws_account_id` field, in a `journal_records.json` file. The last two are fields of the DevOps Agent journal schema, which is why they are read only in that file name. Other spellings such as `AccountId` and `accountId` are deliberately not read. Pass one reads every changed file whole, so an ARN in an untouched part of a file still proves an account ID an added line names bare. +- **Pass one gathers evidence and reports nothing**, from exactly three sources: the account field of an [ARN](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) (the fifth colon-separated field); an object key inside the `text` field of a `tool_summary` block's tool result, in a `journal_records.json` file, where `text` holds JSON as a string and DevOps Agent keys a per-account API result by account ID, so the key itself is the account; and the value of an `aws_account_id` field, in a `journal_records.json` file. The last two are fields of the DevOps Agent journal schema, which is why they are read only in that file name. Other spellings such as `AccountId` and `accountId` are deliberately not read. Pass one reads every changed file whole, so an ARN in an untouched part of a file still proves an account ID an added line names bare. - **Pass two reports, on added lines only:** an ARN whose account field holds a non-allowlisted twelve-digit value, reported as the **whole ARN** rather than the account segment (blanking one field would leave the rest of the ARN in place, and a surviving copy of the account ID elsewhere in the file would rebuild it); every occurrence of an account ID pass one proved, in any file type; and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters, which needs no evidence. - An ARN with no account field is not reported — `arn:aws:s3:::my-internal-bucket`, `arn:aws:iam::aws:policy/...`. There is no reliable way to tell a real bucket from an example one, since anybody may own `arn:aws:s3:::example-bucket`, so reporting them would be noise; redacting a real one is the author's job and catching it is a human reviewer's. Write example ARNs with `123456789012` or `111122223333`, both allowlisted. - Two known gaps. An account ID appearing only as prose in a file that is not a journal is proved by none of the three sources and is not reported. And an ARN is read only where its fields can be lined up, which works for a `*` wildcard and for a CloudFormation `${...}` expression but not for another placeholder syntax in a field before the resource — `{Region}`, ``, `%REGION%` — where the whole ARN fails to parse and a literal account standing beside one goes unreported. Both passes and the lookalikes that must stay silent are pinned by cases in the scanner's `--self-check`, which the check runs on every pull request. diff --git a/.kiro/steering/project-conventions.md b/.kiro/steering/project-conventions.md index 5675cb17..e7a649bc 100644 --- a/.kiro/steering/project-conventions.md +++ b/.kiro/steering/project-conventions.md @@ -202,7 +202,7 @@ Neither piece of tooling is sufficient on its own: A pull request check (`.github/workflows/scan-aws-identifiers.yml`, running `.github/scripts/scan_aws_identifiers.py`) reports unredacted AWS identifiers and applies to every file in the repository, not only to skills. See the "Scanning for AWS Identifiers" section of [CONTRIBUTING.md](../../CONTRIBUTING.md). - An account ID is reported only when something proves it is one; twelve digits on their own are not evidence. Measured across the open pull requests, 54% of the twelve-digit runs on their added lines named no account — a `YYYYMMDDHHMM` datestamp in a resource name, the fractional or integer part of a decimal, a zero-padded counter. The scan therefore runs in two passes. -- **Pass one gathers evidence and reports nothing**, from exactly three sources: the account field of an [ARN](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) (the fifth colon-separated field); an object key in the tool result of a `tool_summary` block, in a `journal_records.json` file, where DevOps Agent keys a per-account API result by account ID; and the value of an `aws_account_id` field, in a `journal_records.json` file. The last two are fields of the DevOps Agent journal schema, which is why they are read only in that file name. Other spellings such as `AccountId` and `accountId` are deliberately not read. Pass one reads every changed file whole, so an ARN in an untouched part of a file still proves an account ID an added line names bare. +- **Pass one gathers evidence and reports nothing**, from exactly three sources: the account field of an [ARN](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) (the fifth colon-separated field); an object key inside the `text` field of a `tool_summary` block's tool result, in a `journal_records.json` file, where `text` holds JSON as a string and DevOps Agent keys a per-account API result by account ID, so the key itself is the account; and the value of an `aws_account_id` field, in a `journal_records.json` file. The last two are fields of the DevOps Agent journal schema, which is why they are read only in that file name. Other spellings such as `AccountId` and `accountId` are deliberately not read. Pass one reads every changed file whole, so an ARN in an untouched part of a file still proves an account ID an added line names bare. - **Pass two reports, on added lines only:** an ARN whose account field holds a non-allowlisted twelve-digit value, reported as the **whole ARN** rather than the account segment (blanking one field would leave the rest of the ARN in place, and a surviving copy of the account ID elsewhere in the file would rebuild it); every occurrence of an account ID pass one proved, in any file type; and an EC2 instance ID as `i-` plus either eight or seventeen hexadecimal characters, which needs no evidence. - An ARN with no account field is not reported — `arn:aws:s3:::my-internal-bucket`, `arn:aws:iam::aws:policy/...`. There is no reliable way to tell a real bucket from an example one, since anybody may own `arn:aws:s3:::example-bucket`, so reporting them would be noise; redacting a real one is the author's job and catching it is a human reviewer's. Write example ARNs with `123456789012` or `111122223333`, both allowlisted. - Two known gaps. An account ID appearing only as prose in a file that is not a journal is proved by none of the three sources and is not reported. And an ARN is read only where its fields can be lined up, which works for a `*` wildcard and for a CloudFormation `${...}` expression but not for another placeholder syntax in a field before the resource — `{Region}`, ``, `%REGION%` — where the whole ARN fails to parse and a literal account standing beside one goes unreported. Both passes and the lookalikes that must stay silent are pinned by cases in the scanner's `--self-check`, which the check runs on every pull request. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 423df74a..a6728905 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -230,7 +230,7 @@ A pull request check ([`.github/workflows/scan-aws-identifiers.yml`](.github/wor | Source | What it looks like | | --- | --- | | The account field of an [ARN](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) | the fifth colon-separated field, as in `arn:aws:iam::123456789012:role/Example`. A `*` in the partition, service or region field is read the same way, so an IAM policy resource such as `arn:aws:logs:*:123456789012:log-group:/aws/lambda/x:*` still proves its account | -| An object key in the tool result of a `tool_summary` block, in a `journal_records.json` file | DevOps Agent records a per-account AWS API result as a map keyed by account ID: `{"123456789012": {"DBInstances": []}}` | +| An object key inside the `text` field of a `tool_summary` block's tool result, in a `journal_records.json` file | `text` holds JSON as a string, and DevOps Agent keys a per-account AWS API result by account ID, so the key itself is the account: `"text": "{\"123456789012\": {\"DBInstances\": []}}"` | | The value of an `aws_account_id` field, in a `journal_records.json` file | `"aws_account_id": "123456789012"`, in a recorded tool input or in agent prose | The two journal sources read fields of the DevOps Agent journal schema, which is why they're read only in a file named `journal_records.json` — the same names elsewhere mean something else. Other spellings an API response or a person might use, `AccountId` and `accountId` among them, are deliberately not read.