From e5811d747d5fa2faaf11e72f437130021ded3689 Mon Sep 17 00:00:00 2001 From: Pierre Souchay Date: Mon, 25 Jul 2022 15:57:41 +0200 Subject: [PATCH 1/5] fix(iam): the order of service principals should not matter As depicted here https://github.com/hashicorp/terraform-provider-aws/issues/9042#issuecomment-1132992037 It seems the order of services is not predictable, causing Terraform to think it has changes to perform This change avoid this by sorting the order always in the same predictable order --- internal/service/iam/policy_model.go | 1 + internal/service/iam/policy_model_test.go | 38 +++++++++++++++++++++-- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/internal/service/iam/policy_model.go b/internal/service/iam/policy_model.go index f120afc124c1..67f400ccdbf6 100644 --- a/internal/service/iam/policy_model.go +++ b/internal/service/iam/policy_model.go @@ -157,6 +157,7 @@ func (ps *IAMPolicyStatementPrincipalSet) UnmarshalJSON(b []byte) error { for _, v := range value.([]interface{}) { values = append(values, v.(string)) } + sort.Sort(sort.Reverse(sort.StringSlice(values))) out = append(out, IAMPolicyStatementPrincipal{Type: key, Identifiers: values}) default: return fmt.Errorf("Unsupported data type %T for IAMPolicyStatementPrincipalSet.Identifiers", vt) diff --git a/internal/service/iam/policy_model_test.go b/internal/service/iam/policy_model_test.go index 7b91f369fbd1..706fb9e226af 100644 --- a/internal/service/iam/policy_model_test.go +++ b/internal/service/iam/policy_model_test.go @@ -1,7 +1,6 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: MPL-2.0 - -package iam +package iam_test import ( "encoding/json" @@ -388,5 +387,40 @@ func TestIAMPolicyStatementConditionSet_MarshalJSON(t *testing.T) { // nosemgrep t.Errorf("IAMPolicyStatementConditionSet.MarshalJSON() = %v, want %v", string(got), string(tc.want)) } }) +) + +func TestUnMarshallOrderOfPrincipalsShouldNotMatter(t *testing.T) { + policy1 := ` + { + "Action": "sts:AssumeRole", + "Principal": { + "Service": ["lambda.amazonaws.com", "service2.amazonaws.com"] + }, + "Effect": "Allow", + "Sid": "" + }` + // Service order is different, but should be the same object for terraform + policy2 := ` + { + "Action": "sts:AssumeRole", + "Principal": { + "Service": ["service2.amazonaws.com", "lambda.amazonaws.com"] + }, + "Effect": "Allow", + "Sid": "" + }` + + var data1 tfiam.IAMPolicyStatement + var data2 tfiam.IAMPolicyStatement + err := json.Unmarshal([]byte(policy1), &data1) + if err != nil { + t.Fatal(err) + } + err = json.Unmarshal([]byte(policy2), &data2) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(data1, data2) { + t.Fatalf("should be equal, but was:\n%#v\nVS\n%#v\n", data1, data2) } } From f9e7c07b34f670f001ad4a5a5c73ee9c06792e99 Mon Sep 17 00:00:00 2001 From: Pierre Souchay Date: Tue, 16 Aug 2022 16:59:50 +0200 Subject: [PATCH 2/5] Added changelog entry --- .changelog/25967.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/25967.txt diff --git a/.changelog/25967.txt b/.changelog/25967.txt new file mode 100644 index 000000000000..f60bec9868d5 --- /dev/null +++ b/.changelog/25967.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +resources/iam_policy: Ensures that order to principals does not matter when comparing +``` From 802c65b3de26ce4158be6a150ba5f38f9aa3180c Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Fri, 12 Apr 2024 16:36:45 -0400 Subject: [PATCH 3/5] iam/policy_document: Sort principals --- .changelog/25967.txt | 4 +-- internal/service/iam/policy_model.go | 10 +++--- internal/service/iam/policy_model_test.go | 41 ++++++++++++----------- 3 files changed, 29 insertions(+), 26 deletions(-) diff --git a/.changelog/25967.txt b/.changelog/25967.txt index f60bec9868d5..bf6c1e4c6e70 100644 --- a/.changelog/25967.txt +++ b/.changelog/25967.txt @@ -1,3 +1,3 @@ -```release-note:enhancement -resources/iam_policy: Ensures that order to principals does not matter when comparing +```release-note:bug +data-source/iam_policy_document: When using multiple principals, sort them to avoid differences based only on order ``` diff --git a/internal/service/iam/policy_model.go b/internal/service/iam/policy_model.go index 67f400ccdbf6..0e5f6812a03b 100644 --- a/internal/service/iam/policy_model.go +++ b/internal/service/iam/policy_model.go @@ -157,7 +157,7 @@ func (ps *IAMPolicyStatementPrincipalSet) UnmarshalJSON(b []byte) error { for _, v := range value.([]interface{}) { values = append(values, v.(string)) } - sort.Sort(sort.Reverse(sort.StringSlice(values))) + sort.Strings(values) out = append(out, IAMPolicyStatementPrincipal{Type: key, Identifiers: values}) default: return fmt.Errorf("Unsupported data type %T for IAMPolicyStatementPrincipalSet.Identifiers", vt) @@ -270,12 +270,12 @@ func PolicyHasValidAWSPrincipals(policy string) (bool, error) { // nosemgrep:ci. for _, principal := range principals { switch x := principal.(type) { case string: - if !isValidPolicyAWSPrincipal(x) { + if !IsValidPolicyAWSPrincipal(x) { return false, nil } case []string: for _, s := range x { - if !isValidPolicyAWSPrincipal(s) { + if !IsValidPolicyAWSPrincipal(s) { return false, nil } } @@ -285,9 +285,9 @@ func PolicyHasValidAWSPrincipals(policy string) (bool, error) { // nosemgrep:ci. return true, nil } -// isValidPolicyAWSPrincipal returns true if a string is a valid AWS Princial for an IAM Policy document +// IsValidPolicyAWSPrincipal returns true if a string is a valid AWS Princial for an IAM Policy document // That is: either an ARN, an AWS account ID, or `*` -func isValidPolicyAWSPrincipal(principal string) bool { // nosemgrep:ci.aws-in-func-name +func IsValidPolicyAWSPrincipal(principal string) bool { // nosemgrep:ci.aws-in-func-name if principal == "*" { return true } diff --git a/internal/service/iam/policy_model_test.go b/internal/service/iam/policy_model_test.go index 706fb9e226af..60a9a18e6112 100644 --- a/internal/service/iam/policy_model_test.go +++ b/internal/service/iam/policy_model_test.go @@ -1,5 +1,6 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: MPL-2.0 + package iam_test import ( @@ -8,6 +9,7 @@ import ( "testing" "github.com/hashicorp/terraform-provider-aws/internal/errs" + tfiam "github.com/hashicorp/terraform-provider-aws/internal/service/iam" ) func TestPolicyHasValidAWSPrincipals(t *testing.T) { // nosemgrep:ci.aws-in-func-name @@ -207,7 +209,7 @@ func TestPolicyHasValidAWSPrincipals(t *testing.T) { // nosemgrep:ci.aws-in-func t.Run(name, func(t *testing.T) { t.Parallel() - valid, err := PolicyHasValidAWSPrincipals(testcase.json) + valid, err := tfiam.PolicyHasValidAWSPrincipals(testcase.json) if testcase.err == nil { if err != nil { @@ -257,7 +259,7 @@ func TestIsValidAWSPrincipal(t *testing.T) { // nosemgrep:ci.aws-in-func-name t.Run(name, func(t *testing.T) { t.Parallel() - a := isValidPolicyAWSPrincipal(testcase.value) + a := tfiam.IsValidPolicyAWSPrincipal(testcase.value) if e := testcase.valid; a != e { t.Fatalf("expected %t, got %t", e, a) @@ -270,45 +272,45 @@ func TestIAMPolicyStatementConditionSet_MarshalJSON(t *testing.T) { // nosemgrep t.Parallel() testcases := map[string]struct { - cs IAMPolicyStatementConditionSet + cs tfiam.IAMPolicyStatementConditionSet want []byte wantErr bool }{ "invalid value type": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "StringLike", Variable: "s3:prefix", Values: 1}, }, wantErr: true, }, "single condition single value": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "StringLike", Variable: "s3:prefix", Values: "one/"}, }, want: []byte(`{"StringLike":{"s3:prefix":"one/"}}`), }, "single condition multiple values": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}}, }, want: []byte(`{"StringLike":{"s3:prefix":["one/","two/"]}}`), }, // Multiple distinct conditions "multiple condition single value": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "ArnNotLike", Variable: "aws:PrincipalArn", Values: "1"}, {Test: "StringLike", Variable: "s3:prefix", Values: "one/"}, }, want: []byte(`{"ArnNotLike":{"aws:PrincipalArn":"1"},"StringLike":{"s3:prefix":"one/"}}`), }, "multiple condition multiple values": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "ArnNotLike", Variable: "aws:PrincipalArn", Values: []string{"1", "2"}}, {Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}}, }, want: []byte(`{"ArnNotLike":{"aws:PrincipalArn":["1","2"]},"StringLike":{"s3:prefix":["one/","two/"]}}`), }, "multiple condition mixed value lengths": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "ArnNotLike", Variable: "aws:PrincipalArn", Values: "1"}, {Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}}, }, @@ -316,28 +318,28 @@ func TestIAMPolicyStatementConditionSet_MarshalJSON(t *testing.T) { // nosemgrep }, // Multiple conditions with duplicated `test` arguments "duplicate condition test single value": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "StringLike", Variable: "s3:prefix", Values: "one/"}, {Test: "StringLike", Variable: "s3:versionid", Values: "abc123"}, }, want: []byte(`{"StringLike":{"s3:prefix":"one/","s3:versionid":"abc123"}}`), }, "duplicate condition test multiple values": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}}, {Test: "StringLike", Variable: "s3:versionid", Values: []string{"abc123", "def456"}}, }, want: []byte(`{"StringLike":{"s3:prefix":["one/","two/"],"s3:versionid":["abc123","def456"]}}`), }, "duplicate condition test mixed value lengths": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "StringLike", Variable: "s3:prefix", Values: "one/"}, {Test: "StringLike", Variable: "s3:versionid", Values: []string{"abc123", "def456"}}, }, want: []byte(`{"StringLike":{"s3:prefix":"one/","s3:versionid":["abc123","def456"]}}`), }, "duplicate condition test mixed value lengths reversed": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}}, {Test: "StringLike", Variable: "s3:versionid", Values: "abc123"}, }, @@ -345,28 +347,28 @@ func TestIAMPolicyStatementConditionSet_MarshalJSON(t *testing.T) { // nosemgrep }, // Multiple conditions with duplicated `test` and `variable` arguments "duplicate condition test and variable single value": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "StringLike", Variable: "s3:prefix", Values: "one/"}, {Test: "StringLike", Variable: "s3:prefix", Values: "two/"}, }, want: []byte(`{"StringLike":{"s3:prefix":["one/","two/"]}}`), }, "duplicate condition test and variable multiple values": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}}, {Test: "StringLike", Variable: "s3:prefix", Values: []string{"three/", "four/"}}, }, want: []byte(`{"StringLike":{"s3:prefix":["one/","two/","three/","four/"]}}`), }, "duplicate condition test and variable mixed value lengths": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "StringLike", Variable: "s3:prefix", Values: "one/"}, {Test: "StringLike", Variable: "s3:prefix", Values: []string{"three/", "four/"}}, }, want: []byte(`{"StringLike":{"s3:prefix":["one/","three/","four/"]}}`), }, "duplicate condition test and variable mixed value lengths reversed": { - cs: IAMPolicyStatementConditionSet{ + cs: tfiam.IAMPolicyStatementConditionSet{ {Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}}, {Test: "StringLike", Variable: "s3:prefix", Values: "three/"}, }, @@ -387,9 +389,10 @@ func TestIAMPolicyStatementConditionSet_MarshalJSON(t *testing.T) { // nosemgrep t.Errorf("IAMPolicyStatementConditionSet.MarshalJSON() = %v, want %v", string(got), string(tc.want)) } }) -) + } +} -func TestUnMarshallOrderOfPrincipalsShouldNotMatter(t *testing.T) { +func TestPolicyUnmarshalServicePrincipalOrder(t *testing.T) { policy1 := ` { "Action": "sts:AssumeRole", From b1dcf0057f16a5cd5a666a5fd6419177aec1427f Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Fri, 12 Apr 2024 17:53:38 -0400 Subject: [PATCH 4/5] iam/test: Parallel --- internal/service/iam/policy_model_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/service/iam/policy_model_test.go b/internal/service/iam/policy_model_test.go index 60a9a18e6112..fc1b7f63f1a1 100644 --- a/internal/service/iam/policy_model_test.go +++ b/internal/service/iam/policy_model_test.go @@ -393,6 +393,8 @@ func TestIAMPolicyStatementConditionSet_MarshalJSON(t *testing.T) { // nosemgrep } func TestPolicyUnmarshalServicePrincipalOrder(t *testing.T) { + t.Parallel() + policy1 := ` { "Action": "sts:AssumeRole", From 9cd53274acd0e621588754ba5e19269604736475 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Fri, 12 Apr 2024 18:03:58 -0400 Subject: [PATCH 5/5] Update changelog --- .changelog/25967.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/25967.txt b/.changelog/25967.txt index bf6c1e4c6e70..987375901c0b 100644 --- a/.changelog/25967.txt +++ b/.changelog/25967.txt @@ -1,3 +1,3 @@ ```release-note:bug -data-source/iam_policy_document: When using multiple principals, sort them to avoid differences based only on order +data-source/aws_iam_policy_document: When using multiple principals, sort them to avoid differences based only on order ```