Skip to content

Fix: Properly close SshClient and avoid reusing proxy SSH sessions #3944

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

Closed
love12yadav wants to merge 6 commits into from
Closed

Fix: Properly close SshClient and avoid reusing proxy SSH sessions #3944

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
68c33b4
Fix: properly manage SshClient lifecycle and proxy sessions
love12yadav Jan 5, 2026
63b4e23
Fix checkstyle issues and SSH client lifecycle handling
love12yadav Jan 5, 2026
d85839d
Merge branch 'master' into fix-ssh-client-lifecycle
Duansg Jan 7, 2026
db36392
Merge branch 'master' into fix-ssh-client-lifecycle
yuluo-yx Jan 8, 2026
c779606
Fix: resolve merge conflict and correct SSH client lifecycle
love12yadav Jan 8, 2026
3d8d848
Merge branch 'master' into fix-ssh-client-lifecycle
Duansg Jan 16, 2026
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
240 changes: 152 additions & 88 deletions ...tor-common/src/main/java/org/apache/hertzbeat/collector/collect/common/ssh/SshHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,17 @@

package org.apache.hertzbeat.collector.collect.common.ssh;

import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.security.GeneralSecurityException;
import java.security.KeyPair;
import java.util.List;
import java.util.Optional;
import java.util.concurrent.TimeUnit;

import lombok.extern.slf4j.Slf4j;

import org.apache.hertzbeat.collector.collect.common.cache.AbstractConnection;
import org.apache.hertzbeat.collector.collect.common.cache.CacheIdentifier;
import org.apache.hertzbeat.collector.collect.common.cache.GlobalConnectionCache;
Expand All @@ -34,173 +41,230 @@
import org.apache.sshd.common.util.security.SecurityUtils;
import org.springframework.util.StringUtils;

import java.io.FileInputStream;
import java.io.IOException;
import java.security.GeneralSecurityException;
import java.util.Optional;
import java.util.concurrent.TimeUnit;

