Skip to content

fix(scripts): catch a multi-line bare "*" Action in the IAM parity check - #1215

Merged
cristim merged 4 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-1136
Oct 25, 2026
Merged

cristim merged 4 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-1136

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

Base

This was stacked on #1174, which has since merged into main (squashed as cc521fa0, with the same content as 0d633e6b). After merging origin/main, the "Files changed" tab shows only this fix: scripts/check-aws-iam-parity.sh and scripts/test-aws-iam-parity.sh. The "On 0d633e6b" column below is the fail-before baseline, which is the same scripts as on main before this PR.

Root cause

scripts/check-aws-iam-parity.sh detected a bare "*" Action with a single-line grep -qiE "\"?actions?\"?[[:space:]]*[:=][[:space:]]*\[?[[:space:]]*[\"']\*[\"']". That caused two problems:

  • Missed multi-line widenings. It only matched when "*" was on the key's own line and was the first list item. A JSON "Action": [\n "*"\n], an HCL actions = [\n "*",\n] / Action = [\n ..., "*"\n], a YAML Action:\n - "*" and a flow list like Action: [logs:CreateLogGroup, "*"] all passed parity.
  • False reports on NotAction. The key match had no left boundary and ignored case, so it also matched NotAction: "*" and not_actions = ["*"]. It reported a "*" that matches no action (and grants nothing) as an action wildcard.

Fix

The script has no structured parser for these files: they mix CFN YAML with !Sub/!If tags, HCL with jsonencode(...), and JSON heredocs inside .tf and .sh.tmpl, and it uses no yq, jq or terraform. So I replaced the grep with a small POSIX awk scan, bare_star_action:

  • It finds each Action/Actions key, matched case-insensitively with tolower. The key must start the line (optionally after a YAML - ) or follow { or ,. That keeps out NotAction, not_actions, var.action == "*" and prose.
  • It follows the key's value across lines: a scalar, a flow/JSON/HCL list tracked by bracket depth (until the matching ]), or a YAML block list (- items at or below the key's column, ending at the first line that is not one).
  • It reports only a quoted lone "*" or '*'. "s3:*" and "logs:Describe*" do not count. Service wildcards in the compared namespaces ("ec2:*") are still caught by the existing token regex. Brackets, # and // inside strings are ignored, and trailing # / // comments are skipped.
  • After a list closes, the rest of the line is scanned again for more keys, as in single-line multi-statement JSON. A line that ends a YAML block list is scanned again as a normal line. CRLF line endings are tolerated.
  • POSIX awk only (no IGNORECASE, no [[:space:]]). Tested under BSD awk (macOS) and mawk 1.3.4 (the awk ubuntu-latest CI runs).

Scanning every tracked .tf/.yaml/.yml/.tmpl/.json file outside frontend/ gives a bare * only for three Azure role-parity fixtures under scripts/testdata/role-parity/. Each has a real "actions": [\n "*"\n]. No AWS IaC file in the repo reports one, and the real-repo parity check still passes.

Tests (scripts/test-aws-iam-parity.sh, cases 17-28)

Each case changes one runtime copy (or the WIF CLI template) only, so parity fails exactly when the extractor reports * for it. The YAML snippets go into the CFN stack's Lambda policy as real statements, before - Sid: CostExplorer. A moved anchor counts as a failure. The HCL snippets are appended as complete resource/data blocks.

# Form File Expected On 0d633e6b
17 YAML block - "*" CFN stack 1 0 (missed)
18 YAML compact block, - '*' at the key's indent CFN stack 1 0 (missed)
19 YAML flow list, "*" not first CFN stack 1 0 (missed)
20 HCL actions = [\n "*",\n] (policy document) TF lambda 1 0 (missed)
21 HCL jsonencode Action = [\n "logs:...",\n "*",\n] TF fargate 1 0 (missed)
22 JSON heredoc "Action": [\n "*"\n] TF fargate 1 0 (missed)
23 JSON "Action":\n [\n ..., "*"\n ] (bracket on next line) WIF CLI template 1 0 (missed)
24 NotAction: "*" (incl. as the first key right after an Action block) and NotAction:\n - "*" CFN stack 0 1 (spurious)
25 not_actions = ["*"], multi-line not_actions, jsonencode NotAction = "*", JSON "NotAction": [\n "*"\n] TF lambda + fargate 0 1 (spurious)
26 Lookalikes: - "s3:*", - 'logs:Describe*', then Resource:\n - "*", Condition - "*" CFN stack 0 0 (guard)
27 Lookalikes: jsonencode "s3:*" list, then Resource = [\n "*",\n], Condition list "*"; JSON "s3:*" then "Resource": [\n "*"\n] TF lambda + fargate 0 0 (guard)
28 - Action: "*" opening the statement right after an Action block list CFN stack 1 1 (guard)

