From 626023f291247ffe75f114ea7e332fc0b2806e34 Mon Sep 17 00:00:00 2001 From: Max Melentyev Date: Tue, 7 May 2024 19:10:55 -0400 Subject: [PATCH] fix: Ignore principals order when comparing policies AWS returns items in a random order so provider-aws should ignore it to not consider policies changed. Signed-off-by: Max Melentyev --- pkg/clients/iam/role_test.go | 37 +++++++-- pkg/utils/policy/compare_test.go | 27 +++++++ pkg/utils/policy/convert.go | 8 +- pkg/utils/policy/parse_test.go | 16 ++-- .../policy/testdata/PrincipalsOrder_a.json | 27 +++++++ .../policy/testdata/PrincipalsOrder_b.json | 27 +++++++ .../policy/testdata/PrincipalsOrder_c.json | 28 +++++++ pkg/utils/policy/types.go | 55 ++++++++++++- pkg/utils/policy/types_test.go | 79 +++++++++++++++++++ 9 files changed, 283 insertions(+), 21 deletions(-) create mode 100644 pkg/utils/policy/testdata/PrincipalsOrder_a.json create mode 100644 pkg/utils/policy/testdata/PrincipalsOrder_b.json create mode 100644 pkg/utils/policy/testdata/PrincipalsOrder_c.json create mode 100644 pkg/utils/policy/types_test.go diff --git a/pkg/clients/iam/role_test.go b/pkg/clients/iam/role_test.go index 11891f2802..a576275156 100644 --- a/pkg/clients/iam/role_test.go +++ b/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()) + } + } } }) } diff --git a/pkg/utils/policy/compare_test.go b/pkg/utils/policy/compare_test.go index af2d38fe83..3ddadef657 100644 --- a/pkg/utils/policy/compare_test.go +++ b/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 { diff --git a/pkg/utils/policy/convert.go b/pkg/utils/policy/convert.go index f71ddac4fa..54e0fa51c3 100644 --- a/pkg/utils/policy/convert.go +++ b/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 diff --git a/pkg/utils/policy/parse_test.go b/pkg/utils/policy/parse_test.go index 5ca5989079..88ea1b2498 100644 --- a/pkg/utils/policy/parse_test.go +++ b/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{ diff --git a/pkg/utils/policy/testdata/PrincipalsOrder_a.json b/pkg/utils/policy/testdata/PrincipalsOrder_a.json new file mode 100644 index 0000000000..c2460c2f5f --- /dev/null +++ b/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"] + }] +} diff --git a/pkg/utils/policy/testdata/PrincipalsOrder_b.json b/pkg/utils/policy/testdata/PrincipalsOrder_b.json new file mode 100644 index 0000000000..92f8dce9c8 --- /dev/null +++ b/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"] + }] +} diff --git a/pkg/utils/policy/testdata/PrincipalsOrder_c.json b/pkg/utils/policy/testdata/PrincipalsOrder_c.json new file mode 100644 index 0000000000..39787beffa --- /dev/null +++ b/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"] + }] +} diff --git a/pkg/utils/policy/types.go b/pkg/utils/policy/types.go index 943f40bdb8..5742fe8c8b 100644 --- a/pkg/utils/policy/types.go +++ b/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) +} diff --git a/pkg/utils/policy/types_test.go b/pkg/utils/policy/types_test.go new file mode 100644 index 0000000000..844014b09d --- /dev/null +++ b/pkg/utils/policy/types_test.go @@ -0,0 +1,79 @@ +package policy + +import ( + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestStringOrSet_Equal(t *testing.T) { + cases := map[string]struct { + a, b StringOrSet + equal bool + }{ + "BothNil": { + a: nil, + b: nil, + equal: true, + }, + "BothEmpty": { + a: NewStringOrSet(), + b: NewStringOrSet(), + equal: true, + }, + "EqualDifferentOrder": { + a: NewStringOrSet("a", "b"), + b: NewStringOrSet("b", "a"), + equal: true, + }, + "NotEqual": { + a: NewStringOrSet("a", "b"), + b: NewStringOrSet("a", "c"), + equal: false, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + if diff := cmp.Diff(tc.a, tc.b); (diff == "") != tc.equal { + t.Errorf("diff: %s", diff) + } + }) + } +} + +func TestStringOrSet_MarshalJSON(t *testing.T) { + cases := map[string]struct { + set StringOrSet + want string + }{ + "nil": { + set: nil, + want: "null", + }, + "Empty": { + set: NewStringOrSet(), + want: "[]", + }, + "Single": { + set: NewStringOrSet("a"), + want: `["a"]`, + }, + "Multiple": { + set: NewStringOrSet("a", "c", "b"), + want: `["a","b","c"]`, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + b, err := tc.set.MarshalJSON() + if err != nil { + t.Fatal(err) + } + if string(b) != tc.want { + t.Errorf("got %s, want %s", b, tc.want) + } + }) + } +}