From 81b665811501e6cbd5f77df4ebeda37405ea65b2 Mon Sep 17 00:00:00 2001 From: Allison Karlitskaya Date: Fri, 25 Oct 2019 13:18:53 +0200 Subject: [PATCH] tls: Use an explicit wsinstance factory Stop using systemd's dependency and template mechanisms to implicitly create template sockets on connection to the factory socket. Replace that with an explicit factory which takes the name of the instance to request creation of. This allows using the fingerprint of the client certificate as a deterministic instance name, which prevents us from having to keep a list of active instances in cockpit-tls. This will also allow for making the pam module more robust, because we can ensure that the invoking cgroup contains the correct key fingerprint in its name. The new cockpit-wsinstance-factory helper needs an update of the SELinux policy (https://github.com/fedora-selinux/selinux-policy-contrib/pull/161). Adjust it in the rpm `%post` until that lands in the distributions. Bind the instance socket and service together, so that when the service stops, it also cleans up the listening socket in systemd. This prevents an unbounded number of abandoned sockets in pid 1. For startup this doesn't really make a difference, as we never expect activating an instance socket and then not using it. There is no knob in systemd to effectively limit the number of cockpit-wsinstance-https@.socket instances, but the corresponding services can be limited by their containing slice. Introduce a system-cockpithttps.slice with reasonably generous limits and which the admin can tweak via an override in /etc. This will eventually be documented together with how to enable client certificates, but until then this isn't relevant in practice (there only ever will be one https instance). --- .gitignore | 2 + src/tls/Makefile-tls.am | 32 +- src/tls/connection.c | 315 ++++++++---------- src/tls/main.c | 14 - src/tls/server.c | 31 -- src/tls/server.h | 3 - src/tls/socket-activation-helper.c | 34 +- src/tls/socket-io.c | 308 +++++++++++++++++ src/tls/socket-io.h | 49 +++ src/tls/test-server.c | 5 +- src/tls/test-socket-activation-helper.sh | 32 +- src/tls/testing.h | 23 ++ src/tls/utils.h | 10 +- src/tls/wsinstance-factory.c | 153 +++++++++ src/tls/wsinstance-start.c | 64 ++++ src/ws/Makefile-ws.am | 1 + ...ckpit-wsinstance-https-factory@.service.in | 7 +- src/ws/cockpit-wsinstance-https@.service.in | 1 + src/ws/cockpit-wsinstance-https@.socket.in | 4 + src/ws/system-cockpithttps.slice | 9 + test/verify/check-connection | 50 ++- tools/cockpit.spec | 8 + tools/debian/cockpit-ws.install | 2 + 23 files changed, 894 insertions(+), 263 deletions(-) create mode 100644 src/tls/socket-io.c create mode 100644 src/tls/socket-io.h create mode 100644 src/tls/testing.h create mode 100644 src/tls/wsinstance-factory.c create mode 100644 src/tls/wsinstance-start.c create mode 100644 src/ws/system-cockpithttps.slice diff --git a/.gitignore b/.gitignore index 1533c995b..fa0e0dc00 100644 --- a/.gitignore +++ b/.gitignore @@ -93,6 +93,8 @@ depcomp /liblvmdbus.a /cockpit-desktop /cockpit-tls +/cockpit-wsinstance-factory +/wsinstance-start /cockpit-ws /cockpit-ws.8 /po/cockpit.pot diff --git a/src/tls/Makefile-tls.am b/src/tls/Makefile-tls.am index 6c41513c4..87feca4e4 100644 --- a/src/tls/Makefile-tls.am +++ b/src/tls/Makefile-tls.am @@ -1,7 +1,25 @@ -libexec_PROGRAMS += cockpit-tls +libexec_PROGRAMS += cockpit-tls cockpit-wsinstance-factory +noinst_PROGRAMS += wsinstance-start + +wsinstance_start_SOURCES = \ + src/tls/utils.h \ + src/tls/socket-io.h \ + src/tls/socket-io.c \ + src/tls/wsinstance-start.c \ + $(NULL) + +cockpit_wsinstance_factory_LDADD = -lsystemd +cockpit_wsinstance_factory_SOURCES = \ + src/tls/utils.h \ + src/tls/socket-io.h \ + src/tls/socket-io.c \ + src/tls/wsinstance-factory.c \ + $(NULL) cockpit_tls_SOURCES = \ src/tls/utils.h \ + src/tls/socket-io.h \ + src/tls/socket-io.c \ src/tls/connection.h \ src/tls/connection.c \ src/tls/server.h \ @@ -41,6 +59,9 @@ TLS_SCRIPT_TESTS = \ $(NULL) test_tls_connection_SOURCES = \ + src/tls/testing.h \ + src/tls/socket-io.h \ + src/tls/socket-io.c \ src/tls/connection.c \ src/tls/test-connection.c \ $(NULL) @@ -49,6 +70,8 @@ test_tls_connection_CFLAGS = $(TEST_CFLAGS) test_tls_connection_LDADD = $(TEST_LDADD) test_tls_server_SOURCES = \ + src/tls/socket-io.h \ + src/tls/socket-io.c \ src/tls/connection.c \ src/tls/server.c \ src/tls/test-server.c \ @@ -57,8 +80,13 @@ test_tls_server_SOURCES = \ test_tls_server_CFLAGS = $(TEST_CFLAGS) test_tls_server_LDADD = $(TEST_LDADD) -socket_activation_helper_SOURCES = src/tls/socket-activation-helper.c socket_activation_helper_CFLAGS = $(TEST_CFLAGS) +socket_activation_helper_SOURCES = \ + src/tls/testing.h \ + src/tls/socket-io.h \ + src/tls/socket-io.c \ + src/tls/socket-activation-helper.c \ + $(NULL) noinst_PROGRAMS += $(TLS_TESTS) socket-activation-helper EXTRA_DIST += $(TLS_SCRIPT_TESTS) diff --git a/src/tls/connection.c b/src/tls/connection.c index 536a08bbf..1b9a30bff 100644 --- a/src/tls/connection.c +++ b/src/tls/connection.c @@ -43,8 +43,8 @@ #include #include -#include #include +#include "socket-io.h" #include "utils.h" /* cockpit-tls TCP server state (singleton) */ @@ -64,18 +64,6 @@ typedef struct #endif } Buffer; - -/* TLS certificate → https ws instance socket name map */ -typedef struct HTTPSInstance { - gnutls_datum_t peer_cert; - char socket_name[30]; - struct HTTPSInstance *next; -} HTTPSInstance; - -static pthread_mutex_t https_instances_mutex = PTHREAD_MUTEX_INITIALIZER; -static HTTPSInstance *https_instances; - - /* a single TCP connection between the client (browser) and cockpit-tls */ typedef struct { int client_fd; @@ -357,180 +345,171 @@ buffer_read_from_tls (Buffer *self, assert (buffer_valid (self)); } -/** - * https_instance_new: Create a new cockpit-ws instance for given peer certificate - */ -static HTTPSInstance* -https_instance_new (const gnutls_datum_t *der) +static bool +request_dynamic_wsinstance (const char *fingerprint) { - struct sockaddr_un factory_sockaddr = { .sun_family = AF_UNIX }; - size_t len; - int r; + struct sockaddr_un addr; + bool status = false; + char reply[20]; int fd; - HTTPSInstance *inst; - r = snprintf (factory_sockaddr.sun_path, sizeof factory_sockaddr.sun_path, - "%s/https-factory.sock", parameters.wsinstance_sockdir); - assert (r < sizeof factory_sockaddr.sun_path); + debug (CONNECTION, "requesting dynamic wsinstance for %s:\n", fingerprint); - /* connect to factory */ - fd = socket (AF_UNIX, SOCK_STREAM, 0); - if (fd < 0) + fd = socket (AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); + if (fd == -1) { - warn ("https_instance_new: failed to create socket"); - return NULL; - } - if (connect (fd, (struct sockaddr *) &factory_sockaddr, sizeof factory_sockaddr) < 0) - { - warn ("https_instance_new: failed to connect to https factory socket"); - return NULL; - } - - inst = callocx (1, sizeof (HTTPSInstance)); - - /* read instance socket name until EOF */ - for (len = 0; len < sizeof inst->socket_name;) { - r = read (fd, inst->socket_name + len, (sizeof inst->socket_name) - len); - if (r < 0 && errno == EINTR) - continue; - if (r == 0) /* EOF */ - break; - len += r; - } - close (fd); - - if (r < 0 || len == 0 || len >= sizeof inst->socket_name) - { - if (r < 0) - warn ("https_instance_new: failed to read instance socket name from factory"); - else if (len == 0) - warnx ("https_instance_new: https instance factory did not send a socket name"); - else - errx (EXIT_FAILURE, "https_instance_new: https instance factory sent too long socket name (max %zu bytes)", sizeof inst->socket_name); - free (inst); - return NULL; - } - - /* clone DER certificate */ - if (der) - { - inst->peer_cert.size = der->size; - inst->peer_cert.data = mallocx (inst->peer_cert.size); - memcpy (inst->peer_cert.data, der->data, inst->peer_cert.size); + warn ("socket() failed"); + goto out; } - return inst; + sockaddr_printf (&addr, "%s/https-factory.sock", parameters.wsinstance_sockdir); + + debug (CONNECTION, " -> connecting to %s...", addr.sun_path); + if (connect (fd, (struct sockaddr *) &addr, sizeof addr) != 0) + { + warn ("connect(%s) failed", addr.sun_path); + goto out; + } + + /* send the fingerprint */ + debug (CONNECTION, " -> success; sending fingerprint..."); + assert (strlen (fingerprint) == FINGERPRINT_LENGTH); + if (!send_all (fd, fingerprint, FINGERPRINT_LENGTH, 5 * 1000000)) + goto out; + + debug (CONNECTION, " -> success; waiting for reply..."); + + /* wait for the systemd job status reply */ + if (!recv_alnum (fd, reply, sizeof reply, 30 * 1000000)) + goto out; + + debug (CONNECTION, " -> got reply '%s'...", reply); + status = strcmp (reply, "done") == 0; + +out: + debug (CONNECTION, " -> %s.", status ? "success" : "fail"); + + if (fd != -1) + close (fd); + + return status; } -/** - * https_instance_has_peer_cert: Check if that instance is for a given GnuTLS DER client certificate - * - * Returns true if either this ws instance has no client certificate and der is - * %NULL or empty, or if both certificates are identical. Otherwise returns false. - */ static bool -https_instance_has_peer_cert (HTTPSInstance *inst, const gnutls_datum_t *der) +get_peer_certificate_fingerprint (gnutls_session_t tls, + char *result_data, + size_t result_size) { - if (!der) - return inst->peer_cert.size == 0; - if (inst->peer_cert.size != der->size) - return false; - return memcmp (inst->peer_cert.data, der->data, der->size) == 0; -} + unsigned char digest_data[256 / 8]; + size_t digest_size = sizeof digest_data; + const gnutls_datum_t no_certificate = { NULL, 0 }; + const gnutls_datum_t *peer_certificate; + peer_certificate = gnutls_certificate_get_peers (tls, NULL); -/** - * connection_ws_socket_name: Get cockpit-ws socket name for this connection - * - * Every client certificate gets its own cockpit-ws instance for better - * isolation, plus one instance for "no certificate". For plain http there's - * two more instances for without and with TLS redirection. - */ -static const char* -connection_ws_socket_name (Connection *self) -{ - const gnutls_datum_t *peer_der; - HTTPSInstance *https_instance; + /* no clientcert → sha256sum('') */ + if (peer_certificate == NULL) + peer_certificate = &no_certificate; - if (parameters.x509_cred == NULL) - { - assert (!self->tls); - return "http.sock"; - } - if (!self->tls) - return "http-redirect.sock"; - - /* https */ - - pthread_mutex_lock (&https_instances_mutex); - - /* find existing ws instance for this peer cert; note that these will never go away on the systemd side during - * cockpit-tls' lifetime -- even if cockpit-ws idles out, it will go back to socket activation and get re-spawned on - * demand. So we don't need to bother with cleaning up the list. */ - peer_der = gnutls_certificate_get_peers (self->tls, NULL); - for (https_instance = https_instances; https_instance; https_instance = https_instance->next) - if (https_instance_has_peer_cert (https_instance, peer_der)) - { - debug (CONNECTION, "connection_ws_socket_name https: peer certificate handled by existing ws instance %s", https_instance->socket_name); - break; - } - - /* none? create a new one */ - if (!https_instance) - { - https_instance = https_instance_new (peer_der); - if (https_instance) - { - debug (CONNECTION, "ws_socket_name https: created new ws instance %s for new peer certificate", https_instance->socket_name); - https_instance->next = https_instances; - https_instances = https_instance; - } - } - - pthread_mutex_unlock (&https_instances_mutex); - - return https_instance ? https_instance->socket_name : NULL; -} - - -/** - * connection_init_ws: Connect to a cockpit-ws instance for a new Connection - */ -static bool -connection_init_ws (Connection *self) -{ - struct sockaddr_un sockaddr = { .sun_family = AF_UNIX }; - const char *sockname = connection_ws_socket_name (self); - int r; - - if (!sockname) + if (gnutls_fingerprint (GNUTLS_DIG_SHA256, peer_certificate, digest_data, &digest_size)) return false; - r = snprintf (sockaddr.sun_path, sizeof sockaddr.sun_path, - "%s/%s", parameters.wsinstance_sockdir, - sockname); - assert (r < sizeof sockaddr.sun_path); + assert (digest_size == sizeof digest_data); + assert (result_size == sizeof digest_data * 2 + 1); - debug (CONNECTION, "connection_init_ws: attempting to connect to socket %s", sockaddr.sun_path); + for (int i = 0; i < sizeof digest_data; i++) + { + int s = snprintf (result_data, result_size, "%02x", digest_data[i]); + assert (s == 2 && 2 < result_size); + result_data += 2; + result_size -= 2; + } - /* connect to ws instance */ + assert (result_size == 1); + assert (result_data[0] == '\0'); + + return true; +} + +static bool +connection_connect_to_dynamic_wsinstance (Connection *self) +{ + struct sockaddr_un addr; + char fingerprint[FINGERPRINT_LENGTH + 1]; + + assert (self->tls != NULL); + + if (!get_peer_certificate_fingerprint (self->tls, fingerprint, sizeof fingerprint)) + return false; + + sockaddr_printf (&addr, "%s/https@%s.sock", parameters.wsinstance_sockdir, fingerprint); + debug (CONNECTION, "Connecting dynamic https wsinstance %s:", addr.sun_path); + + /* fast path: the socket already exists, so we can just connect to it */ + if (connect (self->ws_fd, (struct sockaddr *) &addr, sizeof addr) == 0) + return true; + + if (errno != ENOENT && errno != ECONNREFUSED) + warn ("connect(%s) failed on the first attempt", addr.sun_path); + + debug (CONNECTION, " -> failed (%m). Requesting activation."); + /* otherwise, ask for the instance to be started */ + if (!request_dynamic_wsinstance (fingerprint)) + return false; + + /* ... and try one more time. */ + debug (CONNECTION, " -> trying again"); + if (connect (self->ws_fd, (struct sockaddr *) &addr, sizeof addr) != 0) + { + warn ("connect(%s) failed on the second attempt", addr.sun_path); + return false; + } + + /* otherwise, we're now connected */ + debug (CONNECTION, " -> success!"); + return true; +} + +static bool +connection_connect_to_static_wsinstance (Connection *self) +{ + struct sockaddr_un addr; + const char *base; + + assert (self->tls == NULL); + + if (parameters.x509_cred) + base = "http-redirect.sock"; /* server is expecting https connections */ + else + base = "http.sock"; /* server is expecting http connections */ + + sockaddr_printf (&addr, "%s/%s", parameters.wsinstance_sockdir, base); + debug (CONNECTION, "Connecting dynamic https wsinstance %s:", addr.sun_path); + + if (connect (self->ws_fd, (struct sockaddr *) &addr, sizeof addr) != 0) + { + warn ("connect(%s) failed", addr.sun_path); + return false; + } + + debug (CONNECTION, " -> success!"); + return true; +} + +static bool +connection_connect_to_wsinstance (Connection *self) +{ self->ws_fd = socket (AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); - if (self->ws_fd < 0) + if (self->ws_fd == -1) { warn ("failed to create cockpit-ws client socket"); return false; } - if (connect (self->ws_fd, (struct sockaddr *) &sockaddr, sizeof sockaddr) < 0) - { - /* cockpit-ws crashed? */ - warn ("failed to connect to cockpit-ws"); - return false; - } - - debug (CONNECTION, " connected."); - - return true; + if (self->tls) + return connection_connect_to_dynamic_wsinstance (self); + else + return connection_connect_to_static_wsinstance (self); } /** @@ -719,7 +698,7 @@ connection_thread_main (int fd) debug (CONNECTION, "New thread for fd %i", fd); - if (connection_handshake (&self) && connection_init_ws (&self)) + if (connection_handshake (&self) && connection_connect_to_wsinstance (&self)) connection_thread_loop (&self); debug (CONNECTION, "Thread for fd %i is going to exit now", fd); @@ -864,12 +843,4 @@ connection_cleanup (void) gnutls_certificate_free_credentials (parameters.x509_cred); parameters.x509_cred = NULL; } - - while (https_instances) - { - HTTPSInstance *i = https_instances; - https_instances = i->next; - gnutls_free (i->peer_cert.data); - free (i); - } } diff --git a/src/tls/main.c b/src/tls/main.c index e05b7ef78..3e8c6bd12 100644 --- a/src/tls/main.c +++ b/src/tls/main.c @@ -36,12 +36,10 @@ struct arguments { uint16_t port; bool no_tls; int idle_timeout; - const char *instance_factory_respond; }; #define OPT_NO_TLS 1000 #define OPT_IDLE_TIMEOUT 1001 -#define OPT_INSTANCE_FACTORY_RESPOND 1002 static int arg_parse_int (char *arg, struct argp_state *state, int min, int max, const char *error_msg) @@ -71,9 +69,6 @@ parse_opt (int key, char *arg, struct argp_state *state) case OPT_IDLE_TIMEOUT: arguments->idle_timeout = arg_parse_int (arg, state, 0, INT_MAX, "Invalid idle timeout"); break; - case OPT_INSTANCE_FACTORY_RESPOND: - arguments->instance_factory_respond = arg; - break; default: return ARGP_ERR_UNKNOWN; } @@ -84,7 +79,6 @@ static struct argp_option options[] = { {"no-tls", OPT_NO_TLS, 0, 0, "Don't use TLS" }, {"port", 'p', "PORT", 0, "Local port to bind to (9090 if unset)" }, {"idle-timeout", OPT_IDLE_TIMEOUT, "SECONDS", 0, "Time after which to exit if there are no connections; 0 to run forever (default: 90)" }, - {"instance-factory-respond", OPT_INSTANCE_FACTORY_RESPOND, "SOCKNAME", OPTION_HIDDEN, "cockpit-wsinstance-https-factory@.service helper" }, { 0 } }; @@ -103,17 +97,9 @@ main (int argc, char **argv) arguments.no_tls = false; arguments.port = 9090; arguments.idle_timeout = 90; - arguments.instance_factory_respond = NULL; argp_parse (&argp, argc, argv, 0, 0, &arguments); - /* are we being used as a helper for cockpit-wsinstance-https-factory@.service? */ - if (arguments.instance_factory_respond) - { - server_instance_factory_respond (arguments.instance_factory_respond); - return 0; - } - server_init ("/run/cockpit/wsinstance", arguments.idle_timeout, arguments.port); if (!arguments.no_tls) diff --git a/src/tls/server.c b/src/tls/server.c index ba83692fc..2e54bcd54 100644 --- a/src/tls/server.c +++ b/src/tls/server.c @@ -353,37 +353,6 @@ server_run (void) ; } -/** - * server_instance_factory_respond: https factory helper - * - * Implement src/ws/cockpit-wsinstance-https-factory@.service.in. This is just a glorified - * `/bin/sh -ec 'echo -n https@%i.sock >&3'`, but that violates the SELinux policy, as - * cockpit_ws_t cannot talk to such a unit (it would be initrc_t, not cockpit_ws_exec_t). - */ -void -server_instance_factory_respond (const char *socket_name) -{ - const char *env_listen_fds = secure_getenv ("LISTEN_FDS"); - size_t socket_name_len = strlen (socket_name); - int r; - - assert (socket_name); - assert (socket_name[0] != '\0'); - - /* sanity checks for systemd socket activation */ - if (!env_listen_fds || strcmp (env_listen_fds, "1") != 0 || !check_sd_listen_pid ()) - errx (EXIT_FAILURE, "invalid invocation; must be run from systemd socket-activated service"); - - do - r = write (SD_LISTEN_FDS_START, socket_name, socket_name_len); - while (r < 0 && errno == EINTR); - if (r < 0) - err (EXIT_FAILURE, "failed to write instance socket name to activated socket"); - if (r != socket_name_len) - errx (EXIT_FAILURE, "failed to write instance socket name to activated socket: wrote %i bytes, wanted %zu bytes", r, socket_name_len); - close (SD_LISTEN_FDS_START); -} - unsigned server_num_connections (void) { diff --git a/src/tls/server.h b/src/tls/server.h index a36f77b82..93758e1b7 100644 --- a/src/tls/server.h +++ b/src/tls/server.h @@ -35,9 +35,6 @@ server_run (void); void server_cleanup (void); -void -server_instance_factory_respond (const char *socket_name); - /* these are for unit tests only */ bool server_poll_event (int timeout); diff --git a/src/tls/socket-activation-helper.c b/src/tls/socket-activation-helper.c index a72e400b8..0e962cb4e 100644 --- a/src/tls/socket-activation-helper.c +++ b/src/tls/socket-activation-helper.c @@ -31,6 +31,8 @@ #include #include +#include "socket-io.h" +#include "testing.h" #include "utils.h" #define MAX_COCKPIT_WS_ARGS 5 @@ -42,8 +44,8 @@ static struct instance_type } instance_types[] = { {"https-factory.sock", {}}, /* treated specially */ /* support up to 2 ws instances; increase this if the unit test needs more */ - {"https@0.sock", {"--for-tls-proxy", "--port=0"}}, - {"https@1.sock", {"--for-tls-proxy", "--port=0"}}, + {"https@" SHA256_NIL ".sock", {"--for-tls-proxy", "--port=0"}}, + {"https@" CLIENT_CERT_FINGERPRINT ".sock", {"--for-tls-proxy", "--port=0"}}, {"http-redirect.sock", {"--proxy-tls-redirect", "--no-tls", "--port=0"}}, {"http.sock", {"--no-tls", "--port", "0"}}, }; @@ -52,7 +54,6 @@ static int socket_to_pid[N_ELEMENTS (instance_types)]; static struct pollfd ws_pollfds[N_ELEMENTS (instance_types)]; volatile sig_atomic_t terminated = 0; -unsigned https_instance_counter = 0; static void term (int signum) @@ -145,23 +146,30 @@ spawn_cockpit_ws (const char *ws_path, int fd, static void handle_https_factory (int listen_fd) { - char sock_name[20]; - int res; int fd = accept4 (listen_fd, NULL, NULL, SOCK_CLOEXEC); + char fingerprint[FINGERPRINT_LENGTH + 1]; + const char *result; + + debug (HELPER, "connection to https-factory.sock:"); if (fd < 0) err (EXIT_FAILURE, "accept connection to https-factory.sock"); - res = snprintf (sock_name, sizeof sock_name, "https@%u.sock", https_instance_counter++); - assert (res < sizeof sock_name); - debug (HELPER, "https factory: sending instance %s", sock_name); + debug (HELPER, " -> reading instance name... "); + if (!recv_alnum (fd, fingerprint, sizeof fingerprint, 10 * 1000000)) + errx (EXIT_FAILURE, "failed to read fingerprint"); - /* if the test needs more, increase the number of instances in instance_types */ - assert (https_instance_counter < 3); + debug (HELPER, " -> success: '%s'", fingerprint); + if (strcmp (fingerprint, SHA256_NIL) == 0) /* we check this value from the tests */ + result = "done"; + else + result = "fail"; + + debug (HELPER, " -> sending reply '%s'", result); + if (!send_all (fd, result, strlen (result), 10 * 1000000)) + errx (EXIT_FAILURE, "failed to write https-factory.sock response"); + debug (HELPER, " -> done."); - res = write (fd, sock_name, strlen (sock_name)); - if (res < 0) - err (EXIT_FAILURE, "failed to write https-factory.sock response"); close (fd); } diff --git a/src/tls/socket-io.c b/src/tls/socket-io.c new file mode 100644 index 000000000..5dbc9aa00 --- /dev/null +++ b/src/tls/socket-io.c @@ -0,0 +1,308 @@ +/* + * This file is part of Cockpit. + * + * Copyright (C) 2019 Red Hat, Inc. + * + * Cockpit is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation; either version 2.1 of the License, or + * (at your option) any later version. + * + * Cockpit is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Cockpit; If not, see . + */ + +#ifdef HAVE_CONFIG_H +#include +#endif + +#include "socket-io.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "utils.h" + +static uint64_t +get_elapsed_time (struct timespec *start) +{ + struct timespec now; + int r; + + r = clock_gettime (CLOCK_MONOTONIC, &now); + assert (r == 0); + + if (start->tv_sec == 0 && start->tv_nsec == 0) + *start = now; + + int64_t elapsed = ((int64_t) now.tv_sec - start->tv_sec) * 1000000 + + ((int64_t) now.tv_nsec - start->tv_nsec) / 1000; + + assert (elapsed >= 0); + + return elapsed; +} + +/** + * get_remaining_timeout: + * @start: a timespec struct, initially initialised to { 0, 0 } + * @timeout_remaining: out-value for timeout remaining, in microseconds + * @timeout_us: the total timeout value in microseconds + * + * Uses @start to keep track of how much time of an initial timeout is + * remaining. + * + * This is useful to keep track of multiple-syscall IO operations with + * one global timeout, in the presence of multiple read() or write() + * calls, poll(), and the possibility of EINTR. + * + * On the first call (when @start is filled with zeros), @start is + * initialised and @timeout_remaining will be set to the value of + * @timeout_us. On successive calls (which should usually have the same + * value of @timeout_us), smaller values will be returned in line with + * the passage of time, until there is no timeout remaining. + * + * Returns #true when there has been a non-negative value written into + * @timeout_remaining, and returns #false when the timeout has expired. + */ +bool +get_remaining_timeout (struct timespec *start, + uint64_t *timeout_remaining, + uint64_t timeout_us) +{ + uint64_t elapsed = get_elapsed_time (start); + + debug (SOCKET_IO, " -> %lld of %lld elapsed", (long long) elapsed, (long long) timeout_us); + + if (timeout_us < elapsed) + return false; + + *timeout_remaining = timeout_us - elapsed; + + return true; +} + +static int +wait_for_io (struct timespec *start, + int fd, + short events, + uint64_t timeout_us) +{ + struct pollfd pfd = { .fd = fd, .events = events }; + uint64_t remaining; + int r; + + debug (SOCKET_IO, "wait_for_io(%d, %u, %ju):", fd, (unsigned) events, (uintmax_t) timeout_us); + + if (!get_remaining_timeout (start, &remaining, timeout_us)) + return 0; + + debug (SOCKET_IO, " -> waiting for %jd", (uintmax_t) remaining); + + do + r = poll (&pfd, 1, (remaining + 999) / 1000); + while (r == -1 && errno == ENOENT); + + debug (SOCKET_IO, " -> result is %d/%s", r, r < 0 ? strerror (errno) : "-"); + + return r; +} + +/** + * recv_all: + * @fd: a file descriptor for a connected stream socket + * @buffer: a buffer + * @size: the size of @buffer + * @timeout: a timeout, in microseconds + * + * Attempts to read up to @size - 1 bytes from the connected stream + * socket @fd, followed by EOF. On success, a nul terminator is + * inserted after the last byte and the number of bytes read (which + * might be less than @size - 1) is returned, excluding the nul + * terminator. 0 is a valid result. + * + * On failure (socket errors, timeout, or simply too much data read + * without EOF), -1 is returned. + * + * This function is meant to be used with send_all() on the other side. + */ +static ssize_t +recv_all (int fd, + char *buffer, + size_t size, + int timeout) +{ + struct timespec start = { 0, 0 }; + size_t count = 0; + + debug (SOCKET_IO, "read_all(fd=%d, size=%zu, timeout=%d)", fd, size, timeout); + + /* We need to see recv() return 0 in order to know that we have EOF. + * In order to see that 0, we need to call recv() with a non-empty + * buffer. Conveniently, we can use the byte at the end of the buffer + * that we will write the nul terminator byte into. Without this + * extra byte, we'd need to have a separate throwaway variable and a + * separately-coded function call. + */ + while (count < size && wait_for_io (&start, fd, POLLIN, timeout) == 1) + { + ssize_t s = recv (fd, buffer + count, size - count, MSG_DONTWAIT); + debug (SOCKET_IO, " -> recv returned %zd/%m", s); + + if (s == -1) + { + if (errno == EINTR || errno == EAGAIN) + continue; + + warn ("recv_all() failed"); + return -1; + } + + if (s == 0) + { + /* EOF → success */ + debug (SOCKET_IO, " -> successfully received %zu bytes and EOF.", count); + buffer[count] = '\0'; + return count; + } + + count += s; + } + + /* either the buffer overflowed or we timed out */ + warnx ("recv_all() failed: buffer is full and no EOF received"); + return -1; +} + +/** + * recv_alnum: + * @fd: a file descriptor for a connected stream socket + * @buffer: a buffer + * @size: the size of @buffer + * @timeout: a timeout, in microseconds + * + * Attempts to read a non-empty alphanumeric string up to @size - 1 + * bytes from the connected stream socket @fd, followed by EOF. On + * success, a nul terminator is inserted after the last byte and true is + * returned. The empty string is not a valid result. + * + * On failure (socket errors, timeout, too much data read, no data read, + * or in case the data is not alphanumeric), false is returned. + */ +bool +recv_alnum (int fd, + char *buffer, + size_t size, + int timeout) +{ + ssize_t r; + size_t i; + + r = recv_all (fd, buffer, size, timeout); + + /* we need to have read at least one byte */ + if (r < 1) + return false; + + for (i = 0; i < r; i++) + if (!isalnum (buffer[i])) + return false; + + return true; +} + +/** + * send_all: + * @fd: a file descriptor for a connected stream socket + * @buffer: a buffer + * @size: the size of @buffer + * @timeout: a timeout, in microseconds + * + * Writes exactly @size bytes of @buffer to @fd, followed by EOF (ie: + * SHUT_WR). + * + * If all the bytes are written and the shutdown is successful, #true is + * returned. On failure (socket errors, or timeout) #false is returned. + * + * This function is meant to be used with recv_all() on the other side. + */ +bool +send_all (int fd, + const char *buffer, + size_t size, + int timeout) +{ + struct timespec start = { 0, 0 }; + size_t count = 0; + + while (count < size && wait_for_io (&start, fd, POLLOUT, timeout) == 1) + { + ssize_t s = send (fd, buffer + count, size - count, MSG_DONTWAIT | MSG_NOSIGNAL); + + if (s == -1) + { + if (errno == EINTR || errno == EAGAIN) + continue; + + warn ("send_all() failed"); + return false; + } + + count += s; + } + + if (count != size) + { + warnx ("send_all() timed out"); + return false; + } + + if (shutdown (fd, SHUT_WR) != 0) + { + warn ("send_all(): shutdown(SHUT_WR)"); + return false; + } + + debug (SOCKET_IO, " -> successfully sent all %zu bytes and EOF.", count); + return true; +} + +/** + * sockaddr_printf: + * @addr: a (probably uninitialised) struct sockaddr_in + * @format: a format for a filename + * @...: the arguments for @format + * + * Formats a string and uses it to initialise @addr for a unix socket at + * the resultant path (including filling in of the family field). + * + * If the format operation overflows the buffer in the sockaddr, + * execution is aborted. + */ +void +__attribute__((format(printf, 2, 3))) +sockaddr_printf (struct sockaddr_un *addr, + const char *format, + ...) +{ + va_list ap; + int r; + + va_start (ap, format); + addr->sun_family = AF_UNIX; + r = vsnprintf (addr->sun_path, sizeof addr->sun_path, format, ap); + assert (0 < r && r < sizeof addr->sun_path); + va_end (ap); +} diff --git a/src/tls/socket-io.h b/src/tls/socket-io.h new file mode 100644 index 000000000..bc5d2279e --- /dev/null +++ b/src/tls/socket-io.h @@ -0,0 +1,49 @@ +/* + * This file is part of Cockpit. + * + * Copyright (C) 2019 Red Hat, Inc. + * + * Cockpit is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation; either version 2.1 of the License, or + * (at your option) any later version. + * + * Cockpit is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Cockpit; If not, see . + */ + +#pragma once + +#include +#include +#include +#include +#include +#include + +bool +get_remaining_timeout (struct timespec *start, + uint64_t *timeout_remaining, + uint64_t timeout_us); + +bool +recv_alnum (int fd, + char *buffer, + size_t size, + int timeout); + +bool +send_all (int fd, + const char *buffer, + size_t size, + int timeout); + +void +sockaddr_printf (struct sockaddr_un *addr, + const char *format, + ...) __attribute__((format(printf, 2, 3))); diff --git a/src/tls/test-server.c b/src/tls/test-server.c index 1ca0b7f6b..9159ffeaf 100644 --- a/src/tls/test-server.c +++ b/src/tls/test-server.c @@ -34,6 +34,7 @@ #include #include "connection.h" +#include "testing.h" #include "server.h" #include "common/cockpittest.h" @@ -311,8 +312,8 @@ teardown (TestCase *tc, gconstpointer data) g_assert_cmpint (unlinkat (socket_dir_fd, "http.sock", 0), ==, 0); g_assert_cmpint (unlinkat (socket_dir_fd, "http-redirect.sock", 0), ==, 0); g_assert_cmpint (unlinkat (socket_dir_fd, "https-factory.sock", 0), ==, 0); - g_assert_cmpint (unlinkat (socket_dir_fd, "https@0.sock", 0), ==, 0); - g_assert_cmpint (unlinkat (socket_dir_fd, "https@1.sock", 0), ==, 0); + g_assert_cmpint (unlinkat (socket_dir_fd, "https@" SHA256_NIL ".sock", 0), ==, 0); + g_assert_cmpint (unlinkat (socket_dir_fd, "https@" CLIENT_CERT_FINGERPRINT ".sock", 0), ==, 0); g_assert_cmpint (unlinkat (socket_dir_fd, "ready", 0), ==, 0); close (socket_dir_fd); g_assert_cmpint (g_rmdir (tc->ws_socket_dir), ==, 0); diff --git a/src/tls/test-socket-activation-helper.sh b/src/tls/test-socket-activation-helper.sh index 750e1e78a..a889ac725 100755 --- a/src/tls/test-socket-activation-helper.sh +++ b/src/tls/test-socket-activation-helper.sh @@ -34,9 +34,9 @@ expect_curl() { fi } -# args: -expect_nc() { - OUT=$(nc --unixsock "$SOCKET_DIR/$1" +expect_start() { + OUT=$(./wsinstance-start "$1" "$SOCKET_DIR") if ! echo "$OUT" | grep -q "$2"; then echo "FAIL: output does not contain $2" >&2 echo "$OUT" >&2 @@ -44,6 +44,8 @@ expect_nc() { fi } +SHA256_CERT=fd1245619267040f6aa88d8071bbae3c99d99ac759fdfec99fcc1af4c28ba23c +SHA256_NIL="$(sha256sum < /dev/null | cut -c1-64)" expect_curl http.sock "$SUCCESS" # second call to existing instance @@ -57,23 +59,19 @@ echo "ok 1 http.sock" expect_curl http-redirect.sock "$REDIRECT" echo "ok 2 http-redirect.sock" +expect_start $SHA256_NIL "^done$" +echo "ok 3 https-factory/success" -expect_nc https-factory.sock "^https@0.sock$" -echo "ok 3 https-factory #0" +expect_start "junk" "^fail$" +echo "ok 4 https-factory/fail" - -expect_curl https@0.sock "$SUCCESS" +expect_curl https@$SHA256_NIL.sock "$SUCCESS" # second call to existing instance -expect_curl https@0.sock "$SUCCESS" +expect_curl https@$SHA256_NIL.sock "$SUCCESS" # wait for idle timeout sleep 2 -expect_curl https@0.sock "$SUCCESS" -echo "ok 4 https@0.sock" +expect_curl https@$SHA256_NIL.sock "$SUCCESS" +echo "ok 5 https@$SHA256_NIL.sock" - -expect_nc https-factory.sock "^https@1.sock$" -echo "ok 5 https-factory #1" - - -expect_curl https@1.sock "$SUCCESS" -echo "ok 6 https@1.sock" +expect_curl https@$SHA256_CERT.sock "$SUCCESS" +echo "ok 6 https@$SHA256_CERT.sock" diff --git a/src/tls/testing.h b/src/tls/testing.h new file mode 100644 index 000000000..7c6821620 --- /dev/null +++ b/src/tls/testing.h @@ -0,0 +1,23 @@ +/* + * This file is part of Cockpit. + * + * Copyright (C) 2019 Red Hat, Inc. + * + * Cockpit is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation; either version 2.1 of the License, or + * (at your option) any later version. + * + * Cockpit is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Cockpit; If not, see . + */ + +#pragma once + +#define SHA256_NIL "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" +#define CLIENT_CERT_FINGERPRINT "fd1245619267040f6aa88d8071bbae3c99d99ac759fdfec99fcc1af4c28ba23c" diff --git a/src/tls/utils.h b/src/tls/utils.h index 0ea8892dc..ec92e4963 100644 --- a/src/tls/utils.h +++ b/src/tls/utils.h @@ -25,11 +25,13 @@ #define DEBUG 0 /* messages can be disabled per-domain */ -#define DEBUG_POLL 1 -#define DEBUG_BUFFER 1 -#define DEBUG_IOVEC 1 +#define DEBUG_POLL 0 +#define DEBUG_BUFFER 0 +#define DEBUG_IOVEC 0 #define DEBUG_CONNECTION 1 #define DEBUG_SERVER 1 +#define DEBUG_FACTORY 1 +#define DEBUG_SOCKET_IO 1 /* socket-activation-helper.c */ #define DEBUG_HELPER 1 @@ -46,3 +48,5 @@ #define N_ELEMENTS(arr) (sizeof (arr) / sizeof ((arr)[0])) #define SD_LISTEN_FDS_START 3 /* sd_listen_fds(3) */ + +#define FINGERPRINT_LENGTH (64) /* sha256 hex */ diff --git a/src/tls/wsinstance-factory.c b/src/tls/wsinstance-factory.c new file mode 100644 index 000000000..89e616e35 --- /dev/null +++ b/src/tls/wsinstance-factory.c @@ -0,0 +1,153 @@ +/* + * This file is part of Cockpit. + * + * Copyright (C) 2019 Red Hat, Inc. + * + * Cockpit is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation; either version 2.1 of the License, or + * (at your option) any later version. + * + * Cockpit is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Cockpit; If not, see . + */ + +#ifdef HAVE_CONFIG_H +#include +#endif + +#include +#include +#include + +#include +#include + +#include "socket-io.h" +#include "utils.h" + +#define UNIT_MAX 256 + +static int +match_job_removed (sd_bus_message *message, + void *user_data, + sd_bus_error *error) +{ + const char **job_path = user_data; + const char *result; + const char *path; + + debug (FACTORY, "Received JobRemoved signal:"); + + if (sd_bus_message_read (message, "uoss", NULL, &path, NULL, &result) < 0) + return 0; + + debug (FACTORY, " -> path: %s, result: %s", path, result); + + if (!*job_path || strcmp (path, *job_path) != 0) + return 0; + + /* This is our job. */ + debug (FACTORY, " -> sending result."); + send_all (SD_LISTEN_FDS_START, result, strlen (result), 5 * 1000000); + *job_path = NULL; + + return 0; +} + +int +main (void) +{ + char fingerprint[FINGERPRINT_LENGTH + 1]; + sd_bus_error error = SD_BUS_ERROR_NULL; + sd_bus *bus = NULL; + char unit[UNIT_MAX + 1]; + sd_bus_message *reply = NULL; + const char *job_path = NULL; + char **fdnames; + int r; + + if (sd_listen_fds_with_names (false, &fdnames) != 1 || strcmp (fdnames[0], "connection") != 0) + errx (1, "Must be spawned from a systemd service on a socket with Accept=yes %s", fdnames[0]); + + if (!recv_alnum (SD_LISTEN_FDS_START, fingerprint, sizeof fingerprint, 10 * 1000000)) + errx (1, "Didn't receive fingerprint"); + + r = sd_bus_open_system (&bus); + if (r < 0) + errx (1, "Failed to connect to system bus: %s", strerror (-r)); + + /* We use the job_path variable to communicate with the match function + * in two ways: + * + * - we set it to the path of the job that we're waiting to exit so + * that the match function knows which signal is for us + * + * - once the job is removed, the match function clears the variable + * back to NULL. that's how we know when to stop waiting. + * + * In effect, the duration of job_path being set to non-NULL is more + * or less equal to the duration of the existance of a job object at + * that path. + */ + r = sd_bus_match_signal_async (bus, NULL, + "org.freedesktop.systemd1", "/org/freedesktop/systemd1", + "org.freedesktop.systemd1.Manager", "JobRemoved", + match_job_removed, NULL, &job_path); + if (r < 0) + errx (1, "Failed to install match rule: %s", strerror (-r)); + + /* can't fail, because fingerprint is small */ + r = snprintf (unit, sizeof unit, "cockpit-wsinstance-https@%s.socket", fingerprint); + assert (0 < r && r < sizeof unit); + + debug (FACTORY, "Requesting start of unit %s", unit); + r = sd_bus_call_method (bus, + "org.freedesktop.systemd1", "/org/freedesktop/systemd1", + "org.freedesktop.systemd1.Manager", "StartUnit", + &error, &reply, "ss", unit, "replace"); + if (r < 0) + errx (1, "Method call failed: %s", error.message); + + r = sd_bus_message_read (reply, "o", &job_path); + if (r < 0) + errx (1, "Invalid message response: %s", strerror (-r)); + + debug (FACTORY, " -> job is %s", job_path); + debug (FACTORY, "Waiting for signal."); + + do + r = sd_bus_process (bus, NULL); + while (r > 0); + if (r < 0) + errx (1, "sd_bus_process() failed: %s", strerror (-r)); + + struct timespec start = { 0, 0 }; + uint64_t remaining; + while (job_path && get_remaining_timeout (&start, &remaining, 20 * 1000000)) + { + debug (FACTORY, "sd_bus_wait(%llu)", (long long) remaining); + r = sd_bus_wait (bus, remaining); + if (r < 0) + errx (1, "Error while waiting for bus: %s", strerror (-r)); + + debug (FACTORY, "sd_bus_process():"); + do + r = sd_bus_process (bus, NULL); + while (r > 0); + if (r < 0) + errx (1, "sd_bus_process() failed: %s", strerror (-r)); + debug (FACTORY, " -> done."); + } + + sd_bus_message_unref (reply); + sd_bus_close (bus); + sd_bus_unref (bus); + + return 0; +} diff --git a/src/tls/wsinstance-start.c b/src/tls/wsinstance-start.c new file mode 100644 index 000000000..9ce437e24 --- /dev/null +++ b/src/tls/wsinstance-start.c @@ -0,0 +1,64 @@ +/* + * This file is part of Cockpit. + * + * Copyright (C) 2019 Red Hat, Inc. + * + * Cockpit is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation; either version 2.1 of the License, or + * (at your option) any later version. + * + * Cockpit is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Cockpit; If not, see . + */ + +#ifdef HAVE_CONFIG_H +#include +#endif + +#include +#include +#include +#include + +#include "socket-io.h" + +int +main (int argc, + char **argv) +{ + const char *wsinstance_sockdir = "/run/cockpit/wsinstance"; + struct sockaddr_un addr; + char result[20]; + int fd; + + if (argc != 2 && argc != 3) + errx (1, "usage: ./wsinstance-start [instanceid] [wsinstance_sockdir]"); + + if (argc == 3) + wsinstance_sockdir = argv[2]; + + fd = socket (AF_UNIX, SOCK_STREAM, 0); + if (fd == -1) + err (1, "Couldn't create AF_UNIX socket"); + + sockaddr_printf (&addr, "%s/https-factory.sock", wsinstance_sockdir); + + if (connect (fd, (struct sockaddr *) &addr, sizeof addr) != 0) + err (1, "Couldn't connect to factory socket"); + + if (!send_all (fd, argv[1], strlen (argv[1]), 50 * 1000000)) + errx (1, "Couldn't send instance name"); + + if (!recv_alnum (fd, result, sizeof result, 30 * 1000000)) + errx (1, "Failed to receive result"); + + printf ("%s\n", result); + + return 0; +} diff --git a/src/ws/Makefile-ws.am b/src/ws/Makefile-ws.am index b050ba15b..009130941 100644 --- a/src/ws/Makefile-ws.am +++ b/src/ws/Makefile-ws.am @@ -178,6 +178,7 @@ systemd_units_in = \ $(NULL) nodist_systemdunit_DATA += $(patsubst %.in,%,$(systemd_units_in)) +dist_systemdunit_DATA += src/ws/system-cockpithttps.slice firewalldir = $(prefix)/lib/firewalld/services firewall_DATA = src/ws/cockpit.xml diff --git a/src/ws/cockpit-wsinstance-https-factory@.service.in b/src/ws/cockpit-wsinstance-https-factory@.service.in index 2045abbbc..ede2758a7 100644 --- a/src/ws/cockpit-wsinstance-https-factory@.service.in +++ b/src/ws/cockpit-wsinstance-https-factory@.service.in @@ -1,10 +1,7 @@ [Unit] Description=Cockpit Web Service https instance factory -Requires=cockpit-wsinstance-https@%i.socket -After=cockpit-wsinstance-https@%i.socket Documentation=man:cockpit-ws(8) [Service] -ExecStart=@libexecdir@/cockpit-tls --instance-factory-respond https@%i.sock -User=@user@ -Group=@group@ +ExecStart=@libexecdir@/cockpit-wsinstance-factory +User=root diff --git a/src/ws/cockpit-wsinstance-https@.service.in b/src/ws/cockpit-wsinstance-https@.service.in index 7a6e09a5f..7837467fb 100644 --- a/src/ws/cockpit-wsinstance-https@.service.in +++ b/src/ws/cockpit-wsinstance-https@.service.in @@ -4,6 +4,7 @@ BindsTo=cockpit.service Documentation=man:cockpit-ws(8) [Service] +Slice=system-cockpithttps.slice ExecStart=@libexecdir@/cockpit-ws --for-tls-proxy --port=0 User=@wsinstanceuser@ Group=@wsinstancegroup@ diff --git a/src/ws/cockpit-wsinstance-https@.socket.in b/src/ws/cockpit-wsinstance-https@.socket.in index b20057350..5e1922f57 100644 --- a/src/ws/cockpit-wsinstance-https@.socket.in +++ b/src/ws/cockpit-wsinstance-https@.socket.in @@ -1,6 +1,10 @@ [Unit] Description=Socket for Cockpit Web Service https instance %I BindsTo=cockpit.service +# clean up the socket after the service exits, to prevent fd leak +# this also effectively prevents a DoS by starting arbitrarily many sockets, as +# the services are resource-limited by system-cockpithttps.slice +BindsTo=cockpit-wsinstance-https@%i.service Documentation=man:cockpit-ws(8) [Socket] diff --git a/src/ws/system-cockpithttps.slice b/src/ws/system-cockpithttps.slice new file mode 100644 index 000000000..ea2400277 --- /dev/null +++ b/src/ws/system-cockpithttps.slice @@ -0,0 +1,9 @@ +[Unit] +Description=Resource limits for all cockpit-ws-https@.service instances + +[Slice] +# each instance contains cockpit-ws (with a few threads) and a short-lived +# cockpit-session; the actual user sessions live in user.slice +TasksMax=200 +MemoryHigh=75% +MemoryMax=90% diff --git a/test/verify/check-connection b/test/verify/check-connection index 7e79f9407..4755cb951 100755 --- a/test/verify/check-connection +++ b/test/verify/check-connection @@ -234,11 +234,59 @@ class TestConnection(MachineCase): self.assertIn("HTTP/1.1 200 OK", m.execute("curl --silent -k --head https://127.0.0.1:9090")) expect_actives(True, True, ["http-redirect"], 1) + # instance service+socket going away does not confuse cockpit-tls' bookkeeping + m.execute("systemctl stop cockpit-wsinstance-https@*.service cockpit-wsinstance-https@*.socket") + expect_actives(True, True, ["http-redirect"], 0) + self.assertIn("HTTP/1.1 200 OK", m.execute("curl --silent --show-error -k --head https://127.0.0.1:9090")) + expect_actives(True, True, ["http-redirect"], 1) + # sockets are inaccessible to users, only to cockpit-tls for s in ["http.sock", "https-factory.sock"]: - out = m.execute("su -c '! nc --unixsock /run/cockpit/wsinstance/%s 2>&1 || exit 1' admin" % s) + out = m.execute("su -c '! nc -U /run/cockpit/wsinstance/%s 2>&1 || exit 1' admin" % s) self.assertIn("Permission denied", out) + @skipImage("Atomic doesn't use systemd units", "fedora-atomic") + @skipImage("Introduced in PR #12972", "rhel-8-1-distropkg") + def testHttpsInstanceDoS(self): + m = self.machine + # prevent generating core dump artifacts + m.execute("echo core > /proc/sys/kernel/core_pattern") + m.start_cockpit(tls=True) + + # some netcat versions need an explicit shutdown option, others default to shutting down and don't have -N + n_opt = "-N" if "-N" in m.execute("nc -h 2>&1") else "" + + self.assertIn("HTTP/1.1 200 OK", m.execute("curl --silent -k --head https://127.0.0.1:9090")) + + # number of https instances is bounded (DoS prevention) + # with MaxTasks=200 und 2 threads per ws instance we should have a + # rough limit of 100 instances, so at some point curl should start failing + m.execute("su -s /bin/sh -c 'RC=1; for i in `seq 120`; do " + " echo -n $i | nc %s -U /run/cockpit/wsinstance/https-factory.sock;" + " curl --silent --head --max-time 5 --unix /run/cockpit/wsinstance/https@$i.sock http://dummy > /dev/null || RC=0; " + "done; exit $RC' cockpit-ws" % n_opt) + + for type_ in ["socket", "service"]: + active = int(m.execute("systemctl --no-legend list-units -t %s --state=active " + "'cockpit-wsinstance-https@*' | wc -l" % type_).strip()) + self.assertGreater(active, 50) + self.assertLess(active, 110) + failed = int(m.execute("systemctl --no-legend list-units --state=failed 'cockpit-wsinstance-https@*' | wc -l").strip()) + self.assertGreater(failed, 0) + self.assertLess(failed, 60) # services and sockets + + self.allow_journal_messages(".*cockpit-ws.*dumped core.*") + self.allow_journal_messages(".*Error creating thread: Resource temporarily unavailable.*") + + # initial instance still works + self.assertIn("HTTP/1.1 200 OK", m.execute("curl --silent --show-error -k --head https://127.0.0.1:9090")) + + # can launch new instances after freeing up some old ones + m.execute("systemctl stop cockpit-wsinstance-https@30 cockpit-wsinstance-https@31 cockpit-wsinstance-https@32") + m.execute("echo -n new | nc %s -U /run/cockpit/wsinstance/https-factory.sock" % n_opt) + out = m.execute("curl --silent --show-error --head --unix /run/cockpit/wsinstance/https@new.sock http://dummy") + self.assertIn("HTTP/1.1 200 OK", out) + def testTls(self): m = self.machine b = self.browser diff --git a/tools/cockpit.spec b/tools/cockpit.spec index 1f29ddbe4..5d3ff13fa 100644 --- a/tools/cockpit.spec +++ b/tools/cockpit.spec @@ -421,10 +421,12 @@ The Cockpit Web Service listens on the network, and authenticates users. %{_unitdir}/cockpit-wsinstance-https-factory@.service %{_unitdir}/cockpit-wsinstance-https@.socket %{_unitdir}/cockpit-wsinstance-https@.service +%{_unitdir}/system-cockpithttps.slice %{_prefix}/%{__lib}/tmpfiles.d/cockpit-tempfiles.conf %{_sbindir}/remotectl %{_libdir}/security/pam_ssh_add.so %{_libexecdir}/cockpit-ws +%{_libexecdir}/cockpit-wsinstance-factory %{_libexecdir}/cockpit-tls %{_libexecdir}/cockpit-desktop %attr(4750, root, cockpit-wsinstance) %{_libexecdir}/cockpit-session @@ -469,6 +471,12 @@ EOF rm -rf "$tmp" fi %endif +%if 0%{?rhel} || 0%{?fedora} +# HACK: SELinux policy adjustment for cockpit-tls; see https://github.com/fedora-selinux/selinux-policy-contrib/pull/161 + echo "Applying SELinux policy change for cockpit-wsinstance-factory..." + semanage fcontext -a /usr/libexec/cockpit-wsinstance-factory -t cockpit_ws_exec_t + restorecon /usr/libexec/cockpit-wsinstance-factory +%endif %preun ws %systemd_preun cockpit.socket diff --git a/tools/debian/cockpit-ws.install b/tools/debian/cockpit-ws.install index a55b0f94b..baedd91d5 100644 --- a/tools/debian/cockpit-ws.install +++ b/tools/debian/cockpit-ws.install @@ -13,10 +13,12 @@ lib/systemd/system/cockpit-wsinstance-https-factory@.service lib/systemd/system/cockpit-wsinstance-https-factory.socket lib/systemd/system/cockpit-wsinstance-https@.service lib/systemd/system/cockpit-wsinstance-https@.socket +lib/systemd/system/system-cockpithttps.slice lib/*/security/pam_ssh_add.so usr/lib/tmpfiles.d/cockpit-tempfiles.conf usr/lib/cockpit/cockpit-session usr/lib/cockpit/cockpit-ws +usr/lib/cockpit/cockpit-wsinstance-factory usr/lib/cockpit/cockpit-tls usr/lib/cockpit/cockpit-desktop usr/lib/firewalld/services/cockpit.xml