git: Handle scp-style git addresses in detector, not getter

The "scp-style" remote URL scheme in git is ambiguous with the standard
URL syntax in our previous design where we'd add/accept the ssh://
prefix in both cases; ssh://[email protected]:222/foo could be understood
as either:

 - connect to example.com on port 222 and clone from path "foo"
 - connect to example.com on port 22 and clone from path "222/foo"

Originally we used the first interpretation. but in #129 we switched to
the second in order to support a strange hybrid form where no port number
was specified, but in the process we broke handling of the standard URL
form when a non-standard port is required, as is sometimes the case for
internal git servers running on machines where port 22 is already used for
administrative access.

git itself resolves this ambiguity by not permitting the ssh:// scheme to
be used in conjunction with the scp-like form. Here we mimic that by
supporting the scp-like form _only_ in the detector, and only allowing it
when the ssh:// prefix isn't present. That allows for the following two
scp-like forms to be accepted:

 - [email protected]:foo/bar when the username is "git"
 - git::[email protected]:foo/bar for any other username

We no longer support using the scp-like form with the ssh:// prefix, so
git::ssh://anything.example.com:foo/bar is invalid. To use that form, the
ssh:// part must be omitted, which is consistent with git's own behavior.

The detector already rewrites the scp-like form into the standard URL form,
when no ssh: scheme is given so the getter does not need to do anything
special to deal with it.

In the git getter, we now also check to see if the port number seems to
be non-decimal and return a more actionable error if so, since that is
likely to be caused by someone incorrectly using the ssh:// scheme with
a scp-style address.

README.md

@@ -280,6 +280,16 @@ None
   * `depth` - The Git clone depth. The provided number specifies the last `n`
     revisions to clone from the repository.
 
+
+The `git` getter accepts both URL-style SSH addresses like
+`git::ssh://[email protected]/foo/bar`, and "scp-style" addresses like
+`git::[email protected]/foo/bar`. In the latter case, omitting the `git::`
+force prefix is allowed if the username prefix is exactly `git@`.
+
+The "scp-style" addresses _cannot_ be used in conjunction with the `ssh://`
+scheme prefix, because in that case the colon is used to mark an optional
+port number to connect on, rather than to delimit the path from the host.
+
 ### Mercurial (`hg`)
 
   * `rev` - The Mercurial revision to checkout.

detect_git_test.go

@@ -9,7 +9,10 @@ func TestGitDetector(t *testing.T) {
 		Input  string
 		Output string
 	}{
-		{"[email protected]:hashicorp/foo.git", "git::ssh://[email protected]/hashicorp/foo.git"},
+		{
+			"[email protected]:hashicorp/foo.git",
+			"git::ssh://[email protected]/hashicorp/foo.git",
+		},
 		{
 			"[email protected]:org/project.git?ref=test-branch",
 			"git::ssh://[email protected]/org/project.git?ref=test-branch",
@@ -38,21 +41,35 @@ func TestGitDetector(t *testing.T) {
 			"[email protected]:org/project.git//module/a?ref=test-branch",
 			"git::ssh://[email protected]/org/project.git//module/a?ref=test-branch",
 		},
+		{
+			// Using the scp-like form with the ssh:// prefix is invalid, so
+			// it passes through as-is.
+			"git::ssh://[email protected]:org/project.git",
+			"git::ssh://[email protected]:org/project.git",
+		},
+		{
+			// Already in the canonical form, so no rewriting required
+			// When the ssh: protocol is used explicitly, we recognize it as
+			// URL form rather than SCP-like form, so the part after the colon
+			// is a port number, not part of the path.
+			"git::ssh://[email protected]:2222/hashicorp/foo.git",
+			"git::ssh://[email protected]:2222/hashicorp/foo.git",
+		},
 	}
 
 	pwd := "/pwd"
 	f := new(GitDetector)
-	for i, tc := range cases {
-		output, ok, err := f.Detect(tc.Input, pwd)
-		if err != nil {
-			t.Fatalf("err: %s", err)
-		}
-		if !ok {
-			t.Fatal("not ok")
-		}
+	ds := []Detector{f}
+	for _, tc := range cases {
+		t.Run(tc.Input, func (t *testing.T) {
+			output, err := Detect(tc.Input, pwd, ds)
+			if err != nil {
+				t.Fatalf("unexpected error: %s", err)
+			}
 
-		if output != tc.Output {
-			t.Fatalf("%d: bad: %#v", i, output)
-		}
+			if output != tc.Output {
+				t.Errorf("wrong result\ninput: %s\ngot:   %s\nwant:  %s", tc.Input, output, tc.Output)
+			}
+		})
 	}
 }

get_git.go

@@ -34,6 +34,15 @@ func (g *GitGetter) Get(dst string, u *url.URL) error {
 		return fmt.Errorf("git must be available and on the PATH")
 	}
 
+	// The port number must be parseable as an integer. If not, the user
+	// was probably trying to use a scp-style address, in which case the
+	// ssh:// prefix must be removed to indicate that.
+	if portStr := u.Port(); portStr != "" {
+		if _, err := strconv.ParseUint(portStr, 10, 16); err != nil {
+			return fmt.Errorf("invalid port number %q; if using the \"scp-like\" git address scheme where a colon introduces the path instead, remove the ssh:// portion and use just the git:: prefix", portStr)
+		}
+	}
+
 	// Extract some query parameters we use
 	var ref, sshKey string
 	var depth int
@@ -90,26 +99,6 @@ func (g *GitGetter) Get(dst string, u *url.URL) error {
 		}
 	}
 
