Update relevant channel request callbacks to return a bool

[belak]

This is a breaking change (as it changes a trait), but it tweaks channel request callbacks to return a bool rather than requiring the user to manually call session.channel_success or session.channel_failure. Also, IIRC, the protocol docs specify that the request channel should continue to be serviced even when a session is started, so it makes sense to require users to spin off a background task and return the status.

Alternatively this could be done as a non-breaking change by making the server implementation call session.channel_failure after a channel request is handled.

I do understand there are valid reasons to deny this, but it seemed like an easy place for a user to make a mistake, and I wanted to see how hard this would be to change.

This has the added advantage of changing the defaults of a number of request callbacks to more-secure defaults (deny), and makes it impossible for a user to miss responding to callbacks which require responses. Even if this PR is not accepted, that change should probably be implemented - I would be happy to submit that separately if you'd prefer.

Note that this does not handle sending responses for all requests, only channel requests listed in RFC4254 as having a "want reply" param rather than just "false", even though it may be more correct to respond to malformed requests which have improperly set that byte to "true" even though the RFC specifies "false".

EDIT: with the combination of the channel message stream and the handlers, this should continue to work as expected, at least with sftp, but that's only because it uses .into_stream() which only handles data and doesn't require a reply. I'm not certain the "correct" way to handle this - the channel is definitely useful because it allows you to get an impl AsyncRead / impl AsyncWrite, but it definitely complicates this.

If you have any advice I'd love to hear it.

[Eugeny]

While I think it's generally better this way, it breaks scenarios where consumer wants to wait for something to return a reply. Specifically, it requires them to fully pause Handler processing until the reply is available.

In my case, it's the SSH proxying in warpgate where it needs to first receive the REQUEST_SUCCESS/REQUEST_FAILURE from the other side before replying.

The spec specifically allows for replying later as long as it's still in the correct order.

A more flexible (albeit less clean) solution would be to pass the Handler a oneshot::Sender<bool>, put the corresponding receiver in a queue and have a background task that listens to them in order and sends the replies into the session. And if the sender gets dropped you can assume the request was not handled and reply with a default (REQUEST_FAILURE)

[belak]

I think I follow what you're saying - is there any other way we could work towards something which would automatically respond to any requests if the Handler doesn't do anything?

The two other options I can think of are:

1. Change the default callbacks to respond to requests with a failure - they should need an implementation to handle them correctly.
2. Move to a system where ChannelOpenSession would allow the user to return some sort of ChannelHandler which would be stored by the server and automatically dispatch channel requests to it rather than having to build an abstraction themselves.

Would you be open to either of those?

[Eugeny]

something which would automatically respond to any requests if the Handler doesn't do anything?

The oneshot concept should allow this, or not? On the Receiver side, you would be able to distinguish between

• Handler replies true
• Handler replies false
• Handler drops Sender without replying

[qsantos]

While I think it's generally better this way, it breaks scenarios where consumer wants to wait for something to return a reply. Specifically, it requires them to fully pause Handler processing until the reply is available.

Since the handler methods are async, can’t this be achieved simply enough by awaiting?

[Eugeny]

Yes, but that would stop the session event loop while it's awaiting a decision. Besides, the protocol does not require a success/failure reply immediately after a request, it may be interleaved with other messages