-
Notifications
You must be signed in to change notification settings - Fork 41
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
Bump com.hierynomus:sshj from 0.32.0 to 0.36.0 in /prime-router #11246
Changes from all commits
68de388
014296e
edbe1b7
5fbd168
c03c683
002a3af
b9cf332
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package gov.cdc.prime.router.transport | ||
|
||
import com.google.common.base.Preconditions | ||
import com.hierynomus.sshj.key.KeyAlgorithms | ||
import com.microsoft.azure.functions.ExecutionContext | ||
import gov.cdc.prime.router.Receiver | ||
import gov.cdc.prime.router.Report | ||
|
@@ -306,6 +307,29 @@ class SftpTransport : ITransport, Logging { | |
// allow us to mock SSHClient because there is no dependency injection in this class | ||
fun createDefaultSSHClient(): SSHClient { | ||
val sshConfig = DefaultConfig() | ||
|
||
// Started from version 0.33.0, SSHJ doesn't try to determine RSA-SHA2-* support on fly. | ||
// Instead, it looks only config.getKeyAlgorithms(), which may or may not contain ssh-rsa | ||
// and rsa-sha2-* in any order. The default config stops working with old servers like | ||
// Apache SSHD that doesn't rsa-sha2-* signatures. To make it works with old servers, | ||
// we need to include the KeyAlgorithms.SSHRSA at the top of the list or have higher | ||
// priority than other as below. | ||
sshConfig.keyAlgorithms = listOf( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth creating a tech debt ticket to make this configurable per receiver? I remember you saying this breaks after trying the first three key algorithms, is that still the case or is this not related to that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it isn't worth it to create a tech dept for this. After this fix, it works with all of our reivers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @snesm our Security expert approved the fix. However, he suggested I get with the receiver who still using the SSHRAS key exchange and suggest they upgrade their server to support the new key exchange. |
||
KeyAlgorithms.SSHRSA(), | ||
KeyAlgorithms.EdDSA25519CertV01(), | ||
KeyAlgorithms.EdDSA25519(), | ||
KeyAlgorithms.ECDSASHANistp521CertV01(), | ||
KeyAlgorithms.ECDSASHANistp521(), | ||
KeyAlgorithms.ECDSASHANistp384CertV01(), | ||
KeyAlgorithms.ECDSASHANistp384(), | ||
KeyAlgorithms.ECDSASHANistp256CertV01(), | ||
KeyAlgorithms.ECDSASHANistp256(), | ||
KeyAlgorithms.RSASHA512(), | ||
KeyAlgorithms.RSASHA256(), | ||
KeyAlgorithms.SSHRSACertV01(), | ||
KeyAlgorithms.SSHDSSCertV01(), | ||
KeyAlgorithms.SSHDSA() | ||
) | ||
return SSHClient(sshConfig) | ||
} | ||
} | ||
|
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.
Depending on Java standard library is always a good idea simple operations like this 👍
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.
Thank you @jalbinson