Simplify server certificate and known hosts management #1807

jannfis · 2019-06-22T17:18:15Z

This PR aims to make it more simple to connect SSH and HTTPS repositories to ArgoCD. It addresses #1514 and the following topics:

Make it more easy to add SSH host keys to the list of known hosts across all ArgoCD instances. This is accomplished by outsourcing the known hosts data into a ConfigMap, which in turn gets mounted as volume to the pods. There is no management interface yet, neither via CLI nor via UI, so to add new host keys one has to edit the ConfigMap (argocd-runtime-cm).
Make it possible to connect HTTPS repositories with self-signed certificates, or those signed by unknown CAs. This PR introduces a new switch, "--insecure-skip-server-validation", which should replace the current "--insecure-ignore-host-key" switch. The current switch is kept for backwards compatibilty, but can possibly be removed.
When adding repositories via CLI, the client now lets the server test the connection instead of performing a connection to the repository itself. The server now exposes a verification endpoint.

A management interface for SSH known hosts management has yet to be introduced, as well as an interface for managing HTTPS certificates.

…4-simplify-known-hosts-mgmt

Also, supply a little more repository info during List(), so that the CLI can display them.

This moves repository validation from the client to server before creating it. It also introduces new --insecure-skip-server-validation flag, which shall deprecate --insecure-ignore-host-key in the future. This new switch shall also reflect that HTTPS certificate validation can be skipped now.

jannfis · 2019-06-22T17:20:47Z

This PR is work in progress. I have yet to write tests and polish the change. And since the change is quite substantial, I want to know your thoughts on the general direction this is going.

jannfis

I explicitly chose not to use a subPath mount for this ConfigMap since with subPath, changes to the ConfigMap do not get propagated to the pods automatically (i.e. you have to restart pods for changes to become effective). Using a non-subPath mount, the pods can pick up the change instantly once the ConfigMap has been propagated across the cluster.

alexec · 2019-06-24T17:47:09Z

looks like you're making good progress - let us know if you need any help

jessesuen · 2019-06-25T07:55:10Z

This PR introduces a new switch, "--insecure-skip-server-validation", which should replace the current "--insecure-ignore-host-key" switch. The current switch is kept for backwards compatibilty, but can possibly be removed.

So is this PR basically munging both the disabling of (a) TLS host certificate verification and (b) SSH host key verification into a single option --insecure-skip-server-validation?

I think I'm on-board with this change, however, I think the flag should be named insecure-skip-server-verification instead of validation.

manifests/ha/install.yaml

manifests/ha/namespace-install.yaml

manifests/install.yaml

jannfis · 2019-06-25T15:40:23Z

So is this PR basically munging both the disabling of (a) TLS host certificate verification and (b) SSH host key verification into a single option --insecure-skip-server-validation?

I think I'm on-board with this change, however, I think the flag should be named insecure-skip-server-verification instead of validation.

@jessesuen Yes, correct, I think it would be best to provide a single option for both cases. I am with you that verification is a better term than validation for this process, I will change the name of the switch and also the code symbols accordingly.

I'm also in the process to extend the API to provide some kind of rudimentary certificate and known hosts management, so that we can attach host keys or TLS information to repository FQDNs. I'm a little unsure where this would belong, I see two possibilities:

a dedicated CertificateService with endpoints at e.g. /api/v1/certificate
an extension of RepositoryService

I think the first would make more sense, since certs/known hosts are valid for multiple repositories, when served from same server.

Third possibilities is one I didn't see yet, but maybe someone else has an idea?

alexec

I'd like to request changes. It wasn't clear to me when reviewing what insecure flags. It appears to combine both flags into one uber flag.

How we deal with SSH and HTTPS is usually quite independent in the code, and we know even experienced users find it confusing what flags to use.

What are the options about having a separate flag named similarly to kubectl? E.g. --insecure-skip-tls-verify?

