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

FTP: Refactoring on SSH (jsch -> sshj) (previously "Secure FTP connection intermittently fails") #373

Merged
merged 1 commit into from Jul 12, 2017

Conversation

juanjovazquez
Copy link

@juanjovazquez juanjovazquez commented Jul 2, 2017

The sshj library is used now instead of jsch for ssh handling. Both main and test code have been accordingly updated. The original and motivating issue regarding intermittent cryptography-related failures is no longer observed (at least for the time being).

closes #365

@juanjovazquez
Copy link
Author

@argast According to #197 you worked on key based authentication for the sFTP integration. As explained in #365 we've had to change the ssh library we relied on. I've adapted the code but now the StrictHostKeyCheckingSftpSourceSpec test is not working. It seems that the client is trying to validate an RSA public key when the known_hosts file is declaring a DSA key. Could you please have it a look?. Thanks! :-)

@piotrkwiecinski
Copy link

@juanjovazquez in StrictHostKeyCheckingSftpSourceSpec when I removed

.withOptions(new Pair("HostKeyAlgorithms", "+ssh-dss"))

Tests pass.

In OpenSSHKnownHosts in sshj there is detection of key types:

https://github.com/hierynomus/sshj/blob/master/src/main/java/net/schmizz/sshj/transport/verification/OpenSSHKnownHosts.java#L183

Maybe it's enough? Bare in mind my knowledge is quite limited :)

@argast
Copy link
Contributor

argast commented Jul 3, 2017

Yes, this option was added because test key was in less secure des format which required additional ssh option to authenticate with it.

@piotrkwiecinski
Copy link

@juanjovazquez actually I've made mistake I have the same error...I'll try to play around with it tomorrow.

@juanjovazquez juanjovazquez changed the title WIP: Secure FTP connection intermittently fails FTP: Refactoring on SSH (jsch -> sshj) (before "Secure FTP connection intermittently fails") Jul 5, 2017
@juanjovazquez juanjovazquez changed the title FTP: Refactoring on SSH (jsch -> sshj) (before "Secure FTP connection intermittently fails") FTP: Refactoring on SSH (jsch -> sshj) (previous "Secure FTP connection intermittently fails") Jul 5, 2017
@juanjovazquez juanjovazquez changed the title FTP: Refactoring on SSH (jsch -> sshj) (previous "Secure FTP connection intermittently fails") FTP: Refactoring on SSH (jsch -> sshj) (previously "Secure FTP connection intermittently fails") Jul 5, 2017
case identity: KeyFileSftpIdentity =>
ftpClient.addIdentity(identity.privateKey, identity.publicKey.orNull, identity.password.orNull)
}
// ssh connection
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: maybe we could do without these comments

handler.get(name)
val remoteFile = handler.open(name, Set(OpenMode.READ).asJava)
val is = new remoteFile.RemoteFileInputStream()
if (is != null) is else throw new IOException(s"$name: No such file or directory")
Copy link
Contributor

Choose a reason for hiding this comment

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

could be

Option(is).getOrElse(throw new IOException(s"$name: No such file or directory"))

val remoteFile =
handler.open(name, openModes.asJava)
val os = new remoteFile.RemoteFileOutputStream()
if (os != null) os else throw new IOException(s"Could not write to $name")
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

import OpenMode._
val openModes = Set(WRITE, CREAT) ++ (if (append) Set(APPEND) else Set())
val remoteFile =
handler.open(name, openModes.asJava)
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting: this could be put on the line above

@juanjovazquez
Copy link
Author

@svezfaz Your requests have been applied. Thanks!.

@raboof
Copy link
Member

raboof commented Jul 11, 2017

What is the status of this PR? Is this now a complete replacement for the problematic approach or is there still work to do around host checking?

@juanjovazquez
Copy link
Author

@raboof IMHO this PR is ready for merging.

Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Changes look reasonable, did some testing myself, appears to work.

@raboof raboof merged commit a757056 into akka:master Jul 12, 2017
@raboof raboof mentioned this pull request Jul 21, 2017
@juanjovazquez juanjovazquez deleted the 365-ftp-ssh branch July 24, 2017 16:27
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.

Secure FTP connection intermittently fails
5 participants