diff --git a/internal/sshConn/config.go b/internal/sshConn/config.go index f6fe50a..2808395 100644 --- a/internal/sshConn/config.go +++ b/internal/sshConn/config.go @@ -1,17 +1,64 @@ package sshConn import ( + "context" + "errors" "fmt" "os" + "os/exec" "os/user" "path/filepath" + "runtime" "strconv" "strings" + "time" sshconfig "github.com/ncode/ssh_config" "golang.org/x/crypto/ssh" ) +// matchExecTimeout bounds how long a `Match ... exec "..."` probe may run. +// OpenSSH itself does not impose a timeout, but pretty resolves many hosts in +// a row (including per-host ProxyJump resolution) so we cap each probe to +// avoid hanging the CLI if a user's exec command never returns. +const matchExecTimeout = 10 * time.Second + +// matchExecFunc evaluates `Match ... exec ""` directives. It is a package +// variable so tests can substitute a deterministic stub without shelling out. +// It must return (true, nil) if the command exits with status 0, (false, nil) +// otherwise, and only return a non-nil error for conditions the caller should +// surface (currently unused by the resolver because we do not opt into strict +// mode). +var matchExecFunc = shellMatchExec + +// shellMatchExec runs cmd via the local shell (sh -c / cmd /C on Windows) and +// reports whether it succeeded. The command has already had its %-tokens +// expanded by ssh_config before this function is invoked. +func shellMatchExec(cmd string) (bool, error) { + if strings.TrimSpace(cmd) == "" { + return false, nil + } + ctx, cancel := context.WithTimeout(context.Background(), matchExecTimeout) + defer cancel() + + var c *exec.Cmd + if runtime.GOOS == "windows" { + c = exec.CommandContext(ctx, "cmd", "/C", cmd) + } else { + c = exec.CommandContext(ctx, "/bin/sh", "-c", cmd) + } + c.Stdin = nil + c.Stdout = nil + c.Stderr = nil + + if err := c.Run(); err != nil { + // Any non-zero exit or failure to launch counts as "did not match", + // mirroring OpenSSH's treatment of Match exec probes. + return false, nil + } + return true, nil +} + type SSHConfigPaths struct { User string System string @@ -133,7 +180,10 @@ func (r *SSHConfigResolver) ResolveHost(spec HostSpec, fallbackUser string) (Res if err != nil { return ResolvedHost{}, err } - if proxyJump != "" { + // OpenSSH treats `ProxyJump none` as an explicit opt-out that cancels + // ProxyJump inherited from broader-matching blocks. Skip parsing in that + // case so we don't try to dial the literal host "none". + if proxyJump != "" && !strings.EqualFold(strings.TrimSpace(proxyJump), "none") { resolved.ProxyJump = ParseProxyJump(proxyJump) } @@ -199,7 +249,18 @@ func (r *SSHConfigResolver) resolve(cfg *sshconfig.Config, alias string) (*sshco if cfg == nil { return nil, nil } - ctx := sshconfig.Context{HostArg: alias, OriginalHost: alias, LocalUser: currentUser()} + ctx := sshconfig.Context{ + HostArg: alias, + OriginalHost: alias, + LocalUser: currentUser(), + // Providing Exec enables `Match host X exec "..."` blocks to be + // evaluated. Without it the library silently treats every exec + // predicate as non-matching, which causes directives like + // Match host jump-alias exec "nc -zG 1 primary.example.net 22" + // HostName primary.example.net + // to be skipped, leaving the alias unresolvable via DNS. + Exec: matchExecFunc, + } return cfg.Resolve(ctx) } @@ -230,31 +291,70 @@ func expandPath(path string) string { return path } +// ParseProxyJump splits a ProxyJump value into individual jump hosts. Empty +// components are dropped, and the OpenSSH "none" sentinel (which disables +// ProxyJump) collapses the result to an empty slice so callers never try to +// dial a literal host named "none". func ParseProxyJump(value string) []string { parts := strings.Split(value, ",") jumps := make([]string, 0, len(parts)) for _, part := range parts { trimmed := strings.TrimSpace(part) - if trimmed != "" { - jumps = append(jumps, trimmed) + if trimmed == "" { + continue + } + if strings.EqualFold(trimmed, "none") { + return nil } + jumps = append(jumps, trimmed) } return jumps } +// LoadIdentityFiles returns SSH auth methods for every identity file that +// contains a usable private key locally. +// +// IdentityFile entries that the local process cannot use directly (missing +// files, public-key-only files backing a hardware token such as yubikey-agent, +// or passphrase-protected keys) are skipped so that authentication can still +// proceed through the SSH agent. This mirrors OpenSSH's behaviour, which +// silently tolerates these cases instead of aborting the connection. func LoadIdentityFiles(paths []string) ([]ssh.AuthMethod, error) { methods := make([]ssh.AuthMethod, 0, len(paths)) for _, path := range paths { expanded := expandPath(path) key, err := os.ReadFile(expanded) if err != nil { - return nil, err + if errors.Is(err, os.ErrNotExist) { + continue + } + return nil, fmt.Errorf("unable to read identity file %q: %w", expanded, err) } signer, err := ssh.ParsePrivateKey(key) if err != nil { - return nil, err + if isAgentCoveredIdentity(key, err) { + continue + } + return nil, fmt.Errorf("unable to parse identity file %q: %w", expanded, err) } methods = append(methods, ssh.PublicKeys(signer)) } return methods, nil } + +// isAgentCoveredIdentity reports whether an identity file that we failed to +// parse as a private key should be delegated to the SSH agent. Typical cases: +// +// - The file stores only a public key (e.g. a hardware-token pub key in +// `~/.ssh/...`) while the private half lives on the token itself and is +// exposed exclusively through a signing agent. +// - The key is encrypted and no passphrase is available to this process. +func isAgentCoveredIdentity(data []byte, parseErr error) bool { + if _, ok := parseErr.(*ssh.PassphraseMissingError); ok { + return true + } + if _, _, _, _, err := ssh.ParseAuthorizedKey(data); err == nil { + return true + } + return false +} diff --git a/internal/sshConn/config_test.go b/internal/sshConn/config_test.go index d050d9d..1ad9d84 100644 --- a/internal/sshConn/config_test.go +++ b/internal/sshConn/config_test.go @@ -1,6 +1,7 @@ package sshConn import ( + "crypto/ed25519" "crypto/rand" "crypto/rsa" "crypto/x509" @@ -9,8 +10,11 @@ import ( "os" "path/filepath" "reflect" + "runtime" + "strings" "testing" + "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" ) @@ -80,10 +84,56 @@ func TestResolveHostFallbackUser(t *testing.T) { } } -func TestLoadIdentityFilesFailFast(t *testing.T) { - _, err := LoadIdentityFiles([]string{"/does/not/exist"}) - if err == nil { - t.Fatalf("expected error") +func TestResolveHostUsesCurrentUserWhenNoFallback(t *testing.T) { + // With no explicit user, no User directive, and an empty fallbackUser, + // ResolveHost must fall back to the OS current user. This exercises the + // `userValue = currentUser()` branch that is otherwise masked by the + // explicit fallback used in every other test in this file. + resolver, err := LoadSSHConfig(SSHConfigPaths{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + resolved, err := resolver.ResolveHost(HostSpec{Host: "host-no-user"}, "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resolved.User == "" { + t.Fatalf("expected a non-empty OS user when fallback is empty, got %+v", resolved) + } + if resolved.User != currentUser() { + t.Fatalf("expected OS current user %q, got %q", currentUser(), resolved.User) + } +} + +func TestResolveHostPrefersExplicitAliasOverHost(t *testing.T) { + // HostSpec.Alias takes precedence over Host for config lookup; this + // covers the `alias = spec.Alias` branch that the other tests never hit + // because they always set Host directly. + cfg := "Host canonical\n HostName canonical.internal\n User deploy\n" + userCfg := writeTempConfig(t, cfg) + resolver, err := LoadSSHConfig(SSHConfigPaths{User: userCfg}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + resolved, err := resolver.ResolveHost(HostSpec{Host: "ignored", Alias: "canonical"}, "me") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resolved.Alias != "canonical" { + t.Fatalf("expected alias to win over host, got %+v", resolved) + } + if resolved.Host != "canonical.internal" { + t.Fatalf("expected HostName to resolve via alias, got %+v", resolved) + } +} + +func TestLoadIdentityFilesSkipsMissing(t *testing.T) { + methods, err := LoadIdentityFiles([]string{"/does/not/exist"}) + if err != nil { + t.Fatalf("expected missing identity files to be skipped silently, got error: %v", err) + } + if len(methods) != 0 { + t.Fatalf("expected no auth methods for missing file, got %d", len(methods)) } } @@ -98,6 +148,204 @@ func TestLoadIdentityFilesSuccess(t *testing.T) { } } +func TestLoadIdentityFilesSkipsPublicKey(t *testing.T) { + // Public key only files (e.g. hardware-token identities served by an agent + // such as yubikey-agent) must not abort the connection attempt. We + // synthesize an authorized_keys line from a freshly generated ed25519 key + // so the test has no static key material embedded in the source tree. + pubKey := generateTestPublicKeyLine(t) + path := filepath.Join(t.TempDir(), "hardware_token.pub") + if err := os.WriteFile(path, []byte(pubKey), 0o600); err != nil { + t.Fatalf("failed to write pub key: %v", err) + } + + methods, err := LoadIdentityFiles([]string{path}) + if err != nil { + t.Fatalf("expected public key identity to be skipped silently, got error: %v", err) + } + if len(methods) != 0 { + t.Fatalf("expected no auth methods from public key, got %d", len(methods)) + } +} + +func TestLoadIdentityFilesMixedSkipsUnusable(t *testing.T) { + // A private key is returned, while a co-located public key is ignored so + // that authentication keeps working when the agent handles that identity. + privPath := writeTempKey(t) + pubPath := filepath.Join(t.TempDir(), "hardware.pub") + pubKey := generateTestPublicKeyLine(t) + if err := os.WriteFile(pubPath, []byte(pubKey), 0o600); err != nil { + t.Fatalf("failed to write pub key: %v", err) + } + + methods, err := LoadIdentityFiles([]string{pubPath, privPath, "/does/not/exist"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(methods) != 1 { + t.Fatalf("expected only the parseable private key to produce an auth method, got %d", len(methods)) + } +} + +func TestLoadIdentityFilesSkipsPassphraseProtected(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate key: %v", err) + } + der := x509.MarshalPKCS1PrivateKey(key) + encrypted, err := x509.EncryptPEMBlock(rand.Reader, "RSA PRIVATE KEY", der, []byte("secret"), x509.PEMCipherAES256) + if err != nil { + t.Fatalf("failed to encrypt key: %v", err) + } + data := pem.EncodeToMemory(encrypted) + path := filepath.Join(t.TempDir(), "id_rsa") + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatalf("failed to write key: %v", err) + } + + methods, err := LoadIdentityFiles([]string{path}) + if err != nil { + t.Fatalf("expected passphrase-protected identity to be skipped silently, got error: %v", err) + } + if len(methods) != 0 { + t.Fatalf("expected no auth methods from passphrase-protected key, got %d", len(methods)) + } +} + +func TestLoadIdentityFilesReturnsErrorForMalformedPrivateKey(t *testing.T) { + // Malformed PEM (starts with a PEM header but is not a valid key) should + // still bubble up an error so operators notice the misconfigured file. + path := filepath.Join(t.TempDir(), "id_rsa") + malformed := "-----BEGIN RSA PRIVATE KEY-----\nnot-base64-valid\n-----END RSA PRIVATE KEY-----\n" + if err := os.WriteFile(path, []byte(malformed), 0o600); err != nil { + t.Fatalf("failed to write malformed key: %v", err) + } + _, err := LoadIdentityFiles([]string{path}) + if err == nil { + t.Fatalf("expected error for malformed private key") + } +} + +func TestLoadIdentityFilesWrapsNonNotExistReadError(t *testing.T) { + // Passing a directory where a file is expected produces a read error + // that is not os.ErrNotExist, which must be surfaced with a wrapped, + // path-qualified message rather than swallowed. + dir := t.TempDir() + _, err := LoadIdentityFiles([]string{dir}) + if err == nil { + t.Fatalf("expected read error for directory path") + } + if !strings.Contains(err.Error(), dir) { + t.Fatalf("expected wrapped error to include the offending path, got %q", err.Error()) + } +} + +func TestResolveHostMatchExecApplies(t *testing.T) { + // Models the production pattern where a jump alias resolves to a concrete + // HostName only when the probe command succeeds, e.g.: + // Match host jump-alias exec "nc -zG 1 primary.example.net 22" + // HostName primary.example.net + cfg := `Match host jump-alias exec "probe-primary" + HostName primary.example.net +Match host jump-alias exec "probe-secondary" + HostName secondary.example.net +` + userCfg := writeTempConfig(t, cfg) + resolver, err := LoadSSHConfig(SSHConfigPaths{User: userCfg}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + var seen []string + stubExec(t, func(cmd string) (bool, error) { + seen = append(seen, cmd) + return cmd == "probe-primary", nil + }) + + resolved, err := resolver.ResolveHost(HostSpec{Host: "jump-alias"}, "me") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resolved.Host != "primary.example.net" { + t.Fatalf("expected primary HostName from matching exec block, got %q", resolved.Host) + } + if len(seen) == 0 { + t.Fatalf("expected exec callback to be invoked, got no calls") + } +} + +func TestResolveHostMatchExecAllFailKeepsAlias(t *testing.T) { + cfg := `Match host jump-alias exec "probe-primary" + HostName primary.example.net +Match host jump-alias exec "probe-secondary" + HostName secondary.example.net +` + userCfg := writeTempConfig(t, cfg) + resolver, err := LoadSSHConfig(SSHConfigPaths{User: userCfg}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + stubExec(t, func(cmd string) (bool, error) { return false, nil }) + + resolved, err := resolver.ResolveHost(HostSpec{Host: "jump-alias"}, "me") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // With every exec probe failing, no HostName override applies and the + // alias stays as-is - this is the same state that previously surfaced as + // a DNS "no such host" when wiring was missing entirely. + if resolved.Host != "jump-alias" { + t.Fatalf("expected alias to remain when all exec probes fail, got %q", resolved.Host) + } +} + +func TestShellMatchExecSucceedsForTrue(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX shell not available on Windows CI images") + } + ok, err := shellMatchExec("true") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !ok { + t.Fatal("expected true command to succeed") + } +} + +func TestShellMatchExecFailsForFalse(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX shell not available on Windows CI images") + } + ok, err := shellMatchExec("false") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ok { + t.Fatal("expected false command to report non-match") + } +} + +func TestShellMatchExecEmptyCmd(t *testing.T) { + ok, err := shellMatchExec(" ") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ok { + t.Fatal("expected empty command to report non-match") + } +} + +// stubExec replaces the package-level matchExecFunc for the duration of a test +// and restores it on cleanup. This keeps Match exec evaluation deterministic +// without shelling out. +func stubExec(t *testing.T, fn func(cmd string) (bool, error)) { + t.Helper() + orig := matchExecFunc + matchExecFunc = fn + t.Cleanup(func() { matchExecFunc = orig }) +} + func TestClientConfigForCombinedAuth(t *testing.T) { socketPath := startTestAgent(t) t.Setenv("SSH_AUTH_SOCK", socketPath) @@ -123,6 +371,50 @@ func TestParseProxyJump(t *testing.T) { } } +func TestParseProxyJumpNoneDisablesJumps(t *testing.T) { + // OpenSSH: `ProxyJump none` cancels any inherited ProxyJump; it must not + // be treated as a literal hop. + for _, in := range []string{"none", "None", "NONE", " none "} { + if got := ParseProxyJump(in); len(got) != 0 { + t.Fatalf("ParseProxyJump(%q) = %v, want empty", in, got) + } + } +} + +func TestParseProxyJumpSkipsEmptyComponents(t *testing.T) { + // Empty/whitespace-only segments between commas are dropped rather than + // surfacing as blank jump entries to the dialer. + got := ParseProxyJump(" ,jump1, ,jump2,") + want := []string{"jump1", "jump2"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("ParseProxyJump skipped-empty: got %v, want %v", got, want) + } +} + +func TestResolveHostProxyJumpNoneClearsJumps(t *testing.T) { + // OpenSSH: first matching directive wins for single-valued options, so + // the specific block declaring `ProxyJump none` must come before the + // wildcard block that sets a jump. The resolved value must collapse to + // no jumps rather than the literal host "none". + cfg := `Host solo.internal + ProxyJump none +Host *.internal + ProxyJump jump1 +` + userCfg := writeTempConfig(t, cfg) + resolver, err := LoadSSHConfig(SSHConfigPaths{User: userCfg}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + resolved, err := resolver.ResolveHost(HostSpec{Host: "solo.internal"}, "me") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(resolved.ProxyJump) != 0 { + t.Fatalf("expected no proxy jumps when ProxyJump=none wins, got %v", resolved.ProxyJump) + } +} + func TestResolveProxyJumpSingle(t *testing.T) { cfg := "Host target\n ProxyJump jump1\n" userCfg := writeTempConfig(t, cfg) @@ -245,6 +537,22 @@ func startTestAgent(t *testing.T) string { return socketPath } +// generateTestPublicKeyLine produces a freshly generated authorized_keys-style +// line (type + base64 blob + comment) so tests do not ship static key material +// in-tree. The key is ephemeral and has no corresponding private half on disk. +func generateTestPublicKeyLine(t *testing.T) string { + t.Helper() + pub, _, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("failed to generate ed25519 key: %v", err) + } + sshPub, err := ssh.NewPublicKey(pub) + if err != nil { + t.Fatalf("failed to wrap public key: %v", err) + } + return string(ssh.MarshalAuthorizedKey(sshPub)) +} + func writeTempKey(t *testing.T) string { t.Helper() key, err := rsa.GenerateKey(rand.Reader, 2048) @@ -414,6 +722,45 @@ func TestLoadConfigNonExistentPath(t *testing.T) { } } +func TestLoadConfigSurfacesNonNotExistOpenError(t *testing.T) { + // Using a regular file as an intermediate path component produces an + // ENOTDIR error from os.Open, which is not os.IsNotExist. loadConfig + // must surface that error instead of silently returning nil. + base := filepath.Join(t.TempDir(), "not_a_dir") + if err := os.WriteFile(base, []byte("placeholder"), 0o600); err != nil { + t.Fatalf("failed to seed placeholder file: %v", err) + } + cfg, err := loadConfig(filepath.Join(base, "config")) + if err == nil { + t.Fatalf("expected non-nil error for ENOTDIR path") + } + if cfg != nil { + t.Fatalf("expected nil config when open errors, got %+v", cfg) + } +} + +func TestLoadSSHConfigPropagatesUserLoadError(t *testing.T) { + base := filepath.Join(t.TempDir(), "user_placeholder") + if err := os.WriteFile(base, []byte("x"), 0o600); err != nil { + t.Fatalf("failed to seed placeholder: %v", err) + } + _, err := LoadSSHConfig(SSHConfigPaths{User: filepath.Join(base, "config")}) + if err == nil { + t.Fatalf("expected user loadConfig error to propagate") + } +} + +func TestLoadSSHConfigPropagatesSystemLoadError(t *testing.T) { + base := filepath.Join(t.TempDir(), "sys_placeholder") + if err := os.WriteFile(base, []byte("x"), 0o600); err != nil { + t.Fatalf("failed to seed placeholder: %v", err) + } + _, err := LoadSSHConfig(SSHConfigPaths{System: filepath.Join(base, "config")}) + if err == nil { + t.Fatalf("expected system loadConfig error to propagate") + } +} + func TestResolveNilConfig(t *testing.T) { resolver := &SSHConfigResolver{} result, err := resolver.resolve(nil, "web") @@ -425,6 +772,83 @@ func TestResolveNilConfig(t *testing.T) { } } +func TestResolveHostInvalidPortSurfacesError(t *testing.T) { + cfg := "Host web\n Port not-a-number\n" + userCfg := writeTempConfig(t, cfg) + resolver, err := LoadSSHConfig(SSHConfigPaths{User: userCfg}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + _, err = resolver.ResolveHost(HostSpec{Host: "web"}, "me") + if err == nil { + t.Fatalf("expected invalid Port value to produce a wrapped error") + } + if !strings.Contains(err.Error(), "invalid port") { + t.Fatalf("expected wrapped error to mention invalid port, got %q", err.Error()) + } +} + +func TestResolveHostEmptyAliasSurfacesResolverError(t *testing.T) { + // The underlying ssh_config library requires a non-empty HostArg and + // returns an error otherwise. ResolveHost must surface that rather than + // continuing to connect to a nameless host. + userCfg := writeTempConfig(t, "Host web\n User deploy\n") + resolver, err := LoadSSHConfig(SSHConfigPaths{User: userCfg}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + _, err = resolver.ResolveHost(HostSpec{}, "") + if err == nil { + t.Fatalf("expected error for empty host/alias") + } +} + +func TestGetValueSurfacesUserResolverError(t *testing.T) { + userCfg := writeTempConfig(t, "Host web\n User deploy\n") + resolver, err := LoadSSHConfig(SSHConfigPaths{User: userCfg}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, err := resolver.getValue("", "User"); err == nil { + t.Fatalf("expected resolver error for empty alias via user config") + } +} + +func TestGetValueSurfacesSystemResolverError(t *testing.T) { + // Only a system config is loaded so the error must come from the system + // branch of getValue rather than the user branch. + systemCfg := writeTempConfig(t, "Host web\n User deploy\n") + resolver, err := LoadSSHConfig(SSHConfigPaths{System: systemCfg}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, err := resolver.getValue("", "User"); err == nil { + t.Fatalf("expected resolver error for empty alias via system config") + } +} + +func TestGetAllValuesSurfacesUserResolverError(t *testing.T) { + userCfg := writeTempConfig(t, "Host web\n IdentityFile ~/.ssh/id\n") + resolver, err := LoadSSHConfig(SSHConfigPaths{User: userCfg}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, err := resolver.getAllValues("", "IdentityFile"); err == nil { + t.Fatalf("expected resolver error for empty alias via user config") + } +} + +func TestGetAllValuesSurfacesSystemResolverError(t *testing.T) { + systemCfg := writeTempConfig(t, "Host web\n IdentityFile ~/.ssh/id\n") + resolver, err := LoadSSHConfig(SSHConfigPaths{System: systemCfg}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, err := resolver.getAllValues("", "IdentityFile"); err == nil { + t.Fatalf("expected resolver error for empty alias via system config") + } +} + func TestGetValueWithNilConfigs(t *testing.T) { resolver := &SSHConfigResolver{} val, err := resolver.getValue("web", "HostName")