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 <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:
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:
[email protected]
