Closed hpoettker closed 6 months ago
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.
Attention: 17 lines
in your changes are missing coverage. Please review.
Comparison is base (
50c753d
) 68.85% compared to head (94fcc96
) 68.77%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
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 https://github.com/openssh/openssh-portable/commit/1edb00c58f8a6875fad6a497aa2bacf37f9e6cd5.
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: https://github.com/mwiede/jsch/pull/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: