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 <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++;
+ }
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 <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:
Previously, there was no `strdup()` of the credentials stored within
`creds->userCredential`:
https://github.com/apache/guacamole-server/blob/382d72a26a04008aa936680deed00e3a31086b1e/src/protocols/vnc/auth.c#L40-L41
It looks like this is intentional, with both `settings->username` and
`settings->password` being freed by libvncclient:
https://github.com/apache/guacamole-server/blob/382d72a26a04008aa936680deed00e3a31086b1e/src/protocols/vnc/settings.c#L393-L399
##########
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:
Looks good, but we should add a unit test for this.
##########
File path: src/protocols/rdp/rdp.c
##########
@@ -227,10 +236,69 @@ 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 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++;
+ }
Review comment:
If an administrator provides:
| Parameter | Value |
| --------- | ----- |
| `domain` | (blank) |
| `username` | `foo` |
| `password` | (blank) |
I think the domain is effectively specified - the administrator intends that
the user log in to the default domain / local machine, and that the user should
be prompted for their password. I doubt that the administrator intends that the
user be prompted for their domain and password, with only the username set in
stone.
Perhaps we should prompt for the domain _only_ if the username is also blank?
----------------------------------------------------------------
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]
