BUG/MAJOR: ocsp: Separate refcount per instance and per store

With the current way OCSP responses are stored, a single OCSP response
is stored (in a certificate_ocsp structure) when it is loaded during a
certificate parsing, and each SSL_CTX that references it increments its
refcount. The reference to the certificate_ocsp is kept in the SSL_CTX
linked to each ckch_inst, in an ex_data entry that gets freed when the
context is freed.
One of the downsides of this implementation is that if every ckch_inst
referencing a certificate_ocsp gets detroyed, then the OCSP response is
removed from the system. So if we were to remove all crt-list lines
containing a given certificate (that has an OCSP response), and if all
the corresponding SSL_CTXs were destroyed (no ongoing connection using
them), the OCSP response would be destroyed even if the certificate
remains in the system (as an unused certificate).
In such a case, we would want the OCSP response not to be "usable",
since it is not used by any ckch_inst, but still remain in the OCSP
response tree so that if the certificate gets reused (via an "add ssl
crt-list" command for instance), its OCSP response is still known as
well.
But we would also like such an entry not to be updated automatically
anymore once no instance uses it. An easy way to do it could have been
to keep a reference to the certificate_ocsp structure in the ckch_store
as well, on top of all the ones in the ckch_instances, and to remove the
ocsp response from the update tree once the refcount falls to 1, but it
would not work because of the way the ocsp response tree keys are
calculated. They are decorrelated from the ckch_store and are the actual
OCSP_CERTIDs, which is a combination of the issuer's name hash and key
hash, and the certificate's serial number. So two copies of the same
certificate but with different names would still point to the same ocsp
response tree entry.

The solution that answers to all the needs expressed aboved is actually
to have two reference counters in the certificate_ocsp structure, one
actual reference counter corresponding to the number of "live" pointers
on the certificate_ocsp structure, incremented for every SSL_CTX using
it, and one for the ckch stores.
If the ckch_store reference counter falls to 0, the corresponding
certificate must have been removed via CLI calls ('set ssl cert' for
instance).
If the actual refcount falls to 0, then no live SSL_CTX uses the
response anymore. It could happen if all the corresponding crt-list
lines were removed and there are no live SSL sessions using the
certificate anymore.
If any of the two refcounts becomes 0, we will always remove the
response from the auto update tree, because there's no point in spending
time updating an OCSP response that no new SSL connection will be able
to use. But the certificate_ocsp object won't be removed from the tree
unless both refcounts are 0.

Must be backported up to 2.8. Wait a little bit before backporting.

include/haproxy/ssl_ocsp-t.h

@@ -47,7 +47,8 @@ struct certificate_ocsp {
 	struct ebmb_node key;
 	unsigned char key_data[OCSP_MAX_CERTID_ASN1_LENGTH];
 	unsigned int key_length;
-	int refcount;
+	int refcount_store;		/* Number of ckch_store that reference this certificate_ocsp */
+	int refcount;			/* Number of actual references to this certificate_ocsp (SSL_CTXs mostly) */
 	struct buffer response;
 	long expire;
 	X509 *issuer;

include/haproxy/ssl_ocsp.h

@@ -36,6 +36,7 @@ int ssl_sock_get_ocsp_arg_kt_index(int evp_keytype);
 int ssl_sock_ocsp_stapling_cbk(SSL *ssl, void *arg);
 
 void ssl_sock_free_ocsp(struct certificate_ocsp *ocsp);
+void ssl_sock_free_ocsp_instance(struct certificate_ocsp *ocsp);
 
 int ssl_sock_load_ocsp_response(struct buffer *ocsp_response,
                                 struct certificate_ocsp *ocsp,

src/ssl_ckch.c

@@ -734,8 +734,27 @@ void ssl_sock_free_cert_key_and_chain_contents(struct ckch_data *data)
 		X509_free(data->ocsp_issuer);
 	data->ocsp_issuer = NULL;
 
-	OCSP_CERTID_free(data->ocsp_cid);
-	data->ocsp_cid = NULL;
+
+	/* We need to properly remove the reference to the corresponding
+	 * certificate_ocsp structure if it exists (which it should).
+	 */
+#if ((defined SSL_CTRL_SET_TLSEXT_STATUS_REQ_CB && !defined OPENSSL_NO_OCSP) && !defined OPENSSL_IS_BORINGSSL)
+	if (data->ocsp_cid) {
+		struct certificate_ocsp *ocsp = NULL;
+		unsigned char certid[OCSP_MAX_CERTID_ASN1_LENGTH] = {};
+		unsigned int certid_length = 0;
+
+		if (ssl_ocsp_build_response_key(data->ocsp_cid, (unsigned char*)certid, &certid_length) >= 0) {
+			HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock);
+			ocsp = (struct certificate_ocsp *)ebmb_lookup(&cert_ocsp_tree, certid, OCSP_MAX_CERTID_ASN1_LENGTH);
+			HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock);
+			ssl_sock_free_ocsp(ocsp);
+		}
+
+		OCSP_CERTID_free(data->ocsp_cid);
+		data->ocsp_cid = NULL;
+	}
+#endif
 }
 
 /*

src/ssl_ocsp.c

@@ -382,11 +382,50 @@ int ssl_sock_update_ocsp_response(struct buffer *ocsp_response, char **err)
 
 
 #if !defined OPENSSL_IS_BORINGSSL
+/*
+ * Must be called under ocsp_tree_lock lock.
+ */
+static void ssl_sock_free_ocsp_data(struct certificate_ocsp *ocsp)
+{
+	ebmb_delete(&ocsp->key);
+	eb64_delete(&ocsp->next_update);
+	X509_free(ocsp->issuer);
+	ocsp->issuer = NULL;
+	sk_X509_pop_free(ocsp->chain, X509_free);
+	ocsp->chain = NULL;
+	chunk_destroy(&ocsp->response);
+	if (ocsp->uri) {
+		ha_free(&ocsp->uri->area);
+		ha_free(&ocsp->uri);
+	}
+	free(ocsp);
+}
+
 /*
  * Decrease the refcount of the struct ocsp_response and frees it if it's not
  * used anymore. Also removes it from the tree if free'd.
  */
 void ssl_sock_free_ocsp(struct certificate_ocsp *ocsp)
