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

[GitBox]

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



##
File path: src/protocols/vnc/auth.c
##
@@ -19,27 +19,101 @@
 
 #include "config.h"
 
+#include "argv.h"
 #include "auth.h"
 #include "vnc.h"
 
+#include 
 #include 
+#include 
+#include 
+#include 
 #include 
 #include 
 
+#include 
+#include 
+
 char* guac_vnc_get_password(rfbClient* client) {
 guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-return ((guac_vnc_client*) gc->data)->settings->password;
+guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+guac_vnc_settings* settings = vnc_client->settings;
+
+/* If the client does not support the "required" instruction, just return
+the configuration data. */
+if (!guac_client_owner_supports_required(gc))
+return ((guac_vnc_client*) gc->data)->settings->password;
+
+/* If password isn't around, prompt for it. */
+if (settings->password == NULL) {
+
+guac_argv_register(GUAC_VNC_ARGV_PASSWORD, guac_vnc_argv_callback, 
NULL, 0);
+
+const char* params[] = {GUAC_VNC_ARGV_PASSWORD, NULL};
+
+/* Send the request for password to the owner. */
+guac_client_owner_send_required(gc, params);
+
+/* Wait for the arguments to be returned */
+guac_argv_await(params);
+
+}
+
+return settings->password;

Review comment:
   As this will also be freed by libvncclient, we'll need a `guac_strdup()` 
here just as with `guac_vnc_get_credentials()`.

##
File path: src/protocols/vnc/settings.c
##
@@ -392,11 +393,11 @@ guac_vnc_settings* guac_vnc_parse_args(guac_user* user,
 
 settings->username =
 guac_user_parse_args_string(user, GUAC_VNC_CLIENT_ARGS, argv,
-IDX_USERNAME, ""); /* NOTE: freed by libvncclient */
+IDX_USERNAME, NULL); /* NOTE: freed by libvncclient */
 
 settings->password =
 guac_user_parse_args_string(user, GUAC_VNC_CLIENT_ARGS, argv,
-IDX_PASSWORD, ""); /* NOTE: freed by libvncclient */
+IDX_PASSWORD, NULL); /* NOTE: freed by libvncclient */

Review comment:
   I believe these notes regarding these strings being freed by 
libvncclient are no longer correct.





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/protocols/vnc/auth.c
##
@@ -19,27 +19,100 @@
 
 #include "config.h"
 
+#include "argv.h"
 #include "auth.h"
 #include "vnc.h"
 
+#include 
 #include 
+#include 
+#include 
 #include 
 #include 
 
