Reply with CHANNEL_CLOSE in server handler per RFC 4254 - #675
Conversation
The server-side CHANNEL_CLOSE handler was removing the channel without sending CHANNEL_CLOSE back. RFC 4254 Section 5.3 requires that upon receiving CHANNEL_CLOSE, a party MUST reply with CHANNEL_CLOSE unless already sent. Use enc.close() to match the client-side handler.
| } | ||
| self.channels.remove(&channel_num); | ||
| debug!("handler.channel_close {channel_num:?}"); | ||
| handler.channel_close(channel_num, self).await |
There was a problem hiding this comment.
This calls into user code, the RFC states:
The channel is considered closed for a party when it has both sent and received SSH_MSG_CHANNEL_CLOSE, and the party may then reuse the channel number.
This means currently the channel is still open when handler.channel_close is called, but with this change the channel is going to be closed at that point.
In current russh it's the handler's responsibility to send SSH_MSG_CHANNEL_CLOSE.
There was a problem hiding this comment.
Thanks, you're right. enc.close() removes the channel before the handler runs, which would break handlers that do final work in channel_close. I also realize enc.close() has pending-data logic meant for locally-initiated closes that doesn't apply here.
I'm thinking the fix is to move the CLOSE reply after the handler call and send the byte directly rather than going through enc.close(). Does that track with how you'd expect it to work?
Problem
When the server receives
CHANNEL_CLOSEfrom a client, it removes the channel fromenc.channelsbut never sendsCHANNEL_CLOSEback on the wire.RFC 4254 Section 5.3 requires:
The client-side
CHANNEL_CLOSEhandler already does this correctly viaenc.close(). The server-side handler was callingenc.channels.remove()directly, skipping the reply. This also matches OpenSSH's behavior, wherechan_rcvd_oclose()always ensuresCHANNEL_CLOSEis sent back before freeing the channel.Changes
One-line change in
server/encrypted.rs: replaceenc.channels.remove(&channel_num)withenc.close(channel_num)?, matching the client-side handler.enc.close()sendsCHANNEL_CLOSEon the wire (or defers it if there is pending data) and removes the channel.