[BB-1481] Add grants to iam_roles #91
Conversation
mateovespConductor
requested review from
btipling and
laurenleach
as code owners
WalkthroughParses URL-encoded IAM AssumeRole trust policies to extract AWS principal ARNs, classifies principal types (IAM Role/User), and implements Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Grants as roleResourceType.Grants
participant IAM as IAM Client
participant Helpers as helpers.go
participant Creator as Principal Resource Creator
participant Result
Caller->>Grants: Grants(ctx, resource, token)
Grants->>Grants: validate resource ID, resolve IAM client (use parent if present)
alt no IAM client
Grants->>Result: return error
else IAM client available
Grants->>IAM: GetRole(roleName)
alt role missing or no AssumeRolePolicyDocument
Grants->>Result: return no grants
else role with policy
Grants->>Helpers: extractTrustPrincipals(policyDocument)
Helpers->>Helpers: URL-decode → JSON-unmarshal → filter Allow + sts:AssumeRole / sts:* / *
Helpers-->>Grants: list of principal ARNs
loop for each principal ARN
Grants->>Helpers: detectPrincipalResource(principalARN)
Helpers-->>Grants: (resourceType, id, ok)
alt ok
Grants->>Creator: create principal resource (type, id)
Creator-->>Grants: principal resource or error
Grants->>Grants: build grant via roleAssignmentEntitlement
else
Grants->>Grants: skip principal (log)
end
end
Grants->>Result: return grants
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/connector/helpers.go (1)
242-274: Consider using thearn.Parseutility for more robust ARN parsing.The principal detection logic is correct, but for consistency with other helper functions in this file (e.g.,
iamUserNameFromARN,iamGroupNameFromARN), consider using thearn.Parseutility from the AWS SDK. This would provide more robust validation and cleaner resource type detection.Example refactor:
func detectPrincipalResource(principalARN string) (*v2.ResourceType, string, bool) { + // Skip wildcards + if principalARN == "*" { + return nil, "", false + } + + // Skip service principals + if strings.Contains(principalARN, ":service/") { + return nil, "", false + } + + parsedARN, err := arn.Parse(principalARN) + if err != nil { + return nil, "", false + } + switch { - // Skip wildcards and service principals — not actual IAM identities - case principalARN == "*", - strings.Contains(principalARN, ":service/"): - return nil, "", false - // IAM User ARN (e.g. arn:aws:iam::123456789012:user/Alice) - case strings.Contains(principalARN, ":user/"): + case strings.HasPrefix(parsedARN.Resource, "user/"): return resourceTypeIAMUser, principalARN, true // IAM Role ARN (e.g. arn:aws:iam::123456789012:role/MyRole) - case strings.Contains(principalARN, ":role/"): + case strings.HasPrefix(parsedARN.Resource, "role/"): return resourceTypeRole, principalARN, true // Account root (e.g. arn:aws:iam::123456789012:root) - case strings.HasSuffix(principalARN, ":root"): - parts := strings.Split(principalARN, ":") - if len(parts) >= 5 { - accountID := parts[4] - return resourceTypeAccountIam, accountID, true - } - return nil, "", false + case parsedARN.Resource == "root": + return resourceTypeAccountIam, parsedARN.AccountID, true // Everything else → unsupported or not relevant default: return nil, "", false } }pkg/connector/role.go (2)
135-139: Consider usingarn.Parsefor more robust role name extraction.The current string splitting approach is correct for IAM role ARNs, but for consistency with other helper functions in this codebase (e.g.,
iamUserNameFromARN), consider using thearn.Parseutility from the AWS SDK.Apply this diff:
- parts := strings.Split(resource.Id.Resource, "/") - if len(parts) < 2 { - return nil, "", nil, fmt.Errorf("invalid role ARN: %s", resource.Id.Resource) + parsedARN, err := arn.Parse(resource.Id.Resource) + if err != nil { + return nil, "", nil, fmt.Errorf("invalid role ARN: %w", err) } - roleName := parts[len(parts)-1] + roleName := path.Base(parsedARN.Resource) + if roleName == "/" || roleName == "." { + return nil, "", nil, fmt.Errorf("invalid role resource in ARN: %s", resource.Id.Resource) + }Note: You'll need to import the
arnpackage if not already imported.
150-152: Consider logging when AssumeRolePolicyDocument is nil.While returning nil grants is correct when there's no trust policy, adding a debug or info log would improve observability, helping operators understand why a role has no grants.
Apply this diff:
if roleResp.Role.AssumeRolePolicyDocument == nil { + l.Debug("role has no AssumeRolePolicyDocument, returning no grants", + zap.String("role_name", roleName), + ) return nil, "", nil, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/connector/helpers.go(2 hunks)pkg/connector/role.go(3 hunks)
🔇 Additional comments (5)
pkg/connector/helpers.go (3)
4-6: LGTM!The added imports appropriately support trust policy decoding and JSON parsing.
140-189: LGTM!The trust policy parsing logic is well-structured with appropriate error handling for URL decoding and JSON parsing. The function correctly filters for "Allow" statements with "sts:AssumeRole" actions.
191-240: LGTM!The helper functions correctly handle both single and list forms of Action and Principal fields, which aligns with AWS IAM policy flexibility.
pkg/connector/role.go (2)
6-6: LGTM!The added imports appropriately support the Grants implementation with string manipulation, grant creation, and structured logging.
Also applies to: 15-15, 17-18
161-184: LGTM!The grant creation logic is well-structured with appropriate error handling. The implementation correctly skips invalid principals while logging errors, which ensures resilient operation without failing the entire grant extraction process.
…ts from Coderabbit
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/connector/role.go (1)
169-181: Consider making trust‑policy parse failures a soft error instead of failing GrantsRight now any error from
extractTrustPrincipalscausesGrantsto return an error for this role. Given thatGetRolefailures and empty roles are treated as “log and skip,” it may be more robust to do the same here: log the parse error (with role name and maybe ARN) and return no grants, rather than failing the sync for this resource because of an unexpected or slightly malformed trust policy.For example:
- principals, err := extractTrustPrincipals( - awsSdk.ToString(roleResp.Role.AssumeRolePolicyDocument), - ) - if err != nil { - return nil, "", nil, fmt.Errorf("failed to extract principals for role %s: %w", roleName, err) - } + principals, err := extractTrustPrincipals( + awsSdk.ToString(roleResp.Role.AssumeRolePolicyDocument), + ) + if err != nil { + l.Error("baton-aws: failed to extract trust principals from AssumeRolePolicyDocument, returning no grants", + zap.String("role_name", roleName), + zap.Error(err), + ) + return nil, "", nil, nil + }Optionally, you could also add a debug/info log in the
!okbranch afterdetectPrincipalResourceso that unsupported principal strings don’t silently disappear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/connector/helpers.go(2 hunks)pkg/connector/role.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/helpers.go
🔇 Additional comments (1)
pkg/connector/role.go (1)
114-199: Grants implementation correctly derives role name, fetches trust policy, and maps principals to grantsThe end-to-end flow (resource validation, IAM client resolution, ARN parsing with
path.Base,GetRole, trust principal extraction, andgrant.NewGrantconstruction) is coherent and matches how List+Get need to be combined for IAM. From a correctness standpoint this looks good and I don’t see blocking issues here.Please ensure existing connector tests (or new ones) cover:
- A role with multiple trusted IAM users/roles.
- A role with no
AssumeRolePolicyDocument.- A role whose
AssumeRolePolicyDocumentincludes principals that should be ignored (e.g., services), to confirm grants are emitted only for intended principals.
mateovespConductor
changed the title
| iamClient := o.iamClient | ||
| if resource.ParentResourceId != nil { | ||
| var err error | ||
| iamClient, err = o.awsClientFactory.GetIAMClient(ctx, resource.ParentResourceId.Resource) |
There was a problem hiding this comment.
Just a comment or request for any changes and not any issue. I think this is fine, I don't think these make any API calls to create the clients and they are cached. I don't think this type of cache would be a problem on a lambda, but not 100% sure
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/connector/helpers.go(2 hunks)pkg/connector/role.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (9)
pkg/connector/helpers.go (3)
140-189: LGTM! Well-structured trust policy parsing.The function correctly handles URL decoding, JSON parsing, and statement filtering. The logic for extracting AWS principals from statements with Effect "Allow" and action "sts:AssumeRole" is sound.
191-208: LGTM! Clean type switch implementation.The function correctly handles both single string and slice formats for IAM policy actions.
210-240: LGTM! Handles both string and array principal formats.The function correctly extracts AWS principals from the Principal field, handling both single ARN strings and arrays of ARNs.
pkg/connector/role.go (6)
107-112: LGTM! Proper expansion of grantable resource types.The entitlement now correctly supports grants to IAM users, roles, groups, and SSO users, which aligns with AWS IAM trust policy capabilities.
124-139: LGTM! Proper input validation and client resolution.The function correctly validates the resource and resolves the IAM client based on parent resource context, following the same pattern as the List method.
141-149: LGTM! Correct ARN parsing and role name extraction.Using
path.Basecorrectly extracts the role name from the ARN resource field, even when the role has a path prefix.
154-160: Verify that silently ignoring GetRole errors is intentional.When
GetRolefails, the function logs the error but returnsnil, "", nil, nil(success with no grants) rather than propagating the error. This means:
- Permission errors or transient failures won't fail the sync
- Operators may not notice that grants are missing for this role
- The connector continues processing other resources
Is this error-swallowing behavior intentional to ensure partial sync success, or should GetRole failures propagate as errors?
Similar error handling patterns also appear at:
- Lines 162-165 (nil response handling)
- Lines 167-172 (missing AssumeRolePolicyDocument)
174-179: LGTM! Correct trust policy principal extraction.The function properly extracts principals from the trust policy and propagates errors.
181-216: LGTM! Well-structured grant creation with proper error handling.The loop correctly:
- Filters principals using
detectPrincipalResource- Creates principal resources with appropriate error logging
- Adds
GrantExpandableannotations for role principals to support nested role assumptions- Constructs grants for each valid principal
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/connector/helpers.go (1)
242-248: Simplify the duplicate function comment.The function comment contains redundant text across lines 242-247, making it harder to read. The purpose is stated twice in slightly different ways.
Apply this diff to consolidate the comment:
-// detectPrincipalResource analyzes a principal ARN and determines: -// which Baton resource type it corresponds to (IAM user, IAM role, or account root) -// the resource identifier to use in the Grant -// It returns ok=false when the principal should be ignored. -// detectPrincipalResource determines what type of IAM principal an ARN belongs to. -// Supports IAM users and IAM roles. +// detectPrincipalResource analyzes a principal ARN and determines which Baton resource type +// it corresponds to (IAM user or IAM role) and the resource identifier to use in the Grant. +// Returns ok=false when the principal should be ignored (e.g., account root, service principals). func detectPrincipalResource(principalARN string) (*v2.ResourceType, string, bool) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/helpers.go(2 hunks)
🔇 Additional comments (3)
pkg/connector/helpers.go (3)
4-6: LGTM!The new imports are appropriate for parsing URL-encoded JSON trust policies.
140-189: LGTM!The trust policy parsing logic correctly URL-decodes, filters for Allow+AssumeRole statements, and gracefully handles malformed data. The defensive approach of silently skipping invalid statements is appropriate for resilient parsing of AWS policy documents.
210-240: LGTM!The principal extraction logic correctly handles both single string and list formats for AWS principals, with appropriate nil returns for invalid cases.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/connector/helpers.go (1)
247-253: Comment still mentions account root even though it’s ignored.The function only returns matches for IAM users (
user/) and IAM roles (role/), and returnsok=falsefor account root and other principals, but the second comment line still says “(IAM user, IAM role, or account root)”. This conflicts with the implementation and the later “Supports IAM users and IAM roles” line.Recommend updating the block comment to remove the “account root” mention and clarify what’s ignored, e.g.:
-// detectPrincipalResource analyzes a principal ARN and determines: -// which Baton resource type it corresponds to (IAM user, IAM role, or account root) -// the resource identifier to use in the Grant -// It returns ok=false when the principal should be ignored. -// detectPrincipalResource determines what type of IAM principal an ARN belongs to. -// Supports IAM users and IAM roles. +// detectPrincipalResource analyzes a principal ARN and determines: +// which Baton resource type it corresponds to (IAM user or IAM role) +// the resource identifier to use in the Grant. +// It returns ok=false when the principal should be ignored (e.g., account root, service principals). +// detectPrincipalResource determines what type of IAM principal an ARN belongs to. +// Supports IAM users and IAM roles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/helpers.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
pkg/connector/helpers.go (2)
191-213: Wildcardsts:AssumeRoledetection logic looks good.The updated
containsAssumeRolecorrectly handles both scalar and listActionfields and treats"sts:AssumeRole","sts:*", and"*"as granting AssumeRole. This should cover the common wildcard policy patterns without introducing false positives in typical IAM trust policies.
215-245: AWS principal extraction is safe and focused.
extractAWSPrincipalscleanly restricts itself toPrincipal["AWS"], supports both single-string and list forms, and safely ignores non-map or non-string cases. That matches the expected trust policy shapes and avoids accidentally treating non-AWS principals (e.g.,Service) as grantees.
Add defense in case a statement is created as a single object
Description
Useful links:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.