Cases 26-28 pass on both commits by design. They guard against over-matching and against the YAML block swallowing the next line, which the old single-line grep could not do.

I checked the fixtures parse as intended. terraform fmt -check - accepts all the HCL fixture files. PyYAML with a CFN-tag loader reads the inserted statements as, e.g., {'Action': ['logs:CreateLogGroup', '*']} and {'NotAction': '*'}. jq reads the case-23 policy as "Action":["ec2:CreateTags","*"].

Verification

This is fixture-based script evidence. No cloud APIs are called, and nothing is planned or applied.

bash scripts/check-aws-iam-parity.sh      # OK: ... in parity  (real repo, exit 0)
bash scripts/test-aws-iam-parity.sh       # Results: 28 passed, 0 failed.

# fail-before: new test script against the 0d633e6b check script and sources
#   (git archive 0d633e6b ... + git show 0d633e6b:scripts/check-aws-iam-parity.sh)
#   -> Results: 19 passed, 9 failed.   (cases 17-25, as in the table)

# mawk (CI's awk), in a clean container:
docker run --rm -v "$PWD":/repo:ro ubuntu:24.04 bash -c \
  'cp -r /repo /tmp/r && cd /tmp/r && bash scripts/check-aws-iam-parity.sh && bash scripts/test-aws-iam-parity.sh'
#   -> mawk 1.3.4 20240123; OK: ... in parity; Results: 28 passed, 0 failed.  (no extra packages needed)

shellcheck scripts/check-aws-iam-parity.sh scripts/test-aws-iam-parity.sh   # clean (0.11.0)
PATH=<git-secrets>:$PATH bash scripts/test-git-secrets-allowlist.sh         # 42 PASS, 0 FAIL (incl. full tracked-file scan)
git diff --check                                                             # clean

