From 01a5e4f89f13e3a34375234df70407e762b5bfac Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 24 Apr 2024 14:58:27 +0000 Subject: [PATCH 1/5] refactor: move test auth mw to gittest/ --- gittest/gittest.go | 15 +++++++++++++++ integration/integration_test.go | 17 +---------------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/gittest/gittest.go b/gittest/gittest.go index 348862a8..ab2be1f6 100644 --- a/gittest/gittest.go +++ b/gittest/gittest.go @@ -118,3 +118,18 @@ func WriteFile(t *testing.T, fs billy.Filesystem, path, content string) { err = file.Close() require.NoError(t, err) } + +func BasicAuthMW(username, password string) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if username != "" && password != "" { + authUser, authPass, ok := r.BasicAuth() + if !ok || username != authUser || password != authPass { + w.WriteHeader(http.StatusUnauthorized) + return + } + } + next.ServeHTTP(w, r) + }) + } +} diff --git a/integration/integration_test.go b/integration/integration_test.go index ba63ac6f..bafd1c2f 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -756,27 +756,12 @@ type gitServerOptions struct { func createGitServer(t *testing.T, opts gitServerOptions) string { t.Helper() if opts.authMW == nil { - opts.authMW = checkBasicAuth(opts.username, opts.password) + opts.authMW = gittest.BasicAuthMW(opts.username, opts.password) } srv := httptest.NewServer(opts.authMW(createGitHandler(t, opts))) return srv.URL } -func checkBasicAuth(username, password string) func(http.Handler) http.Handler { - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if username != "" && password != "" { - authUser, authPass, ok := r.BasicAuth() - if !ok || username != authUser || password != authPass { - w.WriteHeader(http.StatusUnauthorized) - return - } - } - next.ServeHTTP(w, r) - }) - } -} - func createGitHandler(t *testing.T, opts gitServerOptions) http.Handler { t.Helper() fs := memfs.New() From 9022f26763f435d55d14677690e8c8fddbafbe50 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 24 Apr 2024 16:11:06 +0000 Subject: [PATCH 2/5] update git tests --- git_test.go | 216 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 171 insertions(+), 45 deletions(-) diff --git a/git_test.go b/git_test.go index 2c6dd13e..f2989de0 100644 --- a/git_test.go +++ b/git_test.go @@ -4,72 +4,198 @@ import ( "context" "io" "net/http/httptest" + "net/url" "os" "testing" "time" "github.com/coder/envbuilder" "github.com/coder/envbuilder/gittest" + "github.com/go-git/go-billy/v5" "github.com/go-git/go-billy/v5/memfs" "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing/object" + githttp "github.com/go-git/go-git/v5/plumbing/transport/http" "github.com/stretchr/testify/require" ) func TestCloneRepo(t *testing.T) { t.Parallel() - t.Run("Clones", func(t *testing.T) { - t.Parallel() + for _, tc := range []struct { + name string + srvUsername string + srvPassword string + username string + password string + mungeURL func(*string) + expectError string + expectClone bool + }{ + { + name: "no auth", + expectClone: true, + }, + { + name: "auth", + srvUsername: "user", + srvPassword: "password", + username: "user", + password: "password", + expectClone: true, + }, + { + name: "invalid auth", + srvUsername: "user", + srvPassword: "password", + expectClone: false, + expectError: "authentication required", + }, + { + name: "invalid auth password", + srvUsername: "user", + srvPassword: "password", + username: "user", + password: "", + expectClone: false, + expectError: "authentication required", + }, + { + name: "invalid auth user", + srvUsername: "user", + srvPassword: "password", + username: "", + password: "password", + expectClone: false, + expectError: "authentication required", + }, + { + name: "user only", + srvUsername: "user", + srvPassword: "", + username: "user", + password: "", + expectClone: true, + }, + { + name: "password only", + srvUsername: "", + srvPassword: "password", + username: "", + password: "password", + expectClone: true, + }, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() - serverFS := memfs.New() - repo := gittest.NewRepo(t, serverFS) - tree, err := repo.Worktree() - require.NoError(t, err) + // We do not overwrite a repo if one is already present. + t.Run("AlreadyCloned", func(t *testing.T) { + srvURL := setupGit(t, tc.srvUsername, tc.srvPassword) + clientFS := memfs.New() + // A repo already exists! + _ = gittest.NewRepo(t, clientFS) + cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{ + Path: "/", + RepoURL: srvURL, + Storage: clientFS, + }) + require.NoError(t, err) + require.False(t, cloned) + }) - gittest.WriteFile(t, serverFS, "README.md", "Hello, world!") - _, err = tree.Add("README.md") - require.NoError(t, err) - commit, err := tree.Commit("Wow!", &git.CommitOptions{ - Author: &object.Signature{ - Name: "Example", - Email: "in@tests.com", - When: time.Now(), - }, - }) - require.NoError(t, err) - _, err = repo.CommitObject(commit) - require.NoError(t, err) + // Basic Auth + t.Run("BasicAuth", func(t *testing.T) { + t.Parallel() + srvURL := setupGit(t, tc.srvUsername, tc.srvPassword) + clientFS := memfs.New() - srv := httptest.NewServer(gittest.NewServer(serverFS)) + cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{ + Path: "/workspace", + RepoURL: srvURL, + Storage: clientFS, + RepoAuth: &githttp.BasicAuth{ + Username: tc.username, + Password: tc.password, + }, + }) + require.Equal(t, tc.expectClone, cloned) + if tc.expectError != "" { + require.ErrorContains(t, err, tc.expectError) + return + } + require.NoError(t, err) + require.True(t, cloned) - clientFS := memfs.New() - cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{ - Path: "/workspace", - RepoURL: srv.URL, - Storage: clientFS, - }) - require.NoError(t, err) - require.True(t, cloned) + readme := mustRead(t, clientFS, "/workspace/README.md") + require.Equal(t, "Hello, world!", readme) + gitConfig := mustRead(t, clientFS, "/workspace/.git/config") + require.Contains(t, gitConfig, srvURL) + }) - file, err := clientFS.OpenFile("/workspace/README.md", os.O_RDONLY, 0644) - require.NoError(t, err) - defer file.Close() - content, err := io.ReadAll(file) - require.NoError(t, err) - require.Equal(t, "Hello, world!", string(content)) - }) + // In-URL-style auth e.g. http://user:password@host:port + t.Run("InURL", func(t *testing.T) { + t.Parallel() + srvURL := setupGit(t, tc.srvUsername, tc.srvPassword) + authURL, err := url.Parse(srvURL) + require.NoError(t, err) + authURL.User = url.UserPassword(tc.username, tc.password) + clientFS := memfs.New() - t.Run("DoesntCloneIfRepoExists", func(t *testing.T) { - t.Parallel() - clientFS := memfs.New() - gittest.NewRepo(t, clientFS) - cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{ - Path: "/", - RepoURL: "https://example.com", - Storage: clientFS, + cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{ + Path: "/workspace", + RepoURL: authURL.String(), + Storage: clientFS, + }) + require.Equal(t, tc.expectClone, cloned) + if tc.expectError != "" { + require.ErrorContains(t, err, tc.expectError) + return + } + require.NoError(t, err) + require.True(t, cloned) + + readme := mustRead(t, clientFS, "/workspace/README.md") + require.Equal(t, "Hello, world!", readme) + gitConfig := mustRead(t, clientFS, "/workspace/.git/config") + // We do not modify the git URL that folks pass in. + require.Contains(t, gitConfig, authURL.String()) + }) }) - require.NoError(t, err) - require.False(t, cloned) + } +} + +func mustRead(t *testing.T, fs billy.Filesystem, path string) string { + t.Helper() + f, err := fs.OpenFile(path, os.O_RDONLY, 0644) + require.NoError(t, err) + content, err := io.ReadAll(f) + require.NoError(t, err) + return string(content) +} + +func setupGit(t *testing.T, user, pass string) (url string) { + serverFS := memfs.New() + repo := gittest.NewRepo(t, serverFS) + tree, err := repo.Worktree() + require.NoError(t, err) + + gittest.WriteFile(t, serverFS, "README.md", "Hello, world!") + _, err = tree.Add("README.md") + require.NoError(t, err) + commit, err := tree.Commit("Wow!", &git.CommitOptions{ + Author: &object.Signature{ + Name: "Example", + Email: "in@tests.com", + When: time.Now(), + }, }) + require.NoError(t, err) + _, err = repo.CommitObject(commit) + require.NoError(t, err) + + authMW := gittest.BasicAuthMW(user, pass) + srv := httptest.NewServer(authMW(gittest.NewServer(serverFS))) + return srv.URL } From 821fbf8fb80a485472d97587b3b65fbad9bb64df Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 24 Apr 2024 16:11:18 +0000 Subject: [PATCH 3/5] update integration tests --- integration/integration_test.go | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index bafd1c2f..f6f8d2ea 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -71,13 +71,37 @@ func TestSucceedsGitAuth(t *testing.T) { username: "kyle", password: "testing", }) - _, err := runEnvbuilder(t, options{env: []string{ + ctr, err := runEnvbuilder(t, options{env: []string{ "GIT_URL=" + url, "DOCKERFILE_PATH=Dockerfile", "GIT_USERNAME=kyle", "GIT_PASSWORD=testing", }}) require.NoError(t, err) + gitConfig := execContainer(t, ctr, "cat /workspaces/.git/config") + require.Contains(t, gitConfig, url) +} + +func TestSucceedsGitAuthInURL(t *testing.T) { + t.Parallel() + gitURL := createGitServer(t, gitServerOptions{ + files: map[string]string{ + "Dockerfile": "FROM " + testImageAlpine, + }, + username: "kyle", + password: "testing", + }) + + u, err := url.Parse(gitURL) + require.NoError(t, err) + u.User = url.UserPassword("kyle", "testing") + ctr, err := runEnvbuilder(t, options{env: []string{ + "GIT_URL=" + u.String(), + "DOCKERFILE_PATH=Dockerfile", + }}) + require.NoError(t, err) + gitConfig := execContainer(t, ctr, "cat /workspaces/.git/config") + require.Contains(t, gitConfig, u.String()) } func TestBuildFromDevcontainerWithFeatures(t *testing.T) { From afa49abe420f3f6473a8ec733b085008b269502e Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 24 Apr 2024 16:11:38 +0000 Subject: [PATCH 4/5] do not set userpassword in git url --- envbuilder.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index 5a85f306..28c8a48d 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -361,12 +361,12 @@ func Run(ctx context.Context, options Options) error { } if options.GitUsername != "" || options.GitPassword != "" { - gitURL, err := url.Parse(options.GitURL) - if err != nil { - return fmt.Errorf("parse git url: %w", err) - } - gitURL.User = url.UserPassword(options.GitUsername, options.GitPassword) - options.GitURL = gitURL.String() + // Previously, we had been placing credentials in the URL + // as well as setting githttp.BasicAuth. + // This was removed as it would leak the credentials used + // to clone the repo into the resulting workspace. + // Users may still hard-code credentials directly into the + // git URL themselves, if required. cloneOpts.RepoAuth = &githttp.BasicAuth{ Username: options.GitUsername, From 52fa5b41b30cc724a932a59f54903138b11c9545 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 25 Apr 2024 08:01:00 +0000 Subject: [PATCH 5/5] address PR comments --- envbuilder.go | 9 ++------- git_test.go | 40 +++++++++++++--------------------------- gittest/gittest.go | 2 +- 3 files changed, 16 insertions(+), 35 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index 28c8a48d..bc63f42f 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -361,13 +361,8 @@ func Run(ctx context.Context, options Options) error { } if options.GitUsername != "" || options.GitPassword != "" { - // Previously, we had been placing credentials in the URL - // as well as setting githttp.BasicAuth. - // This was removed as it would leak the credentials used - // to clone the repo into the resulting workspace. - // Users may still hard-code credentials directly into the - // git URL themselves, if required. - + // NOTE: we previously inserted the credentials into the repo URL. + // This was removed in https://github.com/coder/envbuilder/pull/141 cloneOpts.RepoAuth = &githttp.BasicAuth{ Username: options.GitUsername, Password: options.GitPassword, diff --git a/git_test.go b/git_test.go index f2989de0..c4e98281 100644 --- a/git_test.go +++ b/git_test.go @@ -2,10 +2,12 @@ package envbuilder_test import ( "context" + "fmt" "io" "net/http/httptest" "net/url" "os" + "regexp" "testing" "time" @@ -45,46 +47,29 @@ func TestCloneRepo(t *testing.T) { expectClone: true, }, { - name: "invalid auth", + name: "auth but no creds", srvUsername: "user", srvPassword: "password", expectClone: false, expectError: "authentication required", }, { - name: "invalid auth password", - srvUsername: "user", - srvPassword: "password", - username: "user", - password: "", - expectClone: false, - expectError: "authentication required", - }, - { - name: "invalid auth user", + name: "invalid auth", srvUsername: "user", srvPassword: "password", - username: "", - password: "password", + username: "notuser", + password: "notpassword", expectClone: false, expectError: "authentication required", }, { - name: "user only", - srvUsername: "user", + name: "tokenish username", + srvUsername: "tokentokentoken", srvPassword: "", - username: "user", + username: "tokentokentoken", password: "", expectClone: true, }, - { - name: "password only", - srvUsername: "", - srvPassword: "password", - username: "", - password: "password", - expectClone: true, - }, } { tc := tc t.Run(tc.name, func(t *testing.T) { @@ -131,7 +116,8 @@ func TestCloneRepo(t *testing.T) { readme := mustRead(t, clientFS, "/workspace/README.md") require.Equal(t, "Hello, world!", readme) gitConfig := mustRead(t, clientFS, "/workspace/.git/config") - require.Contains(t, gitConfig, srvURL) + // 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(srvURL)), gitConfig) }) // In-URL-style auth e.g. http://user:password@host:port @@ -159,8 +145,8 @@ func TestCloneRepo(t *testing.T) { readme := mustRead(t, clientFS, "/workspace/README.md") require.Equal(t, "Hello, world!", readme) gitConfig := mustRead(t, clientFS, "/workspace/.git/config") - // We do not modify the git URL that folks pass in. - require.Contains(t, gitConfig, authURL.String()) + // 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) }) }) } diff --git a/gittest/gittest.go b/gittest/gittest.go index ab2be1f6..f1ec7a22 100644 --- a/gittest/gittest.go +++ b/gittest/gittest.go @@ -122,7 +122,7 @@ func WriteFile(t *testing.T, fs billy.Filesystem, path, content string) { func BasicAuthMW(username, password string) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if username != "" && password != "" { + if username != "" || password != "" { authUser, authPass, ok := r.BasicAuth() if !ok || username != authUser || password != authPass { w.WriteHeader(http.StatusUnauthorized)