Skip to content

fix: do not inject GIT_USERNAME and GIT_PASSWORD into git clone URL #141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 25, 2024
Merged

fix: do not inject GIT_USERNAME and GIT_PASSWORD into git clone URL #141

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
01a5e4f
refactor: move test auth mw to gittest/
johnstcn Apr 24, 2024
9022f26
update git tests
johnstcn Apr 24, 2024
821fbf8
update integration tests
johnstcn Apr 24, 2024
afa49ab
do not set userpassword in git url
johnstcn Apr 24, 2024
52fa5b4
address PR comments
johnstcn Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions envbuilder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
216 changes: 171 additions & 45 deletions git_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review: trying to cover all cases outlined here:

https://github.com/coder/envbuilder?tab=readme-ov-file#git-authentication

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: "[email protected]",
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about moving this to gittest?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll open a a separate PR to refactor this 👍

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: "[email protected]",
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
}
15 changes: 15 additions & 0 deletions gittest/gittest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}
43 changes: 26 additions & 17 deletions integration/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -756,27 +780,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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review: moved to gittest

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it intended to be used anywhere else in envbuilder? I'm considering moving it to integration/gittest.go

Ok, I found the place. Speaking of consistency with coder/coder that should land in testutil.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do in a follow-up PR 👍

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()
Expand Down
Loading