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

Fix: if the client knows CA key, it should send host key algo proposal for certificates #733

Merged
merged 3 commits into from
Nov 10, 2021

Conversation

vladimirlagunov
Copy link
Contributor

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 findExistingAlgorithms and 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.

@vladimirlagunov vladimirlagunov marked this pull request as draft October 13, 2021 08:57
@vladimirlagunov vladimirlagunov marked this pull request as ready for review October 13, 2021 10:18
@hierynomus
Copy link
Owner

Would it make sense to use something like testcontainers for the integration testing, so that we can setup different containers for different scenarios. That would make this easier to test maybe?

@vladimirlagunov
Copy link
Contributor Author

I like testcontainers, and use it everywhere, but it would take time, and can cause a huge test refactoring.

@hierynomus
Copy link
Owner

I think a plain replacement of the current test setup might be not too hard, as all integration tests derive from the IntegrationBaseSpec

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.
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
Copy link
Contributor Author

Done.

This pull request also includes #741. It might be rebased on master when #741 is merged.

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.
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.
@hierynomus hierynomus merged commit 7c14098 into hierynomus:master Nov 10, 2021
@vladimirlagunov vladimirlagunov deleted the fix-host-certificates branch November 11, 2021 09:21
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants