From 0db61198e96de757c1cbee4642106371ab9813b5 Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Wed, 1 Jul 2020 16:37:55 -0400 Subject: [PATCH] GUACAMOLE-221: Fix up lots of comments, streamline code, and fix SSH mutex lock. --- src/libguac/guacamole/protocol-types.h | 2 - src/libguac/protocol.c | 21 ++++---- src/protocols/rdp/rdp.h | 7 ++- src/protocols/ssh/client.c | 6 ++- src/protocols/ssh/ssh.c | 70 ++++++++++++++++++++++---- src/protocols/ssh/ssh.h | 6 +++ src/protocols/vnc/vnc.h | 5 +- 7 files changed, 89 insertions(+), 28 deletions(-) diff --git a/src/libguac/guacamole/protocol-types.h b/src/libguac/guacamole/protocol-types.h index 46ca4573..c65bbc29 100644 --- a/src/libguac/guacamole/protocol-types.h +++ b/src/libguac/guacamole/protocol-types.h @@ -20,8 +20,6 @@ #ifndef _GUAC_PROTOCOL_TYPES_H #define _GUAC_PROTOCOL_TYPES_H -#include - /** * Type definitions related to the Guacamole protocol. * diff --git a/src/libguac/protocol.c b/src/libguac/protocol.c index 2c1e132b..78e24fb1 100644 --- a/src/libguac/protocol.c +++ b/src/libguac/protocol.c @@ -96,9 +96,10 @@ ssize_t __guac_socket_write_length_double(guac_socket* socket, double d) { } /** - * Loop through the provided NULL-terminated array, writing the values - * and lengths of the values in the array to the given socket. Return - * zero on success, non-zero on error. + * Loop through the provided NULL-terminated array, writing the values in the + * array to the given socket. Values are written as a series of Guacamole + * protocol elements, including the leading comma and the value length in + * addition to the value itself. Returns zero on success, non-zero on error. * * @param socket * The socket to which the data should be written. @@ -111,7 +112,7 @@ ssize_t __guac_socket_write_length_double(guac_socket* socket, double d) { */ static int guac_socket_write_array(guac_socket* socket, const char** array) { - /* Loop through array, writing provided values to the socket. */ + /* Loop through array, writing provided values to the socket. */ for (int i=0; array[i] != NULL; i++) { if (guac_socket_write_string(socket, ",")) @@ -122,7 +123,7 @@ static int guac_socket_write_array(guac_socket* socket, const char** array) { } - return 0; + return 0; } @@ -1009,14 +1010,10 @@ int guac_protocol_send_required(guac_socket* socket, const char** required) { int ret_val; guac_socket_instruction_begin(socket); - - if (guac_socket_write_string(socket, "8.required")) - return -1; - if (guac_socket_write_array(socket, required)) - return -1; - - ret_val = guac_socket_write_string(socket, ";"); + ret_val = guac_socket_write_string(socket, "8.required") + || guac_socket_write_array(socket, required) + || guac_socket_write_string(socket, ";"); guac_socket_instruction_end(socket); diff --git a/src/protocols/rdp/rdp.h b/src/protocols/rdp/rdp.h index 26684337..3b84b8e9 100644 --- a/src/protocols/rdp/rdp.h +++ b/src/protocols/rdp/rdp.h @@ -183,7 +183,12 @@ typedef struct guac_rdp_client { /** * Flags for tracking events related to the rdp_credential_cond - * pthread condition. + * pthread condition. These flags will be set when credential parameters + * are required by the connection, and cleared when those have been + * provided by the client. All flags are cleared at the start of the + * connection, and then set as the client determines that further + * information is required. + * * * @see GUAC_RDP_CRED_FLAG_USERNAME * @see GUAC_RDP_CRED_FLAG_PASSWORD diff --git a/src/protocols/ssh/client.c b/src/protocols/ssh/client.c index 1bd6647e..986968e2 100644 --- a/src/protocols/ssh/client.c +++ b/src/protocols/ssh/client.c @@ -47,7 +47,7 @@ int guac_client_init(guac_client* client) { /* Init clipboard */ ssh_client->clipboard = guac_common_clipboard_alloc(GUAC_SSH_CLIPBOARD_MAX_LENGTH); - + /* Set handlers */ client->join_handler = guac_ssh_user_join_handler; client->free_handler = guac_ssh_client_free_handler; @@ -120,6 +120,10 @@ int guac_ssh_client_free_handler(guac_client* client) { guac_common_clipboard_free(ssh_client->clipboard); free(ssh_client); + /* Destroy the pthread conditional handler */ + pthread_cond_destroy(&(ssh_client->ssh_credential_cond)); + pthread_mutex_destroy(&(ssh_client->ssh_credential_lock)); + guac_common_ssh_uninit(); return 0; } diff --git a/src/protocols/ssh/ssh.c b/src/protocols/ssh/ssh.c index 40040c01..57effd54 100644 --- a/src/protocols/ssh/ssh.c +++ b/src/protocols/ssh/ssh.c @@ -58,6 +58,35 @@ #include #include +/** + * A data structure that maps parameter names of credentials that will be + * prompted to the actual text that will be displayed on the screen. + */ +typedef struct guac_ssh_credential_prompt_map { + + /** + * The parameter name of the credential that is being prompted for. + */ + char* name; + + /** + * The text that will display to the user during the prompt. + */ + char* prompt; + +} guac_ssh_credential_prompt_map; + +/** + * The map of credentials the user can be prompted for to the text displayed + * on the screen. + */ +guac_ssh_credential_prompt_map ssh_credential_prompt_map[] = { + { GUAC_SSH_PARAMETER_NAME_USERNAME, "Login as: " }, + { GUAC_SSH_PARAMETER_NAME_PASSWORD, "Password: " }, + { GUAC_SSH_PARAMETER_NAME_PASSPHRASE, "Key passphrase: " }, + { NULL, NULL} +}; + /** * This function generates a prompt to the specified instance of guac_client * for the credential specified in the cred_name parameter, which should @@ -74,19 +103,15 @@ static void guac_ssh_get_credential(guac_client *client, char* cred_name) { guac_ssh_client* ssh_client = (guac_ssh_client*) client->data; - /* If the client does not support the "required" instruction, just return. */ - if (!guac_client_owner_supports_required(client)) - return; - /* Lock the terminal thread while prompting for the credential. */ - pthread_mutex_lock(&(ssh_client->term_channel_lock)); + pthread_mutex_lock(&(ssh_client->ssh_credential_lock)); /* Let the owner know what we require. */ guac_client_owner_send_required(client, (const char* []) {cred_name, NULL}); /* Wait for the response, and then unlock the thread. */ - pthread_cond_wait(&(ssh_client->ssh_credential_cond), &(ssh_client->term_channel_lock)); - pthread_mutex_unlock(&(ssh_client->term_channel_lock)); + pthread_cond_wait(&(ssh_client->ssh_credential_cond), &(ssh_client->ssh_credential_lock)); + pthread_mutex_unlock(&(ssh_client->ssh_credential_lock)); } @@ -110,9 +135,21 @@ static guac_common_ssh_user* guac_ssh_get_user(guac_client* client) { guac_common_ssh_user* user; /* Get username */ - while (settings->username == NULL) - guac_ssh_get_credential(client, GUAC_SSH_PARAMETER_NAME_USERNAME); + while (settings->username == NULL) { + + /* Client owner supports required instruction, so send prompt(s) that way. */ + if (guac_client_owner_supports_required(client)) { + guac_ssh_get_credential(client, GUAC_SSH_PARAMETER_NAME_USERNAME); + } + + /* Fall back to terminal prompting. */ + else { + settings->username = guac_terminal_prompt(ssh_client->term, + "Login as: ", true); + } + } + /* Create user object from username */ user = guac_common_ssh_create_user(settings->username); @@ -135,8 +172,18 @@ static guac_common_ssh_user* guac_ssh_get_user(guac_client* client) { "Re-attempting private key import (WITH passphrase)"); /* Prompt for passphrase if missing */ - while (settings->key_passphrase == NULL) - guac_ssh_get_credential(client, GUAC_SSH_PARAMETER_NAME_PASSPHRASE); + while (settings->key_passphrase == NULL) { + + /* Send prompt via required instruction, if supported */ + if (guac_client_owner_supports_required(client)) + guac_ssh_get_credential(client, GUAC_SSH_PARAMETER_NAME_PASSPHRASE); + + /* Fall back to terminal prompt */ + else + settings->key_passphrase = guac_terminal_prompt(ssh_client->term, + "Key passphrase: ", true); + + } /* Reattempt import with passphrase */ if (guac_common_ssh_user_import_key(user, @@ -285,6 +332,7 @@ void* ssh_client_thread(void* data) { } pthread_mutex_init(&ssh_client->term_channel_lock, NULL); + pthread_mutex_init(&ssh_client->ssh_credential_lock, NULL); pthread_cond_init(&(ssh_client->ssh_credential_cond), NULL); /* Open channel for terminal */ diff --git a/src/protocols/ssh/ssh.h b/src/protocols/ssh/ssh.h index bfd2fd1b..1378522e 100644 --- a/src/protocols/ssh/ssh.h +++ b/src/protocols/ssh/ssh.h @@ -90,6 +90,12 @@ typedef struct guac_ssh_client { */ pthread_mutex_t term_channel_lock; + /** + * Lock that will be locked when retrieving required credentials from the + * user, and unlocked when those requirements are satisfied. + */ + pthread_mutex_t ssh_credential_lock; + /** * Condition used when SSH client thread needs to wait for Guacamole * client to pass additional credentials before continuing the connection. diff --git a/src/protocols/vnc/vnc.h b/src/protocols/vnc/vnc.h index 9a2d48a8..62c3c1a1 100644 --- a/src/protocols/vnc/vnc.h +++ b/src/protocols/vnc/vnc.h @@ -158,7 +158,10 @@ typedef struct guac_vnc_client { /** * A field to track flags related to retrieving required credentials - * from the client. + * from the client. These flags will be set when credentials are required + * that have not been provided, and cleared when those credentials are + * provided. All flags are cleared at the start of the connection, and + * then set as the client determines which are required. * * @see GUAC_VNC_COND_FLAG_USERNAME * @see GUAC_VNC_COND_FLAG_PASSWORD