I also wonder, how should we document updating the list of known hosts?

server/repository/repository.go

alexec · 2019-06-25T15:40:34Z

cmd/argocd/commands/repo.go

 			for _, r := range repos.Items {
-				fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", r.Repo, r.Username, r.ConnectionState.Status, r.ConnectionState.Message)
+				fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\n", r.Repo, strconv.FormatBool((r.InsecureIgnoreHostKey || r.Insecure)), r.Username, r.ConnectionState.Status, r.ConnectionState.Message)


minor - you can use %v

alexec · 2019-06-25T15:42:02Z

server/repository/repository.proto

+// RepoAccessResponse is the response to RepoAccessQuery
+message RepoAccessResponse {
+	// Whether access is possible
+	bool accessPossible = 1;


Are these returned? I can't find mention elsewhere in the PR.

Supposed to be used with the next commits - I think it's better to have a dedicated response rather than to display the raw error to the user, even if it'll contain basically the same message later on.

alexec · 2019-06-25T15:43:31Z

util/git/client.go

+	// explicitly replace it with default client for repositories without the
+	// insecure flag set.
+	if IsHTTPSURL(rawRepoURL) {
+		if insecure {


can we always use this library? it's got to work, so lets just always use it

Sure, it's basically the counterpart to IsSSHURL() - but as of know, just a wrapper around a regexp's MatchString(), and not thoroughly tested yet.

I don't think I was clear, I'd like to avoid the if/else here on line 79. I think you can always use the customHttpClient?

alexec · 2019-06-25T15:51:41Z

Oh - I know I've given a lot of feedback on this, but I think you're doing a splendid job on a tricky problem.

jannfis · 2019-06-25T16:09:35Z

Oh - I know I've given a lot of feedback on this, but I think you're doing a splendid job on a tricky problem.

Your feedback is much appreciated!

jannfis · 2019-07-10T16:25:56Z

This requires some more work than expected. So, apologies, I'm working on a good fix for native Git client supporting custom CA bundle.

…4-simplify-known-hosts-mgmt

jannfis · 2019-07-11T15:38:16Z

Found a good solution that works perfectly and is not dirty by using GIT_SSL_CAINFO environment variable instead of using -c http.sslCAInfo command line option. This was well hidden in the documentation of Git. Also, gotta improve my QA processes ;-)

PR now feature complete and hopefully bug-free. Ready for another review or merge, as you see fit.

alexmt · 2019-07-11T16:55:25Z

Great, I'm glad you found GIT_SSL_CAINFO. Kustomize honors env variable while cloning remote bases, so configured known hosts should work for Kustomize remote bases as well.

Thank you for completing it @jannfis !

jannfis · 2019-07-11T17:45:59Z

I think Kustomize remote bases are not covered by the feature-set in this PR, @alexmt - I had a quick look at the code implementing Kustomize in util/kustomize and imho, it would need adaption. Current solution is focused on Git repositories connected to ArgoCD. However, I think it'd be not that hard to teach our Kustomize implementation to use the certificates and/or known hosts as used by the Git repository connectors. But I guess that's in scope for a next PR, and I already see a next challenge :)

alexec · 2019-07-11T17:45:46Z

assets/builtin-policy.csv

@@ -16,6 +17,9 @@ p, role:admin, applications, update, */*, allow
 p, role:admin, applications, delete, */*, allow
 p, role:admin, applications, sync, */*, allow
 p, role:admin, applications, override, */*, allow
+p, role:admin, certificates, create, *, allow


alexec · 2019-07-11T17:49:16Z

cmd/argocd/commands/cert.go

+			}
+		},
+	}
+	command.Flags().BoolVar(&removeAllCerts, "remove-all", false, "Remove all configured certificates of all types from server (DANGER: use with care!)")


This is a nuke all, I feel like I would never want to have the risk of using this. Also, I initially thought this, incorrectly, this was remove all the certificates for the REPOSERVER - could there be a risk others think the same. How much do we need this?

