fix: aws assumeRolePolicyDocument #261
Conversation
WalkthroughThe changes in this pull request modify the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (2)
packages/api/src/router/workspace-integrations.ts (2)
Line range hint
274-282: Consider scoping down the AssumeRole permissions.The current inline policy allows assuming any role (
Resource: "*"). Consider restricting this to only the required roles or using a more specific ARN pattern to follow the principle of least privilege.Example of a more restricted policy:
const policyDocument = { Version: "2012-10-17", Statement: [ { Effect: "Allow", Action: ["sts:AssumeRole"], - Resource: "*", + Resource: [ + "arn:aws:iam::*:role/specific-role-prefix-*", + // Add other specific role patterns as needed + ], }, ], };
Line range hint
386-397: Add error handling for AWS role deletion operations.The AWS API calls for deleting the role policy and role should include error handling to ensure proper cleanup and user feedback.
Consider wrapping the deletion operations in try-catch blocks:
- await iamClient.send( - new DeleteRolePolicyCommand({ - RoleName: roleName, - PolicyName: `${roleName}-ctrlplane-policy`, - }), - ); - - await iamClient.send( - new DeleteRoleCommand({ - RoleName: roleName, - }), - ); + try { + await iamClient.send( + new DeleteRolePolicyCommand({ + RoleName: roleName, + PolicyName: `${roleName}-ctrlplane-policy`, + }), + ); + + await iamClient.send( + new DeleteRoleCommand({ + RoleName: roleName, + }), + ); + } catch (error) { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: `Failed to delete AWS role: ${error.message}`, + }); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/api/src/router/workspace-integrations.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/api/src/router/workspace-integrations.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
🔇 Additional comments (1)
packages/api/src/router/workspace-integrations.ts (1)
223-223: LGTM! Using currentArn directly is more accurate for the trust relationship.
The change to use currentArn directly as the Principal instead of deriving and reconstructing from currentRoleName is correct. This approach:
- Ensures the exact identity is used in the trust relationship
- Prevents potential issues with special characters or formatting in role names
- Maintains the precise scope of the trust relationship
Let's verify the AWS IAM best practices for trust relationships:
✅ Verification successful
Using currentArn directly in the trust relationship is safe and follows AWS IAM best practices
The verification confirms that:
- The change only affects the non-SSO case where
currentArnis used as the Principal - Other AWS role-related code in the codebase properly handles ARNs and trust relationships
- The SSO case continues to use the correct pattern with
aws-reserved/sso.amazonaws.compath
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other AWS role-related code that might need similar updates
# Look for similar AWS role creation patterns
ast-grep --pattern 'AssumeRolePolicyDocument = {
$$$
Principal: {
AWS: $_
}
$$$
}'
# Look for other AWS role-related code
rg -A 5 'AssumeRolePolicyDocument|AssumeRole'
Length of output: 2939
Summary by CodeRabbit
New Features
Bug Fixes