Skip to content

Commit

Permalink
Merge pull request #25967 from pierresouchay/fix_order_of_principals_…
Browse files Browse the repository at this point in the history 
…does_not_matters

fix(iam): the order of service principals should not matter
  • Loading branch information
@YakDriver
YakDriver committed Apr 15, 2024
2 parents adfcd3c + 9cd5327 commit 91f4146
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 22 deletions.
3 changes: 3 additions & 0 deletions .changelog/25967.txt
View file
@@ -0,0 +1,3 @@
```release-note:bug
data-source/aws_iam_policy_document: When using multiple principals, sort them to avoid differences based only on order
```
9 changes: 5 additions & 4 deletions internal/service/iam/policy_model.go
View file
Expand Up @@ -157,6 +157,7 @@ func (ps *IAMPolicyStatementPrincipalSet) UnmarshalJSON(b []byte) error {
for _, v := range value.([]interface{}) {
values = append(values, v.(string))
}
sort.Strings(values)
out = append(out, IAMPolicyStatementPrincipal{Type: key, Identifiers: values})
default:
return fmt.Errorf("Unsupported data type %T for IAMPolicyStatementPrincipalSet.Identifiers", vt)
Expand Down Expand Up @@ -269,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
}
}
Expand All @@ -284,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
}
Expand Down
75 changes: 57 additions & 18 deletions internal/service/iam/policy_model_test.go
View file
@@ -1,14 +1,15 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package iam
package iam_test

import (
"encoding/json"
"reflect"
"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
Expand Down Expand Up @@ -208,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 {
Expand Down Expand Up @@ -258,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)
Expand All @@ -271,103 +272,103 @@ 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/"}},
},
want: []byte(`{"ArnNotLike":{"aws:PrincipalArn":"1"},"StringLike":{"s3:prefix":["one/","two/"]}}`),
},
// 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"},
},
want: []byte(`{"StringLike":{"s3:prefix":["one/","two/"],"s3:versionid":"abc123"}}`),
},
// 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/"},
},
Expand All @@ -390,3 +391,41 @@ func TestIAMPolicyStatementConditionSet_MarshalJSON(t *testing.T) { // nosemgrep
})
}
}

func TestPolicyUnmarshalServicePrincipalOrder(t *testing.T) {
t.Parallel()

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)
}
}

0 comments on commit 91f4146

Please sign in to comment.