Keyboard interactive auth

[joshbenz]

So I had some time to work on this. I think I am in little over my head here, but maybe you can point me in the right direction. I may just need to stare at the codebase longer, but this is what I have so far.

So it sends a Keyboard-Interactive request and receives the AuthInfoRequest that contains the challenges. The user is given a trait to implement in order to fulfill those challenges/prompts. The results of are then sent back to the Auth request and written to the buffer. I think I am definitely missing something important here because after that the session and handle gets dropped. What should happen next is that the server send another AuthInfoRequest, AuthSuccess or AuthFailure. It's almost like client_encrypted_read needs to be called again.

Another thing is that USERAUTH_PK_OK and USERAUTH_INFO_REQUEST share the same number. For the sake of testing just keyboard interactive, I have it in an else-if before PK_OK. At some point I need to differentiate between the two. I noticed that auth_request.current can hold a CurrentRequest that has a KeyboardInteractive variant, but I am unsure of where that should get set.

I appreciate any direction you can give me. I think I don't yet have a clear understanding of the over flow of the code yet.

[Eugeny]

Hey, great work! Random disconnects are usually caused by errors inside client_read_encrypted, you can apply this to make them show up in the debug log:

diff --git a/russh/src/client/mod.rs b/russh/src/client/mod.rs
index b4b62d5..e5ad07a 100644
--- a/russh/src/client/mod.rs
+++ b/russh/src/client/mod.rs
@@ -77,6 +77,7 @@
 
 use std::cell::RefCell;
 use std::collections::HashMap;
+use std::fmt::Debug;
 use std::pin::Pin;
 use std::sync::Arc;
 
@@ -1139,7 +1140,10 @@ async fn reply<H: Handler>(
             session.common.kex = Some(kex);
             Ok((handler, session))
         }
-        None => session.client_read_encrypted(handler, buf).await,
+        None => session.client_read_encrypted(handler, buf).await.map_err(|err| {
+            debug!("client_read_encrypted error: {:?}", err);
+            err
+        }),
     }
 }
@@ -1197,7 +1201,7 @@ impl Default for Config {
 /// Note: this is an `async_trait`. Click `[source]` on the right to see actual async function definitions.
 #[async_trait]
 pub trait Handler: Sized + Send {
-    type Error: From<crate::Error> + Send;
+    type Error: From<crate::Error> + Debug + Send;
 
     /// Called when the server sends us an authentication banner. This
     /// is usually meant to be shown to the user, see

I think that the current API is very limiting though. While passing a callback is the way it's done in most libraries, here it would prevent the user from doing anything async during authentication, as well as retain ownership of the handler for the whole duration. I think something like this could work better:

enum KeyboardInteractiveAuthResponse {
    Success,
    Failure,
    InfoRequest {
        prompts: Vec<String>
    }
}

pub async fn authenticate_keyboard_interactive_start(
    &mut self,
    user: U,
    ...
) -> Result<KeyboardInteractiveAuthResponse, ..> {
}

pub async fn authenticate_keyboard_interactive_respond(
    &mut self,
    responses: Vec<String>
    ...
) -> Result<KeyboardInteractiveAuthResponse, ..> {
}

It allows the user to bail out of k-i authentication, keep ownership of the handler, do async stuff and more closely resembles the protocol.

What do you think?

[joshbenz]

Thanks for the feedback and tips. Yes I definitely see what you mean. Especially since the purpose of keyboard-interactive is expand what authentication methods are possible. Your approach definitely seems simpler to implement and I think it fits with russh better as well. We will just need to inform the user (in the docs or something) that they should use authenticate_keyboard_interactive_respond in some sort of loop since the server can respond with InfoRequest as many times as it wants. I'll have another go at it with that approach next chance I get!

[joshbenz]

I think I am still missing something important. After writing the responses, the next tokio::select gives me SELECT ERROR IO(Custom { kind: UnexpectedEof, error: "early eof" }). Particularly it happens at cipher::read at steam.read_exact, which I believe is reading an ID? I'm not really sure what I am missing that would cause that.

[Eugeny]

Can you check the ssh server debug output? You can start a separate instance in debug mode with /usr/sbin/sshd -d -o Port=3333 -f /etc/ssh/sshd_config

[joshbenz]

The issue turned out to be my test client not calling authenticte_keyboard_interactive_respond() enough times. Oops

[joshbenz]

closes #147

[Eugeny]

Looks great, thank you so much!

[Eugeny]

@all-contributors please add @joshbenz for code

[allcontributors]

@Eugeny

I've put up a pull request to add @joshbenz! 🎉

[joshbenz]

Thanks for working with me through the process and letting me contribute! I really appreciate it. And thank you for taking on this fork of Thrussh. Hopefully I can contribute more in the future.