mike-jumper commented on a change in pull request #228:
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r448554805
##########
File path: src/libguac/guacamole/user.h
##########
@@ -95,6 +95,13 @@ struct guac_user_info {
* is the standard tzdata naming convention.
*/
const char* timezone;
+
+ /**
+ * The Guacamole protocol version that the remote system supports, allowing
+ * support for certain feature support to be negotiated between client and
+ * server.
Review comment:
By "allowing support for certain feature support to be negotiated", did
you mean "allowing for feature support to be negotiated"?
##########
File path: src/libguac/client.c
##########
@@ -633,6 +671,44 @@ static void* __webp_support_callback(guac_user* user,
void* data) {
}
#endif
+/**
+ * Callback function which is invoked by guac_owner_supports_required() to
Review comment:
Typo: `guac_client_owner_supports_required()`
##########
File path: src/libguac/protocol.c
##########
@@ -961,6 +1004,26 @@ int guac_protocol_send_rect(guac_socket* socket,
}
+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, ";");
Review comment:
FYI, given the definition of the return value of this function, this is
equivalent to:
```
ret_val =
guac_socket_write_string(socket, "8.required")
|| guac_socket_write_array(socket, required)
|| guac_socket_write_string(socket, ";");
```
##########
File path: src/libguac/protocol.c
##########
@@ -66,6 +95,37 @@ 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.
Review comment:
If it's worth noting that the lengths are written, it's probably worth
noting that leading commas are added. When reviewing the updated function for
sending `connect`, I initially thought that a comma was missing.
Since both are required aspects of Guacamole protocol formatting, it may be
clearer to note that the values in the array are written as a series of
Guacamole protocol elements, including leading commas and lengths.
##########
File path: src/libguac/guacamole/protocol-types.h
##########
@@ -20,6 +20,8 @@
#ifndef _GUAC_PROTOCOL_TYPES_H
#define _GUAC_PROTOCOL_TYPES_H
+#include <stddef.h>
+
Review comment:
What's this from?
##########
File path: src/libguac/guacamole/protocol.h
##########
@@ -794,6 +794,22 @@ int guac_protocol_send_push(guac_socket* socket, const
guac_layer* layer);
int guac_protocol_send_rect(guac_socket* socket, const guac_layer* layer,
int x, int y, int width, int height);
+/**
+ * Sends a "required" instruction over the given guac_socket connection. This
+ * instruction indicates to the client that an additional parameter is needed
Review comment:
I think it would be more accurate to say "one or more additional
parameters".
##########
File path: src/libguac/client.c
##########
@@ -478,6 +478,44 @@ int guac_client_load_plugin(guac_client* client, const
char* protocol) {
}
+/**
+ * Callback function which is invoked by guac_client_owner_send_required() to
+ * send the required parameters to the specified user, who is the owner of the
+ * client session.
+ *
+ * @param user
+ * The guac_user that will receive the required parameters, who is the
owner
+ * of the client.
+ *
+ * @param data
+ * Pointer to a NULL-terminated array of required parameters that will be
+ * passed on to the owner to continue the connection.
+ *
+ * @return
+ * A pointed to the integer containing the return status of the send
+ * operation.
+ */
+static void* guac_client_owner_send_required_callback(guac_user* user, void*
data) {
+
+ const char** required = (const char **) data;
+
+ /* Send required parameters to owner. */
+ guac_protocol_send_required(user->socket, required);
+
+ return NULL;
+
+}
+
+int guac_client_owner_send_required(guac_client* client, const char**
required) {
+
+ /* Don't send required instruction if client does not support it. */
+ if (!guac_client_owner_supports_required(client))
+ return -1;
+
+ return *(int*) guac_client_for_owner(client,
guac_client_owner_send_required_callback, required);
Review comment:
As written, won't this dereference `NULL`?
##########
File path: src/libguac/protocol.c
##########
@@ -66,6 +95,37 @@ 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.
+ *
+ * @param socket
+ * The socket to which the data should be written.
+ *
+ * @param array
+ * The NULL-terminated array of values to write.
+ *
+ * @return
+ * Zero on success, non-zero on error.
+ */
+static int guac_socket_write_array(guac_socket* socket, const char** array) {
+
+ /* Loop through array, writing provided values to the socket. */
+ for (int i=0; array[i] != NULL; i++) {
+
+ if (guac_socket_write_string(socket, ","))
+ return -1;
+
+ if (__guac_socket_write_length_string(socket, array[i]))
+ return -1;
+
+ }
+
+ return 0;
Review comment:
Something wonky happened to the indentation here.
##########
File path: src/libguac/protocol.c
##########
@@ -1241,3 +1304,34 @@ int guac_protocol_decode_base64(char* base64) {
}
+guac_protocol_version guac_protocol_string_to_version(char* version_string) {
+
+ guac_protocol_version_mapping* current = guac_protocol_version_table;
+ while (current->version != GUAC_PROTOCOL_VERSION_UNKNOWN) {
+
+ if (strcmp(current->version_string, version_string) == 0)
+ return current->version;
+
+ current++;
+
+ }
+
+ return GUAC_PROTOCOL_VERSION_UNKNOWN;
+
+}
+
+const char* guac_protocol_version_to_string(guac_protocol_version version) {
+
+ guac_protocol_version_mapping* current = guac_protocol_version_table;
+ while (current->version) {
+
+ if (current->version == version) {
+ return (const char*) current->version_string;
+ }
+
+ }
+
+ return NULL;
+
+}
Review comment:
Looks good, but I would suggest adding unit tests for these, now that
our unit testing tooling is more reasonable.
https://github.com/apache/guacamole-server/blob/c10ceab7e80f9d6eae9ad98254bf97f41a0de112/README-unit-testing.md
##########
File path: src/protocols/vnc/vnc.h
##########
@@ -133,6 +143,27 @@ typedef struct guac_vnc_client {
* Clipboard encoding-specific writer.
*/
guac_iconv_write* clipboard_writer;
+
+ /**
+ * A lock that will be locked when retrieving required credentials from
+ * the client, and unlocked when credentials have been retrieved.
+ */
+ pthread_mutex_t vnc_credential_lock;
+
+ /**
+ * A condition to use for signaling the thread when credentials have been
+ * retrieved from the client.
+ */
+ pthread_cond_t vnc_credential_cond;
+
+ /**
+ * A field to track flags related to retrieving required credentials
+ * from the client.
Review comment:
The nature of that relation should be documented. Are these flags the
parameters that are required but have not yet been provided? Are they the
parameters that have been provided so far?
##########
File path: src/protocols/rdp/rdp.h
##########
@@ -153,6 +168,29 @@ typedef struct guac_rdp_client {
*/
guac_common_list* available_svc;
+ /**
+ * Lock which is locked when one or more credentials are required to
+ * complete the connection, and unlocked when credentials have been
+ * provided by the client.
+ */
+ pthread_mutex_t rdp_credential_lock;
+
+ /**
+ * Condition which is used when the pthread needs to wait for one or more
+ * credentials to be provided by the client.
+ */
+ pthread_cond_t rdp_credential_cond;
+
+ /**
+ * Flags for tracking events related to the rdp_credential_cond
+ * pthread condition.
Review comment:
As with VNC, the nature of this value with respect to those flags should
be documented here.
##########
File path: src/protocols/ssh/ssh.c
##########
@@ -281,6 +285,7 @@ void* ssh_client_thread(void* data) {
}
pthread_mutex_init(&ssh_client->term_channel_lock, NULL);
+ pthread_cond_init(&(ssh_client->ssh_credential_cond), NULL);
Review comment:
I don't think `term_channel_lock` should reused for
`ssh_credential_cond`. It's specific to guarding access to the SSH terminal
channel.
A pthread condition needs to have its own mutex which guards modification to
the value that is changed when the condition is signaled.
----------------------------------------------------------------
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:
[email protected]
