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

Remove explicit private ACLs, since ACLs are disallowed by default #10

Merged
merged 1 commit into from
Jun 12, 2023

Conversation

agerbens
Copy link
Contributor

Fixes #8

We'll probably want to make a 3.0.0 tag so as not to affect anyone on 2.

@dragonfleas
Copy link
Contributor

I don't mind doing a minor tag release for this. I don't think a major is necessary because it's not a breaking change.

@agerbens
Copy link
Contributor Author

agerbens commented Jun 10, 2023

I think it'll impact anybody that made a bucket with ACLs enabled (the old default). Suddenly having them removed would cause a change on the next terraform apply, wouldn't it?

Though, yeah, I'm not really sure that would count as a breaking change.

Another possible solution for this would be to just enable ACLs and explicitly have a private ACL, like this snippet taken from the linked issue.

resource "aws_s3_bucket" "example" {
  bucket = "my-tf-test-bucket"
}

resource "aws_s3_bucket_ownership_controls" "example" {
  bucket = aws_s3_bucket.example.id
  rule {
    object_ownership = "BucketOwnerPreferred"
  }
}

resource "aws_s3_bucket_acl" "example" {
  depends_on = [aws_s3_bucket_ownership_controls.example]

  bucket = aws_s3_bucket.example.id
  acl    = "private"
}

I think that would make it closer to the pre-April 2023 settings.
Do you have a preference?

@dragonfleas
Copy link
Contributor

I don't necessarily have a strong preference either way since we are blocking public access, I think sticking with defaults that AWS recommends unless required otherwise is wise.

I'm pretty sure we had private ACLs only as a security measure recommended by tfsec anyways.

@agerbens agerbens merged commit 5a7d337 into main Jun 12, 2023
@agerbens agerbens deleted the 8-remove-acls branch June 12, 2023 16:42
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.

As of April 2023, default bucket security settings don't allow ACLs
2 participants