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

Conflict between SSH and SFTP? #707

Open
philchristensen opened this issue Nov 3, 2024 · 5 comments
Open

Conflict between SSH and SFTP? #707

philchristensen opened this issue Nov 3, 2024 · 5 comments

Comments

@philchristensen
Copy link

philchristensen commented Nov 3, 2024

I've run into a strange issue while attempting to implement SFTP support on top of a custom SSH interface. I've narrowed it down to this:

async def server(port=8022):
    """
    Create an AsyncSSH server on the requested port.

    :param port: the port to run the SSH daemon on.
    """
    await asyncio.sleep(1)  # fix for auto-reloading
    await asyncssh.create_server(
        lambda: SSHServer(interact),
        "",
        port,
        server_host_keys=["/etc/ssh/ssh_host_ecdsa_key"],
        # sftp_factory=SFTPServer,
        # allow_scp=True
    )
    await asyncio.Future()

If I run as is, my custom shell works great. If I uncomment the sftp_factory, the SFTP function works fine — but then logins to the shell fail with shell request failed on channel 0. From the server side, I see it authenticate the user, but then close the connection immediately after that:

shell-1     | [conn=0] Accepted SSH client connection
shell-1     | [conn=0]   Local address: 172.18.0.5, port 8022
shell-1     | [conn=0]   Peer address: 192.168.65.1, port 58992
shell-1     | [conn=0] Beginning auth for user phil
shell-1     | Validating public key for phil
shell-1     | Validating public key for phil
shell-1     | Validating public key for phil
shell-1     | Validating password for phil
shell-1     | [conn=0] Auth for user phil succeeded
shell-1     | [conn=0, chan=0] New SSH session requested
shell-1     | [conn=0, chan=0]   PTY created
shell-1     | [conn=0, chan=0]   Line editor enabled
shell-1     | [conn=0, chan=0]   Interactive shell requested
shell-1     | [conn=0, chan=0] Closing channel due to connection close

The SFTPServer code is really new to me, so I'm sure it's an issue on my end, but would appreciate a suggestion about where to start.

Thanks in advance,

-Phil

@ronf
Copy link
Owner

ronf commented Nov 4, 2024

If you pass in sftp_factory to enable SFTP and you also want to allow shell/exec sessions, you should specify either process_factory or session_factory as well, to indicate the coroutine to call when one of those is requested.

Alternately, if you want more control over exactly which sessions are allowed and which handler to use on a case-by-case basis, you should leave these arguments unset and instead implement one or more of the callback methods shell_requested(), exec_requested(), and subsystem_requested() in a custom subclass of SSHServer that you pass into asyncssh.create_server().

@philchristensen
Copy link
Author

Thanks for the response. I'm just a little confused, because in the API docs it reads:

session_factory (callable or coroutine) – (optional) A callable or coroutine handler function which takes AsyncSSH stream objects for stdin, stdout, and stderr that will be called each time a new shell, exec, or subsystem other than SFTP is requested by the client. If not specified, sessions are rejected by default unless the session_requested() method is overridden on the SSHServer object returned by server_factory to make this decision.

My SSHServer class returned by the lambda above definitely implements session_requested():

    def session_requested(self) -> PromptToolkitSSHSession:
        """
        Setup a session and associate the Django User object.
        """
        session = PromptToolkitSSHSession(self.interact, enable_cpr=self.enable_cpr)
        session.user = self.user
        return session

Since I need the self.user object to create my session class, I'm not sure how to implement a separate factory.

I also tried adding the callback methods to the SSHServer class (returning True), but they didn't seem to get called.

Thanks again for a great library. I know I'm just a couple steps away from grokking this part.

@ronf
Copy link
Owner

ronf commented Nov 5, 2024

The session_requested() method will only be called when none of session_factory, process_factory, and sftp_factory are specified as arguments. When one of those is specified, the session open function will use those instead of calling your SSHServer's session_requested() method. These factory arguments were added later as an easier-to-use option when you didn't need any logic to make the session handler decision, just like how the asyncssh.connect() and asyncssh.listen() methods were added to make it easier when you didn't need to specify a custom SSHClient or SSHServer class.

If you want to have your own SSHServer class, you can have its session_requested() method return another method in the SSHServer class. This will have access to "self" on the custom SSHServer class, giving it access to whatever extra state you are keeping there. For instance, you might have the following in your

    def handle_session(self, stdin, stdout, stderr):
        # Add session processing here, referencing "self" to get at data in your SSHServer instance

    def session_requested(self):
        # Set up whatever custom state you need

        return self.handle_session

Keep in mind that session_requested() may be called multiple times, though. If you need to support multiple sessions each with unique custom state, you may need to add extra arguments to the handle_session class and use something like functools.partial() to pass those extra arguments in when a session is started.

If you just need access to the username or some other SSH connection state, there are simpler ways to accomplish that. The easiest way is using process_factory and calling get_extra_info() on that when the process starts up. See the example at https://asyncssh.readthedocs.io/en/latest/#simple-server for more details. Specifically, the process handler could look something like:

def handle_client(process: asyncssh.SSHServerProcess) -> None:
    username = process.get_extra_info('username')
    process.stdout.write(f'Welcome to my SSH server, {username}!\n')
    process.exit(0)

Any other per-session state can be initialized after this process handler starts up.

@Menyadar
Copy link

Menyadar commented Nov 7, 2024

@philchristensen:

    def session_requested(self) -> PromptToolkitSSHSession:
        """
        Setup a session and associate the Django User object.
        """
        session = PromptToolkitSSHSession(self.interact, enable_cpr=self.enable_cpr)
        session.user = self.user
        return session

Using PromptToolkitSSHSession can lead to some very obscure bugs, as promt_toolkit doesn't support more than one ssh connection at a time. Check issue #561. That one is for using their ptpython.PythonRepl, but iirc the same happens for any PromptToolkitSSHSession connection. I don't see that issue closed so i assume it is still active (haven't checked, though, as i've abandoned promt_toolkit when i checked their source code 😉)

@philchristensen
Copy link
Author

Thanks folks. Going to try this today.

Good to know about the PromptToolkitSSHSession. TBH this SFTP feature isn't really a hard requirement I just thought it would be cool.

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

No branches or pull requests

3 participants