From b234d0fa89322785768ce702e687d02fc22af3aa Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Sat, 21 Sep 2024 12:48:19 +0200 Subject: [PATCH 1/2] fix: refine server deny check Fixes #19804. The deny destination check can be made more intuitive by doing the following: * short-circuit any deny destination * first, for any deny server destination, _also_ check if the namespace matches * for any deny namespace destination, reject as before Signed-off-by: Blake Pettersson --- pkg/apis/application/v1alpha1/app_project_types.go | 9 +++++---- pkg/apis/application/v1alpha1/types_test.go | 12 ++++++++++-- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/pkg/apis/application/v1alpha1/app_project_types.go b/pkg/apis/application/v1alpha1/app_project_types.go index b888cd37391b3..37723c8431121 100644 --- a/pkg/apis/application/v1alpha1/app_project_types.go +++ b/pkg/apis/application/v1alpha1/app_project_types.go @@ -461,7 +461,6 @@ func (proj AppProject) IsDestinationPermitted(dst ApplicationDestination, projec func (proj AppProject) isDestinationMatched(dst ApplicationDestination) bool { anyDestinationMatched := false - noDenyDestinationsMatched := true for _, item := range proj.Spec.Destinations { dstNameMatched := dst.Name != "" && globMatch(item.Name, dst.Name, true) @@ -471,12 +470,14 @@ func (proj AppProject) isDestinationMatched(dst ApplicationDestination) bool { matched := (dstServerMatched || dstNameMatched) && dstNamespaceMatched if matched { anyDestinationMatched = true - } else if ((!dstNameMatched && isDenyPattern(item.Name)) || (!dstServerMatched && isDenyPattern(item.Server))) || (!dstNamespaceMatched && isDenyPattern(item.Namespace)) { - noDenyDestinationsMatched = false + } else if (!dstNameMatched && isDenyPattern(item.Name)) || (!dstServerMatched && isDenyPattern(item.Server)) && dstNamespaceMatched { + return false + } else if !dstNamespaceMatched && isDenyPattern(item.Namespace) { + return false } } - return anyDestinationMatched && noDenyDestinationsMatched + return anyDestinationMatched } func isDenyPattern(pattern string) bool { diff --git a/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go index 08b83c238a93d..17213205e40db 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/pkg/apis/application/v1alpha1/types_test.go @@ -320,7 +320,7 @@ func TestAppProject_IsNegatedDestinationPermitted(t *testing.T) { Server: "*", Namespace: "!kube-system", }}, appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "default"}, - isPermitted: false, + isPermitted: true, }, { projDest: []ApplicationDestination{{ Server: "*", Namespace: "*", @@ -339,6 +339,14 @@ func TestAppProject_IsNegatedDestinationPermitted(t *testing.T) { }}, appDest: ApplicationDestination{Name: "test", Namespace: "test"}, isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "*", Namespace: "test", + }, { + Server: "!https://test-server", Namespace: "other", + }}, + appDest: ApplicationDestination{Server: "https://test-server", Namespace: "test"}, + isPermitted: true, }} for _, data := range testData { @@ -350,7 +358,7 @@ func TestAppProject_IsNegatedDestinationPermitted(t *testing.T) { permitted, _ := proj.IsDestinationPermitted(data.appDest, func(project string) ([]*Cluster, error) { return []*Cluster{}, nil }) - assert.Equal(t, data.isPermitted, permitted) + assert.Equalf(t, data.isPermitted, permitted, "appDest mismatch for %+v with project destinations %+v", data.appDest, data.projDest) } } From c00042cb6b86ca2a04c9aac3e58f8e340141ff1a Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Sat, 21 Sep 2024 16:42:07 +0200 Subject: [PATCH 2/2] fix: also assert that server matches on ns deny Signed-off-by: Blake Pettersson --- pkg/apis/application/v1alpha1/app_project_types.go | 2 +- pkg/apis/application/v1alpha1/types_test.go | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/pkg/apis/application/v1alpha1/app_project_types.go b/pkg/apis/application/v1alpha1/app_project_types.go index 37723c8431121..d4fc39723358a 100644 --- a/pkg/apis/application/v1alpha1/app_project_types.go +++ b/pkg/apis/application/v1alpha1/app_project_types.go @@ -472,7 +472,7 @@ func (proj AppProject) isDestinationMatched(dst ApplicationDestination) bool { anyDestinationMatched = true } else if (!dstNameMatched && isDenyPattern(item.Name)) || (!dstServerMatched && isDenyPattern(item.Server)) && dstNamespaceMatched { return false - } else if !dstNamespaceMatched && isDenyPattern(item.Namespace) { + } else if !dstNamespaceMatched && isDenyPattern(item.Namespace) && dstServerMatched { return false } } diff --git a/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go index 17213205e40db..6b5d6014041dd 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/pkg/apis/application/v1alpha1/types_test.go @@ -347,6 +347,14 @@ func TestAppProject_IsNegatedDestinationPermitted(t *testing.T) { }}, appDest: ApplicationDestination{Server: "https://test-server", Namespace: "test"}, isPermitted: true, + }, { + projDest: []ApplicationDestination{{ + Server: "*", Namespace: "*", + }, { + Server: "https://test-server", Namespace: "!other", + }}, + appDest: ApplicationDestination{Server: "https://other-test-server", Namespace: "other"}, + isPermitted: true, }} for _, data := range testData {