+{
+	if (!ocsp)
+		return;
+
+	HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock);
+	ocsp->refcount_store--;
+	if (ocsp->refcount_store <= 0) {
+		eb64_delete(&ocsp->next_update);
+		/* Might happen if some ongoing requests kept using an SSL_CTX
+		 * that referenced this OCSP response after the corresponding
+		 * ckch_store was deleted or changed (via cli commands for
+		 * instance).
+		 */
+		if (ocsp->refcount <= 0)
+			ssl_sock_free_ocsp_data(ocsp);
+	}
+	HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock);
+}
+
+void ssl_sock_free_ocsp_instance(struct certificate_ocsp *ocsp)
 {
 	if (!ocsp)
 		return;
@@ -394,19 +433,15 @@ void ssl_sock_free_ocsp(struct certificate_ocsp *ocsp)
 	HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock);
 	ocsp->refcount--;
 	if (ocsp->refcount <= 0) {
-		ebmb_delete(&ocsp->key);
 		eb64_delete(&ocsp->next_update);
-		X509_free(ocsp->issuer);
-		ocsp->issuer = NULL;
-		sk_X509_pop_free(ocsp->chain, X509_free);
-		ocsp->chain = NULL;
-		chunk_destroy(&ocsp->response);
-		if (ocsp->uri) {
-			ha_free(&ocsp->uri->area);
-			ha_free(&ocsp->uri);
-		}
+		/* Might happen if some ongoing requests kept using an SSL_CTX
+		 * that referenced this OCSP response after the corresponding
+		 * ckch_store was deleted or changed (via cli commands for
+		 * instance).
+		 */
+		if (ocsp->refcount_store <= 0)
+			ssl_sock_free_ocsp_data(ocsp);
 
-		free(ocsp);
 	}
 	HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock);
 }