Extra checks run locally, not committed:

  • Unit probes. 37 snippet probes of bare_star_action gave the same results under BSD awk and mawk. They cover the scalar, list and block forms, NotAction, var.action == "*", principal = "*", !If [..., ["*"], ...], trailing comments and CRLF.
  • Mutation probes. Each of these makes at least one case fail:
    • dropping the key's left boundary
    • letting a YAML block never end
    • ignoring the block indent
    • swallowing the line that ends a block
    • ignoring ]
    • counting any string that ends in *
    • dropping next-line [ handling
    • dropping single-quote strings

No Go code changed, so go build/go vet/golangci-lint/gocyclo don't apply. scripts/test-aws-iam-parity.sh is 496 lines and scripts/check-aws-iam-parity.sh is 292.

Out of scope (noted, not changed)

  • NotAction lists are still read by the token regex. NotAction: [ec2:DescribeX] is still extracted as if ec2:DescribeX were granted, so the per-namespace parity treats deny-lists and grants alike. This was true before this change. Only the bare-* path now skips NotAction.
  • Exotic YAML is not followed. That covers long-form intrinsics under Action: (Action:\n Fn::If:\n - ...), anchors/aliases and multi-line quoted scalars. None appear in the compared files.
  • Parity only compares the files with each other. A bare * added to all three runtime copies at once still passes. That is a separate allowlist question, like section 5 from fix(iac/aws): drop dead organizations:Describe{Account,Organization} grants from runtime trio #1174.

Closes #1136

Maintainers: please mirror the issue's triage labels onto this PR: impact/internal, priority/p3, severity/low, urgency/this-quarter, effort/xs, type/bug.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • IAM parity checks now detect bare "*" actions in single-line and multiline policies across YAML, HCL, and JSON formats. This helps flag wildcard actions consistently while avoiding false positives for NotAction, resource and condition values, and service-specific wildcards.

IAmJSD and others added 2 commits October 20, 2026 01:59
…grants from runtime trio

The CUDly runtime role (CloudFormation stack, Terraform Lambda and Fargate
modules) granted organizations:DescribeAccount and
organizations:DescribeOrganization, but neither the platform nor the pinned
cloud-commitments-go provider ever calls them: the only Organizations API
the runtime uses is ListAccounts (internal/accounts/org_discovery.go and
providers/aws/provider.go). Remove both from all three copies.

Parity alone cannot stop the grants coming back, since adding them to all
three copies keeps the copies equal. check-aws-iam-parity.sh now also fails
when a runtime copy grants any organizations action other than
ListAccounts, and test-aws-iam-parity.sh covers that pre-fix shape.

Closes LeanerCloud#69

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
check-aws-iam-parity.sh found a bare "*" Action only when it sat on the
same line as the key, so a widening written as a JSON array, an HCL
actions = [ list or a YAML "- \"*\"" item passed. Its case-insensitive
"?actions?"? key match also hit NotAction / not_actions, reporting a
"*" that grants nothing as an action wildcard.

Replace the grep with a POSIX awk scan that follows each Action/Actions
value across lines (scalar, flow/JSON/HCL list, YAML block list) and
counts only a quoted lone "*". The key has to start the line or follow
"{" or ",", which leaves NotAction, not_actions, Resource, Condition
values and prose out.

Add fixture cases for each multi-line form, for NotAction and for
lookalikes ("s3:*", Resource and Condition "*" lists).

Closes LeanerCloud#1136

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Walkthrough

The IAM parity checker now detects quoted bare "*" actions in scalar and list forms, including values that span lines. New fixtures cover multiple policy formats and verify that NotAction, resource values, condition values, and lookalikes do not trigger action-wildcard failures.

Changes: IAM parity wildcard detection

Layer / File(s) Summary
Parse bare wildcard actions
scripts/check-aws-iam-parity.sh
A new awk parser scans action keys and scalar or list values across lines. The action extractor uses it and retains existing namespace-token extraction.
Validate wildcard detection
scripts/test-aws-iam-parity.sh
Fixtures cover bare wildcard actions, NotAction, service wildcards, and wildcard values in resource and condition fields across YAML, HCL, JSON, and jsonencode forms.

Priority: Not assessed

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: cristim

Merge Risk: Minimal · up to 0678e

The parity-check change is ready to merge after normal checks; no material issue remains established.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@IAmJSD IAmJSD added impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/this-quarter Within the quarter effort/xs Trivial / one-liner type/bug Defect labels Oct 20, 2026
Resolve the conflict in scripts/test-aws-iam-parity.sh. LeanerCloud#1174 was
squash-merged as cc521fa, and its content is identical to 0d633e6, which
this branch already contains. Main's side of the conflicting hunk added
nothing beyond case 16, so the resolution keeps this branch's cases 17-28
unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator Author

Merged origin/main (a14dc8f2) to resolve the conflict in scripts/test-aws-iam-parity.sh. New head: 48762599.

There were no review findings on the previous head (5446de2f) to address.

Conflict resolution

Verification on 48762599 (all offline: fixture/static checks only, no cloud APIs)

  • bash scripts/check-aws-iam-parity.sh: OK, in parity (exit 0).
  • bash scripts/test-aws-iam-parity.sh: 28 passed, 0 failed, under both gawk 5.2.1 and mawk 1.3.4 (CI's awk, ubuntu:24.04 container, --network none).
  • shellcheck 0.11.0 on both scripts: clean. git diff --check origin/main: clean.
  • git-secrets in a throwaway repo with the repo's setup-git-secrets.sh patterns: the changed scripts plus .gitallowed scan clean, and a planted fake key is flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
  • Sanity checks on the merged tree: go build ./..., go vet ./... and go vet -tags integration ./... are clean. No Go, Terraform or workflow files differ from main, so lint, gocyclo, unit/integration tests and terraform checks don't apply to this diff.

🤖 Generated with Claude Code

@cristim

cristim commented Oct 25, 2026

Copy link
Copy Markdown
Member

Independent review 2 (claude-fleet, cold): APPROVE 4876259

Scope: the range has three commits:

The net diff touches only scripts/check-aws-iam-parity.sh (+83/-6) and scripts/test-aws-iam-parity.sh (+294). There are no Go, migration, frontend or terraform changes.

Findings (none blocking):

  • [non-blocking, test gap] scripts/check-aws-iam-parity.sh:133 (MODE == "list" ... keys(REST)) - Deleting the key re-scan after a multi-line list closes still passes all 28 cases (M3). With that branch dropped, ]}, {"Action": "*"} on the closing line of an earlier Action array would be missed.
  • [non-blocking, test gap] check-aws-iam-parity.sh:107 (the ,/} at depth 0 terminator in scan) - Removing it still passes all 28 cases (M8). Under that mutation, a flow mapping such as {Action: logs:Get}, {Action: "*"} would be missed.
  • [non-blocking, not a regression] bare_star_action misses two forms:
    • a single-quoted YAML key ('Action': '*');
    • JSON with escaped quotes inside a shell string (\"Action\": \"*\").
    • The old grep missed both too, and neither appears in any of the 10 compared files.

Against #1136: this fixes the root cause.

  • The old single-line grep could not follow values across lines, and its unanchored key also matched NotAction and not_actions.
  • The awk scanner fixes both, at the key regex (:125) and in the scan state machine.

Attacked:

  • Edge inputs: 25 run through bare_star_action under BWK awk and mawk (CI runs mawk). All are handled correctly except the two forms above:
    • flow maps and one-line HCL;
    • !If [..., "*"];
    • a comment between the key and its block list, and a comment inside a list;
    • tabs and CRLF;
    • a ] inside a quoted item;
    • a scalar on the next line, and flow continuation lines;
    • a parent-sequence - "*", correctly not counted;
    • NotAction in a flow map, and not_actions;
    • var refs, two statements on one line, and a YAML anchor.
  • Real repo: no bare "*" in any of the 10 files.
    • The callers are the CI steps at ci.yml:1049/1052.
    • runtime_iam_coverage_test.go only names the script in comments.
  • Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
    • M1, the key anchor widened to [a-z_]*actions?: cases 24 and 25 FAIL.
    • M2, >= COL changed to > COL: case 18 FAILS.
    • M3, no keys(REST) after a list closes: 28/28 pass. Gap.
    • M4, FOUND never set: 9 cases FAIL.
    • M5, next after a block list ends: case 28 FAILS.
    • M6, a single quote not treated as a quote: case 18 FAILS.
    • M7, a pending [ on the next line dropped: case 23 FAILS.
    • M8, scan ignores ,/}: 28/28 pass. Gap.
  • Merge: git show --remerge-diff 48762599 shows only conflict-marker removal in test-aws-iam-parity.sh, with no hand edits.

