From 585b4861a6116bede656c69873c31595cc3ebc61 Mon Sep 17 00:00:00 2001 From: jsclarridge <2491291+jsclarridge@users.noreply.github.com> Date: Mon, 15 May 2023 15:50:53 -0700 Subject: [PATCH 1/7] Add object ownership settings --- README.md | 3 +++ main.tf | 20 +++++++++++++++++++- variables.tf | 12 ++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index fb1dd5b..8febba5 100644 --- a/README.md +++ b/README.md @@ -106,6 +106,7 @@ No modules. | [aws_s3_bucket_acl.aws_logs](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_acl) | resource | | [aws_s3_bucket_lifecycle_configuration.aws_logs](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_lifecycle_configuration) | resource | | [aws_s3_bucket_logging.aws_logs](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_logging) | resource | +| [aws_s3_bucket_ownership_controls.aws_logs](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_ownership_controls) | resource | | [aws_s3_bucket_policy.aws_logs](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_policy) | resource | | [aws_s3_bucket_public_access_block.public_access_block](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_public_access_block) | resource | | [aws_s3_bucket_server_side_encryption_configuration.aws_logs](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_server_side_encryption_configuration) | resource | @@ -136,6 +137,7 @@ No modules. | cloudwatch\_logs\_prefix | S3 prefix for CloudWatch log exports. | `string` | `"cloudwatch"` | no | | config\_accounts | List of accounts for Config logs. By default limits to the current account. | `list(string)` | `[]` | no | | config\_logs\_prefix | S3 prefix for AWS Config logs. | `string` | `"config"` | no | +| control\_object\_ownership | Whether to manage S3 Bucket Ownership Controls on this bucket. | `bool` | `false` | no | | create\_public\_access\_block | Whether to create a public\_access\_block restricting public access to the bucket. | `bool` | `true` | no | | default\_allow | Whether all services included in this module should be allowed to write to the bucket by default. Alternatively select individual services. It's recommended to use the default bucket ACL of log-delivery-write. | `bool` | `true` | no | | elb\_accounts | List of accounts for ELB logs. By default limits to the current account. | `list(string)` | `[]` | no | @@ -148,6 +150,7 @@ No modules. | nlb\_account | Account for NLB logs. By default limits to the current account. | `string` | `""` | no | | nlb\_logs\_prefixes | S3 key prefixes for NLB logs. | `list(string)` | ```[ "nlb" ]``` | no | | noncurrent\_version\_retention | Number of days to retain non-current versions of objects if versioning is enabled. | `string` | `30` | no | +| object\_ownership | Object ownership. Valid values: BucketOwnerEnforced, BucketOwnerPreferred or ObjectWriter. | `string` | `"BucketOwnerEnforced"` | no | | redshift\_logs\_prefix | S3 prefix for RedShift logs. | `string` | `"redshift"` | no | | s3\_bucket\_acl | Set bucket ACL per [AWS S3 Canned ACL]() list. | `string` | `"log-delivery-write"` | no | | s3\_bucket\_name | S3 bucket to store AWS logs in. | `string` | n/a | yes | diff --git a/main.tf b/main.tf index c427b51..3cc9846 100644 --- a/main.tf +++ b/main.tf @@ -392,8 +392,26 @@ resource "aws_s3_bucket_policy" "aws_logs" { } resource "aws_s3_bucket_acl" "aws_logs" { + bucket = aws_s3_bucket.aws_logs.id + acl = var.s3_bucket_acl + depends_on = [aws_s3_bucket_ownership_controls.aws_logs] +} + +resource "aws_s3_bucket_ownership_controls" "aws_logs" { + count = var.control_object_ownership ? 1 : 0 + bucket = aws_s3_bucket.aws_logs.id - acl = var.s3_bucket_acl + + rule { + object_ownership = var.object_ownership + } + + # This `depends_on` is to prevent "A conflicting conditional operation is currently in progress against this resource." + depends_on = [ + aws_s3_bucket_policy.aws_logs, + aws_s3_bucket_public_access_block.public_access_block, + aws_s3_bucket.aws_logs + ] } resource "aws_s3_bucket_lifecycle_configuration" "aws_logs" { diff --git a/variables.tf b/variables.tf index 9f0dc3f..20a69d4 100644 --- a/variables.tf +++ b/variables.tf @@ -199,3 +199,15 @@ variable "enable_mfa_delete" { default = false type = bool } + +variable "control_object_ownership" { + description = "Whether to manage S3 Bucket Ownership Controls on this bucket." + type = bool + default = false +} + +variable "object_ownership" { + description = "Object ownership. Valid values: BucketOwnerEnforced, BucketOwnerPreferred or ObjectWriter." + type = string + default = "BucketOwnerEnforced" +} From d10fb340a205f8bf3f5d567da8e2e62306edca8c Mon Sep 17 00:00:00 2001 From: jsclarridge <2491291+jsclarridge@users.noreply.github.com> Date: Tue, 16 May 2023 13:18:41 -0700 Subject: [PATCH 2/7] Remove comment --- main.tf | 1 - 1 file changed, 1 deletion(-) diff --git a/main.tf b/main.tf index 3cc9846..2d7710d 100644 --- a/main.tf +++ b/main.tf @@ -406,7 +406,6 @@ resource "aws_s3_bucket_ownership_controls" "aws_logs" { object_ownership = var.object_ownership } - # This `depends_on` is to prevent "A conflicting conditional operation is currently in progress against this resource." depends_on = [ aws_s3_bucket_policy.aws_logs, aws_s3_bucket_public_access_block.public_access_block, From 4ae85f648e794b93c49edff08e8d8ef7eea8acc2 Mon Sep 17 00:00:00 2001 From: jsclarridge <2491291+jsclarridge@users.noreply.github.com> Date: Mon, 22 May 2023 09:15:11 -0700 Subject: [PATCH 3/7] Add s3 log delivery policy --- README.md | 4 +++- main.tf | 24 ++++++++++++++++++++++++ variables.tf | 14 +++++++++++++- 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 8febba5..8ca575e 100644 --- a/README.md +++ b/README.md @@ -131,13 +131,14 @@ No modules. | allow\_elb | Allow ELB service to log to bucket. | `bool` | `false` | no | | allow\_nlb | Allow NLB service to log to bucket. | `bool` | `false` | no | | allow\_redshift | Allow Redshift service to log to bucket. | `bool` | `false` | no | +| allow\_s3 | Allow S3 service to log to bucket. | `bool` | `false` | no | | cloudtrail\_accounts | List of accounts for CloudTrail logs. By default limits to the current account. | `list(string)` | `[]` | no | | cloudtrail\_logs\_prefix | S3 prefix for CloudTrail logs. | `string` | `"cloudtrail"` | no | | cloudtrail\_org\_id | AWS Organization ID for CloudTrail. | `string` | `""` | no | | cloudwatch\_logs\_prefix | S3 prefix for CloudWatch log exports. | `string` | `"cloudwatch"` | no | | config\_accounts | List of accounts for Config logs. By default limits to the current account. | `list(string)` | `[]` | no | | config\_logs\_prefix | S3 prefix for AWS Config logs. | `string` | `"config"` | no | -| control\_object\_ownership | Whether to manage S3 Bucket Ownership Controls on this bucket. | `bool` | `false` | no | +| control\_object\_ownership | Whether to manage S3 Bucket Ownership Controls on this bucket. | `bool` | `true` | no | | create\_public\_access\_block | Whether to create a public\_access\_block restricting public access to the bucket. | `bool` | `true` | no | | default\_allow | Whether all services included in this module should be allowed to write to the bucket by default. Alternatively select individual services. It's recommended to use the default bucket ACL of log-delivery-write. | `bool` | `true` | no | | elb\_accounts | List of accounts for ELB logs. By default limits to the current account. | `list(string)` | `[]` | no | @@ -155,6 +156,7 @@ No modules. | s3\_bucket\_acl | Set bucket ACL per [AWS S3 Canned ACL]() list. | `string` | `"log-delivery-write"` | no | | s3\_bucket\_name | S3 bucket to store AWS logs in. | `string` | n/a | yes | | s3\_log\_bucket\_retention | Number of days to keep AWS logs around. | `string` | `90` | no | +| s3\_logs\_prefix | S3 prefix for S3 access logs. | `string` | `"s3"` | no | | tags | A mapping of tags to assign to the logs bucket. Please note that tags with a conflicting key will not override the original tag. | `map(string)` | `{}` | no | | versioning\_status | A string that indicates the versioning status for the log bucket. | `string` | `"Disabled"` | no | diff --git a/main.tf b/main.tf index 2d7710d..61b957a 100644 --- a/main.tf +++ b/main.tf @@ -148,6 +148,15 @@ locals { redshift_principal = "arn:${data.aws_partition.current.partition}:iam::${data.aws_redshift_service_account.main.id}:user/logs" redshift_resource = "${local.bucket_arn}/${var.redshift_logs_prefix}/*" + + # + # S3 locals + # + + # doesn't support logging to multiple accounts + s3_effect = var.default_allow || var.allow_s3 ? "Allow" : "Deny" + + s3_resources = "${local.bucket_arn}/${var.s3_logs_prefix}/*" } # @@ -344,6 +353,21 @@ data "aws_iam_policy_document" "main" { resources = [local.bucket_arn] } + # + # S3 server access log bucket policies + # + + statement { + sid = "s3-logs-put-object" + effect = local.s3_effect + principals { + type = "Service" + identifiers = ["logging.s3.amazonaws.com"] + } + actions = ["s3:PutObject"] + resources = local.s3_resources + } + # # Enforce TLS requests only # diff --git a/variables.tf b/variables.tf index 20a69d4..d591b87 100644 --- a/variables.tf +++ b/variables.tf @@ -21,6 +21,12 @@ variable "s3_bucket_acl" { type = string } +variable "s3_logs_prefix" { + description = "S3 prefix for S3 access logs." + default = "s3" + type = string +} + variable "elb_logs_prefix" { description = "S3 prefix for ELB logs." default = "elb" @@ -106,6 +112,12 @@ variable "allow_redshift" { type = bool } +variable "allow_s3" { + description = "Allow S3 service to log to bucket." + default = false + type = bool +} + variable "create_public_access_block" { description = "Whether to create a public_access_block restricting public access to the bucket." default = true @@ -203,7 +215,7 @@ variable "enable_mfa_delete" { variable "control_object_ownership" { description = "Whether to manage S3 Bucket Ownership Controls on this bucket." type = bool - default = false + default = true } variable "object_ownership" { From c2de2261b6fa91c0782745cee950a6eb0c778474 Mon Sep 17 00:00:00 2001 From: jsclarridge <2491291+jsclarridge@users.noreply.github.com> Date: Mon, 22 May 2023 09:24:04 -0700 Subject: [PATCH 4/7] Make s3 resources a list --- main.tf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.tf b/main.tf index 61b957a..fc3d8ac 100644 --- a/main.tf +++ b/main.tf @@ -156,7 +156,7 @@ locals { # doesn't support logging to multiple accounts s3_effect = var.default_allow || var.allow_s3 ? "Allow" : "Deny" - s3_resources = "${local.bucket_arn}/${var.s3_logs_prefix}/*" + s3_resources = ["${local.bucket_arn}/${var.s3_logs_prefix}/*"] } # From 6bcc7d12720759603b353368c9ca3f0e6feff40c Mon Sep 17 00:00:00 2001 From: jsclarridge <2491291+jsclarridge@users.noreply.github.com> Date: Mon, 22 May 2023 17:03:18 -0700 Subject: [PATCH 5/7] Change default acl value --- README.md | 2 +- main.tf | 1 + variables.tf | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 8ca575e..0631354 100644 --- a/README.md +++ b/README.md @@ -153,7 +153,7 @@ No modules. | noncurrent\_version\_retention | Number of days to retain non-current versions of objects if versioning is enabled. | `string` | `30` | no | | object\_ownership | Object ownership. Valid values: BucketOwnerEnforced, BucketOwnerPreferred or ObjectWriter. | `string` | `"BucketOwnerEnforced"` | no | | redshift\_logs\_prefix | S3 prefix for RedShift logs. | `string` | `"redshift"` | no | -| s3\_bucket\_acl | Set bucket ACL per [AWS S3 Canned ACL]() list. | `string` | `"log-delivery-write"` | no | +| s3\_bucket\_acl | Set bucket ACL per [AWS S3 Canned ACL]() list. | `string` | `null` | no | | s3\_bucket\_name | S3 bucket to store AWS logs in. | `string` | n/a | yes | | s3\_log\_bucket\_retention | Number of days to keep AWS logs around. | `string` | `90` | no | | s3\_logs\_prefix | S3 prefix for S3 access logs. | `string` | `"s3"` | no | diff --git a/main.tf b/main.tf index fc3d8ac..10215b5 100644 --- a/main.tf +++ b/main.tf @@ -416,6 +416,7 @@ resource "aws_s3_bucket_policy" "aws_logs" { } resource "aws_s3_bucket_acl" "aws_logs" { + count = var.s3_bucket_acl != null ? 1 : 0 bucket = aws_s3_bucket.aws_logs.id acl = var.s3_bucket_acl depends_on = [aws_s3_bucket_ownership_controls.aws_logs] diff --git a/variables.tf b/variables.tf index d591b87..e66e35f 100644 --- a/variables.tf +++ b/variables.tf @@ -17,7 +17,7 @@ variable "noncurrent_version_retention" { variable "s3_bucket_acl" { description = "Set bucket ACL per [AWS S3 Canned ACL]() list." - default = "log-delivery-write" + default = null type = string } From 01a4f5eafdbaf072c55b78c90da67c953835f421 Mon Sep 17 00:00:00 2001 From: jsclarridge <2491291+jsclarridge@users.noreply.github.com> Date: Mon, 22 May 2023 18:11:28 -0700 Subject: [PATCH 6/7] Add upgrade path to readme --- README.md | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/README.md b/README.md index 0631354..f3eea42 100644 --- a/README.md +++ b/README.md @@ -171,3 +171,57 @@ No modules. | redshift\_logs\_path | S3 path for RedShift logs. | | s3\_bucket\_policy | S3 bucket policy | + +## Upgrade Paths + +### Upgrading from 14.x.x to 15.x.x + +Version 15.x.x updates the module to account for changes made by AWS in April +2023 to the default security settings of new S3 buckets. + +Version 15.x.x of this module adds the following resource and variables. How to +use the new variables will depend on your use case. + +New resource: + +- `aws_s3_bucket_ownership_controls.aws_logs` + +New variables: + +- `allow_s3` +- `control_object_ownership` +- `object_ownership` +- `s3_bucket_acl` +- `s3_logs_prefix` + +Steps for updating existing buckets managed by this module: + +- **Option 1: Disable ACLs.** In order to update an existing log bucket to use + the new AWS recommended defaults, use this module's default values for the new + input variables. Using those settings will disable S3 access control lists for + the bucket and set object ownership to `BucketOwnerEnforced`. Update the log + bucket policy to grant `s3:PutObject` permission to the logging service + principal (`logging.s3.amazonaws.com`). + + Example: + +```text + statement { + sid = "s3-logs-put-object" + effect = "Allow" + principals { + type = "Service" + identifiers = ["logging.s3.amazonaws.com"] + } + actions = ["s3:PutObject"] + resources = ["BUCKET_ARN_PLACEHOLDER/LOGGING_PREFIX_PLACEHOLDER/*"] + } +``` + +- **Option 2: Continue using ACLs.** To continue using ACLs, set `s3_bucket_acl` + to `"log-delivery-write"` and set `object_ownership` to `ObjectWriter` or + `BucketOwnerPreferred`. + +See [Controlling ownership of objects and disabling ACLs for your +bucket](https://docs.aws.amazon.com/AmazonS3/latest/userguide/about-object-ownership.html) +for further details and migration considerations. From 2c00055b4ca9b09c41dd8f35d281968de44c27f8 Mon Sep 17 00:00:00 2001 From: jsclarridge <2491291+jsclarridge@users.noreply.github.com> Date: Tue, 23 May 2023 15:26:27 -0700 Subject: [PATCH 7/7] Clarify order of operations --- README.md | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index f3eea42..197d146 100644 --- a/README.md +++ b/README.md @@ -196,12 +196,17 @@ New variables: Steps for updating existing buckets managed by this module: -- **Option 1: Disable ACLs.** In order to update an existing log bucket to use - the new AWS recommended defaults, use this module's default values for the new - input variables. Using those settings will disable S3 access control lists for - the bucket and set object ownership to `BucketOwnerEnforced`. Update the log - bucket policy to grant `s3:PutObject` permission to the logging service - principal (`logging.s3.amazonaws.com`). +- **Option 1: Disable ACLs.** This module's default values for + `control_object_ownership`, `object_ownership`, and `s3_bucket_acl` follow the + new AWS recommended best practice. For a new S3 bucket, using those settings + will disable S3 access control lists for the bucket and set object ownership + to `BucketOwnerEnforced`. For an existing bucket that is used to store s3 + server access logs, the bucket ACL permissions for the S3 log delivery group + must be migrated to the bucket policy. The changes must be applied + in multiple steps. + +Step 1: Update the log bucket policy to grant `s3:PutObject` permission to the +logging service principal (`logging.s3.amazonaws.com`). Example: @@ -218,6 +223,10 @@ Steps for updating existing buckets managed by this module: } ``` +Step 2: Change `s3_bucket_acl` to `private`. + +Step 3: Change `object_ownership` to `BucketOwnerEnforced`. + - **Option 2: Continue using ACLs.** To continue using ACLs, set `s3_bucket_acl` to `"log-delivery-write"` and set `object_ownership` to `ObjectWriter` or `BucketOwnerPreferred`.