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)) + }) + } +}