From 45b99423a45f3dc25afa29b835d5b5033eaada94 Mon Sep 17 00:00:00 2001 From: Juliano Martinez Date: Mon, 20 Apr 2026 16:41:52 +0200 Subject: [PATCH 1/3] fix(sshConn): skip unusable IdentityFile entries instead of aborting Previously LoadIdentityFiles called ssh.ParsePrivateKey on every file listed as IdentityFile in ssh_config and returned the parse error to the caller on the first failure. That aborted the whole connection attempt with a 'ssh: no key found' error bubbling up from inside the SSH dial. The failure surfaced even when the SSH agent (e.g. yubikey-agent, Secretive, 1Password, gpg-agent) was perfectly capable of covering the requested identity. The most common trigger is a hardware-token setup where the IdentityFile path points at a public-key-only file on disk while the private half lives on the token and is accessed exclusively through the agent. OpenSSH tolerates this and falls back to the agent; pretty did not. LoadIdentityFiles now: - Silently skips IdentityFile entries whose files do not exist (errors.Is(err, os.ErrNotExist)). - Silently skips entries that contain only an authorized_keys-style public key (detected via ssh.ParseAuthorizedKey), so hardware-token identities are delegated to the SSH agent. - Silently skips passphrase-protected keys (ssh.PassphraseMissingError), since this process has no way to prompt for a passphrase. - Still returns a wrapped, path-qualified error for unreadable files and for truly malformed private-key content so real misconfigurations are not hidden. Tests: - Replace TestLoadIdentityFilesFailFast (which asserted the old behaviour) with TestLoadIdentityFilesSkipsMissing. - Add TestLoadIdentityFilesSkipsPublicKey for the hardware-token case. - Add TestLoadIdentityFilesMixedSkipsUnusable to verify that a usable private key alongside unusable entries still produces an auth method. - Add TestLoadIdentityFilesSkipsPassphraseProtected. - Add TestLoadIdentityFilesReturnsErrorForMalformedPrivateKey to pin the one case that must still surface an error. - Introduce a generateTestPublicKeyLine helper that synthesizes a fresh ed25519 authorized_keys line at runtime, so no static key material is checked into the tree. --- internal/sshConn/config.go | 36 ++++++++++- internal/sshConn/config_test.go | 107 ++++++++++++++++++++++++++++++-- 2 files changed, 137 insertions(+), 6 deletions(-) diff --git a/internal/sshConn/config.go b/internal/sshConn/config.go index f6fe50a..dd07041 100644 --- a/internal/sshConn/config.go +++ b/internal/sshConn/config.go @@ -1,6 +1,7 @@ package sshConn import ( + "errors" "fmt" "os" "os/user" @@ -242,19 +243,50 @@ func ParseProxyJump(value string) []string { 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..928675f 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" @@ -11,6 +12,7 @@ import ( "reflect" "testing" + "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" ) @@ -80,10 +82,13 @@ func TestResolveHostFallbackUser(t *testing.T) { } } -func TestLoadIdentityFilesFailFast(t *testing.T) { - _, err := LoadIdentityFiles([]string{"/does/not/exist"}) - if err == nil { - t.Fatalf("expected error") +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 +103,84 @@ 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 TestClientConfigForCombinedAuth(t *testing.T) { socketPath := startTestAgent(t) t.Setenv("SSH_AUTH_SOCK", socketPath) @@ -245,6 +328,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) From 663ab4ae7d0adece329270a8327c581e07e7aff2 Mon Sep 17 00:00:00 2001 From: Juliano Martinez Date: Mon, 20 Apr 2026 16:42:33 +0200 Subject: [PATCH 2/3] fix(sshConn): evaluate Match exec blocks and honour ProxyJump=none pretty resolves ssh_config entries for every target and every jump host, but the sshconfig.Context passed to the resolver left Context.Exec unset. The ssh_config library silently evaluates every 'Match host X exec "..."' block as non-matching when Exec is nil, so directives that pick a reachable jump host dynamically - common in corporate configs, e.g. Match host jump-alias exec "nc -zG 1 primary.example.net 22" HostName primary.example.net Match host jump-alias exec "nc -zG 1 secondary.example.net 22" HostName secondary.example.net never applied. Their HostName override was skipped, the alias stayed literal, and the connection failed at DNS resolution with 'no such host'. Fix: wire an Exec callback through the resolver. - Introduce shellMatchExec: runs the already-token-expanded command via /bin/sh -c (cmd.exe /C on Windows), with stdin/stdout/stderr detached and a 10s timeout cap so a misbehaving probe can't hang the CLI. - Expose it via a package-level matchExecFunc var so tests can stub exec evaluation deterministically without shelling out. This matches the existing stubbing pattern used for connectionFunc / sessionFunc. - Pass matchExecFunc as Context.Exec from SSHConfigResolver.resolve so every host and jump-host resolution benefits without any caller changes in cmd/. Follow-on correctness fix surfaced by the above: once Match exec starts actually firing, 'Match originalhost "jump-*" ProxyJump none' patterns produce ProxyJump="none". OpenSSH treats that as an explicit opt-out that cancels inherited ProxyJump rules, but pretty was parsing it as a literal jump host named "none" and trying to dial it. ResolveHost now drops 'none' before calling ParseProxyJump, and ParseProxyJump also collapses to an empty slice whenever any component equals "none" (case insensitive), guaranteeing no caller ever sees the sentinel. Tests: - TestResolveHostMatchExecApplies: first matching exec probe wins and its HostName is returned. - TestResolveHostMatchExecAllFailKeepsAlias: when every probe fails the alias is left unchanged. - TestShellMatchExecSucceedsForTrue / FailsForFalse / EmptyCmd: pin the default shell callback's semantics on POSIX. - TestParseProxyJumpNoneDisablesJumps: covers 'none', 'None', 'NONE', and surrounding whitespace. - TestResolveHostProxyJumpNoneClearsJumps: end-to-end sanity check that a specific-block ProxyJump=none beats a wildcard block and results in zero jumps. No changes required in ncode/ssh_config: it already parses Match exec and expands %-tokens correctly; it just needs the caller to provide a runtime Exec callback, which is this change. --- internal/sshConn/config.go | 76 ++++++++++++++++- internal/sshConn/config_test.go | 141 ++++++++++++++++++++++++++++++++ 2 files changed, 213 insertions(+), 4 deletions(-) diff --git a/internal/sshConn/config.go b/internal/sshConn/config.go index dd07041..2808395 100644 --- a/internal/sshConn/config.go +++ b/internal/sshConn/config.go @@ -1,18 +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 @@ -134,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) } @@ -200,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) } @@ -231,14 +291,22 @@ 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 } diff --git a/internal/sshConn/config_test.go b/internal/sshConn/config_test.go index 928675f..44eb5e1 100644 --- a/internal/sshConn/config_test.go +++ b/internal/sshConn/config_test.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "reflect" + "runtime" "testing" "golang.org/x/crypto/ssh" @@ -181,6 +182,112 @@ func TestLoadIdentityFilesReturnsErrorForMalformedPrivateKey(t *testing.T) { } } +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) @@ -206,6 +313,40 @@ 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 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) From f68dc89579d3bacf5e4ab7d8f57cadb7a41b6fa3 Mon Sep 17 00:00:00 2001 From: Juliano Martinez Date: Mon, 20 Apr 2026 17:02:28 +0200 Subject: [PATCH 3/3] test(sshConn): close coverage gaps in config resolver Add focused tests for previously-uncovered branches in config.go: - Error propagation from loadConfig for non-ENOENT paths (ENOTDIR via intermediate regular file) and from both user and system config loads in LoadSSHConfig. - Resolver error propagation through getValue and getAllValues when the underlying library rejects an empty HostArg. - ResolveHost paths for HostSpec.Alias precedence, invalid Port values surfacing as a wrapped error, and fallback to currentUser() when no explicit fallback is provided. - LoadIdentityFiles wrapping non-ErrNotExist read errors with the offending path. - ParseProxyJump dropping empty/whitespace-only components between commas. Statement coverage for internal/sshConn/config.go: 84.61% -> 93.75%. Remaining gaps are platform-specific (Windows shell branch in shellMatchExec), environment-dependent (currentUser env fallbacks when user.Current() fails), or structurally unreachable under normal control flow. --- internal/sshConn/config_test.go | 184 ++++++++++++++++++++++++++++++++ 1 file changed, 184 insertions(+) diff --git a/internal/sshConn/config_test.go b/internal/sshConn/config_test.go index 44eb5e1..1ad9d84 100644 --- a/internal/sshConn/config_test.go +++ b/internal/sshConn/config_test.go @@ -11,6 +11,7 @@ import ( "path/filepath" "reflect" "runtime" + "strings" "testing" "golang.org/x/crypto/ssh" @@ -83,6 +84,49 @@ func TestResolveHostFallbackUser(t *testing.T) { } } +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 { @@ -182,6 +226,20 @@ func TestLoadIdentityFilesReturnsErrorForMalformedPrivateKey(t *testing.T) { } } +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.: @@ -323,6 +381,16 @@ func TestParseProxyJumpNoneDisablesJumps(t *testing.T) { } } +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 @@ -654,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") @@ -665,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")