BUG/MINOR: quic: Wrong cluster secret initialization

The function generate_random_cluster_secret() which initializes the cluster secret
when not supplied by configuration is buggy. There 1/256 that the cluster secret
string is empty.

To fix this, one stores the cluster as a reduced size first 128 bits of its own
SHA1 (160 bits) digest, if defined by configuration. If this is not the case, it
is initialized with a 128 bits random value. Furthermore, thus the cluster secret
is always initialized.

As the cluster secret is always initialized, there are several tests which
are for now on useless. This patch removes such tests (if(global.cluster_secret))
in the QUIC code part and at parsing time: no need to check that a cluster
secret was initialized with "quic-force-retry" option.

Must be backported as far as 2.6.

include/haproxy/global-t.h

@@ -85,6 +85,8 @@
 #define GTUNE_LISTENER_MQ_OPT    (1<<28)
 #define GTUNE_LISTENER_MQ_ANY    (GTUNE_LISTENER_MQ_FAIR | GTUNE_LISTENER_MQ_OPT)
 
+extern int cluster_secret_isset; /* non zero means a cluster secret was initiliazed */
+
 /* SSL server verify mode */
 enum {
 	SSL_SERVER_VERIFY_NONE = 0,
@@ -139,7 +141,7 @@ struct global {
 	char *log_send_hostname;   /* set hostname in syslog header */
 	char *server_state_base;   /* path to a directory where server state files can be found */
 	char *server_state_file;   /* path to the file where server states are loaded from */
-	char *cluster_secret;      /* Secret defined as ASCII string */
+	unsigned char cluster_secret[16]; /* 128 bits of an SHA1 digest of a secret defined as ASCII string */
 	struct {
 		int maxpollevents; /* max number of poll events at once */
 		int maxaccept;     /* max number of consecutive accept() */

src/cfgparse-global.c

@@ -11,6 +11,8 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include <import/sha1.h>
+
 #include <haproxy/buf.h>
 #include <haproxy/cfgparse.h>
 #ifdef USE_CPU_AFFINITY
@@ -23,6 +25,8 @@
 #include <haproxy/protocol.h>
 #include <haproxy/tools.h>
 
+int cluster_secret_isset;
+
 /* some keywords that are still being parsed using strcmp() and are not
  * registered anywhere. They are used as suggestions for mistyped words.
  */
@@ -514,6 +518,9 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm)
 		global.tune.options &= GTUNE_USE_FAST_FWD;
 	}
 	else if (strcmp(args[0], "cluster-secret") == 0) {
+		blk_SHA_CTX sha1_ctx;
+		unsigned char sha1_out[20];
+
 		if (alertif_too_many_args(1, file, linenum, args, &err_code))
 			goto out;
 		if (*args[1] == 0) {
@@ -521,13 +528,18 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm)
 			err_code |= ERR_ALERT | ERR_FATAL;
 			goto out;
 		}
-		if (global.cluster_secret != NULL) {
+		if (cluster_secret_isset) {
 			ha_alert("parsing [%s:%d] : '%s' already specified. Continuing.\n", file, linenum, args[0]);
 			err_code |= ERR_ALERT;
 			goto out;
 		}
-		ha_free(&global.cluster_secret);
-		global.cluster_secret = strdup(args[1]);
+
+		blk_SHA1_Init(&sha1_ctx);
+		blk_SHA1_Update(&sha1_ctx, args[1], strlen(args[1]));
+		blk_SHA1_Final(sha1_out, &sha1_ctx);
+		BUG_ON(sizeof sha1_out < sizeof global.cluster_secret);
+		memcpy(global.cluster_secret, sha1_out, sizeof global.cluster_secret);
+		cluster_secret_isset = 1;
 	}
 	else if (strcmp(args[0], "uid") == 0) {
 		if (alertif_too_many_args(1, file, linenum, args, &err_code))

src/cfgparse.c

@@ -2803,7 +2803,6 @@ int check_config_validity()
 	struct cfg_postparser *postparser;
 	struct resolvers *curr_resolvers = NULL;
 	int i;
-	int diag_no_cluster_secret = 0;
 
 	bind_conf = NULL;
 	/*
@@ -4318,13 +4317,6 @@ init_proxies_list_stage2:
 
 #ifdef USE_QUIC
 			if (listener->bind_conf->xprt == xprt_get(XPRT_QUIC)) {
-				if (!global.cluster_secret) {
-					diag_no_cluster_secret = 1;
-					if (listener->bind_conf->options & BC_O_QUIC_FORCE_RETRY) {
-						ha_alert("QUIC listener with quic-force-retry requires global cluster-secret to be set.\n");
-						cfgerr++;
-					}
-				}
 # ifdef USE_QUIC_OPENSSL_COMPAT
 				/* store the last checked bind_conf in bind_conf */
 				if (!(global.tune.options & GTUNE_NO_QUIC) &&
@@ -4375,12 +4367,6 @@ init_proxies_list_stage2:
 			goto init_proxies_list_stage2;
 	}
 
-	if (diag_no_cluster_secret) {
-		ha_diag_warning("Generating a random cluster secret. "
-		                "You should define your own one in the configuration to ensure consistency "
-		                "after reload/restart or across your whole cluster.\n");
-	}
-
 	/*
 	 * Recount currently required checks.
 	 */

src/haproxy.c

@@ -1925,17 +1925,16 @@ static void dump_registered_keywords(void)
 static void generate_random_cluster_secret()
 {
 	/* used as a default random cluster-secret if none defined. */
-	uint64_t rand = ha_random64();
+	uint64_t rand;
 
 	/* The caller must not overwrite an already defined secret. */
-	BUG_ON(global.cluster_secret);
-
-	global.cluster_secret = malloc(8);
-	if (!global.cluster_secret)
-		return;
+	BUG_ON(cluster_secret_isset);
 
+	rand = ha_random64();
 	memcpy(global.cluster_secret, &rand, sizeof(rand));
-	global.cluster_secret[7] = '\0';
+	rand = ha_random64();
+	memcpy(global.cluster_secret + sizeof(rand), &rand, sizeof(rand));
+	cluster_secret_isset = 1;
 }
 
 /*
@@ -2657,7 +2656,7 @@ static void init(int argc, char **argv)
 		exit(1);
 	}
 
-	if (!global.cluster_secret)
+	if (!cluster_secret_isset)
 		generate_random_cluster_secret();
 
 	/*
@@ -2850,7 +2849,6 @@ void deinit(void)
 	ha_free(&global.log_send_hostname);
 	chunk_destroy(&global.log_tag);
 	ha_free(&global.chroot);
-	ha_free(&global.cluster_secret);
 	ha_free(&global.pidfile);
 	ha_free(&global.node);
 	ha_free(&global.desc);

src/quic_conn.c

@@ -477,8 +477,8 @@ int quic_stateless_reset_token_cpy(unsigned char *pos, size_t len,
                                    const unsigned char *salt, size_t saltlen)
 {
 	/* Input secret */
-	const unsigned char *key = (const unsigned char *)global.cluster_secret;
-	size_t keylen = strlen(global.cluster_secret);
+	const unsigned char *key = global.cluster_secret;
+	size_t keylen = sizeof global.cluster_secret;
 	/* Info */
 	const unsigned char label[] = "stateless token";
 	size_t labellen = sizeof label - 1;
@@ -494,25 +494,14 @@ int quic_stateless_reset_token_cpy(unsigned char *pos, size_t len,
  */
 static int quic_stateless_reset_token_init(struct quic_connection_id *conn_id)
 {
-	int ret;
+	/* Output secret */
+	unsigned char *token = conn_id->stateless_reset_token;
+	size_t tokenlen = sizeof conn_id->stateless_reset_token;
+	/* Salt */
+	const unsigned char *cid = conn_id->cid.data;
+	size_t cidlen = conn_id->cid.len;
 
-	if (global.cluster_secret) {
-		/* Output secret */
-		unsigned char *token = conn_id->stateless_reset_token;
-		size_t tokenlen = sizeof conn_id->stateless_reset_token;
-		/* Salt */
-		const unsigned char *cid = conn_id->cid.data;
-		size_t cidlen = conn_id->cid.len;
-
-		ret = quic_stateless_reset_token_cpy(token, tokenlen, cid, cidlen);
-	}
-	else {
-		/* TODO: RAND_bytes() should be replaced */
-		ret = RAND_bytes(conn_id->stateless_reset_token,
-		                 sizeof conn_id->stateless_reset_token) == 1;
-	}
-
-	return ret;
+	return quic_stateless_reset_token_cpy(token, tokenlen, cid, cidlen);
 }
 
 /* Generate a CID directly derived from <orig> CID and <addr> address.

src/quic_rx.c

@@ -1722,8 +1722,8 @@ static int quic_retry_token_check(struct quic_rx_packet *pkt,
 	const unsigned char *salt;
 	unsigned char key[QUIC_TLS_KEY_LEN];
 	unsigned char iv[QUIC_TLS_IV_LEN];
-	const unsigned char *sec = (const unsigned char *)global.cluster_secret;
-	size_t seclen = strlen(global.cluster_secret);
+	const unsigned char *sec = global.cluster_secret;
+	size_t seclen = sizeof global.cluster_secret;
 	EVP_CIPHER_CTX *ctx = NULL;
 	const EVP_CIPHER *aead = EVP_aes_128_gcm();
 	const struct quic_version *qv = qc ? qc->original_version :
@@ -1732,7 +1732,7 @@ static int quic_retry_token_check(struct quic_rx_packet *pkt,
 	TRACE_ENTER(QUIC_EV_CONN_LPKT, qc);
 
 	/* The caller must ensure this. */
-	BUG_ON(!global.cluster_secret || !pkt->token_len);
+	BUG_ON(!pkt->token_len);
 
 	prx = l->bind_conf->frontend;
 	prx_counters = EXTRA_COUNTERS_GET(prx->extra_counters_fe, &quic_stats_module);
@@ -1906,7 +1906,7 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
 	if (pkt->type == QUIC_PACKET_TYPE_INITIAL) {
 		BUG_ON(!pkt->version); /* This must not happen. */
 
-		if (global.cluster_secret && pkt->token_len) {
+		if (pkt->token_len) {
 			if (!quic_retry_token_check(pkt, dgram, l, qc, &token_odcid))
 				goto err;
 		}
@@ -1917,7 +1917,7 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
 			struct quic_connection_id *conn_id;
 			int ipv4;
 
-			if (global.cluster_secret && !pkt->token_len && !(l->bind_conf->options & BC_O_QUIC_FORCE_RETRY) &&
+			if (!pkt->token_len && !(l->bind_conf->options & BC_O_QUIC_FORCE_RETRY) &&
 			    HA_ATOMIC_LOAD(&prx_counters->half_open_conn) >= global.tune.quic_retry_threshold) {
 				TRACE_PROTO("Initial without token, sending retry",
 				            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version);
@@ -1994,7 +1994,7 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
 	}
 	else if (!qc) {
 		TRACE_PROTO("RX non Initial pkt without connection", QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version);
-		if (global.cluster_secret && !send_stateless_reset(l, &dgram->saddr, pkt))
+		if (!send_stateless_reset(l, &dgram->saddr, pkt))
 			TRACE_ERROR("stateless reset not sent", QUIC_EV_CONN_LPKT, qc);
 		goto err;
 	}
@@ -2117,7 +2117,7 @@ static int quic_rx_pkt_parse(struct quic_rx_packet *pkt,
 			/* TODO Retry should be automatically activated if
 			 * suspect network usage is detected.
 			 */
-			if (global.cluster_secret && !token_len) {
+			if (!token_len) {
 				if (l->bind_conf->options & BC_O_QUIC_FORCE_RETRY) {
 					TRACE_PROTO("Initial without token, sending retry",
 					            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version);
@@ -2131,14 +2131,6 @@ static int quic_rx_pkt_parse(struct quic_rx_packet *pkt,
 					goto drop_silent;
 				}
 			}
-			else if (!global.cluster_secret && token_len) {
-				/* Impossible case: a token was received without configured
-				 * cluster secret.
-				 */
-				TRACE_PROTO("Packet dropped", QUIC_EV_CONN_LPKT,
-				            NULL, NULL, NULL, pkt->version);
-				goto drop;
-			}
 
 			pkt->token = pos;
 			pkt->token_len = token_len;

src/quic_tx.c

@@ -1610,8 +1610,8 @@ static int quic_generate_retry_token(unsigned char *token, size_t len,
 	unsigned char salt[QUIC_RETRY_TOKEN_SALTLEN];
 	unsigned char key[QUIC_TLS_KEY_LEN];
 	unsigned char iv[QUIC_TLS_IV_LEN];
-	const unsigned char *sec = (const unsigned char *)global.cluster_secret;
-	size_t seclen = strlen(global.cluster_secret);
+	const unsigned char *sec = global.cluster_secret;
+	size_t seclen = sizeof global.cluster_secret;
 	EVP_CIPHER_CTX *ctx = NULL;
 	const EVP_CIPHER *aead = EVP_aes_128_gcm();
 	uint32_t timestamp = (uint32_t)date.tv_sec;