[Design] Some proposals with the `Handler` traits.

[lowlevl]

Hi, (thanks for the merge of my earlier PR :)) While using and working on the project, I figured some improvements for the Handler traits.

• I noticed the owned pattern with a tuple return for the Handlers traits which I find to be kind of an anti pattern, since having ownership of self and session (when there) is rarely useful as opposed to having &mut references, and allow for moving them outside of scope and never returning, which in turn breaks the server's loop, couldn't we just use &mut references ?

• There are multiple ways of handling Channel events, either via the Channel::wait method, or via the Handler's methods, which is confusing and allows for blocking the server loop if we, for example, execute a command and pipe the I/O in the Handler::exec_request method, shouldn't we remove those methods in favor of the Channel struct ?

• The server-side authentication mechanism is a bit hard to work with when we need to keep the data from the auth attempt, since we need to wrap them in Options for example to be able to store the public-key. This could be fixed by having another trait like so:

trait AuthProvider {
    type Ok = ();
    type Error = ();

    fn none(&self, user: &str) -> Result<Self::Ok, Self::Err>;

    fn password(&self, user: &str, password: &str) -> Result<Self::Ok, Self::Err>;

    fn publickey(&self, user: &str, pkey: &key::PublicKey) -> Result<Self::Ok, Self::Err>;

    /* ... other methods */
}

and having Handler's methods like so:

trait Handler {
    type Auth: AuthProvider;

    async fn auth_succeeded(&mut self, session: Session, auth: &Self::Auth::Ok) -> Result<(), Self::Error>;

    async fn channel_open_session(&mut self, channel: Channel<Msg>, session: Session, auth: &Self::Auth::Ok) -> Result<bool, Self::Error>;

    /* ... etc */
}

• The channel_open_* methods could be condensed with an enum parameters describing the request to only keep channel_open and channel_close methods in the Handler.

Those proposals are things that went through my head while diving into the codebase, but are not tested nor necessarily good, but I wanted to ear what you think about those ideas :)

[Eugeny]

There are multiple ways of handling Channel events, either via the Channel::wait method, or via the Handler's methods, which is confusing and allows for blocking the server loop if we, for example, execute a command and pipe the I/O in the Handler::exec_request method, shouldn't we remove those methods in favor of the Channel struct ?

Agreed, these can go.

The server-side authentication mechanism is a bit hard to work with when we need to keep the data from the auth attempt, since we need to wrap them in Options for example to be able to store the public-key. This could be fixed by having another trait like so:

I feel like this imposes unnecessary "implementation details" on the Handler trait and I dislike the idea of having to pass auth to every method. A possible solution could be to split Handler into AuthHandler and SessionHandler like this:

trait AuthHandler<SH: SessionHandler> {    
    type Ok = ();
    type Error = ();

    fn none(&self, user: &str) -> Result<Self::Ok, Self::Err>;
    fn password(&self, user: &str, password: &str) -> Result<Self::Ok, Self::Err>;
    fn publickey(&self, user: &str, pkey: &key::PublicKey) -> Result<Self::Ok, Self::Err>;

    // Upgrade self to a SessionHandler
    fn auth_succeeded(self, auth: Self::Ok) -> Result<SH, Self::Error>;
}

trait Handler {
    // self.auth could have been stored here by the implementation
    async fn channel_open_session(&mut self, channel: Channel<Msg>, session: Session) -> Result<bool, Self::Error>;

    /* ... etc */
}

(this is just a rough idea, I haven't thought about it too much)

I noticed the owned pattern with a tuple return for the Handlers traits which I find to be kind of an anti pattern, since having ownership of self and session (when there) is rarely useful as opposed to having &mut references, and allow for moving them outside of scope and never returning, which in turn breaks the server's loop, couldn't we just use &mut references ?

This likely relates to how the Handler trait was implemented in thrussh (non-async returning Futures directly). Feel free to give it a try and if &mut self doesn't cause issues with async_trait, I'm happy to merge!

[lowlevl]

Hi there, about the AuthHandler, I'm struggling to move around the state of the current code because it's all based on the Handler trait, also I think that having 3 traits to implement for the server is a bit much, I think AuthHandler could be merged with the Server trait. What do you think ?

Also while diving into the code, I noticed all the parsing and serialization of the SSH packets is done manually, I think this project could benefit from defining packets with binrw for clarity and sturdiness. It's a big rewrite but this would mean the packets reading-writing is the same for clients and servers which is a big plus IMHO ! I might try to see what can be done with this.

[Eugeny]

I think having 3 traits per se is not a bad thing as it helps making illegal states unrepresentable. On a rough check, the AuthHandler/Handler state maps nicely onto Encrypted::state which follows the same separation between authorized and unauthorized sessions.

I'm open to the idea of actually extracting protocol parsing into its own crate for others to reuse!

[lowlevl]

So, I ended up implementing the SSH packet and message structures in https://docs.rs/ssh-packet using binrw, this is currently untested and just from reading the RFCs, but I think russh could benefit from this :)

The idea being to implement Cipher for russh::server::CipherPair and it should be all set for decryting and encrypting Packets using Packet::decrypt and Packet::encrypt !

[ricott1]

I tried to implement the first point of this proposal in https://github.com/warp-tech/russh/pull/247