Verified locally:

  • git merge-tree --write-tree is clean against both f5ad8c6 and current origin/main 00a3f40.
  • bash scripts/test-aws-iam-parity.sh on both merged exports: 28 passed, 0 failed, under BWK awk and under mawk.
  • shellcheck on both scripts: rc 0.
  • Fail-before: the head test against the base checker gives 19 passed and 9 failed. Cases 17-25 fail as expected, and 26-28 pass on base too, because they guard against false positives.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim

cristim commented Oct 25, 2026

Copy link
Copy Markdown
Member

Independent review 2 (claude-fleet, cold), re-review: APPROVE 0678e97

This is a delta from my APPROVE at 4876259. The only new commit is a merge of origin/main (503eb2e):

  • git log --first-parent 487625990ff0b93456d19ae86a1b6756024e22c3..0678e97c0b9014d0c8f0bebc644e544295eae031 shows just that merge.
  • git show --remerge-diff is empty, so the merge has no hand edits.
  • The PR's own change is identical to the version I reviewed.
  • git merge-tree --write-tree origin/main 0678e97c0b9014d0c8f0bebc644e544295eae031 is clean.

Verified locally on a git archive export of 0678e97, with GOTOOLCHAIN=go1.26.10 and no cloud credentials, under the heavy lock:

  • bash scripts/test-aws-iam-parity.sh: 28 passed, 0 failed.
  • bash scripts/check-aws-iam-parity.sh on the real tree: rc 0.

My earlier findings and notes still apply unchanged.

Same reviewer as the earlier review, re-reviewing the new head. Review only: nothing was pushed, and the GitHub review state is unchanged.

@cristim

cristim commented Oct 25, 2026

Copy link
Copy Markdown
Member

VERDICT: APPROVE 0678e97

Independent adversarial review of #1215 (closes #1136) at 0678e97, base main. Its parent 0d633e6 has the same content as main's squash of #1174, and the merge of main 503eb2e is clean, so git diff origin/main...0678e97c contains only scripts/check-aws-iam-parity.sh and scripts/test-aws-iam-parity.sh. The change does what #1136 asks: multi-line JSON, HCL and YAML bare "*" Actions are now extracted, NotAction and not_actions no longer are, and there are fixtures for each form plus NotAction negatives.

Findings: no blocking findings.

