From 67b10aad22843d5a06e34ff6dafd1b711366c89c Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Mon, 30 Nov 2020 16:06:13 +0100 Subject: [PATCH] Respect configured user in SSH Git repository URL We had a hardcoded assumption that the SSH user for a Git repository is always "git". This is however not true in all scenarios, for example when one is making use of Gerrit for team code collaboration, as users there have their own username for (SSH) Git operations. This commit changes the logic of the auth strategy helpers to: 1. Select the auth strategy based on the protocol of the parsed URL, instead of a simple rely on a correct prefix. 2. Use the user information from the parsed URL to configure the user for the public key authentication strategy, with a fallback to `git` if none is defined. Signed-off-by: Hidde Beydals --- controllers/gitrepository_controller.go | 10 +++++--- pkg/git/transport.go | 34 +++++++++++++++++-------- pkg/git/transport_test.go | 24 ++++++++++------- 3 files changed, 46 insertions(+), 22 deletions(-) diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index ae9f621d..c45da376 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -184,15 +184,19 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, repository sour // determine auth method var auth transport.AuthMethod - authStrategy := git.AuthSecretStrategyForURL(repository.Spec.URL) - if repository.Spec.SecretRef != nil && authStrategy != nil { + if repository.Spec.SecretRef != nil { + authStrategy, err := git.AuthSecretStrategyForURL(repository.Spec.URL) + if err != nil { + return sourcev1.GitRepositoryNotReady(repository, sourcev1.AuthenticationFailedReason, err.Error()), err + } + name := types.NamespacedName{ Namespace: repository.GetNamespace(), Name: repository.Spec.SecretRef.Name, } var secret corev1.Secret - err := r.Client.Get(ctx, name, &secret) + err = r.Client.Get(ctx, name, &secret) if err != nil { err = fmt.Errorf("auth secret error: %w", err) return sourcev1.GitRepositoryNotReady(repository, sourcev1.AuthenticationFailedReason, err.Error()), err diff --git a/pkg/git/transport.go b/pkg/git/transport.go index b542a454..a298e1f3 100644 --- a/pkg/git/transport.go +++ b/pkg/git/transport.go @@ -18,7 +18,7 @@ package git import ( "fmt" - "strings" + "net/url" "github.com/go-git/go-git/v5/plumbing/transport" "github.com/go-git/go-git/v5/plumbing/transport/http" @@ -28,14 +28,21 @@ import ( "github.com/fluxcd/pkg/ssh/knownhosts" ) -func AuthSecretStrategyForURL(url string) AuthSecretStrategy { - switch { - case strings.HasPrefix(url, "http"): - return &BasicAuth{} - case strings.HasPrefix(url, "ssh"): - return &PublicKeyAuth{} +const defaultPublicKeyAuthUser = "git" + +func AuthSecretStrategyForURL(URL string) (AuthSecretStrategy, error) { + u, err := url.Parse(URL) + if err != nil { + return nil, fmt.Errorf("failed to parse URL to determine auth strategy: %w", err) + } + switch { + case u.Scheme == "http", u.Scheme == "https": + return &BasicAuth{}, nil + case u.Scheme == "ssh": + return &PublicKeyAuth{user: u.User.Username()}, nil + default: + return nil, fmt.Errorf("no auth secret strategy for scheme %s", u.Scheme) } - return nil } type AuthSecretStrategy interface { @@ -58,7 +65,9 @@ func (s *BasicAuth) Method(secret corev1.Secret) (transport.AuthMethod, error) { return auth, nil } -type PublicKeyAuth struct{} +type PublicKeyAuth struct { + user string +} func (s *PublicKeyAuth) Method(secret corev1.Secret) (transport.AuthMethod, error) { identity := secret.Data["identity"] @@ -67,7 +76,12 @@ func (s *PublicKeyAuth) Method(secret corev1.Secret) (transport.AuthMethod, erro return nil, fmt.Errorf("invalid '%s' secret data: required fields 'identity' and 'known_hosts'", secret.Name) } - pk, err := ssh.NewPublicKeys("git", identity, "") + user := s.user + if user == "" { + user = defaultPublicKeyAuthUser + } + + pk, err := ssh.NewPublicKeys(user, identity, "") if err != nil { return nil, err } diff --git a/pkg/git/transport_test.go b/pkg/git/transport_test.go index d77e11fc..a73e34f0 100644 --- a/pkg/git/transport_test.go +++ b/pkg/git/transport_test.go @@ -66,19 +66,25 @@ var ( func TestAuthSecretStrategyForURL(t *testing.T) { tests := []struct { - name string - url string - want AuthSecretStrategy + name string + url string + want AuthSecretStrategy + wantErr bool }{ - {"HTTP", "http://git.example.com/org/repo.git", &BasicAuth{}}, - {"HTTPS", "https://git.example.com/org/repo.git", &BasicAuth{}}, - {"SSH", "ssh://git.example.com:2222/org/repo.git", &PublicKeyAuth{}}, - {"unsupported", "protocol://example.com", nil}, + {"HTTP", "http://git.example.com/org/repo.git", &BasicAuth{}, false}, + {"HTTPS", "https://git.example.com/org/repo.git", &BasicAuth{}, false}, + {"SSH", "ssh://git.example.com:2222/org/repo.git", &PublicKeyAuth{}, false}, + {"SSH with username", "ssh://example@git.example.com:2222/org/repo.git", &PublicKeyAuth{user: "example"}, false}, + {"unsupported", "protocol://example.com", nil, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := AuthSecretStrategyForURL(tt.url) - if reflect.TypeOf(got) != reflect.TypeOf(tt.want) { + got, err := AuthSecretStrategyForURL(tt.url) + if (err != nil) != tt.wantErr { + t.Errorf("AuthSecretStrategyForURL() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { t.Errorf("AuthSecretStrategyForURL() got = %v, want %v", got, tt.want) } })