From 7f5539930456d4a02a5d9bf479981b2282de4ad6 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Sun, 21 Feb 2021 14:15:17 -0800 Subject: [PATCH 1/3] GUACAMOLE-1174: Clarify behavior of URL parameter appending function. --- src/protocols/kubernetes/url.h | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/protocols/kubernetes/url.h b/src/protocols/kubernetes/url.h index 96e50982..0050e175 100644 --- a/src/protocols/kubernetes/url.h +++ b/src/protocols/kubernetes/url.h @@ -50,8 +50,11 @@ int guac_kubernetes_escape_url_component(char* output, int length, const char* str); /** - * Append the parameter to the endpoint path. - * Value within the path will be URL-escaped as necessary. + * Appends the given query parameter and value to the given buffer. If the + * buffer does not already contain the '?' character denoting the start of the + * query string, it will be added. If the buffer already contains a query + * string, a '&' character will be added before the new parameter. The + * parameter value will automatically be URL-escaped as necessary. * * @param buffer * The buffer which should receive the parameter. It could contain the endpoint path. @@ -61,7 +64,8 @@ int guac_kubernetes_escape_url_component(char* output, int length, * The number of bytes available in the given buffer. * * @param param_name - * The name of the parameter. + * The name of the parameter. If the parameter name contains characters + * with special meaning to URLs, it must already be URL-escaped. * * @param param_value * The value of the parameter. From c7935736da88709fbb390082cfdfbaba37ff094e Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Sun, 21 Feb 2021 14:42:08 -0800 Subject: [PATCH 2/3] GUACAMOLE-1174: Add unit tests for URL utility functions. --- configure.ac | 5 ++ src/protocols/kubernetes/.gitignore | 5 ++ src/protocols/kubernetes/Makefile.am | 1 + src/protocols/kubernetes/tests/Makefile.am | 66 ++++++++++++++++++ src/protocols/kubernetes/tests/url/append.c | 74 +++++++++++++++++++++ src/protocols/kubernetes/tests/url/escape.c | 72 ++++++++++++++++++++ 6 files changed, 223 insertions(+) create mode 100644 src/protocols/kubernetes/.gitignore create mode 100644 src/protocols/kubernetes/tests/Makefile.am create mode 100644 src/protocols/kubernetes/tests/url/append.c create mode 100644 src/protocols/kubernetes/tests/url/escape.c diff --git a/configure.ac b/configure.ac index 09985bbd..56025e9b 100644 --- a/configure.ac +++ b/configure.ac @@ -180,6 +180,10 @@ AC_SUBST([PULSE_INCLUDE], '-I$(top_srcdir)/src/pulse') AC_SUBST([COMMON_SSH_LTLIB], '$(top_builddir)/src/common-ssh/libguac_common_ssh.la') AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh') +# Kubernetes support +AC_SUBST([LIBGUAC_CLIENT_KUBERNETES_LTLIB], '$(top_builddir)/src/protocols/kubernetes/libguac-client-kubernetes.la') +AC_SUBST([LIBGUAC_CLIENT_KUBERNETES_INCLUDE], '-I$(top_srcdir)/src/protocols/kubernetes') + # RDP support AC_SUBST([LIBGUAC_CLIENT_RDP_LTLIB], '$(top_builddir)/src/protocols/rdp/libguac-client-rdp.la') AC_SUBST([LIBGUAC_CLIENT_RDP_INCLUDE], '-I$(top_srcdir)/src/protocols/rdp') @@ -1191,6 +1195,7 @@ AC_CONFIG_FILES([Makefile src/guaclog/man/guaclog.1 src/pulse/Makefile src/protocols/kubernetes/Makefile + src/protocols/kubernetes/tests/Makefile src/protocols/rdp/Makefile src/protocols/rdp/tests/Makefile src/protocols/ssh/Makefile diff --git a/src/protocols/kubernetes/.gitignore b/src/protocols/kubernetes/.gitignore new file mode 100644 index 00000000..9d4e2aec --- /dev/null +++ b/src/protocols/kubernetes/.gitignore @@ -0,0 +1,5 @@ + +# Auto-generated test runner and binary +_generated_runner.c +test_kubernetes + diff --git a/src/protocols/kubernetes/Makefile.am b/src/protocols/kubernetes/Makefile.am index f22e91f8..e9316ce4 100644 --- a/src/protocols/kubernetes/Makefile.am +++ b/src/protocols/kubernetes/Makefile.am @@ -27,6 +27,7 @@ AUTOMAKE_OPTIONS = foreign ACLOCAL_AMFLAGS = -I m4 lib_LTLIBRARIES = libguac-client-kubernetes.la +SUBDIRS = . tests libguac_client_kubernetes_la_SOURCES = \ argv.c \ diff --git a/src/protocols/kubernetes/tests/Makefile.am b/src/protocols/kubernetes/tests/Makefile.am new file mode 100644 index 00000000..ec0c3a8b --- /dev/null +++ b/src/protocols/kubernetes/tests/Makefile.am @@ -0,0 +1,66 @@ +# +# 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. +# +# NOTE: Parts of this file (Makefile.am) are automatically transcluded verbatim +# into Makefile.in. Though the build system (GNU Autotools) automatically adds +# its own license boilerplate to the generated Makefile.in, that boilerplate +# does not apply to the transcluded portions of Makefile.am which are licensed +# to you by the ASF under the Apache License, Version 2.0, as described above. +# + +AUTOMAKE_OPTIONS = foreign +ACLOCAL_AMFLAGS = -I m4 + +# +# Unit tests for Kubernetes support +# + +check_PROGRAMS = test_kubernetes +TESTS = $(check_PROGRAMS) + +test_kubernetes_SOURCES = \ + url/append.c \ + url/escape.c + +test_kubernetes_CFLAGS = \ + -Werror -Wall -pedantic \ + @LIBGUAC_CLIENT_KUBERNETES_INCLUDE@ \ + @LIBGUAC_INCLUDE@ + +test_kubernetes_LDADD = \ + @CUNIT_LIBS@ \ + @LIBGUAC_CLIENT_KUBERNETES_LTLIB@ + +# +# Autogenerate test runner +# + +GEN_RUNNER = $(top_srcdir)/util/generate-test-runner.pl +CLEANFILES = _generated_runner.c + +_generated_runner.c: $(test_kubernetes_SOURCES) + $(AM_V_GEN) $(GEN_RUNNER) $(test_kubernetes_SOURCES) > $@ + +nodist_test_kubernetes_SOURCES = \ + _generated_runner.c + +# Use automake's TAP test driver for running any tests +LOG_DRIVER = \ + env AM_TAP_AWK='$(AWK)' \ + $(SHELL) $(top_srcdir)/build-aux/tap-driver.sh + diff --git a/src/protocols/kubernetes/tests/url/append.c b/src/protocols/kubernetes/tests/url/append.c new file mode 100644 index 00000000..48e3601a --- /dev/null +++ b/src/protocols/kubernetes/tests/url/append.c @@ -0,0 +1,74 @@ +/* + * 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 "url.h" + +#include +#include +#include + +/** + * Verifies that guac_kubernetes_append_endpoint_param() correctly appends + * parameters to URLs that do not already have a query string. + */ +void test_url__append_no_query() { + + char url[256] = "http://example.net"; + + CU_ASSERT(!guac_kubernetes_append_endpoint_param(url, sizeof(url), "foo", "100% test value")); + CU_ASSERT_STRING_EQUAL(url, "http://example.net?foo=100%25%20test%20value"); + +} + +/** + * Verifies that guac_kubernetes_append_endpoint_param() correctly appends + * parameters to URLs that already have a query string. + */ +void test_url__append_existing_query() { + + char url[256] = "http://example.net?foo=test%20value"; + + CU_ASSERT(!guac_kubernetes_append_endpoint_param(url, sizeof(url), "foo2", "yet&another/test\\value")); + CU_ASSERT_STRING_EQUAL(url, "http://example.net?foo=test%20value&foo2=yet%26another%2Ftest%5Cvalue"); + +} + +/** + * Verifies that guac_kubernetes_append_endpoint_param() refuses to overflow + * the bounds of the provided buffer. + */ +void test_url__append_bounds() { + + char url[256]; + + /* Appending "?a=1" to the 18-character string "http://example.net" should + * fail for all buffer sizes with 22 bytes or less, with a 22-byte buffer + * lacking space for the null terminator */ + for (int length = 18; length <= 22; length++) { + strcpy(url, "http://example.net"); + printf("Testing buffer with length %i ...\n", length); + CU_ASSERT(guac_kubernetes_append_endpoint_param(url, length, "a", "1")); + } + + /* A 23-byte buffer should be sufficient */ + strcpy(url, "http://example.net"); + CU_ASSERT(!guac_kubernetes_append_endpoint_param(url, 23, "a", "1")); + +} + diff --git a/src/protocols/kubernetes/tests/url/escape.c b/src/protocols/kubernetes/tests/url/escape.c new file mode 100644 index 00000000..d0e1f535 --- /dev/null +++ b/src/protocols/kubernetes/tests/url/escape.c @@ -0,0 +1,72 @@ +/* + * 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 "url.h" + +#include +#include + +/** + * Verifies that guac_kubernetes_escape_url_component() correctly escapes + * characters that would otherwise have special meaning within URLs. + */ +void test_url__escape_special() { + + char value[256]; + + CU_ASSERT(!guac_kubernetes_escape_url_component(value, sizeof(value), "?foo%20bar\\1/2&3=4")); + CU_ASSERT_STRING_EQUAL(value, "%3Ffoo%2520bar%5C1%2F2%263%3D4"); + +} + +/** + * Verifies that guac_kubernetes_escape_url_component() leaves strings + * untouched if they contain no characters requiring escaping. + */ +void test_url__escape_nospecial() { + + char value[256]; + + CU_ASSERT(!guac_kubernetes_escape_url_component(value, sizeof(value), "potato")); + CU_ASSERT_STRING_EQUAL(value, "potato"); + +} + +/** + * Verifies that guac_kubernetes_escape_url_component() refuses to overflow the + * bounds of the provided buffer. + */ +void test_url__escape_bounds() { + + char value[256]; + + /* Escaping "?potato" (or "potato?") should fail for all buffer sizes with + * 9 bytes or less, with a 9-byte buffer lacking space for the null + * terminator */ + for (int length = 0; length <= 9; length++) { + printf("Testing buffer with length %i ...\n", length); + CU_ASSERT(guac_kubernetes_escape_url_component(value, length, "?potato")); + CU_ASSERT(guac_kubernetes_escape_url_component(value, length, "potato?")); + } + + /* A 10-byte buffer should be sufficient */ + CU_ASSERT(!guac_kubernetes_escape_url_component(value, 10, "?potato")); + +} + From a920932703ca4867f13f53ea9f7c8724802ee685 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Sun, 21 Feb 2021 14:55:25 -0800 Subject: [PATCH 3/3] GUACAMOLE-1174: Correct logic detecting truncation of appended parameter. The previous implementation passed `length - str_len` to `snprintf()`, yet compared the return value to `length`. This is incorrect, as `length` is not the buffer size provided to `snprintf()`. --- src/protocols/kubernetes/url.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/protocols/kubernetes/url.c b/src/protocols/kubernetes/url.c index 4bca0157..ec0e8807 100644 --- a/src/protocols/kubernetes/url.c +++ b/src/protocols/kubernetes/url.c @@ -123,10 +123,14 @@ int guac_kubernetes_append_endpoint_param(char* buffer, int length, char delimiter = '?'; if (qmark) delimiter = '&'; - /* Write the parameter to the buffer */ - int written; - written = snprintf(buffer + str_len, length - str_len, - "%c%s=%s", delimiter, param_name, escaped_param_value); + /* Advance to end of buffer, where the new parameter and delimiter need to + * be appended */ + buffer += str_len; + length -= str_len; + + /* Write the parameter and delimiter to the buffer */ + int written = snprintf(buffer, length, "%c%s=%s", delimiter, + param_name, escaped_param_value); /* The parameter was successfully added if it was written to the given * buffer without truncation */