tomaswolf commented on issue #461:
URL: https://github.com/apache/mina-sshd/issues/461#issuecomment-2106274419

   Sorry for the long message, but when I looked at this it took me 
surprisingly long to find my way through the code. I must say I find the 
implementation of heartbeats _very_ confusing.
   
   **TL;DR**: An immediate work-around for the reporter (@d9e7381f ) might be 
to set `CoreModuleProperties.HEARTBEAT_REPLY_WAIT` to zero. But we should 
change the implementation, too.
   
   **Long version**:
   
   First, Apache MINA SSHD resets the session idle timeout whenever a message 
is received or _is written_. So even sending a heartbeat with `wantReply == 
false` will reset the idle timeout.
   
   So `wantReply == true` makes sense only if one wants to implement the 
OpenSSH behaviour of `ClientAliveCountMax` or `ServerAliveCountMax`: terminate 
the connection once that many heartbeats did _not_ get a reply (yet). But for 
that one doesn't have to wait synchronously for the reply; we have 
[asynchronous global 
requests](https://github.com/apache/mina-sshd/blob/master/docs/technical/global_requests.md#api)
 since version 2.9.0.
   
   Second, _heartbeats are implemented twice_: once in 
`AbstractConnectionService` and then again in `ClientConnectionService`, and 
_there are two sets of configuration properties_.  The server side gets the 
behaviour of `AbstractConnectionService`.
   
   `AbstractConnectionService` is configured with properties 
`CommonModuleProperties.SESSION_HEARTBEAT_TYPE` (default "NONE", i.e., off) and 
`CommonModuleProperties.SESSION_HEARTBEAT_INTERVAL` (default 0, i.e. disabled). 
To enable, one needs to set an interval > 0, and type "IGNORE" or "RESERVED". 
If the heartbeat type is "IGNORE", it'll send a fire-and-forget SSH_MSG_IGNORE. 
If the heartbeat type is "RESERVED", it'll invoke a user handler that is 
supposed to send the heartbeat, and it might choose to do so with a global or 
channel request with `want-reply == true`.
   
   Heartbeats in `ClientConnectionService` have additional configuration 
properties `CoreModuleProperties.HEARTBEAT_INTERVAL` (default zero, i.e. off), 
`CoreModuleProperties.HEARTBEAT_REQUEST` (default "keepal...@sshd.apache.org"), 
and `CoreModuleProperties.HEARTBEAT_REPLY_WAIT` (default **5 minutes**). To 
enable this kind of heartbeat, set an interval > 0. It'll send a 
SSH_MSG_GLOBAL_REQUEST for "keepal...@sshd.apache.org", and because 
HEARTBEAT_REPLY_WAIT > 0, will send it with `wantReply == true` and _will wait 
synchronously for five minutes_ for a reply to arrive. If no reply arrives in 
time, kill the SSH session. (If HEARTBEAT_REQUEST_WAIT is <= 0, the request 
will be sent with `wantReply == false`, i.e., fire-and-forget.) And to 
complicate matters: if this "global request heartbeat" is off 
(HEARTBEAT_INTERVAL <= 0), the above behavior from `AbstractConnectionService` 
kicks in (which is by default disabled).
   
   Plus there's an additional quirk: `AbstractConnectionService` does not send 
heartbeats when a key exchange is ongoing. That was done for 
[SSHD-1059](https://issues.apache.org/jira/browse/SSHD-1059), and since the 
"strict KEX" extension ("Terrapin" mitigation) it's crucial because there must 
not be any SSH_MSG_IGNORE messges during the initial key exchange. The 
mechanism in `ClientConnectionService` has no such provision, but a 
SSH_MSG_GLOBAL_REQUEST message will be delayed anyway until an ongoing key 
exchange is over.
   
   This is a mess. There should be only one set of properties governing 
heartbeats.
   
   The two separate mechanisms seem to have existed for a long time, and 
changing that now would be an API break.
   
   The 5 minutes default value comes from commit 14bbd54a5, which was about 
[SSHD-1020](https://issues.apache.org/jira/browse/SSHD-1020). Reading that 
discussion the issue seems to have been a low-level network I/O read timeout 
(NIO2_READ_TIMEOUT) of ~10 min, and HEARTBEAT_REPLY_WAIT = 0. So even though 
the client sent keep-alives every 90 sec, and the SSH idle timeout got reset, 
the session got killed because nothing was received for 10min because the 
server never sent any reply to these heartbeats because none was asked for. The 
solution was to set NIO2_READ_TIMEOUT = 0 (no timeout for low-level I/O reads) 
and that huge HEARTBEAT_REPLY_WAIT.
   
   An immediate work-around for the reporter might be to set 
`CoreModuleProperties.HEARTBEAT_REPLY_WAIT` to zero. That should work if there 
is no low-level read timeout (NIO2_READ_TIMEOUT = 0).
   
   But we should re-think this mechanism anyway. I propose to change this to 
follow the path taken by OpenSSH:
   
   * Ignore `CoreModuleProperties.HEARTBEAT_REPLY_WAIT` (and deprecate it now).
   * Introduce a `CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX`: an integer, as 
in OpenSSH's `ServerAliveCountMax`. I.e., maximum number of heartbeats to send 
without having gotten a reply.
   
   If `CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX` <= 0, send heartbeats with 
`wantReply == false`.
   
   If > 0, increment a counter whenever a heartbeat is to be sent. If the 
counter is then > HEARTBEAT_NO_REPLY_MAX, throw an exception (killing the 
session). Otherwise send the heartbeat request (as an [asynchronous global 
request](https://github.com/apache/mina-sshd/blob/master/docs/technical/global_requests.md#api))
 with `wantReply == true` and a handler (called when the reply is received) 
that resets the counter to zero.
   
   @gnodet : does that sound reasonable to you?


-- 
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

Reply via email to