-	// For SSH-style URLs, if they use the SCP syntax of host:path, then
-	// the URL will be mangled. We detect that here and correct the path.
-	// Example: host:path/bar will turn into host/path/bar
-	if u.Scheme == "ssh" {
-		if idx := strings.Index(u.Host, ":"); idx > -1 {
-			// Copy the URL so we don't modify the input
-			var newU url.URL = *u
-			u = &newU
-
-			// Path includes the part after the ':'.
-			u.Path = u.Host[idx+1:] + u.Path
-			if u.Path[0] != '/' {
-				u.Path = "/" + u.Path
-			}
-
-			// Host trims up to the :
-			u.Host = u.Host[:idx]
-		}
-	}
-
 	// Clone or update the repository
 	_, err := os.Stat(dst)
 	if err != nil && !os.IsNotExist(err) {

get_git_test.go

@@ -298,6 +298,130 @@ func TestGitGetter_sshKey(t *testing.T) {
 	}
 }
 
+func TestGitGetter_sshSCPStyle(t *testing.T) {
+	if !testHasGit {
+		t.Skip("git not found, skipping")
+	}
+
+	g := new(GitGetter)
+	dst := tempDir(t)
+
+	encodedKey := base64.StdEncoding.EncodeToString([]byte(testGitToken))
+
+	// avoid getting locked by a github authenticity validation prompt
+	os.Setenv("GIT_SSH_COMMAND", "ssh -o StrictHostKeyChecking=no")
+	defer os.Setenv("GIT_SSH_COMMAND", "")
+
+	// This test exercises the combination of the git detector and the
+	// git getter, to make sure that together they make scp-style URLs work.
+	client := &Client{
+		Src: "[email protected]:hashicorp/test-private-repo?sshkey=" + encodedKey,
+		Dst: dst,
+		Pwd: ".",
+
+		Mode: ClientModeDir,
+
+		Detectors: []Detector{
+			new(GitDetector),
+		},
+		Getters: map[string]Getter{
+			"git": g,
+		},
+	}
+
+	if err := client.Get(); err != nil {
+		t.Fatalf("client.Get failed: %s", err)
+	}
+
+	readmePath := filepath.Join(dst, "README.md")
+	if _, err := os.Stat(readmePath); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+}
+
+func TestGitGetter_sshExplicitPort(t *testing.T) {
+	if !testHasGit {
+		t.Skip("git not found, skipping")
+	}
+
+	g := new(GitGetter)
+	dst := tempDir(t)
+
+	encodedKey := base64.StdEncoding.EncodeToString([]byte(testGitToken))
+
+	// avoid getting locked by a github authenticity validation prompt
+	os.Setenv("GIT_SSH_COMMAND", "ssh -o StrictHostKeyChecking=no")
+	defer os.Setenv("GIT_SSH_COMMAND", "")
+
+	// This test exercises the combination of the git detector and the
+	// git getter, to make sure that together they make scp-style URLs work.
+	client := &Client{
+		Src: "git::ssh://[email protected]:22/hashicorp/test-private-repo?sshkey=" + encodedKey,
+		Dst: dst,
+		Pwd: ".",
+
+		Mode: ClientModeDir,
+
+		Detectors: []Detector{
+			new(GitDetector),
+		},
+		Getters: map[string]Getter{
+			"git": g,
+		},
+	}
+
+	if err := client.Get(); err != nil {
+		t.Fatalf("client.Get failed: %s", err)
+	}
+
+	readmePath := filepath.Join(dst, "README.md")
+	if _, err := os.Stat(readmePath); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+}
+
+
+func TestGitGetter_sshSCPStyleInvalidScheme(t *testing.T) {
+	if !testHasGit {
+		t.Skip("git not found, skipping")
+	}
+
+	g := new(GitGetter)
+	dst := tempDir(t)
+
+	encodedKey := base64.StdEncoding.EncodeToString([]byte(testGitToken))
+
+	// avoid getting locked by a github authenticity validation prompt
+	os.Setenv("GIT_SSH_COMMAND", "ssh -o StrictHostKeyChecking=no")
+	defer os.Setenv("GIT_SSH_COMMAND", "")
+
+	// This test exercises the combination of the git detector and the
+	// git getter, to make sure that together they make scp-style URLs work.
+	client := &Client{
+		Src: "git::ssh://[email protected]:hashicorp/test-private-repo?sshkey=" + encodedKey,
+		Dst: dst,
+		Pwd: ".",
+
+		Mode: ClientModeDir,
+
+		Detectors: []Detector{
+			new(GitDetector),
+		},
+		Getters: map[string]Getter{
+			"git": g,
+		},
+	}
+
+	err := client.Get()
+	if err == nil {
+		t.Fatalf("get succeeded; want error")
+	}
+
+	if got, want := err.Error(), `invalid port number "hashicorp"`; !strings.Contains(got, want) {
+		t.Fatalf("wrong error\ngot:  %s\nwant: %s", got, want)
+	}
+}
+
 func TestGitGetter_submodule(t *testing.T) {
 	if !testHasGit {
 		t.Skip("git not found, skipping")