Merge pull request #64532 from tallclair/gitrepo

Automatic merge from submit-queue (batch tested with PRs 64641, 64532). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Validate that GitRepoVolumeSource parameters are not flags to git

**What this PR does / why we need it**:

Validate that GitRepoVolumeSource parameters are not flags to git, as a mitigation for vulnerabilities in git. See https://groups.google.com/d/msg/kubernetes-security-announce/ayqL4LiUcV4/09HL6e11AgAJ

**Release note**:

```release-note
NONE
```

pkg/volume/git_repo/git_repo.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"path"
+	"path/filepath"
 	"strings"
 
 	"k8s.io/api/core/v1"
@@ -90,6 +91,10 @@ func (plugin *gitRepoPlugin) SupportsBulkVolumeVerification() bool {
 }
 
 func (plugin *gitRepoPlugin) NewMounter(spec *volume.Spec, pod *v1.Pod, opts volume.VolumeOptions) (volume.Mounter, error) {
+	if err := validateVolume(spec.Volume.GitRepo); err != nil {
+		return nil, err
+	}
+
 	return &gitRepoVolumeMounter{
 		gitRepoVolume: &gitRepoVolume{
 			volName: spec.Name(),
@@ -190,7 +195,7 @@ func (b *gitRepoVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 		return err
 	}
 
-	args := []string{"clone", b.source}
+	args := []string{"clone", "--", b.source}
 
 	if len(b.target) != 0 {
 		args = append(args, b.target)
@@ -214,7 +219,7 @@ func (b *gitRepoVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 	var subdir string
 
 	switch {
-	case b.target == ".":
+	case len(b.target) != 0 && filepath.Clean(b.target) == ".":
 		// if target dir is '.', use the current dir
 		subdir = path.Join(dir)
 	case len(files) == 1:
@@ -248,6 +253,19 @@ func (b *gitRepoVolumeMounter) execCommand(command string, args []string, dir st
 	return cmd.CombinedOutput()
 }
 
+func validateVolume(src *v1.GitRepoVolumeSource) error {
+	if err := validateNonFlagArgument(src.Repository, "repository"); err != nil {
+		return err
+	}
+	if err := validateNonFlagArgument(src.Revision, "revision"); err != nil {
+		return err
+	}
+	if err := validateNonFlagArgument(src.Directory, "directory"); err != nil {
+		return err
+	}
+	return nil
+}
+
 // gitRepoVolumeUnmounter cleans git repo volumes.
 type gitRepoVolumeUnmounter struct {
 	*gitRepoVolume
@@ -276,3 +294,10 @@ func getVolumeSource(spec *volume.Spec) (*v1.GitRepoVolumeSource, bool) {
 
 	return volumeSource, readOnly
 }
+
+func validateNonFlagArgument(arg, argName string) error {
+	if len(arg) > 0 && arg[0] == '-' {
+		return fmt.Errorf("%q is an invalid value for %s", arg, argName)
+	}
+	return nil
+}

pkg/volume/git_repo/git_repo_test.go

@@ -93,7 +93,7 @@ func TestPlugin(t *testing.T) {
 			},
 			expecteds: []expectedCommand{
 				{
-					cmd: []string{"git", "clone", gitUrl, "target_dir"},
+					cmd: []string{"git", "clone", "--", gitUrl, "target_dir"},
 					dir: "",
 				},
 				{
@@ -120,7 +120,7 @@ func TestPlugin(t *testing.T) {
 			},
 			expecteds: []expectedCommand{
 				{
-					cmd: []string{"git", "clone", gitUrl, "target_dir"},
+					cmd: []string{"git", "clone", "--", gitUrl, "target_dir"},
 					dir: "",
 				},
 			},
@@ -138,7 +138,7 @@ func TestPlugin(t *testing.T) {
 			},
 			expecteds: []expectedCommand{
 				{
-					cmd: []string{"git", "clone", gitUrl},
+					cmd: []string{"git", "clone", "--", gitUrl},
 					dir: "",
 				},
 			},
@@ -158,7 +158,7 @@ func TestPlugin(t *testing.T) {
 			},
 			expecteds: []expectedCommand{
 				{
-					cmd: []string{"git", "clone", gitUrl},
+					cmd: []string{"git", "clone", "--", gitUrl},
 					dir: "",
 				},
 				{
@@ -186,7 +186,7 @@ func TestPlugin(t *testing.T) {
 			},
 			expecteds: []expectedCommand{
 				{
-					cmd: []string{"git", "clone", gitUrl, "."},
+					cmd: []string{"git", "clone", "--", gitUrl, "."},
 					dir: "",
 				},
 				{
@@ -200,6 +200,72 @@ func TestPlugin(t *testing.T) {
 			},
 			isExpectedFailure: false,
 		},
+		{
+			name: "current-dir-mess",
+			vol: &v1.Volume{
+				Name: "vol1",
+				VolumeSource: v1.VolumeSource{
+					GitRepo: &v1.GitRepoVolumeSource{
+						Repository: gitUrl,
+						Revision:   revision,
+						Directory:  "./.",
+					},
+				},
+			},
+			expecteds: []expectedCommand{
+				{
+					cmd: []string{"git", "clone", "--", gitUrl, "./."},
+					dir: "",
+				},
+				{
+					cmd: []string{"git", "checkout", revision},
+					dir: "",
+				},
+				{
+					cmd: []string{"git", "reset", "--hard"},
+					dir: "",
+				},
+			},
+			isExpectedFailure: false,
+		},
+		{
+			name: "invalid-repository",
+			vol: &v1.Volume{
+				Name: "vol1",
+				VolumeSource: v1.VolumeSource{
+					GitRepo: &v1.GitRepoVolumeSource{
+						Repository: "--foo",
+					},
+				},
+			},
+			isExpectedFailure: true,
+		},
+		{
+			name: "invalid-revision",
+			vol: &v1.Volume{
+				Name: "vol1",
+				VolumeSource: v1.VolumeSource{
+					GitRepo: &v1.GitRepoVolumeSource{
+						Repository: gitUrl,
+						Revision:   "--bar",
+					},
+				},
+			},
+			isExpectedFailure: true,
+		},
+		{
+			name: "invalid-directory",
+			vol: &v1.Volume{
+				Name: "vol1",
+				VolumeSource: v1.VolumeSource{
+					GitRepo: &v1.GitRepoVolumeSource{
+						Repository: gitUrl,
+						Directory:  "-b",
+					},
+				},
+			},
+			isExpectedFailure: true,
+		},
 	}
 
 	for _, scenario := range scenarios {