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