Repository navigation
sec(ci): allowlist the roles a per-secret Secret Manager grant may carry - #1213
Conversation
check-gcp-secret-scope.sh finds Secret Manager roles by their text, but an
authoritative google_{project,folder,organization}_iam_policy can take its
policy_data from file(), a variable with no in-tree default or a module
output, so the role it grants never appears in a scanned file and the guard
reported such configurations clean. Fail closed instead: any non-comment
mention of those types, or of data "google_iam_policy", now exits 2, as the
guard already does for the JSON encoding.
CI's terraform fmt check now covers iac/ as well as terraform/, so every
root the guard scans is held to the layout its block tracking was written
for. The self-test reads SCAN_ROOTS and fails if a root has no fmt step.
Closes LeanerCloud#150
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
check-gcp-secret-scope.sh exempted a per-secret binding's `role =` line for any roles/secretmanager.* name, so widening a per-secret grant to roles/secretmanager.admin (the LeanerCloud#504 review's mutation) still exited 0. Inside a per-secret IAM resource every `role =` line must now name roles/secretmanager.secretAccessor or roles/secretmanager.secretVersionAdder as a literal, matched case-sensitively. Any other role there, including one from another service such as roles/owner or a reference the guard cannot resolve, is a finding unless waived with a reason. Those two are the only roles the repo grants per secret and the only ones the issue names. Inline policy_data role lines on a per-secret _iam_policy get the same check. One built from data "google_iam_policy" already exits 2 after LeanerCloud#150; one fed from file() or a variable remains out of reach and is listed under LIMITATIONS. Closes LeanerCloud#505 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 6 billable files and costs up to $1.50. Or wait 59 minutes for your next included review. View limit details
Comment |
|
Independent review 2 (claude-fleet, cold): APPROVE 5edd456 Scope: this PR is stacked on #1207. As the body asks, I reviewed this PR's own commit, e9e1c62..5edd456 (the guard's
Findings (none blocking):
Against #505: this fixes the root cause.
Attacked:
Verified locally:
Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged. |
|
VERDICT: APPROVE 5edd456 Independent adversarial review of #1213 at 5edd456. The PR is stacked on #1207 (still OPEN, head e9e1c62, an ancestor of this head). This verdict covers the delta e9e1c62..5edd456 (commit 5edd456, six files). #1207's commit is in the diff against main, but it is reviewed under #1207 and this verdict does not cover it. Merging main into this branch after #1207 lands changes the head and restarts the gate. The PR fixes #505 as asked. Inside a per-secret IAM resource, scripts/check-gcp-secret-scope.sh:268-273 now requires every Findings (none blocking):
Attacked:
|
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rA2. 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. |
|
@IAmJSD this branch now conflicts with main: #1207, which it was stacked on, was squash-merged as cc132fb. Please rebase this PR's own commits onto main ( |
LeanerCloud#1207, which this branch was stacked on, merged as squash cc132fb. Its content for scripts/check-gcp-secret-scope.sh and scripts/test-gcp-secret-scope.sh is byte-identical to this branch's copy of LeanerCloud#1207 (e9e1c62), so the conflicts are only this branch's own additions on top: kept this branch's side for both files. The diff against main is now exactly this PR's own change. Self-test 60/0; the guard passes on the tree. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged origin/main (now |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB25. 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. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB26. 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. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on |
Root cause
scripts/check-gcp-secret-scope.shexempts therole = "roles/secretmanager.X"line inside a per-secret IAM resource, and its pattern accepted anyX(roles\/secretmanager\.[A-Za-z]+). So once a grant was per-secret, the guard never looked at which role it carried. The #504 review's mutation ofadmin_password_writerfromsecretVersionAddertoroles/secretmanager.adminexited 0.Fix
Inside a per-secret IAM resource (
google_secret_manager_{,regional_}secret_iam_{member,binding,policy}), everyrole =line must now be one of these two literals, matched case-sensitively since IAM role names are case-sensitive:roles/secretmanager.secretAccessorroles/secretmanager.secretVersionAdderAny other
role =line in such a resource is a finding with its own message. That covers a wider Secret Manager role (admin, secretVersionManager, viewer), a role from another service (roles/owner), a reference (local.x,each.value) and near-miss names (wrong case, extra suffix, trailing space). A waiver with a reason on the line above still excuses a deliberate exception, the same way it does for scope-wide grants. The change is intaint(), where #1207's agent pointed. The header, the failure guidance and the OK line are updated to match.Why these two roles: the issue names them. They are also the only roles the repo grants per secret today: 18
google_secret_manager_secret_iam_memberresources acrossterraform/modules/{secrets/gcp,compute/gcp/gke,compute/gcp/cloud-run,compute/gcp/cleanup-function}, 16secretAccessorand 2secretVersionAdder(the twoadmin_password_writers).iac/has none. They match what the app calls (AccessSecretVersion,AddSecretVersion), as the issue traces.Going beyond the issue's suggestion, on purpose: the issue suggested failing on "any other Secret Manager role". This fails on any other role at all on a per-secret resource, including ones that don't mention
secretmanager. If the check only fired onsecretmanagertext,roles/owneron a single secret, or a role routed through a local holdingroles/owner, would still pass, and both grant at least as much assecretmanager.admin. Mutation probe 4 below shows that narrower version misses 2 of the 15 lookalike lines.Do
_policyresources need the same treatment? (the issue's question)Here is what the code actually does with this PR on top of #1207:
_iam_policyshapepolicy_data = data.google_iam_policy.x.policy_datagoogle_iam_policyis rejected before the scan). Verified locally; no extra handling added here.policy_data = jsonencode({ bindings = [{ role = "..." }] })admin(reported)."role": "roles/secretmanager.X")role =line is exempt (#1207'sper-secret-heredoccase).policy_data = file(...), a variable or a module outputiam-policy-lookalikesfixture deliberately expectspolicy_data = var.secret_policy_datato exit 0. This is now listed under LIMITATIONS. None exist in the tree. Refusing that shape would reverse a #1207 decision, so I left it as a follow-up rather than doing it here.Verification
All evidence is local fixtures and scans (the guard is a static text check, so no cloud calls are involved), on macOS with BWK
/usr/bin/awk. CI runs mawk, so I also ran the suite in anubuntu:24.04container, where/usr/bin/awkis mawk:bash scripts/test-gcp-secret-scope.shgives 60 passed, 0 failed, andbash scripts/check-gcp-secret-scope.shon the real repo exits 0.Fail before / pass after. I ran
bash scripts/test-gcp-secret-scope.shwith the new tests and fixtures against #1207's guard (git show e9e1c627:scripts/check-gcp-secret-scope.sh), then against this PR's guard.Before (e9e1c62 guard):
Results: 55 passed, 5 failed.Only the
.xsuffix and trailing-space lookalikes were caught before. The waived case fails before because, with nothing to excuse, its waiver is reported as vacuous.After:
Results: 60 passed, 0 failed.All 7 new cases pass:per-secret-allowed-roles.tf.fixture: both allowed roles in_member(one with aconditionblock),_binding, regional_member, regional_bindingand an inline_policy. Exit 0 before and after.per-secret-admin.tf.fixture: the issue's mutation. Exit 0 before, exit 1 after.per-secret-role-lookalikes.tf.fixture: 15 blocks (admin, secretVersionManager, viewer,secretaccessor,SecretAccessor,secretAccessorAdmin,secretVersionAdderX,secretAccessor.x,"secretAccessor ",roles/owner,local.writer_role, plus admin or secretVersionManager on_binding, regional_member, regional_bindingand inline_policy). A newrun_role_lines_casehelper asserts that the reported line numbers are exactly the file'srole =lines, so one caught block can't hide a missed one.per-secret-role-waived.tf.fixture: a waivedroles/secretmanager.viewerexits 0, and the same file with the waiver stripped exits 1.terraform/modules/compute/gcp/gke/main.tfpasses as written, and a copy with itssecretVersionAdderwidened toroles/secretmanager.adminexits 1. This reproduces the fix(gcp/gke): grant workload SA access to the admin password secret #504 review's mutation on the real file.Admin fixture by hand:
bash scripts/check-gcp-secret-scope.sh scripts/testdata/gcp-secret-scope/per-secret-admin.tf.fixtureprintsOK ...and exits 0 on e9e1c62. With this PR it reportsper-secret-admin.tf.fixture:9: per-secret IAM role is not roles/secretmanager.secretAccessor or roles/secretmanager.secretVersionAdderand exits 1.The real repo still passes:
bash scripts/check-gcp-secret-scope.shgivesOK: ... in 227 Terraform file(s).(exit 0).Mutation probes on the new guard. Each mutated copy was run on the allowed and lookalike fixtures:
"…$anchor (prefix match): lookalike findings 11/15, so the test fails.secretAccessor: the allowed fixture exits 1, so the test fails.secretmanager: 13/15 (missesroles/ownerandlocal.writer_role), so the test fails.Other checks:
shellcheck0.11.0 on both scripts: clean.git initrepo with an isolated HOME, usingscripts/setup-git-secrets.shand.gitallowed: clean.bash scripts/test-git-secrets-allowlist.sh:42 PASS, 0 FAIL.Out of scope / notes
secretAccessorgranted "where only a version-adder is needed", can't be caught by a static allowlist, since both roles are legitimate. That needs a per-binding intent the guard doesn't have._iam_policywithpolicy_datafromfile(), a variable or a module output (see the table above). Possible follow-up, if wanted.Frontend follow-up for the UI owner
None.
A maintainer, please mirror the issue's triage labels on this PR:
impact/internal,triaged,priority/p3,severity/low,urgency/eventually,effort/xs,type/bug.Closes #505
🤖 Generated with Claude Code