From 1fce76535dfaa38c13d49149f99171b506cb32cc Mon Sep 17 00:00:00 2001 From: Jesse Suen Date: Thu, 27 Sep 2018 03:05:43 -0700 Subject: [PATCH 1/2] Switch to go-git for all remote git interactions including auth (issue #651) --- cmd/argocd/commands/repo.go | 2 +- controller/secretcontroller.go | 26 ++--- reposerver/repository/repository.go | 22 ++-- util/git/client.go | 158 ++++++++-------------------- util/git/git.go | 68 +----------- 5 files changed, 65 insertions(+), 211 deletions(-) diff --git a/cmd/argocd/commands/repo.go b/cmd/argocd/commands/repo.go index 3942fb2969ae0..fc35e078057fb 100644 --- a/cmd/argocd/commands/repo.go +++ b/cmd/argocd/commands/repo.go @@ -87,7 +87,7 @@ func NewRepoAddCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command { } command.Flags().StringVar(&repo.Username, "username", "", "username to the repository") command.Flags().StringVar(&repo.Password, "password", "", "password to the repository") - command.Flags().StringVar(&sshPrivateKeyPath, "sshPrivateKeyPath", "", "path to the private ssh key (e.g. ~/.ssh/id_rsa)") + command.Flags().StringVar(&sshPrivateKeyPath, "ssh-private-key-path", "", "path to the private ssh key (e.g. ~/.ssh/id_rsa)") command.Flags().BoolVar(&upsert, "upsert", false, "Override an existing repository with the same name even if the spec differs") return command } diff --git a/controller/secretcontroller.go b/controller/secretcontroller.go index 22d6d94f666b0..fc15e8980b7cf 100644 --- a/controller/secretcontroller.go +++ b/controller/secretcontroller.go @@ -3,16 +3,9 @@ package controller import ( "context" "encoding/json" - "time" - "runtime/debug" + "time" - "github.com/argoproj/argo-cd/common" - "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" - "github.com/argoproj/argo-cd/reposerver" - "github.com/argoproj/argo-cd/reposerver/repository" - "github.com/argoproj/argo-cd/util" - "github.com/argoproj/argo-cd/util/db" log "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -25,6 +18,12 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/cache" "k8s.io/client-go/util/workqueue" + + "github.com/argoproj/argo-cd/common" + "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" + "github.com/argoproj/argo-cd/reposerver" + "github.com/argoproj/argo-cd/util/db" + "github.com/argoproj/argo-cd/util/git" ) type SecretController struct { @@ -93,22 +92,13 @@ func (ctrl *SecretController) getRepoConnectionState(repo *v1alpha1.Repository) ModifiedAt: repo.ConnectionState.ModifiedAt, Status: v1alpha1.ConnectionStatusUnknown, } - closer, client, err := ctrl.repoClientset.NewRepositoryClient() - if err != nil { - log.Errorf("Unable to create repository client: %v", err) - return state - } - - defer util.Close(closer) - - _, err = client.ListDir(context.Background(), &repository.ListDirRequest{Repo: repo, Path: ".gitignore"}) + err := git.TestRepo(repo.Repo, repo.Username, repo.Password, repo.SSHPrivateKey) if err == nil { state.Status = v1alpha1.ConnectionStatusSuccessful } else { state.Status = v1alpha1.ConnectionStatusFailed state.Message = err.Error() } - return state } diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index 36dc2d8b7614c..b05162efd3860 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -76,10 +76,6 @@ func (s *Service) ListDir(ctx context.Context, q *ListDirRequest) (*FileList, er s.repoLock.Lock(gitClient.Root()) defer s.repoLock.Unlock(gitClient.Root()) - err = gitClient.Init() - if err != nil { - return nil, err - } commitSHA, err = checkoutRevision(gitClient, commitSHA) if err != nil { return nil, err @@ -119,10 +115,6 @@ func (s *Service) GetFile(ctx context.Context, q *GetFileRequest) (*GetFileRespo s.repoLock.Lock(gitClient.Root()) defer s.repoLock.Unlock(gitClient.Root()) - err = gitClient.Init() - if err != nil { - return nil, err - } commitSHA, err = checkoutRevision(gitClient, commitSHA) if err != nil { return nil, err @@ -165,10 +157,6 @@ func (s *Service) GenerateManifest(c context.Context, q *ManifestRequest) (*Mani s.repoLock.Lock(gitClient.Root()) defer s.repoLock.Unlock(gitClient.Root()) - err = gitClient.Init() - if err != nil { - return nil, err - } commitSHA, err = checkoutRevision(gitClient, commitSHA) if err != nil { return nil, err @@ -309,13 +297,17 @@ func IdentifyAppSourceTypeByAppPath(appFilePath string) AppSourceType { // checkoutRevision is a convenience function to initialize a repo, fetch, and checkout a revision // Returns the 40 character commit SHA after the checkout has been performed func checkoutRevision(gitClient git.Client, commitSHA string) (string, error) { - err := gitClient.Fetch() + err := gitClient.Init() + if err != nil { + return "", status.Errorf(codes.Internal, "Failed to initialize git repo: %v", err) + } + err = gitClient.Fetch() if err != nil { - return "", err + return "", status.Errorf(codes.Internal, "Failed to fetch git repo: %v", err) } err = gitClient.Checkout(commitSHA) if err != nil { - return "", err + return "", status.Errorf(codes.Internal, "Failed to checkout %s: %v", commitSHA, err) } return gitClient.CommitSHA() } diff --git a/util/git/client.go b/util/git/client.go index ce7e7548d1f34..9f4dc11ffccf4 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -2,11 +2,8 @@ package git import ( "fmt" - "io/ioutil" - "net/url" "os" "os/exec" - "path" "strings" log "github.com/sirupsen/logrus" @@ -29,7 +26,6 @@ type Client interface { LsRemote(revision string) (string, error) LsFiles(path string) ([]string, error) CommitSHA() (string, error) - Reset() error } // ClientFactory is a factory of Git Clients @@ -40,12 +36,9 @@ type ClientFactory interface { // nativeGitClient implements Client interface using git CLI type nativeGitClient struct { - repoURL string - root string - username string - password string - sshPrivateKey string - auth transport.AuthMethod + repoURL string + root string + auth transport.AuthMethod } type factory struct{} @@ -56,11 +49,8 @@ func NewFactory() ClientFactory { func (f *factory) NewClient(repoURL, path, username, password, sshPrivateKey string) (Client, error) { clnt := nativeGitClient{ - repoURL: repoURL, - root: path, - username: username, - password: password, - sshPrivateKey: sshPrivateKey, + repoURL: repoURL, + root: path, } if sshPrivateKey != "" { signer, err := ssh.ParsePrivateKey([]byte(sshPrivateKey)) @@ -83,91 +73,61 @@ func (m *nativeGitClient) Root() string { // Init initializes a local git repository and sets the remote origin func (m *nativeGitClient) Init() error { - var needInit bool - if _, err := os.Stat(m.root); os.IsNotExist(err) { - needInit = true - } else { - _, err = m.runCmd("git", "status") - needInit = err != nil + _, err := git.PlainOpen(m.root) + if err == nil { + return nil } - if needInit { - log.Infof("Initializing %s to %s", m.repoURL, m.root) - _, err := exec.Command("rm", "-rf", m.root).Output() - if err != nil { - return fmt.Errorf("unable to clean repo at %s: %v", m.root, err) - } - err = os.MkdirAll(m.root, 0755) - if err != nil { - return err - } - if _, err := m.runCmd("git", "init"); err != nil { - return err - } - if _, err := m.runCmd("git", "remote", "add", "origin", m.repoURL); err != nil { - return err - } + if err != git.ErrRepositoryNotExists { + return err } - // always set credentials since it can change - err := m.setCredentials() + log.Infof("Initializing %s to %s", m.repoURL, m.root) + _, err = exec.Command("rm", "-rf", m.root).Output() if err != nil { - return err + return fmt.Errorf("unable to clean repo at %s: %v", m.root, err) } - return nil -} - -// setCredentials sets a local credentials file to connect to a remote git repository -func (m *nativeGitClient) setCredentials() error { - if m.password != "" { - log.Debug("Setting password credentials") - gitCredentialsFile := path.Join(m.root, ".git", "credentials") - urlObj, err := url.ParseRequestURI(m.repoURL) - if err != nil { - return err - } - urlObj.User = url.UserPassword(m.username, m.password) - cmdURL := urlObj.String() - err = ioutil.WriteFile(gitCredentialsFile, []byte(cmdURL), 0600) - if err != nil { - return fmt.Errorf("failed to set git credentials: %v", err) - } - _, err = m.runCmd("git", "config", "--local", "credential.helper", fmt.Sprintf("store --file=%s", gitCredentialsFile)) - if err != nil { - return err - } + err = os.MkdirAll(m.root, 0755) + if err != nil { + return err } - if IsSSHURL(m.repoURL) { - sshCmd := gitSSHCommand - if m.sshPrivateKey != "" { - log.Debug("Setting SSH credentials") - sshPrivateKeyFile := path.Join(m.root, ".git", "ssh-private-key") - err := ioutil.WriteFile(sshPrivateKeyFile, []byte(m.sshPrivateKey), 0600) - if err != nil { - return fmt.Errorf("failed to set git credentials: %v", err) - } - sshCmd += " -i " + sshPrivateKeyFile - } - _, err := m.runCmd("git", "config", "--local", "core.sshCommand", sshCmd) - if err != nil { - return err - } + repo, err := git.PlainInit(m.root, false) + if err != nil { + return err } - return nil + _, err = repo.CreateRemote(&config.RemoteConfig{ + Name: git.DefaultRemoteName, + URLs: []string{m.repoURL}, + }) + return err } // Fetch fetches latest updates from origin func (m *nativeGitClient) Fetch() error { - var err error log.Debugf("Fetching repo %s at %s", m.repoURL, m.root) - if _, err = m.runCmd("git", "fetch", "origin", "--tags", "--force"); err != nil { + repo, err := git.PlainOpen(m.root) + if err != nil { return err } + log.Debug("git fetch origin --tags --force") + err = repo.Fetch(&git.FetchOptions{ + RemoteName: git.DefaultRemoteName, + Auth: m.auth, + Tags: git.AllTags, + Force: true, + }) + if err == git.NoErrAlreadyUpToDate { + return nil + } + return err // git fetch does not update the HEAD reference. The following command will update the local // knowledge of what remote considers the “default branch” // See: https://stackoverflow.com/questions/8839958/how-does-origin-head-get-set - if _, err := m.runCmd("git", "remote", "set-head", "origin", "-a"); err != nil { - return err - } - return nil + // NOTE(jessesuen): disabling the following code because: + // 1. we no longer perform a `git checkout HEAD`, instead relying on `ls-remote` and checking + // out a specific SHA1. + // 2. This command is the only other command that we use (excluding fetch/ls-remote) which + // requires remote access, and there appears to be no go-git equivalent to this command. + // _, err = m.runCmd("git", "remote", "set-head", "origin", "-a") + // return err } // LsFiles lists the local working tree, including only files that are under source control @@ -181,34 +141,6 @@ func (m *nativeGitClient) LsFiles(path string) ([]string, error) { return ss[:len(ss)-1], nil } -// Reset resets local changes in a repository -func (m *nativeGitClient) Reset() error { - if _, err := m.runCmd("git", "reset", "--hard", "origin/HEAD"); err != nil { - return err - } - // Delete all local branches (we must first detach so we are not checked out a branch we are about to delete) - if _, err := m.runCmd("git", "checkout", "--detach", "origin/HEAD"); err != nil { - return err - } - branchesOut, err := m.runCmd("git", "for-each-ref", "--format=%(refname:short)", "refs/heads/") - if err != nil { - return err - } - branchesOut = strings.TrimSpace(branchesOut) - if branchesOut != "" { - branches := strings.Split(branchesOut, "\n") - args := []string{"branch", "-D"} - args = append(args, branches...) - if _, err = m.runCmd("git", args...); err != nil { - return err - } - } - if _, err := m.runCmd("git", "clean", "-fd"); err != nil { - return err - } - return nil -} - // Checkout checkout specified git sha func (m *nativeGitClient) Checkout(revision string) error { if revision == "" || revision == "HEAD" { @@ -310,6 +242,8 @@ func (m *nativeGitClient) runCmd(command string, args ...string) (string, error) log.Debug(strings.Join(cmd.Args, " ")) cmd.Dir = m.root env := os.Environ() + env = append(env, "HOME=/dev/null") + env = append(env, "GIT_CONFIG_NOSYSTEM=true") env = append(env, "GIT_ASKPASS=") cmd.Env = env out, err := cmd.Output() diff --git a/util/git/git.go b/util/git/git.go index 11e9f49f605db..7cd7393fd9c3e 100644 --- a/util/git/git.go +++ b/util/git/git.go @@ -1,12 +1,7 @@ package git import ( - "errors" - "fmt" - "io/ioutil" "net/url" - "os" - "os/exec" "regexp" "strings" ) @@ -67,69 +62,12 @@ func IsSSHURL(url string) bool { return strings.HasPrefix(url, "git@") || strings.HasPrefix(url, "ssh://") } -const gitSSHCommand = "ssh -q -F /dev/null -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o ConnectTimeout=20" - -//TODO: Make sure every public method works with '*' repo - -// GetGitCommandEnvAndURL returns URL and env options for git operation -func GetGitCommandEnvAndURL(repo, username, password string, sshPrivateKey string) (string, []string, error) { - cmdURL := repo - env := os.Environ() - if IsSSHURL(repo) { - sshCmd := gitSSHCommand - if sshPrivateKey != "" { - sshFile, err := ioutil.TempFile("", "") - if err != nil { - return "", nil, err - } - _, err = sshFile.WriteString(sshPrivateKey) - if err != nil { - return "", nil, err - } - err = sshFile.Close() - if err != nil { - return "", nil, err - } - sshCmd += " -i " + sshFile.Name() - } - env = append(env, fmt.Sprintf("GIT_SSH_COMMAND=%s", sshCmd)) - } else { - env = append(env, "GIT_ASKPASS=") - repoURL, err := url.ParseRequestURI(repo) - if err != nil { - return "", nil, err - } - - repoURL.User = url.UserPassword(username, password) - cmdURL = repoURL.String() - } - return cmdURL, env, nil -} - // TestRepo tests if a repo exists and is accessible with the given credentials func TestRepo(repo, username, password string, sshPrivateKey string) error { - repo, env, err := GetGitCommandEnvAndURL(repo, username, password, sshPrivateKey) + clnt, err := NewFactory().NewClient(repo, "", username, password, sshPrivateKey) if err != nil { return err } - cmd := exec.Command("git", "ls-remote", repo, "HEAD") - cmd.Env = env - _, err = cmd.Output() - if err != nil { - if exErr, ok := err.(*exec.ExitError); ok { - errOutput := strings.Split(string(exErr.Stderr), "\n")[0] - errOutput = fmt.Sprintf("%s: %s", repo, errOutput) - return errors.New(redactPassword(errOutput, password)) - } - return err - } - return nil -} - -func redactPassword(msg string, password string) string { - if password != "" { - passwordRegexp := regexp.MustCompile("\\b" + regexp.QuoteMeta(password) + "\\b") - msg = passwordRegexp.ReplaceAllString(msg, "*****") - } - return msg + _, err = clnt.LsRemote("HEAD") + return err } From 8997037e20ff71c3dbbea0581682f7cfb1b284a4 Mon Sep 17 00:00:00 2001 From: Jesse Suen Date: Thu, 27 Sep 2018 03:42:19 -0700 Subject: [PATCH 2/2] Fix issue where argocd-server logged credentials in plain text during repo add (issue #653) --- server/server.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/server.go b/server/server.go index f44b817884836..00677fefd3b33 100644 --- a/server/server.go +++ b/server/server.go @@ -367,6 +367,8 @@ func (a *ArgoCDServer) newGRPCServer() *grpc.Server { sensitiveMethods := map[string]bool{ "/session.SessionService/Create": true, "/account.AccountService/UpdatePassword": true, + "/repository.RepositoryService/Create": true, + "/repository.RepositoryService/Update": true, } // NOTE: notice we do not configure the gRPC server here with TLS (e.g. grpc.Creds(creds)) // This is because TLS handshaking occurs in cmux handling