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

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

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



##########
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:
       I see one potential issue with re-arranging things in this way: what if the password has already been provided at this point?  It seems like, the way things have been re-arranged, if the connection provides the password, either as a static value, or using the `${GUAC_PASSWORD}` token, the system will still check if the server supports keyboard-interactive authentication and will still prompt the user for a password?
   
   Whatever changes are made here still need to factor in that the password (or initial password, in the case of MFA) may be provided ahead of time, and the user should not be prompted. For example, in my day job I work in an environment that leverages AD + Azure MFA, so the password portion of the login would generally be passed through using the `${GUAC_PASSWORD}` token that was used to authenticate me to Guacamole, and then a second factor may be required in the form of a token value from an authenticator application or tapping the "Accept" button on the app on my phone.




----------------------------------------------------------------
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