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

allow_encrypted_uploads_only overrides policy #11

Closed
rajcheval opened this issue May 28, 2019 · 4 comments · Fixed by #82
Closed

allow_encrypted_uploads_only overrides policy #11

rajcheval opened this issue May 28, 2019 · 4 comments · Fixed by #82

Comments

@rajcheval
Copy link

If I create a new bucket and pass in a policy and setup allow_encrypted_uploads_only the policy is ignored and bucket policy only contains allow_encrypted_uploads_only related statements.

@darend
Copy link

darend commented May 28, 2019

One way to solve it: hashicorp/terraform-provider-aws#2890

@rajcheval
Copy link
Author

Thanks @darend

I took a quick look at data source aws_iam_policy_document
It has 2 properties source_json and override_json
I am trying to figure out how to implement this.

If policy parameter is specified it will be used to set the soure_json.

Now if a user sets the property allow_encrypted_uploads_only I will set override_json.
If both source_json and override_json are set are their statements merged?
without reading the code or executing this I don't know if I have to set source_json, override_json and the final policy will combine the statements.

I will contribute the final solution back into the repo.

@darend
Copy link

darend commented May 28, 2019

Off hand Im not sure how to do it in this repo because aws_iam_policy_document.bucket_policy is optional. If it was always enabled, then you would do:

data "aws_iam_policy_document" "bucket_policy" {
  count = "${var.enabled == "true" ? 1 : 0}"

  source_json = "${var.policy}"

  statement {
    sid       = "DenyIncorrectEncryptionHeader"
    effect    = "Deny"

....

jaymed added a commit to jaymed/terraform-aws-s3-bucket that referenced this issue Aug 31, 2020
* 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
@jaymed
Copy link
Contributor

jaymed commented Aug 31, 2020

Off hand Im not sure how to do it in this repo because aws_iam_policy_document.bucket_policy is optional. If it was always enabled, then you would do:

data "aws_iam_policy_document" "bucket_policy" {
  count = "${var.enabled == "true" ? 1 : 0}"

  source_json = "${var.policy}"

  statement {
    sid       = "DenyIncorrectEncryptionHeader"
    effect    = "Deny"

....

@darend Thanks for providing the clues for solving this issue. I've submitted a pull request (#49) which uses dynamic blocks to build out the aws_iam_policy_document.bucket_policy resource. The resource merges in the user policy via the source_json argument (as you mention above). I had to remove the s3_bucket policy argument to avoid conflicting updates to the bucket policy. The merged policy now provides both the user defined policy and the additional ssl/encryption policies.

jaymed added a commit to jaymed/terraform-aws-s3-bucket that referenced this issue Sep 1, 2020
* 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

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.

Small formatting change

* Small formatting change after running `terraform fmt`.
jaymed added a commit to jaymed/terraform-aws-s3-bucket that referenced this issue Sep 1, 2020
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants