From 60fcb565c07921d1a67003b9cc4e9f95bfee432a Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Dec 2021 19:20:57 -0500 Subject: [PATCH 1/8] Preserve order of JSON arrays --- internal/service/s3/bucket.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/internal/service/s3/bucket.go b/internal/service/s3/bucket.go index 0f8394fa479..387c4089be0 100644 --- a/internal/service/s3/bucket.go +++ b/internal/service/s3/bucket.go @@ -915,7 +915,14 @@ func resourceBucketRead(d *schema.ResourceData, meta interface{}) error { if err != nil { return fmt.Errorf("policy contains an invalid JSON: %s", err) } - d.Set("policy", policy) + + policyToSet, err := verify.SecondJSONUnlessEquivalent(d.Get("policy").(string), policy) + + if err != nil { + return fmt.Errorf("while setting policy (%s), encountered: %w", policy, err) + } + + d.Set("policy", policyToSet) } } } From 73d590b865c53a1b0dce23a4dbb75a60cec193d0 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Dec 2021 19:21:38 -0500 Subject: [PATCH 2/8] s3/bucket_policy: Preserve order within JSON arrays --- internal/service/s3/bucket_policy.go | 28 +- internal/service/s3/bucket_policy_test.go | 322 +++++++++++++++++++++- 2 files changed, 344 insertions(+), 6 deletions(-) diff --git a/internal/service/s3/bucket_policy.go b/internal/service/s3/bucket_policy.go index ca072ce23cd..f3c63f5cf03 100644 --- a/internal/service/s3/bucket_policy.go +++ b/internal/service/s3/bucket_policy.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/hashicorp/terraform-provider-aws/internal/conns" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" @@ -48,7 +49,12 @@ func resourceBucketPolicyPut(d *schema.ResourceData, meta interface{}) error { conn := meta.(*conns.AWSClient).S3Conn bucket := d.Get("bucket").(string) - policy := d.Get("policy").(string) + + policy, err := structure.NormalizeJsonString(d.Get("policy").(string)) + + if err != nil { + return fmt.Errorf("policy (%s) is an invalid JSON: %w", policy, err) + } log.Printf("[DEBUG] S3 bucket: %s, put policy: %s", bucket, policy) @@ -57,7 +63,7 @@ func resourceBucketPolicyPut(d *schema.ResourceData, meta interface{}) error { Policy: aws.String(policy), } - err := resource.Retry(1*time.Minute, func() *resource.RetryError { + err = resource.Retry(1*time.Minute, func() *resource.RetryError { _, err := conn.PutBucketPolicy(params) if tfawserr.ErrMessageContains(err, "MalformedPolicy", "") { return resource.RetryableError(err) @@ -89,11 +95,25 @@ func resourceBucketPolicyRead(d *schema.ResourceData, meta interface{}) error { v := "" if err == nil && pol.Policy != nil { - v = *pol.Policy + v = aws.StringValue(pol.Policy) + } + + policyToSet, err := verify.SecondJSONUnlessEquivalent(d.Get("policy").(string), v) + + if err != nil { + return fmt.Errorf("while setting policy (%s), encountered: %w", policyToSet, err) } - if err := d.Set("policy", v); err != nil { + + policyToSet, err = structure.NormalizeJsonString(policyToSet) + + if err != nil { + return fmt.Errorf("policy (%s) is an invalid JSON: %w", policyToSet, err) + } + + if err := d.Set("policy", policyToSet); err != nil { return err } + if err := d.Set("bucket", d.Id()); err != nil { return err } diff --git a/internal/service/s3/bucket_policy_test.go b/internal/service/s3/bucket_policy_test.go index c4e288a0f21..ee922d49958 100644 --- a/internal/service/s3/bucket_policy_test.go +++ b/internal/service/s3/bucket_policy_test.go @@ -133,6 +133,86 @@ func TestAccS3BucketPolicy_policyUpdate(t *testing.T) { }) } +// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/11801 +func TestAccS3BucketPolicy_IAMRoleOrder_policyDoc(t *testing.T) { + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_s3_bucket.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID), + Providers: acctest.Providers, + CheckDestroy: testAccCheckBucketDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBucketPolicyIAMRoleOrderIAMPolicyDocConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketExists(resourceName), + ), + }, + { + Config: testAccBucketPolicyIAMRoleOrderIAMPolicyDocConfig(rName), + PlanOnly: true, + }, + { + Config: testAccBucketPolicyIAMRoleOrderIAMPolicyDocConfig(rName), + PlanOnly: true, + }, + { + Config: testAccBucketPolicyIAMRoleOrderIAMPolicyDocConfig(rName), + PlanOnly: true, + }, + }, + }) +} + +// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/11801 +func TestAccS3BucketPolicy_IAMRoleOrder_jsonEncode(t *testing.T) { + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + rName2 := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + rName3 := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_s3_bucket.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID), + Providers: acctest.Providers, + CheckDestroy: testAccCheckBucketDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBucketPolicyIAMRoleOrderJSONEncodeConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketExists(resourceName), + ), + }, + { + Config: testAccBucketPolicyIAMRoleOrderJSONEncodeConfig(rName), + PlanOnly: true, + }, + { + Config: testAccBucketPolicyIAMRoleOrderJSONEncodeOrder2Config(rName2), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketExists(resourceName), + ), + }, + { + Config: testAccBucketPolicyIAMRoleOrderJSONEncodeConfig(rName2), + PlanOnly: true, + }, + { + Config: testAccBucketPolicyIAMRoleOrderJSONEncodeOrder3Config(rName3), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketExists(resourceName), + ), + }, + { + Config: testAccBucketPolicyIAMRoleOrderJSONEncodeConfig(rName3), + PlanOnly: true, + }, + }, + }) +} + func testAccCheckBucketHasPolicy(n string, expectedPolicyText string) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -171,7 +251,7 @@ func testAccCheckBucketHasPolicy(n string, expectedPolicyText string) resource.T func testAccBucketPolicyConfig(bucketName string) string { return fmt.Sprintf(` resource "aws_s3_bucket" "bucket" { - bucket = "%s" + bucket = %[1]q tags = { TestName = "TestAccS3BucketPolicy_basic" @@ -208,7 +288,7 @@ data "aws_iam_policy_document" "policy" { func testAccBucketPolicyConfig_updated(bucketName string) string { return fmt.Sprintf(` resource "aws_s3_bucket" "bucket" { - bucket = "%s" + bucket = %[1]q tags = { TestName = "TestAccS3BucketPolicy_basic" @@ -243,3 +323,241 @@ data "aws_iam_policy_document" "policy" { } `, bucketName) } + +func testAccBucketPolicyIAMRoleOrderBaseConfig(rName string) string { + return fmt.Sprintf(` +data "aws_partition" "current" {} + +resource "aws_iam_role" "test1" { + name = "%[1]s-sultan" + + assume_role_policy = jsonencode({ + Statement = [{ + Action = "sts:AssumeRole" + Effect = "Allow" + Principal = { + Service = "s3.${data.aws_partition.current.dns_suffix}" + } + }] + Version = "2012-10-17" + }) +} + +resource "aws_iam_role" "test2" { + name = "%[1]s-shepard" + + assume_role_policy = jsonencode({ + Statement = [{ + Action = "sts:AssumeRole" + Effect = "Allow" + Principal = { + Service = "s3.${data.aws_partition.current.dns_suffix}" + } + }] + Version = "2012-10-17" + }) +} + +resource "aws_iam_role" "test3" { + name = "%[1]s-tritonal" + + assume_role_policy = jsonencode({ + Statement = [{ + Action = "sts:AssumeRole" + Effect = "Allow" + Principal = { + Service = "s3.${data.aws_partition.current.dns_suffix}" + } + }] + Version = "2012-10-17" + }) +} + +resource "aws_iam_role" "test4" { + name = "%[1]s-artlec" + + assume_role_policy = jsonencode({ + Statement = [{ + Action = "sts:AssumeRole" + Effect = "Allow" + Principal = { + Service = "s3.${data.aws_partition.current.dns_suffix}" + } + }] + Version = "2012-10-17" + }) +} + +resource "aws_iam_role" "test5" { + name = "%[1]s-cazzette" + + assume_role_policy = jsonencode({ + Statement = [{ + Action = "sts:AssumeRole" + Effect = "Allow" + Principal = { + Service = "s3.${data.aws_partition.current.dns_suffix}" + } + }] + Version = "2012-10-17" + }) +} + +resource "aws_s3_bucket" "test" { + bucket = %[1]q + + tags = { + TestName = %[1]q + } +} +`, rName) +} + +func testAccBucketPolicyIAMRoleOrderIAMPolicyDocConfig(rName string) string { + return acctest.ConfigCompose( + testAccBucketPolicyIAMRoleOrderBaseConfig(rName), + fmt.Sprintf(` +data "aws_iam_policy_document" "test" { + policy_id = %[1]q + + statement { + actions = [ + "s3:DeleteBucket", + "s3:ListBucket", + "s3:ListBucketVersions", + ] + effect = "Allow" + principals { + identifiers = [ + aws_iam_role.test2.arn, + aws_iam_role.test1.arn, + aws_iam_role.test4.arn, + aws_iam_role.test3.arn, + aws_iam_role.test5.arn, + ] + type = "AWS" + } + resources = [ + aws_s3_bucket.test.arn, + "${aws_s3_bucket.test.arn}/*", + ] + } +} + +resource "aws_s3_bucket_policy" "bucket" { + bucket = aws_s3_bucket.test.bucket + policy = data.aws_iam_policy_document.test.json +} +`, rName)) +} + +func testAccBucketPolicyIAMRoleOrderJSONEncodeConfig(rName string) string { + return acctest.ConfigCompose( + testAccBucketPolicyIAMRoleOrderBaseConfig(rName), + fmt.Sprintf(` +resource "aws_s3_bucket_policy" "bucket" { + bucket = aws_s3_bucket.test.bucket + + policy = jsonencode({ + Id = %[1]q + Statement = [{ + Action = [ + "s3:DeleteBucket", + "s3:ListBucket", + "s3:ListBucketVersions", + ] + Effect = "Allow" + Principal = { + AWS = [ + aws_iam_role.test2.arn, + aws_iam_role.test1.arn, + aws_iam_role.test4.arn, + aws_iam_role.test3.arn, + aws_iam_role.test5.arn, + ] + } + + Resource = [ + aws_s3_bucket.test.arn, + "${aws_s3_bucket.test.arn}/*", + ] + }] + Version = "2012-10-17" + }) +} +`, rName)) +} + +func testAccBucketPolicyIAMRoleOrderJSONEncodeOrder2Config(rName string) string { + return acctest.ConfigCompose( + testAccBucketPolicyIAMRoleOrderBaseConfig(rName), + fmt.Sprintf(` +resource "aws_s3_bucket_policy" "bucket" { + bucket = aws_s3_bucket.test.bucket + + policy = jsonencode({ + Id = %[1]q + Statement = [{ + Action = [ + "s3:DeleteBucket", + "s3:ListBucket", + "s3:ListBucketVersions", + ] + Effect = "Allow" + Principal = { + AWS = [ + aws_iam_role.test2.arn, + aws_iam_role.test3.arn, + aws_iam_role.test5.arn, + aws_iam_role.test1.arn, + aws_iam_role.test4.arn, + ] + } + + Resource = [ + aws_s3_bucket.test.arn, + "${aws_s3_bucket.test.arn}/*", + ] + }] + Version = "2012-10-17" + }) +} +`, rName)) +} + +func testAccBucketPolicyIAMRoleOrderJSONEncodeOrder3Config(rName string) string { + return acctest.ConfigCompose( + testAccBucketPolicyIAMRoleOrderBaseConfig(rName), + fmt.Sprintf(` +resource "aws_s3_bucket_policy" "bucket" { + bucket = aws_s3_bucket.test.bucket + + policy = jsonencode({ + Id = %[1]q + Statement = [{ + Action = [ + "s3:DeleteBucket", + "s3:ListBucket", + "s3:ListBucketVersions", + ] + Effect = "Allow" + Principal = { + AWS = [ + aws_iam_role.test4.arn, + aws_iam_role.test1.arn, + aws_iam_role.test3.arn, + aws_iam_role.test5.arn, + aws_iam_role.test2.arn, + ] + } + + Resource = [ + aws_s3_bucket.test.arn, + "${aws_s3_bucket.test.arn}/*", + ] + }] + Version = "2012-10-17" + }) +} +`, rName)) +} From a94a3882099665059636c5fc4a980754b063af56 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Dec 2021 19:23:28 -0500 Subject: [PATCH 3/8] Add changelog --- .changelog/21997.txt | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changelog/21997.txt diff --git a/.changelog/21997.txt b/.changelog/21997.txt new file mode 100644 index 00000000000..a836289995a --- /dev/null +++ b/.changelog/21997.txt @@ -0,0 +1,7 @@ +```release-note:bug +resource/aws_s3_bucket: Fix order-related diffs in `policy` +``` + +```release-note:bug +resource/aws_s3_bucket_policy: Fix order-related diffs in `policy` +``` \ No newline at end of file From 54ea64d83e50b0940fdd4a92f009e9f5728defa8 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Dec 2021 19:28:20 -0500 Subject: [PATCH 4/8] Fix a lint --- internal/service/s3/bucket_policy_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/service/s3/bucket_policy_test.go b/internal/service/s3/bucket_policy_test.go index ee922d49958..6e6cddc0ebc 100644 --- a/internal/service/s3/bucket_policy_test.go +++ b/internal/service/s3/bucket_policy_test.go @@ -483,7 +483,7 @@ resource "aws_s3_bucket_policy" "bucket" { ] }] Version = "2012-10-17" - }) + }) } `, rName)) } @@ -520,7 +520,7 @@ resource "aws_s3_bucket_policy" "bucket" { ] }] Version = "2012-10-17" - }) + }) } `, rName)) } @@ -557,7 +557,7 @@ resource "aws_s3_bucket_policy" "bucket" { ] }] Version = "2012-10-17" - }) + }) } `, rName)) } From d0fb77a25792d175d5d3b7edf837e677e6613b40 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Dec 2021 19:47:13 -0500 Subject: [PATCH 5/8] Normalize policy, sync with bucket_policy --- internal/service/s3/bucket.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/internal/service/s3/bucket.go b/internal/service/s3/bucket.go index 387c4089be0..e653f7e5e1b 100644 --- a/internal/service/s3/bucket.go +++ b/internal/service/s3/bucket.go @@ -911,15 +911,16 @@ func resourceBucketRead(d *schema.ResourceData, meta interface{}) error { return err } } else { - policy, err := structure.NormalizeJsonString(aws.StringValue(v)) + policyToSet, err := verify.SecondJSONUnlessEquivalent(d.Get("policy").(string), aws.StringValue(v)) + if err != nil { - return fmt.Errorf("policy contains an invalid JSON: %s", err) + return fmt.Errorf("while setting policy (%s), encountered: %w", aws.StringValue(v), err) } - policyToSet, err := verify.SecondJSONUnlessEquivalent(d.Get("policy").(string), policy) + policyToSet, err = structure.NormalizeJsonString(policyToSet) if err != nil { - return fmt.Errorf("while setting policy (%s), encountered: %w", policy, err) + return fmt.Errorf("policy (%s) contains invalid JSON: %w", d.Get("policy").(string), err) } d.Set("policy", policyToSet) @@ -1451,7 +1452,12 @@ func resourceBucketDelete(d *schema.ResourceData, meta interface{}) error { func resourceBucketPolicyUpdate(conn *s3.S3, d *schema.ResourceData) error { bucket := d.Get("bucket").(string) - policy := d.Get("policy").(string) + + policy, err := structure.NormalizeJsonString(d.Get("policy").(string)) + + if err != nil { + return fmt.Errorf("policy (%s) is an invalid JSON: %w", policy, err) + } if policy != "" { log.Printf("[DEBUG] S3 bucket: %s, put policy: %s", bucket, policy) From 46d6cdad3b67f1b2b740f0ddcf818eded9c6a857 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Dec 2021 20:08:08 -0500 Subject: [PATCH 6/8] Add test for 13144 --- internal/service/s3/bucket_policy_test.go | 78 +++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/internal/service/s3/bucket_policy_test.go b/internal/service/s3/bucket_policy_test.go index 6e6cddc0ebc..a3740351326 100644 --- a/internal/service/s3/bucket_policy_test.go +++ b/internal/service/s3/bucket_policy_test.go @@ -166,6 +166,41 @@ func TestAccS3BucketPolicy_IAMRoleOrder_policyDoc(t *testing.T) { }) } +// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/13144 +func TestAccS3BucketPolicy_IAMRoleOrder_policyDocNotPrincipal(t *testing.T) { + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_s3_bucket.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID), + Providers: acctest.Providers, + CheckDestroy: testAccCheckBucketDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBucketPolicyIAMRoleOrderIAMPolicyDocNotPrincipalConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketExists(resourceName), + ), + }, + { + Config: testAccBucketPolicyIAMRoleOrderIAMPolicyDocNotPrincipalConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketExists(resourceName), + ), + }, + { + Config: testAccBucketPolicyIAMRoleOrderIAMPolicyDocNotPrincipalConfig(rName), + PlanOnly: true, + }, + { + Config: testAccBucketPolicyIAMRoleOrderIAMPolicyDocNotPrincipalConfig(rName), + PlanOnly: true, + }, + }, + }) +} + // Reference: https://github.com/hashicorp/terraform-provider-aws/issues/11801 func TestAccS3BucketPolicy_IAMRoleOrder_jsonEncode(t *testing.T) { rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) @@ -561,3 +596,46 @@ resource "aws_s3_bucket_policy" "bucket" { } `, rName)) } + +func testAccBucketPolicyIAMRoleOrderIAMPolicyDocNotPrincipalConfig(rName string) string { + return acctest.ConfigCompose( + testAccBucketPolicyIAMRoleOrderBaseConfig(rName), + ` +data "aws_caller_identity" "current" {} + +data "aws_iam_policy_document" "test" { + statement { + sid = "DenyInfected" + actions = [ + "s3:GetObject", + "s3:PutObjectTagging", + ] + effect = "Deny" + not_principals { + identifiers = [ + aws_iam_role.test2.arn, + aws_iam_role.test3.arn, + aws_iam_role.test4.arn, + aws_iam_role.test1.arn, + aws_iam_role.test5.arn, + data.aws_caller_identity.current.arn, + ] + type = "AWS" + } + resources = [ + "${aws_s3_bucket.test.arn}/*", + ] + condition { + test = "StringEquals" + variable = "s3:ExistingObjectTag/av-status" + values = ["INFECTED"] + } + } +} + +resource "aws_s3_bucket_policy" "bucket" { + bucket = aws_s3_bucket.test.bucket + policy = data.aws_iam_policy_document.test.json +} +`) +} From bd55f250bc87747e6f2d000bfb71587abc87c1c2 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Dec 2021 20:18:20 -0500 Subject: [PATCH 7/8] Out of the frying pan and lint to the fire --- internal/service/s3/bucket_policy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/s3/bucket_policy_test.go b/internal/service/s3/bucket_policy_test.go index a3740351326..8ef272c50b2 100644 --- a/internal/service/s3/bucket_policy_test.go +++ b/internal/service/s3/bucket_policy_test.go @@ -618,7 +618,7 @@ data "aws_iam_policy_document" "test" { aws_iam_role.test4.arn, aws_iam_role.test1.arn, aws_iam_role.test5.arn, - data.aws_caller_identity.current.arn, + data.aws_caller_identity.current.arn, ] type = "AWS" } From 0eb10f483ccf9ae0238ff3453dfc33f223ea4582 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Dec 2021 20:25:55 -0500 Subject: [PATCH 8/8] Add comment for issue --- internal/service/s3/bucket_policy_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/service/s3/bucket_policy_test.go b/internal/service/s3/bucket_policy_test.go index 8ef272c50b2..5a5d80f5262 100644 --- a/internal/service/s3/bucket_policy_test.go +++ b/internal/service/s3/bucket_policy_test.go @@ -167,6 +167,7 @@ func TestAccS3BucketPolicy_IAMRoleOrder_policyDoc(t *testing.T) { } // Reference: https://github.com/hashicorp/terraform-provider-aws/issues/13144 +// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/20456 func TestAccS3BucketPolicy_IAMRoleOrder_policyDocNotPrincipal(t *testing.T) { rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) resourceName := "aws_s3_bucket.test"