diff --git a/Gopkg.lock b/Gopkg.lock index 1b8765549b..e0b151ff91 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -1646,6 +1646,7 @@ "github.com/yuin/gopher-lua", "golang.org/x/crypto/bcrypt", "golang.org/x/crypto/ssh", + "golang.org/x/crypto/ssh/knownhosts", "golang.org/x/crypto/ssh/terminal", "golang.org/x/net/context", "golang.org/x/oauth2", diff --git a/Procfile b/Procfile index 54712beb83..863c1dac41 100644 --- a/Procfile +++ b/Procfile @@ -2,6 +2,6 @@ controller: sh -c "FORCE_LOG_COLORS=1 ARGOCD_FAKE_IN_CLUSTER=true go run ./cmd/a api-server: sh -c "FORCE_LOG_COLORS=1 ARGOCD_FAKE_IN_CLUSTER=true go run ./cmd/argocd-server/main.go --loglevel debug --redis localhost:${ARGOCD_E2E_REDIS_PORT:-6379} --disable-auth --insecure --dex-server http://localhost:${ARGOCD_E2E_DEX_PORT:-5556} --repo-server localhost:${ARGOCD_E2E_REPOSERVER_PORT:-8081} --port ${ARGOCD_E2E_APISERVER_PORT:-8080} --staticassets ui/dist/app" dex: sh -c "go run ./cmd/argocd-util/main.go gendexcfg -o `pwd`/dist/dex.yaml && docker run --rm -p ${ARGOCD_E2E_DEX_PORT:-5556}:${ARGOCD_E2E_DEX_PORT:-5556} -v `pwd`/dist/dex.yaml:/dex.yaml quay.io/dexidp/dex:v2.14.0 serve /dex.yaml" redis: docker run --rm --name argocd-redis -i -p ${ARGOCD_E2E_REDIS_PORT:-6379}:${ARGOCD_E2E_REDIS_PORT:-6379} redis:5.0.3-alpine --save "" --appendonly no --port ${ARGOCD_E2E_REDIS_PORT:-6379} -repo-server: sh -c "FORCE_LOG_COLORS=1 go run ./cmd/argocd-repo-server/main.go --loglevel debug --port ${ARGOCD_E2E_REPOSERVER_PORT:-8081} --redis localhost:${ARGOCD_E2E_REDIS_PORT:-6379}" +repo-server: sh -c "FORCE_LOG_COLORS=1 ARGOCD_FAKE_IN_CLUSTER=true go run ./cmd/argocd-repo-server/main.go --loglevel debug --port ${ARGOCD_E2E_REPOSERVER_PORT:-8081} --redis localhost:${ARGOCD_E2E_REDIS_PORT:-6379}" ui: sh -c 'cd ui && ${ARGOCD_E2E_YARN_CMD:-yarn} start' -git-server: test/fixture/testrepos/start-git.sh \ No newline at end of file +git-server: test/fixture/testrepos/start-git.sh diff --git a/assets/swagger.json b/assets/swagger.json index 84c325769b..80ce79b827 100644 --- a/assets/swagger.json +++ b/assets/swagger.json @@ -49,7 +49,7 @@ "ApplicationService" ], "summary": "List returns list of applications", - "operationId": "ListMixin6", + "operationId": "ListMixin1", "parameters": [ { "type": "string", @@ -89,7 +89,7 @@ "ApplicationService" ], "summary": "Create creates an application", - "operationId": "CreateMixin6", + "operationId": "CreateMixin1", "parameters": [ { "name": "body", @@ -116,7 +116,7 @@ "ApplicationService" ], "summary": "Update updates an application", - "operationId": "UpdateMixin6", + "operationId": "UpdateMixin1", "parameters": [ { "type": "string", @@ -197,7 +197,7 @@ "ApplicationService" ], "summary": "Get returns an application by name", - "operationId": "GetMixin6", + "operationId": "Get", "parameters": [ { "type": "string", @@ -238,7 +238,7 @@ "ApplicationService" ], "summary": "Delete deletes an application", - "operationId": "DeleteMixin6", + "operationId": "DeleteMixin1", "parameters": [ { "type": "string", @@ -852,7 +852,7 @@ "ClusterService" ], "summary": "List returns list of clusters", - "operationId": "List", + "operationId": "ListMixin3", "parameters": [ { "type": "string", @@ -874,7 +874,7 @@ "ClusterService" ], "summary": "Create creates a cluster", - "operationId": "Create", + "operationId": "CreateMixin3", "parameters": [ { "name": "body", @@ -901,7 +901,7 @@ "ClusterService" ], "summary": "Update updates a cluster", - "operationId": "Update", + "operationId": "UpdateMixin3", "parameters": [ { "type": "string", @@ -934,7 +934,7 @@ "ClusterService" ], "summary": "Get returns a cluster by server address", - "operationId": "GetMixin1", + "operationId": "GetMixin3", "parameters": [ { "type": "string", @@ -957,7 +957,7 @@ "ClusterService" ], "summary": "Delete deletes a cluster", - "operationId": "Delete", + "operationId": "DeleteMixin3", "parameters": [ { "type": "string", @@ -1239,7 +1239,7 @@ "RepositoryService" ], "summary": "List returns list of repos", - "operationId": "ListMixin2", + "operationId": "List", "parameters": [ { "type": "string", @@ -1261,7 +1261,7 @@ "RepositoryService" ], "summary": "Create creates a repo", - "operationId": "CreateMixin2", + "operationId": "Create", "parameters": [ { "name": "body", @@ -1288,7 +1288,7 @@ "RepositoryService" ], "summary": "Update updates a repo", - "operationId": "UpdateMixin2", + "operationId": "Update", "parameters": [ { "type": "string", @@ -1321,7 +1321,7 @@ "RepositoryService" ], "summary": "Delete deletes a repo", - "operationId": "DeleteMixin2", + "operationId": "Delete", "parameters": [ { "type": "string", @@ -1500,7 +1500,7 @@ "SettingsService" ], "summary": "Get returns Argo CD settings", - "operationId": "Get", + "operationId": "GetMixin5", "responses": { "200": { "description": "(empty)", diff --git a/test/e2e/fixture/app/context.go b/test/e2e/fixture/app/context.go index d86ab860b3..9c16231be8 100644 --- a/test/e2e/fixture/app/context.go +++ b/test/e2e/fixture/app/context.go @@ -43,6 +43,11 @@ func (c *Context) CustomCACertAdded() *Context { return c } +func (c *Context) CustomSSHKnownHostsAdded() *Context { + certs.AddCustomSSHKnownHostsKeys() + return c +} + func (c *Context) HTTPSRepoURLAdded() *Context { repos.AddHTTPSRepo(false) return c diff --git a/test/e2e/fixture/certs/certs.go b/test/e2e/fixture/certs/certs.go index c0e31a5373..b2843b6cc9 100644 --- a/test/e2e/fixture/certs/certs.go +++ b/test/e2e/fixture/certs/certs.go @@ -1,12 +1,15 @@ package certs import ( + "io/ioutil" "path/filepath" "github.com/argoproj/argo-cd/errors" "github.com/argoproj/argo-cd/test/e2e/fixture" ) +// Add a custom CA certificate to the test and also create the certificate file +// on the file system, so argocd-server and argocd-repo-server can use it. func AddCustomCACert() { caCertPath, err := filepath.Abs("../fixture/certs/argocd-test-ca.crt") errors.CheckError(err) @@ -14,4 +17,23 @@ func AddCustomCACert() { errors.FailOnErr(fixture.RunCli(args...)) args = []string{"cert", "add-tls", "127.0.0.1", "--from", caCertPath} errors.FailOnErr(fixture.RunCli(args...)) + + certData, err := ioutil.ReadFile(caCertPath) + errors.CheckError(err) + err = ioutil.WriteFile(fixture.TmpDir+"/app/config/tls/localhost", certData, 0644) + errors.CheckError(err) + err = ioutil.WriteFile(fixture.TmpDir+"/app/config/tls/127.0.0.1", certData, 0644) + errors.CheckError(err) +} + +func AddCustomSSHKnownHostsKeys() { + knownHostsPath, err := filepath.Abs("../fixture/certs/ssh_known_hosts") + errors.CheckError(err) + args := []string{"cert", "add-ssh", "--batch", "--from", knownHostsPath} + errors.FailOnErr(fixture.RunCli(args...)) + + knownHostsData, err := ioutil.ReadFile(knownHostsPath) + errors.CheckError(err) + err = ioutil.WriteFile(fixture.TmpDir+"/app/config/ssh/ssh_known_hosts", knownHostsData, 0644) + errors.CheckError(err) } diff --git a/test/e2e/fixture/fixture.go b/test/e2e/fixture/fixture.go index adbd2fb2ef..23f047c72d 100644 --- a/test/e2e/fixture/fixture.go +++ b/test/e2e/fixture/fixture.go @@ -40,7 +40,7 @@ const ( ArgoCDNamespace = "argocd-e2e" // ensure all repos are in one directory tree, so we can easily clean them up - tmpDir = "/tmp/argo-e2e" + TmpDir = "/tmp/argo-e2e" repoDir = "testdata.git" GuestbookPath = "guestbook" @@ -127,7 +127,7 @@ func Name() string { } func repoDirectory() string { - return path.Join(tmpDir, repoDir) + return path.Join(TmpDir, repoDir) } func RepoURL(urlType RepoURLType) string { @@ -336,7 +336,7 @@ func EnsureCleanState(t *testing.T) { SetTLSCerts() // remove tmp dir - CheckError(os.RemoveAll(tmpDir)) + CheckError(os.RemoveAll(TmpDir)) // name based on test name name = dnsFriendly(t.Name()) @@ -344,11 +344,11 @@ func EnsureCleanState(t *testing.T) { id = name + "-" + strings.ToLower(rand.RandString(5)) // create tmp dir - FailOnErr(Run("", "mkdir", "-p", tmpDir)) + FailOnErr(Run("", "mkdir", "-p", TmpDir)) // create TLS and SSH certificate directories - FailOnErr(Run("", "mkdir", "-p", tmpDir+"/app/config/tls")) - FailOnErr(Run("", "mkdir", "-p", tmpDir+"/app/config/ssh")) + FailOnErr(Run("", "mkdir", "-p", TmpDir+"/app/config/tls")) + FailOnErr(Run("", "mkdir", "-p", TmpDir+"/app/config/ssh")) // set-up tmp repo, must have unique name FailOnErr(Run("", "cp", "-Rf", "testdata", repoDirectory())) diff --git a/test/e2e/ssh_repo_test.go b/test/e2e/ssh_repo_test.go index 17fd886b39..e949f55c15 100644 --- a/test/e2e/ssh_repo_test.go +++ b/test/e2e/ssh_repo_test.go @@ -9,7 +9,7 @@ import ( . "github.com/argoproj/argo-cd/test/e2e/fixture/app" ) -func TestCanAccessSSHRepo(t *testing.T) { +func TestCanAccessInsecureSSHRepo(t *testing.T) { Given(t). SSHInsecureRepoURLAdded(). RepoURLType(fixture.RepoURLTypeSSH). @@ -20,3 +20,16 @@ func TestCanAccessSSHRepo(t *testing.T) { Then(). Expect(OperationPhaseIs(OperationSucceeded)) } + +func TestCanAccessSSHRepo(t *testing.T) { + Given(t). + CustomSSHKnownHostsAdded(). + SSHRepoURLAdded(). + RepoURLType(fixture.RepoURLTypeSSH). + Path("config-map"). + When(). + Create(). + Sync(). + Then(). + Expect(OperationPhaseIs(OperationSucceeded)) +} diff --git a/test/fixture/certs/ssh_known_hosts b/test/fixture/certs/ssh_known_hosts new file mode 100644 index 0000000000..c98597b633 --- /dev/null +++ b/test/fixture/certs/ssh_known_hosts @@ -0,0 +1,3 @@ +[localhost]:2222 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBLle3IiLWy+Cwz6/JT3K8PSGAEZAJnaxiWk0u9wkAvbZ9wHTffctg25coBa8J4Oo1l5GTIkezib2C4PjCE01BZM= +[localhost]:2222 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDhRWyu6rg0Kd0ugLxNGZ8gzUjasF4Z0oT16RUC/L9EkJWATAu4TkkoozZ5AcejlS29jUZXTkKt0La4dmIooeMDNd8b5vg1dWzSDDHwxd8Wa/4XZsUlL6zkUFrnqOPaFc/7EwM3I30064zT/Gt0BVvQUxKoT/TTea2KhQqeLmlWh4cVWJBuhZ8YODUf2VD4TSYfvpcqW/jVw2oG8Pj3WIaaG2+Bcp4Q4sJS2K+2kkiqmZ/hiPK1X/UbMRN2zWQBp5UPWFY2ctuC9B8yhLwAyMkHzuWLfB39dNEdn1jTjDsOUWbC3kDsWHsY5gtBxN30NizBWC+83NpaWbrzAlGb0JV1 +[localhost]:2222 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIG2t7Tcavp5oUqbbSwEKRaGwEq94b8BFK16AEBbgRCTp diff --git a/util/db/certificate.go b/util/db/certificate.go index 82fd6bd641..1fa33a818b 100644 --- a/util/db/certificate.go +++ b/util/db/certificate.go @@ -2,11 +2,14 @@ package db import ( "fmt" + "regexp" "strings" "golang.org/x/crypto/ssh" "golang.org/x/net/context" + log "github.com/sirupsen/logrus" + "github.com/argoproj/argo-cd/common" appsv1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" certutil "github.com/argoproj/argo-cd/util/cert" @@ -169,9 +172,24 @@ func (db *db) CreateRepoCertificate(ctx context.Context, certificates *appsv1.Re // Each request can contain multiple certificates of different types, so we // make sure to handle each request accordingly. for _, certificate := range certificates.Items { - // Ensure valid repo server name was given - if !certutil.IsValidHostname(certificate.ServerName, false) { + // Ensure valid repo server name was given only for https certificates. + // For SSH known host entries, we let Go's ssh library do the validation + // later on. + if certificate.CertType == "https" && !certutil.IsValidHostname(certificate.ServerName, false) { return nil, fmt.Errorf("Invalid hostname in request: %s", certificate.ServerName) + } else if certificate.CertType == "ssh" { + // Matches "[hostname]:port" format + reExtract := regexp.MustCompile(`^\[(.*)\]\:[0-9]+$`) + matches := reExtract.FindStringSubmatch(certificate.ServerName) + var hostnameToCheck string + if len(matches) == 0 { + hostnameToCheck = certificate.ServerName + } else { + hostnameToCheck = matches[1] + } + if !certutil.IsValidHostname(hostnameToCheck, false) { + return nil, fmt.Errorf("Invalid hostname in request: %s", hostnameToCheck) + } } if certificate.CertType == "ssh" { @@ -201,14 +219,18 @@ func (db *db) CreateRepoCertificate(ctx context.Context, certificates *appsv1.Re } // Make sure that we received a valid public host key by parsing it - _, _, rawKeyData, _, _, err := ssh.ParseKnownHosts([]byte(fmt.Sprintf("%s %s %s", certificate.ServerName, certificate.CertSubType, certificate.CertData))) + _, hostnames, rawKeyData, _, _, err := ssh.ParseKnownHosts([]byte(fmt.Sprintf("%s %s %s", certificate.ServerName, certificate.CertSubType, certificate.CertData))) if err != nil { return nil, err } + if len(hostnames) == 0 { + log.Errorf("Could not parse hostname for key from token %s", certificate.ServerName) + } + if newEntry { sshKnownHostsList = append(sshKnownHostsList, &SSHKnownHostsEntry{ - Host: certificate.ServerName, + Host: hostnames[0], Data: string(certificate.CertData), SubType: certificate.CertSubType, }) diff --git a/util/db/certificate_test.go b/util/db/certificate_test.go index ee6ead5259..6fa3b7433f 100644 --- a/util/db/certificate_test.go +++ b/util/db/certificate_test.go @@ -292,9 +292,9 @@ func Test_ListCertificate(t *testing.T) { HostNamePattern: "*", CertType: "ssh", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, len(certList.Items), Test_NumSSHKnownHostsExpected) + assert.Len(t, certList.Items, Test_NumSSHKnownHostsExpected) for idx, entry := range certList.Items { assert.Equal(t, entry.ServerName, Test_SSH_Hostname_Entries[idx]) assert.Equal(t, entry.CertSubType, Test_SSH_Subtypes[idx]) @@ -306,9 +306,9 @@ func Test_ListCertificate(t *testing.T) { HostNamePattern: "*", CertType: "https", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, len(certList.Items), Test_NumTLSCertificatesExpected) + assert.Len(t, certList.Items, Test_NumTLSCertificatesExpected) // List all certificates using selector // Expected: List of 10 entries @@ -316,16 +316,16 @@ func Test_ListCertificate(t *testing.T) { HostNamePattern: "*", CertType: "*", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, len(certList.Items), Test_NumTLSCertificatesExpected+Test_NumSSHKnownHostsExpected) + assert.Len(t, certList.Items, Test_NumTLSCertificatesExpected+Test_NumSSHKnownHostsExpected) // List all certificates using nil selector // Expected: List of 10 entries certList, err = db.ListRepoCertificates(context.Background(), nil) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, len(certList.Items), Test_NumTLSCertificatesExpected+Test_NumSSHKnownHostsExpected) + assert.Len(t, certList.Items, Test_NumTLSCertificatesExpected+Test_NumSSHKnownHostsExpected) // List all certificates matching a host name pattern // Expected: List of 4 entries, all with servername gitlab.com @@ -333,9 +333,9 @@ func Test_ListCertificate(t *testing.T) { HostNamePattern: "gitlab.com", CertType: "*", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 4, len(certList.Items)) + assert.Len(t, certList.Items, 4) for _, entry := range certList.Items { assert.Equal(t, "gitlab.com", entry.ServerName) } @@ -345,9 +345,9 @@ func Test_ListCertificate(t *testing.T) { HostNamePattern: "gitlab.com", CertType: "https", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 1, len(certList.Items)) + assert.Len(t, certList.Items, 1) assert.Equal(t, "gitlab.com", certList.Items[0].ServerName) assert.Equal(t, "https", certList.Items[0].CertType) } @@ -367,9 +367,23 @@ func Test_CreateSSHKnownHostEntries(t *testing.T) { }, }, }, false) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 1, len(certList.Items)) + assert.Len(t, certList.Items, 1) + + // Valid known hosts entry + certList, err = db.CreateRepoCertificate(context.Background(), &v1alpha1.RepositoryCertificateList{ + Items: []v1alpha1.RepositoryCertificate{ + { + ServerName: "[foo.example.com]:2222", + CertType: "ssh", + CertData: []byte("ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQDioSMcGxdVkHaQzRjP71nY4mgVHXjuZiYN9NBiUxNZ0DYGjTIENI3uV45XxrS6PQfoyekUlVlHK2jwpcPrqAg6rlAdMD5WIxzvCnFjCuPA6Ljk8p0ZmYbvriDcgtj+UfGEdyUTgxH2gch6KwTY0eAbLue15IuXtoNzpLxk29iGRi5ZXNAbSBjeB3hm2PKLa6LnDqdkvc+nqoYqn1Fvx7ZJIh0apBCJpOtHPON4rnl7QQvNg9pWulZ5GKcpYMRfTpvHyFTEyrsVT5GH38l9s355GqU7GxQ/i6Tj1D0MKrIB2WmdjOnujM/ELLsrkYspMhn8ZRpCphN/LTcrOWsb0AM69drvYlhc6cnNAtC4UXp0GUy1HsBiJCsUm9/1Gz23VLDRvWop8yE8+PE3Ho5eL7ad9wmOG0mSOYEqVvAstmd8vzbD6oRuY8qV8X3tt9ph2tMAve0Qbo0NN3c51c9OfdXtJaSyckjEjaK7zjnArnYfladZZVlf2Tv8FsV0sJmfSAE="), + }, + }, + }, false) + assert.NoError(t, err) + assert.NotNil(t, certList) + assert.Len(t, certList.Items, 1) // Invalid hostname // Result: Error @@ -382,7 +396,7 @@ func Test_CreateSSHKnownHostEntries(t *testing.T) { }, }, }, false) - assert.NotNil(t, err) + assert.Error(t, err) assert.Nil(t, certList) // Check if it really was added @@ -391,9 +405,9 @@ func Test_CreateSSHKnownHostEntries(t *testing.T) { HostNamePattern: "foo.example.com", CertType: "ssh", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 1, len(certList.Items)) + assert.Len(t, certList.Items, 1) // Existing cert, same data, no upsert // Result: no error, should return 0 added certificates @@ -406,9 +420,9 @@ func Test_CreateSSHKnownHostEntries(t *testing.T) { }, }, }, false) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 0, len(certList.Items)) + assert.Len(t, certList.Items, 0) // Existing cert, different data, no upsert // Result: Error @@ -421,7 +435,7 @@ func Test_CreateSSHKnownHostEntries(t *testing.T) { }, }, }, false) - assert.NotNil(t, err) + assert.Error(t, err) assert.Nil(t, certList) // Existing cert, different data, upsert @@ -434,9 +448,9 @@ func Test_CreateSSHKnownHostEntries(t *testing.T) { }, }, }, true) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 1, len(certList.Items)) + assert.Len(t, certList.Items, 1) // Invalid known hosts entry, case 1: key sub type missing // Result: Error @@ -449,7 +463,7 @@ func Test_CreateSSHKnownHostEntries(t *testing.T) { }, }, }, false) - assert.NotNil(t, err) + assert.Error(t, err) assert.Nil(t, certList) // Invalid known hosts entry, case 2: invalid base64 data @@ -463,7 +477,7 @@ func Test_CreateSSHKnownHostEntries(t *testing.T) { }, }, }, false) - assert.NotNil(t, err) + assert.Error(t, err) assert.Nil(t, certList) } @@ -483,9 +497,9 @@ func Test_CreateTLSCertificates(t *testing.T) { }, }, }, false) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 1, len(certList.Items)) + assert.Len(t, certList.Items, 1) // Invalid hostname // Result: Error @@ -498,7 +512,7 @@ func Test_CreateTLSCertificates(t *testing.T) { }, }, }, false) - assert.NotNil(t, err) + assert.Error(t, err) assert.Nil(t, certList) // Check if it really was added @@ -507,9 +521,9 @@ func Test_CreateTLSCertificates(t *testing.T) { HostNamePattern: "foo.example.com", CertType: "https", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 1, len(certList.Items)) + assert.Len(t, certList.Items, 1) // Valid TLS certificates, multiple PEMs in data // Expected: List of 2 entry @@ -522,9 +536,9 @@ func Test_CreateTLSCertificates(t *testing.T) { }, }, }, false) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 2, len(certList.Items)) + assert.Len(t, certList.Items, 2) // Check if it really was added // Result: Return new certificate @@ -532,9 +546,9 @@ func Test_CreateTLSCertificates(t *testing.T) { HostNamePattern: "bar.example.com", CertType: "https", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 2, len(certList.Items)) + assert.Len(t, certList.Items, 2) // Valid TLS certificate, existing cert, same data, no upsert // Expected: List of 0 entry @@ -547,9 +561,9 @@ func Test_CreateTLSCertificates(t *testing.T) { }, }, }, false) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 0, len(certList.Items)) + assert.Len(t, certList.Items, 0) // Valid TLS certificate, existing cert, different data, no upsert // Expected: Error @@ -562,7 +576,7 @@ func Test_CreateTLSCertificates(t *testing.T) { }, }, }, false) - assert.NotNil(t, err) + assert.Error(t, err) assert.Nil(t, certList) // Valid TLS certificate, existing cert, different data, upsert @@ -576,9 +590,9 @@ func Test_CreateTLSCertificates(t *testing.T) { }, }, }, true) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 2, len(certList.Items)) + assert.Len(t, certList.Items, 2) // Check if upsert was successful // Expected: List of 2 entries, matching hostnames & cert types @@ -586,9 +600,9 @@ func Test_CreateTLSCertificates(t *testing.T) { HostNamePattern: "foo.example.com", CertType: "https", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 2, len(certList.Items)) + assert.Len(t, certList.Items, 2) for _, entry := range certList.Items { assert.Equal(t, "foo.example.com", entry.ServerName) assert.Equal(t, "https", entry.CertType) @@ -605,7 +619,7 @@ func Test_CreateTLSCertificates(t *testing.T) { }, }, }, false) - assert.NotNil(t, err) + assert.Error(t, err) assert.Nil(t, certList) // Valid PEM data, new cert, but invalid certificate @@ -619,7 +633,7 @@ func Test_CreateTLSCertificates(t *testing.T) { }, }, }, false) - assert.NotNil(t, err) + assert.Error(t, err) assert.Nil(t, certList) // Invalid PEM data, existing cert, upsert @@ -633,7 +647,7 @@ func Test_CreateTLSCertificates(t *testing.T) { }, }, }, true) - assert.NotNil(t, err) + assert.Error(t, err) assert.Nil(t, certList) // Valid PEM data, existing cert, but invalid certificate, upsert @@ -647,7 +661,7 @@ func Test_CreateTLSCertificates(t *testing.T) { }, }, }, true) - assert.NotNil(t, err) + assert.Error(t, err) assert.Nil(t, certList) } @@ -663,9 +677,9 @@ func Test_RemoveSSHKnownHosts(t *testing.T) { HostNamePattern: "github.com", CertType: "ssh", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 1, len(certList.Items)) + assert.Len(t, certList.Items, 1) // Check whether entry was really removed // Expected: List of 0 entries @@ -673,9 +687,9 @@ func Test_RemoveSSHKnownHosts(t *testing.T) { HostNamePattern: "github.com", CertType: "ssh", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 0, len(certList.Items)) + assert.Len(t, certList.Items, 0) // Remove single SSH known hosts entry by sub type // Expected: List of 1 entry @@ -683,9 +697,9 @@ func Test_RemoveSSHKnownHosts(t *testing.T) { CertType: "ssh", CertSubType: "ssh-ed25519", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 1, len(certList.Items)) + assert.Len(t, certList.Items, 1) // Check whether entry was really removed // Expected: List of 0 entries @@ -693,27 +707,27 @@ func Test_RemoveSSHKnownHosts(t *testing.T) { CertType: "ssh", CertSubType: "ssh-ed25519", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 0, len(certList.Items)) + assert.Len(t, certList.Items, 0) // Remove all remaining SSH known hosts entries // Expected: List of 5 entry certList, err = db.RemoveRepoCertificates(context.Background(), &CertificateListSelector{ CertType: "ssh", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 5, len(certList.Items)) + assert.Len(t, certList.Items, 5) // Check whether the entries were really removed // Expected: List of 0 entries certList, err = db.ListRepoCertificates(context.Background(), &CertificateListSelector{ CertType: "ssh", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 0, len(certList.Items)) + assert.Len(t, certList.Items, 0) } func Test_RemoveTLSCertificates(t *testing.T) { @@ -727,9 +741,9 @@ func Test_RemoveTLSCertificates(t *testing.T) { HostNamePattern: "gitlab.com", CertType: "https", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 1, len(certList.Items)) + assert.Len(t, certList.Items, 1) // Check whether entry was really removed // Expected: List of 0 entries @@ -737,9 +751,9 @@ func Test_RemoveTLSCertificates(t *testing.T) { HostNamePattern: "gitlab.com", CertType: "https", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 0, len(certList.Items)) + assert.Len(t, certList.Items, 0) // Remove all TLS certificate entry for hostname // Expected: List of 2 entry @@ -747,9 +761,9 @@ func Test_RemoveTLSCertificates(t *testing.T) { HostNamePattern: "test.example.com", CertType: "https", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 2, len(certList.Items)) + assert.Len(t, certList.Items, 2) // Check whether entries were really removed // Expected: List of 0 entries @@ -757,8 +771,8 @@ func Test_RemoveTLSCertificates(t *testing.T) { HostNamePattern: "test.example.com", CertType: "https", }) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, certList) - assert.Equal(t, 0, len(certList.Items)) + assert.Len(t, certList.Items, 0) } diff --git a/util/git/client.go b/util/git/client.go index ece473e2bf..fb63f29b2e 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -14,6 +14,7 @@ import ( argoexec "github.com/argoproj/pkg/exec" log "github.com/sirupsen/logrus" "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/knownhosts" "gopkg.in/src-d/go-git.v4" "gopkg.in/src-d/go-git.v4/config" "gopkg.in/src-d/go-git.v4/plumbing" @@ -195,6 +196,13 @@ func newAuth(repoURL string, creds Creds) (transport.AuthMethod, error) { auth := &ssh2.PublicKeys{User: sshUser, Signer: signer} if creds.insecure { auth.HostKeyCallback = ssh.InsecureIgnoreHostKey() + } else { + // Set up validation of SSH known hosts for using our ssh_known_hosts + // file. + auth.HostKeyCallback, err = knownhosts.New(certutil.GetSSHKnownHostsDataPath()) + if err != nil { + log.Errorf("Could not set-up SSH known hosts callback: %v", err) + } } return auth, nil case HTTPSCreds: diff --git a/util/git/creds.go b/util/git/creds.go index bf57febf2f..dd3a399b76 100644 --- a/util/git/creds.go +++ b/util/git/creds.go @@ -10,6 +10,7 @@ import ( log "github.com/sirupsen/logrus" "github.com/argoproj/argo-cd/util" + certutil "github.com/argoproj/argo-cd/util/cert" ) type Creds interface { @@ -171,6 +172,9 @@ func (c SSHCreds) Environ() (io.Closer, []string, error) { // StrictHostKeyChecking will add the host to the knownhosts file, we don't want that - a security issue really, // UserKnownHostsFile=/dev/null is therefore used so we write the new insecure host to /dev/null args = append(args, "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null") + } else { + knownHostsFile := certutil.GetSSHKnownHostsDataPath() + args = append(args, "-o", "StrictHostKeyChecking=yes", "-o", fmt.Sprintf("UserKnownHostsFile=%s", knownHostsFile)) } env = append(env, []string{fmt.Sprintf("GIT_SSH_COMMAND=%s", strings.Join(args, " "))}...) return sshPrivateKeyFile(file.Name()), env, nil diff --git a/util/settings/settings.go b/util/settings/settings.go index 1f664810bb..ea2f37971d 100644 --- a/util/settings/settings.go +++ b/util/settings/settings.go @@ -7,7 +7,6 @@ import ( "crypto/x509" "encoding/base64" "fmt" - "io/ioutil" "os" "strings" "sync" @@ -28,7 +27,6 @@ import ( "github.com/argoproj/argo-cd/common" "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" "github.com/argoproj/argo-cd/util" - certutil "github.com/argoproj/argo-cd/util/cert" "github.com/argoproj/argo-cd/util/password" tlsutil "github.com/argoproj/argo-cd/util/tls" ) @@ -689,18 +687,6 @@ func (mgr *SettingsManager) SaveSSHKnownHostsData(ctx context.Context, knownHost return err } - // If we are running outside a K8S cluster, the ConfigMap mount will not - // automatically write out the SSH known hosts data. We need this mechanism - // for running E2E tests. - if os.Getenv(common.EnvVarFakeInClusterConfig) == "true" { - knownHostsPath := certutil.GetSSHKnownHostsDataPath() - err := ioutil.WriteFile(knownHostsPath, []byte(sshKnownHostsData), 0644) - // We don't return error, but let the user know through log - if err != nil { - log.Errorf("Could not write SSH known hosts data: %v", err) - } - } - return mgr.ResyncInformers() } @@ -721,42 +707,6 @@ func (mgr *SettingsManager) SaveTLSCertificateData(ctx context.Context, tlsCerti return err } - // If we are running outside a K8S cluster, the ConfigMap mount will not - // automatically write out the TLS certificate data. We need this mechanism - // for running E2E tests. - // - // The paradigm here is, that we first remove all files in the TLS data - // directory (there should be only TLS certs in it), and then recreate each - // single cert that is still in the tlsCertificates data map. - if os.Getenv(common.EnvVarFakeInClusterConfig) == "true" { - tlsDataPath := certutil.GetTLSCertificateDataPath() - - // First throw way everything - tlsFiles, err := ioutil.ReadDir(tlsDataPath) - if err != nil { - log.Errorf("Could not open TLS certificate dir %s: %v", tlsDataPath, err) - } else { - for _, file := range tlsFiles { - tlsCertPath := fmt.Sprintf("%s/%s", tlsDataPath, file.Name()) - log.Debugf("Deleting TLS certificate file %s", tlsCertPath) - err := os.Remove(tlsCertPath) - if err != nil { - log.Errorf("Could not delete TLS cert file %s: %v", tlsCertPath, err) - } - } - } - - // Recreate configured TLS certificates - for hostName, certData := range tlsCertificates { - certPath := fmt.Sprintf("%s/%s", tlsDataPath, hostName) - log.Debugf("Writing TLS Certificate data to %s", certPath) - err := ioutil.WriteFile(certPath, []byte(certData), 0644) - if err != nil { - log.Errorf("Could not write PEM data to %s: %v", certPath, err) - } - } - } - return mgr.ResyncInformers() }