Merge pull request #5 from crenshaw-dev/template-patch-changes

small changes
speedfl · Dec 1, 2023 · 47f5743 · 47f5743
2 parents 734cb5e + 27f5d1d
commit 47f5743
Show file tree

Hide file tree

Showing 5 changed files with 289 additions and 122 deletions.
diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go
@@ -568,10 +568,10 @@ func (r *ApplicationSetReconciler) applyTemplatePatch(app *argov1alpha1.Applicat
 	replacedTemplate, err := r.Renderer.Replace(*applicationSetInfo.Spec.TemplatePatch, params, applicationSetInfo.Spec.GoTemplate, applicationSetInfo.Spec.GoTemplateOptions)
 
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("error replacing values in templatePatch: %w", err)
 	}
 
-	return utils.ApplyPatchTemplate(app, replacedTemplate)
+	return applyTemplatePatch(app, replacedTemplate)
 }
 
 func ignoreNotAllowedNamespaces(namespaces []string) predicate.Predicate {
@@ -648,6 +648,8 @@ func (r *ApplicationSetReconciler) createOrUpdateInCluster(ctx context.Context,
 	var firstError error
 	// Creates or updates the application in appList
 	for _, generatedApp := range desiredApplications {
+		// The app's namespace must be the same as the AppSet's namespace to preserve the appsets-in-any-namespace
+		// security boundary.
 		generatedApp.Namespace = applicationSet.Namespace
 
 		appLog := logCtx.WithFields(log.Fields{"app": generatedApp.QualifiedName()})

diff --git a/applicationset/utils/templatePatch.go → applicationset/controllers/templatePatch.go b/applicationset/utils/templatePatch.go → applicationset/controllers/templatePatch.go
@@ -1,22 +1,23 @@
-package utils
+package controllers
 
 import (
 	"encoding/json"
 	"fmt"
 
 	"k8s.io/apimachinery/pkg/util/strategicpatch"
 
+	"github.com/argoproj/argo-cd/v2/applicationset/utils"
 	appv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 )
 
-func ApplyPatchTemplate(app *appv1.Application, templatePatch string) (*appv1.Application, error) {
+func applyTemplatePatch(app *appv1.Application, templatePatch string) (*appv1.Application, error) {
 
 	appString, err := json.Marshal(app)
 	if err != nil {
 		return nil, fmt.Errorf("error while marhsalling Application %w", err)
 	}
 
-	convertedTemplatePatch, err := ConvertYAMLToJSON(templatePatch)
+	convertedTemplatePatch, err := utils.ConvertYAMLToJSON(templatePatch)
 
 	if err != nil {
 		return nil, fmt.Errorf("error while converting template to json %q: %w", convertedTemplatePatch, err)
@@ -26,7 +27,7 @@ func ApplyPatchTemplate(app *appv1.Application, templatePatch string) (*appv1.Ap
 		return nil, fmt.Errorf("invalid templatePatch %q: %w", convertedTemplatePatch, err)
 	}
 
-	data, err := strategicpatch.StrategicMergePatch([]byte(appString), []byte(convertedTemplatePatch), appv1.Application{})
+	data, err := strategicpatch.StrategicMergePatch(appString, []byte(convertedTemplatePatch), appv1.Application{})
 
 	if err != nil {
 		return nil, fmt.Errorf("error while applying templatePatch template to json %q: %w", convertedTemplatePatch, err)
@@ -35,8 +36,11 @@ func ApplyPatchTemplate(app *appv1.Application, templatePatch string) (*appv1.Ap
 	finalApp := appv1.Application{}
 	err = json.Unmarshal(data, &finalApp)
 	if err != nil {
-		return nil, fmt.Errorf("error while unmarhsalling patched application %w", err)
+		return nil, fmt.Errorf("error while unmarhsalling patched application: %w", err)
 	}
 
-	return &finalApp, err
+	// Prevent changes to the `project` field. This helps prevent malicious template patches
+	finalApp.Spec.Project = app.Spec.Project
+
+	return &finalApp, nil
 }
diff --git a/applicationset/controllers/templatePatch_test.go b/applicationset/controllers/templatePatch_test.go
@@ -0,0 +1,249 @@
+package controllers
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	appv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+)
+
+func Test_ApplyTemplatePatch(t *testing.T) {
+	testCases := []struct {
+		name          string
+		appTemplate   *appv1.Application
+		templatePatch string
+		expectedApp   *appv1.Application
+	}{
+		{
+			name: "patch with JSON",
+			appTemplate: &appv1.Application{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Application",
+					APIVersion: "argoproj.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "my-cluster-guestbook",
+					Namespace:  "namespace",
+					Finalizers: []string{"resources-finalizer.argocd.argoproj.io"},
+				},
+				Spec: appv1.ApplicationSpec{
+					Project: "default",
+					Source: &appv1.ApplicationSource{
+						RepoURL:        "https://github.com/argoproj/argocd-example-apps.git",
+						TargetRevision: "HEAD",
+						Path:           "guestbook",
+					},
+					Destination: appv1.ApplicationDestination{
+						Server:    "https://kubernetes.default.svc",
+						Namespace: "guestbook",
+					},
+				},
+			},
+			templatePatch: `{
+				"metadata": {
+					"annotations": {
+						"annotation-some-key": "annotation-some-value"
+					}
+				},
+				"spec": {
+					"source": {
+						"helm": {
+							"valueFiles": [
+								"values.test.yaml",
+								"values.big.yaml"
+							]
+						}
+					},
+					"syncPolicy": {
+						"automated": {
+							"prune": true
+						}
+					}
+				}
+			}`,
+			expectedApp: &appv1.Application{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Application",
+					APIVersion: "argoproj.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "my-cluster-guestbook",
+					Namespace:  "namespace",
+					Finalizers: []string{"resources-finalizer.argocd.argoproj.io"},
+					Annotations: map[string]string{
+						"annotation-some-key": "annotation-some-value",
+					},
+				},
+				Spec: appv1.ApplicationSpec{
+					Project: "default",
+					Source: &appv1.ApplicationSource{
+						RepoURL:        "https://github.com/argoproj/argocd-example-apps.git",
+						TargetRevision: "HEAD",
+						Path:           "guestbook",
+						Helm: &appv1.ApplicationSourceHelm{
+							ValueFiles: []string{
+								"values.test.yaml",
+								"values.big.yaml",
+							},
+						},
+					},
+					Destination: appv1.ApplicationDestination{
+						Server:    "https://kubernetes.default.svc",
+						Namespace: "guestbook",
+					},
+					SyncPolicy: &appv1.SyncPolicy{
+						Automated: &appv1.SyncPolicyAutomated{
+							Prune: true,
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "patch with YAML",
+			appTemplate: &appv1.Application{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Application",
+					APIVersion: "argoproj.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "my-cluster-guestbook",
+					Namespace:  "namespace",
+					Finalizers: []string{"resources-finalizer.argocd.argoproj.io"},
+				},
+				Spec: appv1.ApplicationSpec{
+					Project: "default",
+					Source: &appv1.ApplicationSource{
+						RepoURL:        "https://github.com/argoproj/argocd-example-apps.git",
+						TargetRevision: "HEAD",
+						Path:           "guestbook",
+					},
+					Destination: appv1.ApplicationDestination{
+						Server:    "https://kubernetes.default.svc",
+						Namespace: "guestbook",
+					},
+				},
+			},
+			templatePatch: `
+metadata:
+  annotations:
+    annotation-some-key: annotation-some-value
+spec:
+  source:
+    helm:
+      valueFiles:
+        - values.test.yaml
+        - values.big.yaml
+  syncPolicy:
+    automated:
+      prune: true`,
+			expectedApp: &appv1.Application{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Application",
+					APIVersion: "argoproj.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "my-cluster-guestbook",
+					Namespace:  "namespace",
+					Finalizers: []string{"resources-finalizer.argocd.argoproj.io"},
+					Annotations: map[string]string{
+						"annotation-some-key": "annotation-some-value",
+					},
+				},
+				Spec: appv1.ApplicationSpec{
+					Project: "default",
+					Source: &appv1.ApplicationSource{
+						RepoURL:        "https://github.com/argoproj/argocd-example-apps.git",
+						TargetRevision: "HEAD",
+						Path:           "guestbook",
+						Helm: &appv1.ApplicationSourceHelm{
+							ValueFiles: []string{
+								"values.test.yaml",
+								"values.big.yaml",
+							},
+						},
+					},
+					Destination: appv1.ApplicationDestination{
+						Server:    "https://kubernetes.default.svc",
+						Namespace: "guestbook",
+					},
+					SyncPolicy: &appv1.SyncPolicy{
+						Automated: &appv1.SyncPolicyAutomated{
+							Prune: true,
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "project field isn't overwritten",
+			appTemplate: &appv1.Application{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Application",
+					APIVersion: "argoproj.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "my-cluster-guestbook",
+					Namespace: "namespace",
+				},
+				Spec: appv1.ApplicationSpec{
+					Project: "default",
+					Source: &appv1.ApplicationSource{
+						RepoURL:        "https://github.com/argoproj/argocd-example-apps.git",
+						TargetRevision: "HEAD",
+						Path:           "guestbook",
+					},
+					Destination: appv1.ApplicationDestination{
+						Server:    "https://kubernetes.default.svc",
+						Namespace: "guestbook",
+					},
+				},
+			},
+			templatePatch: `
+spec:
+  project: my-project`,
+			expectedApp: &appv1.Application{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Application",
+					APIVersion: "argoproj.io/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "my-cluster-guestbook",
+					Namespace: "namespace",
+				},
+				Spec: appv1.ApplicationSpec{
+					Project: "default",
+					Source: &appv1.ApplicationSource{
+						RepoURL:        "https://github.com/argoproj/argocd-example-apps.git",
+						TargetRevision: "HEAD",
+						Path:           "guestbook",
+					},
+					Destination: appv1.ApplicationDestination{
+						Server:    "https://kubernetes.default.svc",
+						Namespace: "guestbook",
+					},
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		tcc := tc
+		t.Run(tcc.name, func(t *testing.T) {
+			result, err := applyTemplatePatch(tcc.appTemplate, tcc.templatePatch)
+			require.NoError(t, err)
+			assert.Equal(t, *tcc.expectedApp, *result)
+		})
+	}
+}
+
+func TestError(t *testing.T) {
+	app := &appv1.Application{}
+
+	result, err := applyTemplatePatch(app, "hello world")
+	require.Error(t, err)
+	require.Nil(t, result)
+}