SREP-1332: Add permissions to support adding day2 tag to SecurityGroup #2720
SREP-1332: Add permissions to support adding day2 tag to SecurityGroup #2720ratnam915 wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ratnam915: This pull request references SREP-1332 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
|
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 (1)
WalkthroughTwo IAM policy documents are updated to add a new statement ChangesIAM Policy Tagging Permission
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ratnam915 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@resources/sts/4.14/fedramp-hypershift/openshift_hcp_control_plane_operator_credentials_policy.json`:
- Around line 187-201: Update the policy statement with Sid
"CreateDeleteSecurityGroupTags" to tighten conditions: add aws:TagKeys and Null
constraints so deletions must explicitly target tag keys and cannot remove the
ownership tag. Concretely, in the statement that currently allows
"ec2:CreateTags" and "ec2:DeleteTags" on
"arn:aws-us-gov:ec2:*:*:security-group/*", add Condition clauses (e.g., a
"Null": {"aws:TagKeys":"false"} and a TagKeys equality constraint such as
"ForAllValues:StringEquals": {"aws:TagKeys":["red-hat-managed"]}) so DeleteTags
calls must specify tag keys and cannot delete the "red-hat-managed" ownership
tag.
In
`@resources/sts/hypershift/openshift_hcp_control_plane_operator_credentials_policy.json`:
- Around line 187-201: The policy statement with Sid
"CreateDeleteSecurityGroupTags" is too permissive for ec2:DeleteTags; split it
into two statements: one allowing ec2:CreateTags (keep resource
"arn:aws:ec2:*:*:security-group/*" and any existing constraints) and a separate
one allowing ec2:DeleteTags but add conditions to (1) require explicit tag keys
in every delete request (e.g., "Null": {"aws:TagKeys":"false"} or equivalent)
and (2) forbid deletion of the reserved key by adding a
ForAllValues:StringNotEquals on "aws:TagKeys": ["red-hat-managed"] so requests
cannot remove the red-hat-managed ownership tag; update or remove the original
"CreateDeleteSecurityGroupTags" combined statement accordingly.
🪄 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: Pro Plus
Run ID: ad6452a7-9007-4743-9de6-71f3da0d992c
📒 Files selected for processing (2)
resources/sts/4.14/fedramp-hypershift/openshift_hcp_control_plane_operator_credentials_policy.jsonresources/sts/hypershift/openshift_hcp_control_plane_operator_credentials_policy.json
| { | ||
| "Sid": "CreateDeleteSecurityGroupTags", | ||
| "Effect": "Allow", | ||
| "Action": [ | ||
| "ec2:CreateTags", | ||
| "ec2:DeleteTags" | ||
| ], | ||
| "Resource": [ | ||
| "arn:aws:ec2:*:*:security-group/*" | ||
| ], | ||
| "Condition": { | ||
| "StringEquals": { | ||
| "aws:ResourceTag/red-hat-managed": "true" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n resources/sts/hypershift/openshift_hcp_control_plane_operator_credentials_policy.json | head -250Repository: openshift/managed-cluster-config
Length of output: 7319
🏁 Script executed:
grep -n "red-hat-managed" resources/sts/hypershift/openshift_hcp_control_plane_operator_credentials_policy.jsonRepository: openshift/managed-cluster-config
Length of output: 532
🌐 Web query:
AWS EC2 DeleteTags IAM condition key aws:TagKeys behavior
💡 Result:
The IAM condition key aws:TagKeys is supported for the AWS EC2 DeleteTags action. It allows policies to restrict which tag keys can be deleted based on the tag keys specified in the DeleteTags request. aws:TagKeys is a global request context key that compares the tag keys provided in the request (the keys of tags being deleted) with the keys specified in the policy condition. It is listed as available for EC2 actions in the official service authorization reference and explicitly applicable to ec2:DeleteTags. Common usage includes: - ForAllValues:StringEquals: Allows deletion only if all requested tag keys exactly match the allowed list (e.g., only delete "temporary" tags). Example from similar service (Auto Scaling, analogous to EC2): { "Effect": "Allow", "Action": "autoscaling:DeleteTags", "Resource": "*", "Condition": { "ForAllValues:StringEquals": { "aws:TagKeys": ["temporary"] } } } This pattern applies to EC2 DeleteTags. - Without ForAllValues, it checks if the request keys are a subset of allowed keys. If no tags are specified in DeleteTags (deletes all user tags), aws:TagKeys condition may not apply or evaluates differently, but typically used when specific keys are targeted. Official docs confirm aws:TagKeys for EC2 tagging actions including DeleteTags to enforce allowed keys, preventing deletion of unauthorized tags.
Citations:
- 1: https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazonec2.html
- 2: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_condition-keys.html
- 3: https://docs.aws.amazon.com/IAM/latest/UserGuide/access_tags.html
- 4: https://docs.aws.amazon.com/en_gb/AWSEC2/latest/UserGuide/supported-iam-actions-tagging.html
- 5: https://docs.aws.amazon.com/autoscaling/ec2/userguide/tag-permissions.html
🌐 Web query:
AWS EC2 DeleteTags action aws:ResourceTag condition resource tag protection
💡 Result:
To protect EC2 resource tags from deletion using the DeleteTags action, create an IAM deny policy with a condition on aws:TagKeys. This denies the ec2:DeleteTags action when the request attempts to delete tags with specified protected keys. Example policy: { "Version": "2012-10-17", "Statement": [ { "Sid": "DenyDeleteProtectedTags", "Effect": "Deny", "Action": "ec2:DeleteTags", "Resource": "*", "Condition": { "ForAnyValue:StringEquals": { "aws:TagKeys": ["Name", "Purpose"] } } } ] } This policy denies deletion if any of the specified tag keys (e.g., "Name", "Purpose") are included in the DeleteTags request, regardless of the resource's existing tags. The aws:ResourceTag condition controls access based on existing resource tags but does not directly protect against deletion of those tags. The aws:TagKeys condition matches the tag keys being targeted for deletion in the request. The ec2:DeleteTags action supports tag-related condition keys including aws:TagKeys, aws:RequestTag, and aws:ResourceTag (service-specific variants like ec2:ResourceTag/${TagKey}). No dedicated "tag protection" feature exists beyond IAM conditions; AWS Config can monitor and remediate post-deletion but not prevent it.
Citations:
- 1: https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazonec2.html
- 2: https://stackoverflow.com/questions/62187008/is-there-a-way-i-can-protect-certain-tag-keys-from-deletion-in-aws
- 3: https://docs.aws.amazon.com/IAM/latest/UserGuide/access_tags.html
- 4: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html
- 5: https://docs.aws.amazon.com/autoscaling/ec2/userguide/tag-permissions.html
Prevent deletion of the red-hat-managed ownership tag.
This statement allows deleting any tag on a managed security group, including red-hat-managed. If that tag is removed, later statements guarded by aws:ResourceTag/red-hat-managed=true (DeleteSecurityGroup, SecurityGroupIngressEgress, ManageVPCEndpointWithCondition) no longer match the resource, orphaning it from future management. Additionally, ec2:DeleteTags without tag key constraints can delete all tags when the request omits specific keys. Restrict the action to prevent deletion of the reserved key and require explicit tag keys in every request.
Proposed IAM condition fix
"Condition": {
"StringEquals": {
"aws:ResourceTag/red-hat-managed": "true"
+ },
+ "ForAllValues:StringNotEquals": {
+ "aws:TagKeys": [
+ "red-hat-managed"
+ ]
+ },
+ "Null": {
+ "aws:TagKeys": "false"
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@resources/sts/hypershift/openshift_hcp_control_plane_operator_credentials_policy.json`
around lines 187 - 201, The policy statement with Sid
"CreateDeleteSecurityGroupTags" is too permissive for ec2:DeleteTags; split it
into two statements: one allowing ec2:CreateTags (keep resource
"arn:aws:ec2:*:*:security-group/*" and any existing constraints) and a separate
one allowing ec2:DeleteTags but add conditions to (1) require explicit tag keys
in every delete request (e.g., "Null": {"aws:TagKeys":"false"} or equivalent)
and (2) forbid deletion of the reserved key by adding a
ForAllValues:StringNotEquals on "aws:TagKeys": ["red-hat-managed"] so requests
cannot remove the red-hat-managed ownership tag; update or remove the original
"CreateDeleteSecurityGroupTags" combined statement accordingly.
There was a problem hiding this comment.
Could you please double check if this is still a concern? I see the deletetag action is already constrained by a tag. We want all actions to be constrained by a tag whenever possible.
There was a problem hiding this comment.
@feichashao, thanks for the question — the concern is still valid, and it's worth clarifying why.
You're correct that ec2:DeleteTags is constrained by aws:ResourceTag/red-hat-managed=true. However, this condition only controls which resources the action is authorized against — it does not restrict which tag keys can be deleted from those resources.
Here's the key distinction:
aws:ResourceTag— evaluated against the resource's existing tags at the time the request is made (before the action executes). It answers: "Is this resource allowed to be acted on?"aws:TagKeys— evaluated against the tag keys included in the request itself. It answers: "Which tag keys is this request trying to modify?"
aws:ResourceTag applies to existing resources (e.g., modifying or deleting a security group), while aws:RequestTag applies to new resources being created or when tags are being added.
So with the current policy, this scenario is possible:
- A security group has
red-hat-managed=true✅ - A
ec2:DeleteTagscall is made targeting keyred-hat-managed - IAM evaluates: does the resource have
red-hat-managed=true? → Yes → Allow - The
red-hat-managedtag is deleted - The security group is now orphaned — future statements guarded by
aws:ResourceTag/red-hat-managed=true(e.g.,DeleteSecurityGroup,SecurityGroupIngressEgress,ManageVPCEndpointWithCondition) no longer match it
The aws:TagKeys condition key is specifically designed to address this. These condition keys can be applied to resource-creating actions that support tagging, as well as the ec2:CreateTags and ec2:DeleteTags actions.
In practice, the CPO's UpdateResourceTags() is unlikely to intentionally delete the red-hat-managed system tag, so this is more of a defense-in-depth / least-privilege concern. But if the credentials were ever misused or if there's a bug in the tag reconciliation logic, this gap could result in orphaned security groups.
The previously proposed fix remains the recommended approach to guard against this:
"Condition": {
"StringEquals": {
"aws:ResourceTag/red-hat-managed": "true"
+ },
+ "ForAllValues:StringNotEquals": {
+ "aws:TagKeys": [
+ "red-hat-managed"
+ ]
+ },
+ "Null": {
+ "aws:TagKeys": "false"
}
}This ensures: (1) only red-hat-managed resources can be acted on, (2) the red-hat-managed key itself cannot be deleted, and (3) explicit tag keys must always be present in the request (no accidental blanket deletions).
That said, if this is an accepted risk given the operational context of the CPO, it is a valid trade-off — just worth documenting explicitly.
There was a problem hiding this comment.
Hi @ratnam915 Code Rabbit's comment makes sense to me, but if feels making the policy complicated and not look the same as existing policies.
What is your thought? It is possible to test and validate the suggested change?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@feichashao : Hey Siu, the FedRAMP HCP clusters run the same Control Plane Operator with the same UpdateResourceTags() reconciliation logic for the default worker security group. When a customer modifies ResourceTags on their HostedCluster, the CPO needs ec2:CreateTags and ec2:DeleteTags on the SG regardless of whether it's a commercial or GovCloud cluster. The only difference is the ARN partition (arn:aws-us-gov vs arn:aws), which is already reflected in the FedRAMP policy file.
Without this addition, day-2 tag propagation to the default worker SG would fail on FedRAMP HCP clusters with an AccessDenied error.
Agreed that CodeRabbit's concern is valid — I've pushed the fix. Here's the rationale and test results:
You're right that ec2:DeleteTags is already constrained by aws:ResourceTag/red-hat-managed=true, but that condition only controls which resources can be acted on — it doesn't restrict which tag keys can be deleted from those resources. So a DeleteTags call targeting the red-hat-managed key itself would pass the resource tag check (the tag exists → condition met), then delete it, orphaning the SG from all other policy statements that depend on that tag (DeleteSecurityGroup, SecurityGroupIngressEgress, ManageVPCEndpointWithCondition).
The two new conditions are a standard AWS pattern for this:
Null: { "aws:TagKeys": "false" }— tag keys must be explicitly specified in every request (prevents blanket deletion)ForAllValues:StringNotEquals: { "aws:TagKeys": ["red-hat-managed"] }— none of the tag keys in the request may bered-hat-managed
In practice, the CPO's UpdateResourceTags() only creates/deletes tags from the MapsDiff() output based on the HostedCluster's ResourceTags spec. It won't try to delete red-hat-managed since that tag is applied at SG creation time, not tracked in the tag reconciliation loop. So this is defense-in-depth rather than fixing a real CPO bug.
I have attached the below document with the changes that i have validated:
https://docs.google.com/document/d/1xa-_TIq3oJwM86UDK3NtARKGUSybjfobljJucdTT7hg/edit?tab=t.0
ratnam915
force-pushed
the
feature/SREP-1332
branch
from
fbe0a91 to
8e615e8
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Could you help me understand why we need to add this policy to 4.14 fedramp?
|
@ratnam915: 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. |
3 participants
What type of PR is this?
Bug
What this PR does / Why we need it
This PR adds a new CreateDeleteSecurityGroupTags IAM policy statement to the ControlPlaneOperator credentials policy. This statement grants ec2:CreateTags and ec2:DeleteTags permissions on security groups that are tagged with red-hat-managed: true.
HyperShift supports day-2 tag propagation for the Default Worker SecurityGroup ({infraID}-default-sg). When a customer modifies ResourceTags on their HostedCluster, the Control Plane Operator (CPO) reconciles these changes via UpdateResourceTags(). This function calls both ec2:CreateTags for new/modified tags and ec2:DeleteTags for removed tags.
The existing CreateTagsRestrictedActions statement only permits ec2:CreateTags during resource creation (gated by an ec2:CreateAction condition), and therefore does not cover day-2 tag operations on existing security groups. Without ec2:DeleteTags, tag removal fails silently—the CPO catches the permissions error and continues, but stale tags remain on the security group.Policy files updated
File
ARN partition
resources/sts/hypershift/openshift_hcp_control_plane_operator_credentials_policy.json
arn:aws (Commercial)
resources/sts/4.14/fedramp-hypershift/openshift_hcp_control_plane_operator_credentials_policy.json
arn:aws-us-gov (FedRAMP)
Statement added
{
"Sid": "CreateDeleteSecurityGroupTags",
"Effect": "Allow",
"Action": [
"ec2:CreateTags",
"ec2:DeleteTags"
],
"Resource": [
"arn:aws:ec2:::security-group/*"
],
"Condition": {
"StringEquals": {
"aws:ResourceTag/red-hat-managed": "true"
}
}
}
This statement is scoped to security groups with the red-hat-managed: true resource tag, which is consistent with the existing DeleteSecurityGroup and SecurityGroupIngressEgress statements.Which Jira/Github issue(s) does this PR fix?
Resolves SREP-1332
This MCC policy JSON is the source of truth that Red Hat submits to AWS for the managed policy ROSAControlPlaneOperatorPolicy. The change will take effect in production once AWS publishes the next policy version (v7).
A companion hypershift PR adds the same ec2:DeleteTags permission to the inline CPO policy used in dev/test environments. The two PRs are independent and can be merged in any order.
The new statement is separate from CreateTagsRestrictedActions because that statement uses an ec2:CreateAction condition which only applies at resource creation time and does not cover day-2 tag operations on existing resources.
Pre-checks
Ran unit tests locally — hypershift full test suite passes with the updated policy
Validated the changes in a cluster — confirmed the day-2 tag reconciliation code path in CPO (reconcileDefaultSecurityGroup -> UpdateResourceTags) requires both ec2:CreateTags and ec2:DeleteTags
Summary by CodeRabbit