You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2020/10/17 09:21:23 UTC
[GitHub] [guacamole-server] pjd opened a new pull request #304: GUACAMOLE-141: Implement full support for keyboard-interactive authentication method.
pjd opened a new pull request #304:
URL: https://github.com/apache/guacamole-server/pull/304
The support includes:
- Multiple prompts and responses within a single SSH_MSG_USERAUTH_INFO_REQUEST.
- Multiple SSH_MSG_USERAUTH_INFO_REQUEST / SSH_MSG_USERAUTH_INFO_RESPONSE
exchanges.
- Echo-enabled prompts.
This change should allow guacamole to authenticate guacamole against SSH server that require multifactor authentication.
----------------------------------------------------------------
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
[GitHub] [guacamole-server] pjd commented on a change in pull request #304: GUACAMOLE-141: Implement full support for keyboard-interactive authentication method.
Posted by GitBox <gi...@apache.org>.
pjd commented on a change in pull request #304:
URL: https://github.com/apache/guacamole-server/pull/304#discussion_r507277906
##########
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:
The idea behind 1 was to cover simple cases - just the password. As I've mentioned it won't work for MFA.
IMHO any attempt to make it more configurable would make it hard on the administrators. Also there is no need for the SSH server to offer the same authentication methods to all the users or to offer them the same prompts in the exact same order. If someone wants to implement more precise configuration then he should go ahead, but I don't think it collides with my work here, it's just another step.
After giving it some additional thought I implemented 1&3: changing order and checking 'password' first will cover the easy cases where there is only a password. 3 will cover majority of the remaining cases where password is expected first.
Please note, which is important, that when we try our password with the wrong prompt, the user will still be able to log in, as the server will retry his request and we won't use this password ever again.
I think this is a huge step forward in keyboard-interactive support.
----------------------------------------------------------------
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
[GitHub] [guacamole-server] necouchman commented on a change in pull request #304: GUACAMOLE-141: Implement full support for keyboard-interactive authentication method.
Posted by 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
[GitHub] [guacamole-server] necouchman commented on a change in pull request #304: GUACAMOLE-141: Implement full support for keyboard-interactive authentication method.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #304:
URL: https://github.com/apache/guacamole-server/pull/304#discussion_r507368470
##########
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:
> IMHO any attempt to make it more configurable would make it hard on the administrators.
Yes, you're probably right about that.
> Also there is no need for the SSH server to offer the same authentication methods to all the users or to offer them the same prompts in the exact same order. If someone wants to implement more precise configuration then he should go ahead, but I don't think it collides with my work here, it's just another step.
My point here is just that the behavior of Guacamole as a client should 1) match the vast majority of use-cases, and 2) should be deterministic. For example, I don't think that using the "echo" value of the prompt sent back by the server is good way to determine whether or not the configured value should be tried or not. This ends up being just guessing about what we think might possibly work, and not a solid determination of what the server is going to ask for.
> After giving it some additional thought I implemented 1&3: changing order and checking 'password' first will cover the easy cases where there is only a password. 3 will cover majority of the remaining cases where password is expected first.
I agree that switching back to checking for password, first, will make sure that things work for many servers that accept `password` authentication. However, using the value of the `echo` flag for a prompt just seems to me like guessing at whether or not a pre-configured password should be sent to the server.
> Please note, which is important, that when we try our password with the wrong prompt, the user will still be able to log in, as the server will retry his request and we won't use this password ever again.
Two points to this:
- There are practical situations where sending the wrong value to a prompt (a password to a non-password prompt, or vice-versa) will results in a lockout counter being incremented and eventually lock an account out.
- The design of the Guacamole Client is that, if a password value is provided within the Connection settings, the user will not be prompted for that password. I believe that this suggestion will change that design - that is, the Administrator will provide a value for the password, the client will try to authenticate with that password to a SSH server that supports `keyboard-interactive`, the password may work or may fail, and, instead of the connection shutting down, the user will be prompted for a new input value. Historically we've avoided changes that have the potential to circumvent connection settings that the administrator has specified, and this would be one of those places.
> I think this is a huge step forward in keyboard-interactive support.
I completely agree, and I re-iterate that I'm grateful for the attention you've given this. I just think we have the potential, with a little more thought and work, to solve it in a way that scales to many different environments and situations outside of the specific use-case that is driving your desire to see it fixed. Don't get me wrong - I'm not saying your use-case is unimportant at all, and I'm very glad it has pushed you to contribute, but we should try to solve this issue in a way that doesn't either create problems for other users or create a new problem that we then have to come back and resolve later.
@mike-jumper Thoughts, here - either technical or approach-wise for resolving this? Am I off-base here?
----------------------------------------------------------------
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
[GitHub] [guacamole-server] necouchman commented on a change in pull request #304: GUACAMOLE-141: Implement full support for keyboard-interactive authentication method.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #304:
URL: https://github.com/apache/guacamole-server/pull/304#discussion_r507190356
##########
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:
> 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.
If there's no way to reliably detect what the server is asking for, then I think we at least need to stick with behavior that covers the vast majority of cases, or somehow make it configurable and default to the most common method. I think most 2FA scenarios order things in the following way: username, password (or private key), second factor (OTP, etc.). It sounds like yours is: username, justification, password, second factor?
> 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).
I'm not sure that the order of checking what authentication methods the server supports is as important as understanding what information has already been provided by the client, and being able to determine what the server expects to receive and in what order. A server that expects 2FA/MFA authentication probably is not going to send "password" as an acceptable authentication method, since it is going to be expecting a more interactive login, right? But, even in the situation where the server is requesting `keyboard-interactive`, it's still feasible that the connection would already provide both the username and password (for example, using tokens `${GUAC_USERNAME}` and `${GUAC_PASSWORD}`) and then only prompt the user for other required information (the one-time password, justification, and/or app-based login prompt, etc.).
> 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.
Yeah, I actually toyed with this during the implementation of GUACAMOLE-221 (parameter prompting), and it's a little tricky to do. I'd recommend staying away from this route.
> 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, but prompt the user.
My issue here is that this seems to be based on a tenuous assumption/hope, and may not necessarily be verifiable/reproducible.
I think we need to have a way that either a) reliably detects what is being asked of the user, and/or b) is configurable by the administrator. If we cannot accomplish reliable detection, perhaps we can do something within the connection configuration to allow administrators to specify an order in which a particular server is going to request information? Or maybe that's just too much complication...
----------------------------------------------------------------
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
[GitHub] [guacamole-server] pjd commented on a change in pull request #304: GUACAMOLE-141: Implement full support for keyboard-interactive authentication method.
Posted by GitBox <gi...@apache.org>.
pjd commented on a change in pull request #304:
URL: https://github.com/apache/guacamole-server/pull/304#discussion_r507663782
##########
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:
> > After giving it some additional thought I implemented 1&3: changing order and checking 'password' first will cover the easy cases where there is only a password. 3 will cover majority of the remaining cases where password is expected first.
>
> I agree that switching back to checking for password, first, will make sure that things work for many servers that accept `password` authentication. However, using the value of the `echo` flag for a prompt just seems to me like guessing at whether or not a pre-configured password should be sent to the server.
Well, I want to send the password on the first prompt, but if echo is enabled this means this is not a secret and can be eg. logged. So not sending password when echo is enabled is just a way to keeping the password safe.
> > Please note, which is important, that when we try our password with the wrong prompt, the user will still be able to log in, as the server will retry his request and we won't use this password ever again.
>
> Two points to this:
>
> * There are practical situations where sending the wrong value to a prompt (a password to a non-password prompt, or vice-versa) will results in a lockout counter being incremented and eventually lock an account out.
I don't find this a big problem. You can do test connection to confirm if it is working or not.
> * The design of the Guacamole Client is that, if a password value is provided within the Connection settings, the user will not be prompted for that password. I believe that this suggestion will change that design - that is, the Administrator will provide a value for the password, the client will try to authenticate with that password to a SSH server that supports `keyboard-interactive`, the password may work or may fail, and, instead of the connection shutting down, the user will be prompted for a new input value. Historically we've avoided changes that have the potential to circumvent connection settings that the administrator has specified, and this would be one of those places.
This is a valid point. Indeed, in that case, if the password doesn't match, stopping is probably better choice.
> > I think this is a huge step forward in keyboard-interactive support.
>
> I completely agree, and I re-iterate that I'm grateful for the attention you've given this. I just think we have the potential, with a little more thought and work, to solve it in a way that scales to many different environments and situations outside of the specific use-case that is driving your desire to see it fixed. Don't get me wrong - I'm not saying your use-case is unimportant at all, and I'm very glad it has pushed you to contribute, but we should try to solve this issue in a way that doesn't either create problems for other users or create a new problem that we then have to come back and resolve later.
No worries, I appreciate your comments:)
> @mike-jumper Thoughts, here - either technical or approach-wise for resolving this? Am I off-base here?
----------------------------------------------------------------
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
[GitHub] [guacamole-server] pjd commented on a change in pull request #304: GUACAMOLE-141: Implement full support for keyboard-interactive authentication method.
Posted by 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, but prompt the user.
----------------------------------------------------------------
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
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #304: GUACAMOLE-141: Implement full support for keyboard-interactive authentication method.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #304:
URL: https://github.com/apache/guacamole-server/pull/304#discussion_r579984839
##########
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:
> @mike-jumper Thoughts, here - either technical or approach-wise for resolving this? Am I off-base here?
I agree with the concerns regarding the expected behavior when a password is provided in advance by the administrator. If the administrator provides a password, then that is the sole input that should be used for authentication unless there is some mechanism for the administrator to explicitly dictate otherwise.
I suggest:
1. Maintain the established behavior for cases where the password is provided in advance: prefer "password", use "keyboard-interactive" if needed and only if there is exactly one prompt.
2. If no password is provided in advance, use either authentication method and allow any number of "keyboard-interactive" prompts. It might make sense to prefer "keyboard-interactive" in this case.
The downside to the above would be that it would not be possible to automatically provide a password for SSH servers that use MFA, but it sounds like there is no safe approach for that case (at least not without more complex configuration that allows the administrator to _declare_ their expectations).
----------------------------------------------------------------
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
[GitHub] [guacamole-server] pjd commented on a change in pull request #304: GUACAMOLE-141: Implement full support for keyboard-interactive authentication method.
Posted by 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
[GitHub] [guacamole-server] pjd commented on a change in pull request #304: GUACAMOLE-141: Implement full support for keyboard-interactive authentication method.
Posted by 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, but prompt the user.
----------------------------------------------------------------
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