From 90f616ff983a33cdd2499099bf1bb0fe77765ee2 Mon Sep 17 00:00:00 2001 From: Paul Oostenrijk Date: Sat, 4 Jul 2026 15:48:47 +0200 Subject: [PATCH 1/2] common/sftp: Add ssh_mode: native for using golang's ssh module common/e2e_test: Add tests for both openssh and native mode to be aligned --- sftp/common/e2e_test.go | 448 +++++++++++++++++++++++++++++++++ sftp/common/native_ssh.go | 131 ++++++++++ sftp/common/native_ssh_test.go | 95 +++++++ sftp/common/sftp.go | 65 ++++- sftp/go.mod | 4 + 5 files changed, 735 insertions(+), 8 deletions(-) create mode 100644 sftp/common/e2e_test.go create mode 100644 sftp/common/native_ssh.go create mode 100644 sftp/common/native_ssh_test.go diff --git a/sftp/common/e2e_test.go b/sftp/common/e2e_test.go new file mode 100644 index 00000000..cd675914 --- /dev/null +++ b/sftp/common/e2e_test.go @@ -0,0 +1,448 @@ +package common + +import ( + "crypto/ed25519" + "crypto/rand" + "encoding/pem" + "fmt" + "net/url" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/pkg/sftp" + "github.com/stretchr/testify/assert" + "golang.org/x/crypto/ssh" +) + +func TestConnectSuccess(t *testing.T) { + endpoint := &url.URL{ + Scheme: "sftp", + Host: fixture.host + ":" + fixture.port, + Path: "/", + User: url.User("testuser"), + } + + // Each test is done on both openssh and native mode + tests := []struct { + name string + params map[string]string + }{ + { + name: "with identity", + params: map[string]string{ + "identity": fixture.clientKeyPath, + "known_hosts": fixture.knownHostsPath(), + }, + }, + { + name: "with ssh_private_key", + params: map[string]string{ + "ssh_private_key": fixture.clientKeyPEM, + "known_hosts": fixture.knownHostsPath(), + }, + }, + { + name: "without known_hosts skip verification", + params: map[string]string{ + "identity": fixture.clientKeyPath, + "insecure_ignore_host_key": "true", + }, + }, + { + name: "with identity and known_hosts", + params: map[string]string{ + "identity": fixture.clientKeyPath, + "known_hosts": fixture.knownHostsPath(), + }, + }, + { + name: "with ssh_private_key and known_hosts", + params: map[string]string{ + "ssh_private_key": fixture.clientKeyPEM, + "known_hosts": fixture.knownHostsPath(), + }, + }, + } + modes := []string{"openssh", "native"} + + for _, tt := range tests { + for _, mode := range modes { + t.Run(mode+"-"+tt.name, func(t *testing.T) { + params := cloneParams(tt.params) + params["ssh_mode"] = mode + if mode == "openssh" { + t.Cleanup(func() { + stopOpenSSHMaster(t, endpoint, params) + }) + } + + client, err := Connect(endpoint, params) + if err != nil { + t.Fatalf("failed to connect: %v", err) + } + defer client.Close() + + if _, err := client.Getwd(); err != nil { + t.Fatalf("sftp session unusable: %v", err) + } + }) + } + } +} + +func TestConnectFailure(t *testing.T) { + + endpoint := &url.URL{ + Scheme: "sftp", + Host: fixture.host + ":" + fixture.port, + Path: "/", + User: url.User("testuser"), + } + + tests := []struct { + name string + params map[string]string + errorContains []string + }{ + { + name: "native with missing identity", + params: map[string]string{ + "ssh_mode": "native", + "identity": filepath.Join(fixture.knownHostsDir, "missing_identity"), + "known_hosts": fixture.knownHostsPath(), + }, + errorContains: []string{"failed to read identity file"}, + }, + { + name: "native with invalid known_hosts", + params: map[string]string{ + "ssh_mode": "native", + "ssh_private_key": fixture.clientKeyPEM, + "known_hosts": fixture.unknownHostKeys, + }, + errorContains: []string{"key is unknown"}, + }, + { + name: "openssh with invalid known_hosts", + params: map[string]string{ + "ssh_mode": "openssh", + "ssh_private_key": fixture.clientKeyPEM, + "known_hosts": fixture.unknownHostKeys, + }, + errorContains: []string{"Host key verification failed."}, + }, + { + name: "native with invalid private key", + params: map[string]string{ + "ssh_mode": "native", + "ssh_private_key": "invalid", + "known_hosts": fixture.knownHostsPath(), + }, + errorContains: []string{"failed to parse ssh_private_key", "no key found"}, + }, + { + name: "openssh with invalid private key", + params: map[string]string{ + "ssh_mode": "openssh", + "ssh_private_key": "invalid", + "known_hosts": fixture.knownHostsPath(), + }, + errorContains: []string{"Error loading key", "invalid format"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client, err := Connect(endpoint, tt.params) + for _, errorContains := range tt.errorContains { + assert.ErrorContains(t, err, errorContains, tt.name) + } + assert.Nil(t, client) + }) + } +} + +func cloneParams(params map[string]string) map[string]string { + cloned := make(map[string]string, len(params)+1) + for k, v := range params { + cloned[k] = v + } + return cloned +} + +func stopOpenSSHMaster(t *testing.T, endpoint *url.URL, params map[string]string) { + t.Helper() + + sock, err := controlSock(endpoint, params) + if err != nil { + t.Logf("failed to resolve ssh control socket: %v", err) + return + } + + args := []string{"-S", sock, "-O", "exit"} + if p := endpoint.Port(); p != "" { + args = append(args, "-p", p) + } + if endpoint.User != nil { + args = append(args, "-l", endpoint.User.Username()) + } + args = append(args, endpoint.Hostname()) + + _ = exec.Command("ssh", args...).Run() +} + +// sftpFixture describes the shared, package-wide sftp server container. +type sftpFixture struct { + containerID string + host string // always 127.0.0.1 + port string // mapped host port for container port 22 + clientKeyPEM string // OpenSSH private key accepted by the server + clientKeyPath string // temp identity file accepted by OpenSSH + knownHosts string // known_hosts content matching the server's host key + port + unknownHostKeys string // unknown host keys content + knownHostsDir string // temp dir containing a ready-to-use "known_hosts" file +} + +var fixture *sftpFixture + +// TestMain sets up the sftp fixture and runs the tests, reusing the same fixture over multiple tests. +func TestMain(m *testing.M) { + if _, err := exec.LookPath("docker"); err != nil { + fmt.Fprintln(os.Stderr, "docker not found in PATH; skipping docker-tagged tests") + os.Exit(0) + } + + f, err := startSFTPServer() + if err != nil { + fmt.Fprintf(os.Stderr, "failed to start sftp fixture: %v\n", err) + os.Exit(1) + } + fixture = f + + code := m.Run() + + f.stop() + os.Exit(code) +} + +// startSFTPServer generates an ephemeral client key, boots one atmoz/sftp +// container, and waits until the SFTP subsystem is reachable. +func startSFTPServer() (*sftpFixture, error) { + dir, err := os.MkdirTemp("", "sftp-fixture-") + if err != nil { + return nil, err + } + success := false + defer func() { + if !success { + _ = os.RemoveAll(dir) + } + }() + + // Client key pair: private key is fed to connectNativeSSH, public key is + // authorized on the server. + clientPEM, clientAuthorized, err := genKeyPair() + if err != nil { + return nil, err + } + clientKeyPath := filepath.Join(dir, "id_ed25519") + if err := os.WriteFile(clientKeyPath, []byte(clientPEM), 0600); err != nil { + return nil, err + } + + // atmoz/sftp copies every *.pub under /home//.ssh/keys into the + // user's authorized_keys at startup. + keysDir := filepath.Join(dir, "keys") + if err := os.MkdirAll(keysDir, 0755); err != nil { + return nil, err + } + if err := os.WriteFile(filepath.Join(keysDir, "client.pub"), []byte(clientAuthorized), 0644); err != nil { + return nil, err + } + + // Bind container port 22 to a random loopback port so parallel runs (and + // whatever is already using :22) never collide. + runOut, err := exec.Command("docker", "run", "-d", + "-p", "127.0.0.1::22", + "-v", keysDir+":/home/testuser/.ssh/keys:ro", + "atmoz/sftp", "testuser::1001", + ).Output() + if err != nil { + return nil, fmt.Errorf("docker run: %w", asExecErr(err)) + } + containerID := strings.TrimSpace(string(runOut)) + + f := &sftpFixture{ + containerID: containerID, + host: "127.0.0.1", + clientKeyPEM: clientPEM, + clientKeyPath: clientKeyPath, + knownHostsDir: dir, + } + + port, err := dockerMappedPort(containerID, "22") + if err != nil { + f.stop() + return nil, err + } + f.port = port + + if err := waitForSSH(f, 60*time.Second); err != nil { + f.stop() + return nil, err + } + + // Build known_hosts from the server's *actual* host keys (all algorithms it + // serves), so verification succeeds regardless of which host-key algorithm + // the Go client negotiates. + if err := f.writeKnownHostsFromContainer(); err != nil { + f.stop() + return nil, err + } + + fakeHostKeys := ` + 127.0.0.1 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAID3ZhoyDd985pxjRly55oSAWdvNQEBJFMWceKsIcFpzV + 127.0.0.1 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC0MBAGzc743Qro2yFD/aAQRiw6w4BFzptGxvKSZbJ0oicmwcL756yxIzPyVNC3rM3vpJzq98UDcBuc4KfK3IHwvQqmahbPCJtZJHZzNbQhfHxFDuZSRONTq3c0PUcYlVGGQ56KcAXM4C/ik4MNI/SSNZjh1hxNYzhMp2rlxyUYKA+79Sm3oMXOjd099hu96NeRbV8uhePLT0EV+mS853JLWAXAAvqfNCnOFHeRHuJGUYPsl897PHUsRfKU2bFBztHyTX+s6L+B9l6Dq3wi7cDfczAJPCzT5Ryl6G7Ywp5lEJFn1dXio1cqihdA/iag+IwLXC5hjNWOSczjRn94jtstiXP8uVbluzI2qR/BQ5tHFQcP4F5V/qTgwl0DsMzmkPreBFG6yBMQc5Zth/RdNmX5yVPY0UG2631IVeDLrO9Ep0wbk13f4hcy8wCDBF8f3o7XQZrzw6Db/D+NZL1p4toCCkpxL5FRBWAXB0MIJ1oMbiMLLuM/2At3tTKZ7Lq7eF8= + 127.0.0.1 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBOSjgoFyp8nQBRXL7Y/6nl2HyA59BHMrwMrlIrhBaQ/DTN8Z5SX0WNyTsCnYfatYMh7Vnsf5bF3lSfKnxvlD6dc= + ` + // Write fake host keys to a file + unknownHostKeysPath := filepath.Join(f.knownHostsDir, "unknown_host_keys") + if err := os.WriteFile(unknownHostKeysPath, []byte(fakeHostKeys), 0644); err != nil { + return nil, fmt.Errorf("failed to write host keys: %w", err) + } + f.unknownHostKeys = unknownHostKeysPath + + success = true + return f, nil +} + +// writeKnownHostsFromContainer scrapes the server's public host keys and writes a +// known_hosts file entry (in "[host]:port" form for the mapped port) for each. +func (f *sftpFixture) writeKnownHostsFromContainer() error { + out, err := exec.Command("docker", "exec", f.containerID, + "sh", "-c", "cat /etc/ssh/ssh_host_*_key.pub").Output() + if err != nil { + return fmt.Errorf("read host keys: %w", asExecErr(err)) + } + + var lines []string + for _, l := range strings.Split(strings.TrimSpace(string(out)), "\n") { + fields := strings.Fields(l) + if len(fields) < 2 { + continue + } + // "[host]:port keytype base64" — drop any trailing comment field. + lines = append(lines, fmt.Sprintf("[%s]:%s %s %s", f.host, f.port, fields[0], fields[1])) + } + if len(lines) == 0 { + return fmt.Errorf("no host keys found on server") + } + + f.knownHosts = strings.Join(lines, "\n") + "\n" + return os.WriteFile(f.knownHostsPath(), []byte(f.knownHosts), 0644) +} + +func (f *sftpFixture) knownHostsPath() string { + return filepath.Join(f.knownHostsDir, "known_hosts") +} + +func (f *sftpFixture) stop() { + if f == nil { + return + } + if f.containerID != "" { + _ = exec.Command("docker", "rm", "-f", f.containerID).Run() + } + if f.knownHostsDir != "" { + _ = os.RemoveAll(f.knownHostsDir) + } +} + +// genKeyPair returns an OpenSSH-format private key (PEM) and its matching +// authorized_keys line, using ed25519. No external tooling required. +func genKeyPair() (privPEM string, authorizedKey string, err error) { + _, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + return "", "", err + } + block, err := ssh.MarshalPrivateKey(priv, "") + if err != nil { + return "", "", err + } + signer, err := ssh.NewSignerFromKey(priv) + if err != nil { + return "", "", err + } + return string(pem.EncodeToMemory(block)), string(ssh.MarshalAuthorizedKey(signer.PublicKey())), nil +} + +// dockerMappedPort resolves the host port bound to the given container port. +func dockerMappedPort(containerID, containerPort string) (string, error) { + out, err := exec.Command("docker", "port", containerID, containerPort).Output() + if err != nil { + return "", fmt.Errorf("docker port: %w", asExecErr(err)) + } + // Output looks like "127.0.0.1:49153" (possibly multiple lines). + line := strings.TrimSpace(strings.SplitN(string(out), "\n", 2)[0]) + i := strings.LastIndex(line, ":") + if i < 0 { + return "", fmt.Errorf("unexpected docker port output: %q", string(out)) + } + return line[i+1:], nil +} + +// waitForSSH blocks until the server can serve a full SFTP session, not merely +// accept a TCP connection or complete an SSH handshake. +func waitForSSH(f *sftpFixture, timeout time.Duration) error { + signer, err := ssh.ParsePrivateKey([]byte(f.clientKeyPEM)) + if err != nil { + return err + } + cfg := &ssh.ClientConfig{ + User: "testuser", + Auth: []ssh.AuthMethod{ssh.PublicKeys(signer)}, + HostKeyCallback: ssh.InsecureIgnoreHostKey(), + Timeout: 3 * time.Second, + } + addr := f.host + ":" + f.port + deadline := time.Now().Add(timeout) + var lastErr error + for time.Now().Before(deadline) { + if err := trySFTP(addr, cfg); err == nil { + return nil + } else { + lastErr = err + } + time.Sleep(500 * time.Millisecond) + } + return fmt.Errorf("timed out waiting for sftp at %s: %w", addr, lastErr) +} + +// trySFTP performs one full SSH+SFTP handshake and tears it down. +func trySFTP(addr string, cfg *ssh.ClientConfig) error { + conn, err := ssh.Dial("tcp", addr, cfg) + if err != nil { + return err + } + defer conn.Close() + + client, err := sftp.NewClient(conn) + if err != nil { + return err + } + defer client.Close() + + _, err = client.Getwd() + return err +} + +func asExecErr(err error) error { + if ee, ok := err.(*exec.ExitError); ok { + return fmt.Errorf("%w: %s", err, strings.TrimSpace(string(ee.Stderr))) + } + return err +} diff --git a/sftp/common/native_ssh.go b/sftp/common/native_ssh.go new file mode 100644 index 00000000..494a1fdb --- /dev/null +++ b/sftp/common/native_ssh.go @@ -0,0 +1,131 @@ +package common + +import ( + "fmt" + "net" + "net/url" + "os" + "path/filepath" + + "github.com/pkg/sftp" + "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/agent" + "golang.org/x/crypto/ssh/knownhosts" +) + +// nativeSFTPClient is a wrapper around *sftp.Client that ensures the ssh connection +// is closed when the sftp client is closed. +type nativeSFTPClient struct { + *sftp.Client + conn *ssh.Client +} + +func (c *nativeSFTPClient) Close() error { + err := c.Client.Close() + if cerr := c.conn.Close(); err == nil { + err = cerr + } + return err +} + +func buildAuthMethods(params map[string]string) ([]ssh.AuthMethod, error) { + var methods []ssh.AuthMethod + + if key := params["ssh_private_key"]; key != "" { + signer, err := ssh.ParsePrivateKey([]byte(key)) + if err != nil { + return nil, fmt.Errorf("failed to parse ssh_private_key: %w", err) + } + methods = append(methods, ssh.PublicKeys(signer)) + } + + if id := params["identity"]; id != "" { + data, err := os.ReadFile(id) + if err != nil { + return nil, fmt.Errorf("failed to read identity file: %w", err) + } + signer, err := ssh.ParsePrivateKey(data) + if err != nil { + return nil, fmt.Errorf("failed to parse identity file: %w", err) + } + methods = append(methods, ssh.PublicKeys(signer)) + } + + if sock := params["ssh_auth_sock"]; sock != "" { + if conn, err := net.Dial("unix", sock); err == nil { + ag := agent.NewClient(conn) + methods = append(methods, ssh.PublicKeysCallback(ag.Signers)) + } + } + + if len(methods) == 0 { + return nil, fmt.Errorf("no authentication method available (set identity, ssh_private_key, or ssh_auth_sock)") + } + + return methods, nil +} + +func hostKeyCallback(params map[string]string) (ssh.HostKeyCallback, error) { + if params["insecure_ignore_host_key"] == "true" { + return ssh.InsecureIgnoreHostKey(), nil + } + + khPath := params["known_hosts"] + if khPath == "" { + home, err := os.UserHomeDir() + if err != nil { + return nil, fmt.Errorf("failed to resolve known_hosts path: %w", err) + } + khPath = filepath.Join(home, ".ssh", "known_hosts") + } + + cb, err := knownhosts.New(khPath) + if err != nil { + return nil, fmt.Errorf("failed to load known_hosts (%s): %w", khPath, err) + } + return cb, nil +} + +// connectNativeSSH connects to the remote server using the native SSH client +func connectNativeSSH(endpoint *url.URL, params map[string]string) (*sftp.Client, error) { + user, err := resolveUsername(endpoint, params) + if err != nil { + return nil, err + } + + auth, err := buildAuthMethods(params) + if err != nil { + return nil, err + } + + hkcb, err := hostKeyCallback(params) + if err != nil { + return nil, err + } + + cfg := &ssh.ClientConfig{ + User: user, + Auth: auth, + HostKeyCallback: hkcb, + } + + addr := endpoint.Hostname() + if p := endpoint.Port(); p != "" { + addr += ":" + p + } else { + addr += ":22" + } + + conn, err := ssh.Dial("tcp", addr, cfg) + if err != nil { + return nil, fmt.Errorf("ssh dial: %w", err) + } + + client, err := sftp.NewClient(conn) + if err != nil { + conn.Close() + return nil, err + } + + return client, nil +} diff --git a/sftp/common/native_ssh_test.go b/sftp/common/native_ssh_test.go new file mode 100644 index 00000000..ce772d48 --- /dev/null +++ b/sftp/common/native_ssh_test.go @@ -0,0 +1,95 @@ +package common + +import ( + "net/url" + "os" + "path/filepath" + "testing" +) + +var testSSHCert = ` +-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW +QyNTUxOQAAACCeF/EJkMysMrz2pWZfs95OtIqbDkz5jJHdXEI2aQ+ZQAAAALBS9q/GUvav +xgAAAAtzc2gtZWQyNTUxOQAAACCeF/EJkMysMrz2pWZfs95OtIqbDkz5jJHdXEI2aQ+ZQA +AAAEChwRRqrpme6kwm/PVrr7AmODBU2ZpcMy0eLmOJn6EdpJ4X8QmQzKwyvPalZl+z3k60 +ipsOTPmMkd1cQjZpD5lAAAAAKXBhdWxvb3N0ZW5yaWprQE1hY0Jvb2stUHJvLXZhbi1QYX +VsLmxvY2FsAQIDBA== +-----END OPENSSH PRIVATE KEY----- +` + +var testKnownHosts = ` +127.0.0.1 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAID3ZhoyDd985pxjRly55oSAWdvNQEBJFMWceKsIcFpzV +127.0.0.1 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC0MBAGzc743Qro2yFD/aAQRiw6w4BFzptGxvKSZbJ0oicmwcL756yxIzPyVNC3rM3vpJzq98UDcBuc4KfK3IHwvQqmahbPCJtZJHZzNbQhfHxFDuZSRONTq3c0PUcYlVGGQ56KcAXM4C/ik4MNI/SSNZjh1hxNYzhMp2rlxyUYKA+79Sm3oMXOjd099hu96NeRbV8uhePLT0EV+mS853JLWAXAAvqfNCnOFHeRHuJGUYPsl897PHUsRfKU2bFBztHyTX+s6L+B9l6Dq3wi7cDfczAJPCzT5Ryl6G7Ywp5lEJFn1dXio1cqihdA/iag+IwLXC5hjNWOSczjRn94jtstiXP8uVbluzI2qR/BQ5tHFQcP4F5V/qTgwl0DsMzmkPreBFG6yBMQc5Zth/RdNmX5yVPY0UG2631IVeDLrO9Ep0wbk13f4hcy8wCDBF8f3o7XQZrzw6Db/D+NZL1p4toCCkpxL5FRBWAXB0MIJ1oMbiMLLuM/2At3tTKZ7Lq7eF8= +127.0.0.1 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBOSjgoFyp8nQBRXL7Y/6nl2HyA59BHMrwMrlIrhBaQ/DTN8Z5SX0WNyTsCnYfatYMh7Vnsf5bF3lSfKnxvlD6dc= +` + +func TestBuildAuthMethodsSuccess(t *testing.T) { + got, err := buildAuthMethods(map[string]string{ + "ssh_private_key": testSSHCert, + }) + if err != nil { + t.Fatalf("failed to build auth methods: %v", err) + } + if len(got) != 1 { + t.Fatalf("got %d auth methods, want 1", len(got)) + } +} + +func TestBuildAuthMethodsFailure(t *testing.T) { + got, err := buildAuthMethods(map[string]string{ + "ssh_private_key": "invalid", + }) + if err == nil { + t.Fatalf("expected error, got nil") + } + if got != nil { + t.Fatalf("got %v, want nil", got) + } +} + +func TestHostKeyCallbackFailureInvalidKnownHosts(t *testing.T) { + testDir := t.TempDir() + os.WriteFile(filepath.Join(testDir, "known_hosts"), []byte("invalid"), 0644) + got, err := hostKeyCallback(map[string]string{ + "known_hosts": filepath.Join(testDir, "known_hosts"), + }) + if err == nil { + t.Fatalf("expected error, got nil") + } + if got != nil { + t.Fatalf("got %v, want nil", got) + } +} + +func TestHostKeyCallbackSuccessValid(t *testing.T) { + testDir := t.TempDir() + os.WriteFile(filepath.Join(testDir, "known_hosts"), []byte(testKnownHosts), 0644) + got, err := hostKeyCallback(map[string]string{ + "known_hosts": filepath.Join(testDir, "known_hosts"), + }) + if err != nil { + t.Fatalf("failed to build host key callback: %v", err) + } + if got == nil { + t.Fatalf("got nil, want non-nil") + } +} + +func TestConnectNativeSSHWithoutUsernameFailure(t *testing.T) { + endpoint := &url.URL{ + Scheme: "sftp", + Host: "localhost", + Path: "/", + User: nil, + } + client, err := connectNativeSSH(endpoint, map[string]string{ + "ssh_mode": "native", + }) + if err == nil { + t.Fatalf("expected error, got nil") + } + if client != nil { + t.Fatalf("got %v, want nil", client) + } +} diff --git a/sftp/common/sftp.go b/sftp/common/sftp.go index dd1222c1..b50795f7 100644 --- a/sftp/common/sftp.go +++ b/sftp/common/sftp.go @@ -9,12 +9,44 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "sync" "github.com/pkg/sftp" ) +func Connect(endpoint *url.URL, params map[string]string) (*sftp.Client, error) { + if endpoint == nil { + return nil, fmt.Errorf("nil endpoint") + } + if endpoint.Hostname() == "" { + return nil, fmt.Errorf("missing hostname in endpoint: %q", endpoint.String()) + } + + switch mode := resolveSSHMode(params); mode { + case "openssh": + if runtime.GOOS == "windows" { + return nil, fmt.Errorf("ssh_mode=openssh is not supported on windows") + } + return connectOpenSSH(endpoint, params) + case "native": + return connectNativeSSH(endpoint, params) + default: + return nil, fmt.Errorf("unknown ssh_mode %q (want %q or %q)", mode, "openssh", "native") + } +} + +func resolveSSHMode(params map[string]string) string { + if m := params["ssh_mode"]; m != "" { + return m + } + if runtime.GOOS == "windows" { + return "native" + } + return "openssh" +} + func controlSock(endpoint *url.URL, params map[string]string) (string, error) { if endpoint == nil { return "", fmt.Errorf("nil endpoint") @@ -27,6 +59,7 @@ func controlSock(endpoint *url.URL, params map[string]string) (string, error) { // guard master creation per ControlPath var masterMu sync.Map // map[string]*sync.Mutex + func lockFor(sock string) *sync.Mutex { m, _ := masterMu.LoadOrStore(sock, &sync.Mutex{}) return m.(*sync.Mutex) @@ -93,6 +126,10 @@ func ensureMaster(endpoint *url.URL, params map[string]string) (string, error) { // args = append(args, "-o", "UserKnownHostsFile=/dev/null") ? } + if kh := params["known_hosts"]; kh != "" { + args = append(args, "-o", "UserKnownHostsFile="+kh) + } + if id := params["identity"]; id != "" { args = append(args, "-i", id) } @@ -176,7 +213,7 @@ func ensureMaster(endpoint *url.URL, params map[string]string) (string, error) { return sock, nil } -func Connect(endpoint *url.URL, params map[string]string) (*sftp.Client, error) { +func connectOpenSSH(endpoint *url.URL, params map[string]string) (*sftp.Client, error) { if endpoint == nil { return nil, fmt.Errorf("nil endpoint") } @@ -200,19 +237,21 @@ func Connect(endpoint *url.URL, params map[string]string) (*sftp.Client, error) args = append(args, "-o", "StrictHostKeyChecking=no") } + if kh := params["known_hosts"]; kh != "" { + args = append(args, "-o", "UserKnownHostsFile="+kh) + } + if id := params["identity"]; id != "" { args = append(args, "-i", id) } - // username resolution: forbid both user@host AND username param - if endpoint.User != nil && params["username"] != "" { - return nil, fmt.Errorf("can not use user@host foo syntax and username parameter") - } else if endpoint.User != nil { - args = append(args, "-l", endpoint.User.Username()) - } else if params["username"] != "" { - args = append(args, "-l", params["username"]) + username, err := resolveUsername(endpoint, params) + if err != nil { + return nil, err } + args = append(args, "-l", username) + if p := endpoint.Port(); p != "" { args = append(args, "-p", p) } @@ -267,3 +306,13 @@ func Connect(endpoint *url.URL, params map[string]string) (*sftp.Client, error) return client, nil } + +func resolveUsername(endpoint *url.URL, params map[string]string) (string, error) { + if endpoint.User != nil && params["username"] != "" { + return "", fmt.Errorf("can not use user@host syntax and username parameter") + } + if endpoint.User != nil { + return endpoint.User.Username(), nil + } + return params["username"], nil +} diff --git a/sftp/go.mod b/sftp/go.mod index df7dbcb1..11f8ef97 100644 --- a/sftp/go.mod +++ b/sftp/go.mod @@ -11,6 +11,7 @@ require ( require ( github.com/PlakarKorp/integration-grpc v1.1.0 // indirect + github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/dustin/go-humanize v1.0.1 // indirect github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect github.com/go-git/go-billy/v5 v5.9.0 // indirect @@ -24,7 +25,9 @@ require ( github.com/ncruces/go-strftime v1.0.0 // indirect github.com/nickball/go-aes-key-wrap v0.0.0-20170929221519-1c3aa3e4dfc5 // indirect github.com/pierrec/lz4/v4 v4.1.27 // indirect + github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect + github.com/stretchr/testify v1.11.1 // indirect github.com/tink-crypto/tink-go/v2 v2.6.0 // indirect github.com/vmihailenco/msgpack/v5 v5.4.1 // indirect github.com/vmihailenco/tagparser/v2 v2.0.0 // indirect @@ -38,6 +41,7 @@ require ( google.golang.org/grpc v1.81.1 // indirect google.golang.org/protobuf v1.36.11 // indirect gopkg.in/warnings.v0 v0.1.2 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect modernc.org/libc v1.72.3 // indirect modernc.org/mathutil v1.7.1 // indirect modernc.org/memory v1.11.0 // indirect From 9242db1d237edf6d428392bb84349e15150c53b2 Mon Sep 17 00:00:00 2001 From: Paul Oostenrijk Date: Sat, 4 Jul 2026 16:01:45 +0200 Subject: [PATCH 2/2] remove known_hosts feature as to limit scope of PR (add to seperate pr later) --- sftp/common/e2e_test.go | 148 ++++++++------------------------- sftp/common/native_ssh.go | 21 +---- sftp/common/native_ssh_test.go | 36 -------- sftp/common/sftp.go | 8 -- sftp/go.mod | 6 +- 5 files changed, 35 insertions(+), 184 deletions(-) diff --git a/sftp/common/e2e_test.go b/sftp/common/e2e_test.go index cd675914..ba01e053 100644 --- a/sftp/common/e2e_test.go +++ b/sftp/common/e2e_test.go @@ -14,7 +14,6 @@ import ( "time" "github.com/pkg/sftp" - "github.com/stretchr/testify/assert" "golang.org/x/crypto/ssh" ) @@ -33,37 +32,16 @@ func TestConnectSuccess(t *testing.T) { }{ { name: "with identity", - params: map[string]string{ - "identity": fixture.clientKeyPath, - "known_hosts": fixture.knownHostsPath(), - }, - }, - { - name: "with ssh_private_key", - params: map[string]string{ - "ssh_private_key": fixture.clientKeyPEM, - "known_hosts": fixture.knownHostsPath(), - }, - }, - { - name: "without known_hosts skip verification", params: map[string]string{ "identity": fixture.clientKeyPath, "insecure_ignore_host_key": "true", }, }, { - name: "with identity and known_hosts", - params: map[string]string{ - "identity": fixture.clientKeyPath, - "known_hosts": fixture.knownHostsPath(), - }, - }, - { - name: "with ssh_private_key and known_hosts", + name: "with ssh_private_key", params: map[string]string{ - "ssh_private_key": fixture.clientKeyPEM, - "known_hosts": fixture.knownHostsPath(), + "ssh_private_key": fixture.clientKeyPEM, + "insecure_ignore_host_key": "true", }, }, } @@ -111,45 +89,27 @@ func TestConnectFailure(t *testing.T) { { name: "native with missing identity", params: map[string]string{ - "ssh_mode": "native", - "identity": filepath.Join(fixture.knownHostsDir, "missing_identity"), - "known_hosts": fixture.knownHostsPath(), + "ssh_mode": "native", + "identity": filepath.Join(fixture.dir, "missing_identity"), + "insecure_ignore_host_key": "true", }, errorContains: []string{"failed to read identity file"}, }, - { - name: "native with invalid known_hosts", - params: map[string]string{ - "ssh_mode": "native", - "ssh_private_key": fixture.clientKeyPEM, - "known_hosts": fixture.unknownHostKeys, - }, - errorContains: []string{"key is unknown"}, - }, - { - name: "openssh with invalid known_hosts", - params: map[string]string{ - "ssh_mode": "openssh", - "ssh_private_key": fixture.clientKeyPEM, - "known_hosts": fixture.unknownHostKeys, - }, - errorContains: []string{"Host key verification failed."}, - }, { name: "native with invalid private key", params: map[string]string{ - "ssh_mode": "native", - "ssh_private_key": "invalid", - "known_hosts": fixture.knownHostsPath(), + "ssh_mode": "native", + "ssh_private_key": "invalid", + "insecure_ignore_host_key": "true", }, errorContains: []string{"failed to parse ssh_private_key", "no key found"}, }, { name: "openssh with invalid private key", params: map[string]string{ - "ssh_mode": "openssh", - "ssh_private_key": "invalid", - "known_hosts": fixture.knownHostsPath(), + "ssh_mode": "openssh", + "ssh_private_key": "invalid", + "insecure_ignore_host_key": "true", }, errorContains: []string{"Error loading key", "invalid format"}, }, @@ -158,10 +118,20 @@ func TestConnectFailure(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { client, err := Connect(endpoint, tt.params) + if err == nil { + if client != nil { + client.Close() + } + t.Fatalf("expected error, got nil") + } for _, errorContains := range tt.errorContains { - assert.ErrorContains(t, err, errorContains, tt.name) + if !strings.Contains(err.Error(), errorContains) { + t.Fatalf("got error %q, want it to contain %q", err, errorContains) + } + } + if client != nil { + t.Fatalf("got %v, want nil", client) } - assert.Nil(t, client) }) } } @@ -197,14 +167,12 @@ func stopOpenSSHMaster(t *testing.T, endpoint *url.URL, params map[string]string // sftpFixture describes the shared, package-wide sftp server container. type sftpFixture struct { - containerID string - host string // always 127.0.0.1 - port string // mapped host port for container port 22 - clientKeyPEM string // OpenSSH private key accepted by the server - clientKeyPath string // temp identity file accepted by OpenSSH - knownHosts string // known_hosts content matching the server's host key + port - unknownHostKeys string // unknown host keys content - knownHostsDir string // temp dir containing a ready-to-use "known_hosts" file + containerID string + host string // always 127.0.0.1 + port string // mapped host port for container port 22 + clientKeyPEM string // OpenSSH private key accepted by the server + clientKeyPath string // temp identity file accepted by OpenSSH + dir string // temp dir containing fixture files } var fixture *sftpFixture @@ -281,7 +249,7 @@ func startSFTPServer() (*sftpFixture, error) { host: "127.0.0.1", clientKeyPEM: clientPEM, clientKeyPath: clientKeyPath, - knownHostsDir: dir, + dir: dir, } port, err := dockerMappedPort(containerID, "22") @@ -296,60 +264,10 @@ func startSFTPServer() (*sftpFixture, error) { return nil, err } - // Build known_hosts from the server's *actual* host keys (all algorithms it - // serves), so verification succeeds regardless of which host-key algorithm - // the Go client negotiates. - if err := f.writeKnownHostsFromContainer(); err != nil { - f.stop() - return nil, err - } - - fakeHostKeys := ` - 127.0.0.1 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAID3ZhoyDd985pxjRly55oSAWdvNQEBJFMWceKsIcFpzV - 127.0.0.1 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC0MBAGzc743Qro2yFD/aAQRiw6w4BFzptGxvKSZbJ0oicmwcL756yxIzPyVNC3rM3vpJzq98UDcBuc4KfK3IHwvQqmahbPCJtZJHZzNbQhfHxFDuZSRONTq3c0PUcYlVGGQ56KcAXM4C/ik4MNI/SSNZjh1hxNYzhMp2rlxyUYKA+79Sm3oMXOjd099hu96NeRbV8uhePLT0EV+mS853JLWAXAAvqfNCnOFHeRHuJGUYPsl897PHUsRfKU2bFBztHyTX+s6L+B9l6Dq3wi7cDfczAJPCzT5Ryl6G7Ywp5lEJFn1dXio1cqihdA/iag+IwLXC5hjNWOSczjRn94jtstiXP8uVbluzI2qR/BQ5tHFQcP4F5V/qTgwl0DsMzmkPreBFG6yBMQc5Zth/RdNmX5yVPY0UG2631IVeDLrO9Ep0wbk13f4hcy8wCDBF8f3o7XQZrzw6Db/D+NZL1p4toCCkpxL5FRBWAXB0MIJ1oMbiMLLuM/2At3tTKZ7Lq7eF8= - 127.0.0.1 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBOSjgoFyp8nQBRXL7Y/6nl2HyA59BHMrwMrlIrhBaQ/DTN8Z5SX0WNyTsCnYfatYMh7Vnsf5bF3lSfKnxvlD6dc= - ` - // Write fake host keys to a file - unknownHostKeysPath := filepath.Join(f.knownHostsDir, "unknown_host_keys") - if err := os.WriteFile(unknownHostKeysPath, []byte(fakeHostKeys), 0644); err != nil { - return nil, fmt.Errorf("failed to write host keys: %w", err) - } - f.unknownHostKeys = unknownHostKeysPath - success = true return f, nil } -// writeKnownHostsFromContainer scrapes the server's public host keys and writes a -// known_hosts file entry (in "[host]:port" form for the mapped port) for each. -func (f *sftpFixture) writeKnownHostsFromContainer() error { - out, err := exec.Command("docker", "exec", f.containerID, - "sh", "-c", "cat /etc/ssh/ssh_host_*_key.pub").Output() - if err != nil { - return fmt.Errorf("read host keys: %w", asExecErr(err)) - } - - var lines []string - for _, l := range strings.Split(strings.TrimSpace(string(out)), "\n") { - fields := strings.Fields(l) - if len(fields) < 2 { - continue - } - // "[host]:port keytype base64" — drop any trailing comment field. - lines = append(lines, fmt.Sprintf("[%s]:%s %s %s", f.host, f.port, fields[0], fields[1])) - } - if len(lines) == 0 { - return fmt.Errorf("no host keys found on server") - } - - f.knownHosts = strings.Join(lines, "\n") + "\n" - return os.WriteFile(f.knownHostsPath(), []byte(f.knownHosts), 0644) -} - -func (f *sftpFixture) knownHostsPath() string { - return filepath.Join(f.knownHostsDir, "known_hosts") -} - func (f *sftpFixture) stop() { if f == nil { return @@ -357,8 +275,8 @@ func (f *sftpFixture) stop() { if f.containerID != "" { _ = exec.Command("docker", "rm", "-f", f.containerID).Run() } - if f.knownHostsDir != "" { - _ = os.RemoveAll(f.knownHostsDir) + if f.dir != "" { + _ = os.RemoveAll(f.dir) } } diff --git a/sftp/common/native_ssh.go b/sftp/common/native_ssh.go index 494a1fdb..1e4b7178 100644 --- a/sftp/common/native_ssh.go +++ b/sftp/common/native_ssh.go @@ -5,12 +5,10 @@ import ( "net" "net/url" "os" - "path/filepath" "github.com/pkg/sftp" "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" - "golang.org/x/crypto/ssh/knownhosts" ) // nativeSFTPClient is a wrapper around *sftp.Client that ensures the ssh connection @@ -66,24 +64,7 @@ func buildAuthMethods(params map[string]string) ([]ssh.AuthMethod, error) { } func hostKeyCallback(params map[string]string) (ssh.HostKeyCallback, error) { - if params["insecure_ignore_host_key"] == "true" { - return ssh.InsecureIgnoreHostKey(), nil - } - - khPath := params["known_hosts"] - if khPath == "" { - home, err := os.UserHomeDir() - if err != nil { - return nil, fmt.Errorf("failed to resolve known_hosts path: %w", err) - } - khPath = filepath.Join(home, ".ssh", "known_hosts") - } - - cb, err := knownhosts.New(khPath) - if err != nil { - return nil, fmt.Errorf("failed to load known_hosts (%s): %w", khPath, err) - } - return cb, nil + return ssh.InsecureIgnoreHostKey(), nil } // connectNativeSSH connects to the remote server using the native SSH client diff --git a/sftp/common/native_ssh_test.go b/sftp/common/native_ssh_test.go index ce772d48..1a9e3987 100644 --- a/sftp/common/native_ssh_test.go +++ b/sftp/common/native_ssh_test.go @@ -2,8 +2,6 @@ package common import ( "net/url" - "os" - "path/filepath" "testing" ) @@ -18,12 +16,6 @@ VsLmxvY2FsAQIDBA== -----END OPENSSH PRIVATE KEY----- ` -var testKnownHosts = ` -127.0.0.1 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAID3ZhoyDd985pxjRly55oSAWdvNQEBJFMWceKsIcFpzV -127.0.0.1 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC0MBAGzc743Qro2yFD/aAQRiw6w4BFzptGxvKSZbJ0oicmwcL756yxIzPyVNC3rM3vpJzq98UDcBuc4KfK3IHwvQqmahbPCJtZJHZzNbQhfHxFDuZSRONTq3c0PUcYlVGGQ56KcAXM4C/ik4MNI/SSNZjh1hxNYzhMp2rlxyUYKA+79Sm3oMXOjd099hu96NeRbV8uhePLT0EV+mS853JLWAXAAvqfNCnOFHeRHuJGUYPsl897PHUsRfKU2bFBztHyTX+s6L+B9l6Dq3wi7cDfczAJPCzT5Ryl6G7Ywp5lEJFn1dXio1cqihdA/iag+IwLXC5hjNWOSczjRn94jtstiXP8uVbluzI2qR/BQ5tHFQcP4F5V/qTgwl0DsMzmkPreBFG6yBMQc5Zth/RdNmX5yVPY0UG2631IVeDLrO9Ep0wbk13f4hcy8wCDBF8f3o7XQZrzw6Db/D+NZL1p4toCCkpxL5FRBWAXB0MIJ1oMbiMLLuM/2At3tTKZ7Lq7eF8= -127.0.0.1 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBOSjgoFyp8nQBRXL7Y/6nl2HyA59BHMrwMrlIrhBaQ/DTN8Z5SX0WNyTsCnYfatYMh7Vnsf5bF3lSfKnxvlD6dc= -` - func TestBuildAuthMethodsSuccess(t *testing.T) { got, err := buildAuthMethods(map[string]string{ "ssh_private_key": testSSHCert, @@ -48,34 +40,6 @@ func TestBuildAuthMethodsFailure(t *testing.T) { } } -func TestHostKeyCallbackFailureInvalidKnownHosts(t *testing.T) { - testDir := t.TempDir() - os.WriteFile(filepath.Join(testDir, "known_hosts"), []byte("invalid"), 0644) - got, err := hostKeyCallback(map[string]string{ - "known_hosts": filepath.Join(testDir, "known_hosts"), - }) - if err == nil { - t.Fatalf("expected error, got nil") - } - if got != nil { - t.Fatalf("got %v, want nil", got) - } -} - -func TestHostKeyCallbackSuccessValid(t *testing.T) { - testDir := t.TempDir() - os.WriteFile(filepath.Join(testDir, "known_hosts"), []byte(testKnownHosts), 0644) - got, err := hostKeyCallback(map[string]string{ - "known_hosts": filepath.Join(testDir, "known_hosts"), - }) - if err != nil { - t.Fatalf("failed to build host key callback: %v", err) - } - if got == nil { - t.Fatalf("got nil, want non-nil") - } -} - func TestConnectNativeSSHWithoutUsernameFailure(t *testing.T) { endpoint := &url.URL{ Scheme: "sftp", diff --git a/sftp/common/sftp.go b/sftp/common/sftp.go index b50795f7..229094cc 100644 --- a/sftp/common/sftp.go +++ b/sftp/common/sftp.go @@ -126,10 +126,6 @@ func ensureMaster(endpoint *url.URL, params map[string]string) (string, error) { // args = append(args, "-o", "UserKnownHostsFile=/dev/null") ? } - if kh := params["known_hosts"]; kh != "" { - args = append(args, "-o", "UserKnownHostsFile="+kh) - } - if id := params["identity"]; id != "" { args = append(args, "-i", id) } @@ -237,10 +233,6 @@ func connectOpenSSH(endpoint *url.URL, params map[string]string) (*sftp.Client, args = append(args, "-o", "StrictHostKeyChecking=no") } - if kh := params["known_hosts"]; kh != "" { - args = append(args, "-o", "UserKnownHostsFile="+kh) - } - if id := params["identity"]; id != "" { args = append(args, "-i", id) } diff --git a/sftp/go.mod b/sftp/go.mod index 11f8ef97..19302dfb 100644 --- a/sftp/go.mod +++ b/sftp/go.mod @@ -6,12 +6,12 @@ require ( github.com/PlakarKorp/go-kloset-sdk v1.1.0 github.com/PlakarKorp/kloset v1.1.0 github.com/pkg/sftp v1.13.9 + golang.org/x/crypto v0.52.0 golang.org/x/sync v0.20.0 ) require ( github.com/PlakarKorp/integration-grpc v1.1.0 // indirect - github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/dustin/go-humanize v1.0.1 // indirect github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect github.com/go-git/go-billy/v5 v5.9.0 // indirect @@ -25,14 +25,11 @@ require ( github.com/ncruces/go-strftime v1.0.0 // indirect github.com/nickball/go-aes-key-wrap v0.0.0-20170929221519-1c3aa3e4dfc5 // indirect github.com/pierrec/lz4/v4 v4.1.27 // indirect - github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect - github.com/stretchr/testify v1.11.1 // indirect github.com/tink-crypto/tink-go/v2 v2.6.0 // indirect github.com/vmihailenco/msgpack/v5 v5.4.1 // indirect github.com/vmihailenco/tagparser/v2 v2.0.0 // indirect github.com/zeebo/blake3 v0.2.4 // indirect - golang.org/x/crypto v0.52.0 // indirect golang.org/x/mod v0.36.0 // indirect golang.org/x/net v0.54.0 // indirect golang.org/x/sys v0.45.0 // indirect @@ -41,7 +38,6 @@ require ( google.golang.org/grpc v1.81.1 // indirect google.golang.org/protobuf v1.36.11 // indirect gopkg.in/warnings.v0 v0.1.2 // indirect - gopkg.in/yaml.v3 v3.0.1 // indirect modernc.org/libc v1.72.3 // indirect modernc.org/mathutil v1.7.1 // indirect modernc.org/memory v1.11.0 // indirect