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
