Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added IP-based statement in bucket policy #216

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Added IP-based statement in bucket policy #216

merged 1 commit into from
Mar 4, 2024

Conversation

soya-miyoshi
Copy link
Contributor

@soya-miyoshi soya-miyoshi commented Feb 26, 2024 •
edited
Loading

what

  • Allows users to specify a list of source IP addresses from which access to the S3 bucket is allowed.
  • Adds dynamic statement that uses the NotIpAddress condition to deny access from any IP address not listed in the source_ip_allow_list variable.

why

Use cases:

  • Restricting access to specific physical locations, such as an office or home network

references

@soya-miyoshi soya-miyoshi requested review from a team as code owners February 26, 2024 06:08
joe-niland
joe-niland reviewed Feb 26, 2024
View reviewed changes
main.tf
effect = "Deny"
actions = ["s3:*"]
resources = [local.bucket_arn, "${local.bucket_arn}/*"]
principals {
Copy link
Member

Choose a reason for hiding this comment

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

Is this section required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for not writing the description...
Yes, it is required to achieve what I wrote in the description now.
Could you please review my PR again?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry what I meant was is the principals element required?

Copy link
Contributor Author

@soya-miyoshi soya-miyoshi Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it is required to be a valid policy. If we omit this we will get MalformedPolicyDocument error.

The reason for the wildcard is that I would like to block any principal outside of the specified IP.

joe-niland
joe-niland reviewed Feb 26, 2024
View reviewed changes
Copy link
Member

@joe-niland joe-niland left a comment

Choose a reason for hiding this comment

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

Hi @soya-miyoshi thanks for your contribution.
Could you please fill out the PR template?

@joe-niland
Copy link
Member

Also @soya-miyoshi could you please run the following and commit the result?

make init
make readme
terraform fmt

@soya-miyoshi
Copy link
Contributor Author

soya-miyoshi commented Feb 27, 2024 •
edited
Loading

@joe-niland
Thank you for your quick feedback! I've executed the command you suggested and have updated my commit accordingly. Please let me know if there are any more adjustments or improvements that you think could be made. I'm keen to ensure this is as aligned with your standards as possible.

@hans-d
Copy link

hans-d commented Mar 2, 2024

/terratest

@soya-miyoshi
Copy link
Contributor Author

@hans-d
I ran terraform fmt again, and now the new variable.tf should pass the check.
Can you please run the test again?

@hans-d
Copy link

hans-d commented Mar 3, 2024

/terratest

@hans-d
Copy link

hans-d commented Mar 3, 2024

@joe-niland can you have a recheck again? for me its a lgtm

@soya-miyoshi
Copy link
Contributor Author

@hans-d
Sorry, it seemed that I needed to rebase my branch. Could you please run the check again?

@joe-niland
Copy link
Member

/terratest

@joe-niland joe-niland merged commit d8bc15d into cloudposse:main Mar 4, 2024
10 checks passed
@joe-niland
Copy link
Member

Thanks @soya-miyoshi ! Released as https://github.com/cloudposse/terraform-aws-s3-bucket/releases/tag/4.2.0

@soya-miyoshi
Copy link
Contributor Author

@.joe-niland @.hans-d
Thanks for your work too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe-niland joe-niland joe-niland approved these changes

@Gowiem Gowiem Awaiting requested review from Gowiem Gowiem is a code owner automatically assigned from cloudposse/contributors

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@soya-miyoshi @joe-niland @hans-d