-
Notifications
You must be signed in to change notification settings - Fork 602
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
Implement OpenSSH strict key exchange extension #917
Conversation
Thanks for the PR. It would be great to have some unit and/or integration tests attached! |
Yes, I still need to find a good approach for them. 😄 The integration tests cover most of the changes automatically, but the interesting bits only if the openssh version is new enough. I'll have a look whether there is a way to test things specifically, e.g. by assertions on the logs. Unit tests that are not too brittle might be harder. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #917 +/- ##
============================================
- Coverage 68.85% 68.77% -0.09%
- Complexity 1430 1438 +8
============================================
Files 208 208
Lines 7574 7602 +28
Branches 651 658 +7
============================================
+ Hits 5215 5228 +13
- Misses 2012 2019 +7
- Partials 347 355 +8 ☔ View full report in Codecov by Sentry. |
I updated the alpine base image for the integration tests to 3.19.0 so that they now use OpenSSH 9.6. I also added an integration test that makes specific assertions on the logs. |
Great work! I'm going to merge it. |
(cherry picked from commit a262f51)
Resolves #916
The PR implements the algorithm described in section 1.9 of https://github.com/openssh/openssh-portable/blob/master/PROTOCOL and also follows the changes implemented in the commit openssh/openssh-portable@1edb00c.
All tests are successful. The integration tests work both against the current container and a custom built container with OpenSSH 9.6p1. When run against the latter, the log also show that the resets of sequence numbers are happening, and working correctly as otherwise ChaCha20-Poly1305 should break.
The jsch fork of mwiede also implemented config switches to disable or enforce the strict key exchange extension. But I'm not sure whether it makes sense to maintain such flags long term when OpenSSH itself doesn't have them: mwiede/jsch#461
The Terrapin Scanner referenced in #916 is happy, but I think it only checks whether the additional pseudo-key exchange
kex-strict-c-v00@openssh.com
is being advertised by the client: