[ansible]: mark variables from custom resources as unsafe (#4566)

* Ansible: mark variables from custom resources as unsafe User provided values from the custom resource are tagged unsafe by default. Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com> * Make marking of variables unsafe optional Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com> * mark all inputs to spec as safe Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>
operator-framework · Mar 14, 2021 · 4dede85 · 4dede85
1 parent c6796de
commit 4dede85
Show file tree

Hide file tree

Showing 6 changed files with 202 additions and 0 deletions.
diff --git a/changelog/fragments/mark-variables-unsafe.yaml b/changelog/fragments/mark-variables-unsafe.yaml
@@ -0,0 +1,5 @@
+entries:
+  - description: >
+      Mark the input variables from custom resources as unsafe by default in Ansible operators. 
+    kind: bugfix
+    breaking: false
diff --git a/internal/ansible/runner/runner.go b/internal/ansible/runner/runner.go
@@ -160,6 +160,7 @@ func New(watch watches.Watch, runnerArgs string) (Runner, error) {
 		ansibleVerbosity:    watch.AnsibleVerbosity,
 		ansibleArgs:         runnerArgs,
 		snakeCaseParameters: watch.SnakeCaseParameters,
+		markUnsafe:          watch.MarkUnsafe,
 	}, nil
 }
 
@@ -174,6 +175,7 @@ type runner struct {
 	maxRunnerArtifacts  int
 	ansibleVerbosity    int
 	snakeCaseParameters bool
+	markUnsafe          bool
 	ansibleArgs         string
 }
 
@@ -341,6 +343,12 @@ func (r *runner) makeParameters(u *unstructured.Unstructured) map[string]interfa
 		}
 	}
 
+	if r.markUnsafe {
+		for key, val := range parameters {
+			parameters[key] = markUnsafe(val)
+		}
+	}
+
 	parameters["ansible_operator_meta"] = map[string]string{"namespace": u.GetNamespace(), "name": u.GetName()}
 
 	objKey := escapeAnsibleKey(fmt.Sprintf("_%v_%v", r.GVK.Group, strings.ToLower(r.GVK.Kind)))
@@ -360,6 +368,34 @@ func (r *runner) makeParameters(u *unstructured.Unstructured) map[string]interfa
 	return parameters
 }
 
+// markUnsafe recursively checks for string values and marks them unsafe.
+// for eg:
+//		spec:
+//			key: "val"
+// would be marked unsafe in JSON format as:
+//		spec:
+//			key: map{__ansible_unsafe:"val"}
+func markUnsafe(values interface{}) interface{} {
+	switch v := values.(type) {
+	case []interface{}:
+		var p []interface{}
+		for _, n := range v {
+			p = append(p, markUnsafe(n))
+		}
+		return p
+	case map[string]interface{}:
+		m := make(map[string]interface{})
+		for k, v := range v {
+			m[k] = markUnsafe(v)
+		}
+		return m
+	case string:
+		return map[string]interface{}{"__ansible_unsafe": values}
+	default:
+		return values
+	}
+}
+
 // escapeAnsibleKey - replaces characters that would result in an inaccessible Ansible parameter with underscores
 // ie, _cert-manager.k8s.io would be converted to _cert_manager_k8s_io
 func escapeAnsibleKey(key string) string {

diff --git a/internal/ansible/runner/runner_test.go b/internal/ansible/runner/runner_test.go
@@ -244,3 +244,130 @@ func TestAnsibleVerbosityString(t *testing.T) {
 		}
 	}
 }
+
+func TestMakeParameters(t *testing.T) {
+	var (
+		inputSpec string = "testKey"
+	)
+
+	testCases := []struct {
+		name               string
+		inputParams        unstructured.Unstructured
+		expectedSafeParams interface{}
+	}{
+		{
+			name: "should mark values passed as string unsafe",
+			inputParams: unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"spec": map[string]interface{}{
+						inputSpec: "testVal",
+					},
+				},
+			},
+			expectedSafeParams: map[string]interface{}{
+				"__ansible_unsafe": "testVal",
+			},
+		},
+		{
+			name: "should not mark integers unsafe",
+			inputParams: unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"spec": map[string]interface{}{
+						inputSpec: 3,
+					},
+				},
+			},
+			expectedSafeParams: 3,
+		},
+		{
+			name: "should recursively mark values in dictionary as unsafe",
+			inputParams: unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"spec": map[string]interface{}{
+						inputSpec: map[string]interface{}{
+							"testsubKey1": "val1",
+							"testsubKey2": "val2",
+						},
+					},
+				},
+			},
+			expectedSafeParams: map[string]interface{}{
+				"testsubKey1": map[string]interface{}{
+					"__ansible_unsafe": "val1",
+				},
+				"testsubKey2": map[string]interface{}{
+					"__ansible_unsafe": "val2",
+				},
+			},
+		},
+		{
+			name: "should recursively mark values in list as unsafe",
+			inputParams: unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"spec": map[string]interface{}{
+						inputSpec: []interface{}{
+							"testVal1",
+							"testVal2",
+						},
+					},
+				},
+			},
+			expectedSafeParams: []interface{}{
+				map[string]interface{}{
+					"__ansible_unsafe": "testVal1",
+				},
+				map[string]interface{}{
+					"__ansible_unsafe": "testVal2",
+				},
+			},
+		},
+		{
+			name: "should recursively mark values in list/dict as unsafe",
+			inputParams: unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"spec": map[string]interface{}{
+						inputSpec: []interface{}{
+							"testVal1",
+							"testVal2",
+							map[string]interface{}{
+								"testVal3": 3,
+								"testVal4": "__^&{__)",
+							},
+						},
+					},
+				},
+			},
+			expectedSafeParams: []interface{}{
+				map[string]interface{}{
+					"__ansible_unsafe": "testVal1",
+				},
+				map[string]interface{}{
+					"__ansible_unsafe": "testVal2",
+				},
+				map[string]interface{}{
+					"testVal3": 3,
+					"testVal4": map[string]interface{}{
+						"__ansible_unsafe": "__^&{__)",
+					},
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		testRunner := runner{
+			markUnsafe: true,
+		}
+		parameters := testRunner.makeParameters(&tc.inputParams)
+
+		val, ok := parameters[inputSpec]
+		if !ok {
+			t.Fatalf("Error occurred, value %s in spec is missing", inputSpec)
+		} else {
+			eq := reflect.DeepEqual(val, tc.expectedSafeParams)
+			if !eq {
+				t.Errorf("Error occurred, parameters %v are not marked unsafe", val)
+			}
+		}
+	}
+}
diff --git a/internal/ansible/watches/testdata/valid.yaml.tmpl b/internal/ansible/watches/testdata/valid.yaml.tmpl
@@ -4,6 +4,12 @@
   kind: NoFinalizer
   playbook: {{ .ValidPlaybook }}
   reconcilePeriod: 2s
+- version: v1alpha1
+  group: app.example.com
+  kind: WithUnsafeMarked
+  playbook: {{ .ValidPlaybook }}
+  reconcilePeriod: 2s
+  markUnsafe: True
 - version: v1alpha1
   group: app.example.com
   kind: Playbook

