necouchman commented on a change in pull request #228:
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r491255370
##########
File path: src/protocols/vnc/auth.c
##########
@@ -19,27 +19,100 @@
#include "config.h"
+#include "argv.h"
#include "auth.h"
#include "vnc.h"
+#include <guacamole/argv.h>
#include <guacamole/client.h>
+#include <guacamole/protocol.h>
+#include <guacamole/socket.h>
#include <rfb/rfbclient.h>
#include <rfb/rfbproto.h>
+#include <pthread.h>
+#include <string.h>
+
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:
Okay, so a few follow-up questions:
1) Do you know if there's a particular reason why the VNC settings use an
empty string instead of `NULL` to indicate that a value has not been provided?
I'm not familiar enough with the internals of VNC to know if this is required
by libvncclient or if the VNC servers expect empty strings rather than no value
at all? It looks like most (though certainly not all) of the other places
throughout the settings code among the various protocols use `NULL`. I'm
guessing there's history and reason?
2) The `settings->username` and `settings->password` value are freed by
`libvncclient`, but what about `creds`? I suppose that `settings` and all of
its members are going to be around as long as the connection is active, so
there's not really any risk that `settings->username` will get freed prior to
being used as part of `creds`, but what about the other way around. If I
remember my C memory handling correctly, doing the above
(`creds->userCredential.username = settings->username`) just points
`creds->userCredential.username` at the same spot in memory where
`settings->username` is pointed - what if `libvncclient` frees `creds` and its
members before the connection closes? Presumably that would have adverse
impacts on the continued ability for the client to read `settings->username`?
Or am I totally off in the weeds, here?
3) If `strdup` is used to copy these values, presumably `creds` would also
need to be cleaned up at some point - either within `libvncclient` or manually,
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:
[email protected]