/**
* ssh helper
* SSH helper
*/
@Slf4j
public class SshHelper {

private static final GlobalConnectionCache CONNECTION_COMMON_CACHE = GlobalConnectionCache.getInstance();
private static final GlobalConnectionCache CONNECTION_COMMON_CACHE =
GlobalConnectionCache.getInstance();

public static ClientSession getConnectSession(
String host,
String port,
String username,
String password,
String privateKey,
String privateKeyPassphrase,
int timeout,
boolean reuseConnection)
throws IOException, GeneralSecurityException {

public static ClientSession getConnectSession(String host, String port, String username, String password, String privateKey,
String privateKeyPassphrase, int timeout, boolean reuseConnection)
throws IOException, GeneralSecurityException {
CacheIdentifier identifier = CacheIdentifier.builder()
.ip(host).port(port)
.username(username).password(password)
.build();
.ip(host)
.port(port)
.username(username)
.password(password)
.build();

ClientSession clientSession = null;

if (reuseConnection) {
Optional<AbstractConnection<?>> cacheOption = CONNECTION_COMMON_CACHE.getCache(identifier, true);
Optional<AbstractConnection<?>> cacheOption =
CONNECTION_COMMON_CACHE.getCache(identifier, true);

if (cacheOption.isPresent()) {
SshConnect sshConnect = (SshConnect) cacheOption.get();
clientSession = sshConnect.getConnection();
try {
if (clientSession == null || clientSession.isClosed() || clientSession.isClosing()) {
if (clientSession == null
|| clientSession.isClosed()
|| clientSession.isClosing()) {
clientSession = null;
CONNECTION_COMMON_CACHE.removeCache(identifier);
}
} catch (Exception e) {
log.warn(e.getMessage());
log.warn("Failed to validate cached ssh session", e);
clientSession = null;
CONNECTION_COMMON_CACHE.removeCache(identifier);
}
}

if (clientSession != null) {
return clientSession;
}
}
SshClient sshClient = CommonSshClient.getSshClient();

SshClient sshClient = CommonSshClient.getSshClient();
clientSession = sshClient.connect(username, host, Integer.parseInt(port))
.verify(timeout, TimeUnit.MILLISECONDS).getSession();
.verify(timeout, TimeUnit.MILLISECONDS)
.getSession();

if (StringUtils.hasText(password)) {
clientSession.addPasswordIdentity(password);
} else if (StringUtils.hasText(privateKey)) {
var resourceKey = PrivateKeyUtils.writePrivateKey(host, privateKey);
FilePasswordProvider passwordProvider = (session, resource, index) -> {
if (StringUtils.hasText(privateKeyPassphrase)) {
return privateKeyPassphrase;
}
return null;
};
SecurityUtils.loadKeyPairIdentities(null, () -> resourceKey, new FileInputStream(resourceKey), passwordProvider)
.forEach(clientSession::addPublicKeyIdentity);
} // else auth with localhost private public key certificates

// auth
if (!clientSession.auth().verify(timeout, TimeUnit.MILLISECONDS).isSuccess()) {
String resourceKey = PrivateKeyUtils.writePrivateKey(host, privateKey);
FilePasswordProvider passwordProvider =
(session, resource, index) ->
StringUtils.hasText(privateKeyPassphrase)
? privateKeyPassphrase
: null;

Iterable<KeyPair> keyPairs =
SecurityUtils.loadKeyPairIdentities(
null,
() -> resourceKey,
new FileInputStream(resourceKey),
passwordProvider);
Comment on lines +114 to +119
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

This FileInputStream is not always closed on method exit.

Suggested change
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);
}

Copilot uses AI. Check for mistakes.

if (keyPairs != null) {
keyPairs.forEach(clientSession::addPublicKeyIdentity);
}
}

if (!clientSession.auth()
.verify(timeout, TimeUnit.MILLISECONDS)
.isSuccess()) {
clientSession.close();
throw new IllegalArgumentException("ssh auth failed.");
}

if (reuseConnection) {
SshConnect sshConnect = new SshConnect(clientSession);
CONNECTION_COMMON_CACHE.addCache(identifier, sshConnect);
CONNECTION_COMMON_CACHE.addCache(identifier, new SshConnect(clientSession));
}

return clientSession;
}

public static ClientSession getConnectSession(SshProtocol sshProtocol, int timeout, boolean reuseConnection, boolean useProxy)
throws IOException, GeneralSecurityException {
public static ClientSession getConnectSession(
SshProtocol sshProtocol,
int timeout,
boolean reuseConnection,
boolean useProxy)
throws IOException, GeneralSecurityException {

CacheIdentifier identifier = CacheIdentifier.builder()
.ip(sshProtocol.getHost()).port(sshProtocol.getPort())
.username(sshProtocol.getUsername()).password(sshProtocol.getPassword())
.build();
.ip(sshProtocol.getHost())
.port(sshProtocol.getPort())
.username(sshProtocol.getUsername())
.password(sshProtocol.getPassword())
.build();

ClientSession clientSession = null;
// When using ProxyJump, force connection reuse:
// Apache MINA SSHD will pass the proxy password error to the target host in proxy scenarios, causing the first connection to fail.
// Reusing connections can skip duplicate authentication and avoid this problem.
if (reuseConnection || useProxy) {
Optional<AbstractConnection<?>> cacheOption = CONNECTION_COMMON_CACHE.getCache(identifier, true);

if (reuseConnection && !useProxy) {
Optional<AbstractConnection<?>> cacheOption =
CONNECTION_COMMON_CACHE.getCache(identifier, true);

if (cacheOption.isPresent()) {
SshConnect sshConnect = (SshConnect) cacheOption.get();
clientSession = sshConnect.getConnection();
try {
if (clientSession == null || clientSession.isClosed() || clientSession.isClosing()) {
if (clientSession == null
|| clientSession.isClosed()
|| clientSession.isClosing()) {
clientSession = null;
CONNECTION_COMMON_CACHE.removeCache(identifier);
}
} catch (Exception e) {
log.warn(e.getMessage());
log.warn("Failed to validate cached ssh session", e);
clientSession = null;
CONNECTION_COMMON_CACHE.removeCache(identifier);
}
}

if (clientSession != null) {
return clientSession;
}
}

SshClient sshClient = CommonSshClient.getSshClient();
HostConfigEntry proxyConfig = new HostConfigEntry();

if (useProxy && StringUtils.hasText(sshProtocol.getProxyHost())) {
String proxySpec = String.format("%s@%s:%d", sshProtocol.getProxyUsername(), sshProtocol.getProxyHost(), Integer.parseInt(sshProtocol.getProxyPort()));
String proxySpec = String.format(
"%s@%s:%d",
sshProtocol.getProxyUsername(),
sshProtocol.getProxyHost(),
Integer.parseInt(sshProtocol.getProxyPort()));

proxyConfig.setHostName(sshProtocol.getHost());
proxyConfig.setHost(sshProtocol.getHost());
proxyConfig.setPort(Integer.parseInt(sshProtocol.getPort()));
proxyConfig.setUsername(sshProtocol.getUsername());
proxyConfig.setProxyJump(proxySpec);

// 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());
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
log.debug("Loaded proxy server password authentication: {}@{}", sshProtocol.getProxyUsername(), sshProtocol.getProxyHost());
}

if (StringUtils.hasText(sshProtocol.getProxyPrivateKey())) {
proxyConfig.setIdentities(List.of(sshProtocol.getProxyPrivateKey()));
log.debug("Proxy private key loaded into HostConfigEntry");
proxyConfig.setIdentities(
List.of(sshProtocol.getProxyPrivateKey()));
}
}

if (useProxy && StringUtils.hasText(sshProtocol.getProxyHost())) {
try {
try {
if (useProxy && StringUtils.hasText(sshProtocol.getProxyHost())) {
clientSession = sshClient.connect(proxyConfig)
.verify(timeout, TimeUnit.MILLISECONDS).getSession();
}
finally {
sshClient.removePasswordIdentity(sshProtocol.getProxyPassword());
.verify(timeout, TimeUnit.MILLISECONDS)
.getSession();
} else {
clientSession = sshClient.connect(
sshProtocol.getUsername(),
sshProtocol.getHost(),
Integer.parseInt(sshProtocol.getPort()))
.verify(timeout, TimeUnit.MILLISECONDS)
.getSession();
}
} else {
clientSession = sshClient.connect(sshProtocol.getUsername(), sshProtocol.getHost(), Integer.parseInt(sshProtocol.getPort()))
.verify(timeout, TimeUnit.MILLISECONDS).getSession();
}

if (StringUtils.hasText(sshProtocol.getPassword())) {
clientSession.addPasswordIdentity(sshProtocol.getPassword());
} else if (StringUtils.hasText(sshProtocol.getPrivateKey())) {
var resourceKey = PrivateKeyUtils.writePrivateKey(sshProtocol.getHost(), sshProtocol.getPrivateKey());
try (InputStream keyStream = new FileInputStream(resourceKey)) {
FilePasswordProvider passwordProvider = (session, resource, index) -> {
if (StringUtils.hasText(sshProtocol.getPrivateKeyPassphrase())) {
return sshProtocol.getPrivateKeyPassphrase();
if (StringUtils.hasText(sshProtocol.getPassword())) {
clientSession.addPasswordIdentity(sshProtocol.getPassword());
} else if (StringUtils.hasText(sshProtocol.getPrivateKey())) {
String resourceKey = PrivateKeyUtils.writePrivateKey(
sshProtocol.getHost(),
sshProtocol.getPrivateKey());

try (InputStream keyStream = new FileInputStream(resourceKey)) {
FilePasswordProvider passwordProvider =
(session, resource, index) ->
StringUtils.hasText(
sshProtocol.getPrivateKeyPassphrase())
? sshProtocol.getPrivateKeyPassphrase()
: null;

Iterable<KeyPair> keyPairs =
SecurityUtils.loadKeyPairIdentities(
null,
() -> resourceKey,
keyStream,
passwordProvider);

if (keyPairs != null) {
keyPairs.forEach(clientSession::addPublicKeyIdentity);
}
return null;
};
Iterable<KeyPair> keyPairs = SecurityUtils.loadKeyPairIdentities(null, () -> resourceKey, keyStream, passwordProvider);
if (keyPairs != null) {
keyPairs.forEach(clientSession::addPublicKeyIdentity);
} else {
log.error("Failed to load private key pairs from: {}", resourceKey);
}
} catch (IOException e) {
log.error("Error reading private key file: {}", e.getMessage());
}
} // else auth with localhost private public key certificates

// auth
if (!clientSession.auth().verify(timeout, TimeUnit.MILLISECONDS).isSuccess()) {
clientSession.close();
throw new IllegalArgumentException("ssh auth failed.");
}
if (reuseConnection || useProxy) {
SshConnect sshConnect = new SshConnect(clientSession);
CONNECTION_COMMON_CACHE.addCache(identifier, sshConnect);
if (!clientSession.auth()
.verify(timeout, TimeUnit.MILLISECONDS)
.isSuccess()) {
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
.isSuccess()) {
.isSuccess()) {
if (clientSession != null && clientSession.isOpen()) {
clientSession.close();
}

Copilot uses AI. Check for mistakes.
throw new IllegalArgumentException("ssh auth failed.");
}

if (reuseConnection && !useProxy) {
CONNECTION_COMMON_CACHE.addCache(
identifier, new SshConnect(clientSession));
}

return clientSession;

} catch (Exception e) {
if (clientSession != null && clientSession.isOpen()) {
clientSession.close();
}
throw e;
}
return clientSession;
}
}