Skip to content

feat: improve proxy SSH client management and configuration #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+39 −0
Open

feat: improve proxy SSH client management and configuration #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .claude/settings.local.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"permissions": {
"allow": [
"mcp__github__get_issue"
],
"deny": [],
"ask": []
}
}
6 changes: 6 additions & 0 deletions easyssh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,18 +292,24 @@ func (ssh_conf *MakeConfig) Connect() (*ssh.Session, *ssh.Client, error) {
conn = result.conn
err = result.err
case <-ctx.Done():
proxyClient.Close()
return nil, nil, fmt.Errorf("%w: %v", ErrProxyDialTimeout, ctx.Err())
}

if err != nil {
proxyClient.Close()
return nil, nil, err
}

ncc, chans, reqs, err := ssh.NewClientConn(conn, net.JoinHostPort(ssh_conf.Server, ssh_conf.Port), targetConfig)
if err != nil {
proxyClient.Close()
return nil, nil, err
Comment on lines +295 to 307
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While adding proxyClient.Close() on these error paths is correct to prevent resource leaks, the repetition of this call in multiple places could be simplified. You could consider using a defer with a flag to control when the client is closed. This would make the code cleaner and less prone to errors if more exit paths are added in the future.

For example:

		proxyClient, err := ssh.Dial(...)
		if err != nil {
			return nil, nil, err
		}
		closeProxy := true
		defer func() {
			if closeProxy {
				proxyClient.Close()
			}
		}()

		// ... existing logic ...

		// On successful connection before returning:
		client = ssh.NewClient(ncc, chans, reqs)
		closeProxy = false // Prevent deferred close on success path

This way, you only need to manage the closeProxy flag, and the defer statement handles the cleanup on all error returns automatically.

}

// Close the proxy client after successfully establishing the target connection
// The target connection (ncc) is now independent of the proxy client
proxyClient.Close()
Comment on lines +310 to +312
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Closing the proxyClient here is incorrect and will lead to a bug. The connection to the target server (client) is tunneled through the proxyClient. If you close the proxyClient, the underlying connection for client will also be closed, making it unusable immediately after creation.

The comment on lines 310-311 is misleading; the target connection ncc is dependent on the proxyClient remaining open.

While this change correctly closes the proxyClient in error scenarios to prevent resource leaks, closing it on a successful connection path introduces a problem. The proxyClient should only be closed when the main client is closed.

To fix the resource leak on the success path, you would need a way to close the proxyClient when the returned client is closed. This typically requires a more significant refactoring, for example by changing the Connect method's signature to also return the proxyClient so the caller can manage its lifecycle, or by returning a wrapper around the ssh.Client that closes both clients. For now, these lines should be removed to fix the immediate bug, though the resource leak on the success path will remain and needs to be addressed separately.

client = ssh.NewClient(ncc, chans, reqs)
Comment on lines +310 to 313
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Closing proxyClient breaks the target connection.

Closing proxyClient immediately after establishing the target connection will break that connection. Here's why:

  1. proxyClient.Dial() creates a channel through the proxy to the target
  2. ssh.NewClientConn() wraps that channel in ncc
  3. The channel remains dependent on proxyClient's underlying transport
  4. Closing proxyClient closes its transport connection, which closes all channels including the one used by ncc

This is likely causing the pipeline failures in TestProxyClient (lines 348-350) and TestProxyClientSSHCommand (lines 368-371) which report "ssh: unexpected packet in response to channel open: ".

The proxy client must remain alive for the lifetime of the target connection. Consider these alternatives:

Solution 1: Store proxyClient in the returned *ssh.Client and close it when the client is closed (requires wrapping or extending the client).

Solution 2: Return proxyClient as an additional return value so callers can manage its lifecycle:

-func (ssh_conf *MakeConfig) Connect() (*ssh.Session, *ssh.Client, error) {
+func (ssh_conf *MakeConfig) Connect() (*ssh.Session, *ssh.Client, io.Closer, error) {

Then callers close it after closing the client. This would be a breaking API change requiring updates to all call sites.

Solution 3: Keep the current API but document that the proxy connection lifecycle is managed internally and sessions/clients must be explicitly closed by callers (remove lines 310-312).

Given the breaking change implications, Solution 3 (removing lines 310-312 entirely) may be most pragmatic for now, with Solution 1 or 2 considered for a future major version.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In easyssh.go around lines 310 to 313, closing proxyClient immediately after
creating the target ssh client breaks the underlying channel (proxyClient's
transport closes and kills ncc); remove the proxyClient.Close() call so the
proxy client remains alive for the lifetime of the returned *ssh.Client, or
alternatively implement a wrapper that stores proxyClient and closes it when the
returned client is closed (for now remove the premature close as the pragmatic
fix).

} else {
client, err = ssh.Dial(string(ssh_conf.Protocol), net.JoinHostPort(ssh_conf.Server, ssh_conf.Port), targetConfig)
Expand Down
24 changes: 24 additions & 0 deletions easyssh_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,3 +666,27 @@ func TestProxyGoroutineLeak(t *testing.T) {
"Goroutine leak detected: initial=%d, final=%d", initialGoroutines, finalGoroutines)
}

// TestProxyClientCleanup tests that proxy clients are properly closed during multiple Connect calls
func TestProxyClientCleanup(t *testing.T) {
ssh := &MakeConfig{
Server: "10.255.255.1", // Non-routable IP
User: "testuser",
Port: "22",
KeyPath: "./tests/.ssh/id_rsa",
Timeout: 500 * time.Millisecond, // Short timeout
Proxy: DefaultConfig{
User: "testuser",
Server: "10.255.255.2", // Another non-routable IP for proxy
Port: "22",
KeyPath: "./tests/.ssh/id_rsa",
Timeout: 500 * time.Millisecond,
},
}

// Test multiple connect attempts - they should all fail gracefully without leaking resources
for i := 0; i < 3; i++ {
_, _, err := ssh.Connect()
// Should have error due to non-routable IP, but no resource leaks
assert.NotNil(t, err, "Connect should fail with timeout/connection error")
}
}
Loading