Fix issue DPTP-4756 Add CF template#5263
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Warning Review limit reached
More reviews will be available in 15 minutes and 42 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThree new CloudFormation templates establish a three-tier STS role-chaining model for CI: a home role (OIDC trust in build cluster accounts), a hub role (trusted by home roles, assumes target roles), and a target role ( ChangesAWS STS Role Chaining CloudFormation Templates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
openshift-ci
Bot
added
the
approved
![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
🧹 Nitpick comments (2)
hack/aws-sts-roles/home-role.cf (1)
34-50: 💤 Low valueConsider using native YAML syntax for consistency with other templates.
The hub-role.cf and target-role.cf templates use native YAML for
AssumeRolePolicyDocument, but this template embeds a JSON string via!Sub |. Functionally equivalent, but YAML syntax would be more maintainable and consistent across the PR.♻️ Suggested YAML syntax
RoleName: ci-step-runner - AssumeRolePolicyDocument: !Sub | - { - "Version": "2012-10-17", - "Statement": [{ - "Effect": "Allow", - "Principal": { "Federated": "${OIDCProviderArn}" }, - "Action": "sts:AssumeRoleWithWebIdentity", - "Condition": { - "StringLike": { - "${OIDCProviderURL}:sub": "system:serviceaccount:ci-op-*:*" - }, - "StringEquals": { - "${OIDCProviderURL}:aud": "sts.amazonaws.com" - } - } - }] - } + AssumeRolePolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Principal: + Federated: !Ref OIDCProviderArn + Action: sts:AssumeRoleWithWebIdentity + Condition: + StringLike: + !Sub "${OIDCProviderURL}:sub": "system:serviceaccount:ci-op-*:*" + StringEquals: + !Sub "${OIDCProviderURL}:aud": "sts.amazonaws.com"🤖 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/home-role.cf` around lines 34 - 50, The AssumeRolePolicyDocument in home-role.cf uses embedded JSON string syntax with !Sub |, while other templates (hub-role.cf and target-role.cf) use native YAML syntax. Convert the AssumeRolePolicyDocument from the embedded JSON string format to native YAML structure by removing the !Sub | and the quoted JSON, and instead represent the Version, Statement, Effect, Principal, Action, and Condition as direct YAML nested objects and mappings. This will maintain the same functionality while improving consistency and maintainability across all three role templates.hack/aws-sts-roles/target-role.cf (1)
76-83: ⚖️ Poor tradeoffSecurity note:
iam:*andsts:*are very broad permissions.The
iam:*action allows modifying any IAM entity in the target account, including the ci-step-runner role itself. Combined withsts:*, this permits lateral movement to any role in the account.The description states this mirrors
origin-ci-robot-provision, so this is likely intentional parity. If you want to tighten this in the future, consider:
- Scoping
iam:*to specific resource patterns (e.g., roles/policies for cluster provisioning)- Restricting
sts:AssumeRoleto known installer roles🤖 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 76 - 83, The Sid IAMFull and STSAssumeRole statements in the policy have overly broad permissions that could enable lateral movement. To improve security, replace the wildcard actions "iam:*" and "sts:*" with more specific permissions. For the IAMFull statement, restrict the Action to only necessary IAM operations for cluster provisioning (such as iam:CreateRole, iam:PutRolePolicy, iam:GetRole, etc.) and scope the Resource to specific resource patterns like arn:aws:iam::account:role/cluster-* or similar relevant patterns. For the STSAssumeRole statement, change the Action from "sts:*" to specifically "sts:AssumeRole" and restrict the Resource to only known installer role ARNs instead of using "*". Document why these specific permissions are necessary for your provisioning workflow.
🤖 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 7-9: The documentation comment describing the assume chain in the
target-role.cf file is incomplete and omits the home role hop. Update the assume
chain comment that currently describes the flow from per-test SA token through
ci-step-runner in the hub account to also include the intermediate home role
step from the build cluster account. The corrected chain should include all
three hops: per-test SA token (OIDC) to ci-step-runner (home-role.cf, build
cluster account) to ci-step-runner (hub-role.cf, hub account) to ci-step-runner
(this, target account). Ensure the documentation in the comments reflects the
complete chain as defined in both home-role.cf and hub-role.cf files.
---
Nitpick comments:
In `@hack/aws-sts-roles/home-role.cf`:
- Around line 34-50: The AssumeRolePolicyDocument in home-role.cf uses embedded
JSON string syntax with !Sub |, while other templates (hub-role.cf and
target-role.cf) use native YAML syntax. Convert the AssumeRolePolicyDocument
from the embedded JSON string format to native YAML structure by removing the
!Sub | and the quoted JSON, and instead represent the Version, Statement,
Effect, Principal, Action, and Condition as direct YAML nested objects and
mappings. This will maintain the same functionality while improving consistency
and maintainability across all three role templates.
In `@hack/aws-sts-roles/target-role.cf`:
- Around line 76-83: The Sid IAMFull and STSAssumeRole statements in the policy
have overly broad permissions that could enable lateral movement. To improve
security, replace the wildcard actions "iam:*" and "sts:*" with more specific
permissions. For the IAMFull statement, restrict the Action to only necessary
IAM operations for cluster provisioning (such as iam:CreateRole,
iam:PutRolePolicy, iam:GetRole, etc.) and scope the Resource to specific
resource patterns like arn:aws:iam::account:role/cluster-* or similar relevant
patterns. For the STSAssumeRole statement, change the Action from "sts:*" to
specifically "sts:AssumeRole" and restrict the Resource to only known installer
role ARNs instead of using "*". Document why these specific permissions are
necessary for your provisioning workflow.
🪄 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: 846fd2a8-149e-4239-965e-568f25e50a07
📒 Files selected for processing (3)
hack/aws-sts-roles/home-role.cfhack/aws-sts-roles/hub-role.cfhack/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)
| Trusts the ci-step-runner hub role. The assume chain is: | ||
| per-test SA token (OIDC) -> ci-step-runner (hub-role.cf, hub account) | ||
| -> ci-step-runner (this, target account) |
There was a problem hiding this comment.
Documentation error: missing home role hop in the assume chain.
The chain description omits the home role step. Per home-role.cf and hub-role.cf, the full chain is:
per-test SA token (OIDC) -> ci-step-runner (home-role.cf, build cluster acct)
-> ci-step-runner (hub-role.cf, hub account)
-> ci-step-runner (this, target account)
📝 Suggested fix
Trusts the ci-step-runner hub role. The assume chain is:
- per-test SA token (OIDC) -> ci-step-runner (hub-role.cf, hub account)
+ per-test SA token (OIDC) -> ci-step-runner (home-role.cf, build cluster acct)
+ -> ci-step-runner (hub-role.cf, hub account)
-> ci-step-runner (this, target account)🤖 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 7 - 9, The documentation
comment describing the assume chain in the target-role.cf file is incomplete and
omits the home role hop. Update the assume chain comment that currently
describes the flow from per-test SA token through ci-step-runner in the hub
account to also include the intermediate home role step from the build cluster
account. The corrected chain should include all three hops: per-test SA token
(OIDC) to ci-step-runner (home-role.cf, build cluster account) to ci-step-runner
(hub-role.cf, hub account) to ci-step-runner (this, target account). Ensure the
documentation in the comments reflects the complete chain as defined in both
home-role.cf and hub-role.cf files.
|
/lgtm |
bear-redhat
force-pushed
the
issue/DPTP-4756
branch
from
38d8b96 to
d3fca01
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bear-redhat, hector-vido 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 |
|
/override ci/prow/integration |
|
/override ci/prow/integration |
|
@bear-redhat: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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. |
|
Scheduling tests matching the |
|
@bear-redhat: Overrode contexts on behalf of bear-redhat: ci/prow/integration 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. |
|
@bear-redhat: 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. |
2 participants
AWS STS Role Chaining Infrastructure
This PR adds three AWS CloudFormation templates to establish a secure, cross-account role chaining mechanism for CI test execution using OIDC-based authentication instead of static AWS credentials.
The Role Chain Architecture
The templates implement a three-hop assume-role chain that enables per-test pods to authenticate to AWS without embedding credentials in the cluster:
Home Role (
home-role.cf): Deployed once in each build cluster's AWS account, this role trusts the cluster's OIDC provider. It allows per-test ServiceAccount tokens to be exchanged for temporary credentials viasts:AssumeRoleWithWebIdentity, restricted to test namespaces matching the patternci-op-*with the audiencests.amazonaws.com.Hub Role (
hub-role.cf): Deployed centrally in a shared hub account, this role trusts all home roles from build clusters and permits assuming target roles. The hub role serves as a central trust anchor that allows the architecture to scale to multiple target accounts without requiring each to maintain a list of all build cluster home roles.Target Role (
target-role.cf): Deployed in each AWS account that tests need to access (e.g., aws-5 account), this role trusts only the hub role and provides comprehensive AWS permissions matching the legacyorigin-ci-robot-provisionuser. It grants full access to CloudFormation, EC2, S3, IAM, Route53, ECR, EFS, Lambda, Secrets Manager, Elastic Load Balancing, and related services—all required for CI infrastructure provisioning and testing.Authentication Flow
When a CI test pod requires AWS access:
sts.amazonaws.comsts:AssumeRoleWithWebIdentityagainst the home role using the tokenKey Benefits