fix: ensure repositories are correctly marked with inherited creds …

…in CLI output (#13428) (#13808) * tests: ensure `InheritedCreds` is propagated via repo API endpoints * fix: ensure `InheritedCreds` is propagated via repo API endpoints * tests: add e2e test for `argocd repo get` with inherited credentials * fix(cli): prioritise value of `InheritedCreds` over `HasCredentials()` Since the API does not return sensitive information `HasCredentials()` will return false for all scenarios except when username/password is used as credentials. Given the current logic this means that the code will never even check `InheritedCreds` resulting in an output of `false` for `CREDS` column (in the case of inherited credentials). Note: There remains a bug in this code in that any repo that has explicit (sensitive) credentials (e.g. SSH private key) will still be displayed as `CREDS = false`. --------- Signed-off-by: OneMatchFox <878612+onematchfox@users.noreply.github.com>
argoproj · May 29, 2023 · 801e195 · 801e195
1 parent deaaf9a
commit 801e195
Show file tree

Hide file tree

Showing 5 changed files with 114 additions and 17 deletions.
diff --git a/cmd/argocd/commands/repo.go b/cmd/argocd/commands/repo.go
@@ -3,6 +3,7 @@ package commands
 import (
 	"fmt"
 	"os"
+	"strconv"
 	"text/tabwriter"
 
 	log "github.com/sirupsen/logrus"
@@ -233,15 +234,12 @@ func printRepoTable(repos appsv1.Repositories) {
 	_, _ = fmt.Fprintf(w, "TYPE\tNAME\tREPO\tINSECURE\tOCI\tLFS\tCREDS\tSTATUS\tMESSAGE\tPROJECT\n")
 	for _, r := range repos {
 		var hasCreds string
-		if !r.HasCredentials() {
-			hasCreds = "false"
+		if r.InheritedCreds {
+			hasCreds = "inherited"
 		} else {
-			if r.InheritedCreds {
-				hasCreds = "inherited"
-			} else {
-				hasCreds = "true"
-			}
+			hasCreds = strconv.FormatBool(r.HasCredentials())
 		}
+
 		_, _ = fmt.Fprintf(w, "%s\t%s\t%s\t%v\t%v\t%v\t%s\t%s\t%s\t%s\n", r.Type, r.Name, r.Repo, r.IsInsecure(), r.EnableOCI, r.EnableLFS, hasCreds, r.ConnectionState.Status, r.ConnectionState.Message, r.Project)
 	}
 	_ = w.Flush()

diff --git a/server/repository/repository.go b/server/repository/repository.go
@@ -5,6 +5,7 @@ import (
 	"reflect"
 
 	"context"
+
 	"github.com/argoproj/gitops-engine/pkg/utils/kube"
 	"github.com/argoproj/gitops-engine/pkg/utils/text"
 	log "github.com/sirupsen/logrus"
@@ -163,6 +164,7 @@ func (s *Server) Get(ctx context.Context, q *repositorypkg.RepoQuery) (*appsv1.R
 		GitHubAppEnterpriseBaseURL: repo.GitHubAppEnterpriseBaseURL,
 		Proxy:                      repo.Proxy,
 		Project:                    repo.Project,
+		InheritedCreds:             repo.InheritedCreds,
 	}
 
 	item.ConnectionState = s.getConnectionState(ctx, item.Repo, q.ForceRefresh)
@@ -186,15 +188,16 @@ func (s *Server) ListRepositories(ctx context.Context, q *repositorypkg.RepoQuer
 			}
 			// remove secrets
 			items = append(items, &appsv1.Repository{
-				Repo:      repo.Repo,
-				Type:      rType,
-				Name:      repo.Name,
-				Username:  repo.Username,
-				Insecure:  repo.IsInsecure(),
-				EnableLFS: repo.EnableLFS,
-				EnableOCI: repo.EnableOCI,
-				Proxy:     repo.Proxy,
-				Project:   repo.Project,
+				Repo:           repo.Repo,
+				Type:           rType,
+				Name:           repo.Name,
+				Username:       repo.Username,
+				Insecure:       repo.IsInsecure(),
+				EnableLFS:      repo.EnableLFS,
+				EnableOCI:      repo.EnableOCI,
+				Proxy:          repo.Proxy,
+				Project:        repo.Project,
+				InheritedCreds: repo.InheritedCreds,
 			})
 		}
 	}

diff --git a/server/repository/repository_test.go b/server/repository/repository_test.go
@@ -87,7 +87,18 @@ var (
 			Destinations: []appsv1.ApplicationDestination{{Server: "*", Namespace: "*"}},
 		},
 	}
-
+	fakeRepo = appsv1.Repository{
+		Repo:           "https://test",
+		Type:           "test",
+		Name:           "test",
+		Username:       "argo",
+		Insecure:       false,
+		EnableLFS:      false,
+		EnableOCI:      false,
+		Proxy:          "test",
+		Project:        "argocd",
+		InheritedCreds: true,
+	}
 	guestbookApp = &appsv1.Application{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "Application",
@@ -196,6 +207,33 @@ func TestRepositoryServer(t *testing.T) {
 		assert.Equal(t, repo.Repo, url)
 	})
 
+	t.Run("Test_GetInherited", func(t *testing.T) {
+		repoServerClient := mocks.RepoServerServiceClient{}
+		repoServerClient.On("TestRepository", mock.Anything, mock.Anything).Return(&apiclient.TestRepositoryResponse{}, nil)
+		repoServerClientset := mocks.Clientset{RepoServerServiceClient: &repoServerClient}
+
+		url := "https://test"
+		db := &dbmocks.ArgoDB{}
+		testRepo := &appsv1.Repository{
+			Repo:           url,
+			Type:           "git",
+			Username:       "foo",
+			InheritedCreds: true,
+		}
+		db.On("GetRepository", context.TODO(), url).Return(testRepo, nil)
+		db.On("RepositoryExists", context.TODO(), url).Return(true, nil)
+
+		s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projInformer, testNamespace, settingsMgr)
+		repo, err := s.Get(context.TODO(), &repository.RepoQuery{
+			Repo: url,
+		})
+		assert.Nil(t, err)
+
+		testRepo.ConnectionState = repo.ConnectionState // overwrite connection state on our test object to simplify comparison below
+
+		assert.Equal(t, testRepo, repo)
+	})
+
 	t.Run("Test_GetWithErrorShouldReturn403", func(t *testing.T) {
 		repoServerClient := mocks.RepoServerServiceClient{}
 		repoServerClientset := mocks.Clientset{RepoServerServiceClient: &repoServerClient}
@@ -279,6 +317,23 @@ func TestRepositoryServer(t *testing.T) {
 		assert.Equal(t, repo.Repo, "test")
 	})
 
+	t.Run("Test_ListRepositories", func(t *testing.T) {
+		repoServerClient := mocks.RepoServerServiceClient{}
+		repoServerClient.On("TestRepository", mock.Anything, mock.Anything).Return(&apiclient.TestRepositoryResponse{}, nil)
+		repoServerClientset := mocks.Clientset{RepoServerServiceClient: &repoServerClient}
+		enforcer := newEnforcer(kubeclientset)
+
+		url := "https://test"
+		db := &dbmocks.ArgoDB{}
+		db.On("GetRepository", context.TODO(), url).Return(nil, nil)
+		db.On("ListHelmRepositories", context.TODO(), mock.Anything).Return(nil, nil)
+		db.On("ListRepositories", context.TODO()).Return([]*appsv1.Repository{&fakeRepo, &fakeRepo}, nil)
+
+		s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projInformer, testNamespace, settingsMgr)
+		resp, err := s.ListRepositories(context.TODO(), &repository.RepoQuery{})
+		assert.NoError(t, err)
+		assert.Equal(t, 2, len(resp.Items))
+	})
 }
 
 func TestRepositoryServerListApps(t *testing.T) {

diff --git a/test/e2e/fixture/fixture.go b/test/e2e/fixture/fixture.go
@@ -92,6 +92,7 @@ type ACL struct {
 const (
 	RepoURLTypeFile                 = "file"
 	RepoURLTypeHTTPS                = "https"
+	RepoURLTypeHTTPSOrg             = "https-org"
 	RepoURLTypeHTTPSClientCert      = "https-cc"
 	RepoURLTypeHTTPSSubmodule       = "https-sub"
 	RepoURLTypeHTTPSSubmoduleParent = "https-par"
@@ -103,6 +104,8 @@ const (
 	RepoURLTypeHelmOCI              = "helm-oci"
 	GitUsername                     = "admin"
 	GitPassword                     = "password"
+	GithubAppID                     = "2978632978"
+	GithubAppInstallationID         = "7893789433789"
 	GpgGoodKeyID                    = "D56C4FCA57A46444"
 	HelmOCIRegistryURL              = "localhost:5000/myrepo"
 )
@@ -251,6 +254,7 @@ const (
 	EnvRepoURLTypeSSHSubmodule         = "ARGOCD_E2E_REPO_SSH_SUBMODULE"
 	EnvRepoURLTypeSSHSubmoduleParent   = "ARGOCD_E2E_REPO_SSH_SUBMODULE_PARENT"
 	EnvRepoURLTypeHTTPS                = "ARGOCD_E2E_REPO_HTTPS"
+	EnvRepoURLTypeHTTPSOrg             = "ARGOCD_E2E_REPO_HTTPS_ORG"
 	EnvRepoURLTypeHTTPSClientCert      = "ARGOCD_E2E_REPO_HTTPS_CLIENT_CERT"
 	EnvRepoURLTypeHTTPSSubmodule       = "ARGOCD_E2E_REPO_HTTPS_SUBMODULE"
 	EnvRepoURLTypeHTTPSSubmoduleParent = "ARGOCD_E2E_REPO_HTTPS_SUBMODULE_PARENT"
@@ -272,6 +276,9 @@ func RepoURL(urlType RepoURLType) string {
 	// Git server via HTTPS
 	case RepoURLTypeHTTPS:
 		return GetEnvWithDefault(EnvRepoURLTypeHTTPS, "https://localhost:9443/argo-e2e/testdata.git")
+	// Git "organisation" via HTTPS
+	case RepoURLTypeHTTPSOrg:
+		return GetEnvWithDefault(EnvRepoURLTypeHTTPSOrg, "https://localhost:9443/argo-e2e")
 	// Git server via HTTPS - Client Cert protected
 	case RepoURLTypeHTTPSClientCert:
 		return GetEnvWithDefault(EnvRepoURLTypeHTTPSClientCert, "https://localhost:9444/argo-e2e/testdata.git")

diff --git a/test/e2e/repo_management_test.go b/test/e2e/repo_management_test.go
@@ -7,9 +7,11 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	repositorypkg "github.com/argoproj/argo-cd/v2/pkg/apiclient/repository"
+	"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 	"github.com/argoproj/argo-cd/v2/test/e2e/fixture"
 	"github.com/argoproj/argo-cd/v2/test/e2e/fixture/app"
 	"github.com/argoproj/argo-cd/v2/test/e2e/fixture/repos"
+	. "github.com/argoproj/argo-cd/v2/util/errors"
 	argoio "github.com/argoproj/argo-cd/v2/util/io"
 	"github.com/argoproj/argo-cd/v2/util/settings"
 )
@@ -52,6 +54,38 @@ func TestAddRemovePublicRepo(t *testing.T) {
 	})
 }
 
+func TestGetRepoWithInheritedCreds(t *testing.T) {
+	app.Given(t).And(func() {
+		// create repo credentials
+		FailOnErr(fixture.RunCli("repocreds", "add", fixture.RepoURL(fixture.RepoURLTypeHTTPSOrg), "--github-app-id", fixture.GithubAppID, "--github-app-installation-id", fixture.GithubAppInstallationID, "--github-app-private-key-path", repos.CertKeyPath))
+
+		repoUrl := fixture.RepoURL(fixture.RepoURLTypeHTTPS)
+
+		// Hack: First we need to create repo with valid credentials
+		FailOnErr(fixture.RunCli("repo", "add", repoUrl, "--username", fixture.GitUsername, "--password", fixture.GitPassword, "--insecure-skip-server-verification"))
+
+		// Then, we remove username/password so that the repo inherits the credentials from our repocreds
+		conn, repoClient, err := fixture.ArgoCDClientset.NewRepoClient()
+		assert.NoError(t, err)
+		defer argoio.Close(conn)
+
+		_, err = repoClient.UpdateRepository(context.Background(), &repositorypkg.RepoUpdateRequest{
+			Repo: &v1alpha1.Repository{
+				Repo: repoUrl,
+			},
+		})
+		assert.NoError(t, err)
+
+		// CLI output should indicate that repo has inherited credentials
+		out, err := fixture.RunCli("repo", "get", repoUrl)
+		assert.NoError(t, err)
+		assert.Contains(t, out, "inherited")
+
+		_, err = fixture.RunCli("repo", "rm", repoUrl)
+		assert.NoError(t, err)
+	})
+}
+
 func TestUpsertExistingRepo(t *testing.T) {
 	app.Given(t).And(func() {
 		fixture.SetRepos(settings.RepositoryCredentials{URL: fixture.RepoURL(fixture.RepoURLTypeFile)})