From 72a6fd00a47afc40f2e3030341223e8a2fc10c17 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 27 Nov 2024 12:21:55 +0100 Subject: [PATCH] Rename backwards compatible credentials pattern --- git/client.go | 25 +++++++++++++------------ git/client_test.go | 16 ++++++++-------- pkg/cmd/pr/checkout/checkout.go | 2 +- pkg/cmd/repo/create/create.go | 2 +- pkg/cmd/repo/sync/git.go | 2 +- 5 files changed, 24 insertions(+), 23 deletions(-) diff --git a/git/client.go b/git/client.go index 439199a46..b2cfbce45 100644 --- a/git/client.go +++ b/git/client.go @@ -101,18 +101,19 @@ func (c *Client) Command(ctx context.Context, args ...string) (*Command, error) // as opposed to using a string which might compile with `client.AuthenticatedCommand(ctx, "fetch")`. // // It is only usable when constructed by another function in the package because the empty pattern, -// without insecure set to true, will result in an error in AuthenticatedCommand. +// without allMatching set to true, will result in an error in AuthenticatedCommand. // -// Callers can currently opt-in to an insecure mode for backwards compatibility by using -// InsecureAllMatchingCredentialsPattern. +// Callers can currently opt-in to an slightly less secure mode for backwards compatibility by using +// AllMatchingCredentialsPattern. type CredentialPattern struct { - insecure bool // should only be constructable via InsecureAllMatchingCredentialPattern - pattern string + allMatching bool // should only be constructable via AllMatchingCredentialsPattern + pattern string } -// InsecureAllMatchingCredentialsPattern allows for opting in to an insecure mode for backwards compatibility. -var InsecureAllMatchingCredentialsPattern = CredentialPattern{insecure: true, pattern: ""} -var disallowedCredentialPattern = CredentialPattern{insecure: false, pattern: ""} +// AllMatchingCredentialsPattern allows for setting gh as credential helper for all hosts. +// However, we should endeavour to remove it as it's less secure. +var AllMatchingCredentialsPattern = CredentialPattern{allMatching: true, pattern: ""} +var disallowedCredentialPattern = CredentialPattern{allMatching: false, pattern: ""} // WM-TODO: Are there any funny remotes that might not resolve to a URL? func CredentialPatternFromRemote(ctx context.Context, c *Client, remote string) (CredentialPattern, error) { @@ -145,7 +146,7 @@ func (c *Client) AuthenticatedCommand(ctx context.Context, credentialPattern Cre var preArgs []string if credentialPattern == disallowedCredentialPattern { return nil, fmt.Errorf("empty credential pattern is not allowed unless provided explicitly") - } else if credentialPattern == InsecureAllMatchingCredentialsPattern { + } else if credentialPattern == AllMatchingCredentialsPattern { preArgs = []string{"-c", "credential.helper="} preArgs = append(preArgs, "-c", fmt.Sprintf("credential.helper=%s", credHelper)) } else { @@ -611,7 +612,7 @@ func (c *Client) Fetch(ctx context.Context, remote string, refspec string, mods if refspec != "" { args = append(args, refspec) } - cmd, err := c.AuthenticatedCommand(ctx, InsecureAllMatchingCredentialsPattern, args...) + cmd, err := c.AuthenticatedCommand(ctx, AllMatchingCredentialsPattern, args...) if err != nil { return err } @@ -626,7 +627,7 @@ func (c *Client) Pull(ctx context.Context, remote, branch string, mods ...Comman if remote != "" && branch != "" { args = append(args, remote, branch) } - cmd, err := c.AuthenticatedCommand(ctx, InsecureAllMatchingCredentialsPattern, args...) + cmd, err := c.AuthenticatedCommand(ctx, AllMatchingCredentialsPattern, args...) if err != nil { return err } @@ -638,7 +639,7 @@ func (c *Client) Pull(ctx context.Context, remote, branch string, mods ...Comman func (c *Client) Push(ctx context.Context, remote string, ref string, mods ...CommandModifier) error { args := []string{"push", "--set-upstream", remote, ref} - cmd, err := c.AuthenticatedCommand(ctx, InsecureAllMatchingCredentialsPattern, args...) + cmd, err := c.AuthenticatedCommand(ctx, AllMatchingCredentialsPattern, args...) if err != nil { return err } diff --git a/git/client_test.go b/git/client_test.go index 4ba6f0843..f643caa90 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -71,7 +71,7 @@ func TestClientAuthenticatedCommand(t *testing.T) { { name: "when credential pattern is TODO, credential helper matches everything", path: "path/to/gh", - pattern: InsecureAllMatchingCredentialsPattern, + pattern: AllMatchingCredentialsPattern, wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"path/to/gh" auth git-credential`, "fetch"}, }, { @@ -82,12 +82,12 @@ func TestClientAuthenticatedCommand(t *testing.T) { }, { name: "fallback when GhPath is not set", - pattern: InsecureAllMatchingCredentialsPattern, + pattern: AllMatchingCredentialsPattern, wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"gh" auth git-credential`, "fetch"}, }, { - name: "errors when attempting to use an empty pattern that isn't marked insecure", - pattern: CredentialPattern{insecure: false, pattern: ""}, + name: "errors when attempting to use an empty pattern that isn't marked all matching", + pattern: CredentialPattern{allMatching: false, pattern: ""}, wantErr: fmt.Errorf("empty credential pattern is not allowed unless provided explicitly"), }, } @@ -1565,8 +1565,8 @@ func TestCredentialPatternFromGitURL(t *testing.T) { name: "Given a well formed gitURL, it returns the corresponding CredentialPattern", gitURL: "https://github.com/OWNER/REPO", wantCredentialPattern: CredentialPattern{ - pattern: "https://github.com", - insecure: false, + pattern: "https://github.com", + allMatching: false, }, }, { @@ -1601,8 +1601,8 @@ func TestCredentialPatternFromRemote(t *testing.T) { name: "Given a well formed remote, it returns the corresponding CredentialPattern", remote: "https://github.com/OWNER/REPO", wantCredentialPattern: CredentialPattern{ - pattern: "https://github.com", - insecure: false, + pattern: "https://github.com", + allMatching: false, }, }, { diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index d7243af60..c4549d89b 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -255,7 +255,7 @@ func executeCmds(client *git.Client, credentialPattern git.CredentialPattern, cm case "submodule": cmd, err = client.AuthenticatedCommand(context.Background(), credentialPattern, args...) case "fetch": - cmd, err = client.AuthenticatedCommand(context.Background(), git.InsecureAllMatchingCredentialsPattern, args...) + cmd, err = client.AuthenticatedCommand(context.Background(), git.AllMatchingCredentialsPattern, args...) default: cmd, err = client.Command(context.Background(), args...) } diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index c732a3759..ec7026759 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -676,7 +676,7 @@ func createFromLocal(opts *CreateOptions) error { } if opts.Push && repoType == bare { - cmd, err := opts.GitClient.AuthenticatedCommand(context.Background(), git.InsecureAllMatchingCredentialsPattern, "push", baseRemote, "--mirror") + cmd, err := opts.GitClient.AuthenticatedCommand(context.Background(), git.AllMatchingCredentialsPattern, "push", baseRemote, "--mirror") if err != nil { return err } diff --git a/pkg/cmd/repo/sync/git.go b/pkg/cmd/repo/sync/git.go index b0bd26abd..0a0be1ed6 100644 --- a/pkg/cmd/repo/sync/git.go +++ b/pkg/cmd/repo/sync/git.go @@ -55,7 +55,7 @@ func (g *gitExecuter) CurrentBranch() (string, error) { func (g *gitExecuter) Fetch(remote, ref string) error { args := []string{"fetch", "-q", remote, ref} - cmd, err := g.client.AuthenticatedCommand(context.Background(), git.InsecureAllMatchingCredentialsPattern, args...) + cmd, err := g.client.AuthenticatedCommand(context.Background(), git.AllMatchingCredentialsPattern, args...) if err != nil { return err }