Skip to content
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.

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

INT-4243: Upgrade to sshd 1.4 #2090

Closed
wants to merge 4 commits into from

Conversation

garyrussell
Copy link
Contributor

JIRA: https://jira.spring.io/browse/INT-4243

Remove newline char from key exchange.

we could consider just backporting the newline stripping if updating the test server version will cause issues with IO

JIRA: https://jira.spring.io/browse/INT-4243

Remove newline char from key exchange.
byte[] decodeBuffer = Base64.decodeBase64(StreamUtils.copyToByteArray(stream));
byte[] keyBytes = StreamUtils.copyToByteArray(stream);
// strip any newline chars
while (keyBytes[keyBytes.length - 1] == 0x0a || keyBytes[keyBytes.length - 1] == 0x0c) {
Copy link
Member

Choose a reason for hiding this comment

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

Any ideas why that failed sporadically?
Thanks

Merging BTW

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my comment on the JIRA - problem is unrelated but we should do the upgrade anyway.

server.setKeyPairProvider(new SimpleGeneratorHostKeyProvider(new File("hostkey.ser")));
server.setSubsystemFactories(Collections.<NamedFactory<Command>>singletonList(new SftpSubsystemFactory()));
server.setFileSystemFactory(
new VirtualFileSystemFactory(Paths.get(remoteTemporaryFolder.getRoot().getAbsolutePath())));
Copy link
Member

Choose a reason for hiding this comment

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

Why remoteTemporaryFolder.getRoot().toPath() doesn't work for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok; fine

while (keyBytes[keyBytes.length - 1] == 0x0a || keyBytes[keyBytes.length - 1] == 0x0c) {
keyBytes = Arrays.copyOf(keyBytes, keyBytes.length - 1);
}
byte[] decodeBuffer = Base64Utils.decode(keyBytes);
Copy link
Member

Choose a reason for hiding this comment

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

Hm... Fails for me on Windows like this:

java.lang.IllegalArgumentException: Illegal base64 character d
	at java.util.Base64$Decoder.decode0(Base64.java:714)
	at java.util.Base64$Decoder.decode(Base64.java:526)
	at org.springframework.util.Base64Utils.decode(Base64Utils.java:63)
	at org.springframework.integration.sftp.session.SftpServerTests.decodePublicKey(SftpServerTests.java:135)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, my mistake should be 0x0d not ox0c

More diagnostics.

It appears the new server doesn't close the file when the session is closed.

In the streaming test, consume the stream before closing the session.
import java.io.File;
import java.io.InputStream;
import java.util.List;
import java.util.regex.Matcher;

import org.apache.log4j.lf5.util.StreamUtils;
Copy link
Member

Choose a reason for hiding this comment

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

??? Let me guess you were going to use org.springframework.util.StreamUtils

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hehe

@@ -117,12 +119,14 @@ public void testSftpInboundStreamFlow() throws Exception {
assertNotNull(message);
assertThat(message.getPayload(), instanceOf(InputStream.class));
assertThat(message.getHeaders().get(FileHeaders.REMOTE_FILE), isOneOf(" sftpSource1.txt", "sftpSource2.txt"));
StreamUtils.copy((InputStream) message.getPayload(), new ByteArrayOutputStream());
Copy link
Member

Choose a reason for hiding this comment

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

Well, my solution is actually like this:

((InputStream) message.getPayload()).close();

Because the problem with the file on Windows that we have an open stream to it. And, yeah..., exactly payload is InputStream in case of streaming inbound channel adapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok; feel free to fix that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, windows did not say that the file deletion failed, just silently left the file in place, preventing the deletion of the directory.

Copy link
Member

Choose a reason for hiding this comment

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

Well, actually it does, if we check the result of the File.delete().
And that is exactly what we are doing now with all those your new log diagnostics.

OK, merging having your agreement...
I also will increase the timeout in that Redis test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right that's what I am saying; on my Windows VM, the file.delete() was successful (I didn't get the "couldn't delete" message) - that's why I added the additional "Contents" log when the directory delete fails.

}
}
}
file.delete();
logger.info("Deleting: " + file + " in " + testName.getMethodName());
Copy link
Member

Choose a reason for hiding this comment

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

I think we have figure out the problem with files and we don't need these diagnostics any more.
Let me know and I'll polish on merge respectively.

Thank you for the fix any way!

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 figured it won't hurt leaving these at INFO level - in case we have problems in future.

@artembilan
Copy link
Member

Merged as 4bb3f50 and cherry-picked to 4.3.x as 96331d5

@artembilan artembilan closed this Mar 11, 2017
@artembilan
Copy link
Member

Back-ported to 4.2.x as 8036a4b

@garyrussell garyrussell deleted the INT-4243 branch March 17, 2017 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants