[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #228: GUACAMOLE-221: Implement guacd support for prompting

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

mike-jumper commented on a change in pull request #228:
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r448050227



##########
File path: src/libguac/guacamole/protocol-types.h
##########
@@ -276,5 +278,50 @@ typedef enum guac_line_join_style {
     GUAC_LINE_JOIN_ROUND = 0x2
 } guac_line_join_style;
 
+/**
+ * Track the various protocol versions supported by guacd to help negotiate
+ * features that may not be supported by various versions of the client.
+ */
+typedef enum guac_protocol_version {
+    /**
+     * Original protocol version 1.0.0, which lacks support for negotiating
+     * parameters and protocol version.
+     */
+    VERSION_1_0_0 = 100,
+            
+    /**
+     * Protocol version 1.1.0, which includes support for parameter and version
+     * negotiation and for sending timezone information from the client
+     * to the server.
+     */
+    VERSION_1_1_0 = 110,
+            
+    /**
+     * Protocol version 1.2.0, which supports the "required" instruction,
+     * allowing connections in guacd to request information from the client and
+     * await a response.
+     */
+    VERSION_1_2_0 = 120

Review comment:
       Using a convention like `120` for 1.2.0 will be tricky to maintain if a version like 1.10.0 needs to be represented. A convention based on hex might be more maintainable while also being easier to parse with bitwise operations:
   
   For example:
   
       VERSION_1_2_0 = 0x010200
   

##########
File path: src/libguac/guacamole/protocol-constants.h
##########
@@ -38,7 +38,7 @@
  * This version is passed by the __guac_protocol_send_args() function from the
  * server to the client during the client/server handshake.
  */
-#define GUACAMOLE_PROTOCOL_VERSION "VERSION_1_1_0"
+#define GUACAMOLE_PROTOCOL_VERSION "VERSION_1_2_0"

Review comment:
       I believe this would now need to be `VERSION_1_3_0`.

##########
File path: src/libguac/guacamole/protocol-types.h
##########
@@ -276,5 +278,50 @@ typedef enum guac_line_join_style {
     GUAC_LINE_JOIN_ROUND = 0x2
 } guac_line_join_style;
 
+/**
+ * Track the various protocol versions supported by guacd to help negotiate
+ * features that may not be supported by various versions of the client.
+ */
+typedef enum guac_protocol_version {
+    /**
+     * Original protocol version 1.0.0, which lacks support for negotiating
+     * parameters and protocol version.
+     */
+    VERSION_1_0_0 = 100,
+            
+    /**
+     * Protocol version 1.1.0, which includes support for parameter and version
+     * negotiation and for sending timezone information from the client
+     * to the server.
+     */
+    VERSION_1_1_0 = 110,
+            
+    /**
+     * Protocol version 1.2.0, which supports the "required" instruction,
+     * allowing connections in guacd to request information from the client and
+     * await a response.
+     */
+    VERSION_1_2_0 = 120
+
+} guac_protocol_version;
+
+/**
+ * Structure mapping the enum value of a protocol version to the string
+ * representation of the version.
+ */
+typedef struct guac_protocol_version_mapping {
+    
+    /**
+     * The enum value representing the selected protocol version.
+     */
+    guac_protocol_version version;
+    
+    /**
+     * The string value representing the protocol version.
+     */
+    char* version_string;
+    
+} guac_protocol_version_mapping;

Review comment:
       I recommend keeping this structure private to libguac, with the functionality it facilitates being exposed only via the function parsing version strings.

##########
File path: src/guacd/proc.c
##########
@@ -340,6 +340,9 @@ static void guacd_exec_proc(guacd_proc* proc, const char* protocol) {
         owner = 0;
 
     }
+    
+    /* Enable keep alive on the user socket */

Review comment:
       This is on the broadcast socket.

##########
File path: src/libguac/guacamole/protocol-types.h
##########
@@ -276,5 +278,50 @@ typedef enum guac_line_join_style {
     GUAC_LINE_JOIN_ROUND = 0x2
 } guac_line_join_style;
 
+/**
+ * Track the various protocol versions supported by guacd to help negotiate
+ * features that may not be supported by various versions of the client.
+ */
+typedef enum guac_protocol_version {
+    /**
+     * Original protocol version 1.0.0, which lacks support for negotiating
+     * parameters and protocol version.
+     */
+    VERSION_1_0_0 = 100,
+            
+    /**
+     * Protocol version 1.1.0, which includes support for parameter and version
+     * negotiation and for sending timezone information from the client
+     * to the server.
+     */
+    VERSION_1_1_0 = 110,
+            
+    /**
+     * Protocol version 1.2.0, which supports the "required" instruction,
+     * allowing connections in guacd to request information from the client and
+     * await a response.
+     */
+    VERSION_1_2_0 = 120
+
+} guac_protocol_version;

Review comment:
       With 1.2.0 now being released, and the new `required` instruction being part of 1.3.0, we'll need a constant for that version, as well.

##########
File path: src/libguac/guacamole/protocol-types.h
##########
@@ -276,5 +278,50 @@ typedef enum guac_line_join_style {
     GUAC_LINE_JOIN_ROUND = 0x2
 } guac_line_join_style;
 
+/**
+ * Track the various protocol versions supported by guacd to help negotiate
+ * features that may not be supported by various versions of the client.
+ */
+typedef enum guac_protocol_version {
+    /**
+     * Original protocol version 1.0.0, which lacks support for negotiating
+     * parameters and protocol version.
+     */
+    VERSION_1_0_0 = 100,

Review comment:
       Unlike Java, enum constants within C have global scope. They aren't directly tied to the type. In this case, this means that `VERSION_1_0_0`, etc. are all global integer constants.
   
   These constants should be namespaced, presumably by `GUAC_PROTOCOL_`.

##########
File path: src/libguac/client.c
##########
@@ -478,6 +478,29 @@ int guac_client_load_plugin(guac_client* client, const char* protocol) {
 
 }
 
+int guac_client_owner_send_required(guac_client* client, const char** required) {
+
+    /* Don't send require instruction if client does not support it. */
+    if (!guac_client_supports_require(client))
+        return -1;
+    
+    int retval;
+
+    pthread_rwlock_rdlock(&(client->__users_lock));
+
+    /* Invoke callback with current owner */
+    retval = guac_protocol_send_required(client->__owner->socket, required);
+    
+    /* Flush the socket */
+    guac_socket_flush(client->__owner->socket);
+
+    pthread_rwlock_unlock(&(client->__users_lock));

Review comment:
       Rather than manually lock `__users_lock`, we should leverage `guac_client_for_owner()`.

##########
File path: src/libguac/client.c
##########
@@ -633,6 +656,12 @@ static void* __webp_support_callback(guac_user* user, void* data) {
 }
 #endif
 
+int guac_client_supports_require(guac_client* client) {
+    
+    return guac_user_supports_require(client->__owner);

Review comment:
       1. It isn't safe to directly reference `client->__owner` without guarding that reference with one of the `guac_client_for_*()` functions.
   
      It would be safe to do this using `guac_client_for_user()`, but you would probably want to just use `guac_client_for_owner()`. The `guac_client_supports_webp()` function is a good example of how referencing a `guac_user` can be done safely:
   
      https://github.com/apache/guacamole-server/blob/c10ceab7e80f9d6eae9ad98254bf97f41a0de112/src/libguac/client.c#L636-L650
   
   2. Determining whether the owner supports `require` doesn't answer the question of whether the client (all users of the connection) supports `require`. In this case, I think the problem is just in the naming.
   
      If testing the owner, I suggest something more like `guac_client_owner_supports_require()` to avoid confusing the behavior of this function with the established conventions of `guac_client_supports_webp()`.
   

##########
File path: src/libguac/guacamole/protocol.h
##########
@@ -1007,5 +1023,29 @@ int guac_protocol_send_name(guac_socket* socket, const char* name);
  */
 int guac_protocol_decode_base64(char* base64);
 
+/**
+ * Given a string representation of a protocol version, search the mapping
+ * to find the matching enum version, or zero if none is found.
+ * 
+ * @param version_string
+ *     The string representation of the protocol version.
+ * 
+ * @return 
+ *     The enum value of the protocol version, or zero if none is found.

Review comment:
       I think it might be worthwhile to explicitly define an enum value for unknown protocol versions.

##########
File path: src/libguac/protocol.c
##########
@@ -1241,3 +1287,35 @@ 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) {
+        
+        if (strcmp(current->version_string, version_string) == 0)
+            return current->version;
+        
+        current++;
+        
+    }
+    
+    return 0;
+    
+}
+
+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) {
+            char* version_string = malloc(sizeof(current->version_string));
+            return version_string;

Review comment:
       The part of this that copies the actual text of the version string is missing.
   
   Rather than allocate new memory, I would recommend simply returning a direct `const char*` to the string in the table.

##########
File path: src/libguac/client.c
##########
@@ -478,6 +478,29 @@ int guac_client_load_plugin(guac_client* client, const char* protocol) {
 
 }
 
+int guac_client_owner_send_required(guac_client* client, const char** required) {
+
+    /* Don't send require instruction if client does not support it. */
+    if (!guac_client_supports_require(client))
+        return -1;

Review comment:
       Is it `required` or `require`?

##########
File path: src/libguac/guacamole/socket.h
##########
@@ -136,11 +136,25 @@ void guac_socket_free(guac_socket* socket);
  * to ensure neither side of the socket times out while the socket is open.
  * This ping will take the form of a "nop" instruction.
  *
+ * @deprecated
+ *     Manually starting the keep alive process on sockets is no longer
+ *     necessary, as the sockets enable the "nop" ping by default.
+ * 
  * @param socket
  *     The guac_socket to declare as requiring an automatic keep-alive ping.
  */
 void guac_socket_require_keep_alive(guac_socket* socket);
 
+/**
+ * Enables the keep-alive ping functionality on the given socket, causing it
+ * to automatically send a "nop" instruction ping periodically while the
+ * socket is open.
+ * 
+ * @param socket
+ *     The guac_socket on which to enable the automatic keep-alive ping.
+ */
+void guac_socket_enable_keep_alive(guac_socket* socket);

Review comment:
       Why a new function rather than `guac_socket_require_keep_alive()`?

##########
File path: src/common-ssh/ssh.c
##########
@@ -360,7 +361,7 @@ 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: ");
+        common_session->credential_handler(client, GUAC_SSH_PARAMETER_NAME_PASSWORD);

Review comment:
       For SSH/telnet, this should probably support both the old-style terminal prompt and the prompt leveraging `required`, internally selecting between these options depending on owner support.

##########
File path: src/libguac/guacamole/socket.h
##########
@@ -136,11 +136,25 @@ void guac_socket_free(guac_socket* socket);
  * to ensure neither side of the socket times out while the socket is open.
  * This ping will take the form of a "nop" instruction.
  *
+ * @deprecated
+ *     Manually starting the keep alive process on sockets is no longer
+ *     necessary, as the sockets enable the "nop" ping by default.
+ * 

Review comment:
       This is no longer the case, correct? guacd will turn on the keep-alive ping automatically for the only socket that should generally require it (the broadcast socket), but this function no longer needs to be deprecated?




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