feat: implement SSH agent for Windows with tests #2644
Conversation
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR adds platform-specific SSH identity agent handling to support both Unix and Windows environments. It introduces abstracted Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/remote/connparse/connparse.go (2)
100-110: Redundant assignment ofrestwhen URI starts with//.When
uristarts with"//", line 101 setsrest = strings.TrimPrefix(uri, "//"). Then later at lines 141-142, whenscheme == ""andstrings.HasPrefix(uri, "//")is true,restis assigned again with the identical value. This is redundant.Also, line 106's
TrimPrefix(split[1], "//")seems unusual—after splitting on"://", the remainder shouldn't typically start with"//". Could you clarify the use case this handles?if strings.HasPrefix(uri, "//") { rest = strings.TrimPrefix(uri, "//") } else { split := strings.SplitN(uri, "://", 2) if len(split) > 1 { scheme = split[0] - rest = strings.TrimPrefix(split[1], "//") + rest = split[1] } else { rest = split[0] } }
162-166: Consider extracting the complex path condition into a helper for readability.The condition on line 164 is difficult to parse due to its length. Extracting this to a named helper function would improve maintainability.
+// needsPrecedingSlash returns true if the path needs a "/" prefix +func needsPrecedingSlash(path string) bool { + if len(path) <= 1 { + return false + } + if windowsDriveRegex.MatchString(path) { + return false + } + prefixes := []string{"/", "~", "./", "../", ".\\", "..\\"} + for _, p := range prefixes { + if strings.HasPrefix(path, p) { + return false + } + } + return path != ".." +} if scheme == ConnectionTypeWsh { if host == "" { host = wshrpc.LocalConnName } if strings.HasPrefix(remotePath, "/~") { remotePath = strings.TrimPrefix(remotePath, "/") - } else if addPrecedingSlash && (len(remotePath) > 1 && !windowsDriveRegex.MatchString(remotePath) && !strings.HasPrefix(remotePath, "/") && !strings.HasPrefix(remotePath, "~") && !strings.HasPrefix(remotePath, "./") && !strings.HasPrefix(remotePath, "../") && !strings.HasPrefix(remotePath, ".\\") && !strings.HasPrefix(remotePath, "..\\") && remotePath != "..") { + } else if addPrecedingSlash && needsPrecedingSlash(remotePath) { remotePath = "/" + remotePath } }pkg/remote/sshagent_windows_test.go (1)
10-19: LGTM! Consider adding error type verification for robustness.The timeout test is well-structured. For more precise validation, you could additionally check that the error indicates a connection failure (e.g.,
os.IsTimeoutor checking for specific error messages) rather than just any error.func TestDialIdentityAgentWindowsTimeout(t *testing.T) { start := time.Now() _, err := dialIdentityAgent(`\\.\\pipe\\waveterm-nonexistent-agent`) if err == nil { t.Skip("unexpectedly connected to a test pipe; skipping") } + // Optionally verify error indicates connection/timeout failure + t.Logf("dialIdentityAgent returned expected error: %v", err) if time.Since(start) > 3*time.Second { t.Fatalf("dialIdentityAgent exceeded expected timeout window") } }pkg/remote/sshclient.go (1)
905-926: Good platform-aware SSH agent resolution.The priority order (env var → Windows default → shell fallback) is sensible. The Windows pipe path
\\.\pipe\openssh-ssh-agentis the standard OpenSSH agent location.Minor edge case: If the shell command succeeds but
SSH_AUTH_SOCKis unset/empty in the shell environment,ExpandHomeDir("")returns".", causing the agent path to be set to the current directory. This would fail gracefully at dial time, but you could add an emptiness check to avoid the unnecessary dial attempt:} else { shellPath := shellutil.DetectLocalShellPath() authSockCommand := exec.Command(shellPath, "-c", "echo ${SSH_AUTH_SOCK}") sshAuthSock, err := authSockCommand.Output() - if err == nil { - agentPath, err := wavebase.ExpandHomeDir(trimquotes.TryTrimQuotes(strings.TrimSpace(string(sshAuthSock)))) - if err != nil { - return nil, err + if err == nil { + trimmedSock := strings.TrimSpace(string(sshAuthSock)) + if trimmedSock != "" { + agentPath, err := wavebase.ExpandHomeDir(trimquotes.TryTrimQuotes(trimmedSock)) + if err != nil { + return nil, err + } + sshKeywords.SshIdentityAgent = utilfn.Ptr(agentPath) + } else { + log.Printf("SSH_AUTH_SOCK is empty in shell environment\n") } - sshKeywords.SshIdentityAgent = utilfn.Ptr(agentPath) } else { log.Printf("unable to find SSH_AUTH_SOCK: %v\n", err) } }pkg/remote/sshagent_windows.go (1)
12-16: Clean Windows named pipe implementation using go-winio.The
dialIdentityAgentfunction correctly handles Windows OpenSSH agent connections via named pipes. The 2-second timeout is reasonable for local agent connections, and the function signature matches the Unix counterpart for cross-platform compatibility.Ensure the
github.com/Microsoft/go-winiodependency is properly declared ingo.mod.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
docs/docs/connections.mdx(1 hunks)go.mod(1 hunks)pkg/remote/connparse/connparse.go(2 hunks)pkg/remote/sshagent_unix.go(1 hunks)pkg/remote/sshagent_unix_test.go(1 hunks)pkg/remote/sshagent_windows.go(1 hunks)pkg/remote/sshagent_windows_test.go(1 hunks)pkg/remote/sshclient.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-20T00:10:31.048Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1998
File: go.mod:37-37
Timestamp: 2025-02-20T00:10:31.048Z
Learning: The golang.org/x/sync package should remain at version v0.11.0 in go.mod, as explicitly confirmed by the maintainer.
Applied to files:
go.mod
🧬 Code graph analysis (1)
pkg/remote/sshclient.go (4)
pkg/util/utilfn/utilfn.go (2)
SafeDeref(941-947)Ptr(955-957)pkg/wavebase/wavebase.go (1)
ExpandHomeDir(149-163)pkg/trimquotes/trimquotes.go (1)
TryTrimQuotes(21-24)pkg/util/shellutil/shellutil.go (1)
DetectLocalShellPath(83-101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (go)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (6)
docs/docs/connections.mdx (1)
170-177: LGTM! Clear documentation for the new SSH agent detection feature.The resolution order is well-documented and provides actionable guidance for Windows users regarding the OpenSSH Authentication Agent service.
pkg/remote/connparse/connparse.go (1)
115-134: LGTM!The
parseGenericPathandparseWshPathhelpers are well-structured. The WSL regex handling correctly extracts the distribution name while preserving the path.pkg/remote/sshagent_unix.go (1)
7-10: Consider adding a timeout for consistency with Windows implementation.The Unix dialer has no timeout, while the Windows implementation reportedly uses a 2-second timeout. Unix domain socket connections are typically fast, but adding a timeout could prevent hangs if the socket exists but the agent is unresponsive. This should be verified against the actual Windows implementation to ensure consistency across platforms.
go.mod (1)
6-6: go-winio v0.6.2 is the latest stable version.The dependency is current and well-chosen for Windows named pipe support.
pkg/remote/sshagent_unix_test.go (1)
11-35: LGTM!The test correctly validates the Unix socket dialing path. Good use of
t.TempDir()for automatic cleanup, proper goroutine synchronization with the channel, and appropriatedeferfor the listener.pkg/remote/sshclient.go (1)
616-625: LGTM!The refactored agent dialing logic correctly uses the platform-specific
dialIdentityAgentfunction. Good improvement to include the agent path in the error log for debugging. The graceful fallback (logging error but continuing) allows authentication to proceed with other methods if the agent is unavailable.
There was a problem hiding this comment.
Pull request overview
This PR implements cross-platform SSH agent support for Windows by adding named pipe connectivity alongside existing Unix domain socket support. The implementation introduces platform-specific dialer functions and prioritizes SSH_AUTH_SOCK environment variable detection across all platforms.
Key changes:
- Platform-aware SSH agent dialing: Windows uses named pipes via
go-winio, Unix systems use domain sockets - Unified agent path resolution: prioritizes
SSH_AUTH_SOCK, falls back to Windows OpenSSH pipe or Unix shell detection - Cross-platform unit tests covering dialer functionality and timeout behavior
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/remote/sshclient.go | Refactors agent path detection to check SSH_AUTH_SOCK first, adds Windows-specific fallback to OpenSSH named pipe, improves error logging with agent path context |
| pkg/remote/sshagent_windows.go | Implements Windows SSH agent connection via named pipes with 2-second timeout using go-winio library |
| pkg/remote/sshagent_windows_test.go | Tests Windows pipe dialing with timeout verification for non-existent agent |
| pkg/remote/sshagent_unix.go | Implements Unix SSH agent connection via domain sockets |
| pkg/remote/sshagent_unix_test.go | Tests Unix socket dialing with mock listener to verify successful connection |
| pkg/remote/connparse/connparse.go | Refactors URI parsing to handle "//" prefix before scheme splitting, renames variable from split to parts for clarity |
| go.mod | Adds github.com/Microsoft/go-winio v0.6.2 dependency for Windows named pipe support |
| go.sum | Adds checksums for go-winio dependency |
| docs/docs/connections.mdx | Documents SSH agent detection priority and platform-specific behavior for Windows, macOS, and Linux |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @andya1lan. * #2644 (comment) The following files were modified: * `pkg/remote/connparse/connparse.go` * `pkg/remote/sshagent_windows.go` * `pkg/remote/sshclient.go`
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/remote/sshagent_windows_test.go (1)
10-21: Well-structured timeout test.The test clearly validates that dialing a non-existent Windows named pipe completes within the expected timeout window. The 3-second threshold provides reasonable buffer over the 2-second implementation timeout.
Optionally, consider verifying the error type or message to ensure it's a connection/timeout error rather than an unexpected error type:
func TestDialIdentityAgentWindowsTimeout(t *testing.T) { start := time.Now() _, err := dialIdentityAgent(`\\.\\pipe\\waveterm-nonexistent-agent`) if err == nil { t.Skip("unexpectedly connected to a test pipe; skipping") } - // Optionally verify error indicates connection/timeout failure t.Logf("dialIdentityAgent returned expected error: %v", err) + // Verify it's a connection-related error (not a panic or programming error) + if err.Error() == "" { + t.Fatalf("error has empty message: %v", err) + } if time.Since(start) > 3*time.Second { t.Fatalf("dialIdentityAgent exceeded expected timeout window") } }pkg/remote/connparse/connparse.go (1)
28-42: Clarify and possibly extendneedsPrecedingSlashfor Windows UNC-style pathsThe helper looks good and nicely centralizes the old conditional logic, including Windows drive letters and relative/tilde forms. One possible gap is UNC-style Windows paths like
\\server\share: forwsh://host/\\server\sharethis helper would currently returntrueand cause/\\server\share, which is probably not desired if UNC paths are meant to be supported as-is.If UNC-style paths should be treated as already “absolute”, consider adding a
"\\"prefix to the disallowed list:- disallowedPrefixes := []string{"/", "~", "./", "../", ".\\", "..\\"} + disallowedPrefixes := []string{"/", "~", "./", "../", ".\\", "..\\", "\\\\"}If UNC paths are out of scope for WSH URIs, it might still be worth documenting that assumption in a comment to avoid future regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/remote/connparse/connparse.go(4 hunks)pkg/remote/sshagent_windows_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T05:34:23.892Z
Learnt from: L1l1thLY
Repo: wavetermdev/waveterm PR: 2249
File: pkg/waveai/anthropicbackend.go:60-62
Timestamp: 2025-08-13T05:34:23.892Z
Learning: Go's path.Join function does NOT treat arguments with leading slashes as absolute paths that discard previous arguments. path.Join(currentPath, "/v1/messages") correctly preserves currentPath and appends the segments, e.g., path.Join("/api", "/v1/messages") returns "/api/v1/messages".
Applied to files:
pkg/remote/connparse/connparse.go
🔇 Additional comments (2)
pkg/remote/connparse/connparse.go (2)
116-126: New leading-//and WSH/WSL parsing paths look consistent; consider a couple of edge-case testsThe refactor to:
- treat leading
//as remote shorthand (defaultingschemetowsh),- route
//wsl://distro/pathandwsh://wsl://distro/paththroughparseWshPath, and- use
parseGenericPatheverywhere else,is coherent and keeps the control flow readable.
A few edge cases would be good to explicitly lock in via tests to avoid surprises:
- URIs with extra slashes, e.g.
///host/pathand//wsl://distro(no trailing/path).- Plain
wsl://distro/path(nowsh://and no leading//), which now becomesScheme: "wsl", Host: "distro", Path: "path"viaparseGenericPath. Confirm this is intended and doesn’t need WSH-specific handling.- Bare
//or//hostwith and without a trailing slash: confirm the desired host/path split and resultingConnectionvalues.The implementation itself looks correct; this is mostly about ensuring the new behaviours are captured in tests and match the desired UX.
Also applies to: 131-150, 152-167
173-181: Interaction betweenaddPrecedingSlashandneedsPrecedingSlashfor WSH URIsThe final WSH-specific normalization:
- Defaults empty
hosttowshrpc.LocalConnName.- Normalizes
/~to~.- Uses
addPrecedingSlash && needsPrecedingSlash(remotePath)to decide whether to prefix/.This cleanly separates:
- Explicit
wsh://...URIs (whereaddPrecedingSlashstaystrue) from- Shorthand / no-scheme URIs (where
addPrecedingSlashis forcedfalseto preserve user-supplied paths like~/foo,./foo, Windows drives, etc.).Given
needsPrecedingSlashalready guards empty/one-char paths and all the “special” prefixes, this looks safe and resolves the previous ad-hoc condition nicely.I’d just recommend adding a couple of focused tests around:
wsh://host/foo→/foowsh://host/C:/path→C:/path(no leading/)foo/~/foo/C:\foo(no scheme) remaining unmodified inPath.
|
@andya1lan thanks for submitting this! will take a look and try to get it merged |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/remote/sshclient.go (1)
20-20: IdentityAgent/SSH_AUTH_SOCK handling is solid; consider a couple of edge‑case tweaksThe new logic generally looks good:
- Using
runtime.GOOSto default to\\.\pipe\openssh-ssh-agenton Windows is a sensible choice when no IdentityAgent is configured.- On non-Windows, resolving
SSH_AUTH_SOCKvia the detected shell helps in GUI/launchd scenarios where the app’s own environment may not carry the agent variables, addressing the earlier concern about an ineffective fallback.A few refinements you might consider:
Shell command for SSH_AUTH_SOCK
Using
echo ${SSH_AUTH_SOCK}assumes POSIX-style$var/${var}expansion. For users whose$SHELLisfish, this may fail syntactically. A slightly more portable command would be:authSockCommand := exec.Command(shellPath, "-c", "echo \"$SSH_AUTH_SOCK\"")This keeps POSIX shells happy and is also valid in fish, while still trimming whitespace afterward.
Special IdentityAgent values from ssh_config
OpenSSH’s
IdentityAgentsupports special values like:
none→ disable use of any agent"SSH_AUTH_SOCK"→ read the socket location from theSSH_AUTH_SOCKenvironment variable- values beginning with
$→ treat as an environment variable holding the socket path (manpages.debian.org)Today, those values would be treated as literal paths and passed through
ExpandHomeDir, which is slightly surprising compared to OpenSSH. In a future cleanup, you might:
- Map an expanded value of
noneto “no agent” by leavingSshIdentityAgentnil/empty.- When the raw value is exactly
SSH_AUTH_SOCK, resolve fromos.Getenv("SSH_AUTH_SOCK")or the same shell-based lookup you already use.- When the raw value starts with
$, look up the corresponding environment variable instead of treating it as a filename.This would bring your
IdentityAgenthandling in line withssh_config(5)semantics without changing the new Windows/Unix defaults.Also applies to: 905-924
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/docs/connections.mdx(1 hunks)pkg/remote/sshagent_windows.go(1 hunks)pkg/remote/sshclient.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/remote/sshagent_windows.go
🔇 Additional comments (3)
docs/docs/connections.mdx (1)
170-177: SSH Agent detection docs align with implementationThe resolution order and platform-specific details here match the behavior in
findSshConfigKeywords/createClientConfig(IdentityAgent first, then Windows named pipe or SSH_AUTH_SOCK on Unix). This is a clear, accurate description of the new logic.pkg/remote/sshclient.go (2)
237-242: Password callback logging is clear and safeThe extra
[conndebug]log when a password comes from the secret store (without logging the secret itself) is helpful and low-risk, and the interactive path is unchanged.
616-621: NewdialIdentityAgentusage and IdentitiesOnly guard look correctSwitching to
dialIdentityAgent(agentPath)and gating it on!SshIdentitiesOnly && agentPath != ""correctly centralizes platform-specific dialing and respectsIdentitiesOnlysemantics. The existing behavior of falling back to key files when dialing fails is preserved.
|
@andya1lan i scoped down this change to ONLY affect windows (and not add new SSH_AUTH_SOCK environment variable logic for macos/linux). I also removed the connparse.ParseURI() changes. I didn't do that because those changes were necessarily wrong, only that I wanted a really clean PR to merge your good windows changes -- windows dialing the named pipe, and the fallback to \.\pipe\openssh-ssh-agent -- and not have to worry about verifying the other changes right now. I'm trying to pull together a big patch release for windows (I put in a lot of windows changes myself over the past week), and by making this smaller I felt like I could get it merged for THIS release instead of having to test more and delay it to the next release. If you'd still like to get the SSH_AUTH_SOCK env change in (or the connparse changes), feel free to resubmit a PR for those changes only and I can review it for the next release. |
3 participants
#2643
Implementation
SSH_AUTH_SOCK, otherwise fall back to the default OpenSSH agent pipe//./pipe/openssh-ssh-agent.