fix:Add a new rule for Tencent Cloud CAM #99
Conversation
Reviewer's GuideAdds a new OPA rule package for Tencent Cloud CAM users that flags active access keys with administrator-level permissions, along with associated rule metadata/fixture files. Flow diagram for Tencent Cloud CAM admin access key risk ruleflowchart TD
A[input AccessKeys and AttachedUserPolicyDetail] --> B{AccessKeys is not null}
B -->|no| Z[Set risk to false]
B -->|yes| C[Iterate over each access key p in AccessKeys]
C --> D{p.Status == Active}
D -->|no| C
D -->|yes| E{AttachedUserPolicyDetail is not null}
E -->|no| C
E -->|yes| F[Iterate over each policy p2 in AttachedUserPolicyDetail]
%% Branch 1: Direct AdministratorAccess policy name
F --> G{p2.AttachedUserPolicy.PolicyName == AdministratorAccess}
G -->|yes| H[Add p to root_ak]
G -->|no| I[Check policy document]
%% Branch 2: Policy document with wildcard admin
I --> J[json.unmarshal p2.PolicyDocument into parsed_policy]
J --> K{parsed_policy.statement not null}
K -->|no| C
K -->|yes| L[Iterate over each statement in parsed_policy.statement]
L --> M{statement.action == *}
M -->|no| L
M -->|yes| N{statement.effect == allow}
N -->|no| L
N -->|yes| O{no statement.condition}
O -->|no| L
O -->|yes| P[Add p to root_ak]
H --> Q{more access keys?}
P --> Q
Q -->|yes| C
Q -->|no| R{count root_ak > 0}
R -->|yes| S[Set risk to true]
R -->|no| Z[Set risk to false]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @wzx-ob, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adds a new security rule for Tencent Cloud CAM users. The rule, implemented using OPA, aims to detect and flag CAM users who have active Access Keys associated with highly privileged policies, thereby enhancing cloud security posture by identifying potential over-permissioning risks. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The two
root_akrules share a lot of duplicated logic (filters oninput.AccessKeysandAttachedUserPolicyDetail); consider factoring the common predicates into a helper rule or comprehension to make the policy easier to maintain. - The
AccessKeyId := p.AccessKeyIdassignment is never used in eitherroot_akrule; if it’s not needed for downstream processing, remove it to avoid confusion. - The package name and directory (
TENCENT_CLOUD_CAM User_...) include a space and a numeric suffix; if there is a project convention for stable, space-free identifiers, aligning with it will make this rule easier to reference and reuse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The two `root_ak` rules share a lot of duplicated logic (filters on `input.AccessKeys` and `AttachedUserPolicyDetail`); consider factoring the common predicates into a helper rule or comprehension to make the policy easier to maintain.
- The `AccessKeyId := p.AccessKeyId` assignment is never used in either `root_ak` rule; if it’s not needed for downstream processing, remove it to avoid confusion.
- The package name and directory (`TENCENT_CLOUD_CAM User_...`) include a space and a numeric suffix; if there is a project convention for stable, space-free identifiers, aligning with it will make this rule easier to reference and reuse.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new OPA rule to identify Tencent Cloud CAM users with active access keys and administrator-level permissions. The overall goal is clear and valuable for security auditing. However, the implementation of the Rego policy can be significantly improved for better readability, maintainability, and efficiency. My review includes a detailed comment with a suggested refactoring of the policy to align with Rego best practices, making it cleaner and more performant.
| risk if { | ||
| count(root_ak) > 0 | ||
| } | ||
| root_ak contains p if { | ||
| input.AccessKeys != null | ||
| some p in input.AccessKeys | ||
| AccessKeyId :=p.AccessKeyId | ||
| p.Status == "Active" | ||
| input.AttachedUserPolicyDetail != null | ||
| some p2 in input.AttachedUserPolicyDetail | ||
| p2.AttachedUserPolicy.PolicyName == "AdministratorAccess" | ||
| } | ||
| root_ak contains p if { | ||
| input.AccessKeys != null | ||
| some p in input.AccessKeys | ||
| AccessKeyId :=p.AccessKeyId | ||
| p.Status == "Active" | ||
| input.AttachedUserPolicyDetail != null | ||
| some p2 in input.AttachedUserPolicyDetail | ||
| json.unmarshal(p2.PolicyDocument, parsed_policy) | ||
| parsed_policy.statement != null | ||
| some statement in parsed_policy.statement | ||
| statement.action == "*" | ||
| statement.effect == "allow" | ||
| not statement.condition | ||
| } |
There was a problem hiding this comment.
The current implementation of the policy can be significantly improved in terms of clarity, efficiency, and maintainability.
- Code Duplication: The logic to find an active access key is repeated in both
root_akrules. - Inefficiency: The policy builds a set (
root_ak) of all active access keys, only to check if this set is non-empty. It's more efficient to use boolean helper rules to directly determine if a risk exists, avoiding the creation of a potentially large set. - Unused Variables: The
AccessKeyIdvariable is assigned on lines 13 and 22 but never used, adding noise to the code. - Readability: The logic is complex and nested. Breaking it down into smaller, single-purpose helper rules would make the policy much easier to understand and maintain.
I suggest refactoring the policy to use helper rules, which is a common and recommended practice in Rego. This will make the code cleaner, more efficient, and more readable.
risk if {
has_active_ak
has_admin_policy
}
has_active_ak {
input.AccessKeys != null
some p in input.AccessKeys
p.Status == "Active"
}
has_admin_policy {
input.AttachedUserPolicyDetail != null
some p2 in input.AttachedUserPolicyDetail
is_administrator_policy(p2)
}
is_administrator_policy(policy) {
policy.AttachedUserPolicy.PolicyName == "AdministratorAccess"
}
is_administrator_policy(policy) {
json.unmarshal(policy.PolicyDocument, parsed_policy)
parsed_policy.statement != null
some statement in parsed_policy.statement
statement.action == "*"
statement.effect == "allow"
not statement.condition
}
3 participants
Thank you for your contribution to CloudRec!
What About:
java)go)opa)Description:
Add a new rule for Tencent Cloud CAM
Summary by Sourcery
New Features: