Restrict manifest paths to be in the kustomize directory tree (#29)

* Restrict manifest paths to be in the kustomize directory tree

This is an added security measure so that the user cannot specify a
manifest path that is not related to their Kustomize configuration.

Relates:
stolostron/backlog#18236

Signed-off-by: mprahl <[email protected]>

* Fix a typo in a test name

Signed-off-by: mprahl <[email protected]>

Author: mprahl

cmd/main.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
+	"path"
 
 	"github.com/open-cluster-management/policy-generator-plugin/internal"
 	"github.com/spf13/pflag"
@@ -50,13 +51,18 @@ func errorAndExit(msg string, formatArgs ...interface{}) {
 // It reads the file, processes and validates the contents, uses the contents to
 // generate policies, and returns the generated policies as a byte array.
 func processGeneratorConfig(filePath string) []byte {
+	cwd, err := os.Getwd()
+	if err != nil {
+		errorAndExit("failed to determine the current directory: %v", err)
+	}
+
 	p := internal.Plugin{}
 	fileData, err := ioutil.ReadFile(filePath)
 	if err != nil {
 		errorAndExit("failed to read file '%s': %s", filePath, err)
 	}
 
-	err = p.Config(fileData)
+	err = p.Config(fileData, path.Dir(cwd))
 	if err != nil {
 		errorAndExit("error processing the PolicyGenerator file '%s': %s", filePath, err)
 	}

docs/policygenerator-reference.yaml

@@ -63,7 +63,9 @@ policies:
     # Required. The list of Kubernetes resource object manifests to include in the policy.
     manifests:
       # Required. Path to a single file or a flat directory of files relative to the
-      # kustomization.yaml file.
+      # kustomization.yaml file. This path cannot be in a directory outside of the directory with
+      # the kustomization.yaml file. Subdirectories within the directory with kustomization.yaml
+      # file are allowed.
       - path: ""
         # Optional. (See policy[0].complianceType for description.)
         complianceType: "musthave"

internal/plugin.go

@@ -38,6 +38,8 @@ type Plugin struct {
 	Policies       []types.PolicyConfig `json:"policies" yaml:"policies"`
 	// A set of all placement rule names that have been processed or generated
 	allPlrs map[string]bool
+	// The base of the directory tree to restrict all manifest files to be within
+	baseDirectory string
 	// This is a mapping of cluster selectors formatted as the return value of getCsKey to placement
 	// rule names. This is used to find common cluster selectors that can be consolidated to a
 	// single placement rule.
@@ -58,7 +60,7 @@ var defaults = types.PolicyDefaults{
 
 // Config validates the input PolicyGenerator configuration, applies any missing defaults, and
 // configures the Policy object.
-func (p *Plugin) Config(config []byte) error {
+func (p *Plugin) Config(config []byte, baseDirectory string) error {
 	err := yaml.Unmarshal(config, p)
 	const errTemplate = "the PolicyGenerator configuration file is invalid: %w"
 	if err != nil {
@@ -72,6 +74,8 @@ func (p *Plugin) Config(config []byte) error {
 	}
 	p.applyDefaults(unmarshaledConfig)
 
+	p.baseDirectory = baseDirectory
+
 	return p.assertValidConfig()
 }
 
@@ -353,6 +357,11 @@ func (p *Plugin) assertValidConfig() error {
 			if err != nil {
 				return fmt.Errorf("could not read the manifest path %s", manifest.Path)
 			}
+
+			err = verifyManifestPath(p.baseDirectory, manifest.Path)
+			if err != nil {
+				return err
+			}
 		}
 
 		if policy.Name == "" {

internal/plugin_config_test.go

@@ -78,7 +78,7 @@ policies:
 	)
 
 	p := Plugin{}
-	err := p.Config([]byte(exampleConfig))
+	err := p.Config([]byte(exampleConfig), tmpDir)
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -165,7 +165,7 @@ policies:
 		configMapPath,
 	)
 	p := Plugin{}
-	err := p.Config([]byte(defaultsConfig))
+	err := p.Config([]byte(defaultsConfig), tmpDir)
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -214,7 +214,7 @@ policies:
     - path: input/configmap.yaml
 `
 	p := Plugin{}
-	err := p.Config([]byte(config))
+	err := p.Config([]byte(config), "")
 	if err == nil {
 		t.Fatal("Expected an error but did not get one")
 	}
@@ -223,7 +223,7 @@ policies:
 	assertEqual(t, err.Error(), expected)
 }
 
-func TestCreateInvalidPolicyName(t *testing.T) {
+func TestConfigInvalidPolicyName(t *testing.T) {
 	t.Parallel()
 	tmpDir := t.TempDir()
 	createConfigMap(t, tmpDir, "configmap.yaml")
@@ -247,7 +247,7 @@ policies:
 	)
 
 	p := Plugin{}
-	err := p.Config([]byte(defaultsConfig))
+	err := p.Config([]byte(defaultsConfig), tmpDir)
 	if err == nil {
 		t.Fatal("Expected an error but did not get one")
 	}
@@ -267,7 +267,7 @@ policyDefaults:
   namespace: my-policies
 `
 	p := Plugin{}
-	err := p.Config([]byte(config))
+	err := p.Config([]byte(config), "")
 	if err == nil {
 		t.Fatal("Expected an error but did not get one")
 	}
@@ -276,6 +276,43 @@ policyDefaults:
 	assertEqual(t, err.Error(), expected)
 }
 
+func TestConfigInvalidPath(t *testing.T) {
+	t.Parallel()
+	tmpDir := t.TempDir()
+	createConfigMap(t, tmpDir, "configmap.yaml")
+	configMapPath := path.Join(tmpDir, "configmap.yaml")
+	policyNS := "my-policies"
+	policyName := "policy-app-config"
+	defaultsConfig := fmt.Sprintf(
+		`
+apiVersion: policy.open-cluster-management.io/v1
+kind: PolicyGenerator
+metadata:
+  name: policy-generator-name
+policyDefaults:
+  namespace: %s
+policies:
+- name: %s
+  manifests:
+    - path: %s
+`,
+		policyNS, policyName, configMapPath,
+	)
+
+	p := Plugin{}
+	// Provide a base directory that isn't in the same directory tree as tmpDir.
+	baseDir := t.TempDir()
+	err := p.Config([]byte(defaultsConfig), baseDir)
+	if err == nil {
+		t.Fatal("Expected an error but did not get one")
+	}
+
+	expected := fmt.Sprintf(
+		"the manifest path %s is not in the same directory tree as the kustomization.yaml file", configMapPath,
+	)
+	assertEqual(t, err.Error(), expected)
+}
+
 func TestConfigMultiplePlacements(t *testing.T) {
 	t.Parallel()
 	const config = `
@@ -295,7 +332,7 @@ policies:
     - path: input/configmap.yaml
 `
 	p := Plugin{}
-	err := p.Config([]byte(config))
+	err := p.Config([]byte(config), "")
 	if err == nil {
 		t.Fatal("Expected an error but did not get one")
 	}
@@ -331,7 +368,7 @@ policies:
 		path.Join(tmpDir, "configmap2.yaml"),
 	)
 	p := Plugin{}
-	err := p.Config([]byte(config))
+	err := p.Config([]byte(config), tmpDir)
 	if err == nil {
 		t.Fatal("Expected an error but did not get one")
 	}
@@ -352,7 +389,7 @@ policies:
 - name: policy-app-config
 `
 	p := Plugin{}
-	err := p.Config([]byte(config))
+	err := p.Config([]byte(config), "")
 	if err == nil {
 		t.Fatal("Expected an error but did not get one")
 	}
@@ -381,7 +418,7 @@ policies:
 		manifestPath,
 	)
 	p := Plugin{}
-	err := p.Config([]byte(config))
+	err := p.Config([]byte(config), tmpDir)
 	if err == nil {
 		t.Fatal("Expected an error but did not get one")
 	}
@@ -409,7 +446,7 @@ policies:
 		path.Join(tmpDir, "configmap.yaml"),
 	)
 	p := Plugin{}
-	err := p.Config([]byte(config))
+	err := p.Config([]byte(config), tmpDir)
 	if err == nil {
 		t.Fatal("Expected an error but did not get one")
 	}
@@ -442,7 +479,7 @@ policies:
 		path.Join(tmpDir, "configmap.yaml"),
 	)
 	p := Plugin{}
-	err := p.Config([]byte(config))
+	err := p.Config([]byte(config), tmpDir)
 	if err == nil {
 		t.Fatal("Expected an error but did not get one")
 	}

internal/plugin_test.go

@@ -16,6 +16,7 @@ func TestGenerate(t *testing.T) {
 	tmpDir := t.TempDir()
 	createConfigMap(t, tmpDir, "configmap.yaml")
 	p := Plugin{}
+	p.baseDirectory = tmpDir
 	p.PlacementBindingDefaults.Name = "my-placement-binding"
 	p.PolicyDefaults.Placement.Name = "my-placement-rule"
 	p.PolicyDefaults.Namespace = "my-policies"
@@ -158,6 +159,7 @@ func TestGenerateSeparateBindings(t *testing.T) {
 	tmpDir := t.TempDir()
 	createConfigMap(t, tmpDir, "configmap.yaml")
 	p := Plugin{}
+	p.baseDirectory = tmpDir
 	p.PolicyDefaults.Namespace = "my-policies"
 	policyConf := types.PolicyConfig{
 		Name: "policy-app-config",
@@ -305,6 +307,7 @@ func TestGenerateMissingBindingName(t *testing.T) {
 	tmpDir := t.TempDir()
 	createConfigMap(t, tmpDir, "configmap.yaml")
 	p := Plugin{}
+	p.baseDirectory = tmpDir
 	p.PlacementBindingDefaults.Name = ""
 	p.PolicyDefaults.Placement.Name = "my-placement-rule"
 	p.PolicyDefaults.Namespace = "my-policies"

internal/utils.go

@@ -9,6 +9,8 @@ import (
 	"io/ioutil"
 	"os"
 	"path"
+	"path/filepath"
+	"strings"
 
 	"github.com/open-cluster-management/policy-generator-plugin/internal/expanders"
 	"github.com/open-cluster-management/policy-generator-plugin/internal/types"
@@ -280,3 +282,30 @@ func unmarshalManifestBytes(manifestBytes []byte) (*[]map[string]interface{}, er
 
 	return &yamlDocs, nil
 }
+
+// verifyManifestPath verifies that the manifest path is in the directory tree under baseDirectory.
+// An error is returned if it is not or the paths couldn't be properly resolved.
+func verifyManifestPath(baseDirectory string, manifestPath string) error {
+	absPath, err := filepath.Abs(manifestPath)
+	if err != nil {
+		return fmt.Errorf("could not resolve the manifest path %s to an absolute path", manifestPath)
+	}
+
+	relPath, err := filepath.Rel(baseDirectory, absPath)
+	if err != nil {
+		return fmt.Errorf(
+			"could not resolve the manifest path %s to a relative path from the kustomization.yaml file",
+			manifestPath,
+		)
+	}
+
+	parDir := ".." + string(filepath.Separator)
+	if strings.HasPrefix(relPath, parDir) || relPath == ".." {
+		return fmt.Errorf(
+			"the manifest path %s is not in the same directory tree as the kustomization.yaml file",
+			manifestPath,
+		)
+	}
+
+	return nil
+}

internal/utils_test.go

@@ -4,6 +4,7 @@ package internal
 import (
 	"fmt"
 	"io/ioutil"
+	"os"
 	"path"
 	"reflect"
 	"testing"
@@ -672,3 +673,68 @@ func TestUnmarshalManifestFileNotObject(t *testing.T) {
 	)
 	assertEqual(t, err.Error(), expected)
 }
+
+// nolint: paralleltest
+func TestVerifyManifestPath(t *testing.T) {
+	baseDirectory := t.TempDir()
+	cwd, err := os.Getwd()
+	if err != nil {
+		t.Fatalf("Failed to get the current working directory: %v", err)
+	}
+
+	defer func() {
+		err := os.Chdir(cwd)
+		if err != nil {
+			// panic since this could affect other tests that haven't yet run
+			panic(fmt.Sprintf("Couldn't go back to the original working directory: %v", err))
+		}
+	}()
+
+	err = os.Chdir(baseDirectory)
+	if err != nil {
+		t.Fatalf("Failed to change the working directory to %s: %v", baseDirectory, err)
+	}
+
+	manifestPath := path.Join(baseDirectory, "configmap.yaml")
+	yamlContent := "---\nkind: ConfigMap"
+	err = ioutil.WriteFile(manifestPath, []byte(yamlContent), 0o666)
+	if err != nil {
+		t.Fatalf("Failed to write %s", manifestPath)
+	}
+
+	tests := []struct {
+		ManifestPath   string
+		ExpectedErrMsg string
+	}{
+		{manifestPath, ""},
+		{"configmap.yaml", ""},
+		{
+			"../../../../temp.yaml",
+			"the manifest path ../../../../temp.yaml is not in the same directory tree as the kustomization.yaml file",
+		},
+		{
+			"..",
+			"the manifest path .. is not in the same directory tree as the kustomization.yaml file",
+		},
+		{
+			"/super/secret",
+			"the manifest path /super/secret is not in the same directory tree as the kustomization.yaml file",
+		},
+	}
+
+	for _, test := range tests {
+		test := test
+		// nolint: paralleltest
+		t.Run(
+			"manifestPath="+test.ManifestPath,
+			func(t *testing.T) {
+				err := verifyManifestPath(baseDirectory, test.ManifestPath)
+				if err == nil {
+					assertEqual(t, "", test.ExpectedErrMsg)
+				} else {
+					assertEqual(t, err.Error(), test.ExpectedErrMsg)
+				}
+			},
+		)
+	}
+}