tomaswolf commented on PR #446:
URL: https://github.com/apache/mina-sshd/pull/446#issuecomment-1870636101
I find this very hard to review.
1. That reformatting should be avoided. It indicates some problem with the
configuration in your IDE or in your maven build. Please ensure that your IDE
uses the same settings as the maven build, then such spurious reformatting
should not happen.
2. There is completely unrelated stuff in this change. These things must not
be in this change at all.
- The command execution timeout looks suspicious to me. At least the
timeout for running a command should by default be zero, as it was before.
- There was some refactoring grouping together isolated variables
related to KEX (SessionCountersDetails/SessionKexDetails). Probably not needed
to implement strict KEX.
3. I don't think a custom SSH config property "UseStrictKex" is needed.
4. In fact I don't think any customization flag is needed at all. (Also not
in CoreModuleProperties.)
5. This needs tests against `OpenSSH 9.6`. A container test once that
OpenSSH version is available in some Linux repositories; until then, at least
manual testing. We do have various container tests using older OpenSSH
versions, so that bit of interoperability should already be covered.
As I understand it:
Both parties can unconditionally include the "strict kex" flag in their
KEX_INIT message. "strict kex" is active if and only if both parties announce
it in their initial KEX_INIT proposal. So both parties know whether strict kex
is active once they have received the peer's proposal. If a party concludes
that strict kex is active, it can:
- check that the receive sequence number of the peer's KEX_INIT is 1. If
not, there were earlier messages, and they disconnect.
- It does not seem necessary to actively avoid sending IGNORE/DEBUG
messages before one's own KEX_INIT. Normally that shouldn't happen anyway. If
it is needed, it could be done unconditionally (i.e., irrespective of whether
strict kex will be active).
- as long as `initialKexDone == false`, only allow KEX messages. Reception
of any other message causes the party to disconnect.
- When a party sends its own NEW_KEYS message, it resets the send sequence
number to zero after having encoded the NEW_KEYS message itself. (So basically
where it installs the new keys it also resets the message sequence counter.)
- When a party receives a NEW_KEYS message, it resets the receive sequence
number to zero after having decoded the message. (Same here: where we install
the new keys, we also reset the counter.)
Protocol-wise, this should be a fairly simple change. I would refrain from
any beautification or not strictly needed refactoring; such things can be done
in later changes once the basic modified protocol works. Probably unit tests
would be the bulk of the change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
