feat: improve proxy SSH client management and configuration #99
Conversation
- Add settings.local.json file to configure Claude permissions - Ensure proxy SSH client connections are closed on timeout, error, or after successful connection in Connect method - Add a test to verify proxy clients are properly cleaned up during multiple Connect attempts fix #88 Signed-off-by: appleboy <[email protected]>
WalkthroughAdds local Claude settings. Updates proxy connection handling in easyssh.go to close proxyClient across success and error paths. Introduces a test validating cleanup on repeated proxy connection attempts. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Connect as MakeConfig.Connect
participant Proxy as ProxyServer
participant Target as TargetHost
participant SSH as SSHClient
Caller->>Connect: Connect(ctx, cfg with Proxy)
rect rgb(240,248,255)
note right of Connect: Proxy dial phase
Connect->>Proxy: ssh.Dial(protocol, server:port)
alt Context timeout or dial error
Connect-->>Proxy: proxyClient.Close()
Connect-->>Caller: error
else Dial OK
note right of Connect: Establish target connection via proxy
Connect->>Target: net.Dial/NewClientConn
alt NewClientConn error
Connect-->>Proxy: proxyClient.Close()
Connect-->>Caller: error
else Success
note right of Connect: Proxy no longer needed once ncc ready
Connect-->>Proxy: proxyClient.Close()
Connect->>SSH: NewClient(ncc)
Connect-->>Caller: *SSH client*
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Summary of ChangesHello @appleboy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the reliability and resource efficiency of the SSH proxy client by implementing robust connection cleanup logic. It ensures that proxy connections are always closed, regardless of whether the connection attempt succeeds or fails, thereby preventing potential resource leaks. A dedicated test case has been added to validate this improved cleanup behavior, alongside a minor update for Claude permissions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to improve SSH proxy client handling by ensuring connections are closed to prevent resource leaks. The changes correctly add cleanup logic for error scenarios, such as timeouts or connection failures. However, there is a critical issue where the proxy client is also closed upon a successful connection, which will cause the established SSH connection to fail immediately. I've left a comment explaining this bug and suggesting how to address the underlying resource leak issue properly. I've also included a suggestion to refactor the error handling to make it cleaner and more maintainable. The new test case for cleanup on connection failure is a good addition.
| // Close the proxy client after successfully establishing the target connection | ||
| // The target connection (ncc) is now independent of the proxy client | ||
| proxyClient.Close() |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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 pathThis way, you only need to manage the closeProxy flag, and the defer statement handles the cleanup on all error returns automatically.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.claude/settings.local.json (1)
1-9: Reconsider committing local configuration files.Files with
.localsuffix are typically user-specific configurations that should not be committed to version control. Consider:
- Adding
*.local.jsonto.gitignore- Providing a
.claude/settings.example.jsontemplate that users can copy- Documenting how developers should configure their local settings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.claude/settings.local.json(1 hunks)easyssh.go(1 hunks)easyssh_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using gofumpt (make fmt)
Run go vet to catch common issues (make vet)
Prefer secure cipher configurations; only enable UseInsecureCipher for legacy/compatibility needs
Enable SSH host key/fingerprint verification when available
Use golang.org/x/crypto/ssh for SSH protocol operations
Use github.com/ScaleFT/sshkeys for private key parsing (with passphrase support)
Maintain documented method signatures: Run() returns (stdout, stderr, isTimeout, error) and Stream() returns (stdoutChan, stderrChan, doneChan, errChan, error)
When configuring Proxy on MakeConfig, use DefaultConfig to define jumphost parameters
Files:
easyssh_test.goeasyssh.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use github.com/stretchr/testify in tests
Files:
easyssh_test.go
🧬 Code graph analysis (1)
easyssh_test.go (1)
easyssh.go (2)
MakeConfig(52-81)DefaultConfig(84-108)
🪛 GitHub Actions: Lint and Testing
easyssh_test.go
[error] 348-350: TestProxyClient failed. Received unexpected error: ssh: unexpected packet in response to channel open:
[error] 368-371: TestProxyClientSSHCommand failed. Expected 'drone-scp' but got '', and ssh: unexpected packet in response to channel open:
🔇 Additional comments (4)
easyssh.go (3)
295-296: LGTM! Proper cleanup on timeout.Correctly closes the proxy client when the context times out during the proxy dial operation, preventing resource leaks in this error path.
299-302: LGTM! Proper cleanup on dial error.Correctly closes the proxy client when
proxyClient.Dial()returns an error, preventing resource leaks in this error path.
304-308: LGTM! Proper cleanup on connection setup error.Correctly closes the proxy client when
ssh.NewClientConn()fails, preventing resource leaks in this error path.easyssh_test.go (1)
669-692: Test correctly validates error-path cleanup.The test appropriately verifies that failed connection attempts (using non-routable IPs) don't leak resources. Multiple Connect() attempts should fail gracefully without accumulating proxy client resources.
However, consider adding a test for the success path to verify that after a successful proxy connection is established:
- The connection remains functional (can execute commands)
- Resources are eventually cleaned up when the client is closed
Example test case:
func TestProxyClientSuccessPath(t *testing.T) { ssh := &MakeConfig{ User: "drone-scp", Server: "localhost", Port: "22", KeyPath: "./tests/.ssh/id_rsa", Proxy: DefaultConfig{ User: "drone-scp", Server: "localhost", Port: "22", KeyPath: "./tests/.ssh/id_rsa", }, } session, client, err := ssh.Connect() assert.NoError(t, err) assert.NotNil(t, session) assert.NotNil(t, client) // Verify connection is functional after successful connect err = session.Run("echo test") assert.NoError(t, err) // Cleanup session.Close() client.Close() }This would help catch issues where the proxy connection is prematurely closed in the success path (which appears to be causing the current pipeline failures).
| // Close the proxy client after successfully establishing the target connection | ||
| // The target connection (ncc) is now independent of the proxy client | ||
| proxyClient.Close() | ||
| client = ssh.NewClient(ncc, chans, reqs) |
There was a problem hiding this comment.
Critical: Closing proxyClient breaks the target connection.
Closing proxyClient immediately after establishing the target connection will break that connection. Here's why:
proxyClient.Dial()creates a channel through the proxy to the targetssh.NewClientConn()wraps that channel inncc- The channel remains dependent on
proxyClient's underlying transport - Closing
proxyClientcloses its transport connection, which closes all channels including the one used byncc
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).
1 participant
fix #88
Summary by CodeRabbit
Bug Fixes
Tests
Chores