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

[SSHD-1262] Eliminate buffering in TCP/IP port forwarding #223

Merged
merged 1 commit into from May 18, 2022

Conversation

tomaswolf
Copy link
Member

TCP/IP port forwarding started reading on the local port before the SSH
tunnel was established, i.e., before SSH_MSG_CHANNEL_OPEN_CONFIRMATION
was received. It buffered data until then, and then tried to flush the
buffer once the confirmation was received.

This didn't work if the channel window was exhausted while that buffered
data was to be written, and in particular it failed when the channel was
opened with an initial window size of zero.

Change the whole forwarding setup: first set up the SSH tunnel, only
then start reading from the local port. That way, no data needs to be
buffered at all.

Use a ChannelAsyncOutputStream to forward data read into the SSH tunnel;
this nicely adapts to the channel window. To avoid concurrent writes on
this channel, suspend reading from the local port before writing, and
resume it only after the data was written. This automatically throttles
reading to the speed of writing, which is what one wants in this case.

Implement IOSession.suspendRead() and IOSession.resumeRead() also for
the MINA and Netty back-ends. Fix the implementation for the NIO2
back-end: the former implementation of resumeRead() could cause
concurrent reads if the channel indeed did an asynchronous read, and if
the channel did a synchronous read it led to deep completion handler
chains on the stack, and could delay the next read on some other
session's completion handler. Resolve this for our use-case by
scheduling an asynchronous read explicitly if necessary, and letting
the normal read completion handler do the read if possible.

Do this also for the TcpipServerChannel: it started reading from the
connected port before it even sent SSH_MSG_CHANNEL_OPEN_CONFIRMATION.
Remove the BufferedIoOutputStream and use a ChannelAsyncOutputStream
always.

Include technical documentation on TCP/IP port forwarding.

@lgoldstein
Copy link
Contributor

Seems OK to me - I like your approach - much cleaner. I do have one concern - I was not able to determine whether pending peer packets are flushed before EOF is sent and the channel is closed.

@tomaswolf
Copy link
Member Author

I was not able to determine whether pending peer packets are flushed before EOF is sent and the channel is closed.

They should be. On a graceful close, the channel first closes the ChannelAsyncOutputStream gracefully. The stream's close future is fulfilled only once the last write has been completed. Sending the EOF happens in a future listener on the stream's close future, so it goes out after the stream is closed, and it's done not via the stream but directly via the SSH session.

See TcpipServerChannel, lines 208ff and TcpipClientChannel, lines 157ff. I've added comments at both places.

@tomaswolf
Copy link
Member Author

BTW: apparently this also fixes SSHD-1256.

@lgoldstein
Copy link
Contributor

I was not able to determine whether pending peer packets are flushed before EOF is sent and the channel is closed.

They should be. On a graceful close, the channel first closes the ChannelAsyncOutputStream gracefully. The stream's close future is fulfilled only once the last write has been completed. Sending the EOF happens in a future listener on the stream's close future, so it goes out after the stream is closed, and it's done not via the stream but directly via the SSH session.

Great

TCP/IP port forwarding started reading on the local port before the SSH
tunnel was established, i.e., before SSH_MSG_CHANNEL_OPEN_CONFIRMATION
was received. It buffered data until then, and then tried to flush the
buffer once the confirmation was received.

This didn't work if the channel window was exhausted while that buffered
data was to be written, and in particular it failed when the channel was
opened with an initial window size of zero.

Change the whole forwarding setup: first set up the SSH tunnel, only
then start reading from the local port. That way, no data needs to be
buffered at all.

Use a ChannelAsyncOutputStream to forward data read into the SSH tunnel;
this nicely adapts to the channel window. To avoid concurrent writes on
this channel, suspend reading from the local port before writing, and
resume it only after the data was written. This automatically throttles
reading to the speed of writing, which is what one wants in this case.

Implement IOSession.suspendRead() and IOSession.resumeRead() also for
the MINA and Netty back-ends. Fix the implementation for the NIO2
back-end: the former implementation of resumeRead() could cause
concurrent reads if the channel indeed did an asynchronous read, and if
the channel did a synchronous read it led to deep completion handler
chains on the stack, and could delay the next read on some other
session's completion handler. Resolve this for our use-case by
scheduling an asynchronous read explicitly if necessary, and letting
the normal read completion handler do the read if possible.

Do this also for the TcpipServerChannel: it started reading from the
connected port before it even sent SSH_MSG_CHANNEL_OPEN_CONFIRMATION.
Remove the BufferedIoOutputStream and use a ChannelAsyncOutputStream
always.

Run the forwarding writes through ThreadUtils.runAsInternal(): the
write might otherwise block if a KEX was on-going, but we should never
block NIO threads. With ThreadUtils.runAsInternal(), the write may
enqueue a packet, but the write future will be fulfilled only once this
enqueued packet was flushed when KEX is done, so we won't read from the
port until then and thus don't run the risk that a fast producer puts
thousands of packets onto the internal queue during KEX.

Fix a bug in AbstractChannel that became apparent with this change: when
a peer disconnects before having sent SSH_MSG_OPEN_CONFIRMATION for a
channel, that channel cannot be closed gracefully because the peer's id
for it is unknown.

Include technical documentation on TCP/IP port forwarding.
@tomaswolf tomaswolf merged commit b91a424 into apache:master May 18, 2022
@tomaswolf tomaswolf deleted the sshd-1262 branch June 7, 2022 07:07
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.

None yet

2 participants