[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #321: GUACAMOLE-1133: initialize GCrypt in VNC protocol prior to client start-up

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

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



##########
File path: src/protocols/vnc/vnc.c
##########
@@ -49,11 +49,17 @@
 #include <guacamole/wol.h>
 #include <rfb/rfbclient.h>
 #include <rfb/rfbproto.h>
+#ifdef LIBVNCSERVER_WITH_CLIENT_GCRYPT
+#include <gcrypt.h>
+#endif
 
 #include <stdlib.h>
 #include <string.h>
 #include <time.h>
 
+/**
+ * The client key to use to identify this VNC client.
+ */

Review comment:
       The documentation for `GUAC_VNC_CLIENT_KEY` exists in `vnc.h`:
   
   https://github.com/apache/guacamole-server/blob/68c5dd1730804409abf96bda43f11e0dafde4224/src/protocols/vnc/vnc.h#L167-L171

##########
File path: src/protocols/vnc/vnc.c
##########
@@ -133,6 +139,18 @@ rfbClient* guac_vnc_get_client(guac_client* client) {
     /* TLS Locking and Unlocking */
     rfb_client->LockWriteToTLS = guac_vnc_lock_write_to_tls;
     rfb_client->UnlockWriteToTLS = guac_vnc_unlock_write_to_tls;
+#endif
+    
+#ifdef LIBVNCSERVER_WITH_CLIENT_GCRYPT
+    
+    guac_client_log(client, GUAC_LOG_DEBUG, "GCrypt initialization started.");
+    gcry_check_version(NULL);
+    gcry_control(GCRYCTL_SUSPEND_SECMEM_WARN);
+    gcry_control(GCRYCTL_INIT_SECMEM, 16384, 0);
+    gcry_control(GCRYCTL_RESUME_SECMEM_WARN);
+    gcry_control(GCRYCTL_INITIALIZATION_FINISHED, 0);
+    guac_client_log(client, GUAC_LOG_DEBUG, "GCrypt initialization completed.");

Review comment:
       `guac_common_ssh_init()` does similar GCrypt initialization if libssh has been built against GCrypt:
   
   https://github.com/apache/guacamole-server/blob/68c5dd1730804409abf96bda43f11e0dafde4224/src/common-ssh/ssh.c#L142-L149
   
   Which brings a few questions:
   
   * Why `gcry_check_version(NULL)` vs. `gcry_check_version(SOMETHING_SPECIFIC)`?
   * There's a good deal of initialization here that we aren't doing in the common SSH code. Is that a problem?
   * What happens if GCrypt is initialized twice due to libvncclient and libssh2 having both been built against it?




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