diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index e08bb1e5d60..3f6c9567667 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -598,7 +598,7 @@ func validateGitRepoVolumeSource(gitRepo *api.GitRepoVolumeSource, fldPath *fiel allErrs = append(allErrs, field.Required(fldPath.Child("repository"), "")) } - pathErrs := validateVolumeSourcePath(gitRepo.Directory, fldPath.Child("directory")) + pathErrs := validateLocalDescendingPath(gitRepo.Directory, fldPath.Child("directory")) allErrs = append(allErrs, pathErrs...) return allErrs } @@ -660,6 +660,11 @@ func validateSecretVolumeSource(secretSource *api.SecretVolumeSource, fldPath *f if len(secretSource.SecretName) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("secretName"), "")) } + itemsPath := fldPath.Child("items") + for i, kp := range secretSource.Items { + itemPath := itemsPath.Index(i) + allErrs = append(allErrs, validateKeyToPath(&kp, itemPath)...) + } return allErrs } @@ -668,6 +673,23 @@ func validateConfigMapVolumeSource(configMapSource *api.ConfigMapVolumeSource, f if len(configMapSource.Name) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("name"), "")) } + itemsPath := fldPath.Child("items") + for i, kp := range configMapSource.Items { + itemPath := itemsPath.Index(i) + allErrs = append(allErrs, validateKeyToPath(&kp, itemPath)...) + } + return allErrs +} + +func validateKeyToPath(kp *api.KeyToPath, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if len(kp.Key) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("key"), "")) + } + if len(kp.Path) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("path"), "")) + } + allErrs = append(allErrs, validateLocalNonReservedPath(kp.Path, fldPath.Child("path"))...) return allErrs } @@ -723,7 +745,7 @@ func validateDownwardAPIVolumeSource(downwardAPIVolume *api.DownwardAPIVolumeSou if len(downwardAPIVolumeFile.Path) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("path"), "")) } - allErrs = append(allErrs, validateVolumeSourcePath(downwardAPIVolumeFile.Path, fldPath.Child("path"))...) + allErrs = append(allErrs, validateLocalNonReservedPath(downwardAPIVolumeFile.Path, fldPath.Child("path"))...) if downwardAPIVolumeFile.FieldRef != nil { allErrs = append(allErrs, validateObjectFieldSelector(downwardAPIVolumeFile.FieldRef, &validDownwardAPIFieldPathExpressions, fldPath.Child("fieldRef"))...) if downwardAPIVolumeFile.ResourceFieldRef != nil { @@ -740,44 +762,32 @@ func validateDownwardAPIVolumeSource(downwardAPIVolume *api.DownwardAPIVolumeSou // This validate will make sure targetPath: // 1. is not abs path -// 2. does not start with '../' -// 3. does not contain '/../' -// 4. does not end with '/..' -func validateSubPath(targetPath string, fldPath *field.Path) field.ErrorList { +// 2. does not have any element which is ".." +func validateLocalDescendingPath(targetPath string, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if path.IsAbs(targetPath) { allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must be a relative path")) } - if strings.HasPrefix(targetPath, "../") { - allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must not start with '../'")) - } - if strings.Contains(targetPath, "/../") { - allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must not contain '/../'")) - } - if strings.HasSuffix(targetPath, "/..") { - allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must not end with '/..'")) + + // TODO: this assumes the OS of apiserver & nodes are the same + parts := strings.Split(targetPath, string(os.PathSeparator)) + for _, item := range parts { + if item == ".." { + allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must not contain '..'")) + } } return allErrs } // This validate will make sure targetPath: // 1. is not abs path -// 2. does not contain '..' +// 2. does not contain any '..' elements // 3. does not start with '..' -func validateVolumeSourcePath(targetPath string, fldPath *field.Path) field.ErrorList { +func validateLocalNonReservedPath(targetPath string, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - if path.IsAbs(targetPath) { - allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must be a relative path")) - } - // TODO assume OS of api server & nodes are the same for now - items := strings.Split(targetPath, string(os.PathSeparator)) - - for _, item := range items { - if item == ".." { - allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must not contain '..'")) - } - } - if strings.HasPrefix(items[0], "..") && len(items[0]) > 2 { + allErrs = append(allErrs, validateLocalDescendingPath(targetPath, fldPath)...) + // Don't report this error if the check for .. elements already caught it. + if strings.HasPrefix(targetPath, "..") && !strings.HasPrefix(targetPath, "../") { allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must not start with '..'")) } return allErrs @@ -1262,7 +1272,7 @@ func validateVolumeMounts(mounts []api.VolumeMount, volumes sets.String, fldPath } mountpoints.Insert(mnt.MountPath) if len(mnt.SubPath) > 0 { - allErrs = append(allErrs, validateSubPath(mnt.SubPath, fldPath.Child("subPath"))...) + allErrs = append(allErrs, validateLocalDescendingPath(mnt.SubPath, fldPath.Child("subPath"))...) } } return allErrs @@ -1918,7 +1928,7 @@ func validateSeccompProfile(p string, fldPath *field.Path) field.ErrorList { return nil } if strings.HasPrefix(p, "localhost/") { - return validateSubPath(strings.TrimPrefix(p, "localhost/"), fldPath) + return validateLocalDescendingPath(strings.TrimPrefix(p, "localhost/"), fldPath) } return field.ErrorList{field.Invalid(fldPath, p, "must be a valid seccomp profile")} } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index d3fe378ec84..c403897a024 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -776,6 +776,72 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) { } } +func TestValidateKeyToPath(t *testing.T) { + testCases := []struct { + kp api.KeyToPath + ok bool + errtype field.ErrorType + }{ + { + kp: api.KeyToPath{Key: "k", Path: "p"}, + ok: true, + }, + { + kp: api.KeyToPath{Key: "k", Path: "p/p/p/p"}, + ok: true, + }, + { + kp: api.KeyToPath{Key: "k", Path: "p/..p/p../p..p"}, + ok: true, + }, + { + kp: api.KeyToPath{Key: "", Path: "p"}, + ok: false, + errtype: field.ErrorTypeRequired, + }, + { + kp: api.KeyToPath{Key: "k", Path: ""}, + ok: false, + errtype: field.ErrorTypeRequired, + }, + { + kp: api.KeyToPath{Key: "k", Path: "..p"}, + ok: false, + errtype: field.ErrorTypeInvalid, + }, + { + kp: api.KeyToPath{Key: "k", Path: "../p"}, + ok: false, + errtype: field.ErrorTypeInvalid, + }, + { + kp: api.KeyToPath{Key: "k", Path: "p/../p"}, + ok: false, + errtype: field.ErrorTypeInvalid, + }, + { + kp: api.KeyToPath{Key: "k", Path: "p/.."}, + ok: false, + errtype: field.ErrorTypeInvalid, + }, + } + + for i, tc := range testCases { + errs := validateKeyToPath(&tc.kp, field.NewPath("field")) + if tc.ok && len(errs) > 0 { + t.Errorf("[%d] unexpected errors: %v", i, errs) + } else if !tc.ok && len(errs) == 0 { + t.Errorf("[%d] expected error type %v", i, tc.errtype) + } else if len(errs) > 1 { + t.Errorf("[%d] expected only one error, got %d", i, len(errs)) + } else if !tc.ok { + if errs[0].Type != tc.errtype { + t.Errorf("[%d] expected error type %v, got %v", i, tc.errtype, errs[0].Type) + } + } + } +} + func TestValidateVolumes(t *testing.T) { lun := int32(1) successCase := []api.Volume{ @@ -787,8 +853,14 @@ func TestValidateVolumes(t *testing.T) { {Name: "awsebs", VolumeSource: api.VolumeSource{AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{VolumeID: "my-PD", FSType: "ext4", Partition: 1, ReadOnly: false}}}, {Name: "gitrepo", VolumeSource: api.VolumeSource{GitRepo: &api.GitRepoVolumeSource{Repository: "my-repo", Revision: "hashstring", Directory: "target"}}}, {Name: "gitrepodot", VolumeSource: api.VolumeSource{GitRepo: &api.GitRepoVolumeSource{Repository: "my-repo", Directory: "."}}}, + {Name: "gitrepodotdotfoo", VolumeSource: api.VolumeSource{GitRepo: &api.GitRepoVolumeSource{Repository: "my-repo", Directory: "..foo"}}}, {Name: "iscsidisk", VolumeSource: api.VolumeSource{ISCSI: &api.ISCSIVolumeSource{TargetPortal: "127.0.0.1", IQN: "iqn.2015-02.example.com:test", Lun: 1, FSType: "ext4", ReadOnly: false}}}, - {Name: "secret", VolumeSource: api.VolumeSource{Secret: &api.SecretVolumeSource{SecretName: "my-secret"}}}, + {Name: "secret1", VolumeSource: api.VolumeSource{Secret: &api.SecretVolumeSource{SecretName: "my-secret"}}}, + {Name: "secret2", VolumeSource: api.VolumeSource{Secret: &api.SecretVolumeSource{SecretName: "my-secret", Items: []api.KeyToPath{{Key: "key", Path: "filename"}}}}}, + {Name: "secret3", VolumeSource: api.VolumeSource{Secret: &api.SecretVolumeSource{SecretName: "my-secret", Items: []api.KeyToPath{{Key: "key", Path: "dir/filename"}}}}}, + {Name: "cfgmap1", VolumeSource: api.VolumeSource{ConfigMap: &api.ConfigMapVolumeSource{LocalObjectReference: api.LocalObjectReference{Name: "my-cfgmap"}}}}, + {Name: "cfgmap2", VolumeSource: api.VolumeSource{ConfigMap: &api.ConfigMapVolumeSource{LocalObjectReference: api.LocalObjectReference{Name: "my-cfgmap"}, Items: []api.KeyToPath{{Key: "key", Path: "filename"}}}}}, + {Name: "cfgmap3", VolumeSource: api.VolumeSource{ConfigMap: &api.ConfigMapVolumeSource{LocalObjectReference: api.LocalObjectReference{Name: "my-cfgmap"}, Items: []api.KeyToPath{{Key: "key", Path: "dir/filename"}}}}}, {Name: "glusterfs", VolumeSource: api.VolumeSource{Glusterfs: &api.GlusterfsVolumeSource{EndpointsName: "host1", Path: "path", ReadOnly: false}}}, {Name: "flocker", VolumeSource: api.VolumeSource{Flocker: &api.FlockerVolumeSource{DatasetName: "datasetName"}}}, {Name: "rbd", VolumeSource: api.VolumeSource{RBD: &api.RBDVolumeSource{CephMonitors: []string{"foo"}, RBDImage: "bar", FSType: "ext4"}}}, @@ -840,19 +912,23 @@ func TestValidateVolumes(t *testing.T) { if len(errs) != 0 { t.Errorf("expected success: %v", errs) } - if len(names) != len(successCase) || !names.HasAll("abc", "123", "abc-123", "empty", "gcepd", "gitrepo", "secret", "iscsidisk", "cinder", "cephfs", "flexvolume", "fc") { + if len(names) != len(successCase) || !names.HasAll("abc", "123", "abc-123", "empty", "gcepd", "gitrepo", "secret1", "secret2", "secret3", "cfgmap1", "cfgmap2", "cfgmap3", "iscsidisk", "cinder", "cephfs", "flexvolume", "fc") { t.Errorf("wrong names result: %v", names) } emptyVS := api.VolumeSource{EmptyDir: &api.EmptyDirVolumeSource{}} emptyPortal := api.VolumeSource{ISCSI: &api.ISCSIVolumeSource{TargetPortal: "", IQN: "iqn.2015-02.example.com:test", Lun: 1, FSType: "ext4", ReadOnly: false}} emptyIQN := api.VolumeSource{ISCSI: &api.ISCSIVolumeSource{TargetPortal: "127.0.0.1", IQN: "", Lun: 1, FSType: "ext4", ReadOnly: false}} + secretMissingPath := api.VolumeSource{Secret: &api.SecretVolumeSource{SecretName: "s", Items: []api.KeyToPath{{Key: "key", Path: ""}}}} + secretDotDot := api.VolumeSource{Secret: &api.SecretVolumeSource{SecretName: "s", Items: []api.KeyToPath{{Key: "key", Path: "../foo"}}}} + cfgmapMissingPath := api.VolumeSource{ConfigMap: &api.ConfigMapVolumeSource{LocalObjectReference: api.LocalObjectReference{Name: "c"}, Items: []api.KeyToPath{{Key: "key", Path: ""}}}} + cfgmapDotDot := api.VolumeSource{ConfigMap: &api.ConfigMapVolumeSource{LocalObjectReference: api.LocalObjectReference{Name: "c"}, Items: []api.KeyToPath{{Key: "key", Path: "../foo"}}}} emptyHosts := api.VolumeSource{Glusterfs: &api.GlusterfsVolumeSource{EndpointsName: "", Path: "path", ReadOnly: false}} emptyPath := api.VolumeSource{Glusterfs: &api.GlusterfsVolumeSource{EndpointsName: "host", Path: "", ReadOnly: false}} emptyName := api.VolumeSource{Flocker: &api.FlockerVolumeSource{DatasetName: ""}} emptyMon := api.VolumeSource{RBD: &api.RBDVolumeSource{CephMonitors: []string{}, RBDImage: "bar", FSType: "ext4"}} emptyImage := api.VolumeSource{RBD: &api.RBDVolumeSource{CephMonitors: []string{"foo"}, RBDImage: "", FSType: "ext4"}} emptyCephFSMon := api.VolumeSource{CephFS: &api.CephFSVolumeSource{Monitors: []string{}}} - startsWithDots := api.VolumeSource{GitRepo: &api.GitRepoVolumeSource{Repository: "foo", Directory: "..dots/bar"}} + startsWithDots := api.VolumeSource{GitRepo: &api.GitRepoVolumeSource{Repository: "foo", Directory: "../dots/bar"}} containsDots := api.VolumeSource{GitRepo: &api.GitRepoVolumeSource{Repository: "foo", Directory: "dots/../bar"}} absPath := api.VolumeSource{GitRepo: &api.GitRepoVolumeSource{Repository: "foo", Directory: "/abstarget"}} emptyPathName := api.VolumeSource{DownwardAPI: &api.DownwardAPIVolumeSource{Items: []api.DownwardAPIVolumeFile{{Path: "", @@ -929,6 +1005,26 @@ func TestValidateVolumes(t *testing.T) { field.ErrorTypeRequired, "iscsi.iqn", "", }, + "secret with missing path": { + []api.Volume{{Name: "secret-dot-dot", VolumeSource: secretMissingPath}}, + field.ErrorTypeRequired, + "secret.items[0]", "", + }, + "secret with ..": { + []api.Volume{{Name: "secret-dot-dot", VolumeSource: secretDotDot}}, + field.ErrorTypeInvalid, + "secret.items[0]", "", + }, + "configmap with missing path": { + []api.Volume{{Name: "cfgmap-dot-dot", VolumeSource: cfgmapMissingPath}}, + field.ErrorTypeRequired, + "configMap.items[0]", "", + }, + "configmap with ..": { + []api.Volume{{Name: "cfgmap-dot-dot", VolumeSource: cfgmapDotDot}}, + field.ErrorTypeInvalid, + "configMap.items[0]", "", + }, "empty hosts": { []api.Volume{{Name: "badhost", VolumeSource: emptyHosts}}, field.ErrorTypeRequired, @@ -1002,7 +1098,7 @@ func TestValidateVolumes(t *testing.T) { "starts with '..'": { []api.Volume{{Name: "badprefix", VolumeSource: startsWithDots}}, field.ErrorTypeInvalid, - "gitRepo.directory", `must not start with '..'`, + "gitRepo.directory", `must not contain '..'`, }, "contains '..'": { []api.Volume{{Name: "containsdots", VolumeSource: containsDots}},