[GitHub] [guacamole-server] pjd commented on a change in pull request #304: GUACAMOLE-141: Implement full support for keyboard-interactive authentication method.

[GitBox <gi...@apache.org>]

pjd commented on a change in pull request #304:
URL: https://github.com/apache/guacamole-server/pull/304#discussion_r507012810



##########
File path: src/common-ssh/ssh.c
##########
@@ -358,68 +350,63 @@ static int guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
 
     }
 
-    /* Attempt authentication with username + password. */
-    if (user->password == NULL && common_session->credential_handler)
-            user->password = common_session->credential_handler(client, "Password: ");
-    
-    /* Authenticate with password, if provided */
-    if (user->password != NULL) {
+    /* Check if keyboard-interactive auth is supported on the server */
+    if (strstr(user_authlist, "keyboard-interactive") != NULL &&
+        common_session->credential_handler != NULL) {

Review comment:
       Hmm, good point. Unfortunately with keyboard-interactive it is hard to tell what the server will be asking for.
   In majority of cases yes, the server will ask for a password.
   
   In our product for example, we can ask the user for a login reason in the first step. For this we enable echo and that would be very unfortunate to just paste the password there, making it visible.
   
   Let me propose few solutions:
   
   1. Change the order and check if the server supports 'password' method first, before checking for 'keyboard-interactive'. This way we cannot go wrong, as this method expects just password. The problem here is that it won't address your use case, as any type of MFA requires keyboard-interactive (well, not really always, but let's not complicate it any further for now).
   
   2. Add a way for the user to define a prompt that he wants to trigger password insertion. For example GUAC_PROMPT. If we see this prompt (or prompt that matches this regexp) we paste the password. Not very user-friendly.
   
   3. I think my favorite one. If the first prompt in the first USERAUTH_INFO_REQUEST request has echo disabled, insert the password. Try to do it only once - if the password was pasted into wrong prompt server will hopefully ask again and we won't retry with this password.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org