From 0256fa138aaa37530e6c72c820193b501d98e1c9 Mon Sep 17 00:00:00 2001 From: Jake Hyde Date: Tue, 13 Aug 2024 22:53:24 -0400 Subject: [PATCH 1/3] Add validation to data directories on creation --- .../v1/cluster/validator.go | 105 ++++++- .../v1/cluster/validator_test.go | 284 +++++++++++++++++- 2 files changed, 371 insertions(+), 18 deletions(-) diff --git a/pkg/resources/provisioning.cattle.io/v1/cluster/validator.go b/pkg/resources/provisioning.cattle.io/v1/cluster/validator.go index 10ed8b19..bfdd54e2 100644 --- a/pkg/resources/provisioning.cattle.io/v1/cluster/validator.go +++ b/pkg/resources/provisioning.cattle.io/v1/cluster/validator.go @@ -5,8 +5,10 @@ import ( "encoding/base64" "fmt" "net/http" + "path/filepath" "regexp" "slices" + "strings" v1 "github.com/rancher/rancher/pkg/apis/provisioning.cattle.io/v1" rkev1 "github.com/rancher/rancher/pkg/apis/rke.cattle.io/v1" @@ -168,7 +170,8 @@ func (p *provisioningAdmitter) validateSystemAgentDataDirectory(oldCluster, newC return admission.ResponseAllowed() } -// validateDataDirectories will validate updates to the cluster object to ensure the data directories are not changed. +// validateDataDirectories will ensure that data directories are properly formatted on creation, not duplicated or embed +// each other, and will also validate updates to the cluster object to ensure the data directories are not changed. // The only exception when a data directory is allowed to be changed is if cluster.Spec.AgentEnvVars has an env var with // a name of "CATTLE_AGENT_VAR_DIR", which Rancher will perform a one-time migration to set the // cluster.Spec.RKEConfig.DataDirectories.SystemAgent field for the cluster. validateAgentEnvVars will ensure @@ -177,6 +180,9 @@ func (p *provisioningAdmitter) validateDataDirectories(request *admission.Reques if newCluster.Spec.RKEConfig == nil { return admission.ResponseAllowed() } + distro := newCluster.Spec.RKEConfig.DataDirectories.K8sDistro + provisioning := newCluster.Spec.RKEConfig.DataDirectories.Provisioning + systemAgent := newCluster.Spec.RKEConfig.DataDirectories.SystemAgent // cannot set "CATTLE_AGENT_VAR_DIR" on create anymore, but still valid as a field until cluster is migrated. if request.Operation == admissionv1.Create { if slices.ContainsFunc(newCluster.Spec.AgentEnvVars, func(envVar rkev1.EnvVar) bool { @@ -185,6 +191,21 @@ func (p *provisioningAdmitter) validateDataDirectories(request *admission.Reques return admission.ResponseBadRequest( fmt.Sprintf(`"%s" cannot be set within "cluster.Spec.RKEConfig.AgentEnvVars": use "cluster.Spec.RKEConfig.DataDirectories.SystemAgent"`, systemAgentVarDirEnvVar)) } + dataDirectories := map[string]string{ + "Distro": distro, + "Provisioning": provisioning, + "System Agent": systemAgent, + } + for name, dir := range dataDirectories { + response := validateDataDirectoryFormat(dir, name) + if !response.Allowed { + return response + } + } + response := validateDataDirectoryHierarchy(dataDirectories) + if !response.Allowed { + return response + } return admission.ResponseAllowed() } if request.Operation != admissionv1.Update { @@ -194,11 +215,87 @@ func (p *provisioningAdmitter) validateDataDirectories(request *admission.Reques if response := p.validateSystemAgentDataDirectory(oldCluster, newCluster); !response.Allowed { return response } - if oldCluster.Spec.RKEConfig.DataDirectories.Provisioning != newCluster.Spec.RKEConfig.DataDirectories.Provisioning { + if oldCluster.Spec.RKEConfig.DataDirectories.K8sDistro != distro { + return admission.ResponseBadRequest("Distro data directory cannot be changed after cluster creation") + } + if oldCluster.Spec.RKEConfig.DataDirectories.Provisioning != provisioning { return admission.ResponseBadRequest("Provisioning data directory cannot be changed after cluster creation") } - if oldCluster.Spec.RKEConfig.DataDirectories.K8sDistro != newCluster.Spec.RKEConfig.DataDirectories.K8sDistro { - return admission.ResponseBadRequest("Distro data directory cannot be changed after cluster creation") + + return admission.ResponseAllowed() +} + +// validateDataDirectoryFormat ensures that no data directory contains a relative path, environment variables, +// shell expressions, or references to the current or parent directory via use of "./" and "../" respectively. +// dir is the path of the data directory, and name corresponds to a print friendly name for this data directory. +func validateDataDirectoryFormat(dir, name string) *admissionv1.AdmissionResponse { + if dir == "" { + return admission.ResponseAllowed() + } + if !filepath.IsAbs(dir) { + return admission.ResponseBadRequest( + fmt.Sprintf("%s data directory must be an absolute path", name)) + } + if strings.ContainsAny(dir, "\"'`*?#~=%$|&;<>{}[]()") { + return admission.ResponseBadRequest( + fmt.Sprintf("%s data directory cannot contain shell expressions", name)) + } + if filepath.Clean(dir) != dir { + return admission.ResponseBadRequest( + fmt.Sprintf("%s data directory is not clean", name)) + } + + return admission.ResponseAllowed() +} + +// validateDataDirectoryHierarchy ensures that no directories are equal, and no directories include other directories. +// dataDirs is a map with keys corresponding to print friendly names for these data directories, and values representing +// the specific data directories. +func validateDataDirectoryHierarchy(dataDirs map[string]string) *admissionv1.AdmissionResponse { + paths := make([]struct { + name string + path string + }, 0, len(dataDirs)) + for name, dir := range dataDirs { + // do not attempt to validate empty directory + if dir == "" { + continue + } + paths = append(paths, struct { + name string + path string + }{ + name: name, + path: dir, + }) + } + + for i := range paths { + for j := i + 1; j < len(paths); j++ { + path1 := paths[i] + path2 := paths[j] + + if path1.path == path2.path { + return admission.ResponseBadRequest( + fmt.Sprintf("%s data directory cannot be equal to %s data directory", path1.name, path2.name)) + } + + // check if paths contain one another + if matched, err := filepath.Match(fmt.Sprintf("%s%c*", path1.path, filepath.Separator), path2.path); err != nil { + return admission.ResponseBadRequest( + fmt.Sprintf("error determining if %s data directory is nested inside %s data directory: %s", path2.name, path1.name, err.Error())) + } else if matched { + return admission.ResponseBadRequest( + fmt.Sprintf("%s data directory cannot be nested inside %s data directory", path2.name, path1.name)) + } + if matched, err := filepath.Match(fmt.Sprintf("%s%c*", path2.path, filepath.Separator), path1.path); err != nil { + return admission.ResponseBadRequest( + fmt.Sprintf("error determining if %s data directory is nested inside %s data directory: %s", path1.name, path2.name, err.Error())) + } else if matched { + return admission.ResponseBadRequest( + fmt.Sprintf("%s data directory cannot be nested inside %s data directory", path1.name, path2.name)) + } + } } return admission.ResponseAllowed() diff --git a/pkg/resources/provisioning.cattle.io/v1/cluster/validator_test.go b/pkg/resources/provisioning.cattle.io/v1/cluster/validator_test.go index 8928e868..01ee1cc9 100644 --- a/pkg/resources/provisioning.cattle.io/v1/cluster/validator_test.go +++ b/pkg/resources/provisioning.cattle.io/v1/cluster/validator_test.go @@ -652,7 +652,7 @@ func TestValidateDataDirectories(t *testing.T) { AgentEnvVars: []rkev1.EnvVar{ { Name: "CATTLE_AGENT_VAR_DIR", - Value: "a", + Value: "/a", }, }, }, @@ -660,6 +660,139 @@ func TestValidateDataDirectories(t *testing.T) { oldCluster: nil, shouldSucceed: false, }, + { + name: "CREATE distro data dir is relative", + request: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Operation: admissionv1.Create}}, + cluster: &v1.Cluster{ + Spec: v1.ClusterSpec{ + RKEConfig: &v1.RKEConfig{ + RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ + DataDirectories: rkev1.DataDirectories{ + K8sDistro: "a", + }, + }, + }, + }, + }, + shouldSucceed: false, + }, + { + name: "CREATE provisioning data dir is relative", + request: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Operation: admissionv1.Create}}, + cluster: &v1.Cluster{ + Spec: v1.ClusterSpec{ + RKEConfig: &v1.RKEConfig{ + RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ + DataDirectories: rkev1.DataDirectories{ + Provisioning: "a", + }, + }, + }, + }, + }, + shouldSucceed: false, + }, + { + name: "CREATE system agent data dir is relative", + request: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Operation: admissionv1.Create}}, + cluster: &v1.Cluster{ + Spec: v1.ClusterSpec{ + RKEConfig: &v1.RKEConfig{ + RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ + DataDirectories: rkev1.DataDirectories{ + SystemAgent: "a", + }, + }, + }, + }, + }, + shouldSucceed: false, + }, + { + name: "CREATE distro data dir == provisioning data dir", + request: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Operation: admissionv1.Create}}, + cluster: &v1.Cluster{ + Spec: v1.ClusterSpec{ + RKEConfig: &v1.RKEConfig{ + RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ + DataDirectories: rkev1.DataDirectories{ + K8sDistro: "/a", + Provisioning: "/a", + }, + }, + }, + }, + }, + shouldSucceed: false, + }, + { + name: "CREATE distro data dir == system agent data dir", + request: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Operation: admissionv1.Create}}, + cluster: &v1.Cluster{ + Spec: v1.ClusterSpec{ + RKEConfig: &v1.RKEConfig{ + RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ + DataDirectories: rkev1.DataDirectories{ + K8sDistro: "/a", + SystemAgent: "/a", + }, + }, + }, + }, + }, + shouldSucceed: false, + }, + { + name: "CREATE provisioning data dir == system agent data dir", + request: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Operation: admissionv1.Create}}, + cluster: &v1.Cluster{ + Spec: v1.ClusterSpec{ + RKEConfig: &v1.RKEConfig{ + RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ + DataDirectories: rkev1.DataDirectories{ + Provisioning: "/a", + SystemAgent: "/a", + }, + }, + }, + }, + }, + shouldSucceed: false, + }, + { + name: "CREATE distro data dir contains provisioning data dir", + request: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Operation: admissionv1.Create}}, + cluster: &v1.Cluster{ + Spec: v1.ClusterSpec{ + RKEConfig: &v1.RKEConfig{ + RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ + DataDirectories: rkev1.DataDirectories{ + K8sDistro: "/a", + Provisioning: "/a/b", + }, + }, + }, + }, + }, + shouldSucceed: false, + }, + { + name: "CREATE provisioning data dir contains distro data dir", + request: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Operation: admissionv1.Create}}, + cluster: &v1.Cluster{ + Spec: v1.ClusterSpec{ + RKEConfig: &v1.RKEConfig{ + RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ + DataDirectories: rkev1.DataDirectories{ + K8sDistro: "/a/b", + Provisioning: "/a", + }, + }, + }, + }, + }, + shouldSucceed: false, + }, { name: "Delete", request: &admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Operation: admissionv1.Delete}}, @@ -669,7 +802,7 @@ func TestValidateDataDirectories(t *testing.T) { AgentEnvVars: []rkev1.EnvVar{ { Name: "CATTLE_AGENT_VAR_DIR", - Value: "a", + Value: "/a", }, }, }, @@ -710,7 +843,7 @@ func TestValidateDataDirectories(t *testing.T) { AgentEnvVars: []rkev1.EnvVar{ { Name: "CATTLE_AGENT_VAR_DIR", - Value: "a", + Value: "/a", }, }, }, @@ -721,7 +854,7 @@ func TestValidateDataDirectories(t *testing.T) { AgentEnvVars: []rkev1.EnvVar{ { Name: "CATTLE_AGENT_VAR_DIR", - Value: "a", + Value: "/a", }, }, }, @@ -737,7 +870,7 @@ func TestValidateDataDirectories(t *testing.T) { AgentEnvVars: []rkev1.EnvVar{ { Name: "CATTLE_AGENT_VAR_DIR", - Value: "a", + Value: "/a", }, }, }, @@ -748,7 +881,7 @@ func TestValidateDataDirectories(t *testing.T) { AgentEnvVars: []rkev1.EnvVar{ { Name: "CATTLE_AGENT_VAR_DIR", - Value: "b", + Value: "/b", }, }, }, @@ -763,7 +896,7 @@ func TestValidateDataDirectories(t *testing.T) { RKEConfig: &v1.RKEConfig{ RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ DataDirectories: rkev1.DataDirectories{ - SystemAgent: "a", + SystemAgent: "/a", }, }, }, @@ -775,7 +908,7 @@ func TestValidateDataDirectories(t *testing.T) { AgentEnvVars: []rkev1.EnvVar{ { Name: "CATTLE_AGENT_VAR_DIR", - Value: "a", + Value: "/a", }, }, }, @@ -790,7 +923,7 @@ func TestValidateDataDirectories(t *testing.T) { RKEConfig: &v1.RKEConfig{ RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ DataDirectories: rkev1.DataDirectories{ - SystemAgent: "a", + SystemAgent: "/a", }, }, }, @@ -801,7 +934,7 @@ func TestValidateDataDirectories(t *testing.T) { RKEConfig: &v1.RKEConfig{ RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ DataDirectories: rkev1.DataDirectories{ - SystemAgent: "b", + SystemAgent: "/b", }, }, }, @@ -817,7 +950,7 @@ func TestValidateDataDirectories(t *testing.T) { RKEConfig: &v1.RKEConfig{ RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ DataDirectories: rkev1.DataDirectories{ - Provisioning: "a", + Provisioning: "/a", }, }, }, @@ -828,7 +961,7 @@ func TestValidateDataDirectories(t *testing.T) { RKEConfig: &v1.RKEConfig{ RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ DataDirectories: rkev1.DataDirectories{ - Provisioning: "b", + Provisioning: "/b", }, }, }, @@ -844,7 +977,7 @@ func TestValidateDataDirectories(t *testing.T) { RKEConfig: &v1.RKEConfig{ RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ DataDirectories: rkev1.DataDirectories{ - K8sDistro: "a", + K8sDistro: "/a", }, }, }, @@ -855,7 +988,7 @@ func TestValidateDataDirectories(t *testing.T) { RKEConfig: &v1.RKEConfig{ RKEClusterSpecCommon: rkev1.RKEClusterSpecCommon{ DataDirectories: rkev1.DataDirectories{ - K8sDistro: "b", + K8sDistro: "/b", }, }, }, @@ -875,3 +1008,126 @@ func TestValidateDataDirectories(t *testing.T) { }) } } + +func TestValidateDataDirectoryFormat(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + dir string + expected bool + }{ + { + name: "relative", + dir: "home", + expected: false, + }, + { + name: "trailing slash", + dir: "/home/", + expected: false, + }, + { + name: "env var", + dir: "/$HOME", + expected: false, + }, + { + name: "env var", + dir: "/${HOME}", + expected: false, + }, + { + name: "expr", + dir: "/`pwd`", + expected: false, + }, + { + name: "expr", + dir: "/$(pwd)", + expected: false, + }, + { + name: "current directory", + dir: "/./tmp", + expected: false, + }, + { + name: "current directory", + dir: "/tmp/.", + expected: false, + }, + { + name: "parent directory", + dir: "/tmp/../tmp", + expected: false, + }, + { + name: "current directory", + dir: "/tmp/..", + expected: false, + }, + { + name: "valid", + dir: "/tmp", + expected: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + response := validateDataDirectoryFormat(tt.dir, "Test") + assert.Equal(t, tt.expected, response.Allowed) + }) + } +} + +func TestValidateDataDirectoryHierarchy(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + dataDirs map[string]string + expected bool + }{ + { + name: "equal paths", + dataDirs: map[string]string{ + "a": "/a", + "b": "/a", + }, + expected: false, + }, + { + name: "nested paths", + dataDirs: map[string]string{ + "a": "/a", + "b": "/a/b", + }, + expected: false, + }, + { + name: "nested paths", + dataDirs: map[string]string{ + "a": "/a/b", + "b": "/a", + }, + expected: false, + }, + { + name: "distinct paths", + dataDirs: map[string]string{ + "a": "/a", + "b": "/b", + }, + expected: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + response := validateDataDirectoryHierarchy(tt.dataDirs) + assert.Equal(t, tt.expected, response.Allowed) + }) + } +} From b4e54349f0e16f9cca90a945fbd14228e812337c Mon Sep 17 00:00:00 2001 From: Jake Hyde Date: Tue, 13 Aug 2024 23:06:00 -0400 Subject: [PATCH 2/3] Update provisioning cluster docs --- .../provisioning.cattle.io/v1/cluster/Cluster.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/pkg/resources/provisioning.cattle.io/v1/cluster/Cluster.md b/pkg/resources/provisioning.cattle.io/v1/cluster/Cluster.md index 12f4b091..6b25f0d1 100644 --- a/pkg/resources/provisioning.cattle.io/v1/cluster/Cluster.md +++ b/pkg/resources/provisioning.cattle.io/v1/cluster/Cluster.md @@ -1,11 +1,23 @@ ## Validation Checks +### On Create + +#### Data Directories + +Prevent the creation of new objects with an env var (under `spec.agentEnvVars`) with a name of `CATTLE_AGENT_VAR_DIR`. +Prevent the creation of new objects with an invalid data directory. An invalid data directory is defined as the +following: +- Is not an absolute path (i.e. does not start with `/`) +- Attempts to include environment variables (e.g. `$VARIABLE` or `${VARIABLE}`) +- Attempts to include shell expressions (e.g. `$(command)` or `` `command` ``) +- Equal to another data directory +- Attempts to nest another data directory + ### On Update #### Data Directories -Prevent the creation of new objects with an env var (under `spec.agentEnvVars`) with a name of `CATTLE_AGENT_VAR_DIR`. -On update, also prevent new env vars with this name from being added but allow them to be removed. Rancher will perform +On update, prevent new env vars with this name from being added but allow them to be removed. Rancher will perform a one-time migration to move the system-agent data dir definition to the top level field from the `AgentEnvVars` section. A secondary validator will ensure that the effective data directory for the `system-agent` is not different from the one chosen during cluster creation. Additionally, the changing of a data directory for the `system-agent`, From acb58843bad6d030ff00b3858957f030835350be Mon Sep 17 00:00:00 2001 From: Jake Hyde Date: Tue, 13 Aug 2024 23:06:51 -0400 Subject: [PATCH 3/3] go generate --- docs.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/docs.md b/docs.md index 21396ed3..76e300dc 100644 --- a/docs.md +++ b/docs.md @@ -379,12 +379,24 @@ When a UserAttribute is updated, the following checks take place: ### Validation Checks +#### On Create + +##### Data Directories + +Prevent the creation of new objects with an env var (under `spec.agentEnvVars`) with a name of `CATTLE_AGENT_VAR_DIR`. +Prevent the creation of new objects with an invalid data directory. An invalid data directory is defined as the +following: +- Is not an absolute path (i.e. does not start with `/`) +- Attempts to include environment variables (e.g. `$VARIABLE` or `${VARIABLE}`) +- Attempts to include shell expressions (e.g. `$(command)` or `` `command` ``) +- Equal to another data directory +- Attempts to nest another data directory + #### On Update ##### Data Directories -Prevent the creation of new objects with an env var (under `spec.agentEnvVars`) with a name of `CATTLE_AGENT_VAR_DIR`. -On update, also prevent new env vars with this name from being added but allow them to be removed. Rancher will perform +On update, prevent new env vars with this name from being added but allow them to be removed. Rancher will perform a one-time migration to move the system-agent data dir definition to the top level field from the `AgentEnvVars` section. A secondary validator will ensure that the effective data directory for the `system-agent` is not different from the one chosen during cluster creation. Additionally, the changing of a data directory for the `system-agent`,