(aws-s3-deployment): BucketDeployment grants itself wider permissions than needed
- 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
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