When is client_address None in russh::server::Server::new_client? And what to do when that happens?

[samuela]

I've always found the type

fn new_client(&mut self, client_address: Option<std::net::SocketAddr>) -> Self::Handler {

confusing. Specifically: When is client_address ever None?

At first I took this to be a peculiarity of the API, a sort of this-should-never-happen case. But after exposing my server to traffic in the wild, I've actually witnessed a crash due to client_address being None. So,

First question: under what circumstances is client_address None?

Now, supposing that client_address is None, the question is what to do. I'd like to simply reject all requests and immediately close these sessions. I haven't found a way to express this cleanly yet.

• new_client does not allow returning a Result or Error-y type, so there's no chance to raise the issue at that level.
• Perhaps I could introduce structs MyActualHandler, RejectEverythingHandler and impl russh::server::Handler on both of them. Then I can specify type Handler = Box<dyn russh::server::Handler>;. But this fails due to

error[E0038]: the trait `russh::server::Handler` cannot be made into an object
  --> src/bin/sshenanigans.rs:68:22
   |
68 |   type Handler = Box<dyn russh::server::Handler>;
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^ `russh::server::Handler` cannot be made into an object
   |
   = note: the trait cannot be made into an object because it requires `Self: Sized`
   = note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>

• You might reach for https://crates.io/crates/either, and then impl russh::server::Handler for Either<MyActualHandler, RejectEverythingHandler> (or better yet type parameterized), but that runs into this issue.

Second question: What's the recommended pattern in this case? Can type definitions/constraints be relaxed in such a way as to make this more straightforward?

[samuela]

My current workaround is something like

struct OptionalHandler(Option<ServerHandler>);

#[async_trait]
impl russh::server::Handler for OptionalHandler {
  type Error = <ServerHandler as russh::server::Handler>::Error;

  async fn auth_none(self, username: &str) -> Result<(Self, Auth), Self::Error> {
    match self.0 {
      Some(handler) => handler
        .auth_none(username)
        .await
        .map(|(handler, auth)| (Self(Some(handler)), auth)),
      None => anyhow::bail!("no client_address"),
    }
  }

  // like 22 more of these method definitions.....
}

I know that macros could DRY some of this templating, but still all the complexity feels gratuitous.

[Eugeny]

client_addr is null when TcpStream::peer_addr fails: https://docs.rs/tokio/latest/tokio/net/struct.TcpStream.html#method.peer_addr Don't ask me what the conditions are for that :) You could just store client_addr in your handle and reject auth if it's is_none().

…

________________________________ From: Samuel Ainsworth ***@***.***> Sent: Wednesday, January 17, 2024 4:55:09 AM To: warp-tech/russh ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [warp-tech/russh] When is `client_address` None in `russh::server::Server::new_client`? And what to do when that happens? (Issue #226) My current workaround is something like struct OptionalHandler(Option<ServerHandler>); #[async_trait] impl russh::server::Handler for OptionalHandler { type Error = <ServerHandler as russh::server::Handler>::Error; async fn auth_none(self, username: &str) -> Result<(Self, Auth), Self::Error> { match self.0 { Some(handler) => handler .auth_none(username) .await .map(|(handler, auth)| (Self(Some(handler)), auth)), None => anyhow::bail!("no client_address"), } } // like 22 more of these method definitions..... } I know that macros could DRY some of this templating, but still all the complexity feels gratuitous. — Reply to this email directly, view it on GitHub<#226 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AABHNRBL4YL6T4HYXEV3MFTYO5DR3AVCNFSM6AAAAABB526K52VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJUHA4TMMZYGM>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[samuela]

Gotcha, ok looks like this traces all the way back to https://doc.rust-lang.org/nightly/std/net/struct.TcpStream.html#method.peer_addr. From there it goes to (platform dependent) https://stdrs.dev/nightly/x86_64-unknown-linux-gnu/std/sys_common/net/struct.TcpStream.html#method.peer_addr. On linux, that goes to https://stdrs.dev/nightly/x86_64-unknown-linux-gnu/libc/fn.getpeername.html.

You could just store client_addr in your handle and reject auth if it's is_none().

I thought about that, but then I realized I'd have to put some boilerplate code in every single handler method. I was hoping there might be a cleaner way?

[Eugeny]

It's enough to add it to auth_* methods as other methods can't be called until the user is authenticated.

[samuela]

Ah I see. I was not aware of that invariance. Perhaps we could encode it at the type level? I guess this brings us back to your point in #188 (comment)