diff --git a/README b/README index 8446d97dc2..2a277192c0 100644 --- a/README +++ b/README @@ -166,6 +166,12 @@ guacd currently takes several command-line options: if running in the foreground, the console. Legal values are debug, info, warning, and error. The default value is info. + -T MILLISECONDS + + Sets the maximum number of milliseconds that guacd will wait for the + next message from a connected client before closing the connection. + The value must be a positive integer and defaults to 15000. + -f Causes guacd to run in the foreground, rather than automatically forking into the background. @@ -182,4 +188,3 @@ Please report any bugs encountered by opening a new issue in the JIRA system hosted at: https://issues.apache.org/jira/browse/GUACAMOLE - diff --git a/configure.ac b/configure.ac index 9ba60102a0..8543dd6e79 100644 --- a/configure.ac +++ b/configure.ac @@ -1492,6 +1492,7 @@ AC_CONFIG_FILES([Makefile src/libguac/Makefile src/libguac/tests/Makefile src/guacd/Makefile + src/guacd/tests/Makefile src/guacd/man/guacd.8 src/guacd/man/guacd.conf.5 src/guacenc/Makefile diff --git a/src/guacd-docker/README.md b/src/guacd-docker/README.md index b6235a3340..b19cb822ff 100644 --- a/src/guacd-docker/README.md +++ b/src/guacd-docker/README.md @@ -38,9 +38,19 @@ Connecting to guacd from an application docker run --name some-app --link some-guacd:guacd -d application-that-uses-guacd +Configuring the client timeout +------------------------------ + +By default, guacd waits up to 15000 milliseconds for the next message from a +connected client before closing the connection. Set `CLIENT_TIMEOUT` to a +positive number of milliseconds to override this value: + + docker run --name some-guacd -d -e CLIENT_TIMEOUT=45000 guacamole/guacd + +An explicit `-T` command-line option takes precedence over `CLIENT_TIMEOUT`. + Reporting issues ================ Please report any bugs encountered by opening a new issue in [our JIRA](https://issues.apache.org/jira/browse/GUACAMOLE/). - diff --git a/src/guacd-docker/bin/entrypoint.sh b/src/guacd-docker/bin/entrypoint.sh index e7da4a341f..d99fa1fcb1 100755 --- a/src/guacd-docker/bin/entrypoint.sh +++ b/src/guacd-docker/bin/entrypoint.sh @@ -5,6 +5,12 @@ if [ -n "$GUACD_LOG_LEVEL" ]; then echo "WARNING: The GUACD_LOG_LEVEL environment variable has been deprecated in favor of the LOG_LEVEL environment variable. Please migrate your configuration when possible." >&2 fi +# Allow the client timeout to be overridden with CLIENT_TIMEOUT. Explicit +# command-line options take precedence over this environment variable. +if [ -n "$CLIENT_TIMEOUT" ]; then + set -- -T "$CLIENT_TIMEOUT" "$@" +fi + # Listen on 0.0.0.0:4822, logging messages at the info level. Allow log level # to be overridden with LOG_LEVEL, and other behavior to be overridden with # additional command-line options passed to Docker. diff --git a/src/guacd/Makefile.am b/src/guacd/Makefile.am index 72d6d9ec65..d859a3392c 100644 --- a/src/guacd/Makefile.am +++ b/src/guacd/Makefile.am @@ -28,6 +28,7 @@ AUTOMAKE_OPTIONS = foreign AM_CPPFLAGS = -include config.h sbin_PROGRAMS = guacd +SUBDIRS = . tests man_MANS = \ man/guacd.8 \ @@ -97,4 +98,3 @@ systemd/guacd.service: systemd/guacd.service.in -e 's,[@]systemduser[@],$(systemduser),g' \ < systemd/guacd.service.in > systemd/guacd.service endif - diff --git a/src/guacd/conf-args.c b/src/guacd/conf-args.c index a338e365f8..50f6174c66 100644 --- a/src/guacd/conf-args.c +++ b/src/guacd/conf-args.c @@ -33,7 +33,7 @@ int guacd_conf_parse_args(guacd_config* config, int argc, char** argv) { /* Parse arguments */ int opt; - while ((opt = getopt(argc, argv, "l:b:p:L:C:K:fv")) != -1) { + while ((opt = getopt(argc, argv, "l:b:p:L:T:C:K:fv")) != -1) { /* -l: Bind port */ if (opt == 'l') { @@ -77,6 +77,19 @@ int guacd_conf_parse_args(guacd_config* config, int argc, char** argv) { } + /* -T: Client timeout */ + else if (opt == 'T') { + + int timeout = guacd_parse_client_timeout(optarg); + if (timeout < 0) { + fprintf(stderr, "Client timeout must be a positive integer number of milliseconds no greater than %i.\n", GUACD_MAX_CLIENT_TIMEOUT); + return 1; + } + + config->client_timeout = timeout; + + } + #ifdef ENABLE_SSL /* -C SSL certificate */ else if (opt == 'C') { @@ -107,6 +120,7 @@ int guacd_conf_parse_args(guacd_config* config, int argc, char** argv) { " [-b LISTENADDRESS]" " [-p PIDFILE]" " [-L LEVEL]" + " [-T MILLISECONDS]" #ifdef ENABLE_SSL " [-C CERTIFICATE_FILE]" " [-K PEM_FILE]" @@ -122,4 +136,3 @@ int guacd_conf_parse_args(guacd_config* config, int argc, char** argv) { return 0; } - diff --git a/src/guacd/conf-file.c b/src/guacd/conf-file.c index aa8214c1e7..760df1fefb 100644 --- a/src/guacd/conf-file.c +++ b/src/guacd/conf-file.c @@ -61,6 +61,22 @@ static int guacd_conf_callback(const char* section, const char* param, const cha return 0; } + /* Client timeout */ + else if (strcmp(param, "client_timeout") == 0) { + + int timeout = guacd_parse_client_timeout(value); + + /* Invalid client timeout */ + if (timeout < 0) { + guacd_conf_parse_error = "Client timeout must be a positive integer number of milliseconds within the supported range"; + return 1; + } + + config->client_timeout = timeout; + return 0; + + } + } /* Options related to daemon startup */ @@ -183,6 +199,7 @@ guacd_config* guacd_conf_load(void) { /* Load defaults */ conf->bind_host = guac_strdup(GUACD_DEFAULT_BIND_HOST); conf->bind_port = guac_strdup(GUACD_DEFAULT_BIND_PORT); + conf->client_timeout = GUACD_DEFAULT_CLIENT_TIMEOUT; conf->pidfile = NULL; conf->foreground = 0; conf->print_version = 0; @@ -223,4 +240,3 @@ guacd_config* guacd_conf_load(void) { return conf; } - diff --git a/src/guacd/conf-parse.c b/src/guacd/conf-parse.c index af5fc45169..d2aac79a86 100644 --- a/src/guacd/conf-parse.c +++ b/src/guacd/conf-parse.c @@ -23,8 +23,36 @@ #include #include +#include +#include #include +int guacd_parse_client_timeout(const char* value) { + + char* end; + + /* Reject empty values, signs, whitespace, and all other non-digits */ + if (*value == '\0') + return -1; + + for (const char* current = value; *current != '\0'; current++) { + if (!isdigit((unsigned char) *current)) + return -1; + } + + errno = 0; + long timeout = strtol(value, &end, 10); + + /* Value must be a positive integer that can be safely converted to the + * microsecond timeout expected by libguac */ + if (errno == ERANGE || end == value || *end != '\0' + || timeout <= 0 || timeout > GUACD_MAX_CLIENT_TIMEOUT) + return -1; + + return (int) timeout; + +} + /* * Simple recursive descent parser for an INI-like conf file grammar. The * grammar is, roughly: @@ -532,4 +560,3 @@ int guacd_parse_log_level(const char* name) { return -1; } - diff --git a/src/guacd/conf-parse.h b/src/guacd/conf-parse.h index dc4b3ecabf..bad2968ffa 100644 --- a/src/guacd/conf-parse.h +++ b/src/guacd/conf-parse.h @@ -53,6 +53,19 @@ int guacd_parse_conf(guacd_param_callback* callback, char* buffer, int length, v */ int guacd_parse_log_level(const char* name); +/** + * Parses the given client timeout, returning the corresponding number of + * milliseconds, or -1 if the value is invalid or cannot safely be converted + * to the microsecond timeout expected by libguac. + * + * @param value + * The timeout value to parse, in milliseconds. + * + * @return + * The parsed positive timeout in milliseconds, or -1 if invalid. + */ +int guacd_parse_client_timeout(const char* value); + /** * Human-readable description of the current error, if any. */ @@ -65,4 +78,3 @@ extern char* guacd_conf_parse_error; extern char* guacd_conf_parse_error_location; #endif - diff --git a/src/guacd/conf.h b/src/guacd/conf.h index e65f1c9fb2..3c35f0dd09 100644 --- a/src/guacd/conf.h +++ b/src/guacd/conf.h @@ -22,6 +22,8 @@ #include +#include + /** * The default host that guacd should bind to, if no other host is explicitly * specified. @@ -34,6 +36,18 @@ */ #define GUACD_DEFAULT_BIND_PORT "4822" +/** + * The default number of milliseconds that guacd should wait for messages from + * a connected client before closing the connection. + */ +#define GUACD_DEFAULT_CLIENT_TIMEOUT 15000 + +/** + * The maximum client timeout, in milliseconds. The corresponding timeout in + * microseconds must fit within the signed integer accepted by libguac. + */ +#define GUACD_MAX_CLIENT_TIMEOUT (INT_MAX / 1000) + /** * The contents of a guacd configuration file. */ @@ -49,6 +63,12 @@ typedef struct guacd_config { */ char* bind_port; + /** + * The number of milliseconds to wait for messages from a connected client + * before closing the connection. + */ + int client_timeout; + /** * The file to write the PID in, if any. */ @@ -84,4 +104,3 @@ typedef struct guacd_config { } guacd_config; #endif - diff --git a/src/guacd/connection.c b/src/guacd/connection.c index cd5ca64e80..7e3f0bc82a 100644 --- a/src/guacd/connection.c +++ b/src/guacd/connection.c @@ -256,11 +256,16 @@ static int guacd_add_user(guacd_proc* proc, guac_parser* parser, guac_socket* so * The socket associated with the new connection that must be routed to * a new or existing process within the given map. * + * @param usec_timeout + * The number of microseconds to wait for the next message from the + * connected client before closing the connection. + * * @return * Zero if the connection was successfully routed, non-zero if routing has * failed. */ -static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) { +static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket, + int usec_timeout) { guac_parser* parser = guac_parser_alloc(); @@ -269,7 +274,7 @@ static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) { guac_error_message = NULL; /* Get protocol from select instruction */ - if (guac_parser_expect(parser, socket, GUACD_USEC_TIMEOUT, "select")) { + if (guac_parser_expect(parser, socket, usec_timeout, "select")) { /* Log error */ guacd_log_handshake_failure(); @@ -323,7 +328,7 @@ static int guacd_route_connection(guacd_proc_map* map, guac_socket* socket) { identifier); /* Create new process */ - proc = guacd_create_proc(identifier); + proc = guacd_create_proc(identifier, usec_timeout); new_process = 1; } @@ -397,6 +402,7 @@ void* guacd_connection_thread(void* data) { guacd_proc_map* map = params->map; int connected_socket_fd = params->connected_socket_fd; + int usec_timeout = params->usec_timeout; guac_socket* socket; @@ -423,7 +429,7 @@ void* guacd_connection_thread(void* data) { #endif /* Route connection according to Guacamole, creating a new process if needed */ - if (guacd_route_connection(map, socket)) + if (guacd_route_connection(map, socket, usec_timeout)) guac_socket_free(socket); guac_mem_free(params); diff --git a/src/guacd/connection.h b/src/guacd/connection.h index 53e9b4ab6a..bf8e9a157d 100644 --- a/src/guacd/connection.h +++ b/src/guacd/connection.h @@ -49,6 +49,12 @@ typedef struct guacd_connection_thread_params { */ int connected_socket_fd; + /** + * The number of microseconds to wait for messages from the connected + * client before closing the connection. + */ + int usec_timeout; + } guacd_connection_thread_params; /** @@ -117,4 +123,3 @@ typedef struct guacd_connection_io_thread_params { void* guacd_connection_io_thread(void* data); #endif - diff --git a/src/guacd/daemon.c b/src/guacd/daemon.c index d5df5829f1..ba0367e917 100644 --- a/src/guacd/daemon.c +++ b/src/guacd/daemon.c @@ -352,6 +352,7 @@ int main(int argc, char* argv[]) { /* Log start */ guacd_log(GUAC_LOG_INFO, "Guacamole proxy daemon (guacd) version " VERSION " started"); + guacd_log(GUAC_LOG_INFO, "Client timeout is %i milliseconds", config->client_timeout); /* Get addresses for binding */ if ((retval = getaddrinfo(config->bind_host, config->bind_port, @@ -576,6 +577,7 @@ int main(int argc, char* argv[]) { params->map = map; params->connected_socket_fd = connected_socket_fd; + params->usec_timeout = config->client_timeout * 1000; #ifdef ENABLE_SSL params->ssl_context = ssl_context; diff --git a/src/guacd/man/guacd.8.in b/src/guacd/man/guacd.8.in index d32e8ebde9..f20def9298 100644 --- a/src/guacd/man/guacd.8.in +++ b/src/guacd/man/guacd.8.in @@ -27,6 +27,7 @@ guacd \- Guacamole proxy daemon [\fB-l\fR \fIPORT\fR] [\fB-p\fR \fIPID FILE\fR] [\fB-L\fR \fILOG LEVEL\fR] +[\fB-T\fR \fIMILLISECONDS\fR] [\fB-C\fR \fICERTIFICATE FILE\fR] [\fB-K\fR \fIKEY FILE\fR] [\fB-f\fR] @@ -76,6 +77,12 @@ and The default value is .B info. .TP +\fB\-T\fR \fIMILLISECONDS\fR +Sets the maximum number of milliseconds that +.B guacd +will wait for the next message from a connected client before closing the +connection. The value must be a positive integer and defaults to 15000. +.TP \fB\-f\fR Causes .B guacd diff --git a/src/guacd/man/guacd.conf.5.in b/src/guacd/man/guacd.conf.5.in index 554bf649ef..7c83201692 100644 --- a/src/guacd/man/guacd.conf.5.in +++ b/src/guacd/man/guacd.conf.5.in @@ -101,6 +101,12 @@ Requires to bind to a specific port when listening for connections. By default, .B guacd will bind to port 4822. +.TP +\fBclient_timeout\fR \fB=\fR \fIMILLISECONDS\fR +Sets the maximum number of milliseconds that +.B guacd +will wait for the next message from a connected client before closing the +connection. The value must be a positive integer and defaults to 15000. . .SH DAEMON PARAMETERS .TP @@ -169,6 +175,7 @@ pid_file = /var/run/guacd.pid bind_host = localhost bind_port = 4822 +client_timeout = 15000 [ssl] diff --git a/src/guacd/proc.c b/src/guacd/proc.c index 3041834dad..61b07a6490 100644 --- a/src/guacd/proc.c +++ b/src/guacd/proc.c @@ -102,7 +102,7 @@ static void* guacd_user_thread(void* data) { user->owner = params->owner; /* Handle user connection from handshake until disconnect/completion */ - guac_user_handle_connection(user, GUACD_USEC_TIMEOUT); + guac_user_handle_connection(user, proc->usec_timeout); /* Stop client and prevent future users if all users are disconnected */ if (client->connected_users == 0) { @@ -550,7 +550,7 @@ static void guacd_close_inherited_fds(int keep_fd) { } -guacd_proc* guacd_create_proc(const char* protocol) { +guacd_proc* guacd_create_proc(const char* protocol, int usec_timeout) { int sockets[2]; @@ -571,6 +571,8 @@ guacd_proc* guacd_create_proc(const char* protocol) { return NULL; } + proc->usec_timeout = usec_timeout; + /* Associate new client */ proc->client = guac_client_alloc(); if (proc->client == NULL) { diff --git a/src/guacd/proc.h b/src/guacd/proc.h index 4ee749f0ab..497689080b 100644 --- a/src/guacd/proc.h +++ b/src/guacd/proc.h @@ -25,19 +25,6 @@ #include -/** - * The number of milliseconds to wait for messages in any phase before - * timing out and closing the connection with an error. - */ -#define GUACD_TIMEOUT 15000 - -/** - * The number of microseconds to wait for messages in any phase before - * timing out and closing the connection with an error. This is always - * equal to GUACD_TIMEOUT * 1000. - */ -#define GUACD_USEC_TIMEOUT (GUACD_TIMEOUT*1000) - /** * The number of seconds to wait for any particular guac_client instance * to be freed following disconnect. If the free operation does not complete @@ -76,6 +63,12 @@ typedef struct guacd_proc { */ guac_client* client; + /** + * The number of microseconds to wait for messages from each user before + * closing their connection. + */ + int usec_timeout; + } guacd_proc; /** @@ -87,12 +80,16 @@ typedef struct guacd_proc { * @param protocol * The protocol for which this process is client being created. * + * @param usec_timeout + * The number of microseconds to wait for messages from users of the + * created client before closing their connections. + * * @return * A newly-allocated process structure pointing to the file descriptor of * the background process specific to the specified protocol, or NULL of * the process could not be created. */ -guacd_proc* guacd_create_proc(const char* protocol); +guacd_proc* guacd_create_proc(const char* protocol, int usec_timeout); /** * Signals the given process to stop accepting new users and clean up. This @@ -104,4 +101,3 @@ guacd_proc* guacd_create_proc(const char* protocol); void guacd_proc_stop(guacd_proc* proc); #endif - diff --git a/src/guacd/tests/Makefile.am b/src/guacd/tests/Makefile.am new file mode 100644 index 0000000000..42c966c08f --- /dev/null +++ b/src/guacd/tests/Makefile.am @@ -0,0 +1,51 @@ +# +# 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. +# + +AUTOMAKE_OPTIONS = foreign + +AM_CPPFLAGS = -include config.h +ACLOCAL_AMFLAGS = -I m4 + +check_PROGRAMS = test_guacd +TESTS = $(check_PROGRAMS) + +test_guacd_SOURCES = \ + ../conf-parse.c \ + client-timeout.c + +test_guacd_CFLAGS = \ + -Werror -Wall -pedantic \ + -I$(top_srcdir)/src/guacd \ + @LIBGUAC_INCLUDE@ + +test_guacd_LDADD = \ + @CUNIT_LIBS@ + +GEN_RUNNER = $(top_srcdir)/util/generate-test-runner.pl +CLEANFILES = _generated_runner.c + +_generated_runner.c: $(test_guacd_SOURCES) + $(AM_V_GEN) $(GEN_RUNNER) $(test_guacd_SOURCES) > $@ + +nodist_test_guacd_SOURCES = \ + _generated_runner.c + +LOG_DRIVER = \ + env AM_TAP_AWK='$(AWK)' \ + $(SHELL) $(top_srcdir)/build-aux/tap-driver.sh diff --git a/src/guacd/tests/client-timeout.c b/src/guacd/tests/client-timeout.c new file mode 100644 index 0000000000..762339776b --- /dev/null +++ b/src/guacd/tests/client-timeout.c @@ -0,0 +1,58 @@ +/* + * 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 "conf.h" +#include "conf-parse.h" + +#include + +#include + +void test_client_timeout__accepts_supported_positive_milliseconds() { + + char maximum[32]; + snprintf(maximum, sizeof(maximum), "%i", GUACD_MAX_CLIENT_TIMEOUT); + + CU_ASSERT_EQUAL(guacd_parse_client_timeout("1"), 1); + CU_ASSERT_EQUAL(guacd_parse_client_timeout("15000"), 15000); + CU_ASSERT_EQUAL(guacd_parse_client_timeout("45000"), 45000); + CU_ASSERT_EQUAL(guacd_parse_client_timeout(maximum), + GUACD_MAX_CLIENT_TIMEOUT); + +} + +void test_client_timeout__rejects_unsafe_or_malformed_values() { + + char above_maximum[32]; + snprintf(above_maximum, sizeof(above_maximum), "%li", + (long) GUACD_MAX_CLIENT_TIMEOUT + 1); + + CU_ASSERT_EQUAL(guacd_parse_client_timeout(""), -1); + CU_ASSERT_EQUAL(guacd_parse_client_timeout("0"), -1); + CU_ASSERT_EQUAL(guacd_parse_client_timeout("-1"), -1); + CU_ASSERT_EQUAL(guacd_parse_client_timeout("+1"), -1); + CU_ASSERT_EQUAL(guacd_parse_client_timeout(" 1"), -1); + CU_ASSERT_EQUAL(guacd_parse_client_timeout("1 "), -1); + CU_ASSERT_EQUAL(guacd_parse_client_timeout("1.5"), -1); + CU_ASSERT_EQUAL(guacd_parse_client_timeout("45000ms"), -1); + CU_ASSERT_EQUAL(guacd_parse_client_timeout(above_maximum), -1); + CU_ASSERT_EQUAL(guacd_parse_client_timeout("999999999999999999999999"), + -1); + +}