It was convenient during test cycles, but you are right, it's quite a dangerous option (therefore the fat DANGER sign). However, the same could be accomplished by using a * wildcard as the repo server name, which is not even really documented.

I see two options:

Ask the user with a prompt if he really wants to perform that dangerous operation when --remove-all is specified or * was given as the repo-server name without any other selector such as --cert-type

Discard both possibilities for good, and have the user delete data from ConfigMap objects if they really want to get rid of all certificates.

Thinking of it, sh*t happens too often and I guess variant 2 is the better one. Will remove the code from the CLI. Do you think we should check on server side as well, whether '*' was specified in case someone writes against the API?

alexec · 2019-07-11T17:52:03Z

docs/user-guide/private-repositories.md

+Example for adding a HTTPS repository to ArgoCD without verifying the server's certificate (**Caution:** This is **not** recommended for production use):
+
+```bash
+argocd repo add --insecure-repository https://git.example.com/test-repo


I'm not sure about the name of this flag, it's not the repository that is insecure, it is that we do not check the security of the connection. Hmmmmmm....

In the CLI, it still is --insecure-skip-server-verification. But I guess there is still some disagreement on the name of the switch. --insecure is already occupied for the connection between CLI and ArgoCD server component.

But personally I think, when the connection to the repository is insecure, the repository itself should be considered insecure as well - just for the fact that you don't know if ArgoCD is really connecting to the right repo, and not a MITM-installed one. If the flag is --insecure-repository, it should be clear to not trust that repository. And to not use it for production purposes.

alexec · 2019-07-11T17:54:08Z

server/certificate/certificate.go

+	if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceCertificates, rbacpolicy.ActionGet, ""); err != nil {
+		return nil, err
+	}
+	certList, err := s.db.ListRepoCertificates(ctx, &db.CertificateListSelector{


minor - you could just return certList, err here (same below)

Thanks! Will fix.

alexec · 2019-07-11T17:56:29Z

server/certificate/certificate.proto

+
+service CertificateService {
+  // List all available certificates
+  rpc List(RepositoryCertificateQuery) returns (github.com.argoproj.argo_cd.pkg.apis.application.v1alpha1.RepositoryCertificateList) {


Ok. You need to rename these as they'll get weird swagger operation IDs (something like Mixin5).

I recommend "verb+nou"n names so you get sensible REST IDs.

E.g.

rpc ListCertificates(RepositoryCertificateQuery) rpc CreateCertificate(RepositoryCertificateCreateRequest) rpc DeleteCertificate(RepositoryCertificateQuery)

Ah, that is the reason for the swagger going wild. Thanks for the clarification, will rename those methods to the names you proposed, they sound clear & concise to me.

alexec · 2019-07-11T17:57:26Z

util/cert/cert.go

+
+	certificateList := make([]string, 0)
+
+	// TODO: Implement maximum amount of data to parse


Yes, it's still something that I have to do but deemed not important enough for now. I guess the maximum POST size is already limited by gRPC anyway.

alexec · 2019-07-11T18:00:04Z

util/db/db.go

@@ -37,6 +37,13 @@ type ArgoDB interface {

 	// ListHelmRepoURLs lists configured helm repositories
 	ListHelmRepos(ctx context.Context) ([]*appv1.HelmRepository, error)
+
+	// ListRepoCerticifates lists all configured certificates
+	ListRepoCertificates(ctx context.Context, selector *CertificateListSelector) (*appv1.RepositoryCertificateList, error)


sure, I don't feel strongly about it

alexec · 2019-07-11T18:01:07Z

util/settings/settings.go

-	PasswordSecret        *apiv1.SecretKeySelector `json:"passwordSecret,omitempty"`
-	SSHPrivateKeySecret   *apiv1.SecretKeySelector `json:"sshPrivateKeySecret,omitempty"`
-	InsecureIgnoreHostKey bool                     `json:"insecureIgnoreHostKey,omitempty"`
+	// The URL to the repository


alexec · 2019-07-11T18:01:44Z

util/settings/settings.go

+// Returns the ConfigMap with the given name from the cluster.
+// The ConfigMap must be labeled with "app.kubernetes.io/part-of: argocd" in
+// order to be retrievable.
+func (mgr *SettingsManager) GetNamedConfigMap(configMapName string) (*apiv1.ConfigMap, error) {


minor - all config map are named - aren't they - how about GetConfigMapByName?

Much better, clear & concise. Will rename it, thanks!

alexec · 2019-07-11T18:04:38Z

docs/user-guide/private-repositories.md

+
+If you are connecting a repository on a HTTPS server using a self-signed certificate, or a certificate signed by a custom Certificate Authority (CA) which are not known to ArgoCD, the repository will not be added due to security reasons. This is indicated by an error message such as `x509: certificate signed by unknown authority`.
+
+1. You can let ArgoCD connect the repository in an insecure way, without verifying the server's certificate at all. This can be accomplished by using the `--insecure-repository` flag when adding the repository with the `argocd` CLI utility. However, this should be done only for non-production setups, as it imposes a serious security issue through possible man-in-the-middle attacks.


this is incorrect?

I'm unsure about what you mean. What exactly is incorrect?

I think @alexec means it supposed to be insecure-ignore-host-key instead of --insecure

alexec

Conditional approval - Alex M will be following up.

alexmt · 2019-07-11T23:33:17Z

Trying to implement e2e tests for it

This change addresses some of the remaining minor changes that were requested during development/review cycles of argoproj#1807: - Rename GetNamedConfigMap() to GetConfigMapByName() - Rename CRUD methods of CertificateService to something more sensible, so that swagger doesn't get confused. - Get rid of possibly dangerous --remove-all switch in "cert rm" command, and also prevent user from accidentally specifying a single wildcard as repo-server name

…#1933)

…es (#1934) * Make sure insecure flag works for remote Kustomize bases

jannfis added 15 commits June 22, 2019 16:58

New helper method: IsHTTPSURL()

da766ef

Add support for repos without a valid TLS certificate

9301d24

Initial conversion of ssh_known_hosts to ConfigMap

773548a

New Insecure property in type Repository

e68c859

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

57ce4ba

…4-simplify-known-hosts-mgmt

Add ValidateAccess() for easy server side repo validation

134ebec

Also, supply a little more repository info during List(), so that the CLI can display them.

Add Insecure property to Repository definition

9335882

New API method ValidateAccess()

7111261

Remove keys command

e138f54

Fix lint

449f322

Fix linter

3eb17da

Use the new ConfigMap for SSH known hosts

8cd0ddf

Build image without ssh_known_hosts, instead mount the ConfigMap

02b9b5a

pre-commit files

a995cde

jannfis added 2 commits June 22, 2019 19:30

Update manifest count to reflect new CM manifest

6fe74c6

Use new Insecure flag for Reposerver as well

41c7940

jannfis commented Jun 23, 2019

View reviewed changes

Display logic belongs to frontend, not backend

81892da

basvandenbrink mentioned this pull request Jun 25, 2019

Unable to create application from private SSH repository: key is unknown #1743

Closed

alexec reviewed Jun 25, 2019

View reviewed changes

manifests/ha/install.yaml Show resolved Hide resolved

alexec reviewed Jun 25, 2019

View reviewed changes

manifests/ha/namespace-install.yaml Show resolved Hide resolved

alexec reviewed Jun 25, 2019

View reviewed changes

manifests/install.yaml Show resolved Hide resolved

alexec reviewed Jun 25, 2019

View reviewed changes

jannfis added 5 commits July 10, 2019 23:03

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

7553aee

…4-simplify-known-hosts-mgmt

Use env var for setting Git CAInfo

469433d

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

dacd002

…4-simplify-known-hosts-mgmt

Fix previous merge commit - stupid me

e844858

Update API documentation & codegen

7072d29

alexec requested changes Jul 11, 2019

View reviewed changes

alexec reviewed Jul 11, 2019

View reviewed changes

alexec approved these changes Jul 11, 2019

View reviewed changes

alexec merged commit 9cf744f into argoproj:master Jul 11, 2019

alexmt mentioned this pull request Jul 12, 2019

Argo CD doesn't support a private key with passphrase #1894

Open

jannfis mentioned this pull request Jul 12, 2019

Adress some change requests remaining from #1807 #1931

Merged

alexec pushed a commit that referenced this pull request Jul 12, 2019

Adress some change requests remaining from #1807 (#1931)

22dfc7e

jannfis added a commit to jannfis/argo-cd that referenced this pull request Jul 12, 2019

Update user documentation for argoproj#1807 to reflect current switches

3c30ce3

jannfis added a commit to jannfis/argo-cd that referenced this pull request Jul 12, 2019

Add initial operator's documentation for argoproj#1807

8aeb4aa

alexec pushed a commit that referenced this pull request Jul 12, 2019

Update user and operator documentation for change introduced in #1807 (…

958239f

…#1933)

This was referenced Jul 15, 2019

Add support for Git LFS enabled repositories (fixes #1853) #1941

Merged

[Feature] Allow adding private repositories with Custom CA via UI #1670

Closed

jannfis referenced this pull request Jul 19, 2019

Issues #1513 - Make sure insecure flag works for remote Kustomize bas…

d3c850b

…es (#1934) * Make sure insecure flag works for remote Kustomize bases

This was referenced Jul 19, 2019

Add support for connecting repositories using TLS client certs (fixes #1945) #1960

Merged

Naming of switches on argocd repo add command #1980

Closed

Simplify whitelisting self signed cert of git server #1514

Closed

alexec added this to the v1.2 milestone Jul 24, 2019

jannfis mentioned this pull request Jul 25, 2019

Documentation for upgrading ArgoCD & breaking changes in master/latest #2016

Closed

tksm mentioned this pull request May 16, 2023

feat: add ssh.github.com to SSH known hosts #13592

Merged

13 tasks


		certificateList := make([]string, 0)

		// TODO: Implement maximum amount of data to parse


		If you are connecting a repository on a HTTPS server using a self-signed certificate, or a certificate signed by a custom Certificate Authority (CA) which are not known to ArgoCD, the repository will not be added due to security reasons. This is indicated by an error message such as `x509: certificate signed by unknown authority`.

		1. You can let ArgoCD connect the repository in an insecure way, without verifying the server's certificate at all. This can be accomplished by using the `--insecure-repository` flag when adding the repository with the `argocd` CLI utility. However, this should be done only for non-production setups, as it imposes a serious security issue through possible man-in-the-middle attacks.

Simplify server certificate and known hosts management #1807

Simplify server certificate and known hosts management #1807

Conversation

jannfis commented Jun 22, 2019 • edited Loading

jannfis commented Jun 22, 2019

jannfis left a comment

Choose a reason for hiding this comment

alexec commented Jun 24, 2019

jessesuen commented Jun 25, 2019

jannfis commented Jun 25, 2019

alexec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexec commented Jun 25, 2019

jannfis commented Jun 25, 2019

jannfis commented Jul 10, 2019

jannfis commented Jul 11, 2019

alexmt commented Jul 11, 2019

jannfis commented Jul 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannfis Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexec Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexec left a comment

Choose a reason for hiding this comment

alexmt commented Jul 11, 2019

jannfis commented Jun 22, 2019 •

edited

Loading

jannfis Jul 11, 2019 •

edited

Loading

alexec Jul 11, 2019 •

edited

Loading