Skip to content

Commit 851001c

Browse files
almailerartembilan
authored andcommitted
spring-projectsGH-10188: Close ClientSession in DefaultSftpSF on auth fail
Fixes: spring-projects#10188 * Prevent a connection leak due to `ClientSession` going out of scope in the `DefaultSftpSessionFactory.getSession()` when connecting is successful but authentication fails. Signed-off-by: alastair Mailer <[email protected]> **Auto-cherry-pick to `6.5.x` & `6.4.x`**
1 parent a3476d2 commit 851001c

File tree

2 files changed

+42
-4
lines changed

2 files changed

+42
-4
lines changed

spring-integration-sftp/src/main/java/org/springframework/integration/sftp/session/DefaultSftpSessionFactory.java

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@
7979
* @author Christian Tzolov
8080
* @author Adama Sorho
8181
* @author Darryl Smith
82+
* @author Alastair Mailer
8283
*
8384
* @since 2.0
8485
*/
@@ -351,7 +352,13 @@ private ClientSession initClientSession() throws IOException {
351352
.verify(verifyTimeout)
352353
.getSession();
353354

354-
clientSession.auth().verify(verifyTimeout);
355+
try {
356+
clientSession.auth().verify(verifyTimeout);
357+
}
358+
catch (IOException ex) {
359+
clientSession.close();
360+
throw ex;
361+
}
355362

356363
return clientSession;
357364
}

spring-integration-sftp/src/test/java/org/springframework/integration/sftp/session/SftpSessionFactoryTests.java

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
import org.springframework.core.task.SimpleAsyncTaskExecutor;
5353

5454
import static org.assertj.core.api.Assertions.assertThat;
55+
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
5556
import static org.assertj.core.api.Assertions.assertThatNoException;
5657
import static org.assertj.core.api.Assertions.fail;
5758
import static org.awaitility.Awaitility.await;
@@ -61,17 +62,18 @@
6162
* @author Artem Bilan
6263
* @author Auke Zaaiman
6364
* @author Darryl Smith
65+
* @author Alastair Mailer
6466
*
6567
* @since 3.0.2
6668
*/
67-
public class SftpSessionFactoryTests {
69+
class SftpSessionFactoryTests {
6870

6971
/*
7072
* Verify the socket is closed if the channel.connect() fails.
7173
* INT-3305
7274
*/
7375
@Test
74-
public void testConnectFailSocketOpen() throws Exception {
76+
void testConnectFailSocketOpen() throws Exception {
7577
try (SshServer server = SshServer.setUpDefaultServer()) {
7678
server.setPasswordAuthenticator((arg0, arg1, arg2) -> true);
7779
server.setPort(0);
@@ -118,7 +120,7 @@ public void testConnectFailSocketOpen() throws Exception {
118120
}
119121

120122
@Test
121-
public void concurrentGetSessionDoesntCauseFailure() throws Exception {
123+
void concurrentGetSessionDoesntCauseFailure() throws Exception {
122124
try (SshServer server = SshServer.setUpDefaultServer()) {
123125
server.setPasswordAuthenticator((arg0, arg1, arg2) -> true);
124126
server.setPort(0);
@@ -343,4 +345,33 @@ protected SftpClient createSftpClient(ClientSession clientSession,
343345
}
344346
}
345347

348+
/*
349+
* Verify the socket is closed if authentication fails.
350+
*/
351+
@Test
352+
void noClientSessionLeakOnAuthError() throws Exception {
353+
try (SshServer server = SshServer.setUpDefaultServer()) {
354+
server.setPasswordAuthenticator((arg0, arg1, arg2) -> false);
355+
server.setPort(0);
356+
server.setKeyPairProvider(new SimpleGeneratorHostKeyProvider(new File("hostkey.ser").toPath()));
357+
server.setSubsystemFactories(Collections.singletonList(new SftpSubsystemFactory()));
358+
server.start();
359+
360+
DefaultSftpSessionFactory sftpSessionFactory = new DefaultSftpSessionFactory();
361+
sftpSessionFactory.setHost("localhost");
362+
sftpSessionFactory.setPort(server.getPort());
363+
sftpSessionFactory.setUser("user");
364+
sftpSessionFactory.setAllowUnknownKeys(true);
365+
366+
assertThatIllegalStateException()
367+
.isThrownBy(sftpSessionFactory::getSession)
368+
.withCauseInstanceOf(SshException.class)
369+
.withStackTraceContaining("No more authentication methods available");
370+
371+
await().untilAsserted(() -> assertThat(server.getActiveSessions()).hasSize(0));
372+
373+
sftpSessionFactory.destroy();
374+
}
375+
}
376+
346377
}

0 commit comments

Comments
 (0)