Fix: Properly close SshClient and avoid reusing proxy SSH sessions #3944
Conversation
|
Hi maintainers 👋 |
|
Hi maintainers 👋 All requested fixes are now complete. Could someone please review the PR and approve the pending workflows when convenient? Thanks! |
1 similar comment
|
Hi maintainers 👋 All requested fixes are now complete. Could someone please review the PR and approve the pending workflows when convenient? Thanks! |
There was a problem hiding this comment.
Pull request overview
This pull request addresses SSH ProxyJump authentication leakage issues in Apache MINA SSHD by preventing reuse of proxy SSH sessions and ensuring proper resource cleanup.
Changes:
- Modified connection caching logic to exclude proxy connections from reuse (
reuseConnection && !useProxy) - Added exception handling to close ClientSession on failures in proxy scenarios
- Improved error logging messages for better diagnostics
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Apache SSHD requires the password for the proxy to be preloaded into the sshClient instance before connecting | ||
| if (StringUtils.hasText(sshProtocol.getProxyPassword())) { | ||
| sshClient.addPasswordIdentity(sshProtocol.getProxyPassword()); |
There was a problem hiding this comment.
Adding proxy password to the shared SshClient instance creates a potential security issue. Since CommonSshClient.getSshClient() returns a singleton SSH_CLIENT that is shared across all connections, adding a proxy password here will persist on that client instance and could leak to subsequent connections. This proxy password should be removed after use or the password identity should be added to the session instead of the client.
| CONNECTION_COMMON_CACHE.addCache(identifier, sshConnect); | ||
| if (!clientSession.auth() | ||
| .verify(timeout, TimeUnit.MILLISECONDS) | ||
| .isSuccess()) { |
There was a problem hiding this comment.
Authentication failure does not close the clientSession before throwing the exception. This creates a resource leak. The clientSession should be closed here similar to line 129 in the first method.
| .isSuccess()) { | |
| .isSuccess()) { | |
| if (clientSession != null && clientSession.isOpen()) { | |
| clientSession.close(); | |
| } |
| Iterable<KeyPair> keyPairs = | ||
| SecurityUtils.loadKeyPairIdentities( | ||
| null, | ||
| () -> resourceKey, | ||
| new FileInputStream(resourceKey), | ||
| passwordProvider); |
There was a problem hiding this comment.
This FileInputStream is not always closed on method exit.
| Iterable<KeyPair> keyPairs = | |
| SecurityUtils.loadKeyPairIdentities( | |
| null, | |
| () -> resourceKey, | |
| new FileInputStream(resourceKey), | |
| passwordProvider); | |
| Iterable<KeyPair> keyPairs; | |
| try (InputStream is = new FileInputStream(resourceKey)) { | |
| keyPairs = | |
| SecurityUtils.loadKeyPairIdentities( | |
| null, | |
| () -> resourceKey, | |
| is, | |
| passwordProvider); | |
| } |
|
Please review Copilot's results. |
4 participants
Problem
When using SSH ProxyJump, Apache MINA SSHD may leak authentication state.
Reusing sessions or incorrectly closing SshClient could cause the first
connection to fail or leave resources open.
Solution
Impact
Fixes #3925