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.

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

Policy to allow only ssl uploads #49

Closed
wants to merge 2 commits into from
Closed

Conversation

jaymed
Copy link
Contributor

@jaymed jaymed commented Aug 31, 2020

what

  • Adds enable flag to allow only ssl/https bucket uploads.
  • Includes logic to merge other policies enabled by the user such as the string policy passed in via the policy variable and the other encryption policy. This prevents overriding the user defined policy (mentioned in issue allow_encrypted_uploads_only overrides policy #11) as long as the sid values are distinct.

why

  • Provides compliance with AWS Config rule s3-bucket-ssl-requests-only.
  • Fixes an outstanding issues which prevents users from specifying their own policy string and enabling userful policies pre-defined within the module.

references

@aknysh
Copy link
Member

aknysh commented Sep 1, 2020

@jaymed thanks for the PR, please rebase

jaymed added 2 commits August 31, 2020 21:17
* Added policy to allow only ssl/https uploads
* Merge user defined bucket policy with any additional/enabled policies defined within the module. This prevents overriding the user defined policy (`var.policy`) as long as the `sid` values are distinct.

Closes cloudposse#11
* Allow merging the user defined policy by specifying it in the `source_json` of the default `aws_s3_bucket` resource. This simplifies the previous commit which created a new resource.
* Remove the s3 bucket resource `policy` argument to avoid colliding with the merged s3 bucket policy document that will define the overall bucket policy.
@jaymed
Copy link
Contributor Author

jaymed commented Sep 1, 2020

@jaymed thanks for the PR, please rebase

Thanks! I just rebased. Please take a look.

@mergify
Copy link

mergify bot commented Jan 14, 2021

This pull request is now in conflict. Could you fix it @jaymed? 🙏

jamengual pushed a commit that referenced this pull request Apr 4, 2021
* Merge user defined policy with ssl policy

* Allow merging the user defined policy by specifying it in the `source_json` of the default `aws_s3_bucket` resource. This simplifies the previous commit which created a new resource.
* Remove the s3 bucket resource `policy` argument to avoid colliding with the merged s3 bucket policy document that will define the overall bucket policy.

Conflicts:
	main.tf

* use module.this.enabled

* Auto Format

* Update main.tf

Co-authored-by: Jay Medrano <[email protected]>
Co-authored-by: cloudpossebot <[email protected]>
@pperzyna
Copy link
Contributor

pperzyna commented Apr 4, 2021

@jaymed You can close this PR, as PR #82 has been merged.

@jamengual
Copy link
Contributor

This is already addressed in the new versions

@jamengual jamengual closed this Apr 9, 2021
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.

allow_encrypted_uploads_only overrides policy
4 participants