Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update relevant channel request callbacks to return a bool #348

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

belak
Copy link
Contributor

@belak belak commented Sep 19, 2024

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.

This is a breaking change, 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`.

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.

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".
@belak belak force-pushed the belak/simpler-channel-replies branch from 4242b34 to 144f58a Compare September 19, 2024 15:21
@Eugeny
Copy link
Owner

Eugeny commented Sep 22, 2024

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
Copy link
Contributor Author

belak commented Sep 22, 2024

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
Copy link
Owner

Eugeny commented Sep 23, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants