Ignore principals order when comparing policies

pkg/clients/iam/role_test.go

@@ -2,6 +2,8 @@ package iam
 
 import (
 	"net/url"
+	"regexp"
+	"strings"
 	"testing"
 	"time"
 
@@ -65,10 +67,6 @@ var (
 	permissionBoundary = "arn:aws:iam::111111111111:policy/permission-boundary"
 	createDate         = time.Now()
 	region             = "us-east-1"
-	// There are flaky failures when \s+ is used in line matchers to match diff lines.
-	// Instead, this regex collapses all whitespaces into a single space,
-	// and line matchers use single space.
-	// compactSpaceRegex = regexp.MustCompile(`\s+`)
 )
 
 func roleParams(m ...func(*v1beta1.RoleParameters)) *v1beta1.RoleParameters {
@@ -262,8 +260,9 @@ func TestIsRoleUpToDate(t *testing.T) {
 	}
 
 	cases := map[string]struct {
-		args args
-		want bool
+		args     args
+		want     bool
+		wantDiff []*regexp.Regexp
 	}{
 		"SameFields": {
 			args: args{
@@ -366,6 +365,11 @@ func TestIsRoleUpToDate(t *testing.T) {
 				},
 			},
 			want: false,
+			wantDiff: []*regexp.Regexp{
+				regexp.MustCompile("Found observed difference in IAM role"),
+				regexp.MustCompile(`- AssumeRolePolicyDocument: &"(%\w\w)+Statement`),
+				regexp.MustCompile(`\+ AssumeRolePolicyDocument: &"(%\w\w)+Version`),
+			},
 		},
 		"DifferentFields": {
 			args: args{
@@ -391,6 +395,11 @@ func TestIsRoleUpToDate(t *testing.T) {
 				},
 			},
 			want: false,
+			wantDiff: []*regexp.Regexp{
+				regexp.MustCompile("Found observed difference in IAM role"),
+				regexp.MustCompile(`- Path: &"/"`),
+				regexp.MustCompile(`\+ Path: &"//"`),
+			},
 		},
 	}
 
@@ -401,7 +410,21 @@ func TestIsRoleUpToDate(t *testing.T) {
 				t.Errorf("r: unexpected error: %v", err)
 			}
 			if diff := cmp.Diff(tc.want, got); diff != "" {
-				t.Errorf("r: -want, +got:\n%s", testDiff)
+				t.Errorf("r: -want, +got:\n%s", diff)
+			}
+			if tc.wantDiff == nil {
+				if diff := cmp.Diff("", testDiff); diff != "" {
+					t.Errorf("r: -want, +got:\n%s", diff)
+				}
+			} else {
+				// cmp randomly uses either regular or non-breaking spaces.
+				// Replace them all with regular spaces.
+				compactDiff := strings.Join(strings.Fields(testDiff), " ")
+				for _, wantDiff := range tc.wantDiff {
+					if !wantDiff.MatchString(compactDiff) {
+						t.Errorf("expected:\n%s\nto match:\n%s", testDiff, wantDiff.String())
+					}
+				}
 			}
 		})
 	}

pkg/utils/policy/compare_test.go

@@ -19,6 +19,15 @@ var (
 
 	//go:embed testdata/Issue1892_b_min.json
 	policyIssue1892bMin string
+
+	//go:embed testdata/PrincipalsOrder_a.json
+	policyPrincipalsOrderA string
+
+	//go:embed testdata/PrincipalsOrder_b.json
+	policyPrincipalsOrderB string
+
+	//go:embed testdata/PrincipalsOrder_c.json
+	policyPrincipalsOrderC string
 )
 
 func TestCompareRawPolicies(t *testing.T) {
@@ -51,6 +60,24 @@ func TestCompareRawPolicies(t *testing.T) {
 				equals: true,
 			},
 		},
