Fix: if the client knows CA key, it should send host key algo proposal for certificates #733
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vladimirlagunov
force-pushed
the
fix-host-certificates
branch
from
ce18111
to
d68f27b
Compare
|
Would it make sense to use something like |
|
I like testcontainers, and use it everywhere, but it would take time, and can cause a huge test refactoring. |
|
I think a plain replacement of the current test setup might be not too hard, as all integration tests derive from the |
vladimirlagunov
added a commit
to vladimirlagunov/sshj
that referenced
this pull request
Nov 10, 2021
It allows running different SSH servers with different configurations in tests, giving ability to cover more bugs, like mentioned in hierynomus#733.
Merged
vladimirlagunov
added a commit
to vladimirlagunov/sshj
that referenced
this pull request
Nov 10, 2021
Required to verify the case with wrong host key algorithm proposals. See hierynomus#733
vladimirlagunov
force-pushed
the
fix-host-certificates
branch
from
d68f27b
to
6eb1f90
Compare
hierynomus
pushed a commit
that referenced
this pull request
Nov 10, 2021
* Replace abstract class IntegrationBaseSpec with composition through IntegrationTestUtil * Switch to testcontainers in integration tests It allows running different SSH servers with different configurations in tests, giving ability to cover more bugs, like mentioned in #733.
…l for certificates
Required to verify the case with wrong host key algorithm proposals. See hierynomus#733
…icKeyAuthWithCertificateSpec Prevents from starting unnecessary SSHD containers, making the tests run a bit faster when they are launched separately.
vladimirlagunov
force-pushed
the
fix-host-certificates
branch
from
6eb1f90
to
cd5779e
Compare
vladimirlagunov
added a commit
to vladimirlagunov/sshj
that referenced
this pull request
Jan 21, 2022
* Replace abstract class IntegrationBaseSpec with composition through IntegrationTestUtil * Switch to testcontainers in integration tests It allows running different SSH servers with different configurations in tests, giving ability to cover more bugs, like mentioned in hierynomus#733. (cherry picked from commit d5805a6)
vladimirlagunov
added a commit
to vladimirlagunov/sshj
that referenced
this pull request
Jan 21, 2022
…l for certificates (hierynomus#733) * Fix: if the client knows CA key, it should send host key algo proposal for certificates * Run specific SSH server in KeyWithCertificateSpec Required to verify the case with wrong host key algorithm proposals. See hierynomus#733 * Split KeyWithCertificateSpec into HostKeyWithCertificateSpec and PublicKeyAuthWithCertificateSpec Prevents from starting unnecessary SSHD containers, making the tests run a bit faster when they are launched separately. (cherry picked from commit 7c14098)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
My bad, I should have verified the new version before insisting on the release.
After updating the library, the tests written in advance on our side kept failing. Bisecting our code base didn't bring any results. Then I bisected SSHJ and found the commit where host key certificates broke down: 14bf93e The client started sending wrong host key algorithms proposals to the server for certificates. Undoubtedly, it's the tests problem that the host certificate tests didn't catch the bug.
Then I started figuring out why our local tests catched the bug, but tests from SSHJ didn't. Comparing good and bad OpenSSH server logs, I came across the way how kex proposals are generated. By default, Proposal.java sends an intersection of key types from the config, and key types from the known hosts file. However, if the intersection is empty, it sends the key types from the config. The intersection wasn't empty in the intellij tests, but was empty here, and the expected key type was sent to the server in these tests, not verifying the kex proposal logic. IntelliJ SSH tests use testcontainers, and are able to launch various servers. In contrast, these tests connect to a container started preliminary, therefore can't change the server configuration on the fly. That's why I had decided to write a filter for the host keys in the SSHJ test, which turned out to be buggy.
Luckily, the bug has an easy workaround on the users' side. I managed to get it working correctly on our side by overriding
findExistingAlgorithmsand adding a bit of reflection magic. I guess, everyone else could do the same for the first time. So, I don't treat the pull request as a show-stopper.Indeed, the bug fix looks ugly. Feels like the knowledge about various key types is not encapsulated well. Please, propose a better fix if you know it.