-
Notifications
You must be signed in to change notification settings - Fork 641
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
Added possibility configure SFTP connection using private key #197. #202
Conversation
argast
commented
Feb 23, 2017
- Added SftpIdentity case class to allow configuring private/public key,
- Added Java API methods to configure SftpIdentity from Java
- Added Scala spec and Java test that should fail password based authentication and revert to private key one,
- Added docs paragraph to describe this option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!. Thanks for bringing this to the table.
@@ -97,7 +97,8 @@ object RemoteFileSettings { | |||
host: InetAddress, | |||
port: Int = DefaultSftpPort, | |||
credentials: FtpCredentials = AnonFtpCredentials, | |||
strictHostKeyChecking: Boolean = true | |||
strictHostKeyChecking: Boolean = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we deal with the case of strict host key checking in the context of this issue?. On that case, we probably should let specify a known_hosts
custom file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be useful as well. I will add it to SftpSettings then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional testing covering this would be welcome :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, will do :)
Some(SftpIdentity(name, privateKey, Some(password))) | ||
|
||
/** Java API */ | ||
def createSftpIdentity(name: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the format on signatures is not following the previous ones along the project. It's just a nitpick but important in the long term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to use withFoo methods similar to amqp model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nop, more silly than that. Just enter a carriage return after the parentheses so that scalafmt
line up left on the next line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I like the way withFoo works, it's a nicer way to handle Option in Java so I think I will migrate java API methods to withFoo way.
case class SftpIdentity(name: String, | ||
privateKey: Array[Byte], | ||
password: Option[Array[Byte]] = None, | ||
publicKey: Option[Array[Byte]] = None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be interesting to offer variants of the identity specification based on files on the classpath in addition to these Array[Byte]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good idea, I will add additional case class with possibility to specify location of key files and passphrase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just needs a rebase.
- Added SftpIdentity case class to allow configuring private/public key, - Added option to configure known_hosts file and test to check its usage. - Added spec that should fail password based authentication and revert to private key one, - Added docs paragraph to describe this option.
Thanks for the review. I rebased the code. |
Great. Thanks! |