Add repository credential management API and CLI (addresses #2136) by jannfis · Pull Request #2207 · argoproj/argo-cd

jannfis · 2019-08-23T13:37:15Z

This PR adds repository credential management to ArgoCD, as discussed in #2136. It changes the following:

~~Add the following CRUD operations for repository credentials in RepositoryService API~~ Introduces new RepoCredsService API which implements the following CRUD operations for managing repository credentials:
- ListRepositoryCredentials()
- CreateRepositoryCredentials()
- UpdateRepositoryCredentials()
- DeleteRepositoryCredentials()
Deprecation of the following CRUD operations for repository management in RepositoryService API. The deprecated methods still available for backwards compatibility, but I think should be safe to remove with the release of v2.0:
- List() deprecated in favour of ListRepositories()
- Create() deprecated in favour of CreateRepository()
- Update() deprecated in favour of UpdateRepository()
- Delete() deprecated in favour of DeleteRepository()
~~Changes the semantics of argocd repo sub-commands by introducing the --creds switch to sub-commands list, add and rm to perform operations on repository credentials instead of repositories.~~
Introduces the repocreds sub-command to CLI, which can be used to manage repository credential templates. It mimics repo sub-command to some extent and supports the add, rm and list operations, with --upsert support.
Introduces ~~--force-refresh~~ --refresh <mode> switch to argocd repo list command and accompanyingrefresh field in RepoQuery message of RepositoryService API to force a cache refresh of the listed repository's connection status
Update 08/26/19: Changes behaviour of credential lookup from "first match" to "best match", i.e. given the following two cred URLs https://reposerver/reposand https://reposerver/repos/private, credential lookup for repository https://reposerver/repos/private/myprivate would return credentials configured for https://reposerver/repos/private, not https://reposerver/repos
Update 08/26/19 2: Adds initial functionality to the web UI for listing, adding and removing repository credential templates - all built into the 'Repositories' settings page. Also added ability to force a cache reload of repositories with a new button on top of the page. See screenshots.
Update 08/27/19: Added documentation for Operator Manual (declarative) and User Manual (Web UI and CLI)
Update 09/28/19: Introduce new repocreds sub-command to CLI and removes --creds switch (and accompanying functionality) from repo sub-command (docs still need updating).

CLI examples:

[eris@docker-build argo-cd]$ ./dist/argocd repocreds --list
URL_PATTERN                 USERNAME  SSH_CREDS  TLS_CREDS
https://docker-build/repos  test      false      false
[eris@docker-build argo-cd]$ ./dist/argocd repo list
REPO                                            INSECURE  LFS    USER  STATUS      MESSAGE
https://docker-build/repos/argocd-example-apps  false     false  test  Successful
[eris@docker-build argo-cd]$ ./dist/argocd repocreds rm https://docker-build/repos
[eris@docker-build argo-cd]$ ./dist/argocd repo list --refresh hard
REPO                                            INSECURE  LFS    USER  STATUS  MESSAGE
https://docker-build/repos/argocd-example-apps  false     false  -     Failed  Unable to connect to repository: authentication required

…6-creds-management

alexec · 2019-08-23T23:53:35Z

looking good so far - I've some polish comments - but I'll wait until this is Ready For Review

…ured

…el resource

…6-creds-management

jannfis · 2019-08-27T18:38:53Z

I think this PR is complete now. Sorry for it being so large again, I owe you guys a beer or two for all the review work I generate.

alexec

Another great PR. I've just added some comments as I'm on vacation for a few days now.

cmd/argocd/commands/repo.go

pkg/apis/application/v1alpha1/types.go

server/repository/repository.go

server/repository/repository.proto

util/db/repository.go

cmd/argocd/commands/repo.go

jannfis · 2019-08-29T16:31:49Z

Regarding the removal of insecureIgnoreHostKey from the credential templates, this was a design decision. It has a) not been well documented (no mention of this setting in the docs regarding repository credentials), and b) I see it as a potentially dangerous option, and also one that shouldn't be necessary anymore with the introduction of certificate management. The problem I see with it that it's a "fire & forget" option to open a potential huge security gap in the setup. It should be set using consent for each repository, if at all (i.e. must "hurt").

alexec

The Helm work is now merged. You'll need to git merge master

