- 
                Notifications
    
You must be signed in to change notification settings  - Fork 4.3k
 
          feat(s3): add grantReplicationPermission for IAM Role permissions
          #34138
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 18 commits
1c53bb0
              32bb56c
              ce97233
              86fe648
              901cbbc
              4f0ae47
              6e1d9dc
              2e65d3d
              231ff71
              57721e6
              a3c715a
              40b6e2b
              9aa8929
              2837964
              b970f0a
              ad18d1d
              d2c434e
              3fedd55
              bdbfd76
              419d893
              a23abfa
              4c7c9d7
              83aa959
              30990b4
              a0df50e
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -265,6 +265,18 @@ export interface IBucket extends IResource { | |
| */ | ||
| grantReadWrite(identity: iam.IGrantable, objectsKeyPattern?: any): iam.Grant; | ||
| 
     | 
||
| /** | ||
| * Allows permissions for replication operation to bucket replication role. | ||
| * | ||
| * If an encryption key is used, permission to use the key for | ||
| * encrypt/decrypt will also be granted. | ||
| * | ||
| * @param identity The principal | ||
| * @param props The properties of the replication source and destination buckets. | ||
| * @returns The `iam.Grant` object, which represents the grant of permissions. | ||
| */ | ||
| grantReplicationPermission(identity: iam.IGrantable, props: GrantReplicationPermissionProps): iam.Grant; | ||
| 
     | 
||
| /** | ||
| * Allows unrestricted access to objects from this bucket. | ||
| * | ||
| 
          
            
          
           | 
    @@ -496,6 +508,45 @@ export interface BucketAttributes { | |
| readonly notificationsHandlerRole?: iam.IRole; | ||
| } | ||
| 
     | 
||
| /** | ||
| * The properties for the destination bucket for granting replication permission. | ||
| */ | ||
| export interface GrantReplicationPermissionDestinationProps { | ||
| /** | ||
| * The destination bucket | ||
| */ | ||
| readonly bucket: IBucket; | ||
| 
     | 
||
| /** | ||
| * The KMS key to use for encryption if a destination bucket needs to be encrypted with a customer-managed KMS key. | ||
| * | ||
| * @default - no KMS key is used for replication. | ||
| */ | ||
| readonly encryptionKey?: kms.IKey; | ||
| } | ||
| 
     | 
||
| /** | ||
| * The properties for the destination bucket for granting replication permission. | ||
| */ | ||
| export interface GrantReplicationPermissionProps { | ||
| /** | ||
| * The KMS key used to decrypt objects in the source bucket for replication. | ||
| * **Required if** the source bucket is encrypted with a customer-managed KMS key. | ||
| * | ||
| * @default - it's assumed the source bucket is not encrypted with a customer-managed KMS key. | ||
| */ | ||
| readonly sourceDecryptionKey?: kms.IKey; | ||
| 
     | 
||
| /** | ||
| * The destination buckets for replication. | ||
| * Specify the KMS key to use for encryption if a destination bucket needs to be encrypted with a customer-managed KMS key. | ||
| * Required one or more destination buckets. | ||
| * | ||
| * @default - empty array | ||
| */ | ||
| readonly destinations: GrantReplicationPermissionDestinationProps[]; | ||
| } | ||
| 
     | 
||
| /** | ||
| * Represents an S3 Bucket. | ||
| * | ||
| 
          
            
          
           | 
    @@ -845,6 +896,60 @@ export abstract class BucketBase extends Resource implements IBucket { | |
| this.arnForObjects(objectsKeyPattern)); | ||
| } | ||
| 
     | 
||
| /** | ||
| * Grant replication permission to a principal. | ||
| * This method allows the principal to perform replication operations on this bucket. | ||
| * | ||
| * Note that when calling this function for source or destination buckets that support KMS encryption, | ||
| * you need to specify the KMS key for encryption and the KMS key for decryption, respectively. | ||
| * | ||
| * @param identity The principal to grant replication permission to. | ||
| * @param props The properties of the replication source and destination buckets. | ||
| */ | ||
| public grantReplicationPermission(identity: iam.IGrantable, props: GrantReplicationPermissionProps): iam.Grant { | ||
| if (props.destinations.length === 0) { | ||
| throw new ValidationError('destinations must be specified', this); | ||
                
       | 
||
| } | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please add unit test for throwing this error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @badmintoncryer Thank you for your review, yes I have added a unit test that you noted.  | 
||
| 
     | 
||
| // add permissions to the role | ||
| // @see https://docs.aws.amazon.com/AmazonS3/latest/userguide/setting-repl-config-perm-overview.html | ||
| let result = this.grant(identity, ['s3:GetReplicationConfiguration', 's3:ListBucket'], [], Lazy.string({ produce: () => this.bucketArn })); | ||
| 
     | 
||
| const g1 = this.grant( | ||
| identity, | ||
| ['s3:GetObjectVersionForReplication', 's3:GetObjectVersionAcl', 's3:GetObjectVersionTagging'], | ||
| [], | ||
| Lazy.string({ produce: () => this.arnForObjects('*') }), | ||
| ); | ||
| result = result.combine(g1); | ||
| 
     | 
||
| const destinationBuckets = props.destinations.map(destination => destination.bucket); | ||
| if (destinationBuckets.length > 0) { | ||
| const g2 = iam.Grant.addToPrincipalOrResource({ | ||
| grantee: identity, | ||
| actions: ['s3:ReplicateObject', 's3:ReplicateDelete', 's3:ReplicateTags', 's3:ObjectOwnerOverrideToBucketOwner'], | ||
| resourceArns: destinationBuckets.map(bucket => Lazy.string({ produce: () => bucket.arnForObjects('*') })), | ||
| resource: this, | ||
| }); | ||
| result = result.combine(g2); | ||
| } | ||
| 
     | 
||
| props.destinations.forEach(destination => { | ||
| const g = destination.encryptionKey?.grantEncrypt(identity); | ||
| if (g !== undefined) { | ||
| result = result.combine(g); | ||
| } | ||
| }); | ||
| 
     | 
||
| // If KMS key encryption is enabled on the source bucket, configure the decrypt permissions. | ||
| const g3 = this.encryptionKey?.grantDecrypt(identity); | ||
| if (g3 !== undefined) { | ||
| result = result.combine(g3); | ||
| } | ||
| 
     | 
||
| return result; | ||
| } | ||
| 
     | 
||
| /** | ||
| * Allows unrestricted access to objects from this bucket. | ||
| * | ||
| 
          
            
          
           | 
    @@ -2811,42 +2916,20 @@ export class Bucket extends BucketBase { | |
| } | ||
| }); | ||
| 
     | 
||
| const destinationBuckets = props.replicationRules.map(rule => rule.destination); | ||
| const kmsKeys = props.replicationRules.map(rule => rule.kmsKey).filter(kmsKey => kmsKey !== undefined) as kms.IKey[]; | ||
| 
     | 
||
| let replicationRole: iam.IRole; | ||
| if (!props.replicationRole) { | ||
| replicationRole = new iam.Role(this, 'ReplicationRole', { | ||
| assumedBy: new iam.ServicePrincipal('s3.amazonaws.com'), | ||
| roleName: FeatureFlags.of(this).isEnabled(cxapi.SET_UNIQUE_REPLICATION_ROLE_NAME) ? PhysicalName.GENERATE_IF_NEEDED : 'CDKReplicationRole', | ||
| }); | ||
| 
     | 
||
| // add permissions to the role | ||
| // @see https://docs.aws.amazon.com/AmazonS3/latest/userguide/setting-repl-config-perm-overview.html | ||
| replicationRole.addToPrincipalPolicy(new iam.PolicyStatement({ | ||
| actions: ['s3:GetReplicationConfiguration', 's3:ListBucket'], | ||
| resources: [Lazy.string({ produce: () => this.bucketArn })], | ||
| effect: iam.Effect.ALLOW, | ||
| })); | ||
| replicationRole.addToPrincipalPolicy(new iam.PolicyStatement({ | ||
| actions: ['s3:GetObjectVersionForReplication', 's3:GetObjectVersionAcl', 's3:GetObjectVersionTagging'], | ||
| resources: [Lazy.string({ produce: () => this.arnForObjects('*') })], | ||
| effect: iam.Effect.ALLOW, | ||
| })); | ||
| if (destinationBuckets.length > 0) { | ||
| replicationRole.addToPrincipalPolicy(new iam.PolicyStatement({ | ||
| actions: ['s3:ReplicateObject', 's3:ReplicateDelete', 's3:ReplicateTags', 's3:ObjectOwnerOverrideToBucketOwner'], | ||
| resources: destinationBuckets.map(bucket => bucket.arnForObjects('*')), | ||
| effect: iam.Effect.ALLOW, | ||
| })); | ||
| } | ||
| 
     | 
||
| kmsKeys.forEach(kmsKey => { | ||
| kmsKey.grantEncrypt(replicationRole); | ||
| this.grantReplicationPermission(replicationRole, { | ||
| sourceDecryptionKey: props.encryptionKey, | ||
| destinations: props.replicationRules.map(rule => ({ | ||
| encryptionKey: rule.kmsKey, | ||
| bucket: rule.destination, | ||
| })), | ||
| }); | ||
| 
     | 
||
| // If KMS key encryption is enabled on the source bucket, configure the decrypt permissions. | ||
| this.encryptionKey?.grantDecrypt(replicationRole); | ||
| } else { | ||
| replicationRole = props.replicationRole; | ||
| } | ||
| 
          
            
          
           | 
    ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSDoc for destinations property says default tag - empty array, but the implementation throws an error if the array is empty. I would suggest removing the default tag since there is no default value, and clarify that at least one destination is required
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ozelalisen Thanks. The points you suggested are absolutely right. How about adopting the following policy?
I aimed to clearly convey the behavior of this property by distinguishing between the default value itself and the conditions under which that default is valid or invalid.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing the issue, that looks good to me!