Attacked:

  • At the head, bash scripts/check-aws-iam-parity.sh passes on the real repo and bash scripts/test-aws-iam-parity.sh gives 28 passed, 0 failed. The results are the same with BSD awk and with mawk 1.3.4 put first on PATH. shellcheck 0.11.0 is clean on both scripts.
  • Fail-before: I ran the new test script against main's check-aws-iam-parity.sh and got 19 passed, 9 failed. The failures are exactly cases 17-25: seven missed multi-line forms and two spurious NotAction reports. That matches the PR's table.
  • 22 ad hoc snippets fed to the extracted bare_star_action, under both awks. Correctly reported: a YAML flow mapping, one-line JSON with two statements, JSON "Action" : [ ..., "*" ] with spaces, an upper-case key, a CRLF block list, a deeper-indented block list, a compact block list followed by another key, !Sub "*", !If [C, ["*"], [...]], and a trailing # ] comment inside a list. Correctly not reported: NotAction after an Action list on the same line, var.action == "*", unquoted s3:*, a "ec2:X]" string inside a list, Resource after a block list, and multi-line not_actions.
  • Regressions against the old grep: every single-line form it caught (key at line start, or after { / ,, with : or =, quoted or bare key, either quote style, bracket optional) is still caught, so the left-boundary rule loses no real file form.
  • Mutation probes on a scratch copy, each reverted, with the test-script result for each:
    1. Loosen the key's left boundary: 2 failed.
    2. Make the YAML block list never end: 2 failed.
    3. Ignore ]: 6 failed.
    4. Count any string ending in *: 2 failed.
    5. Disable YAML block lists: 2 failed.

@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 0678e97c0b9014d0c8f0bebc644e544295eae031

Blocking:

  • none

Non-blocking:

  • scripts/check-aws-iam-parity.sh:127 (keys): a few YAML forms still slip through. All of them fail open, and none appears in the compared files today:
    • A single-quoted key ('Action': '*') is missed, because the key regex allows only ".
    • A short-form intrinsic on the line after the key is missed: Action:\n !If [IsProd, ["*"], [...]]. In MODE == "pending", only a - , [ or quote line is followed. The same !If on the key's own line is caught.
      The body's out-of-scope note covers long-form Fn::If but not this short form. Both would be cheap to add if you want them.
  • scripts/test-aws-iam-parity.sh is 496 lines, which is close to the 500-line guideline.

Attacked:

  • Fail-before: main's check-aws-iam-parity.sh against this PR's test script gives 19 passed, 9 failed, exactly cases 17-25 as the body's table says. With the PR script restored it is 28/28.
  • Ran 18 snippet probes of bare_star_action, extracted from the script, under gawk 5.2.1 and mawk 1.3.4 (ubuntu:24.04, CI's awk). Results were identical under both.
    • Caught: single-line minified JSON, concat(["*"], ...), comments and blank lines inside lists, a compact YAML block, Action: !If [..., "*", ...] on one line, and Action: ["*"] # x followed by NotAction.
    • Correctly ignored: Resource:\n - "*" and NotAction: "*".
    • Missed: the two forms above.
  • Ran the real-repo check-aws-iam-parity.sh (OK) and test-aws-iam-parity.sh (28 passed) on gawk, and both again in ubuntu:24.04 with mawk.
  • Also ran: shellcheck (clean), git diff --check (clean), secrets-scan (clean; planted key flagged), test-git-secrets-allowlist.sh (137 PASS). No Go code changed.

Body-stale:

  • "test-git-secrets-allowlist.sh ... 42 PASS": it is now 137 PASS on this head, because main has grown. Immaterial.

Reviewer: rB2. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

@cristim

cristim commented Oct 25, 2026

Copy link
Copy Markdown
Member

Local verification at 0678e97c0b9014d0c8f0bebc644e544295eae031 merged onto current main (5aa3df5b; merge-tree c07d1b1e; macOS):

  • actionlint (when workflows changed), shellcheck on the changed scripts and every changed scripts/test-*.sh pass on the merged tree.
  • Nothing was applied to any cloud.

@cristim

cristim commented Oct 25, 2026

Copy link
Copy Markdown
Member

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at 0678e97c0b9014d0c8f0bebc644e544295eae031).

@cristim
cristim marked this pull request as ready for review October 25, 2026 20:22
@cristim
cristim merged commit c3ef735 into LeanerCloud:main Oct 25, 2026
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm type/bug Defect urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(scripts): IAM parity check misses a multi-line bare "*" Action

2 participants