aws / aws/aws-cdk

(aws-s3-deployment): BucketDeployment grants itself wider permissions than needed

Open
#35,610 1 comment 0 reactions 0 assignees View on GitHub
@aws-cdk/aws-s3-deployment effort/medium feature-request p1
Dominant language
TypeScript
Stars
12.9k
Forks
4.6k
Avg merge
1d 19h
Merged PRs (30d)
71

Description

### Describe the bug

In [`bucket-deployment.ts`](https://github.com/aws/aws-cdk/blob/7379a4bb7d632c207cc67d3b038c9a8d4ed142bc/packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts#L403), the BucketDeployment construct grants its execution role access to read/write the entire bucket. However, if the bucket was configured with a non-empty `destinationKeyPrefix` (as is recommended), it only needs to read or write objects with the given key prefix.

### Regression Issue

- [ ] Select this option if this issue appears to be a regression.

### Last Known Working CDK Library Version

_No response_

### Expected Behavior

The role that is configured by BucketDeployment should be configured with the least authority needed to do its job.

### Current Behavior

The role that is configured by BucketDeployment has access to read/write/delete objects outside of the expected prefix. This permission is slightly broader than it needs.

### Reproduction Steps

For example, if you have this snippet in a CDK stack:

```typescript
const jobAssetsPrefix = 'job-assets/';
this.jobAssetsDeployment = new BucketDeployment(this, 'JobAssets', {
sources: [],
destinationBucket: this.generalAssetsBucket,
destinationKeyPrefix: jobAssetsPrefix,
prune: true,
});
```

you will find that the cfn template includes a policy like this (trimmed for readability):

```json
"CustomCDKBucketDeployment8693BB64968944B69AAFB0CC9EB8756CServiceRoleDefaultPolicy88902FDF": {
"Type": "AWS::IAM::Policy",
"Properties": {
"PolicyDocument": {
"Statement": [
{
"Action": [
"s3:GetObject*",
"s3:GetBucket*",
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
"Resource": [
{"Fn::GetAtt": ["CommonAssetsBucket276E20FC","Arn"] },
{"Fn::Join": ["",[{"Fn::GetAtt": ["CommonAssetsBucket276E20FC","Arn"]}, "/*"]] }
]
},
```

That last resource entry would ideally look like

```json
{"Fn::Join": ["",[{"Fn::GetAtt": ["CommonAssetsBucket276E20FC","Arn"]}, "/job-assets/*"]] }
```

### Possible Solution

Where currently the construct has ([here](https://github.com/aws/aws-cdk/blob/7379a4bb7d632c207cc67d3b038c9a8d4ed142bc/packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts#L403)):

```typescript
this.destinationBucket.grantReadWrite(handler);
if (props.accessControl) {
this.destinationBucket.grantPutAcl(handler);
}
```

it would instead compute an object pattern based on `props.destinationKeyPrefix`, and pass in that pattern as the second argument to `.grantReadWrite()` and `.grantPutAcl()`. A missing or empty prefix would result in a pattern of `'*'`, but a prefix of (for example) `job-assets/` would result in a pattern of `"job-assets/*"`.

I'm not 100% sure what the right behavior of a prefix of `/` or `/some-path/` would be. Some users would expect the leading slash to be stripped, to avoid two consecutive slashes in the resulting policy ARN.

### Additional Information/Context

This isn't a terribly critical excess permission, since it only applies to the deployment lambda and roles that can invoke that lambda are presumably already privileged enough to do bad things in other ways; but when auditing roles in a stack to ensure no over-broad permissions were granted, this sticks out.

### AWS CDK Library version (aws-cdk-lib)

v2.217.0

### AWS CDK CLI version

n/a

### Node.js Version

20

### OS

Amazon Linux 2

### Language

TypeScript

### Language Version

_No response_

### Other information

_No response_

Contributor guide

Open the contributing guide

Research direction

Start in packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts at the grantReadWrite and grantPutAcl calls around line 403. Trace how destinationKeyPrefix is handled, then inspect the generated policy for the empty, job-assets/, /, and /some-path/ cases described in the issue. Done means object resources are limited to the intended prefix while an empty prefix preserves current access.

Written by the indexing model from the issue text.

Assessment

Tech stack
aws, typescript
Domain
cloud, security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.