fix(aws-sts): add EC2 snapshot permissions for minted CI users#5295
fix(aws-sts): add EC2 snapshot permissions for minted CI users#5295deepsm007 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates the ChangesIAM Policy Update
Estimated code review effort: 1 (Trivial) | ~5 minutes Related PRs: None identified. 🚥 Pre-merge checks | ✅ 17✅ Passed checks (17 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Couldn't analyze Error: Server ErrorThe server encountered an error and could not complete your request.Please try again in 30 seconds. Comment |
deepsm007
force-pushed
the
fix/aws-sts-snapshot-cco-user-policy
branch
from
9dea8e7 to
59a82b8
Compare
|
/lgtm |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/aws-sts-roles/target-role.cf`:
- Around line 123-133: The EC2 snapshot permissions in the `CITargetRole` policy
are redundant because `EC2Full` already grants them, so remove this inline
`EC2Snapshots` statement from the role’s policy. If the goal is to constrain
minted CI users, move the snapshot actions into the user-scoped policy that is
applied through `iam:PutUserPolicy` in the CI user provisioning flow instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ee5bdf9b-faae-44d1-9dea-e0b9527be375
📒 Files selected for processing (1)
hack/aws-sts-roles/target-role.cf
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
| - PolicyName: ci-user-permission-boundary | ||
| PolicyDocument: | ||
| Version: "2012-10-17" | ||
| Statement: | ||
| - Sid: EC2Snapshots | ||
| Effect: Allow | ||
| Action: | ||
| - "ec2:CreateSnapshot" | ||
| - "ec2:DescribeSnapshots" | ||
| - "ec2:DeleteSnapshot" | ||
| Resource: "*" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where minted-user policies / PutUserPolicy documents and permission boundaries are defined.
rg -nP -C3 'PutUserPolicy|PermissionsBoundary|permission-boundary|CreateSnapshot' \
-g '!**/vendor/**'Repository: openshift/ci-tools
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## target-role.cf excerpt\n'
nl -ba hack/aws-sts-roles/target-role.cf | sed -n '1,220p'
printf '\n## searches\n'
rg -n 'ci-user-permission-boundary|CCOPutUserPolicy|PermissionsBoundary|PutUserPolicy|permission-boundary|CreateSnapshot|EC2Full|CITargetRole' hack aws-sts-roles . -g '!**/vendor/**' || trueRepository: openshift/ci-tools
Length of output: 223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## target-role.cf around the cited lines\n'
nl -ba hack/aws-sts-roles/target-role.cf | sed -n '40,170p'
printf '\n## all aws-sts role templates and generators\n'
fd -a '(*role*.cf|*sts*|*aws-sts*)' hack aws-sts-roles . || true
printf '\n## symbol searches in relevant tree\n'
rg -n 'ci-user-permission-boundary|CCOPutUserPolicy|PermissionsBoundary|PutUserPolicy|CreateSnapshot|DeleteSnapshot|DescribeSnapshots|EC2Full|CITargetRole|target-role' hack . -g '!**/vendor/**' || trueRepository: openshift/ci-tools
Length of output: 238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## files under hack/aws-sts-roles\n'
fd -a . hack/aws-sts-roles
printf '\n## grep for role/policy generation paths\n'
rg -n 'policy document|permission boundary|permissions boundary|PutUserPolicy|UserPolicy|minted user|CCO|CCOPutUserPolicy|STS role|target role|EC2Snapshots|EC2Full' hack . -g '!**/vendor/**' || true
printf '\n## inspect target-role.cf if present\n'
if [ -f hack/aws-sts-roles/target-role.cf ]; then
nl -ba hack/aws-sts-roles/target-role.cf | sed -n '1,220p'
fiRepository: openshift/ci-tools
Length of output: 6549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## user-min-permissions references\n'
rg -n -C3 'iam:PutUserPolicy|user-min-permissions|permission-boundary|CreateSnapshot|DeleteSnapshot|DescribeSnapshots|EC2Full|ci-step-runner-target|ci-user-permission-boundary' ci-operator/step-registry/ipi/conf/aws/user-min-permissions || true
printf '\n## file excerpt\n'
if [ -f ci-operator/step-registry/ipi/conf/aws/user-min-permissions/ipi-conf-aws-user-min-permissions-commands.sh ]; then
nl -ba ci-operator/step-registry/ipi/conf/aws/user-min-permissions/ipi-conf-aws-user-min-permissions-commands.sh | sed -n '200,260p'
fiRepository: openshift/release
Length of output: 3909
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('hack/aws-sts-roles/target-role.cf')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 40 <= i <= 150:
print(f"{i:4d}: {line}")
PYRepository: openshift/ci-tools
Length of output: 4226
Move these snapshot actions off CITargetRole EC2Full already covers them here, so this inline policy has no effect on the role. If the intent is to scope minted CI users, put the statement in the user policy written via iam:PutUserPolicy instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/aws-sts-roles/target-role.cf` around lines 123 - 133, The EC2 snapshot
permissions in the `CITargetRole` policy are redundant because `EC2Full` already
grants them, so remove this inline `EC2Snapshots` statement from the role’s
policy. If the goal is to constrain minted CI users, move the snapshot actions
into the user-scoped policy that is applied through `iam:PutUserPolicy` in the
CI user provisioning flow instead.
59a82b8 to
7055b37
Compare
|
New changes are detected. LGTM label has been removed. |
openshift-ci
Bot
removed
the
approved
deepsm007
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bear-redhat, deepsm007 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bear-redhat, deepsm007 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bear-redhat, deepsm007 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/aws-sts-roles/target-role.cf`:
- Around line 119-129: The current policy block in target-role.cf is misleading
because ci-user-permission-boundary is only an additive inline policy on the
target role, not a real permissions boundary or user-scoped policy. Update the
naming in the target role definition to match the actual behavior, or if the
intent is to enforce a boundary for minted CI users, add the proper
PermissionsBoundary attachment and/or explicit iam:PutUserPolicy flow in the
role/policy resources; use the ci-user-permission-boundary identifier and the
surrounding target role IAM statements to locate the affected section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: af4c616e-032d-49f9-8011-8451376f241d
📒 Files selected for processing (1)
hack/aws-sts-roles/target-role.cf
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
| - PolicyName: ci-user-permission-boundary | ||
| PolicyDocument: | ||
| Version: "2012-10-17" | ||
| Statement: | ||
| - Sid: EC2Snapshots | ||
| Effect: Allow | ||
| Action: | ||
| - "ec2:CreateSnapshot" | ||
| - "ec2:DescribeSnapshots" | ||
| - "ec2:DeleteSnapshot" | ||
| Resource: "*" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Policy name misrepresents what this does.
ci-user-permission-boundary is a plain additive inline policy on the target role — it is neither an IAM permissions boundary (which attaches via the PermissionsBoundary property) nor scoped to any user. Per the PR objectives this was meant to bound minted CI users and pair with iam:PutUserPolicy, but the diff contains neither a boundary attachment nor a PutUserPolicy grant (note iam:* at Line 78 already covers PutUserPolicy). Rename to reflect actual behavior, or implement the boundary/user-scoping the name implies.
As per coding guidelines: "A good name is long enough to fully communicate what the item is or does."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/aws-sts-roles/target-role.cf` around lines 119 - 129, The current policy
block in target-role.cf is misleading because ci-user-permission-boundary is
only an additive inline policy on the target role, not a real permissions
boundary or user-scoped policy. Update the naming in the target role definition
to match the actual behavior, or if the intent is to enforce a boundary for
minted CI users, add the proper PermissionsBoundary attachment and/or explicit
iam:PutUserPolicy flow in the role/policy resources; use the
ci-user-permission-boundary identifier and the surrounding target role IAM
statements to locate the affected section.
Source: Coding guidelines
deepsm007
force-pushed
the
fix/aws-sts-snapshot-cco-user-policy
branch
from
01c68b3 to
1d85142
Compare
|
@deepsm007: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
ci-user-permission-boundaryinline policy with EC2 snapshot create/describe/delete actions for minted CI usersiam:PutUserPolicyon the target role so CCO can attach policies to minted usershttps://redhat-internal.slack.com/archives/CBN38N3MW/p1783356991822999?thread_ts=1783354331.075049&cid=CBN38N3MW
/cc @openshift/test-platform
This PR updates the AWS STS target role template (
hack/aws-sts-roles/target-role.cf,CITargetRole, used by CCO) by adding a new inline policy (ci-user-permission-boundary) to serve as the permission boundary for minted CI users.Practically, CI user provisioning via CCO can now apply this boundary to allow EC2 snapshot lifecycle operations—
ec2:CreateSnapshot,ec2:DescribeSnapshots, andec2:DeleteSnapshoton all snapshot resources—supporting CI workflows that require snapshot creation, inspection, and cleanup.