From a74d5d2b53bb5392c4a46711d3852639b409481f Mon Sep 17 00:00:00 2001 From: Darren Shepherd Date: Fri, 11 Aug 2023 23:47:47 -0700 Subject: [PATCH] Make checkSARs idempotent The previous logic would return randomly sorted results. Signed-off-by: Darren Shepherd --- pkg/apis/internal.acorn.io/v1/appspec.go | 22 ++++++++++---- pkg/apis/internal.acorn.io/v1/appspec_test.go | 29 +++++++++++++++++++ .../apigroups/acorn/apps/validator.go | 28 +++++++++++++----- 3 files changed, 66 insertions(+), 13 deletions(-) diff --git a/pkg/apis/internal.acorn.io/v1/appspec.go b/pkg/apis/internal.acorn.io/v1/appspec.go index 1e64b07c8..0ea4d5dab 100644 --- a/pkg/apis/internal.acorn.io/v1/appspec.go +++ b/pkg/apis/internal.acorn.io/v1/appspec.go @@ -509,6 +509,16 @@ func sortEquals(left, right []string) bool { return slices.Equal(left, right) } +func combineSlice(left, right []string) (result []string) { + result = append(result, left...) + for _, x := range right { + if !slices.Contains(left, x) { + result = append(result, x) + } + } + return +} + func canCombine(left, right PolicyRule) (combined PolicyRule, ok bool) { var ( singleDiff bool @@ -519,7 +529,7 @@ func canCombine(left, right PolicyRule) (combined PolicyRule, ok bool) { if singleDiff { return combined, false } - combined.Verbs = append(combined.Verbs, right.Verbs...) + combined.Verbs = combineSlice(combined.Verbs, right.Verbs) singleDiff = true } @@ -527,7 +537,7 @@ func canCombine(left, right PolicyRule) (combined PolicyRule, ok bool) { if singleDiff { return combined, false } - combined.APIGroups = append(combined.APIGroups, right.APIGroups...) + combined.APIGroups = combineSlice(combined.APIGroups, right.APIGroups) singleDiff = true } @@ -535,7 +545,7 @@ func canCombine(left, right PolicyRule) (combined PolicyRule, ok bool) { if singleDiff { return combined, false } - combined.Resources = append(combined.Resources, right.Resources...) + combined.Resources = combineSlice(combined.Resources, right.Resources) singleDiff = true } @@ -543,7 +553,7 @@ func canCombine(left, right PolicyRule) (combined PolicyRule, ok bool) { if singleDiff { return combined, false } - combined.ResourceNames = append(combined.ResourceNames, right.ResourceNames...) + combined.ResourceNames = combineSlice(combined.ResourceNames, right.ResourceNames) singleDiff = true } @@ -551,7 +561,7 @@ func canCombine(left, right PolicyRule) (combined PolicyRule, ok bool) { if singleDiff { return combined, false } - combined.NonResourceURLs = append(combined.NonResourceURLs, right.NonResourceURLs...) + combined.NonResourceURLs = combineSlice(combined.NonResourceURLs, right.NonResourceURLs) singleDiff = true } @@ -559,7 +569,7 @@ func canCombine(left, right PolicyRule) (combined PolicyRule, ok bool) { if singleDiff { return combined, false } - combined.Scopes = append(combined.Scopes, right.Scopes...) + combined.Scopes = combineSlice(combined.Scopes, right.Scopes) singleDiff = true } diff --git a/pkg/apis/internal.acorn.io/v1/appspec_test.go b/pkg/apis/internal.acorn.io/v1/appspec_test.go index ced56500b..1a5a03f31 100644 --- a/pkg/apis/internal.acorn.io/v1/appspec_test.go +++ b/pkg/apis/internal.acorn.io/v1/appspec_test.go @@ -57,6 +57,35 @@ func TestSimplify(t *testing.T) { }, })) + autogold.Expect(Permissions{Rules: []PolicyRule{ + {PolicyRule: rbacv1.PolicyRule{ + Verbs: []string{"a", "b"}, + Resources: []string{"x", "y", "z"}, + }}, + }}).Equal(t, Simplify(Permissions{ + Rules: []PolicyRule{ + { + PolicyRule: rbacv1.PolicyRule{ + Verbs: []string{"a"}, + Resources: []string{"x", "y"}, + }, + }, + { + PolicyRule: rbacv1.PolicyRule{ + Verbs: []string{"b"}, + Resources: []string{"x", "y"}, + }, + }, + + { + PolicyRule: rbacv1.PolicyRule{ + Verbs: []string{"a", "b"}, + Resources: []string{"x", "y", "z"}, + }, + }, + }, + })) + autogold.Expect(Permissions{Rules: []PolicyRule{ {PolicyRule: rbacv1.PolicyRule{ Verbs: []string{"a", "b"}, diff --git a/pkg/server/registry/apigroups/acorn/apps/validator.go b/pkg/server/registry/apigroups/acorn/apps/validator.go index 054a7750c..ec43ab834 100644 --- a/pkg/server/registry/apigroups/acorn/apps/validator.go +++ b/pkg/server/registry/apigroups/acorn/apps/validator.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "sort" "strings" "sync" "time" @@ -299,17 +300,29 @@ func (s *Validator) checkRemoteAccess(ctx context.Context, namespace, image stri return nil } +func sarToPolicyRules(sars []sarRequest) []v1.PolicyRule { + result := make([]v1.PolicyRule, 0, len(sars)) + sort.Slice(sars, func(i, j int) bool { + return sars[i].Order < sars[j].Order + }) + for _, sar := range sars { + result = append(result, sar.Rule) + } + return result +} + func (s *Validator) checkSARs(ctx context.Context, sars []sarRequest) (granted, rejected []v1.Permissions, _ error) { var ( lock sync.Mutex - grantedMap = map[string][]v1.PolicyRule{} - rejectedMap = map[string][]v1.PolicyRule{} + grantedMap = map[string][]sarRequest{} + rejectedMap = map[string][]sarRequest{} ) eg, ctx := errgroup.WithContext(ctx) eg.SetLimit(5) - for _, sar := range sars { + for i, sar := range sars { sar := sar + sar.Order = i eg.Go(func() error { ok, err := s.check(ctx, &sar.SAR) if err != nil { @@ -318,9 +331,9 @@ func (s *Validator) checkSARs(ctx context.Context, sars []sarRequest) (granted, lock.Lock() defer lock.Unlock() if ok { - grantedMap[sar.ServiceName] = append(grantedMap[sar.ServiceName], sar.Rule) + grantedMap[sar.ServiceName] = append(grantedMap[sar.ServiceName], sar) } else { - rejectedMap[sar.ServiceName] = append(rejectedMap[sar.ServiceName], sar.Rule) + rejectedMap[sar.ServiceName] = append(rejectedMap[sar.ServiceName], sar) } return nil }) @@ -333,14 +346,14 @@ func (s *Validator) checkSARs(ctx context.Context, sars []sarRequest) (granted, for _, key := range typed.SortedKeys(grantedMap) { granted = append(granted, v1.Permissions{ ServiceName: key, - Rules: grantedMap[key], + Rules: sarToPolicyRules(grantedMap[key]), }) } for _, key := range typed.SortedKeys(rejectedMap) { rejected = append(rejected, v1.Permissions{ ServiceName: key, - Rules: rejectedMap[key], + Rules: sarToPolicyRules(rejectedMap[key]), }) } @@ -359,6 +372,7 @@ type sarRequest struct { SAR authv1.SubjectAccessReview ServiceName string Rule v1.PolicyRule + Order int } func (s *Validator) getSARNonResourceRole(sar *authv1.SubjectAccessReview, serviceName string, rule v1.PolicyRule) (result []sarRequest, _ error) {