You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2020/06/27 20:32:09 UTC
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #228: GUACAMOLE-221: Implement guacd support for prompting
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 <guacamole/protocol.h>
+#include <guacamole/socket.h>
+#include <guacamole/user.h>
+
+#include <pthread.h>
+#include <stdlib.h>
+#include <string.h>
+
+/**
+ * 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:
Thoughts on replacing repeated use of this pattern with a convenience function at the libguac level? I can imagine something like `guac_user_receive_string()` that takes over the `guac_stream` and accepts a callback for the final value would result in a fair amount of code deletion.
##########
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);
+ guac_socket_flush(client->socket);
+
+ /* Wait for condition. */
+ pthread_cond_wait(&(rdp_client->rdp_credential_cond), &(rdp_client->rdp_credential_lock));
Review comment:
We'll need to be careful here - this will potentially block indefinitely. There are additional cases that aren't yet being handled that we need to consider:
* If the connection is closed while this is waiting, is there anything to ensure it stops waiting? Does the blockage need to stop prior to attempting to free FreeRDP structures?
* This lock and the condition need to be properly destroyed in the free handler. I see some new cleanup within the VNC free handler, but not for RDP.
* `pthread_cond_destroy()` is documented as potentially failing with `EBUSY` if another thread is using the condition with `pthread_cond_wait()`: https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_cond_destroy.html
There is failsafe code in guacd which will forcibly terminate the process if the connection closes yet fails to clean itself up in a reasonable time, but we should aim to avoid that needing to kick in.
##########
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);
+ guac_socket_flush(client->socket);
Review comment:
Similar to [`guac_client_supports_webp()`](https://github.com/apache/guacamole-server/blob/c10ceab7e80f9d6eae9ad98254bf97f41a0de112/src/libguac/guacamole/client.h#L706) and [`guac_user_supports_webp()`](https://github.com/apache/guacamole-server/blob/c10ceab7e80f9d6eae9ad98254bf97f41a0de112/src/libguac/guacamole/user.h#L837), I think we will need `guac_client_supports_required()` and `guac_user_supports_required()` so that implementations can avoid leveraging protocol features that aren't supported by a particular user or client.
As an older client will just silently ignore the unrecognized `require` instruction, an implementation relying on `require` would wait for an `argv` response that is doomed never to arrive.
I imagine this may involve some modification to `guac_user_info` to allow the protocol version to be stored.
##########
File path: src/common-ssh/ssh.c
##########
@@ -360,7 +360,7 @@ static int guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
/* Attempt authentication with username + password. */
if (user->password == NULL && common_session->credential_handler)
- user->password = common_session->credential_handler(client, "Password: ");
+ common_session->credential_handler(client, "password");
Review comment:
With parts of the various protocols now referencing their own parameters by name, I think we should define constants within those protocols for the parameters we will reference this way, to ensure that:
1. The exact string for any parameter we need to reference by name is defined and maintained in exactly one location.
2. Any usage of that string can be checked by the compiler.
##########
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:
Can these ever be the empty string? It looks to me that argument parsing will ensure that an omitted parameter will be represented by `NULL`:
https://github.com/apache/guacamole-server/blob/c10ceab7e80f9d6eae9ad98254bf97f41a0de112/src/protocols/rdp/settings.c#L760-L773
If the concern here is an empty `argv` stream, perhaps the handling of the stream should ensure that it consistently assigns `NULL` for the relevant setting in those cases, such that code further downstream need not be aware of those concerns?
##########
File path: src/protocols/rdp/rdp.h
##########
@@ -153,6 +168,25 @@ typedef struct guac_rdp_client {
*/
guac_common_list* available_svc;
+ /**
+ * Lock which is locked when one or more credentials are required to
+ * complete the connection, and unlocked when credentials have been
+ * provided by the client.
+ */
+ pthread_mutex_t rdp_credential_lock;
+
+ /**
+ * Condition which is used when the pthread needs to wait for one or more
+ * credentials to be provided by the client.
+ */
+ pthread_cond_t rdp_credential_cond;
+
+ /**
+ * Flags for tracking events related to the rdp_credential_cond
+ * pthread condition.
Review comment:
Recommend pointing to the specific flags with `@see`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org