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