SSHServer.server_requested uses returned value for accept_handler?

[joshuabroekhuijsen]

I see that in Sep 2023 a feature was added to allow port forwards to have an accept_handler so per-origin forwards could be decided. Great feature. I'm confused as to how it's implemented in the "_process_tcpip_forward_global_request" flow.

_finish_port_forward has listener = self._owner.server_requested(listen_host, listen_port), and then if that listener is callable (or an awaitable that yields a callable), passes it as the accept_handler to forward_local_port:

                listener = await self.forward_local_port(
                    listen_host, listen_port,
                    listen_host, listen_port, listener)

This doesn't seem documented anywhere I can see. Docstring and type hinting for server_requested both indicate the return should be True, False, SSHListener, or an awaitable yielding one of those, but there even appears to be a test case checking that this works in test_forward:

class _TCPAcceptHandlerServer(Server):
    """Server for testing forwarding accept handler"""

    async def server_requested(self, listen_host, listen_port):
        """Handle a request to create a new socket listener"""

        def accept_handler(_orig_host: str, _orig_port: int) -> bool:
            return True

        return accept_handler

I'm digging into some stuff for other code I'm writing and this tripped me up: not a giant issue, but I thought some documentation might help clear up this slightly-obtuse path to declare an accept_handler. As a secondary issue, if you return a callable then it will not be used as the sshlistener (this would be a weird case where somebody, I guess, subclassed SSHListener and added a call method, hoping to do both?).

[ronf]

Thanks for the report! The documentation was updated to add accept_handler as an explicit argument in the SSHClientConnection.forward_local_port() and SSHClientConnection.forward_local_port_to_path() methods, but not for returning an accept handler from within SSHServer.server_requested(). I'll look into fixing this.

To provide a bit more detail, the server_requested() method is called each time a client makes a request to open a listening port on the server side and begin passing arriving connections over the SSH connection, forwarding them to some local host and port. In AsyncSSH, this would be done with a call on the client side to forward_remote_port(). It would also apply when the client called create_server() to set up a listener but then handle the traffic itself.

The challenge here is that the server_requested() function can only accept or reject the request to open a listener based on the listen_host and listen_port which the client requests. Either you reject the request and don't open a listener at all, or you accept the request and allow connections from arbitrary IP addresses and ports to connect to the listener which gets set up. If a server wanted to allow connections from some IP/port combinations but not from others, there's no way to do that from within server_requested(), as it finishes running as soon as the listener is set up and doesn't get called later when connections begin to arrive. That's the purpose of the accept handler. If you provide an accept handler, it will be called with the client host and port and can then return True or False depending on whether it wants to allow the connection from that host & port. This handler will be called once for each new connection which connects to the listening port. If server_requested() returns True, it will accept all connections to the listening host & port regardless of the inbound connection's host & port, forwarding all the connections over the SSH tunnel back to the client.

This is similar to what happens when a client calls forward_local_port(), accepting incoming connections on the client side and forwarding them over the SSH connection to become outbound connections from the SSH server. Each time a connection comes in to the local listening socket, the accept handler is called (if set) to decide whether to accept the connection or not. If it returns True, it will forward the session over the SSH connection, requesting an outbound connection for each session to the dest_host and dest_port passed into forward_local_port().

You're right that choosing to treat a returned callable as an accept handler does overload things a bit, but you never really need to support both at once. If you provide your own SSHListener, you would be responsible for handling new inbound connections yourself and could do the checking on each to decide whether to allow the connection not. Alternately, you could have a function which calls into something like forward_local_port() to get the SSHListener to return, passing whatever you want as the accept_handler argument to that. Note that the SSHListener instance returned is NOT a callable, so there's no confusion there.