fix: refine deny destination checks (#20045)

* 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 <[email protected]> * fix: also assert that server matches on ns deny Signed-off-by: Blake Pettersson <[email protected]> --------- Signed-off-by: Blake Pettersson <[email protected]>
argoproj · Oct 3, 2024 · be880ad · be880ad
1 parent 86519ca
commit be880ad
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 6 deletions.
diff --git 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) && dstServerMatched {
+			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
@@ -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,22 @@ 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,
+	}, {
+		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 {
@@ -350,7 +366,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)
 	}
 }