[BB-1367] add delete iam user resource (NEW PERMISSIONS NEEDED) #89
Conversation
agustin-conductor
requested review from
btipling and
laurenleach
as code owners
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughUpdates iamUserResourceType.Delete to accept an additional parent IAM context parameter; it derives the username from the ARN (resourceId), obtains an IAM client (optionally from the parentResourceID), checks existence, and performs the same sequential cleanup and deletion flow as before. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant R as iamUserResourceType.Delete
participant P as parentResourceID (optional)
participant I as AWS IAM API
C->>R: Delete(ctx, resourceId, parentResourceID?)
alt parentResourceID provided
R->>P: Resolve IAM client / context
P-->>R: IAM client
else no parentResourceID
R->>R: Use default IAM client
end
R->>R: Parse user name from ARN
R->>I: GetUser(user)
alt User not found (NoSuchEntity)
R-->>C: return success (no-op)
else User exists
rect rgba(200,235,255,0.25)
note right of R: Sequential cleanup (login profile, keys, certs, SSH, service creds, MFA, policies, groups)
R->>I: DeleteLoginProfile(user)
R->>I: ListAccessKeys(user) / DeleteAccessKey(...)
R->>I: ListSigningCertificates(user) / DeleteSigningCertificate(...)
R->>I: ListSSHPublicKeys(user) / DeleteSSHPublicKey(...)
R->>I: ListServiceSpecificCredentials(user) / DeleteServiceSpecificCredential(...)
R->>I: ListMFADevices(user) / DeactivateMFADevice(...)
R->>I: ListUserPolicies(user) / DeleteUserPolicy(...)
R->>I: ListAttachedUserPolicies(user) / DetachUserPolicy(...)
R->>I: ListGroupsForUser(user) / RemoveUserFromGroup(...)
end
R->>I: DeleteUser(user)
R-->>C: return annotations, nil
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Given we're close on caching going into baton-sdk, let's wait to containerize this connector and not put this change into C1.
![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 (2)
pkg/connector/iam_user.go (2)
341-351: Consider pagination for ListAccessKeys operation.The
ListAccessKeysAPI can return paginated results when a user has many access keys. The current implementation only processes the first page of results, potentially leaving access keys undeleted.Apply this pattern to handle pagination:
- keys, err := iamClient.ListAccessKeys(ctx, &iam.ListAccessKeysInput{UserName: awsStringUserName}) - if err != nil { - return nil, fmt.Errorf("aws-connector: failed to list access keys: %w", err) - } - - for _, key := range keys.AccessKeyMetadata { - _, err = iamClient.DeleteAccessKey(ctx, &iam.DeleteAccessKeyInput{UserName: awsStringUserName, AccessKeyId: awsSdk.String(awsSdk.ToString(key.AccessKeyId))}) + var marker *string + for { + keys, err := iamClient.ListAccessKeys(ctx, &iam.ListAccessKeysInput{ + UserName: awsStringUserName, + Marker: marker, + }) if err != nil { - return nil, fmt.Errorf("aws-connector: failed to delete access key: %w", err) + return nil, fmt.Errorf("aws-connector: failed to list access keys: %w", err) } + + for _, key := range keys.AccessKeyMetadata { + _, err = iamClient.DeleteAccessKey(ctx, &iam.DeleteAccessKeyInput{ + UserName: awsStringUserName, + AccessKeyId: awsSdk.String(awsSdk.ToString(key.AccessKeyId)), + }) + if err != nil { + return nil, fmt.Errorf("aws-connector: failed to delete access key: %w", err) + } + } + + if !keys.IsTruncated { + break + } + marker = keys.Marker }Similar pagination handling should be applied to other List operations (signing certificates, SSH keys, service-specific credentials, MFA devices, user policies, attached policies, and groups).
401-413: Virtual MFA devices may not be fully cleaned up.While
DeactivateMFADevicehandles hardware and SMS-based MFA devices, virtual MFA devices also require deletion usingDeleteVirtualMFADevice. The current implementation may leave orphaned virtual MFA device resources.After deactivating MFA devices, add virtual MFA device deletion:
for _, device := range mfaDevices.MFADevices { _, err = iamClient.DeactivateMFADevice(ctx, &iam.DeactivateMFADeviceInput{UserName: awsStringUserName, SerialNumber: awsSdk.String(awsSdk.ToString(device.SerialNumber))}) if err != nil { return nil, fmt.Errorf("aws-connector: failed to deactivate MFA device: %w", err) } + + // Delete virtual MFA devices (serial number format: arn:aws:iam::account-id:mfa/user-name) + if strings.Contains(awsSdk.ToString(device.SerialNumber), ":mfa/") { + _, err = iamClient.DeleteVirtualMFADevice(ctx, &iam.DeleteVirtualMFADeviceInput{ + SerialNumber: awsSdk.String(awsSdk.ToString(device.SerialNumber)), + }) + if err != nil { + return nil, fmt.Errorf("aws-connector: failed to delete virtual MFA device: %w", err) + } + } }Note: This will require adding
iam:DeleteVirtualMFADeviceto the DeletePermissions policy statement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci.yamlis excluded by none and included by none
📒 Files selected for processing (1)
pkg/connector/iam_user.go(1 hunks)
🔇 Additional comments (4)
pkg/connector/iam_user.go (4)
292-292: Method signature correctly updated to support cross-account deletion.The addition of the
parentResourceIDparameter addresses the requirement for cross-account IAM user deletion, allowing the method to resolve the correct account-specific IAM client.
304-310: Cross-account client resolution correctly implemented.This logic properly addresses the previous review concern by resolving the account-specific IAM client when
parentResourceIDis provided, preventing silent no-ops for identities in delegated accounts.
312-324: User existence check correctly implements idempotent delete semantics.The method appropriately treats a missing user as a successful deletion, and uses the resolved
iamClientfor the GetUser call, ensuring cross-account operations work correctly.
457-465: Final user deletion correctly implemented.The final
DeleteUseroperation uses the resolvediamClientand has appropriate error handling.
|
This connector seems to be missing a capabilities yaml and workflow? |
looks like it, there is not baton_capabilities.json in the repo, adding capabilities_and_config.yaml, let me know if there is any reason at all for not adding it. |
| } | ||
|
|
||
| // Delete all access keys | ||
| // Permission needed: iam:ListAccessKeys, iam:DeleteAccessKey |
There was a problem hiding this comment.
Are these necessary permissions mentioned in the Readme file or in doc-info.md? This would also make it clear which permissions are required. The implementation is good and makes sense.
luisina-santos
left a comment
luisina-santos
left a comment
There was a problem hiding this comment.
reminder: DOCS ticket needs to be created for @mindymo to take care of new permissions detailed in order to Delete IAM Users.
Docs ticket -> https://conductorone.atlassian.net/browse/DOCS-397 |
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
I read though https://docs.aws.amazon.com/IAM/latest/UserGuide/id_users_remove.html#id_users_deleting_cli and looked the sdk to see some of the functions used and it all looks correct.
REQUIRES NEW PERMISSIONS
Adds delete for IAM user resource
The following permissions are required:
{ "Version": "2012-10-17", "Statement": [ { "Sid": "ReadPermissions", "Effect": "Allow", "Action": [ "iam:GetUser", "iam:ListAccessKeys", "iam:ListSigningCertificates", "iam:ListSSHPublicKeys", "iam:ListServiceSpecificCredentials", "iam:ListMFADevices", "iam:ListUserPolicies", "iam:ListAttachedUserPolicies", "iam:ListGroupsForUser" ], "Resource": "*" }, { "Sid": "DeletePermissions", "Effect": "Allow", "Action": [ "iam:DeleteLoginProfile", "iam:DeleteAccessKey", "iam:DeleteSigningCertificate", "iam:DeleteSSHPublicKey", "iam:DeleteServiceSpecificCredential", "iam:DeactivateMFADevice", "iam:DeleteUserPolicy", "iam:DetachUserPolicy", "iam:RemoveUserFromGroup", "iam:DeleteUser" ], "Resource": "*" } ] }Summary by CodeRabbit