diff --git a/internal/ansible/watches/watches.go b/internal/ansible/watches/watches.go
@@ -52,6 +52,7 @@ type Watch struct {
 	WatchDependentResources     bool                      `yaml:"watchDependentResources"`
 	WatchClusterScopedResources bool                      `yaml:"watchClusterScopedResources"`
 	SnakeCaseParameters         bool                      `yaml:"snakeCaseParameters"`
+	MarkUnsafe                  bool                      `yaml:"markUnsafe"`
 	Selector                    metav1.LabelSelector      `yaml:"selector"`
 
 	// Not configurable via watches.yaml
@@ -76,6 +77,7 @@ var (
 	watchDependentResourcesDefault     = true
 	watchClusterScopedResourcesDefault = false
 	snakeCaseParametersDefault         = true
+	markUnsafeDefault                  = false
 	selectorDefault                    = metav1.LabelSelector{}
 
 	// these are overridden by cmdline flags
@@ -127,6 +129,7 @@ type alias struct {
 	WatchDependentResources     *bool                     `yaml:"watchDependentResources,omitempty"`
 	WatchClusterScopedResources *bool                     `yaml:"watchClusterScopedResources,omitempty"`
 	SnakeCaseParameters         *bool                     `yaml:"snakeCaseParameters"`
+	MarkUnsafe                  *bool                     `yaml:"markUnsafe"`
 	Blacklist                   []schema.GroupVersionKind `yaml:"blacklist,omitempty"`
 	Finalizer                   *Finalizer                `yaml:"finalizer"`
 	Selector                    tempLabelSelector         `yaml:"selector"`
@@ -162,6 +165,10 @@ func (w *Watch) setValuesFromAlias(tmp alias) error {
 		tmp.SnakeCaseParameters = &snakeCaseParametersDefault
 	}
 
+	if tmp.MarkUnsafe == nil {
+		tmp.MarkUnsafe = &markUnsafeDefault
+	}
+
 	gvk := schema.GroupVersionKind{
 		Group:   tmp.Group,
 		Version: tmp.Version,
@@ -183,6 +190,7 @@ func (w *Watch) setValuesFromAlias(tmp alias) error {
 	w.ManageStatus = *tmp.ManageStatus
 	w.WatchDependentResources = *tmp.WatchDependentResources
 	w.SnakeCaseParameters = *tmp.SnakeCaseParameters
+	w.MarkUnsafe = *tmp.MarkUnsafe
 	w.WatchClusterScopedResources = *tmp.WatchClusterScopedResources
 	w.Finalizer = tmp.Finalizer
 	w.AnsibleVerbosity = getAnsibleVerbosity(gvk, ansibleVerbosityDefault)
@@ -316,6 +324,7 @@ func New(gvk schema.GroupVersionKind, role, playbook string, vars map[string]int
 		WatchDependentResources:     watchDependentResourcesDefault,
 		WatchClusterScopedResources: watchClusterScopedResourcesDefault,
 		SnakeCaseParameters:         snakeCaseParametersDefault,
+		MarkUnsafe:                  markUnsafeDefault,
 		Finalizer:                   finalizer,
 		AnsibleVerbosity:            ansibleVerbosityDefault,
 		Selector:                    selectorDefault,

diff --git a/internal/ansible/watches/watches_test.go b/internal/ansible/watches/watches_test.go
@@ -82,6 +82,9 @@ func TestNew(t *testing.T) {
 				t.Fatalf("Unexpected snakeCaseParameters %v expected %v", watch.SnakeCaseParameters,
 					snakeCaseParametersDefault)
 			}
+			if watch.MarkUnsafe != markUnsafeDefault {
+				t.Fatalf("Unexpected markUnsafe %v expected %v", watch.MarkUnsafe, markUnsafeDefault)
+			}
 			if watch.WatchClusterScopedResources != watchClusterScopedResourcesDefault {
 				t.Fatalf("Unexpected watchClusterScopedResources %v expected %v",
 					watch.WatchClusterScopedResources, watchClusterScopedResourcesDefault)
@@ -147,6 +150,18 @@ func TestLoad(t *testing.T) { //nolint:gocyclo
 			WatchDependentResources:     true,
 			WatchClusterScopedResources: false,
 			SnakeCaseParameters:         true,
+			MarkUnsafe:                  false,
+		},
+		Watch{
+			GroupVersionKind: schema.GroupVersionKind{
+				Version: "v1alpha1",
+				Group:   "app.example.com",
+				Kind:    "WithUnsafeMarked",
+			},
+			Playbook:        validTemplate.ValidPlaybook,
+			ManageStatus:    true,
+			ReconcilePeriod: twoSeconds,
+			MarkUnsafe:      true,
 		},
 		Watch{
 			GroupVersionKind: schema.GroupVersionKind{
@@ -536,6 +551,10 @@ func TestLoad(t *testing.T) { //nolint:gocyclo
 					t.Fatalf("The GVK: %v unexpected reconcile period: %v expected reconcile period: %v", gvk,
 						gotWatch.ReconcilePeriod, expectedWatch.ReconcilePeriod)
 				}
+				if gotWatch.MarkUnsafe != expectedWatch.MarkUnsafe {
+					t.Fatalf("The GVK: %v unexpected mark unsafe: %v expected mark unsafe: %v", gvk,
+						gotWatch.MarkUnsafe, expectedWatch.MarkUnsafe)
+				}
 
 				for i, val := range expectedWatch.Blacklist {
 					if val != gotWatch.Blacklist[i] {