Skip to content

fix: added support for generating S3 permissions using Bucket references #648

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danrivett
Copy link
Contributor

Resolves #647

Note: I'll add unit tests if the proposed solution in this PR is correct. This solution works for me, but I wasn't sure if there was a better way to fix this issue.

`arn:aws:s3:::${bucket}/*`,
],
resource: resolveS3BucketReferences(bucket, [
`arn:aws:s3:::\${bucket}`,
Copy link
Contributor Author

@danrivett danrivett Apr 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to keep the strings as template literals and just escape the ${bucket} so it wasn't interpolated here, but in the resolveS3BucketReferences() function.

This is because other affected strings below uses other variables such as ${prefix} and ${key} and so they needed to remain template literals to have those interpolated correctly.

So I decided for consistency I would keep them all as template literals and uniformly escape the ${bucket} references.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After review, I changed my mind and decided to use unescaped single quoted strings with a comment as I felt it was easier for the reader to understand why bucket wasn't being interpolated directly here.

@zirkelc
Copy link
Collaborator

zirkelc commented May 1, 2025

It looks good. Could you add a test so we can see the compiled IAM template?

@danrivett
Copy link
Contributor Author

It looks good. Could you add a test so we can see the compiled IAM template?

That's great. Sorry for the delay here. I'm currently under a project deadline squeeze for the next 3 weeks, but I'll get to it once that we've hit that release milestone unless someone else wants to add one ahead of that.

@danrivett danrivett force-pushed the 647-resolve-s3-bucket-reference-in-iam-permissions branch from 6795e01 to a0f6bd2 Compare June 13, 2025 03:00
Comment on lines 666 to 678
resource: resolveS3BucketReferences(bucket, [
'arn:aws:s3:::${bucket}',
'arn:aws:s3:::${bucket}/*',
]),
},
{
action: 's3:List*',
resource: [
`arn:aws:s3:::${bucket}`,
`arn:aws:s3:::${bucket}/*`,
],
resource: resolveS3BucketReferences(bucket, [
'arn:aws:s3:::${bucket}',
'arn:aws:s3:::${bucket}/*',
]),
Copy link
Contributor Author

@danrivett danrivett Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer string interpolate the ${bucket} here (hence the change to single quotes), instead we interpolate it inside the new resolveS3BucketReferences() function as that handles being passed not just a string literal for the bucket value, but also an intrinsic function.

(I forgot I added a comment about this in my initial PR submission, so I added a comment there to clarify).

@danrivett danrivett force-pushed the 647-resolve-s3-bucket-reference-in-iam-permissions branch from a0f6bd2 to 228433e Compare June 13, 2025 03:06
@danrivett danrivett force-pushed the 647-resolve-s3-bucket-reference-in-iam-permissions branch from 228433e to 14da7bf Compare June 13, 2025 03:12
@danrivett
Copy link
Contributor Author

danrivett commented Jun 13, 2025

@zirkelc I updated this PR to rebase it on the latest master branch as well as added unit tests. Please can you review the tests.

Full disclosure: I used Claude Code to generate the tests, which I then lightly edited and reviewed. They seem fine to me and consistent with other tests in that file, but I don't know the codebase well enough to know for sure.

If there are any further code review actions, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect IAM Permissions generated when S3 Bucket Reference used
2 participants