diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2020-08-04 22:01:24 +0300 |
---|---|---|
committer | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2020-08-04 22:01:24 +0300 |
commit | e00006824131e613881a28abed24d2f1cf8c7db2 (patch) | |
tree | cca08f5f6e003edfd5d4140758bee70a7e74479c | |
parent | 0fb7caf0703e3f78085054c367dcba9f640140de (diff) |
repository: do not persist config when creating from URL
When creating a new repository from URL, we currently execute the
equivalent of `git clone -c $CFG1`. There's a slight gotcha
here in that `git clone -c` will persist the configuration into the new
repo's gitconfig file, while `git -c $CFG1 clone` does not persist the
configuration to disk. There's two parts we're thus currently persisting
to disk with one being "http.followRedirects" and the other one being
"http.$URL.extraHeader". While the former one doesn't hurt much (but is
not required to be persisted), the extra header is used to pass along
credentials to the remote. As a result, we accidentally persist user
credentials to disk in an unexpected way.
Fix the issue by instead passing all configuration options as global
configuration parameters to `git` instead of passing them to `git
clone`.
-rw-r--r-- | changelogs/unreleased/security-pks-create-from-url-creds.yml | 5 | ||||
-rw-r--r-- | internal/service/repository/create_from_url.go | 13 | ||||
-rw-r--r-- | internal/service/repository/create_from_url_test.go | 2 |
3 files changed, 14 insertions, 6 deletions
diff --git a/changelogs/unreleased/security-pks-create-from-url-creds.yml b/changelogs/unreleased/security-pks-create-from-url-creds.yml new file mode 100644 index 000000000..392d0a731 --- /dev/null +++ b/changelogs/unreleased/security-pks-create-from-url-creds.yml @@ -0,0 +1,5 @@ +--- +title: Fix injection of arbitrary `http.*` options +merge_request: +author: +type: security diff --git a/internal/service/repository/create_from_url.go b/internal/service/repository/create_from_url.go index 57cb89946..db76a4e25 100644 --- a/internal/service/repository/create_from_url.go +++ b/internal/service/repository/create_from_url.go @@ -23,10 +23,13 @@ func cloneFromURLCommand(ctx context.Context, repoURL, repositoryFullPath string return nil, helper.ErrInternal(err) } - flags := []git.Option{ + globalFlags := []git.Option{ + git.ValueFlag{Name: "-c", Value: "http.followRedirects=false"}, + } + + cloneFlags := []git.Option{ git.Flag{Name: "--bare"}, git.Flag{Name: "--quiet"}, - git.ValueFlag{Name: "-c", Value: "http.followRedirects=false"}, } if u.User != nil { @@ -41,12 +44,12 @@ func cloneFromURLCommand(ctx context.Context, repoURL, repositoryFullPath string u.User = nil authHeader := fmt.Sprintf("Authorization: Basic %s", base64.StdEncoding.EncodeToString([]byte(creds))) - flags = append(flags, git.ValueFlag{Name: "-c", Value: fmt.Sprintf("http.%s.extraHeader=%s", u.String(), authHeader)}) + globalFlags = append(globalFlags, git.ValueFlag{Name: "-c", Value: fmt.Sprintf("http.extraHeader=%s", authHeader)}) } - return git.SafeBareCmd(ctx, git.CmdStream{Err: stderr}, nil, nil, git.SubCmd{ + return git.SafeBareCmd(ctx, git.CmdStream{Err: stderr}, nil, globalFlags, git.SubCmd{ Name: "clone", - Flags: flags, + Flags: cloneFlags, PostSepArgs: []string{u.String(), repositoryFullPath}, }) } diff --git a/internal/service/repository/create_from_url_test.go b/internal/service/repository/create_from_url_test.go index 526c2cf64..b08737e38 100644 --- a/internal/service/repository/create_from_url_test.go +++ b/internal/service/repository/create_from_url_test.go @@ -77,7 +77,7 @@ func TestCloneRepositoryFromUrlCommand(t *testing.T) { expectedScrubbedURL := "https://www.example.com/secretrepo.git" expectedBasicAuthHeader := fmt.Sprintf("Authorization: Basic %s", base64.StdEncoding.EncodeToString([]byte("user:pass!?@"))) - expectedHeader := fmt.Sprintf("http.%s.extraHeader=%s", expectedScrubbedURL, expectedBasicAuthHeader) + expectedHeader := fmt.Sprintf("http.extraHeader=%s", expectedBasicAuthHeader) var args = cmd.Args() require.Contains(t, args, expectedScrubbedURL) |