+		"PrincipalsOrder Equal": {
+			args: args{
+				policyA: policyPrincipalsOrderA,
+				policyB: policyPrincipalsOrderB,
+			},
+			want: want{
+				equals: true,
+			},
+		},
+		"PrincipalsOrder NotEqual": {
+			args: args{
+				policyA: policyPrincipalsOrderA,
+				policyB: policyPrincipalsOrderC,
+			},
+			want: want{
+				equals: false,
+			},
+		},
 	}
 
 	for name, tc := range cases {

pkg/utils/policy/convert.go

@@ -82,16 +82,16 @@ func convertResourcePolicyPrincipal(p *common.ResourcePrincipal) *Principal {
 	res := Principal{
 		AllowAnon: p.AllowAnon,
 		Federated: p.Federated,
-		Service:   p.Service,
+		Service:   NewStringOrSet(p.Service...),
 	}
 	for _, pr := range p.AWSPrincipals {
 		switch {
 		case pr.AWSAccountID != nil:
-			res.AWSPrincipals = append(res.AWSPrincipals, *pr.AWSAccountID)
+			res.AWSPrincipals = res.AWSPrincipals.Add(*pr.AWSAccountID)
 		case pr.IAMRoleARN != nil:
-			res.AWSPrincipals = append(res.AWSPrincipals, *pr.IAMRoleARN)
+			res.AWSPrincipals = res.AWSPrincipals.Add(*pr.IAMRoleARN)
 		case pr.UserARN != nil:
-			res.AWSPrincipals = append(res.AWSPrincipals, *pr.UserARN)
+			res.AWSPrincipals = res.AWSPrincipals.Add(*pr.UserARN)
 		}
 	}
 	return &res

pkg/utils/policy/parse_test.go

@@ -39,13 +39,13 @@ func TestParsePolicy(t *testing.T) {
 						{
 							SID: ptr.To("AllowPutObjectS3ServerAccessLogsPolicy"),
 							Principal: &Principal{
-								Service: StringOrArray{
+								Service: NewStringOrSet(
 									"logging.s3.amazonaws.com",
-								},
+								),
 								Federated: ptr.To("cognito-identity.amazonaws.com"),
-								AWSPrincipals: StringOrArray{
+								AWSPrincipals: NewStringOrSet(
 									"123456789012",
-								},
+								),
 							},
 							Effect: "Allow",
 							Action: StringOrArray{
@@ -78,15 +78,15 @@ func TestParsePolicy(t *testing.T) {
 						{
 							SID: ptr.To("AllowPutObjectS3ServerAccessLogsPolicy"),
 							Principal: &Principal{
-								Service: StringOrArray{
+								Service: NewStringOrSet(
 									"logging.s3.amazonaws.com",
 									"s3.amazonaws.com",
-								},
+								),
 								Federated: ptr.To("cognito-identity.amazonaws.com"),
-								AWSPrincipals: StringOrArray{
+								AWSPrincipals: NewStringOrSet(
 									"123456789012",
 									"452356421222",
-								},
+								),
 							},
 							Effect: "Allow",
 							Action: StringOrArray{

pkg/utils/policy/testdata/PrincipalsOrder_a.json

@@ -0,0 +1,27 @@
+{
+  "Version": "2012-10-17",
+  "Statement": [{
+    "Effect": "Allow",
+    "Principal": {
+      "Service": [
+        "eks.amazonaws.com",
+        "sqs.amazonaws.com"
+      ],
+      "AWS": [
+        "arn:aws:iam::123456789012:bbb",
+        "arn:aws:iam::123456789012:aaa"
+      ]
+    },
+    "NotPrincipal": {
+      "Service": [
+        "s3.amazonaws.com",
+        "ec2.amazonaws.com"
+      ],
+      "AWS": [
+        "arn:aws:iam::123456789012:ddd",
+        "arn:aws:iam::123456789012:ccc"
+      ]
+    },
+    "Action": ["sts:AssumeRole"]
+  }]
+}

pkg/utils/policy/testdata/PrincipalsOrder_b.json

@@ -0,0 +1,27 @@
+{
+  "Version": "2012-10-17",
+  "Statement": [{
+    "Effect": "Allow",
+    "Principal": {
+      "Service": [
+        "sqs.amazonaws.com",
+        "eks.amazonaws.com"
+      ],
+      "AWS": [
+        "arn:aws:iam::123456789012:aaa",
+        "arn:aws:iam::123456789012:bbb"
+      ]
+    },
+    "NotPrincipal": {
+      "Service": [
+        "ec2.amazonaws.com",
+        "s3.amazonaws.com"
+      ],
+      "AWS": [
+        "arn:aws:iam::123456789012:ccc",
+        "arn:aws:iam::123456789012:ddd"
+      ]
+    },
+    "Action": ["sts:AssumeRole"]
+  }]
+}

pkg/utils/policy/testdata/PrincipalsOrder_c.json

@@ -0,0 +1,28 @@
+{
+  "Version": "2012-10-17",
+  "Statement": [{
+    "Effect": "Allow",
+    "Principal": {
+      "Service": [
+        "sqs.amazonaws.com",
+        "eks.amazonaws.com"
+      ],
+      "AWS": [
+        "arn:aws:iam::123456789012:aaa",
+        "arn:aws:iam::123456789012:bbb"
+      ]
+    },
+    "NotPrincipal": {
+      "Service": [
+        "ec2.amazonaws.com",
+        "s3.amazonaws.com"
+      ],
+      "AWS": [
+        "arn:aws:iam::123456789012:ccc",
+        "arn:aws:iam::123456789012:eee",
+        "arn:aws:iam::123456789012:ddd"
+      ]
+    },
+    "Action": ["sts:AssumeRole"]
+  }]
+}

pkg/utils/policy/types.go

@@ -2,6 +2,7 @@ package policy
 
 import (
 	"encoding/json"
+	"sort"
 	"strconv"
 )
 
@@ -98,7 +99,7 @@ type Principal struct {
 	// This list contains the all of the AWS IAM users which are affected
 	// by the policy statement.
 	// +optional
-	AWSPrincipals StringOrArray `json:"AWS,omitempty"`
+	AWSPrincipals StringOrSet `json:"AWS,omitempty"`
 
 	// This string contains the identifier for any federated web identity
 	// provider.
@@ -107,7 +108,7 @@ type Principal struct {
 
 	// Service define the services which can have access to this bucket
 	// +optional
-	Service StringOrArray `json:"Service,omitempty"`
+	Service StringOrSet `json:"Service,omitempty"`
 }
 
 type unmarshalPrinciple Principal
@@ -207,3 +208,53 @@ func (s *StringOrArray) UnmarshalJSON(data []byte) error {
 	*s = list
 	return nil
 }
+
+// StringOrSet is a string array that supports parsing from a single string
+// as a single entry array. Order of elements is not respected when comparing
+// two StringOrSet objects.
+type StringOrSet map[string]struct{}
+
+func NewStringOrSet(values ...string) StringOrSet {
+	set := make(StringOrSet, len(values))
+	for _, s := range values {
+		set[s] = struct{}{}
+	}
+	return set
+}
+
+// Add adds a value to the set.
+func (s StringOrSet) Add(value string) StringOrSet {
+	if s == nil {
+		s = make(StringOrSet, 1)
+	}
+	s[value] = struct{}{}
+	return s
+}
+
+// UnmarshalJSON unmarshals data into s.
+func (s *StringOrSet) UnmarshalJSON(data []byte) error {
+	var single string
+	if err := json.Unmarshal(data, &single); err == nil {
+		*s = NewStringOrSet(single)
+		return nil
+	}
+	list := []string{}
+	if err := json.Unmarshal(data, &list); err != nil {
+		return err
+	}
+	*s = NewStringOrSet(list...)
+	return nil
+}
+
+// MarshalJSON marshals StringOrSet as an array.
+func (s *StringOrSet) MarshalJSON() ([]byte, error) {
+	var array []string
+	if s != nil && *s != nil {
+		array = make([]string, 0, len(*s))
+		for k := range *s {
+			array = append(array, k)
+		}
+		sort.Strings(array)
+	}
+	return json.Marshal(array)
+}