moensch · 2019-09-13T06:01:17Z

@jannfis Would you consider a reduced version of this PR against the 1.2 branch to just fix the call to CreateRepository with no credentials provided?
I was going to get started on a fix for #2305 but then found your open PR which I suspect won't make it in until 2.0.

…6-creds-management

alexec · 2019-10-03T16:44:41Z

Don't forget to let us know when it is ready for review.

jannfis · 2019-10-03T18:08:11Z

I'm still working on moving the credentials part out of RepositoryService to give the credentials a dedicated place to exist. This is more work than I expected, as I'm also giving it complete new data structures and types, so there'll be less confusion about what's actually a repository and what's a credential template.

jannfis · 2019-10-04T19:12:33Z

I've introduced RepoCredsService with the latest commit, which handles CRUD operations for repository credentials. I have also changed semantics of RepositoryCredentials a little, so we now have a dedicated data structure for handling credential templates, as opposed to handle them using stub repository structures. This always introduced confusion of "what is a repository" and "what is a credential template", and which information should be where.

Final steps are now to adapt the UI to the new semantics, and also to merge the conflicts from master (I hope for the last time).

jannfis · 2019-10-05T20:22:45Z

@alexec I think this is now finally ready for review. Please note that the change is now more invasive than initially was planned, but should be non breaking in terms of configuration - except when someone had InsecureIgnoreHostKey set in repository.credentials configuration (which I doubt, because it was not documented).

The UI still needs a little love, it's a little hackish implementation. I was thinking about moving the credentials part to its own settings page in the future, but for now, the current implementation in the repositories section seems good enough.

alexec

This looks good me. I'd like to target if for v1.4 rather than v1.3 as we're going to branch v1.3 today (hopefully). @alexmt @jessesuen do you want to review the API changes please?

alexec · 2019-10-07T19:27:04Z

cmd/argocd/commands/repocreds.go

+			if sshPrivateKeyPath != "" {
+				if ok, _ := git.IsSSHURL(repo.URL); ok {
+					keyData, err := ioutil.ReadFile(sshPrivateKeyPath)
+					if err != nil {


minor - could use error.Check(..)

alexec · 2019-10-07T19:28:51Z

cmd/argocd/commands/repocreds.go

+// Print the repository credentials as table
+func printRepoCredsTable(repos []appsv1.RepoCreds) {
+	w := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0)
+	fmt.Fprintf(w, "URL_PATTERN\tUSERNAME\tSSH_CREDS\tTLS_CREDS\n")


minor - if you want to provide parseable output, you could add a -o yaml option

alexec · 2019-10-07T19:29:54Z

pkg/apis/application/v1alpha1/types.go

+// RepoCreds holds a repository credentials definition
+type RepoCreds struct {
+	// URL is the URL that this credentials matches to
+	URL string `json:"url" protobuf:"bytes,1,opt,name=url"`


minor - do we want document how these are matched?

alexec · 2019-10-07T19:31:49Z

pkg/apis/application/v1alpha1/types_test.go

+			want: true,
+		},
+	}
+	for _, tt := range tests {


I think you could simply this test to two lines, as any field other than EnableLFSshould not matter

alexec · 2019-10-07T19:33:03Z

server/repocreds/repocreds.go

+	r := q.Creds
+
+	if r.URL == "" {
+		return nil, status.Errorf(codes.InvalidArgument, "must specify URL")


alexec · 2019-10-07T19:34:17Z

server/repository/repository.go

+// repo and evaluate the results. Unless forceRefresh is set to true, the
+// result may be retrieved out of the cache.
+func (s *Server) getConnectionState(ctx context.Context, url string, forceRefresh bool) appsv1.ConnectionState {
+	if !forceRefresh {


Why do we need force refresh? I'm worried that we're using the wrong key for the correction state.

I think it'd be useful when you change repository credentials, you can see whether the change was successful or not instantly. Credentials aren't (can't be) checked for validity.

The other possibility would be to invalidate the cache on connection state for every repository that matches the credentials' URL prefix, when the credentials change.

alexec · 2019-10-07T19:35:06Z

server/repository/repository.go

+		if err != nil {
+			return nil, err
+		}
+		if repoCreds != nil {


I think this func accepts nil? So no need to guard.

alexec · 2019-10-16T23:26:49Z

This has been approved but needs to be sync'd with master. @jannfis do you want to do this?

jannfis added 10 commits August 23, 2019 15:05

Initial support for managing repo credentials via CLI

ffae897

Merge branch 'master' of https://github.com/argoproj/argo-cd into 213…

a97f049

…6-creds-management

Codegen

27718b1

Adapt unit tests to new semantics

106bda6

Adapt comments in repository.proto

4ece5cc

Workaround some race condition in K8s fake clientset

c9968ee

Also copy repository settings when creating

ed5cbb1

Merge branch 'master' of https://github.com/argoproj/argo-cd into 213…

d992570

…6-creds-management

Add test for repo.CopySettingsFrom()

af56b7d

Add tests for repo.IsInsecure() and repo.IsLFSEnabled()

813036b

jannfis added 12 commits August 24, 2019 17:15

Return InheritedCreds property to API client in ListRepositories()

3236790

Change output of repo list to display whether a repo has creds config…

3194caf

…ured

Enable longest match instead of first on repo credential lookup

38d0add

Adapt tests for getRepositoryCredentialIndex() to new semantics

d5e817f

Add and adapt End-to-End tests for credential management and repo access

b2c7d1c

Change REST resource names for credentials to be non-ambigous

45105a5

Even better REST paths for repo creds without introducing new top lev…

2a72546

…el resource

Basic functionality for managing repo credentials via web UI

7c2ce63

Cosmetics

bd85463

Document repository credential management

bae9c1e

Merge branch 'master' of https://github.com/argoproj/argo-cd into 213…

49fe1b2

…6-creds-management

Pre-commit

917f20b

jannfis marked this pull request as ready for review August 27, 2019 18:35

alexec reviewed Aug 27, 2019

View reviewed changes

alexec requested changes Sep 9, 2019

View reviewed changes

moensch mentioned this pull request Sep 13, 2019

Must always supply credentials when adding repo despite common secret being present in repository.credentials #2305

Closed

jannfis added 9 commits September 30, 2019 22:59

Adapt tests to reality

a558ccd

Merge branch 'master' of https://github.com/argoproj/argo-cd into 213…

45f4e0f

…6-creds-management

Codegen

49591bf

Merge branch 'master' of https://github.com/argoproj/argo-cd into 213…

45cabd2

…6-creds-management

Only copy credentials if none were set in source

ffc39f8

Merge branch 'master' of https://github.com/argoproj/argo-cd into 213…

8422853

…6-creds-management

Codegen

a3c69d8

Do not copy repo settings in CopyCredentialsFrom()

ea4ee8b

Merge branch 'master' of https://github.com/argoproj/argo-cd into 213…

12e5181

…6-creds-management

Say hello to RepoCredsService

d727681

jannfis added 5 commits October 4, 2019 22:13

Merge branch 'master' into 2136-creds-management

2de337e

UI implementation fixes and refactoring of service

90058c0

Remove debug statements and outcommented code

d4f4244

Fix data structures

3ee94b9

Correctly refresh creds after creation

533b0c6

alexec self-requested a review October 7, 2019 18:31

alexec self-assigned this Oct 7, 2019

alexec approved these changes Oct 7, 2019

View reviewed changes

alexec added do not merge and removed do not merge labels Oct 7, 2019

alexec added this to the v1.4 milestone Oct 10, 2019

Merge branch 'master' into 2136-creds-management

9584f52

alexec merged commit 37641cf into argoproj:master Oct 17, 2019

danielhelfand mentioned this pull request Apr 15, 2022

cli: remove use of deprecated API calls in argocd repo #9114

Closed

Conversation

jannfis commented Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexec commented Aug 23, 2019

Uh oh!

jannfis commented Aug 27, 2019

Uh oh!

alexec left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jannfis commented Aug 29, 2019

Uh oh!

alexec left a comment

Choose a reason for hiding this comment

Uh oh!

moensch commented Sep 13, 2019

Uh oh!

alexec commented Oct 3, 2019

Uh oh!

jannfis commented Oct 3, 2019

Uh oh!

jannfis commented Oct 4, 2019

Uh oh!

jannfis commented Oct 5, 2019

Uh oh!

alexec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexec commented Oct 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jannfis commented Aug 23, 2019 •

edited

Loading