Skip to content

GH-8581: Do not overwrite configuration of external provided SshClient #8584

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
wants to merge 2 commits into from
Closed

GH-8581: Do not overwrite configuration of external provided SshClient #8584

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
115dc1e
GH-8581: Do not overwrite configuration of external provided SshClient
azaaiman Mar 26, 2023
cb1f108
GH-8581: Do not overwrite configuration of external provided SshClient
azaaiman Mar 28, 2023
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
63 changes: 36 additions & 27 deletions ...src/main/java/org/springframework/integration/sftp/session/DefaultSftpSessionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
* @author Pat Turner
* @author Artem Bilan
* @author Krzysztof Debski
* @author Auke Zaaiman
*
* @since 2.0
*/
Expand Down Expand Up @@ -335,36 +336,44 @@ private void doInitClient() throws IOException {
if (this.port <= 0) {
this.port = SshConstants.DEFAULT_PORT;
}
ServerKeyVerifier serverKeyVerifier =
this.allowUnknownKeys ? AcceptAllServerKeyVerifier.INSTANCE : RejectAllServerKeyVerifier.INSTANCE;
if (this.knownHosts != null) {
serverKeyVerifier = new ResourceKnownHostsServerKeyVerifier(this.knownHosts);
}
this.sshClient.setServerKeyVerifier(serverKeyVerifier);

this.sshClient.setPasswordIdentityProvider(PasswordIdentityProvider.wrapPasswords(this.password));
if (this.privateKey != null) {
IoResource<Resource> privateKeyResource =
new AbstractIoResource<>(Resource.class, this.privateKey) {

@Override
public InputStream openInputStream() throws IOException {
return getResourceValue().getInputStream();
}
};
try {
Collection<KeyPair> keys =
SecurityUtils.getKeyPairResourceParser()
.loadKeyPairs(null, privateKeyResource,
FilePasswordProvider.of(this.privateKeyPassphrase));
this.sshClient.setKeyIdentityProvider(KeyIdentityProvider.wrapKeyPairs(keys));

doInitInnerClient();

this.sshClient.start();
}

private void doInitInnerClient() throws IOException {
if (this.isInnerClient) {
ServerKeyVerifier serverKeyVerifier =
this.allowUnknownKeys ? AcceptAllServerKeyVerifier.INSTANCE : RejectAllServerKeyVerifier.INSTANCE;
if (this.knownHosts != null) {
serverKeyVerifier = new ResourceKnownHostsServerKeyVerifier(this.knownHosts);
}
catch (GeneralSecurityException ex) {
throw new IOException("Cannot load private key: " + this.privateKey.getFilename(), ex);
this.sshClient.setServerKeyVerifier(serverKeyVerifier);

this.sshClient.setPasswordIdentityProvider(PasswordIdentityProvider.wrapPasswords(this.password));
if (this.privateKey != null) {
IoResource<Resource> privateKeyResource =
new AbstractIoResource<>(Resource.class, this.privateKey) {

@Override
public InputStream openInputStream() throws IOException {
return getResourceValue().getInputStream();
}
};
try {
Collection<KeyPair> keys =
SecurityUtils.getKeyPairResourceParser()
.loadKeyPairs(null, privateKeyResource,
FilePasswordProvider.of(this.privateKeyPassphrase));
this.sshClient.setKeyIdentityProvider(KeyIdentityProvider.wrapKeyPairs(keys));
}
catch (GeneralSecurityException ex) {
throw new IOException("Cannot load private key: " + this.privateKey.getFilename(), ex);
}
}
this.sshClient.setUserInteraction(this.userInteraction);
}
this.sshClient.setUserInteraction(this.userInteraction);
this.sshClient.start();
}

@Override
Expand Down
26 changes: 26 additions & 0 deletions ...p/src/test/java/org/springframework/integration/sftp/session/SftpSessionFactoryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
import java.util.Collections;
import java.util.List;

import org.apache.sshd.client.SshClient;
import org.apache.sshd.client.auth.password.PasswordIdentityProvider;
import org.apache.sshd.client.keyverifier.AcceptAllServerKeyVerifier;
import org.apache.sshd.common.SshException;
import org.apache.sshd.server.SshServer;
import org.apache.sshd.server.keyprovider.SimpleGeneratorHostKeyProvider;
Expand All @@ -35,10 +38,12 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.fail;
import static org.awaitility.Awaitility.await;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
Copy link
Member

Choose a reason for hiding this comment

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

Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-sftp/src/test/java/org/springframework/integration/sftp/session/SftpSessionFactoryTests.java:41:47: Using a static member import should be avoided - org.junit.jupiter.api.Assertions.assertDoesNotThrow. [AvoidStaticImport]

We prefer to use assertion from AsssertJ library. See assertThatNoException() instead of JUnit's one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I replaced it.


/**
* @author Gary Russell
* @author Artem Bilan
* @author Auke Zaaiman
*
* @since 3.0.2
*/
Expand Down Expand Up @@ -126,4 +131,25 @@ public void concurrentGetSessionDoesntCauseFailure() throws IOException {
}
}

@Test
void externallyProvidedSshClientShouldNotHaveItsConfigurationOverwritten() throws IOException {
try (SshServer server = SshServer.setUpDefaultServer()) {
server.setPasswordAuthenticator((arg0, arg1, arg2) -> true);
server.setPort(0);
server.setKeyPairProvider(new SimpleGeneratorHostKeyProvider(new File("hostkey.ser").toPath()));
server.setSubsystemFactories(Collections.singletonList(new SftpSubsystemFactory()));
server.start();

SshClient externalClient = SshClient.setUpDefaultClient();
externalClient.setServerKeyVerifier(AcceptAllServerKeyVerifier.INSTANCE);
externalClient.setPasswordIdentityProvider(PasswordIdentityProvider.wrapPasswords("pass"));

DefaultSftpSessionFactory sftpSessionFactory = new DefaultSftpSessionFactory(externalClient, false);
sftpSessionFactory.setHost("localhost");
sftpSessionFactory.setPort(server.getPort());
sftpSessionFactory.setUser("user");

assertDoesNotThrow(() -> sftpSessionFactory.getSession().connect());
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that this test really reflects what we would expect from its name.
I don't even think that we need to test against the real server and make connections.
There is probably just enough to verify against some mock that its method calls don't happen or so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have thought about using some mock.
But the init code does start a connection and tries to authenticate.

What you would want is something like this:

@Test
void externallyProvidedSshClientShouldNotHaveItsConfigurationOverwritten() throws IOException {
	SshClient externalClient = spy(SshClient.setUpDefaultClient());

	DefaultSftpSessionFactory sftpSessionFactory = new DefaultSftpSessionFactory(externalClient, false);
	sftpSessionFactory.setHost("localhost");
	sftpSessionFactory.setUser("user");

	assertThatNoException().isThrownBy(() -> sftpSessionFactory.getSession());

	verify(externalClient, never()).setServerKeyVerifier(any());
	verify(externalClient, never()).setPasswordIdentityProvider(any());
	verify(externalClient, never()).setKeyIdentityProvider(any());
	verify(externalClient, never()).setUserInteraction(any());
}

But that would raise an exception:

java.lang.IllegalStateException: failed to create SFTP Session
	at org.springframework.integration.sftp.session.DefaultSftpSessionFactory.getSession(DefaultSftpSessionFactory.java:292)
...
Caused by: org.apache.sshd.common.SshException: No more authentication methods available
...
	at org.springframework.integration.sftp.session.DefaultSftpSessionFactory.initClientSession(DefaultSftpSessionFactory.java:319)
	at org.springframework.integration.sftp.session.DefaultSftpSessionFactory.getSession(DefaultSftpSessionFactory.java:282)
	... 89 more
Caused by: org.apache.sshd.common.SshException: No more authentication methods available
	at org.apache.sshd.client.session.ClientUserAuthService.tryNext(ClientUserAuthService.java:379)

I doubt it would be wise to mock the better part of the calls which happen in the init towards the SshClient.

Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... I see: the integration with that SshClient API is very heavy, therefore it is probably better to exchange a performance of the test in favor of clear code and direct calls instead of our error-prone mocks.
So, never mind then and thank you for your thoughts!
Please, consider to fix an assert in favor of assertThatNoException() - and we are good to merge this.

}
}
}