Merge pull request kubernetes#29837 from thockin/validate-dotdot-in-projections

Automatic merge from submit-queue

Validate .. in projections, fix tests

I noticed this when reviewing another PR.  I fixed it but the test was beyond comprehension, so I fixed that too.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.kubernetes.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.kubernetes.io/reviews/kubernetes/kubernetes/29837)
<!-- Reviewable:end -->

Author: Kubernetes Submit Queue

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
 }
 
@@ -715,22 +737,26 @@ func validateFlockerVolumeSource(flocker *api.FlockerVolumeSource, fldPath *fiel
 	return allErrs
 }
 
-var validDownwardAPIFieldPathExpressions = sets.NewString("metadata.name", "metadata.namespace", "metadata.labels", "metadata.annotations")
+var validDownwardAPIFieldPathExpressions = sets.NewString(
+	"metadata.name",
+	"metadata.namespace",
+	"metadata.labels",
+	"metadata.annotations")
 
 func validateDownwardAPIVolumeSource(downwardAPIVolume *api.DownwardAPIVolumeSource, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
-	for _, downwardAPIVolumeFile := range downwardAPIVolume.Items {
-		if len(downwardAPIVolumeFile.Path) == 0 {
+	for _, file := range downwardAPIVolume.Items {
+		if len(file.Path) == 0 {
 			allErrs = append(allErrs, field.Required(fldPath.Child("path"), ""))
 		}
-		allErrs = append(allErrs, validateVolumeSourcePath(downwardAPIVolumeFile.Path, fldPath.Child("path"))...)
-		if downwardAPIVolumeFile.FieldRef != nil {
-			allErrs = append(allErrs, validateObjectFieldSelector(downwardAPIVolumeFile.FieldRef, &validDownwardAPIFieldPathExpressions, fldPath.Child("fieldRef"))...)
-			if downwardAPIVolumeFile.ResourceFieldRef != nil {
+		allErrs = append(allErrs, validateLocalNonReservedPath(file.Path, fldPath.Child("path"))...)
+		if file.FieldRef != nil {
+			allErrs = append(allErrs, validateObjectFieldSelector(file.FieldRef, &validDownwardAPIFieldPathExpressions, fldPath.Child("fieldRef"))...)
+			if file.ResourceFieldRef != nil {
 				allErrs = append(allErrs, field.Invalid(fldPath, "resource", "fieldRef and resourceFieldRef can not be specified simultaneously"))
 			}
-		} else if downwardAPIVolumeFile.ResourceFieldRef != nil {
-			allErrs = append(allErrs, validateContainerResourceFieldSelector(downwardAPIVolumeFile.ResourceFieldRef, &validContainerResourceFieldPathExpressions, fldPath.Child("resourceFieldRef"), true)...)
+		} else if file.ResourceFieldRef != nil {
+			allErrs = append(allErrs, validateContainerResourceFieldSelector(file.ResourceFieldRef, &validContainerResourceFieldPathExpressions, fldPath.Child("resourceFieldRef"), true)...)
 		} else {
 			allErrs = append(allErrs, field.Required(fldPath, "one of fieldRef and resourceFieldRef is required"))
 		}
@@ -740,44 +766,33 @@ 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 '..'"))
+			break // even for `../../..`, one error is sufficient to make the point
+		}
 	}
 	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 +1277,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 +1933,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")}
 }