fix: improve git URL parsing (#486)

* Removes the dependency on git-urls for git URL parsing/validation
* Uses go-git's transport.NewEndpoint instead, with some additional massaging to keep our current behaviour in line.
* Fixes the SSH server implementation in gittest that was causing us to be unable to properly test SSH clones.
* Fixes another test flake I ran into in log.

Author: johnstcn

docs/git-auth.md

@@ -1,4 +1,27 @@
-# Git Authentication
+# Supported URL Formats
+
+Envbuilder supports three distinct types of Git URLs:
+
+1) Valid URLs with scheme (e.g. `https://user:password@host.tld:12345/path/to/repo`)
+2) SCP-like URLs (e.g. `git@host.tld:path/to/repo.git`)
+3) Filesystem URLs (require the `git` executable to be available)
+
+Based on the type of URL, one of two authentication methods will be used:
+
+| Git URL format          | GIT_USERNAME | GIT_PASSWORD | Auth Method |
+| ------------------------|--------------|--------------|-------------|
+| https?://host.tld/repo  | Not Set      | Not Set      | None        |
+| https?://host.tld/repo  | Not Set      | Set          | HTTP Basic  |
+| https?://host.tld/repo  | Set          | Not Set      | HTTP Basic  |
+| https?://host.tld/repo  | Set          | Set          | HTTP Basic  |
+| ssh://host.tld/repo     | -            | -            | SSH         |
+| git://host.tld/repo     | -            | -            | SSH         |
+| file://path/to/repo     | -            | -            | None        |
+| path/to/repo            | -            | -            | None        |
+| user@host.tld:path/repo | -            | -            | SSH         |
+| All other formats       | -            | -            | SSH         |
+
+# Authentication Methods
 
 Two methods of authentication are supported:
 

git/git.go

