Background
In PR #1 (#1), a project-level roles/aiplatform.user grant was added for the ambient-code/pull-reviews repository via Workload Identity Federation. A code review comment flagged that scoping the IAM binding at the project level is broader than necessary.
Issue
The google_project_iam_member resource for pull_reviews_vertex_ai_access grants roles/aiplatform.user at the project level. Any workflow in ambient-code/pull-reviews (any branch, any actor) can obtain this role, which may be broader than required.
Suggested Improvement
Consider one or both of the following:
- Scope the
roles/aiplatform.user grant to the minimum necessary resource (e.g., a specific Vertex AI resource) rather than at the project level.
- Add a stricter IAM condition on the binding using mapped WIF attributes (e.g.,
attribute.ref, attribute.job_workflow_ref) to restrict which workflows/refs can assume the role.
Security vs. Usability Trade-off
Note that tightening the attribute_condition (e.g., adding assertion.ref == 'refs/heads/main') would improve security but comes at a cost: it would block WIF authentication for workflows running on work-in-progress feature branches, making it harder to test GitHub Actions prior to merging to main. Any future hardening effort should weigh this trade-off and consider whether a separate, more permissive pool/provider can be used for development/staging, or whether branch-scoped testing is an acceptable restriction.
References
Background
In PR
#1(#1), a project-levelroles/aiplatform.usergrant was added for theambient-code/pull-reviewsrepository via Workload Identity Federation. A code review comment flagged that scoping the IAM binding at the project level is broader than necessary.Issue
The
google_project_iam_memberresource forpull_reviews_vertex_ai_accessgrantsroles/aiplatform.userat the project level. Any workflow inambient-code/pull-reviews(any branch, any actor) can obtain this role, which may be broader than required.Suggested Improvement
Consider one or both of the following:
roles/aiplatform.usergrant to the minimum necessary resource (e.g., a specific Vertex AI resource) rather than at the project level.attribute.ref,attribute.job_workflow_ref) to restrict which workflows/refs can assume the role.Security vs. Usability Trade-off
Note that tightening the
attribute_condition(e.g., addingassertion.ref == 'refs/heads/main') would improve security but comes at a cost: it would block WIF authentication for workflows running on work-in-progress feature branches, making it harder to test GitHub Actions prior to merging tomain. Any future hardening effort should weigh this trade-off and consider whether a separate, more permissive pool/provider can be used for development/staging, or whether branch-scoped testing is an acceptable restriction.References
#1: feat: add pull-reviews to wif #1@ktdreyer