Skip to content

Commit

Permalink
fix: ensure repositories are correctly marked with inherited creds …
Browse files Browse the repository at this point in the history 
…in CLI output (argoproj#13428)

* tests: ensure `InheritedCreds` is propagated via repo API endpoints

Signed-off-by: OneMatchFox <878612+onematchfox@users.noreply.github.com>

* fix: ensure `InheritedCreds` is propagated via repo API endpoints

Signed-off-by: OneMatchFox <878612+onematchfox@users.noreply.github.com>

* tests: add e2e test for `argocd repo get` with inherited credentials

Signed-off-by: OneMatchFox <878612+onematchfox@users.noreply.github.com>

* 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>

---------

Signed-off-by: OneMatchFox <878612+onematchfox@users.noreply.github.com>
  • Loading branch information
@onematchfox
onematchfox authored and xiaowu.zhu committed Aug 9, 2023
1 parent 348c54d commit c3d913e
Show file tree
Hide file tree
Showing 5 changed files with 85 additions and 16 deletions.
12 changes: 5 additions & 7 deletions cmd/argocd/commands/repo.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package commands
import (
"fmt"
"os"
"strconv"
"text/tabwriter"

log "github.com/sirupsen/logrus"
Expand Down Expand Up @@ -250,15 +251,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()
Expand Down
2 changes: 2 additions & 0 deletions server/repository/repository.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,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)
Expand Down Expand Up @@ -196,6 +197,7 @@ func (s *Server) ListRepositories(ctx context.Context, q *repositorypkg.RepoQuer
Proxy: repo.Proxy,
Project: repo.Project,
ForceHttpBasicAuth: repo.ForceHttpBasicAuth,
InheritedCreds: repo.InheritedCreds,
})
}
}
Expand Down
46 changes: 37 additions & 9 deletions server/repository/repository_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,16 @@ var (
},
}
fakeRepo = appsv1.Repository{
Repo: "https://test",
Type: "test",
Name: "test",
Username: "argo",
Insecure: false,
EnableLFS: false,
EnableOCI: false,
Proxy: "test",
Project: "argocd",
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{
Expand Down Expand Up @@ -208,6 +209,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}
Expand Down
7 changes: 7 additions & 0 deletions test/e2e/fixture/fixture.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ type ACL struct {
const (
RepoURLTypeFile = "file"
RepoURLTypeHTTPS = "https"
RepoURLTypeHTTPSOrg = "https-org"
RepoURLTypeHTTPSClientCert = "https-cc"
RepoURLTypeHTTPSSubmodule = "https-sub"
RepoURLTypeHTTPSSubmoduleParent = "https-par"
Expand All @@ -103,6 +104,8 @@ const (
RepoURLTypeHelmOCI = "helm-oci"
GitUsername = "admin"
GitPassword = "password"
GithubAppID = "2978632978"
GithubAppInstallationID = "7893789433789"
GpgGoodKeyID = "D56C4FCA57A46444"
HelmOCIRegistryURL = "localhost:5000/myrepo"
)
Expand Down Expand Up @@ -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"
Expand All @@ -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")
Expand Down
34 changes: 34 additions & 0 deletions test/e2e/repo_management_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)})
Expand Down

0 comments on commit c3d913e

Please sign in to comment.