+#include 
+#include 
+
 char* guac_vnc_get_password(rfbClient* client) {
 guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-return ((guac_vnc_client*) gc->data)->settings->password;
+guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+guac_vnc_settings* settings = vnc_client->settings;
+
+/* If the client does not support the "required" instruction, just return
+the configuration data. */
+if (!guac_client_owner_supports_required(gc))
+return ((guac_vnc_client*) gc->data)->settings->password;
+
+/* If password isn't around, prompt for it. */
+if (settings->password == NULL || strcmp(settings->password, "") == 0) {
+
+guac_argv_register(GUAC_VNC_ARGV_PASSWORD, guac_vnc_argv_callback, 
NULL, 0);
+
+const char* params[] = {GUAC_VNC_ARGV_PASSWORD, NULL};
+
+/* Send the request for password to the owner. */
+guac_client_owner_send_required(gc, params);
+
+/* Wait for the arguments to be returned */
+guac_argv_await(params);
+
+}
+
+return settings->password;
+
 }
 
 rfbCredential* guac_vnc_get_credentials(rfbClient* client, int credentialType) 
{
+
 guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-guac_vnc_settings* settings = ((guac_vnc_client*) gc->data)->settings;
+guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+guac_vnc_settings* settings = vnc_client->settings;
 
+/* Handle request for Username/Password credentials */
 if (credentialType == rfbCredentialTypeUser) {
 rfbCredential *creds = malloc(sizeof(rfbCredential));
-creds->userCredential.username = settings->username;
-creds->userCredential.password = settings->password;
+
+/* If the client supports the "required" instruction, prompt for and
+   update those. */
+if (guac_client_owner_supports_required(gc)) {
+char* params[3] = {NULL};
+int i = 0;
+
+/* Check if username is null or empty. */
+if (settings->username == NULL) {
+guac_argv_register(GUAC_VNC_ARGV_USERNAME, 
guac_vnc_argv_callback, NULL, 0);
+params[i] = GUAC_VNC_ARGV_USERNAME;
+i++;
+}
+
+/* Check if password is null or empty. */
+if (settings->password == NULL) {
+guac_argv_register(GUAC_VNC_ARGV_PASSWORD, 
guac_vnc_argv_callback, NULL, 0);
+params[i] = GUAC_VNC_ARGV_PASSWORD;
+i++;
+}
+
+params[i] = NULL;
+
+/* If we have empty parameters, request them. */
+if (i > 0) {
+
+/* Send required parameters to owner. */
+guac_client_owner_send_required(gc, (const char**) params);
+
+/* Wait for the parameters to be returned. */
+guac_argv_await((const char**) params);
+
+return creds;
+
+}
+}
+
+/* Copy the values and return the credential set. */
+creds->userCredential.username = strdup(settings->username);
+creds->userCredential.password = strdup(settings->password);

Review comment:
   With the switch to using `NULL`, I think this is likely fine. Just need 
to now:
   
   * Use the `NULL`-friendly `guac_strdup()`.
   * Free `settings->username` and `settings->password` within 
`guac_vnc_settings_free()`.





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/libguac/string.c
##
@@ -81,6 +81,17 @@ size_t guac_strlcat(char* restrict dest, const char* 
restrict src, size_t n) {
 
 }
 
+char* guac_strdup(const char* str) {

Review comment:
   The tests you've added look good to me.





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/protocols/rdp/rdp.c
##
@@ -227,10 +236,70 @@ static BOOL rdp_freerdp_authenticate(freerdp* instance, 
char** username,
 
 rdpContext* context = instance->context;
 guac_client* client = ((rdp_freerdp_context*) context)->client;
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+char* params[4] = {NULL};
+int i = 0;
+
+/* If the client does not support the "required" instruction, warn and
+ * quit.
+ */
+if (!guac_client_owner_supports_required(client)) {
+guac_client_log(client, GUAC_LOG_WARNING, "Client does not support the 
"
+"\"required\" instruction. No authentication parameters will "
+"be requested.");
+return TRUE;
+}
 
-/* Warn if connection is likely to fail due to lack of credentials */
-guac_client_log(client, GUAC_LOG_INFO,
-"Authentication requested but username or password not given");
+/* If the username is undefined, add it to the requested parameters. */
+if (settings->username == NULL) {
+guac_argv_register(GUAC_RDP_ARGV_USERNAME, guac_rdp_argv_callback, 
NULL, 0);
+params[i] = GUAC_RDP_ARGV_USERNAME;
+i++;
+
+/* If username is undefined and domain is also undefined, request 
domain. */
+if (settings->domain == NULL) {
+guac_argv_register(GUAC_RDP_ARGV_DOMAIN, guac_rdp_argv_callback, 
NULL, 0);
+params[i] = GUAC_RDP_ARGV_DOMAIN;
+i++;
+}
+
+}
+
+/* If the password is undefined, add it to the requested parameters. */
+if (settings->password == NULL) {
+guac_argv_register(GUAC_RDP_ARGV_PASSWORD, guac_rdp_argv_callback, 
NULL, 0);
+params[i] = GUAC_RDP_ARGV_PASSWORD;
+i++;
+}
+
+/* NULL-terminate the array. */
+params[i] = NULL;
+
+if (i > 0) {
+
+/* Send required parameters to the owner. */
+guac_client_owner_send_required(client, (const char**) params);
+
+guac_argv_await((const char**) params);
+
+/* Free old values and get new values from settings. */
+if (*username != NULL)
+free(*username);
+
+if (*password != NULL)
+free(*password);
+
+if (*domain != NULL)
+free(*domain);

Review comment:
   By the way, there's no need to check a pointer against `NULL` before 
invoking `free()`. If the pointer passed to `free()` is `NULL`, no operation is 
performed.
   





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/protocols/vnc/auth.c
##
@@ -19,27 +19,100 @@
 
 #include "config.h"
 
+#include "argv.h"
 #include "auth.h"
 #include "vnc.h"
 
+#include 
 #include 
+#include 
+#include 
 #include 
 #include 
 
+#include 
+#include 
+
 char* guac_vnc_get_password(rfbClient* client) {
 guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-return ((guac_vnc_client*) gc->data)->settings->password;
+guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+guac_vnc_settings* settings = vnc_client->settings;
+
+/* If the client does not support the "required" instruction, just return
+the configuration data. */
+if (!guac_client_owner_supports_required(gc))
+return ((guac_vnc_client*) gc->data)->settings->password;
+
+/* If password isn't around, prompt for it. */
+if (settings->password == NULL || strcmp(settings->password, "") == 0) {
+
+guac_argv_register(GUAC_VNC_ARGV_PASSWORD, guac_vnc_argv_callback, 
NULL, 0);
+
+const char* params[] = {GUAC_VNC_ARGV_PASSWORD, NULL};
+
+/* Send the request for password to the owner. */
+guac_client_owner_send_required(gc, params);
+
+/* Wait for the arguments to be returned */
+guac_argv_await(params);
+
+}
+
+return settings->password;
+
 }
 
 rfbCredential* guac_vnc_get_credentials(rfbClient* client, int credentialType) 
{
+
 guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-guac_vnc_settings* settings = ((guac_vnc_client*) gc->data)->settings;
+guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+guac_vnc_settings* settings = vnc_client->settings;
 
+/* Handle request for Username/Password credentials */
 if (credentialType == rfbCredentialTypeUser) {
 rfbCredential *creds = malloc(sizeof(rfbCredential));
-creds->userCredential.username = settings->username;
-creds->userCredential.password = settings->password;
+
+/* If the client supports the "required" instruction, prompt for and
+   update those. */
+if (guac_client_owner_supports_required(gc)) {
+char* params[3] = {NULL};
+int i = 0;
+
+/* Check if username is null or empty. */
+if (settings->username == NULL) {
+guac_argv_register(GUAC_VNC_ARGV_USERNAME, 
guac_vnc_argv_callback, NULL, 0);
+params[i] = GUAC_VNC_ARGV_USERNAME;
+i++;
+}
+
+/* Check if password is null or empty. */
+if (settings->password == NULL) {
+guac_argv_register(GUAC_VNC_ARGV_PASSWORD, 
guac_vnc_argv_callback, NULL, 0);
+params[i] = GUAC_VNC_ARGV_PASSWORD;
+i++;
+}
+
+params[i] = NULL;
+
+/* If we have empty parameters, request them. */
+if (i > 0) {
+
+/* Send required parameters to owner. */
+guac_client_owner_send_required(gc, (const char**) params);
+
+/* Wait for the parameters to be returned. */
+guac_argv_await((const char**) params);
+
+return creds;
+
+}
+}
+
+/* Copy the values and return the credential set. */
+creds->userCredential.username = strdup(settings->username);
+creds->userCredential.password = strdup(settings->password);

Review comment:
   Briefly checking the libvncclient source, it looks like it always checks 
for `NULL` for both the username and password before taking any action, and so 
it should be safe for us to migrate to using `NULL` instead of `""`.
   
   Good point on the `rfbCredential` that we allocate here. I had not checked 
this before, but it is indeed freed by libvncclient, just after libvncclient 
frees the username and password, as part of `FreeUserCredential()` - an 
internal function invoked by the parts of libvncclient that handle auth after 
they are done handling auth.
   
   If there is concern that we may need to continue referencing the 
username/password ourselves, then yes, I agree they should be strdup'd and we 
should manually free our copies when disconnect happens. It looks like 
libvncclient frees the memory associated with credentials immediately after 
use, so any attempt to use those values after they have already been used by 
libvncclient will segfault (or worse).





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:
us...@infra.apache

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

[GitBox]

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



##
File path: src/protocols/vnc/auth.c
##
@@ -19,27 +19,100 @@
 
 #include "config.h"
 
+#include "argv.h"
 #include "auth.h"
 #include "vnc.h"
 
+#include 
 #include 
+#include 
+#include 
 #include 
 #include 
 
+#include 
+#include 
+
 char* guac_vnc_get_password(rfbClient* client) {
 guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-return ((guac_vnc_client*) gc->data)->settings->password;
+guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+guac_vnc_settings* settings = vnc_client->settings;
+
+/* If the client does not support the "required" instruction, just return
+the configuration data. */
+if (!guac_client_owner_supports_required(gc))
+return ((guac_vnc_client*) gc->data)->settings->password;
+
+/* If password isn't around, prompt for it. */
+if (settings->password == NULL || strcmp(settings->password, "") == 0) {
+
+guac_argv_register(GUAC_VNC_ARGV_PASSWORD, guac_vnc_argv_callback, 
NULL, 0);
+
+const char* params[] = {GUAC_VNC_ARGV_PASSWORD, NULL};
+
+/* Send the request for password to the owner. */
+guac_client_owner_send_required(gc, params);
+
+/* Wait for the arguments to be returned */
+guac_argv_await(params);
+
+}
+
+return settings->password;
+
 }
 
 rfbCredential* guac_vnc_get_credentials(rfbClient* client, int credentialType) 
{
+
 guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-guac_vnc_settings* settings = ((guac_vnc_client*) gc->data)->settings;
+guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+guac_vnc_settings* settings = vnc_client->settings;
 
+/* Handle request for Username/Password credentials */
 if (credentialType == rfbCredentialTypeUser) {
 rfbCredential *creds = malloc(sizeof(rfbCredential));
-creds->userCredential.username = settings->username;
-creds->userCredential.password = settings->password;
+
+/* If the client supports the "required" instruction, prompt for and
+   update those. */
+if (guac_client_owner_supports_required(gc)) {
+char* params[3] = {NULL};
+int i = 0;
+
+/* Check if username is null or empty. */
+if (settings->username == NULL) {
+guac_argv_register(GUAC_VNC_ARGV_USERNAME, 
guac_vnc_argv_callback, NULL, 0);
+params[i] = GUAC_VNC_ARGV_USERNAME;
+i++;
+}
+
+/* Check if password is null or empty. */
+if (settings->password == NULL) {
+guac_argv_register(GUAC_VNC_ARGV_PASSWORD, 
guac_vnc_argv_callback, NULL, 0);
+params[i] = GUAC_VNC_ARGV_PASSWORD;
+i++;
+}
+
+params[i] = NULL;
+
+/* If we have empty parameters, request them. */
+if (i > 0) {
+
+/* Send required parameters to owner. */
+guac_client_owner_send_required(gc, (const char**) params);
+
+/* Wait for the parameters to be returned. */
+guac_argv_await((const char**) params);
+
+return creds;
+
+}
+}
+
+/* Copy the values and return the credential set. */
+creds->userCredential.username = strdup(settings->username);
+creds->userCredential.password = strdup(settings->password);

Review comment:
   Briefly checking the libvncclient source, it looks like it always checks 
for `NULL` for both the username and password before taking any action, and so 
it should be safe for us to migrate to using `NULL` instead of `""`.
   
   Good point on the `rfbCredential` that we allocate here. I had not checked 
this before, but it is indeed freed by libvncclient, just after libvncclient 
frees the username and password, as part of `FreeUserCredential()` - an 
internal function invoked by the parts of libvncclient that handle auth after 
they are done handling auth.
   
   If there is concern that we may need to continue referencing the 
username/password ourselves, then yes, I agree they should be strdup'd and we 
should manually free our copies when disconnect happens. It looks like 
libvncclient frees the memory associated with credentials immediately after 
use, so any attempt to use those vales after they have already been used by 
libvncclient will segfault (or worse).





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:
us...@infra.apache.

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

[GitBox]

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



##
File path: src/protocols/vnc/auth.c
##
@@ -19,27 +19,100 @@
 
 #include "config.h"
 
+#include "argv.h"
 #include "auth.h"
 #include "vnc.h"
 
+#include 
 #include 
+#include 
+#include 
 #include 
 #include 
 
+#include 
+#include 
+
 char* guac_vnc_get_password(rfbClient* client) {
 guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-return ((guac_vnc_client*) gc->data)->settings->password;
+guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+guac_vnc_settings* settings = vnc_client->settings;
+
+/* If the client does not support the "required" instruction, just return
+the configuration data. */
+if (!guac_client_owner_supports_required(gc))
+return ((guac_vnc_client*) gc->data)->settings->password;
+
+/* If password isn't around, prompt for it. */
+if (settings->password == NULL || strcmp(settings->password, "") == 0) {
+
+guac_argv_register(GUAC_VNC_ARGV_PASSWORD, guac_vnc_argv_callback, 
NULL, 0);
+
+const char* params[] = {GUAC_VNC_ARGV_PASSWORD, NULL};
+
+/* Send the request for password to the owner. */
+guac_client_owner_send_required(gc, params);
+
+/* Wait for the arguments to be returned */
+guac_argv_await(params);
+
+}
+
+return settings->password;
+
 }
 
 rfbCredential* guac_vnc_get_credentials(rfbClient* client, int credentialType) 
{
+
 guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-guac_vnc_settings* settings = ((guac_vnc_client*) gc->data)->settings;
+guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+guac_vnc_settings* settings = vnc_client->settings;
 
+/* Handle request for Username/Password credentials */
 if (credentialType == rfbCredentialTypeUser) {
 rfbCredential *creds = malloc(sizeof(rfbCredential));
-creds->userCredential.username = settings->username;
-creds->userCredential.password = settings->password;
+
+/* If the client supports the "required" instruction, prompt for and
+   update those. */
+if (guac_client_owner_supports_required(gc)) {
+char* params[3] = {NULL};
+int i = 0;
+
+/* Check if username is null or empty. */
+if (settings->username == NULL) {
+guac_argv_register(GUAC_VNC_ARGV_USERNAME, 
guac_vnc_argv_callback, NULL, 0);
+params[i] = GUAC_VNC_ARGV_USERNAME;
+i++;
+}
+
+/* Check if password is null or empty. */
+if (settings->password == NULL) {
+guac_argv_register(GUAC_VNC_ARGV_PASSWORD, 
guac_vnc_argv_callback, NULL, 0);
+params[i] = GUAC_VNC_ARGV_PASSWORD;
+i++;
+}

Review comment:
   Does this work? Checking the assignments to `settings->username` and 
`settings->password`, an empty username/password is stored as a 
dynamically-allocated empty string, not `NULL`:
   
   
https://github.com/apache/guacamole-server/blob/382d72a26a04008aa936680deed00e3a31086b1e/src/protocols/vnc/settings.c#L393-L399

##
File path: src/protocols/vnc/auth.c
##
@@ -19,27 +19,100 @@
 
 #include "config.h"
 
+#include "argv.h"
 #include "auth.h"
 #include "vnc.h"
 
+#include 
 #include 
+#include 
+#include 
 #include 
 #include 
 
+#include 
+#include 
+
 char* guac_vnc_get_password(rfbClient* client) {
 guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-return ((guac_vnc_client*) gc->data)->settings->password;
+guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+guac_vnc_settings* settings = vnc_client->settings;
+
+/* If the client does not support the "required" instruction, just return
+the configuration data. */
+if (!guac_client_owner_supports_required(gc))
+return ((guac_vnc_client*) gc->data)->settings->password;
+
+/* If password isn't around, prompt for it. */
+if (settings->password == NULL || strcmp(settings->password, "") == 0) {
+
+guac_argv_register(GUAC_VNC_ARGV_PASSWORD, guac_vnc_argv_callback, 
NULL, 0);
+
+const char* params[] = {GUAC_VNC_ARGV_PASSWORD, NULL};
+
+/* Send the request for password to the owner. */
+guac_client_owner_send_required(gc, params);
+
+/* Wait for the arguments to be returned */
+guac_argv_await(params);
+
+}
+
+return settings->password;
+
 }
 
 rfbCredential* guac_vnc_get_credentials(rfbClient* client, int credentialType) 
{
+
 guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-guac_vnc_settings* settings = ((guac_vnc_client*) gc->data)->setti

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

[GitBox]

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



##
File path: src/protocols/rdp/rdp.c
##
@@ -227,10 +234,60 @@ static BOOL rdp_freerdp_authenticate(freerdp* instance, 
char** username,
 
 rdpContext* context = instance->context;
 guac_client* client = ((rdp_freerdp_context*) context)->client;
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+char* params[4] = {};
+int i = 0;
+
+/* If the client does not support the "required" instruction, warn and
+ * quit.
+ */
+if (!guac_client_owner_supports_required(client)) {
+guac_client_log(client, GUAC_LOG_WARNING, "Client does not support the 
"
+"\"required\" instruction. No authentication parameters will "
+"be requested.");
+return TRUE;
+}
 
-/* Warn if connection is likely to fail due to lack of credentials */
-guac_client_log(client, GUAC_LOG_INFO,
-"Authentication requested but username or password not given");
+/* If the username is undefined, add it to the requested parameters. */
+if (settings->username == NULL) {
+guac_argv_register(GUAC_RDP_ARGV_USERNAME, guac_rdp_argv_callback, 
NULL, 0);
+params[i] = GUAC_RDP_ARGV_USERNAME;
+i++;
+}
+
+/* If the password is undefined, add it to the requested parameters. */
+if (settings->password == NULL) {
+guac_argv_register(GUAC_RDP_ARGV_PASSWORD, guac_rdp_argv_callback, 
NULL, 0);
+params[i] = GUAC_RDP_ARGV_PASSWORD;
+i++;
+}
+
+/* If the domain is undefined, add it to the requested parameters. */
+if (settings->domain == NULL) {
+guac_argv_register(GUAC_RDP_ARGV_DOMAIN, guac_rdp_argv_callback, NULL, 
0);
+params[i] = GUAC_RDP_ARGV_DOMAIN;
+i++;
+}
+
+/* NULL-terminate the array. */
+params[i] = NULL;
+
+if (i > 0) {
+
+/* Send required parameters to the owner. */
+guac_client_owner_send_required(client, (const char**) params);
+
+guac_argv_await((const char**) params);
+
+/* Get new values from settings. */
+*username = settings->username;
+*password = settings->password;
+*domain = settings->domain;

Review comment:
   I think `guac_rdp_strdup()` is needed here. The standard `strdup()` will 
segfault if given `NULL`, whereas `guac_rdp_strdup()` will simply and safely 
return `NULL`.
   
   The definition for `guac_rdp_strdup()` could be moved to `settings.h`. Or we 
could consider adding a `guac_strdup()` to libguac's `string.h`.





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/libguac/client.c
##
@@ -633,6 +668,40 @@ static void* __webp_support_callback(guac_user* user, 
void* data) {
 }
 #endif
 
+/**
+ * A callback function which is invoked by 
guac_client_owner_supports_required()
+ * to determine if the owner of a client supports the "required" instruction,
+ * updating the flag to indicate support.
+ * 
+ * @param user
+ * The guac_user that will be checked for "required" instruction support.
+ * 
+ * @param data
+ * A pointer to an int containing the status for support of the "required"
+ * instruction.  This will be 0 if the owner does not support this
+ * instruction, or 1 if the owner does support it.
+ * 
+ * @return
+ * Always NULL.
+ */
+static void* guac_owner_supports_required_callback(guac_user* user, void* 
data) {
+
+data = (void *) ((intptr_t) guac_user_supports_required(user));

Review comment:
   Ah, OK. If `guac_user_supports_required()` already handles this case, I 
have no issue with this.





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/libguac/client.c
##
@@ -633,6 +668,40 @@ static void* __webp_support_callback(guac_user* user, 
void* data) {
 }
 #endif
 
+/**
+ * A callback function which is invoked by 
guac_client_owner_supports_required()
+ * to determine if the owner of a client supports the "required" instruction,
+ * updating the flag to indicate support.
+ * 
+ * @param user
+ * The guac_user that will be checked for "required" instruction support.
+ * 
+ * @param data
+ * A pointer to an int containing the status for support of the "required"
+ * instruction.  This will be 0 if the owner does not support this
+ * instruction, or 1 if the owner does support it.
+ * 
+ * @return
+ * Always NULL.
+ */
+static void* guac_owner_supports_required_callback(guac_user* user, void* 
data) {
+
+data = (void *) ((intptr_t) guac_user_supports_required(user));

Review comment:
   This effectively discards the result of `guac_user_supports_required()`. 
I think you mean something like:
   
   ```c
   *((int*) data) = (int) ((intptr_t) guac_user_supports_required(user));
   ```
   
   That said, `guac_client_for_owner()` provides a mechanism for returning a 
value from the callback which is being leveraged by 
`guac_client_owner_send_required()` above. Why not do the same here?

##
File path: src/protocols/vnc/vnc.h
##
@@ -45,6 +45,16 @@
 
 #include 
 
+/**
+ * A flag for tracking the status of requesting username from client.
+ */
+#define GUAC_VNC_COND_FLAG_USERNAME 1
+
+/**
+ * A flag for tracking the status of requesting password from client.
+ */
+#define GUAC_VNC_COND_FLAG_PASSWORD 2
+

Review comment:
   Are these flags still used?

##
File path: src/libguac/client.c
##
@@ -633,6 +668,40 @@ static void* __webp_support_callback(guac_user* user, 
void* data) {
 }
 #endif
 
+/**
+ * A callback function which is invoked by 
guac_client_owner_supports_required()
+ * to determine if the owner of a client supports the "required" instruction,
+ * updating the flag to indicate support.
+ * 
+ * @param user
+ * The guac_user that will be checked for "required" instruction support.
+ * 
+ * @param data
+ * A pointer to an int containing the status for support of the "required"
+ * instruction.  This will be 0 if the owner does not support this
+ * instruction, or 1 if the owner does support it.
+ * 
+ * @return
+ * Always NULL.
+ */
+static void* guac_owner_supports_required_callback(guac_user* user, void* 
data) {
+
+data = (void *) ((intptr_t) guac_user_supports_required(user));

Review comment:
   Same here - beware that `user` can be `NULL` if the owner has left the 
connection.

##
File path: src/libguac/client.c
##
@@ -478,6 +478,41 @@ int guac_client_load_plugin(guac_client* client, const 
char* protocol) {
 
 }
 
+/**
+ * A 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
+ * A pointer to a NULL-terminated array of required parameters that will be
+ * passed on to the owner to continue the connection.
+ * 
+ * @return
+ * The integer status of the operation, cast as void*.
+ */
+static void* guac_client_owner_send_required_callback(guac_user* user, void* 
data) {
+
+const char** required = (const char **) data;
+
+/* Send required parameters to owner. */
+return (void*) ((intptr_t) guac_protocol_send_required(user->socket, 
required));

Review comment:
   Beware that `user` can be `NULL` if the owner has left the connection. 
See:
   
   
https://guacamole.apache.org/doc/1.1.0/libguac/client_8h.html#af3f4ed85d98b16376e2cdc031ff1b44a

##
File path: src/protocols/vnc/user.c
##
@@ -98,6 +99,9 @@ int guac_vnc_user_join_handler(guac_user* user, int argc, 
char** argv) {
 /* Inbound (client to server) clipboard transfer */
 if (!settings->disable_paste)
 user->clipboard_handler = guac_vnc_clipboard_handler;
+
+/* Updates to connection parameters */
+user->argv_handler = guac_argv_handler;

Review comment:
   Same here - I believe this should be set only for the connection owner.

##
File path: src/protocols/rdp/user.c
##
@@ -116,6 +117,9 @@ int guac_rdp_user_join_handler(guac_user* user, int argc, 
char** argv) {
 
 /* Inbound arbitrary named pipes */
 user->pipe_handler = guac_rdp_pipe_svc_pipe_handler;
+
+/* Handler for updating parameters during connection. */
+user->argv_handler = guac_argv_handler;

Review comment:
   I bel

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

[GitBox]

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



##
File path: src/libguac/client.c
##
@@ -633,6 +669,44 @@ static void* __webp_support_callback(guac_user* user, 
void* data) {
 }
 #endif
 
+/**
+ * A callback function which is invoked by 
guac_client_owner_supports_required()
+ * to determine if the owner of a client supports the "required" instruction,
+ * updating the flag to indicate support.
+ * 
+ * @param user
+ * The guac_user that will be checked for "required" instruction support.
+ * 
+ * @param data
+ * A pointer to an int containing the status for support of the "required"
+ * instruction.  This will be 0 if the owner does not support this
+ * instruction, or 1 if the owner does support it.
+ * 
+ * @return
+ * Always NULL.
+ */
+static void* guac_owner_supports_required_callback(guac_user* user, void* 
data) {
+
+int* required_supported = (int *) data;
+
+/* Check if owner supports required */
+if (*required_supported)
+*required_supported = guac_user_supports_required(user);

Review comment:
   This looks to be written to allow the callback to be used similar to the 
check for WebP support (with each user of a connection being tested for the 
needed support and the test passing only if all connected users have support), 
but I only see `guac_client_owner_supports_required()` and the naming of this 
callback looks like this is intentionally limited in scope.
   
   Why not simply assign to `*required_supported` directly, without the test 
for current value and without the initialization of `required_supported`?

##
File path: src/libguac/client.c
##
@@ -478,6 +478,42 @@ int guac_client_load_plugin(guac_client* client, const 
char* protocol) {
 
 }
 
+/**
+ * A 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
+ * A pointer to a NULL-terminated array of required parameters that will be
+ * passed on to the owner to continue the connection.
+ * 
+ * @return
+ * A pointer to an integer containing the return status of the send
+ * operation.

Review comment:
   The line:
   
   ```c
   return (void*) ((intptr_t) guac_protocol_send_required(user->socket, 
required));
   ```
   
   isn't returning a pointer to an integer, but rather an integer value that 
has been cast to `void*` via `intptr_t`.

##
File path: src/libguac/guacamole/protocol.h
##
@@ -1007,5 +1023,32 @@ 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 GUAC_PROTOCOL_VERSION_UNKNOWN if no
+ * match is found.
+ * 
+ * @param version_string
+ * The string representation of the protocol version.
+ * 
+ * @return 
+ * The enum value of the protocol version, or GUAC_PROTOCOL_VERSION_UNKNOWN
+ * if no match is found.
+ */
+guac_protocol_version guac_protocol_string_to_version(char* version_string);

Review comment:
   If `guac_protocol_string_to_version()` does not modify the contents of 
`version_string`, it should be `const char*`.

##
File path: src/protocols/vnc/argv.c
##
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "config.h"
+#include "argv.h"
+#include "vnc.h"
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+int guac_vnc_argv_callback(guac_user* user, const char* mimetype,
+const char* name, const char* value, void* data) {
+
+guac_client* client = user->client;
+guac_vnc_client* vnc_client = (guac_vnc_client*) client->data;
+guac_vnc_settings* settings = vnc_client->settings;
+
+/* Update username */
+if (strcmp(name, GUAC_VNC_ARGV_USERNAME) == 0) {
+free(settings->username);
+   

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

[GitBox]

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 
+

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 prov

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

[GitBox]

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



##
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:
   Yep - there's a standard C99 way to losslessly transform between `int` 
and ` void*` and back: `intptr_t`, provided by `stdint.h`.
   
   Here's an example of `int` to `void*`:
   
   
https://github.com/apache/guacamole-server/blob/c10ceab7e80f9d6eae9ad98254bf97f41a0de112/src/guaclog/keydef.c#L166
   
   And here's an example of `void*` to `int`:
   
   
https://github.com/apache/guacamole-server/blob/c10ceab7e80f9d6eae9ad98254bf97f41a0de112/src/guaclog/keydef.c#L143
   
   If `intptr_t` weren't a possibility, the next option would be to pass 
everything that the callback needs within a `struct`, including a location to 
store what would otherwise be its return value.





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/protocols/ssh/ssh.c
##
@@ -57,6 +58,38 @@
 #include 
 #include 
 
+/**
+ * This function generates a prompt to the specified instance of guac_client
+ * for the credential specified in the cred_name parameter, which should
+ * be a valid SSH connection parameter.
+ * 
+ * @param client
+ * The guac_client object associated with the current connection
+ * where additional credentials are required.
+ * 
+ * @param cred_name
+ * The name of the parameter to prompt for in the client.
+ */
+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 "require" instruction, just return. 
*/
+if (!guac_client_supports_require(client))

Review comment:
   Yep.





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:
us...@infra.apache.org

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

[GitBox]

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



##
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:
   Well ... internally, yes, I agree it's more of an "enable" than a 
"require".
   
   The idea behind the original `guac_socket_require_keep_alive()` naming was 
to define that function as a means of declaring a requirement, rather than 
imposing a specific method for socket implementations to satisfy that 
requirement, decoupling the statement of need from how/whether the need is met.
   
   Admittedly, the wording in the documentation of 
`guac_socket_require_keep_alive()` which explicitly specifies a "keep-alive 
ping" and states that "this ping will take the form of a "nop" instruction" 
probably defeats that intent.





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:
us...@infra.apache.org

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

[GitBox]

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



##
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:
   Ah, I see what you're saying. Yes, that's correct, there would be no 
protocol version 1.2.0.





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:
us...@infra.apache.org

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

[GitBox]

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

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

[GitBox]

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



##
File path: src/libguac/protocol.c
##
@@ -961,6 +975,50 @@ int guac_protocol_send_rect(guac_socket* socket,
 
 }
 
+/**
+ * Sends the "required" instruction on the given socket, looping
+ * through all the items provided in the NULL-terminated array,
+ * and closing out the instruction.  Returns zero on success, or
+ * non-zero on error.
+ *
+ * @param socket
+ * The socket on which to write the instruction.
+ *
+ * @param required
+ * The NULL-terminated array of required parameters to send
+ * to the client.
+ *
+ * @return
+ * Zero if successful, non-zero on error.
+ */
+static int __guac_protocol_send_required(guac_socket* socket,
+const char** required) {
+
+// The socket should be kept alive while waiting for user response.
+guac_socket_require_keep_alive(socket);

Review comment:
   In fact ... given that there are definitely cases where a `guac_socket` 
does not need a `nop` ping (the tee socket writing screen recordings to a file, 
for example), perhaps the keep-alive should only be set by default where it is 
known to be correct, like right after initialization of the `guac_client` via 
`guac_client_load_plugin()` succeeds within guacd? (Or as part of documented 
behavior of `guac_client_load_plugin()`?)
   
   Even assuming that the broadcast socket needs a keep-alive ping might be 
overly presumptuous on the utility of a socket that broadcasts to all users.





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/libguac/protocol.c
##
@@ -961,6 +975,50 @@ int guac_protocol_send_rect(guac_socket* socket,
 
 }
 
+/**
+ * Sends the "required" instruction on the given socket, looping
+ * through all the items provided in the NULL-terminated array,
+ * and closing out the instruction.  Returns zero on success, or
+ * non-zero on error.
+ *
+ * @param socket
+ * The socket on which to write the instruction.
+ *
+ * @param required
+ * The NULL-terminated array of required parameters to send
+ * to the client.
+ *
+ * @return
+ * Zero if successful, non-zero on error.
+ */
+static int __guac_protocol_send_required(guac_socket* socket,
+const char** required) {
+
+// The socket should be kept alive while waiting for user response.
+guac_socket_require_keep_alive(socket);

Review comment:
   In fact ... given that there are definitely cases where a `guac_socket` 
does not need a `nop` ping (the tee socket writing screen recordings to a file, 
for example), perhaps the keep-alive should only be set by default where it is 
known to be correct, like right after initialization of the `guac_client` via 
`guac_client_load_plugin()` succeeds within guacd?
   
   Even assuming that the broadcast socket needs a keep-alive ping might be 
overly presumptuous on the utility of a socket that broadcasts to all users.





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/libguac/protocol.c
##
@@ -961,6 +975,50 @@ int guac_protocol_send_rect(guac_socket* socket,
 
 }
 
+/**
+ * Sends the "required" instruction on the given socket, looping
+ * through all the items provided in the NULL-terminated array,
+ * and closing out the instruction.  Returns zero on success, or
+ * non-zero on error.
+ *
+ * @param socket
+ * The socket on which to write the instruction.
+ *
+ * @param required
+ * The NULL-terminated array of required parameters to send
+ * to the client.
+ *
+ * @return
+ * Zero if successful, non-zero on error.
+ */
+static int __guac_protocol_send_required(guac_socket* socket,
+const char** required) {
+
+// The socket should be kept alive while waiting for user response.
+guac_socket_require_keep_alive(socket);

Review comment:
   Hm ... looking over 9bfcbbf3a576054f82ce9bce6ef2a0497304ca8a, I'm not 
sure `guac_socket` allows for this to be done safely by default:
   
   1. The current architecture of `guac_socket` relies on it being impossible 
for that `guac_socket` to be used before its handlers have been initialized. 
This is because a pointer to the new `guac_socket` simply does not exist 
outside the internals of the various `guac_socket` implementations until they 
have finished their init process.
   
  If `guac_socket` will now create this thread as part of its own init 
(which happens first), then there is no guarantee that the handlers will be 
properly initialized by the implementation  before the thread starts trying to 
write `nop` instructions, leading to unstable behavior.
   
   2. Creating the keep-alive ping on the broadcast socket is nice and 
efficient as it only results in a new single thread, with that thread covering 
all users of a connection. Creating it across the board would result in a new 
thread for the broadcast socket plus new threads for each of the per-user 
sockets it writes to, a bit of a waste.
   
   It would be safe for implementations to start the keep-alive ping on their 
own, but I don't think `guac_socket` can do this on behalf of all 
implementations. There's just no way for those implementations to let 
`guac_socket` know that initialization is complete.
   
   My knee-jerk thoughts on the above would be to say that we should start the 
keep-alive by default within the broadcast socket implementation and leave it 
at that.
   
   What do you think?





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/protocols/rdp/argv.c
##
@@ -0,0 +1,188 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "config.h"
+#include "argv.h"
+#include "rdp.h"
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+/**
+ * All RDP connection settings which may be updated by unprivileged users
+ * through "argv" streams.
+ */
+typedef enum guac_rdp_argv_setting {
+
+/**
+ * The username of the connection.
+ */
+GUAC_RDP_ARGV_SETTING_USERNAME,
+
+/**
+ * The password to authenticate the connection.
+ */
+GUAC_RDP_ARGV_SETTING_PASSWORD,
+
+/**
+ * The domain to use for connection authentication.
+ */
+GUAC_RDP_ARGV_SETTING_DOMAIN,
+
+} guac_rdp_argv_setting;
+
+/**
+ * The value or current status of a connection parameter received over an
+ * "argv" stream.
+ */
+typedef struct guac_rdp_argv {
+
+/**
+ * The specific setting being updated.
+ */
+guac_rdp_argv_setting setting;
+
+/**
+ * Buffer space for containing the received argument value.
+ */
+char buffer[GUAC_RDP_ARGV_MAX_LENGTH];
+
+/**
+ * The number of bytes received so far.
+ */
+int length;
+
+} guac_rdp_argv;
+
+/**
+ * Handler for "blob" instructions which appends the data from received blobs
+ * to the end of the in-progress argument value buffer.
+ *
+ * @see guac_user_blob_handler
+ */
+static int guac_rdp_argv_blob_handler(guac_user* user,
+guac_stream* stream, void* data, int length) {
+
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+
+/* Calculate buffer size remaining, including space for null terminator,
+adjusting received length accordingly */
+int remaining = sizeof(argv->buffer) - argv->length - 1;
+if (length > remaining)
+length = remaining;
+
+/* Append received data to end of buffer */
+memcpy(argv->buffer + argv->length, data, length);
+argv->length += length;
+
+return 0;
+
+}

Review comment:
   BTW, an idea just occurred to me that may work well here. We currently 
provide a number of convenience functions for parsing arguments like 
`guac_user_parse_args_string()`:
   
   
https://github.com/apache/guacamole-server/blob/master/src/libguac/guacamole/user.h#L875-L876
   
   Perhaps we should provide analogous `argv` variants?





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/protocols/rdp/argv.c
##
@@ -0,0 +1,188 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "config.h"
+#include "argv.h"
+#include "rdp.h"
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+/**
+ * All RDP connection settings which may be updated by unprivileged users
+ * through "argv" streams.
+ */
+typedef enum guac_rdp_argv_setting {
+
+/**
+ * The username of the connection.
+ */
+GUAC_RDP_ARGV_SETTING_USERNAME,
+
+/**
+ * The password to authenticate the connection.
+ */
+GUAC_RDP_ARGV_SETTING_PASSWORD,
+
+/**
+ * The domain to use for connection authentication.
+ */
+GUAC_RDP_ARGV_SETTING_DOMAIN,
+
+} guac_rdp_argv_setting;
+
+/**
+ * The value or current status of a connection parameter received over an
+ * "argv" stream.
+ */
+typedef struct guac_rdp_argv {
+
+/**
+ * The specific setting being updated.
+ */
+guac_rdp_argv_setting setting;
+
+/**
+ * Buffer space for containing the received argument value.
+ */
+char buffer[GUAC_RDP_ARGV_MAX_LENGTH];
+
+/**
+ * The number of bytes received so far.
+ */
+int length;
+
+} guac_rdp_argv;
+
+/**
+ * Handler for "blob" instructions which appends the data from received blobs
+ * to the end of the in-progress argument value buffer.
+ *
+ * @see guac_user_blob_handler
+ */
+static int guac_rdp_argv_blob_handler(guac_user* user,
+guac_stream* stream, void* data, int length) {
+
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+
+/* Calculate buffer size remaining, including space for null terminator,
+adjusting received length accordingly */
+int remaining = sizeof(argv->buffer) - argv->length - 1;
+if (length > remaining)
+length = remaining;
+
+/* Append received data to end of buffer */
+memcpy(argv->buffer + argv->length, data, length);
+argv->length += length;
+
+return 0;
+
+}

Review comment:
   BTW, an idea just occurred to me that may work well here. We currently 
provide a number of convenience functions for parsing arguments like 
`guac_user_parse_args_string()`:
   
   
https://github.com/apache/guacamole-server/blob/c10ceab7e80f9d6eae9ad98254bf97f41a0de112/src/libguac/guacamole/user.h#L875-L876
   
   Perhaps we should provide analogous `argv` variants?





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/protocols/rdp/rdp.c
##
@@ -227,10 +227,53 @@ static BOOL rdp_freerdp_authenticate(freerdp* instance, 
char** username,
 
 rdpContext* context = instance->context;
 guac_client* client = ((rdp_freerdp_context*) context)->client;
-
-/* Warn if connection is likely to fail due to lack of credentials */
-guac_client_log(client, GUAC_LOG_INFO,
-"Authentication requested but username or password not given");
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+char* params[4] = {};
+int i = 0;
+
+if (settings->username == NULL || strcmp(settings->username, "") == 0) {
+params[i] = "username";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_USERNAME;
+i++;
+}
+
+if (settings->password == NULL || strcmp(settings->password, "") == 0) {
+params[i] = "password";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_PASSWORD;
+i++;
+}
+
+if (settings->domain == NULL || strcmp(settings->domain, "") == 0) {
+params[i] = "domain";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_DOMAIN;
+i++;
+}

Review comment:
   For `settings->foo` coming from `guac_user_parse_args_string(..., 
NULL)`, yes, this will always be `NULL`. It would only perhaps be `""` if the 
call were `guac_user_parse_args_string(..., "")`.
   
   BUT, it is likely possible for things to be `""` if the value is set via an 
empty `argv` stream. If that's the case, then I would suggest ensuring that 
handling of the `argv` stream uses `NULL` rather than `""` if it receives zero 
bytes overall.





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/libguac/protocol.c
##
@@ -961,6 +975,50 @@ int guac_protocol_send_rect(guac_socket* socket,
 
 }
 
+/**
+ * Sends the "required" instruction on the given socket, looping
+ * through all the items provided in the NULL-terminated array,
+ * and closing out the instruction.  Returns zero on success, or
+ * non-zero on error.
+ *
+ * @param socket
+ * The socket on which to write the instruction.
+ *
+ * @param required
+ * The NULL-terminated array of required parameters to send
+ * to the client.
+ *
+ * @return
+ * Zero if successful, non-zero on error.
+ */
+static int __guac_protocol_send_required(guac_socket* socket,
+const char** required) {
+
+// The socket should be kept alive while waiting for user response.
+guac_socket_require_keep_alive(socket);

Review comment:
   > Presumably once it's set on a socket it does not have to be re-set - 
so the usages toward the beginning of the `guac_vnc_client_thread()` and 
`ssh_client_thread()` functions set it for the duration of the SSH session, 
correct?
   
   Correct.
   
   > Is there any reason not to just add it to the main RDP thread, prior to 
authentication?
   
   Nope. It imposes very minimal overhead, and is already designed to not 
bother sending pings unless nothing has been sent along the `guac_socket` in a 
while.
   
   > With 3 out of 5 protocols implementing the requirement for keep-alives 
very early on in the connection process, and assuming the keep-alives are then 
active for the duration of the connections, is there any reason not to just set 
it across the board?
   
   Actually ... I think this would be fine, too. That fact would need to be 
documented, but it seems like that would be best. We can then mark 
`guac_socket_require_keep_alive()` as deprecated and note that it is now a 
no-op.





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/protocols/rdp/rdp.c
##
@@ -227,10 +227,53 @@ static BOOL rdp_freerdp_authenticate(freerdp* instance, 
char** username,
 
 rdpContext* context = instance->context;
 guac_client* client = ((rdp_freerdp_context*) context)->client;
-
-/* Warn if connection is likely to fail due to lack of credentials */
-guac_client_log(client, GUAC_LOG_INFO,
-"Authentication requested but username or password not given");
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+char* params[4] = {};
+int i = 0;
+
+if (settings->username == NULL || strcmp(settings->username, "") == 0) {
+params[i] = "username";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_USERNAME;
+i++;
+}
+
+if (settings->password == NULL || strcmp(settings->password, "") == 0) {
+params[i] = "password";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_PASSWORD;
+i++;
+}
+
+if (settings->domain == NULL || strcmp(settings->domain, "") == 0) {
+params[i] = "domain";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_DOMAIN;
+i++;
+}
+
+/* NULL-terminate the array. */
+params[i] = NULL;
+
+if (i > 0) {
+/* Lock the client thread. */
+pthread_mutex_lock(&(rdp_client->rdp_credential_lock));
+
+/* Send require params and flush socket. */
+guac_protocol_send_required(client->socket, (const char**) params);

Review comment:
   > So, should the `guac_protocol_send_required()` function be re-worked 
to take `guac_user` instead of a `guac_socket`?
   
   No, I think the `guac_protocol_send_*()` family of functions should remain 
unaware of the concerns of `guac_user`, `guac_client`, etc. They are intended 
as low-level functions for sending instructions over a `guac_socket`.
   
   > Or should I implement some intermediate function in `user.c` that uses 
this `guac_protocol_send_required()` function and just passes through the 
`guac_user->socket`?
   
   This would be the way, yes.
   
   As this would be a common need, if you're looking to avoid having to do this 
everywhere, I think it could make sense to provide a convenience function like 
`guac_client_owner_send_required()` or similar.





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/protocols/rdp/rdp.c
##
@@ -227,10 +227,53 @@ static BOOL rdp_freerdp_authenticate(freerdp* instance, 
char** username,
 
 rdpContext* context = instance->context;
 guac_client* client = ((rdp_freerdp_context*) context)->client;
-
-/* Warn if connection is likely to fail due to lack of credentials */
-guac_client_log(client, GUAC_LOG_INFO,
-"Authentication requested but username or password not given");
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+char* params[4] = {};
+int i = 0;
+
+if (settings->username == NULL || strcmp(settings->username, "") == 0) {
+params[i] = "username";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_USERNAME;
+i++;
+}
+
+if (settings->password == NULL || strcmp(settings->password, "") == 0) {
+params[i] = "password";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_PASSWORD;
+i++;
+}
+
+if (settings->domain == NULL || strcmp(settings->domain, "") == 0) {
+params[i] = "domain";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_DOMAIN;
+i++;
+}
+
+/* NULL-terminate the array. */
+params[i] = NULL;
+
+if (i > 0) {
+/* Lock the client thread. */
+pthread_mutex_lock(&(rdp_client->rdp_credential_lock));
+
+/* Send require params and flush socket. */
+guac_protocol_send_required(client->socket, (const char**) params);

Review comment:
   Alrighty, a couple of issues there:
   
   1. Nothing should be referencing the private portions of libguac structures 
except libguac. To safely access the owner of a connection, you really do need 
to use `guac_client_for_owner()`.
   
  To see why, take a look at the implementation of 
`guac_client_for_owner()`:
   
  
https://github.com/apache/guacamole-server/blob/c10ceab7e80f9d6eae9ad98254bf97f41a0de112/src/libguac/client.c#L366-L371
   
  Use of that lock is critical. If you just reference `__owner` directly, 
there is no guarantee that something like the following won't happen:
   
  1. The current, non-`NULL` value of `__owner` is stored somewhere (a 
variable, the stack for a function call, etc.)
  2. The owner leaves the connection and their corresponding `guac_user` is 
freed.
  3. The stored pointer now points to freed memory, which may already have 
been reused elsewhere, and chaos ensues.
   
  Using `guac_client_for_owner()` should work just fine whenever you need a 
`guac_user`. In any case that you would be tempted to reference 
`client->__owner->whatever`, you can instead do `user->whatever` for the user 
passed to the callback.
   
   2. You probably actually want to use the broadcast socket for 
`guac_socket_require_keep_alive()`.
   
  The utility of that function is that it ensures the other side of the 
connection doesn't drop out while waiting for data from the server. It is true 
that we probably only want to request `argv` from the owner, but if the 
connection is shared, we don't want _any_ of those connections to drop out. A 
user sharing the connection shouldn't get disconnected just because the owner 
of the connection is taking their time to enter their credentials.





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/protocols/rdp/rdp.c
##
@@ -227,10 +227,53 @@ static BOOL rdp_freerdp_authenticate(freerdp* instance, 
char** username,
 
 rdpContext* context = instance->context;
 guac_client* client = ((rdp_freerdp_context*) context)->client;
-
-/* Warn if connection is likely to fail due to lack of credentials */
-guac_client_log(client, GUAC_LOG_INFO,
-"Authentication requested but username or password not given");
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+char* params[4] = {};
+int i = 0;
+
+if (settings->username == NULL || strcmp(settings->username, "") == 0) {
+params[i] = "username";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_USERNAME;
+i++;
+}
+
+if (settings->password == NULL || strcmp(settings->password, "") == 0) {
+params[i] = "password";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_PASSWORD;
+i++;
+}
+
+if (settings->domain == NULL || strcmp(settings->domain, "") == 0) {
+params[i] = "domain";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_DOMAIN;
+i++;
+}
+
+/* NULL-terminate the array. */
+params[i] = NULL;
+
+if (i > 0) {
+/* Lock the client thread. */
+pthread_mutex_lock(&(rdp_client->rdp_credential_lock));
+
+/* Send require params and flush socket. */
+guac_protocol_send_required(client->socket, (const char**) params);

Review comment:
   Alrighty, a couple of issues there:
   
   1. Nothing should be referencing the private portions of libguac structures 
except libguac. To safely access the owner of a connection, you really do need 
to use `guac_client_for_owner()`.
   
  It should work just fine. In any case that you would be tempted to 
reference `client->__owner->whatever`, you can instead do `user->whatever` for 
the user passed to the callback.
   
   2. You probably actually want to use the broadcast socket for 
`guac_socket_require_keep_alive()`.
   
  The utility of that function is that it ensures the other side of the 
connection doesn't drop out while waiting for data from the server. It is true 
that we probably only want to request `argv` from the owner, but if the 
connection is shared, we don't want _any_ of those connections to drop out. A 
user sharing the connection shouldn't get disconnected just because the owner 
of the connection is taking their time to enter their credentials.





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:
us...@infra.apache.org

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

[GitBox]

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



##
File path: src/protocols/rdp/rdp.c
##
@@ -227,10 +227,53 @@ static BOOL rdp_freerdp_authenticate(freerdp* instance, 
char** username,
 
 rdpContext* context = instance->context;
 guac_client* client = ((rdp_freerdp_context*) context)->client;
-
-/* Warn if connection is likely to fail due to lack of credentials */
-guac_client_log(client, GUAC_LOG_INFO,
-"Authentication requested but username or password not given");
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+char* params[4] = {};
+int i = 0;
+
+if (settings->username == NULL || strcmp(settings->username, "") == 0) {
+params[i] = "username";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_USERNAME;
+i++;
+}
+
+if (settings->password == NULL || strcmp(settings->password, "") == 0) {
+params[i] = "password";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_PASSWORD;
+i++;
+}
+
+if (settings->domain == NULL || strcmp(settings->domain, "") == 0) {
+params[i] = "domain";
+rdp_client->rdp_credential_flags |= GUAC_RDP_CRED_FLAG_DOMAIN;
+i++;
+}
+
+/* NULL-terminate the array. */
+params[i] = NULL;
+
+if (i > 0) {
+/* Lock the client thread. */
+pthread_mutex_lock(&(rdp_client->rdp_credential_lock));
+
+/* Send require params and flush socket. */
+guac_protocol_send_required(client->socket, (const char**) params);

Review comment:
   `client->socket` is the broadcast socket which will send instructions to 
all users sharing the current connection. This should probably be isolated to 
the `owner`, with handling of any relevant `argv` also specific to just the 
owner.

##
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 the required instruction over the given guac_socket connection.  This

Review comment:
   I know what you mean here, but the phrase `Sends the required 
instruction ...` sounds like this functions sends whichever instruction is 
required by some unspecified criteria. I think something like `Sends a 
"required" instruction ...` would be a bit clearer, but then again ... it might 
just be impossible to avoid this due to the name.

##
File path: src/libguac/protocol.c
##
@@ -961,6 +975,50 @@ int guac_protocol_send_rect(guac_socket* socket,
 
 }
 
+/**
+ * Sends the "required" instruction on the given socket, looping
+ * through all the items provided in the NULL-terminated array,
+ * and closing out the instruction.  Returns zero on success, or
+ * non-zero on error.
+ *
+ * @param socket
+ * The socket on which to write the instruction.
+ *
+ * @param required
+ * The NULL-terminated array of required parameters to send
+ * to the client.
+ *
+ * @return
+ * Zero if successful, non-zero on error.
+ */
+static int __guac_protocol_send_required(guac_socket* socket,
+const char** required) {
+
+// The socket should be kept alive while waiting for user response.
+guac_socket_require_keep_alive(socket);

Review comment:
   I think this should be the responsibility of the caller, not a side 
effect of invoking `guac_protocol_send_required()`. Only the caller can know 
whether keep-alive pings are required or whether their implementation is 
already active enough on its own.

##
File path: src/protocols/rdp/argv.c
##
@@ -0,0 +1,188 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "config.h"
+#include "argv.h"
+#include "rdp.h"
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+/**
+ * All RDP connection settings which may be updated by unprivileged users
+ * through "argv" streams.
+ */
+t

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

[GitBox]

mike-jumper commented on a change in pull request #228: GUACAMOLE-221: 
Implement guacd support for prompting
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r371009915
 
 

 ##
 File path: src/protocols/rdp/argv.c
 ##
 @@ -0,0 +1,192 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "config.h"
+#include "argv.h"
+#include "rdp.h"
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+/**
+ * All RDP connection settings which may be updated by unprivileged users
+ * through "argv" streams.
+ */
+typedef enum guac_rdp_argv_setting {
+
+/**
+ * The username of the connection.
+ */
+GUAC_RDP_ARGV_SETTING_USERNAME,
+
+/**
+ * The password to authenticate the connection.
+ */
+GUAC_RDP_ARGV_SETTING_PASSWORD,
+
+/**
+ * The domain to use for connection authentication.
+ */
+GUAC_RDP_ARGV_SETTING_DOMAIN,
+
+} guac_rdp_argv_setting;
+
+/**
+ * The value or current status of a connection parameter received over an
+ * "argv" stream.
+ */
+typedef struct guac_rdp_argv {
+
+/**
+ * The specific setting being updated.
+ */
+guac_rdp_argv_setting setting;
+
+/**
+ * Buffer space for containing the received argument value.
+ */
+char buffer[GUAC_RDP_ARGV_MAX_LENGTH];
+
+/**
+ * The number of bytes received so far.
+ */
+int length;
+
+} guac_rdp_argv;
+
+/**
+ * Handler for "blob" instructions which appends the data from received blobs
+ * to the end of the in-progress argument value buffer.
+ *
+ * @see guac_user_blob_handler
+ */
+static int guac_rdp_argv_blob_handler(guac_user* user,
+guac_stream* stream, void* data, int length) {
+
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+
+/* Calculate buffer size remaining, including space for null terminator,
+ * adjusting received length accordingly */
+int remaining = sizeof(argv->buffer) - argv->length - 1;
+if (length > remaining)
+length = remaining;
+
+/* Append received data to end of buffer */
+memcpy(argv->buffer + argv->length, data, length);
+argv->length += length;
+
+return 0;
+
+}
+
+/**
+ * Handler for "end" instructions which applies the changes specified by the
+ * argument value buffer associated with the stream.
+ *
+ * @see guac_user_end_handler
+ */
+static int guac_rdp_argv_end_handler(guac_user* user,
+guac_stream* stream) {
+
+guac_client* client = user->client;
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+
+/* Append null terminator to value */
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+argv->buffer[argv->length] = '\0';
 
 Review comment:
   Yep, definitely necessary. Nothing handling the received data as a string 
will be able to function safely without that null terminator.
   
   The beauty of `strndup(foo, length)` is that it will automatically add that 
null terminator and ensure that there is space for it within the 
newly-allocated buffer. Assuming the string within the buffer is indeed 
`length` characters long, `strndup(..., length)` would allocate buffer space of 
at least `length + 1` bytes.
   
   Overall:
   
   * We do need that null terminator.
   * It feels weird to know the length of the string ahead of time, add a null 
terminator, and then recalculate that length value with `strlen()` based on the 
null terminator we just added.
   * If we're going to use `strlen()` to feed a `malloc()` and `strcpy()`, 
that's a common enough operation that there's a standard POSIX function for 
that: `strdup()`
   * `strdup()` depends on having a null terminator within the string being 
copied, yet it's unsafe to add a null terminator within that buffer because we 
don't know for certain there is space. Our choices then are:
 1. Use `strndup()` OR
 2. Use `malloc(length + 1)`, `memcpy(..., length)` the old buffer to the 
new buffer, and then add the null terminator to the new buffer.
 3. Ensure that there _is_ buffer space for the null terminator, document 
for `GUAC_RDP_ARGV_MAX_LENGTH` that it includ

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

[GitBox]

mike-jumper commented on a change in pull request #228: GUACAMOLE-221: 
Implement guacd support for prompting
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r371009915
 
 

 ##
 File path: src/protocols/rdp/argv.c
 ##
 @@ -0,0 +1,192 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "config.h"
+#include "argv.h"
+#include "rdp.h"
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+/**
+ * All RDP connection settings which may be updated by unprivileged users
+ * through "argv" streams.
+ */
+typedef enum guac_rdp_argv_setting {
+
+/**
+ * The username of the connection.
+ */
+GUAC_RDP_ARGV_SETTING_USERNAME,
+
+/**
+ * The password to authenticate the connection.
+ */
+GUAC_RDP_ARGV_SETTING_PASSWORD,
+
+/**
+ * The domain to use for connection authentication.
+ */
+GUAC_RDP_ARGV_SETTING_DOMAIN,
+
+} guac_rdp_argv_setting;
+
+/**
+ * The value or current status of a connection parameter received over an
+ * "argv" stream.
+ */
+typedef struct guac_rdp_argv {
+
+/**
+ * The specific setting being updated.
+ */
+guac_rdp_argv_setting setting;
+
+/**
+ * Buffer space for containing the received argument value.
+ */
+char buffer[GUAC_RDP_ARGV_MAX_LENGTH];
+
+/**
+ * The number of bytes received so far.
+ */
+int length;
+
+} guac_rdp_argv;
+
+/**
+ * Handler for "blob" instructions which appends the data from received blobs
+ * to the end of the in-progress argument value buffer.
+ *
+ * @see guac_user_blob_handler
+ */
+static int guac_rdp_argv_blob_handler(guac_user* user,
+guac_stream* stream, void* data, int length) {
+
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+
+/* Calculate buffer size remaining, including space for null terminator,
+ * adjusting received length accordingly */
+int remaining = sizeof(argv->buffer) - argv->length - 1;
+if (length > remaining)
+length = remaining;
+
+/* Append received data to end of buffer */
+memcpy(argv->buffer + argv->length, data, length);
+argv->length += length;
+
+return 0;
+
+}
+
+/**
+ * Handler for "end" instructions which applies the changes specified by the
+ * argument value buffer associated with the stream.
+ *
+ * @see guac_user_end_handler
+ */
+static int guac_rdp_argv_end_handler(guac_user* user,
+guac_stream* stream) {
+
+guac_client* client = user->client;
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+
+/* Append null terminator to value */
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+argv->buffer[argv->length] = '\0';
 
 Review comment:
   Yep, definitely necessary. Nothing handling the received data as a string 
will be able to function safely without that null terminator.
   
   The beauty of `strndup(foo, length)` is that it will automatically add that 
null terminator and ensure that there is space for it within the 
newly-allocated buffer. Assuming the string within the buffer is indeed 
`length` characters long, `strndup(..., length)` would allocate buffer space of 
at least `length + 1` bytes.
   
   Overall:
   
   * We do need that null terminator.
   * It feels weird to know the length of the string ahead of time, add a null 
terminator, and then recalculate that length value with `strlen()` based on the 
null terminator we just added.
   * If we're going to use `strlen()` to feed a `malloc()` and `strcpy()`, 
that's a common enough operation that there's a standard POSIX function for 
that: `strdup()`
   * `strdup()` depends on having a null terminator within the string being 
copied, yet it's unsafe to add a null terminator within that buffer because we 
don't know for certain there is space. Our choices then are:
 1. Use `strndup()` OR
 2. Use `malloc(length + 1)`, `memcpy(..., length)` the old buffer to the 
new buffer, and then add the null terminator to the new buffer.


This is an automated message from the Apache Git Service.

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

[GitBox]

mike-jumper commented on a change in pull request #228: GUACAMOLE-221: 
Implement guacd support for prompting
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r371009915
 
 

 ##
 File path: src/protocols/rdp/argv.c
 ##
 @@ -0,0 +1,192 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "config.h"
+#include "argv.h"
+#include "rdp.h"
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+/**
+ * All RDP connection settings which may be updated by unprivileged users
+ * through "argv" streams.
+ */
+typedef enum guac_rdp_argv_setting {
+
+/**
+ * The username of the connection.
+ */
+GUAC_RDP_ARGV_SETTING_USERNAME,
+
+/**
+ * The password to authenticate the connection.
+ */
+GUAC_RDP_ARGV_SETTING_PASSWORD,
+
+/**
+ * The domain to use for connection authentication.
+ */
+GUAC_RDP_ARGV_SETTING_DOMAIN,
+
+} guac_rdp_argv_setting;
+
+/**
+ * The value or current status of a connection parameter received over an
+ * "argv" stream.
+ */
+typedef struct guac_rdp_argv {
+
+/**
+ * The specific setting being updated.
+ */
+guac_rdp_argv_setting setting;
+
+/**
+ * Buffer space for containing the received argument value.
+ */
+char buffer[GUAC_RDP_ARGV_MAX_LENGTH];
+
+/**
+ * The number of bytes received so far.
+ */
+int length;
+
+} guac_rdp_argv;
+
+/**
+ * Handler for "blob" instructions which appends the data from received blobs
+ * to the end of the in-progress argument value buffer.
+ *
+ * @see guac_user_blob_handler
+ */
+static int guac_rdp_argv_blob_handler(guac_user* user,
+guac_stream* stream, void* data, int length) {
+
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+
+/* Calculate buffer size remaining, including space for null terminator,
+ * adjusting received length accordingly */
+int remaining = sizeof(argv->buffer) - argv->length - 1;
+if (length > remaining)
+length = remaining;
+
+/* Append received data to end of buffer */
+memcpy(argv->buffer + argv->length, data, length);
+argv->length += length;
+
+return 0;
+
+}
+
+/**
+ * Handler for "end" instructions which applies the changes specified by the
+ * argument value buffer associated with the stream.
+ *
+ * @see guac_user_end_handler
+ */
+static int guac_rdp_argv_end_handler(guac_user* user,
+guac_stream* stream) {
+
+guac_client* client = user->client;
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+
+/* Append null terminator to value */
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+argv->buffer[argv->length] = '\0';
 
 Review comment:
   Yep, definitely necessary. Nothing handling the received data as a string 
will be able to function safely without that null terminator.
   
   The beauty of `strndup(foo, length)` is that it will automatically add that 
null terminator and ensure that there is space for it within the 
newly-allocated buffer. Assuming the string within the buffer is indeed 
`length` characters long, `strndup(..., length)` would allocate buffer space of 
at least `length + 1` bytes.


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:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitBox]

mike-jumper commented on a change in pull request #228: GUACAMOLE-221: 
Implement guacd support for prompting
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r370984896
 
 

 ##
 File path: src/protocols/rdp/argv.c
 ##
 @@ -0,0 +1,192 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "config.h"
+#include "argv.h"
+#include "rdp.h"
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+/**
+ * All RDP connection settings which may be updated by unprivileged users
+ * through "argv" streams.
+ */
+typedef enum guac_rdp_argv_setting {
+
+/**
+ * The username of the connection.
+ */
+GUAC_RDP_ARGV_SETTING_USERNAME,
+
+/**
+ * The password to authenticate the connection.
+ */
+GUAC_RDP_ARGV_SETTING_PASSWORD,
+
+/**
+ * The domain to use for connection authentication.
+ */
+GUAC_RDP_ARGV_SETTING_DOMAIN,
+
+} guac_rdp_argv_setting;
+
+/**
+ * The value or current status of a connection parameter received over an
+ * "argv" stream.
+ */
+typedef struct guac_rdp_argv {
+
+/**
+ * The specific setting being updated.
+ */
+guac_rdp_argv_setting setting;
+
+/**
+ * Buffer space for containing the received argument value.
+ */
+char buffer[GUAC_RDP_ARGV_MAX_LENGTH];
+
+/**
+ * The number of bytes received so far.
+ */
+int length;
+
+} guac_rdp_argv;
+
+/**
+ * Handler for "blob" instructions which appends the data from received blobs
+ * to the end of the in-progress argument value buffer.
+ *
+ * @see guac_user_blob_handler
+ */
+static int guac_rdp_argv_blob_handler(guac_user* user,
+guac_stream* stream, void* data, int length) {
+
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+
+/* Calculate buffer size remaining, including space for null terminator,
+ * adjusting received length accordingly */
+int remaining = sizeof(argv->buffer) - argv->length - 1;
+if (length > remaining)
+length = remaining;
+
+/* Append received data to end of buffer */
+memcpy(argv->buffer + argv->length, data, length);
+argv->length += length;
+
+return 0;
+
+}
+
+/**
+ * Handler for "end" instructions which applies the changes specified by the
+ * argument value buffer associated with the stream.
+ *
+ * @see guac_user_end_handler
+ */
+static int guac_rdp_argv_end_handler(guac_user* user,
+guac_stream* stream) {
+
+guac_client* client = user->client;
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+
+/* Append null terminator to value */
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+argv->buffer[argv->length] = '\0';
 
 Review comment:
   If `buffer` points to a buffer of exactly `length` bytes, then 
`buffer[length]` is outside the buffer, not the last byte of the buffer.


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:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitBox]

mike-jumper commented on a change in pull request #228: GUACAMOLE-221: 
Implement guacd support for prompting
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r370970991
 
 

 ##
 File path: src/protocols/rdp/argv.c
 ##
 @@ -0,0 +1,192 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "config.h"
+#include "argv.h"
+#include "rdp.h"
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+/**
+ * All RDP connection settings which may be updated by unprivileged users
+ * through "argv" streams.
+ */
+typedef enum guac_rdp_argv_setting {
+
+/**
+ * The username of the connection.
+ */
+GUAC_RDP_ARGV_SETTING_USERNAME,
+
+/**
+ * The password to authenticate the connection.
+ */
+GUAC_RDP_ARGV_SETTING_PASSWORD,
+
+/**
+ * The domain to use for connection authentication.
+ */
+GUAC_RDP_ARGV_SETTING_DOMAIN,
+
+} guac_rdp_argv_setting;
+
+/**
+ * The value or current status of a connection parameter received over an
+ * "argv" stream.
+ */
+typedef struct guac_rdp_argv {
+
+/**
+ * The specific setting being updated.
+ */
+guac_rdp_argv_setting setting;
+
+/**
+ * Buffer space for containing the received argument value.
+ */
+char buffer[GUAC_RDP_ARGV_MAX_LENGTH];
+
+/**
+ * The number of bytes received so far.
+ */
+int length;
+
+} guac_rdp_argv;
+
+/**
+ * Handler for "blob" instructions which appends the data from received blobs
+ * to the end of the in-progress argument value buffer.
+ *
+ * @see guac_user_blob_handler
+ */
+static int guac_rdp_argv_blob_handler(guac_user* user,
+guac_stream* stream, void* data, int length) {
+
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+
+/* Calculate buffer size remaining, including space for null terminator,
+ * adjusting received length accordingly */
+int remaining = sizeof(argv->buffer) - argv->length - 1;
+if (length > remaining)
+length = remaining;
+
+/* Append received data to end of buffer */
+memcpy(argv->buffer + argv->length, data, length);
+argv->length += length;
+
+return 0;
+
+}
+
+/**
+ * Handler for "end" instructions which applies the changes specified by the
+ * argument value buffer associated with the stream.
+ *
+ * @see guac_user_end_handler
+ */
+static int guac_rdp_argv_end_handler(guac_user* user,
+guac_stream* stream) {
+
+guac_client* client = user->client;
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+
+/* Append null terminator to value */
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+argv->buffer[argv->length] = '\0';
 
 Review comment:
   Is there a guarantee that there will be space for this null terminator 
within `argv->buffer`?


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:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitBox]

mike-jumper commented on a change in pull request #228: GUACAMOLE-221: 
Implement guacd support for prompting
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r370970739
 
 

 ##
 File path: src/libguac/protocol.c
 ##
 @@ -961,6 +961,37 @@ int guac_protocol_send_rect(guac_socket* socket,
 
 }
 
+static int __guac_protocol_send_required(guac_socket* socket,
 
 Review comment:
   Please document this function.
   
   Also if sending a NULL-terminated array of strings as a series of 
instruction elements is common here (it looks like we've done this with at 
least `args` and `connect`, as well), I think it's worth considering whether we 
should extract the commonality so there is one internal function leveraged by 
all three, similar to how we have `__guac_socket_write_length_string()` and 
`__guac_socket_write_length_int()`.


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:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitBox]

mike-jumper commented on a change in pull request #228: GUACAMOLE-221: 
Implement guacd support for prompting
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r370971349
 
 

 ##
 File path: src/protocols/rdp/rdp.h
 ##
 @@ -166,6 +181,17 @@ typedef struct guac_rdp_client {
  * reallocated during reconnection.
  */
 pthread_mutex_t rdp_lock;
+
+/**
+ * Condition which is used when the pthread needs to wait for a
+ * particular condition to be met.
 
 Review comment:
   That particular condition needs to be pretty well defined, at least on our 
part, for waiting on this condition to make sense. If this condition and the 
associated flags are to be used for requesting arguments, for requesting 
credentials, etc. we can make this much clearer by specifically intending them 
for that use and documenting them as such.


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:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitBox]

mike-jumper commented on a change in pull request #228: GUACAMOLE-221: 
Implement guacd support for prompting
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r370971128
 
 

 ##
 File path: src/protocols/rdp/argv.c
 ##
 @@ -0,0 +1,192 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "config.h"
+#include "argv.h"
+#include "rdp.h"
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+/**
+ * All RDP connection settings which may be updated by unprivileged users
+ * through "argv" streams.
+ */
+typedef enum guac_rdp_argv_setting {
+
+/**
+ * The username of the connection.
+ */
+GUAC_RDP_ARGV_SETTING_USERNAME,
+
+/**
+ * The password to authenticate the connection.
+ */
+GUAC_RDP_ARGV_SETTING_PASSWORD,
+
+/**
+ * The domain to use for connection authentication.
+ */
+GUAC_RDP_ARGV_SETTING_DOMAIN,
+
+} guac_rdp_argv_setting;
+
+/**
+ * The value or current status of a connection parameter received over an
+ * "argv" stream.
+ */
+typedef struct guac_rdp_argv {
+
+/**
+ * The specific setting being updated.
+ */
+guac_rdp_argv_setting setting;
+
+/**
+ * Buffer space for containing the received argument value.
+ */
+char buffer[GUAC_RDP_ARGV_MAX_LENGTH];
+
+/**
+ * The number of bytes received so far.
+ */
+int length;
+
+} guac_rdp_argv;
+
+/**
+ * Handler for "blob" instructions which appends the data from received blobs
+ * to the end of the in-progress argument value buffer.
+ *
+ * @see guac_user_blob_handler
+ */
+static int guac_rdp_argv_blob_handler(guac_user* user,
+guac_stream* stream, void* data, int length) {
+
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+
+/* Calculate buffer size remaining, including space for null terminator,
+ * adjusting received length accordingly */
+int remaining = sizeof(argv->buffer) - argv->length - 1;
+if (length > remaining)
+length = remaining;
+
+/* Append received data to end of buffer */
+memcpy(argv->buffer + argv->length, data, length);
+argv->length += length;
+
+return 0;
+
+}
+
+/**
+ * Handler for "end" instructions which applies the changes specified by the
+ * argument value buffer associated with the stream.
+ *
+ * @see guac_user_end_handler
+ */
+static int guac_rdp_argv_end_handler(guac_user* user,
+guac_stream* stream) {
+
+guac_client* client = user->client;
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+
+/* Append null terminator to value */
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+argv->buffer[argv->length] = '\0';
+
+/* Apply changes to chosen setting */
+switch (argv->setting) {
+
+/* Update RDP username. */
+case GUAC_RDP_ARGV_SETTING_USERNAME:
+free(settings->username);
+settings->username = malloc(strlen(argv->buffer) * sizeof(char));
+strcpy(settings->username, argv->buffer);
 
 Review comment:
   `strdup()` could be easier than `malloc(strlen(...))` followed by 
`strcpy()`. That said, it looks like already have the length with 
`argv->length`, and so manually terminating the input, recalculating the length 
with `strlen()`, etc. feels superfluous.
   
   Perhaps `strndup(..., argv->length)` would solve everything?


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:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitBox]

mike-jumper commented on a change in pull request #228: GUACAMOLE-221: 
Implement guacd support for prompting
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r370971307
 
 

 ##
 File path: src/protocols/rdp/rdp.h
 ##
 @@ -48,6 +48,21 @@
 #include 
 #include 
 
+/**
+ * A flag for tracking if we are waiting conditionally on a username.
+ */
+#define GUAC_RDP_COND_FLAG_USERNAME 1
+
+/**
+ * A flag for tracking if we are waiting conditionally on a password.
+ */
+#define GUAC_RDP_COND_FLAG_PASSWORD 2
+
+/**
+ * A flag for tracking if we are waiting conditionally on a domain.
+ */
+#define GUAC_RDP_COND_FLAG_DOMAIN 3
 
 Review comment:
   Since 3 is represented in binary as `11`, it cannot be safely used as the 
value of a bitwise flag intended to represent a state independent of the other 
flags. They would all need to be powers of two. I think you meant 4 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:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitBox]

mike-jumper commented on a change in pull request #228: GUACAMOLE-221: 
Implement guacd support for prompting
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r370971377
 
 

 ##
 File path: src/protocols/ssh/ssh.h
 ##
 @@ -89,6 +89,12 @@ typedef struct guac_ssh_client {
  * Lock dictating access to the SSH terminal channel.
  */
 pthread_mutex_t term_channel_lock;
+
+/**
+ * Condition used when SSH client thread needs to wait for Guacamole
+ * client response.
 
 Review comment:
   Same here - I think the usage of this condition should much more specific 
than an arbitrary client response of some unknown kind. It would be better to 
be explicit in the naming and documentation of this and its corresponding flags.


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:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitBox]

mike-jumper commented on a change in pull request #228: GUACAMOLE-221: 
Implement guacd support for prompting
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r370971221
 
 

 ##
 File path: src/protocols/rdp/argv.c
 ##
 @@ -0,0 +1,192 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "config.h"
+#include "argv.h"
+#include "rdp.h"
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+/**
+ * All RDP connection settings which may be updated by unprivileged users
+ * through "argv" streams.
+ */
+typedef enum guac_rdp_argv_setting {
+
+/**
+ * The username of the connection.
+ */
+GUAC_RDP_ARGV_SETTING_USERNAME,
+
+/**
+ * The password to authenticate the connection.
+ */
+GUAC_RDP_ARGV_SETTING_PASSWORD,
+
+/**
+ * The domain to use for connection authentication.
+ */
+GUAC_RDP_ARGV_SETTING_DOMAIN,
+
+} guac_rdp_argv_setting;
+
+/**
+ * The value or current status of a connection parameter received over an
+ * "argv" stream.
+ */
+typedef struct guac_rdp_argv {
+
+/**
+ * The specific setting being updated.
+ */
+guac_rdp_argv_setting setting;
+
+/**
+ * Buffer space for containing the received argument value.
+ */
+char buffer[GUAC_RDP_ARGV_MAX_LENGTH];
+
+/**
+ * The number of bytes received so far.
+ */
+int length;
+
+} guac_rdp_argv;
+
+/**
+ * Handler for "blob" instructions which appends the data from received blobs
+ * to the end of the in-progress argument value buffer.
+ *
+ * @see guac_user_blob_handler
+ */
+static int guac_rdp_argv_blob_handler(guac_user* user,
+guac_stream* stream, void* data, int length) {
+
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+
+/* Calculate buffer size remaining, including space for null terminator,
+ * adjusting received length accordingly */
+int remaining = sizeof(argv->buffer) - argv->length - 1;
+if (length > remaining)
+length = remaining;
+
+/* Append received data to end of buffer */
+memcpy(argv->buffer + argv->length, data, length);
+argv->length += length;
+
+return 0;
+
+}
+
+/**
+ * Handler for "end" instructions which applies the changes specified by the
+ * argument value buffer associated with the stream.
+ *
+ * @see guac_user_end_handler
+ */
+static int guac_rdp_argv_end_handler(guac_user* user,
+guac_stream* stream) {
+
+guac_client* client = user->client;
+guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+guac_rdp_settings* settings = rdp_client->settings;
+
+/* Append null terminator to value */
+guac_rdp_argv* argv = (guac_rdp_argv*) stream->data;
+argv->buffer[argv->length] = '\0';
+
+/* Apply changes to chosen setting */
+switch (argv->setting) {
+
+/* Update RDP username. */
+case GUAC_RDP_ARGV_SETTING_USERNAME:
+free(settings->username);
+settings->username = malloc(strlen(argv->buffer) * sizeof(char));
+strcpy(settings->username, argv->buffer);
+rdp_client->rdp_cond_flags ^= GUAC_RDP_COND_FLAG_USERNAME;
 
 Review comment:
   Is the intent here to always set, always clear, or toggle the flag?


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:
us...@infra.apache.org


With regards,
Apache Git Services