@@ -626,13 +661,13 @@ void ssl_sock_ocsp_free_func(void *parent, void *ptr, CRYPTO_EX_DATA *ad, int id
 		ocsp_arg = ptr;
 
 		if (ocsp_arg->is_single) {
-			ssl_sock_free_ocsp(ocsp_arg->s_ocsp);
+			ssl_sock_free_ocsp_instance(ocsp_arg->s_ocsp);
 			ocsp_arg->s_ocsp = NULL;
 		} else {
 			int i;
 
 			for (i = 0; i < SSL_SOCK_NUM_KEYTYPES; i++) {
-				ssl_sock_free_ocsp(ocsp_arg->m_ocsp[i]);
+				ssl_sock_free_ocsp_instance(ocsp_arg->m_ocsp[i]);
 				ocsp_arg->m_ocsp[i] = NULL;
 			}
 		}
@@ -908,7 +943,7 @@ static int ssl_ocsp_task_schedule()
 }
 REGISTER_POST_CHECK(ssl_ocsp_task_schedule);
 
-void ssl_sock_free_ocsp(struct certificate_ocsp *ocsp);
+void ssl_sock_free_ocsp_instance(struct certificate_ocsp *ocsp);
 
 void ssl_destroy_ocsp_update_task(void)
 {
@@ -930,7 +965,7 @@ void ssl_destroy_ocsp_update_task(void)
 	task_destroy(ocsp_update_task);
 	ocsp_update_task = NULL;
 
-	ssl_sock_free_ocsp(ssl_ocsp_task_ctx.cur_ocsp);
+	ssl_sock_free_ocsp_instance(ssl_ocsp_task_ctx.cur_ocsp);
 	ssl_ocsp_task_ctx.cur_ocsp = NULL;
 
 	if (ssl_ocsp_task_ctx.hc) {
@@ -1189,7 +1224,7 @@ static struct task *ssl_ocsp_update_responses(struct task *task, void *context,
 			/* Reinsert the entry into the update list so that it can be updated later */
 			ssl_ocsp_update_insert(ocsp);
 			/* Release the reference kept on the updated ocsp response. */
-			ssl_sock_free_ocsp(ctx->cur_ocsp);
+			ssl_sock_free_ocsp_instance(ctx->cur_ocsp);
 			ctx->cur_ocsp = NULL;
 
 			HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock);
@@ -1299,7 +1334,7 @@ static struct task *ssl_ocsp_update_responses(struct task *task, void *context,
 		++ctx->cur_ocsp->num_failure;
 		ssl_ocsp_update_insert_after_error(ctx->cur_ocsp);
 		/* Release the reference kept on the updated ocsp response. */
-		ssl_sock_free_ocsp(ctx->cur_ocsp);
+		ssl_sock_free_ocsp_instance(ctx->cur_ocsp);
 		ctx->cur_ocsp = NULL;
 	}
 	if (hc)
@@ -1328,7 +1363,7 @@ static struct task *ssl_ocsp_update_responses(struct task *task, void *context,
 	if (hc)
 		httpclient_stop_and_destroy(hc);
 	/* Release the reference kept on the updated ocsp response. */
-	ssl_sock_free_ocsp(ctx->cur_ocsp);
+	ssl_sock_free_ocsp_instance(ctx->cur_ocsp);
 	HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock);
 	/* Set next_wakeup to the new first entry of the tree */
 	eb = eb64_first(&ocsp_update_tree);
@@ -1416,7 +1451,15 @@ static int cli_parse_update_ocsp_response(char **args, char *payload, struct app
 	update_once = (ocsp->next_update.node.leaf_p == NULL);
 	eb64_delete(&ocsp->next_update);
 
-	/* Insert the entry at the beginning of the update tree. */
+	/* Insert the entry at the beginning of the update tree.
+	 * We don't need to increase the reference counter on the
+	 * certificate_ocsp structure because we would not have a way to
+	 * decrease it afterwards since this update operation is asynchronous.
+	 * If the corresponding entry were to be destroyed before the update can
+	 * be performed, which is pretty unlikely, it would not be such a
+	 * problem because that would mean that the OCSP response is not
+	 * actually used.
+	 */
 	ocsp->next_update.key = 0;
 	eb64_insert(&ocsp_update_tree, &ocsp->next_update);
 	ocsp->update_once = update_once;
@@ -1655,6 +1698,14 @@ static int cli_io_handler_show_ocspresponse(struct appctx *appctx)
 #endif
 }
 
+static void cli_release_show_ocspresponse(struct appctx *appctx)
+{
+	struct show_ocspresp_cli_ctx *ctx = appctx->svcctx;
+
+	if (ctx)
+		ssl_sock_free_ocsp_instance(ctx->ocsp);
+}
+
 /* Check if the ckch_store and the entry does have the same configuration */
 int ocsp_update_check_cfg_consistency(struct ckch_store *store, struct crtlist_entry *entry, char *crt_path, char **err)
 {
@@ -1915,7 +1966,7 @@ smp_fetch_ssl_ocsp_success_cnt(const struct arg *args, struct sample *smp, const
 static struct cli_kw_list cli_kws = {{ },{
 	{ { "set", "ssl", "ocsp-response", NULL }, "set ssl ocsp-response <resp|payload>       : update a certificate's OCSP Response from a base64-encode DER",      cli_parse_set_ocspresponse, NULL },
 
-	{ { "show", "ssl", "ocsp-response", NULL },"show ssl ocsp-response [[text|base64] id]  : display the IDs of the OCSP responses used in memory, or the details of a single OCSP response (in text or base64 format)", cli_parse_show_ocspresponse, cli_io_handler_show_ocspresponse, NULL },
+	{ { "show", "ssl", "ocsp-response", NULL },"show ssl ocsp-response [[text|base64] id]  : display the IDs of the OCSP responses used in memory, or the details of a single OCSP response (in text or base64 format)", cli_parse_show_ocspresponse, cli_io_handler_show_ocspresponse, cli_release_show_ocspresponse },
 	{ { "show", "ssl", "ocsp-updates", NULL }, "show ssl ocsp-updates                      : display information about the next 'nb' ocsp responses that will be updated automatically", cli_parse_show_ocsp_updates, cli_io_handler_show_ocsp_updates, cli_release_show_ocsp_updates },
 #if ((defined SSL_CTRL_SET_TLSEXT_STATUS_REQ_CB && !defined OPENSSL_NO_OCSP) && !defined OPENSSL_IS_BORINGSSL)
 	{ { "update", "ssl", "ocsp-response", NULL }, "update ssl ocsp-response <certfile>     : send ocsp request and update stored ocsp response",                  cli_parse_update_ocsp_response, NULL, NULL },

src/ssl_sock.c

@@ -1124,6 +1124,7 @@ static int ssl_sock_load_ocsp(const char *path, SSL_CTX *ctx, struct ckch_data *
 	struct buffer *ocsp_uri = get_trash_chunk();
 	char *err = NULL;
 	size_t path_len;
+	int inc_refcount_store = 0;
 
 	x = data->cert;
 	if (!x)
@@ -1159,8 +1160,10 @@ static int ssl_sock_load_ocsp(const char *path, SSL_CTX *ctx, struct ckch_data *
 	if (!issuer)
 		goto out;
 
-	if (!data->ocsp_cid)
+	if (!data->ocsp_cid) {
 		data->ocsp_cid = OCSP_cert_to_id(0, x, issuer);
+		inc_refcount_store = 1;
+	}
 	if (!data->ocsp_cid)
 		goto out;
 
@@ -1187,6 +1190,9 @@ static int ssl_sock_load_ocsp(const char *path, SSL_CTX *ctx, struct ckch_data *
 #endif
 	SSL_CTX_get_tlsext_status_cb(ctx, &callback);
 
+	if (inc_refcount_store)
+		iocsp->refcount_store++;
+
 	if (!callback) {
 		struct ocsp_cbk_arg *cb_arg;
 		EVP_PKEY *pkey;