OCPBUGS-87829: Scope minted AWS IAM policies to cluster-owned resources#1043
OCPBUGS-87829: Scope minted AWS IAM policies to cluster-owned resources#1043dlom wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/severity-important
|
@dlom: This pull request references Jira Issue OCPBUGS-87829, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughInfrastructure-aware IAM policy generation now scopes supported allow statements with resource-tag conditions, threads the infrastructure name through minted-user flows, and aligns cluster tag validation and tagging with shared tag constants. Tests and verification tooling cover the new action classification. ChangesInfra-tag-aware IAM policy updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/assign @JoelSpeed |
openshift-ci
Bot
added
the
approved
|
@dlom: This pull request references Jira Issue OCPBUGS-87829, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1043 +/- ##
==========================================
+ Coverage 47.06% 47.31% +0.24%
==========================================
Files 97 97
Lines 12560 12608 +48
==========================================
+ Hits 5911 5965 +54
+ Misses 5994 5987 -7
- Partials 655 656 +1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/aws/actuator/actuator.go (1)
236-278: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThread
InfrastructureNameinto this simulation path too.The minted-user policy path now compares policies with
infra.Status.InfrastructureName, but the fallback/current-credential simulation below still buildsccaws.SimulateParamswith onlyRegion. Any principal whose policy depends on the newaws:ResourceTag/kubernetes.io/cluster/<infra>condition can be simulated without that context and look insufficient even when the tagged-resource policy is valid.Proposed fix
- region, err := awsutils.LoadInfrastructureRegion(a.Client, logger) - if err != nil { - return true, err - } + infra, err := utils.GetInfrastructure(a.Client) + if err != nil { + return true, err + } + region, err := awsutils.LoadInfrastructureRegion(a.Client, logger) + if err != nil { + return true, err + } simParams := &ccaws.SimulateParams{ - Region: region, + Region: region, + InfraName: infra.Status.InfrastructureName, }🤖 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 `@pkg/aws/actuator/actuator.go` around lines 236 - 278, The fallback simulation path in the actuator still omits InfrastructureName, so policies using the aws:ResourceTag/kubernetes.io/cluster/<infra> condition can be evaluated incorrectly. Update the ccaws.SimulateParams construction in this branch to carry infra.Status.InfrastructureName as well, mirroring the minted-user path’s use of getDesiredUserPolicy/awsPolicyEqualsDesiredPolicy, and thread that value through any simulation helper that consumes SimulateParams.
🤖 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.
Outside diff comments:
In `@pkg/aws/actuator/actuator.go`:
- Around line 236-278: The fallback simulation path in the actuator still omits
InfrastructureName, so policies using the
aws:ResourceTag/kubernetes.io/cluster/<infra> condition can be evaluated
incorrectly. Update the ccaws.SimulateParams construction in this branch to
carry infra.Status.InfrastructureName as well, mirroring the minted-user path’s
use of getDesiredUserPolicy/awsPolicyEqualsDesiredPolicy, and thread that value
through any simulation helper that consumes SimulateParams.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e41cf280-6879-4c47-8e5b-dd77167b9c21
📒 Files selected for processing (4)
pkg/aws/actuator/actuator.gopkg/aws/actuator/actuator_test.gopkg/aws/utils.gopkg/operator/credentialsrequest/credentialsrequest_controller_test.go
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlom, JoelSpeed 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 |
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/verify-action-maps.sh`:
- Around line 21-31: The no-argument release image resolution path uses curl
without checking that it is installed, so the preflight should fail earlier with
a clear missing-dependency error. Update the command checks at the top of
verify-action-maps.sh to include curl alongside oc, jq, and yq, and keep the
RELEASE_IMAGE fallback flow unchanged so the missing binary is caught before
attempting the nightly lookup.
- Around line 107-113: The extraction currently scans every quoted
service:Action string in utils_test.go, so it can be satisfied by unrelated test
samples instead of only the payload list. Update verify-action-maps.sh to limit
the grep/sort pipeline to the payloadAWSActions section in
pkg/aws/utils_test.go, using the payloadAWSActions symbol as the anchor so only
canonical payload actions are counted.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a32a2ba9-6f07-443a-8aaf-155980e53c11
📒 Files selected for processing (5)
hack/verify-action-maps.shpkg/aws/actuator/actuator.gopkg/aws/actuator/actuator_test.gopkg/aws/utils.gopkg/aws/utils_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/aws/actuator/actuator_test.go
- pkg/aws/utils.go
- pkg/aws/actuator/actuator.go
Add an implicit StringEquals condition to every Allow statement in minted IAM user policies, requiring the standard kubernetes.io/cluster infraName tag (with a value of "owned"). This restricts minted credentials to resources tagged as belonging to the cluster, reducing the blast radius if credentials are compromised (CVE-2026-10843). The CVE advisory suggests tightening Resource ARNs from "*" to specific ARNs, but this isn't feasible: CredentialsRequest manifests are authored before cluster infrastructure exists, so resource ARNs for specific clusters are not known when the CR is authored. Not all AWS actions support the aws:ResourceTag condition. Actions that create new resources fail (because the resource doesn't exist yet). Describe and List actions lack resource-level permissions. Some services don't support it at all. However, within the payload, the list is both finite and rarely changed. As such, we can easily classify the entire list into supported/incompatible categories. Unrecognized actions produce an error state, with directions pointing the developer to update the classification list in the CCO. The condition is not applied to Deny statements (this would actually narrow and weaken the policy), nor is it applied to the auto-generated iam:GetUser self-lookup statement, which is already scoped to just the own user's ARN.
|
/test e2e-upgrade |
|
/override ci/prow/security |
|
@dlom: Overrode contexts on behalf of dlom: ci/prow/security DetailsIn response to this:
Instructions 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. |
|
@dlom: all tests passed! 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. |
|
/payload 4.22 nightly blocking |
|
@dlom: trigger 13 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/49a327b0-71b3-11f1-8eaa-a502797692f3-0 |
3 participants
xref: OCPBUGS-87829
Add an implicit
StringEqualscondition to everyAllowstatement in minted IAM user policies, requiring the standardkubernetes.io/clusterinfraNametag (with a value of"owned"). This restricts minted credentials to resources tagged as belonging to the cluster, reducing the blast radius if credentials are compromised (CVE-2026-10843).The CVE advisory suggests tightening
ResourceARNs from "*" to specific ARNs, but this isn't feasible:CredentialsRequestmanifests are authored before cluster infrastructure exists, so resource ARNs for specific clusters are not known when the CR is authored.the
aws:ResourceTagcondition key is not evaluated by all AWS api actions. For actions that do not support it, AWS silently ignores the condition and the action is allowed unconditionally. These actions are never destructive. This means that the fix only affects destructive actions, the same category of actions explicitely called out by the CVE.The condition is not applied to
Denystatements (this would actually narrow and weaken the policy), nor is it applied to the auto-generatediam:GetUserself-lookup statement, which is already scoped to just the own user's ARN.Summary by CodeRabbit
aws:ResourceTag/kubernetes.io/cluster/<infra>StringEquals-scoped permissions alongside unscoped ones where applicable.Denyrules remain unchanged.