@@ -10,9 +10,9 @@ import (
 	"os"
 	"strings"
 
+	"github.com/coder/envbuilder/internal/ebutil"
 	"github.com/coder/envbuilder/options"
 
-	giturls "github.com/chainguard-dev/git-urls"
 	"github.com/go-git/go-billy/v5"
 	"github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/plumbing"
@@ -49,18 +49,17 @@ type CloneRepoOptions struct {
 //
 // The bool returned states whether the repository was cloned or not.
 func CloneRepo(ctx context.Context, logf func(string, ...any), opts CloneRepoOptions) (bool, error) {
-	parsed, err := giturls.Parse(opts.RepoURL)
+	parsed, err := ebutil.ParseRepoURL(opts.RepoURL)
 	if err != nil {
 		return false, fmt.Errorf("parse url %q: %w", opts.RepoURL, err)
 	}
-	logf("Parsed Git URL as %q", parsed.Redacted())
 
 	thinPack := true
 
 	if !opts.ThinPack {
 		thinPack = false
 		logf("ThinPack options is false, Marking thin-pack as unsupported")
-	} else if parsed.Hostname() == "dev.azure.com" {
+	} else if parsed.Host == "dev.azure.com" {
 		// Azure DevOps requires capabilities multi_ack / multi_ack_detailed,
 		// which are not fully implemented and by default are included in
 		// transport.UnsupportedCapabilities.
@@ -92,12 +91,9 @@ func CloneRepo(ctx context.Context, logf func(string, ...any), opts CloneRepoOpt
 	if err != nil {
 		return false, fmt.Errorf("mkdir %q: %w", opts.Path, err)
 	}
-	reference := parsed.Fragment
-	if reference == "" && opts.SingleBranch {
-		reference = "refs/heads/main"
+	if parsed.Reference == "" && opts.SingleBranch {
+		parsed.Reference = "refs/heads/main"
 	}
-	parsed.RawFragment = ""
-	parsed.Fragment = ""
 	fs, err := opts.Storage.Chroot(opts.Path)
 	if err != nil {
 		return false, fmt.Errorf("chroot %q: %w", opts.Path, err)
@@ -120,10 +116,10 @@ func CloneRepo(ctx context.Context, logf func(string, ...any), opts CloneRepoOpt
 	}
 
 	_, err = git.CloneContext(ctx, gitStorage, fs, &git.CloneOptions{
-		URL:             parsed.String(),
+		URL:             parsed.Cleaned,
 		Auth:            opts.RepoAuth,
 		Progress:        opts.Progress,
-		ReferenceName:   plumbing.ReferenceName(reference),
+		ReferenceName:   plumbing.ReferenceName(parsed.Reference),
 		InsecureSkipTLS: opts.Insecure,
 		Depth:           opts.Depth,
 		SingleBranch:    opts.SingleBranch,
@@ -245,18 +241,23 @@ func LogHostKeyCallback(logger func(string, ...any)) gossh.HostKeyCallback {
 // If SSH_KNOWN_HOSTS is not set, the SSH auth method will be configured
 // to accept and log all host keys. Otherwise, host key checking will be
 // performed as usual.
+//
+// Git URL formats may only consist of the following:
+//  1. A valid URL with a scheme
+//  2. An SCP-like URL (e.g. git@host.tld:path/to/repo.git)
+//  3. Local filesystem paths (require `git` executable)
 func SetupRepoAuth(logf func(string, ...any), options *options.Options) transport.AuthMethod {
 	if options.GitURL == "" {
 		logf("❔ No Git URL supplied!")
 		return nil
 	}
-	parsedURL, err := giturls.Parse(options.GitURL)
+	parsedURL, err := ebutil.ParseRepoURL(options.GitURL)
 	if err != nil {
 		logf("❌ Failed to parse Git URL: %s", err.Error())
 		return nil
 	}
 
-	if parsedURL.Scheme == "http" || parsedURL.Scheme == "https" {
+	if parsedURL.Protocol == "http" || parsedURL.Protocol == "https" {
 		// Special case: no auth
 		if options.GitUsername == "" && options.GitPassword == "" {
 			logf("👤 Using no authentication!")
@@ -272,7 +273,7 @@ func SetupRepoAuth(logf func(string, ...any), options *options.Options) transpor
 		}
 	}
 
-	if parsedURL.Scheme == "file" {
+	if parsedURL.Protocol == "file" {
 		// go-git will try to fallback to using the `git` command for local
 		// filesystem clones. However, it's more likely than not that the
 		// `git` command is not present in the container image. Log a warning

git/git_test.go

@@ -4,13 +4,12 @@ import (
 	"context"
 	"crypto/ed25519"
 	"encoding/base64"
-	"fmt"
 	"io"
 	"net/http/httptest"
 	"net/url"
 	"os"
 	"path/filepath"
-	"regexp"
+	"strings"
 	"testing"
 
 	"github.com/coder/envbuilder/git"
@@ -76,6 +75,18 @@ func TestCloneRepo(t *testing.T) {
 			password:    "",
 			expectClone: true,
 		},
+		{
+			name: "whitespace in URL",
+			mungeURL: func(u *string) {
+				if u == nil {
+					t.Errorf("expected non-nil URL")
+					return
+				}
+				*u = "  " + *u + "  "
+				t.Logf("munged URL: %q", *u)
+			},
+			expectClone: true,
+		},
 	} {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
@@ -87,6 +98,9 @@ func TestCloneRepo(t *testing.T) {
 				_ = gittest.NewRepo(t, srvFS, gittest.Commit(t, "README.md", "Hello, world!", "Wow!"))
 				authMW := mwtest.BasicAuthMW(tc.srvUsername, tc.srvPassword)
 				srv := httptest.NewServer(authMW(gittest.NewServer(srvFS)))
+				if tc.mungeURL != nil {
+					tc.mungeURL(&srv.URL)
+				}
 				clientFS := memfs.New()
 				// A repo already exists!
 				_ = gittest.NewRepo(t, clientFS)
@@ -106,6 +120,9 @@ func TestCloneRepo(t *testing.T) {
 				_ = gittest.NewRepo(t, srvFS, gittest.Commit(t, "README.md", "Hello, world!", "Wow!"))
 				authMW := mwtest.BasicAuthMW(tc.srvUsername, tc.srvPassword)
 				srv := httptest.NewServer(authMW(gittest.NewServer(srvFS)))
+				if tc.mungeURL != nil {
+					tc.mungeURL(&srv.URL)
+				}
 				clientFS := memfs.New()
 
 				cloned, err := git.CloneRepo(context.Background(), t.Logf, git.CloneRepoOptions{
@@ -129,7 +146,7 @@ func TestCloneRepo(t *testing.T) {
 				require.Equal(t, "Hello, world!", readme)
 				gitConfig := mustRead(t, clientFS, "/workspace/.git/config")
 				// Ensure we do not modify the git URL that folks pass in.
-				require.Regexp(t, fmt.Sprintf(`(?m)^\s+url\s+=\s+%s\s*$`, regexp.QuoteMeta(srv.URL)), gitConfig)
+				require.Contains(t, gitConfig, strings.TrimSpace(srv.URL))
 			})
 
 			// In-URL-style auth e.g. http://user:password@host:port
@@ -139,15 +156,19 @@ func TestCloneRepo(t *testing.T) {
 				_ = gittest.NewRepo(t, srvFS, gittest.Commit(t, "README.md", "Hello, world!", "Wow!"))
 				authMW := mwtest.BasicAuthMW(tc.srvUsername, tc.srvPassword)
 				srv := httptest.NewServer(authMW(gittest.NewServer(srvFS)))
-
 				authURL, err := url.Parse(srv.URL)
 				require.NoError(t, err)
 				authURL.User = url.UserPassword(tc.username, tc.password)
+				cloneURL := authURL.String()
+				if tc.mungeURL != nil {
+					tc.mungeURL(&cloneURL)
+				}
+
 				clientFS := memfs.New()
 
 				cloned, err := git.CloneRepo(context.Background(), t.Logf, git.CloneRepoOptions{
 					Path:    "/workspace",
-					RepoURL: authURL.String(),
+					RepoURL: cloneURL,
 					Storage: clientFS,
 				})
 				require.Equal(t, tc.expectClone, cloned)
@@ -162,7 +183,7 @@ func TestCloneRepo(t *testing.T) {
 				require.Equal(t, "Hello, world!", readme)
 				gitConfig := mustRead(t, clientFS, "/workspace/.git/config")
 				// Ensure we do not modify the git URL that folks pass in.
-				require.Regexp(t, fmt.Sprintf(`(?m)^\s+url\s+=\s+%s\s*$`, regexp.QuoteMeta(authURL.String())), gitConfig)
+				require.Contains(t, gitConfig, strings.TrimSpace(cloneURL))
 			})
 		})
 	}
@@ -238,10 +259,9 @@ func TestShallowCloneRepo(t *testing.T) {
 func TestCloneRepoSSH(t *testing.T) {
 	t.Parallel()
 
-	t.Run("AuthSuccess", func(t *testing.T) {
+	t.Run("Success", func(t *testing.T) {
 		t.Parallel()
 
-		// TODO: test the rest of the cloning flow. This just tests successful auth.
 		tmpDir := t.TempDir()
 		srvFS := osfs.New(tmpDir, osfs.WithChrootOS())
 
@@ -264,10 +284,9 @@ func TestCloneRepoSSH(t *testing.T) {
 				},
 			},
 		})
-		// TODO: ideally, we want to test the entire cloning flow.
-		// For now, this indicates successful ssh key auth.
-		require.ErrorContains(t, err, "repository not found")
-		require.False(t, cloned)
+		require.NoError(t, err)
+		require.True(t, cloned)
+		require.Equal(t, "Hello, world!", mustRead(t, clientFS, "/workspace/README.md"))
 	})
 
 	t.Run("AuthFailure", func(t *testing.T) {
@@ -399,12 +418,12 @@ func TestSetupRepoAuth(t *testing.T) {
 		// Anything that is not https:// or http:// is treated as SSH.
 		kPath := writeTestPrivateKey(t)
 		opts := &options.Options{
-			GitURL:               "git://git@host.tld:repo/path",
+			GitURL:               "git://git@host.tld:12345/path",
 			GitSSHPrivateKeyPath: kPath,
 		}
 		auth := git.SetupRepoAuth(t.Logf, opts)
 		_, ok := auth.(*gitssh.PublicKeys)
-		require.True(t, ok)
+		require.True(t, ok, "expected SSH auth for git:// URL")
 	})
 
 	t.Run("SSH/GitUsername", func(t *testing.T) {
@@ -422,7 +441,7 @@ func TestSetupRepoAuth(t *testing.T) {
 	t.Run("SSH/PrivateKey", func(t *testing.T) {
 		kPath := writeTestPrivateKey(t)
 		opts := &options.Options{
-			GitURL:               "ssh://git@host.tld:repo/path",
+			GitURL:               "ssh://git@host.tld/repo/path",
 			GitSSHPrivateKeyPath: kPath,
 		}
 		auth := git.SetupRepoAuth(t.Logf, opts)
@@ -436,7 +455,7 @@ func TestSetupRepoAuth(t *testing.T) {
 
 	t.Run("SSH/Base64PrivateKey", func(t *testing.T) {
 		opts := &options.Options{
-			GitURL:                 "ssh://git@host.tld:repo/path",
+			GitURL:                 "ssh://git@host.tld/repo/path",
 			GitSSHPrivateKeyBase64: base64EncodeTestPrivateKey(),
 		}
 		auth := git.SetupRepoAuth(t.Logf, opts)
@@ -452,7 +471,7 @@ func TestSetupRepoAuth(t *testing.T) {
 
 	t.Run("SSH/NoAuthMethods", func(t *testing.T) {
 		opts := &options.Options{
-			GitURL: "ssh://git@host.tld:repo/path",
+			GitURL: "git@host.tld:repo/path",
 		}
 		auth := git.SetupRepoAuth(t.Logf, opts)
 		require.Nil(t, auth) // TODO: actually test SSH_AUTH_SOCK
@@ -481,6 +500,28 @@ func TestSetupRepoAuth(t *testing.T) {
 		auth := git.SetupRepoAuth(t.Logf, opts)
 		require.Nil(t, auth)
 	})
+
+	t.Run("Whitespace", func(t *testing.T) {
+		kPath := writeTestPrivateKey(t)
+		opts := &options.Options{
+			GitURL:               "ssh://git@host.tld/repo path",
+			GitSSHPrivateKeyPath: kPath,
+		}
+		auth := git.SetupRepoAuth(t.Logf, opts)
+		_, ok := auth.(*gitssh.PublicKeys)
+		require.True(t, ok)
+	})
+
+	t.Run("LeadingTrailingWhitespace", func(t *testing.T) {
+		kPath := writeTestPrivateKey(t)
+		opts := &options.Options{
+			GitURL:               " ssh://git@host.tld/repo/path ",
+			GitSSHPrivateKeyPath: kPath,
+		}
+		auth := git.SetupRepoAuth(t.Logf, opts)
+		_, ok := auth.(*gitssh.PublicKeys)
+		require.True(t, ok)
+	})
 }
 
 func mustRead(t *testing.T, fs billy.Filesystem, path string) string {

go.mod

@@ -13,7 +13,6 @@ require (
 	cdr.dev/slog v1.6.2-0.20240126064726-20367d4aede6
 	github.com/GoogleContainerTools/kaniko v1.9.2
 	github.com/breml/rootcerts v0.2.10
-	github.com/chainguard-dev/git-urls v1.0.2
 	github.com/coder/coder/v2 v2.10.1-0.20240704130443-c2d44d16a352
 	github.com/coder/retry v1.5.1
 	github.com/coder/serpent v0.8.0

go.sum

@@ -152,8 +152,6 @@ github.com/certifi/gocertifi v0.0.0-20200922220541-2c3bb06c6054/go.mod h1:sGbDF6
 github.com/cespare/xxhash/v2 v2.1.1/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
 github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44=
 github.com/cespare/xxhash/v2 v2.2.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
-github.com/chainguard-dev/git-urls v1.0.2 h1:pSpT7ifrpc5X55n4aTTm7FFUE+ZQHKiqpiwNkJrVcKQ=
-github.com/chainguard-dev/git-urls v1.0.2/go.mod h1:rbGgj10OS7UgZlbzdUQIQpT0k/D4+An04HJY7Ol+Y/o=
 github.com/charmbracelet/lipgloss v0.8.0 h1:IS00fk4XAHcf8uZKc3eHeMUTCxUH6NkaTrdyCQk84RU=
 github.com/charmbracelet/lipgloss v0.8.0/go.mod h1:p4eYUZZJ/0oXTuCQKFF8mqyKCz0ja6y+7DniDDw5KKU=
 github.com/chenzhuoyu/base64x v0.0.0-20230717121745-296ad89f973d h1:77cEq6EriyTZ0g/qfRdp61a3Uu/AWrgIq2s0ClJV1g0=

integration/integration_test.go

@@ -434,14 +434,16 @@ func TestGitSSHAuth(t *testing.T) {
 		_ = gittest.NewRepo(t, srvFS, gittest.Commit(t, "Dockerfile", "FROM "+testImageAlpine, "Initial commit"))
 		tr := gittest.NewServerSSH(t, srvFS, signer.PublicKey())
 
-		_, err = runEnvbuilder(t, runOpts{env: []string{
+		ctr, err := runEnvbuilder(t, runOpts{env: []string{
 			envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"),
-			envbuilderEnv("GIT_URL", tr.String()+"."),
+			envbuilderEnv("GIT_URL", tr.String()),
 			envbuilderEnv("GIT_SSH_PRIVATE_KEY_BASE64", base64Key),
 		}})
-		// TODO: Ensure it actually clones but this does mean we have
-		// successfully authenticated.
-		require.ErrorContains(t, err, "repository not found")
+		require.NoError(t, err)
+		dockerFilePath := execContainer(t, ctr, "find /workspaces -name Dockerfile")
+		require.NotEmpty(t, dockerFilePath)
+		dockerFile := execContainer(t, ctr, "cat "+dockerFilePath)
+		require.Contains(t, dockerFile, testImageAlpine)
 	})
 
 	t.Run("Base64/Failure", func(t *testing.T) {