From 7a0f20f13964ae3761fae6e096cc39909994f1c6 Mon Sep 17 00:00:00 2001 From: Sergiy Kulanov Date: Tue, 4 Aug 2026 14:54:18 +0300 Subject: [PATCH] EPMDEDP-17256: fix: Verify SSH host keys for all git, GitServer and Gerrit connections Every outbound SSH connection accepted any host key: the git provider used InsecureIgnoreHostKey for all repository traffic including the packless transport, the GitServer connectivity check did the same for every provider, and the Gerrit client used a callback that unconditionally returned nil. A network-position attacker could impersonate a git server to intercept pushed source or serve falsified refs. Host keys are now verified against a known_hosts file shipped by the chart as a ConfigMap and mounted as a directory rather than with subPath, so the kubelet refreshes it in place and added entries take effect without restarting the operator. The file is seeded with keys for github.com, gitlab.com and bitbucket.org. In the go-git paths the host key callback is left unset rather than replaced: go-git then derives HostKeyAlgorithms from known_hosts as well, which avoids spurious key-mismatch failures when a server offers a key type that is absent from the file. Failures name the host, the port and the remediation command through GitServer .status.error. A key mismatch is reported distinctly from an unknown host and never suggests re-scanning, since that would trust whatever key was just presented. BREAKING CHANGE: GitServers authenticating over SSH to a host outside the seeded providers must have their host keys added to knownHosts.entries before upgrading. GitServers using token authentication are unaffected. Note: git operations for the github, gitlab and bitbucket providers connect on port 22 regardless of spec.sshPort, so a server on another port needs both entry forms pinned. Design decisions, recorded here rather than as comments describing what the code does not do: - No switch disables verification. An operator-wide opt-out would be turned on once and never turned off. - The go-git paths leave AuthMethod.HostKeyCallback unset instead of supplying one, so go-git also derives HostKeyAlgorithms from known_hosts; supplying a callback suppresses that and turns a missing entry into a reported key mismatch. - The chart mounts the ConfigMap as a directory rather than with subPath, because the kubelet only refreshes in-place directory mounts, so a host key can be added without restarting the operator. - The Dockerfile's SSH_KNOWN_HOSTS moves onto the chart's path so the image default and the chart agree on one file instead of two. Signed-off-by: Sergiy Kulanov --- Dockerfile | 2 +- api/v1/git_server_types.go | 3 + .../crd/bases/v2.edp.epam.com_gitservers.yaml | 3 + controllers/gitserver/ssh.go | 11 +- controllers/gitserver/ssh_test.go | 42 ++++ deploy-templates/README.md | 2 + .../crds/v2.edp.epam.com_gitservers.yaml | 3 + deploy-templates/templates/deployment.yaml | 19 +- .../templates/ssh-known-hosts-cm.yaml | 33 +++ deploy-templates/values.yaml | 14 ++ docs/api.md | 5 +- docs/ssh-known-hosts.md | 130 ++++++++++ pkg/gerrit/gerrit.go | 21 +- pkg/gerrit/gerrit_test.go | 12 + pkg/git/provider.go | 50 +++- pkg/git/provider_test.go | 32 +++ pkg/git/transport.go | 8 +- pkg/sshhostkey/hostkey.go | 134 +++++++++++ pkg/sshhostkey/hostkey_test.go | 223 ++++++++++++++++++ 19 files changed, 722 insertions(+), 25 deletions(-) create mode 100644 deploy-templates/templates/ssh-known-hosts-cm.yaml create mode 100644 docs/ssh-known-hosts.md create mode 100644 pkg/sshhostkey/hostkey.go create mode 100644 pkg/sshhostkey/hostkey_test.go diff --git a/Dockerfile b/Dockerfile index fcd75d35..40858f09 100644 --- a/Dockerfile +++ b/Dockerfile @@ -5,7 +5,7 @@ ARG TARGETARCH ENV ASSETS_DIR=/usr/local/bin \ HOME=/home/codebase-operator \ OPERATOR=/usr/local/bin/codebase-operator \ - SSH_KNOWN_HOSTS=/home/codebase-operator/.ssh/known_hosts \ + SSH_KNOWN_HOSTS=/etc/codebase-operator/ssh/ssh_known_hosts \ USER_NAME=codebase-operator \ USER_UID=1001 diff --git a/api/v1/git_server_types.go b/api/v1/git_server_types.go index 379d942d..ae2c5478 100644 --- a/api/v1/git_server_types.go +++ b/api/v1/git_server_types.go @@ -32,6 +32,9 @@ type GitServerSpec struct { // - secretString: Webhook secret for validating webhook requests // - username: Git username to override the default GitUser // For Gerrit provider, only id_rsa key is required and used. + // When id_rsa is present the operator connects over SSH and verifies the server's + // host key against the operator's ssh-known-hosts ConfigMap; hosts other than + // github.com, gitlab.com and bitbucket.org must be added there first. // +kubebuilder:example:=my-git-credentials // +required NameSshKeySecret string `json:"nameSshKeySecret"` diff --git a/config/crd/bases/v2.edp.epam.com_gitservers.yaml b/config/crd/bases/v2.edp.epam.com_gitservers.yaml index 29d179fc..64da28e4 100644 --- a/config/crd/bases/v2.edp.epam.com_gitservers.yaml +++ b/config/crd/bases/v2.edp.epam.com_gitservers.yaml @@ -83,6 +83,9 @@ spec: - secretString: Webhook secret for validating webhook requests - username: Git username to override the default GitUser For Gerrit provider, only id_rsa key is required and used. + When id_rsa is present the operator connects over SSH and verifies the server's + host key against the operator's ssh-known-hosts ConfigMap; hosts other than + github.com, gitlab.com and bitbucket.org must be added there first. example: my-git-credentials type: string skipWebhookSSLVerification: diff --git a/controllers/gitserver/ssh.go b/controllers/gitserver/ssh.go index f4bb970f..a9059d9a 100644 --- a/controllers/gitserver/ssh.go +++ b/controllers/gitserver/ssh.go @@ -9,6 +9,7 @@ import ( "github.com/epam/edp-codebase-operator/v2/pkg/gerrit" "github.com/epam/edp-codebase-operator/v2/pkg/model" + "github.com/epam/edp-codebase-operator/v2/pkg/sshhostkey" "github.com/epam/edp-codebase-operator/v2/pkg/util" ) @@ -33,6 +34,8 @@ func checkGitServerConnection(data gitSshData, log logr.Logger) error { c *ssh.Client ) + // NewSession already enriches host key failures; enriching again here would + // repeat the whole remediation sentence inside GitServer .status.error. if s, c, err = sshClient.NewSession(); err != nil { return fmt.Errorf("failed to create ssh session: %w", err) } @@ -61,12 +64,18 @@ func sshInitFromSecret(data gitSshData, logger logr.Logger) (*gerrit.SSHClient, return nil, err } + hostKeyCallback, hostKeyAlgorithms, err := sshhostkey.ClientConfig(data.Host, data.Port) + if err != nil { + return nil, err + } + sshConfig := &ssh.ClientConfig{ User: data.User, Auth: []ssh.AuthMethod{ sshAuth, }, - HostKeyCallback: ssh.InsecureIgnoreHostKey(), + HostKeyCallback: hostKeyCallback, + HostKeyAlgorithms: hostKeyAlgorithms, } cl := &gerrit.SSHClient{ diff --git a/controllers/gitserver/ssh_test.go b/controllers/gitserver/ssh_test.go index 20ef016f..3d8b169e 100644 --- a/controllers/gitserver/ssh_test.go +++ b/controllers/gitserver/ssh_test.go @@ -1,11 +1,53 @@ package gitserver import ( + "os" + "path/filepath" "testing" + "github.com/go-logr/logr" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func Test_sshInitFromSecret_verifiesHostKeys(t *testing.T) { + knownHosts := filepath.Join(t.TempDir(), "ssh_known_hosts") + require.NoError(t, os.WriteFile(knownHosts, []byte( + "[git.example.com]:2222 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l9GKJl\n", + ), 0o600)) + t.Setenv("SSH_KNOWN_HOSTS", knownHosts) + + client, err := sshInitFromSecret(gitSshData{ + Host: "git.example.com", + User: "git", + Key: testKey, + Port: 2222, + }, logr.Discard()) + require.NoError(t, err) + + // The connection must never be established without a host key check. + require.NotNil(t, client.Config.HostKeyCallback) + // Algorithms must be narrowed to what is on file, otherwise a server offering + // an unpinned key type fails as a mismatch rather than as a missing entry. + assert.Equal(t, []string{"ssh-ed25519"}, client.Config.HostKeyAlgorithms) +} + +func Test_sshInitFromSecret_failsWithoutKnownHosts(t *testing.T) { + t.Setenv("SSH_KNOWN_HOSTS", filepath.Join(t.TempDir(), "absent")) + + client, err := sshInitFromSecret(gitSshData{ + Host: "git.example.com", + User: "git", + Key: testKey, + Port: 22, + }, logr.Discard()) + + require.Error(t, err) + assert.Nil(t, client) + // This message reaches the user through GitServer .status.error. + assert.Contains(t, err.Error(), "known_hosts") +} + func Test_publicKey(t *testing.T) { tests := []struct { name string diff --git a/deploy-templates/README.md b/deploy-templates/README.md index 5c7c2eb1..c789540a 100644 --- a/deploy-templates/README.md +++ b/deploy-templates/README.md @@ -44,6 +44,8 @@ A Helm chart for KubeRocketCI Codebase Operator | jira.name | string | `"jira"` | JiraServer CR name | | jira.quickLink | object | `{"enabled":true}` | Enable creation of QuickLink for Jira | | jira.rootUrl | string | `"https://jiraeu.example.com"` | URL to Jira server | +| knownHosts.entries | string | `""` (no self-hosted servers pinned) | Host keys for self-hosted git servers, in known_hosts format, one per line. Obtain them with `ssh-keyscan -t rsa,ecdsa,ed25519 -p ` and verify the fingerprints out-of-band before trusting them. Servers on a port other than 22 must use the bracket form, e.g. `[git.example.com]:2222 ssh-ed25519 AAAA...`. | +| knownHosts.includeDefaultProviders | bool | `true` | Include the shipped host keys for github.com, gitlab.com and bitbucket.org. Disable only if you pin these hosts yourself through `entries`. | | name | string | `"codebase-operator"` | component name | | nodeSelector | object | `{}` | | | podLabels | object | `{}` | Labels to be added to the pod | diff --git a/deploy-templates/crds/v2.edp.epam.com_gitservers.yaml b/deploy-templates/crds/v2.edp.epam.com_gitservers.yaml index 29d179fc..64da28e4 100644 --- a/deploy-templates/crds/v2.edp.epam.com_gitservers.yaml +++ b/deploy-templates/crds/v2.edp.epam.com_gitservers.yaml @@ -83,6 +83,9 @@ spec: - secretString: Webhook secret for validating webhook requests - username: Git username to override the default GitUser For Gerrit provider, only id_rsa key is required and used. + When id_rsa is present the operator connects over SSH and verifies the server's + host key against the operator's ssh-known-hosts ConfigMap; hosts other than + github.com, gitlab.com and bitbucket.org must be added there first. example: my-git-credentials type: string skipWebhookSSLVerification: diff --git a/deploy-templates/templates/deployment.yaml b/deploy-templates/templates/deployment.yaml index f0bd666e..cd0f6be7 100644 --- a/deploy-templates/templates/deployment.yaml +++ b/deploy-templates/templates/deployment.yaml @@ -42,11 +42,19 @@ spec: - containerPort: 9443 name: webhook-server protocol: TCP + {{- end }} volumeMounts: + {{- if .Values.enableWebhooks }} - mountPath: /tmp/k8s-webhook-server/serving-certs name: cert readOnly: true - {{- end }} + {{- end }} + # A directory mount, not subPath: the kubelet only refreshes in-place + # directory mounts, so host keys added to the ConfigMap take effect + # without restarting the operator. + - mountPath: /etc/codebase-operator/ssh + name: ssh-known-hosts + readOnly: true imagePullPolicy: "{{ .Values.imagePullPolicy }}" {{- if .Values.securityContext }} securityContext: {{ toYaml .Values.securityContext | nindent 12 }} @@ -76,6 +84,8 @@ spec: value: {{ .Values.enableWebhooks | quote }} - name: BRANCH_STALE_CHECK_INTERVAL value: {{ .Values.branchStaleCheckInterval | quote }} + - name: SSH_KNOWN_HOSTS + value: /etc/codebase-operator/ssh/ssh_known_hosts {{ toYaml .Values.envs | indent 12 }} resources: {{ toYaml .Values.resources | indent 12 }} @@ -91,10 +101,13 @@ spec: tolerations: {{- toYaml . | nindent 8 }} {{- end }} - {{- if .Values.enableWebhooks }} volumes: + {{- if .Values.enableWebhooks }} - name: cert secret: defaultMode: 420 secretName: {{ .Values.name }}-webhook-certs - {{- end }} + {{- end }} + - name: ssh-known-hosts + configMap: + name: {{ .Values.name }}-ssh-known-hosts diff --git a/deploy-templates/templates/ssh-known-hosts-cm.yaml b/deploy-templates/templates/ssh-known-hosts-cm.yaml new file mode 100644 index 00000000..a1491ea6 --- /dev/null +++ b/deploy-templates/templates/ssh-known-hosts-cm.yaml @@ -0,0 +1,33 @@ +{{/* +Always rendered, even when it holds no entries: the operator resolves +SSH_KNOWN_HOSTS to a file inside it, and a missing file produces a generic +"unable to find any valid known_hosts file" error instead of the actionable +per-host message. + +Edits are picked up by a running operator without a restart - see the +volumeMount comment in deployment.yaml. +*/}} +apiVersion: v1 +kind: ConfigMap +metadata: + name: {{ .Values.name }}-ssh-known-hosts + labels: + {{- include "codebase-operator.labels" . | nindent 4 }} +data: + ssh_known_hosts: | + # Host keys for the public git providers, verified against the keys published + # by each vendor. Regenerate with: ssh-keyscan -t rsa,ecdsa,ed25519 + {{- if .Values.knownHosts.includeDefaultProviders }} + github.com ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBEmKSENjQEezOmxkZMy7opKgwFB9nkt5YRrYMjNuG5N87uRgg6CLrbo5wAdT/y6v0mKV0U2w0WZ2YB/++Tpockg= + github.com ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l9GKJl + github.com ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQCj7ndNxQowgcQnjshcLrqPEiiphnt+VTTvDP6mHBL9j1aNUkY4Ue1gvwnGLVlOhGeYrnZaMgRK6+PKCUXaDbC7qtbW8gIkhL7aGCsOr/C56SJMy/BCZfxd1nWzAOxSDPgVsmerOBYfNqltV9/hWCqBywINIR+5dIg6JTJ72pcEpEjcYgXkE2YEFXV1JHnsKgbLWNlhScqb2UmyRkQyytRLtL+38TGxkxCflmO+5Z8CSSNY7GidjMIZ7Q4zMjA2n1nGrlTDkzwDCsw+wqFPGQA179cnfGWOWRVruj16z6XyvxvjJwbz0wQZ75XK5tKSb7FNyeIEs4TT4jk+S4dhPeAUC5y+bDYirYgM4GC7uEnztnZyaVWQ7B381AK4Qdrwt51ZqExKbQpTUNn+EjqoTwvqNj4kqx5QUCI0ThS/YkOxJCXmPUWZbhjpCg56i+2aB6CmK2JGhn57K5mj0MNdBXA4/WnwH6XoPWJzK5Nyu2zB3nAZp+S5hpQs+p1vN1/wsjk= + gitlab.com ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBFSMqzJeV9rUzU4kWitGjeR4PWSa29SPqJ1fVkhtj3Hw9xjLVXVYrU9QlYWrOLXBpQ6KWjbjTDTdDkoohFzgbEY= + gitlab.com ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAfuCHKVTjquxvt6CM6tdG4SLp1Btn/nOeHHE5UOzRdf + gitlab.com ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCsj2bNKTBSpIYDEGk9KxsGh3mySTRgMtXL583qmBpzeQ+jqCMRgBqB98u3z++J1sKlXHWfM9dyhSevkMwSbhoR8XIq/U0tCNyokEi/ueaBMCvbcTHhO7FcwzY92WK4Yt0aGROY5qX2UKSeOvuP4D6TPqKF1onrSzH9bx9XUf2lEdWT/ia1NEKjunUqu1xOB/StKDHMoX4/OKyIzuS0q/T1zOATthvasJFoPrAjkohTyaDUz2LN5JoH839hViyEG82yB+MjcFV5MU3N1l1QL3cVUCh93xSaua1N85qivl+siMkPGbO5xR/En4iEY6K2XPASUEMaieWVNTRCtJ4S8H+9 + bitbucket.org ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBPIQmuzMBuKdWeF4+a2sjSSpBK0iqitSQ+5BM9KhpexuGt20JpTVM7u5BDZngncgrqDMbWdxMWWOGtZ9UgbqgZE= + bitbucket.org ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIIazEu89wgQZ4bqs3d63QSMzYVa0MuJ2e2gKTKqu+UUO + bitbucket.org ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQDQeJzhupRu0u0cdegZIa8e86EG2qOCsIsD1Xw0xSeiPDlCr7kq97NLmMbpKTX6Esc30NuoqEEHCuc7yWtwp8dI76EEEB1VqY9QJq6vk+aySyboD5QF61I/1WeTwu+deCbgKMGbUijeXhtfbxSxm6JwGrXrhBdofTsbKRUsrN1WoNgUa8uqN1Vx6WAJw1JHPhglEGGHea6QICwJOAr/6mrui/oB7pkaWKHj3z7d1IC4KWLtY47elvjbaTlkN04Kc/5LFEirorGYVbt15kAUlqGM65pk6ZBxtaO3+30LVlORZkxOh+LKL/BvbZ/iRNhItLqNyieoQj/uh/7Iv4uyH/cV/0b4WDSd3DptigWq84lJubb9t/DnZlrJazxyDCulTmKdOR7vs9gMTo+uoIrPSb8ScTtvw65+odKAlBj59dhnVp9zd7QUojOpXlL62Aw56U4oO+FALuevvMjiWeavKhJqlR7i5n9srYcrNV7ttmDw7kf/97P5zauIhxcjX+xHv4M= + {{- end }} + {{- with .Values.knownHosts.entries }} + {{- tpl . $ | nindent 4 }} + {{- end }} diff --git a/deploy-templates/values.yaml b/deploy-templates/values.yaml index 100eeec0..f34391b2 100644 --- a/deploy-templates/values.yaml +++ b/deploy-templates/values.yaml @@ -88,3 +88,17 @@ enableWebhooks: true # marking missing ones with the Stale condition and the app.edp.epam.com/stale label. # Accepts Go duration strings (e.g. 24h, 30m); "0" disables the check. branchStaleCheckInterval: 24h + +# SSH host key verification. Every SSH connection the operator makes is verified +# against these entries; there is no way to disable verification. GitServers that +# authenticate with a token over HTTPS are unaffected. +knownHosts: + # -- Include the shipped host keys for github.com, gitlab.com and bitbucket.org. + # Disable only if you pin these hosts yourself through `entries`. + includeDefaultProviders: true + # -- Host keys for self-hosted git servers, in known_hosts format, one per line. + # Obtain them with `ssh-keyscan -t rsa,ecdsa,ed25519 -p ` and verify + # the fingerprints out-of-band before trusting them. Servers on a port other than + # 22 must use the bracket form, e.g. `[git.example.com]:2222 ssh-ed25519 AAAA...`. + # @default -- `""` (no self-hosted servers pinned) + entries: "" diff --git a/docs/api.md b/docs/api.md index 8f17829c..4662b1ac 100644 --- a/docs/api.md +++ b/docs/api.md @@ -1346,7 +1346,10 @@ Optional keys: - id_rsa: SSH private key for Git operations over SSH - secretString: Webhook secret for validating webhook requests - username: Git username to override the default GitUser -For Gerrit provider, only id_rsa key is required and used.
+For Gerrit provider, only id_rsa key is required and used. +When id_rsa is present the operator connects over SSH and verifies the server's +host key against the operator's ssh-known-hosts ConfigMap; hosts other than +github.com, gitlab.com and bitbucket.org must be added there first.
true diff --git a/docs/ssh-known-hosts.md b/docs/ssh-known-hosts.md new file mode 100644 index 00000000..64218ec6 --- /dev/null +++ b/docs/ssh-known-hosts.md @@ -0,0 +1,130 @@ +# SSH host key verification + +Every SSH connection the codebase-operator makes to a git server is verified +against a `known_hosts` file. This includes repository clone, fetch and push, +the packless branch operations, the GitServer connectivity check, and Gerrit +administrative commands. + +**Verification cannot be disabled.** An operator that accepts any host key can be +made to hand source code and credentials to whoever controls the network path. + +GitServers that authenticate with a token over HTTPS are not affected — they +never open an SSH connection. Only GitServers whose credentials secret contains +an `id_rsa` key are. + +## Where the host keys live + +The Helm chart creates a ConfigMap named `-ssh-known-hosts` and mounts +it into the operator at `/etc/codebase-operator/ssh`. The operator reads +`/etc/codebase-operator/ssh/ssh_known_hosts`, via the `SSH_KNOWN_HOSTS` +environment variable set by the chart. + +The ConfigMap ships with host keys for `github.com`, `gitlab.com` and +`bitbucket.org`, so SSH GitServers pointing at those providers work with no +further configuration. + +The ConfigMap is mounted as a directory rather than with `subPath`, so the +kubelet refreshes it in place and the operator re-reads it on the next +connection. **Adding a host key does not require restarting the operator.** + +## Adding a self-hosted git server + +Collect the server's host keys: + +```sh +ssh-keyscan -t rsa,ecdsa,ed25519 git.example.com +``` + +For a server on a port other than 22, pass `-p` and note that the output uses +the bracket form, which is what `known_hosts` requires: + +```sh +ssh-keyscan -t rsa,ecdsa,ed25519 -p 2222 git.example.com +# [git.example.com]:2222 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAA... +``` + +Verify the fingerprints against a source other than the connection you just +made — your git server's documentation or its administrator: + +```sh +ssh-keyscan -t rsa,ecdsa,ed25519 git.example.com | ssh-keygen -lf - +``` + +`ssh-keyscan` output is only as trustworthy as the network it ran over. Pinning +a key you scanned through a compromised path pins the attacker's key. + +Then add the lines to the chart values: + +```yaml +knownHosts: + entries: | + [git.example.com]:2222 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAA... + [git.example.com]:2222 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAAB... +``` + +Or, for an immediate fix without a Helm upgrade, edit the ConfigMap directly — +note that a subsequent `helm upgrade` will overwrite it, so fold the entry into +your values afterwards: + +```sh +kubectl edit configmap -ssh-known-hosts +``` + +## Which port to pin + +Pin the port the operator actually connects on, which is not always +`spec.sshPort`: + +| Operation | Port used | known_hosts entry | +|---|---|---| +| GitServer connectivity check, and all Gerrit access | `spec.sshPort` | `[git.example.com]:2222 ...` | +| Repository clone, fetch, push and branch operations for github, gitlab and bitbucket providers | always 22 | `git.example.com ...` | + +The second row is a consequence of the repository URL format for those +providers: `GetSSHUrl` builds the scp-style `git@host:path.git`, which carries +no port, so the SSH client uses 22. `spec.sshPort` is honoured only for Gerrit +URLs and for the connectivity check. + +**If your git server's `sshPort` is not 22, pin both forms**, or the GitServer +will report healthy while codebase operations fail (or the reverse). Scan each +port separately: + +```sh +ssh-keyscan -t rsa,ecdsa,ed25519 -p 2222 git.example.com # [git.example.com]:2222 ... +ssh-keyscan -t rsa,ecdsa,ed25519 git.example.com # git.example.com ... +``` + +Both usually return the same keys, since it is normally the same server reached +on two ports. + +## Diagnosing failures + +A GitServer whose host key is missing or wrong reports the reason in its status: + +```sh +kubectl get gitserver -o jsonpath='{.status.error}' +``` + +| Message | Meaning | Action | +|---|---|---| +| `SSH host key for is not present in known_hosts` | The server is not pinned | Add its keys as above | +| `SSH host key mismatch for ` | The server presented a key that differs from the pinned one | **Do not simply replace the entry.** Either the server was rekeyed or the connection is being intercepted. Confirm the new key with the server's administrator first | +| `failed to load SSH known_hosts` | The file is missing or unreadable | Check that the ConfigMap exists and is mounted | + +The same messages appear on `Codebase` and `CodebaseBranch` status when the +failure happens during a repository operation. + +## Upgrading from a release without host key verification + +Before upgrading, list the GitServers that use SSH: + +```sh +kubectl get gitservers -o json | jq -r '.items[] | select(.spec.gitProvider) | + "\(.metadata.name) \(.spec.gitHost):\(.spec.sshPort) secret=\(.spec.nameSshKeySecret)"' +``` + +For each one whose secret contains an `id_rsa` key and whose host is not +github.com, gitlab.com or bitbucket.org, add its host keys to +`knownHosts.entries` as part of the upgrade. GitServers left unpinned will +report `connected: false` until their keys are added; no data is lost and the +operator recovers on the next reconcile once the entry is present. diff --git a/pkg/gerrit/gerrit.go b/pkg/gerrit/gerrit.go index 0fe14fe0..8db0d058 100644 --- a/pkg/gerrit/gerrit.go +++ b/pkg/gerrit/gerrit.go @@ -4,11 +4,12 @@ import ( "encoding/json" "fmt" "io" - "net" "os" "github.com/go-logr/logr" "golang.org/x/crypto/ssh" + + "github.com/epam/edp-codebase-operator/v2/pkg/sshhostkey" ) // Client is an interface for Gerrit client. @@ -65,14 +66,16 @@ func (s *SSHClient) RunCommand(cmd *SSHCommand) ([]byte, error) { } func (s *SSHClient) NewSession() (*ssh.Session, *ssh.Client, error) { - connection, err := ssh.Dial("tcp", fmt.Sprintf("%s:%d", s.Host, s.Port), s.Config) + hostPort := sshhostkey.HostPort(s.Host, int(s.Port)) + + connection, err := ssh.Dial("tcp", hostPort, s.Config) if err != nil { - return nil, nil, fmt.Errorf("failed to dial: %s", err) + return nil, nil, fmt.Errorf("failed to dial: %w", sshhostkey.Enrich(err, hostPort)) } session, err := connection.NewSession() if err != nil { - return nil, nil, fmt.Errorf("failed to create session: %s", err) + return nil, nil, fmt.Errorf("failed to create session: %w", err) } return session, connection, nil @@ -84,14 +87,18 @@ func SshInit(port int32, sshPrivateKey, host, user string, logger logr.Logger) ( return nil, fmt.Errorf("failed to get Public Key from Private one: %w", err) } + hostKeyCallback, hostKeyAlgorithms, err := sshhostkey.ClientConfig(host, port) + if err != nil { + return nil, err + } + sshConfig := &ssh.ClientConfig{ User: user, Auth: []ssh.AuthMethod{ ssh.PublicKeys(pubkey), }, - HostKeyCallback: ssh.HostKeyCallback(func(hostname string, remote net.Addr, key ssh.PublicKey) error { - return nil - }), + HostKeyCallback: hostKeyCallback, + HostKeyAlgorithms: hostKeyAlgorithms, } cl := SSHClient{ Config: sshConfig, diff --git a/pkg/gerrit/gerrit_test.go b/pkg/gerrit/gerrit_test.go index c9868804..1fe60b89 100644 --- a/pkg/gerrit/gerrit_test.go +++ b/pkg/gerrit/gerrit_test.go @@ -6,6 +6,8 @@ import ( "crypto/x509" "encoding/pem" "log" + "os" + "path/filepath" "testing" "github.com/go-logr/logr" @@ -15,6 +17,16 @@ import ( func setupSuite(tb testing.TB) (func(tb testing.TB), string) { log.Println("setup suite") + // Pin known_hosts to an empty file inside the test's temp dir. Without this + // the SSH client falls back to the developer's ~/.ssh/known_hosts and the + // tests pass locally while failing on a clean CI runner that has none. + knownHosts := filepath.Join(tb.TempDir(), "ssh_known_hosts") + if err := os.WriteFile(knownHosts, nil, 0o600); err != nil { + tb.Fatalf("failed to write test known_hosts: %v", err) + } + + tb.Setenv("SSH_KNOWN_HOSTS", knownHosts) + pk, err := rsa.GenerateKey(rand.Reader, 1024) if err != nil { tb.Fatal("failed to generate test private key") diff --git a/pkg/git/provider.go b/pkg/git/provider.go index c2fe4081..8d620553 100644 --- a/pkg/git/provider.go +++ b/pkg/git/provider.go @@ -19,6 +19,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" codebaseApi "github.com/epam/edp-codebase-operator/v2/api/v1" + "github.com/epam/edp-codebase-operator/v2/pkg/sshhostkey" ) const ( @@ -99,12 +100,14 @@ func (p *GitProvider) getAuth() (transport.AuthMethod, error) { return nil, fmt.Errorf("failed to parse SSH private key: %w", err) } + // When AuthMethod carries no HostKeyCallback, go-git loads known_hosts itself + // and derives HostKeyAlgorithms from it. Setting a callback here suppresses + // that derivation, so a server whose key type is absent from known_hosts + // fails as a key mismatch instead of a missing entry. + // See go-git plumbing/transport/ssh.command.connect. auth := &ssh.PublicKeys{ User: p.config.SSHUser, Signer: signer, - HostKeyCallbackHelper: ssh.HostKeyCallbackHelper{ - HostKeyCallback: ssh2.InsecureIgnoreHostKey(), - }, } return auth, nil @@ -119,6 +122,31 @@ func (p *GitProvider) getAuth() (transport.AuthMethod, error) { return nil, nil } +// remoteErr passes non-host-key errors through untouched, so it is safe to wrap +// every error returned by a remote operation. +func remoteErr(err error, repoURL string) error { + return sshhostkey.Enrich(err, sshTarget(repoURL)) +} + +func sshTarget(repoURL string) string { + ep, err := transport.NewEndpoint(repoURL) + if err != nil { + return repoURL + } + + return sshhostkey.HostPort(ep.Host, ep.Port) +} + +// originURL lets directory-based operations name the host they failed to verify. +func originURL(repo *git.Repository) string { + remote, err := repo.Remote("origin") + if err != nil || len(remote.Config().URLs) == 0 { + return "" + } + + return remote.Config().URLs[0] +} + // getTokenAuth formats token authentication based on the git provider type. func (p *GitProvider) getTokenAuth() transport.AuthMethod { switch p.config.GitProvider { @@ -168,7 +196,7 @@ func (p *GitProvider) Clone(ctx context.Context, repoURL, destination string) er repo, err := git.PlainCloneContext(ctx, destination, false, cloneOptions) if err != nil { - return fmt.Errorf("failed to clone repository: %w", err) + return fmt.Errorf("failed to clone repository: %w", remoteErr(err, repoURL)) } log.Info("Repository cloned successfully, now fetching all branches and tags") @@ -186,7 +214,7 @@ func (p *GitProvider) Clone(ctx context.Context, repoURL, destination string) er err = repo.FetchContext(ctx, fetchOptions) if err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) { - return fmt.Errorf("failed to fetch all branches and tags: %w", err) + return fmt.Errorf("failed to fetch all branches and tags: %w", remoteErr(err, repoURL)) } log.Info("All branches and tags fetched successfully") @@ -287,7 +315,7 @@ func (p *GitProvider) Push(ctx context.Context, directory string, refspecs ...st err = repo.PushContext(ctx, pushOptions) if err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) { - return fmt.Errorf("failed to push: %w", err) + return fmt.Errorf("failed to push: %w", remoteErr(err, originURL(repo))) } log.Info("Changes pushed successfully") @@ -332,7 +360,7 @@ func (p *GitProvider) Checkout(ctx context.Context, directory, branchName string err = repo.FetchContext(ctx, fetchOptions) if err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) { - return fmt.Errorf("failed to fetch: %w", err) + return fmt.Errorf("failed to fetch: %w", remoteErr(err, originURL(repo))) } // The refspec above maps remote branches straight into local @@ -412,7 +440,7 @@ func (p *GitProvider) ListRemoteBranches(ctx context.Context, repoURL string) ([ return nil, nil } - return nil, fmt.Errorf("failed to list remote references: %w", err) + return nil, fmt.Errorf("failed to list remote references: %w", remoteErr(err, repoURL)) } branches := make([]string, 0, len(refs)) @@ -465,6 +493,12 @@ func (p *GitProvider) CheckPermissions(ctx context.Context, repoURL string) erro return nil } + // A host key failure is neither a permission nor a lookup problem, so it + // must not be reported as one. + if sshhostkey.IsVerificationError(err) { + return remoteErr(err, repoURL) + } + return fmt.Errorf("permission denied or repository not found: %w", err) } diff --git a/pkg/git/provider_test.go b/pkg/git/provider_test.go index fbaefd1d..141990e7 100644 --- a/pkg/git/provider_test.go +++ b/pkg/git/provider_test.go @@ -1203,3 +1203,35 @@ func TestGitProvider_CheckoutRemoteBranch_NoRemote(t *testing.T) { }) } } + +// Host key lookups are keyed on the port actually dialled, which the repository +// URL determines: ssh:// carries a port, the scp-style form cannot. +func TestSshTarget(t *testing.T) { + tests := []struct { + name string + repoURL string + want string + }{ + { + name: "ssh url carries its port", + repoURL: "ssh://gerrit.example.com:29418/my-project", + want: "gerrit.example.com:29418", + }, + { + name: "scp-style url has no port and defaults to 22", + repoURL: "git@github.com:owner/repo.git", + want: "github.com:22", + }, + { + name: "ssh url with a non-standard port", + repoURL: "ssh://git@git.example.com:2222/owner/repo.git", + want: "git.example.com:2222", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, sshTarget(tt.repoURL)) + }) + } +} diff --git a/pkg/git/transport.go b/pkg/git/transport.go index 40528cd2..4c4032ba 100644 --- a/pkg/git/transport.go +++ b/pkg/git/transport.go @@ -89,7 +89,7 @@ func (p *GitProvider) CreateRemoteBranchViaRefUpdate(ctx context.Context, repoUR session, err := c.NewReceivePackSession(ep, auth) if err != nil { - return fmt.Errorf("failed to open receive-pack session: %w", err) + return fmt.Errorf("failed to open receive-pack session: %w", remoteErr(err, repoURL)) } defer func() { @@ -98,7 +98,7 @@ func (p *GitProvider) CreateRemoteBranchViaRefUpdate(ctx context.Context, repoUR advRefs, err := session.AdvertisedReferencesContext(ctx) if err != nil { - return fmt.Errorf("failed to get advertised references: %w", err) + return fmt.Errorf("failed to get advertised references: %w", remoteErr(err, repoURL)) } branchRef := plumbing.NewBranchReferenceName(branchName) @@ -162,7 +162,7 @@ func (p *GitProvider) advertisedReferences(ctx context.Context, repoURL string) session, err := c.NewUploadPackSession(ep, auth) if err != nil { - return nil, nil, fmt.Errorf("failed to open upload-pack session: %w", err) + return nil, nil, fmt.Errorf("failed to open upload-pack session: %w", remoteErr(err, repoURL)) } advRefs, err := session.AdvertisedReferencesContext(ctx) @@ -175,7 +175,7 @@ func (p *GitProvider) advertisedReferences(ctx context.Context, repoURL string) return nil, nil, fmt.Errorf("remote repository is empty or missing: %w", ErrReferenceNotFound) } - return nil, nil, fmt.Errorf("failed to get advertised references: %w", err) + return nil, nil, fmt.Errorf("failed to get advertised references: %w", remoteErr(err, repoURL)) } return advRefs, func() { _ = session.Close() }, nil diff --git a/pkg/sshhostkey/hostkey.go b/pkg/sshhostkey/hostkey.go new file mode 100644 index 00000000..963c62e9 --- /dev/null +++ b/pkg/sshhostkey/hostkey.go @@ -0,0 +1,134 @@ +// Package sshhostkey centralises SSH host key verification for every outbound +// SSH connection the operator makes. +// +// Verification is mandatory. The known_hosts source is a single file shared by +// the whole process, located by SSH_KNOWN_HOSTS and re-read on every +// connection, so entries added to it apply to connections already in flight. +package sshhostkey + +import ( + "errors" + "fmt" + "net" + "os" + "strconv" + "strings" + + gitssh "github.com/go-git/go-git/v5/plumbing/transport/ssh" + "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/knownhosts" +) + +// knownHostsEnvVar is go-git's own lookup variable, reused so that the go-git +// code paths and the raw golang.org/x/crypto/ssh code paths always agree on +// which file is authoritative. +const knownHostsEnvVar = "SSH_KNOWN_HOSTS" + +// ClientConfig returns the host key callback and the host key algorithms to set +// on a golang.org/x/crypto/ssh.ClientConfig. +// +// Both values must be applied together. Restricting HostKeyAlgorithms to the +// types actually recorded for the host prevents the server from offering a key +// type that is absent from known_hosts, which the handshake would otherwise +// report as a key mismatch rather than as the missing entry it really is. +// +// Code that talks to a git remote through go-git must NOT use this function: +// go-git derives both values itself when AuthMethod leaves HostKeyCallback nil. +func ClientConfig(host string, port int32) (ssh.HostKeyCallback, []string, error) { + db, err := gitssh.NewKnownHostsDb() + if err != nil { + return nil, nil, fmt.Errorf("failed to load SSH known_hosts (%s): %w", Source(), err) + } + + return db.HostKeyCallback(), db.HostKeyAlgorithms(HostPort(host, int(port))), nil +} + +func HostPort(host string, port int) string { + if port == 0 { + port = defaultSSHPort + } + + return net.JoinHostPort(host, strconv.Itoa(port)) +} + +// Source is for error messages only; the file itself is opened by go-git. +func Source() string { + if path := os.Getenv(knownHostsEnvVar); path != "" { + return path + } + + return "~/.ssh/known_hosts, /etc/ssh/ssh_known_hosts" +} + +// Enrich turns a host key verification failure into an actionable message. +// Errors of any other kind are returned unchanged, so it is safe to apply to +// every error returned by an SSH operation. +func Enrich(err error, hostPort string) error { + if err == nil { + return nil + } + + var revokedErr *knownhosts.RevokedError + if errors.As(err, &revokedErr) { + return fmt.Errorf( + "SSH host key for %s is marked as revoked in known_hosts (%s), refusing to connect: %w", + hostPort, Source(), err, + ) + } + + var keyErr *knownhosts.KeyError + if !errors.As(err, &keyErr) { + return err + } + + // A KeyError carrying known keys means the host is on file but presented a + // different key: either the server was rekeyed, or the connection is being + // intercepted. Never suggest "just add the key" for this case. + if len(keyErr.Want) > 0 { + return fmt.Errorf( + "SSH host key mismatch for %s: the server presented a key that does not match known_hosts (%s). "+ + "This means either the git server's host key was rotated, or the connection is being intercepted. "+ + "Verify the new key out-of-band before replacing the entry: %w", + hostPort, Source(), err, + ) + } + + return fmt.Errorf( + "SSH host key for %s is not present in known_hosts (%s). "+ + "Add it to the operator's ssh-known-hosts ConfigMap, for example: ssh-keyscan -p %s %s: %w", + hostPort, Source(), portOf(hostPort), hostOf(hostPort), err, + ) +} + +func hostOf(hostPort string) string { + if host, _, err := net.SplitHostPort(hostPort); err == nil { + return host + } + + return hostPort +} + +func portOf(hostPort string) string { + if _, port, err := net.SplitHostPort(hostPort); err == nil { + return port + } + + return strconv.Itoa(defaultSSHPort) +} + +const defaultSSHPort = 22 + +// IsVerificationError reports whether err was caused by host key verification +// rather than by connectivity, authentication or the git protocol. +func IsVerificationError(err error) bool { + var keyErr *knownhosts.KeyError + + var revokedErr *knownhosts.RevokedError + + if errors.As(err, &keyErr) || errors.As(err, &revokedErr) { + return true + } + + // go-git reports a missing or unreadable known_hosts file as a plain error. + return err != nil && strings.Contains(err.Error(), "known_hosts") +} diff --git a/pkg/sshhostkey/hostkey_test.go b/pkg/sshhostkey/hostkey_test.go new file mode 100644 index 00000000..1003d656 --- /dev/null +++ b/pkg/sshhostkey/hostkey_test.go @@ -0,0 +1,223 @@ +package sshhostkey + +import ( + "crypto/ed25519" + "crypto/rand" + "errors" + "net" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/knownhosts" +) + +const testHost = "git.example.com" + +const testPort = 2222 + +func newHostKey(t *testing.T) ssh.PublicKey { + t.Helper() + + pub, _, err := ed25519.GenerateKey(rand.Reader) + require.NoError(t, err) + + key, err := ssh.NewPublicKey(pub) + require.NoError(t, err) + + return key +} + +func writeKnownHosts(t *testing.T, key ssh.PublicKey) { + t.Helper() + + path := filepath.Join(t.TempDir(), "ssh_known_hosts") + line := knownhosts.Line([]string{HostPort(testHost, testPort)}, key) + + require.NoError(t, os.WriteFile(path, []byte(line+"\n"), 0o600)) + t.Setenv(knownHostsEnvVar, path) +} + +func TestHostPort(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + host string + port int + want string + }{ + {name: "explicit port", host: testHost, port: 2222, want: "git.example.com:2222"}, + {name: "zero port falls back to 22", host: testHost, port: 0, want: "git.example.com:22"}, + {name: "ipv6 is bracketed", host: "::1", port: 22, want: "[::1]:22"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tt.want, HostPort(tt.host, tt.port)) + }) + } +} + +func TestClientConfig_AcceptsPinnedKey(t *testing.T) { + key := newHostKey(t) + writeKnownHosts(t, key) + + callback, algorithms, err := ClientConfig(testHost, testPort) + require.NoError(t, err) + require.NotNil(t, callback) + + // Restricting the advertised algorithms to those on file is what keeps a + // missing entry from surfacing as a key mismatch during the handshake. + assert.Contains(t, algorithms, key.Type()) + + addr := &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: testPort} + assert.NoError(t, callback(HostPort(testHost, testPort), addr, key)) +} + +func TestClientConfig_RejectsUnpinnedKey(t *testing.T) { + writeKnownHosts(t, newHostKey(t)) + + callback, _, err := ClientConfig(testHost, testPort) + require.NoError(t, err) + + addr := &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: testPort} + + // A different key for a pinned host is exactly the interception case. + err = callback(HostPort(testHost, testPort), addr, newHostKey(t)) + require.Error(t, err) + + var keyErr *knownhosts.KeyError + + require.ErrorAs(t, err, &keyErr) + assert.NotEmpty(t, keyErr.Want, "a pinned host presenting a new key must report the expected keys") +} + +func TestClientConfig_RejectsUnknownHost(t *testing.T) { + writeKnownHosts(t, newHostKey(t)) + + callback, _, err := ClientConfig(testHost, testPort) + require.NoError(t, err) + + addr := &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 22} + + err = callback(HostPort("other.example.com", 22), addr, newHostKey(t)) + require.Error(t, err) + + var keyErr *knownhosts.KeyError + + require.ErrorAs(t, err, &keyErr) + assert.Empty(t, keyErr.Want, "an unpinned host has no expected keys") +} + +func TestClientConfig_MissingKnownHostsFile(t *testing.T) { + t.Setenv(knownHostsEnvVar, filepath.Join(t.TempDir(), "does-not-exist")) + + _, _, err := ClientConfig(testHost, testPort) + require.Error(t, err) + assert.Contains(t, err.Error(), "known_hosts") +} + +func TestEnrich(t *testing.T) { + t.Parallel() + + otherErr := errors.New("connection refused") + + tests := []struct { + name string + err error + wantNil bool + wantSame bool + wantContain []string + wantAbsent []string + }{ + { + name: "nil passes through", + err: nil, + wantNil: true, + }, + { + name: "unrelated error is untouched", + err: otherErr, + wantSame: true, + }, + { + name: "unknown host suggests ssh-keyscan", + err: &knownhosts.KeyError{}, + wantContain: []string{"not present in known_hosts", "ssh-keyscan -p 2222 git.example.com"}, + }, + { + name: "mismatch warns about interception", + err: &knownhosts.KeyError{Want: []knownhosts.KnownKey{{}}}, + wantContain: []string{"mismatch", "intercepted", "out-of-band"}, + // Telling an operator to add the key would walk them into trusting + // whatever the attacker presented. + wantAbsent: []string{"ssh-keyscan"}, + }, + { + name: "revoked key is refused", + err: &knownhosts.RevokedError{}, + wantContain: []string{"revoked"}, + wantAbsent: []string{"ssh-keyscan"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got := Enrich(tt.err, HostPort(testHost, testPort)) + + if tt.wantNil { + assert.NoError(t, got) + return + } + + require.Error(t, got) + + if tt.wantSame { + assert.Equal(t, tt.err, got) + return + } + + for _, want := range tt.wantContain { + assert.Contains(t, got.Error(), want) + } + + for _, absent := range tt.wantAbsent { + assert.NotContains(t, got.Error(), absent) + } + + assert.ErrorIs(t, got, tt.err, "the original error must stay in the chain") + }) + } +} + +func TestIsVerificationError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + want bool + }{ + {name: "nil", err: nil, want: false}, + {name: "key error", err: &knownhosts.KeyError{}, want: true}, + {name: "revoked error", err: &knownhosts.RevokedError{}, want: true}, + {name: "missing known_hosts file", err: errors.New("unable to find any valid known_hosts file"), want: true}, + {name: "permission denied", err: errors.New("ssh: handshake failed"), want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tt.want, IsVerificationError(tt.err)) + }) + } +}