BUG/MEDIUM: 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 ckch_inst that references it increments
its refcount. The reference to the certificate_ocsp is actually kept in
the SSL_CTX linked to each ckch_inst, in an ex_data entry that gets
freed when he context is freed.
One of the downside of this implementation is that is 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), the 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
for the actual ckch instances and one for the ckch stores. If the
instance refcount becomes 0 then we remove the entry from the auto
update tree, and if the store reference becomes 0 we can then remove the
OCSP response from the tree. This would allow to chain some "del ssl
crt-list" and "add ssl crt-list" CLI commands without losing any
functionality.

Must be backported to 2.8.

diff --git a/include/haproxy/ssl_ocsp-t.h b/include/haproxy/ssl_ocsp-t.h
index 028d6fa09ae1279e10c4a5aa193e1e9aa208144c..c083871fec82bd334633ba010f33c8c1be605050 100644 (file)
--- a/include/haproxy/ssl_ocsp-t.h
+++ b/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_instance;          /* Number of ckch_inst that reference this certificate_ocsp */
        struct buffer response;
        long expire;
        X509 *issuer;

diff --git a/include/haproxy/ssl_ocsp.h b/include/haproxy/ssl_ocsp.h
index 54a1b8831fd449d96d4ca2d432fe80e51940695f..8a4197cf3d53db883b36470df223bebfa1187681 100644 (file)
--- a/include/haproxy/ssl_ocsp.h
+++ b/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,

diff --git a/src/ssl_ckch.c b/src/ssl_ckch.c
index 24ff1649a174ec656b6b357c42008175318b0fbe..1eef87ae2011d6cca222850d56fcc14f7b44f30e 100644 (file)
--- a/src/ssl_ckch.c
+++ b/src/ssl_ckch.c
@@ -721,8 +721,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
 }
 
 /*

diff --git a/src/ssl_ocsp.c b/src/ssl_ocsp.c
index 3e7408a667ec9ebabf2bded5f79b22d658246f75..63a0d90445b5a2e9f5120b20b1c985fc9fb8a546 100644 (file)
--- a/src/ssl_ocsp.c
+++ b/src/ssl_ocsp.c
@@ -392,8 +392,9 @@ void ssl_sock_free_ocsp(struct certificate_ocsp *ocsp)
                return;
 
        HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock);
-       ocsp->refcount--;
-       if (ocsp->refcount <= 0) {
+       ocsp->refcount_store--;
+       if (ocsp->refcount_store <= 0) {
+               BUG_ON(ocsp->refcount_instance > 0);
                ebmb_delete(&ocsp->key);
                eb64_delete(&ocsp->next_update);
                X509_free(ocsp->issuer);
@@ -411,6 +412,19 @@ void ssl_sock_free_ocsp(struct certificate_ocsp *ocsp)
        HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock);
 }
 
+void ssl_sock_free_ocsp_instance(struct certificate_ocsp *ocsp)
+{
+       if (!ocsp)
+               return;
+
+       HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock);
+       ocsp->refcount_instance--;
+       if (ocsp->refcount_instance <= 0) {
+               eb64_delete(&ocsp->next_update);
+       }
+       HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock);
+}
+
 
 /*
  * This function dumps the details of an OCSP_CERTID. It is based on
@@ -626,13 +640,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;
                        }
                }
@@ -1189,7 +1203,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);
@@ -1232,7 +1246,7 @@ static struct task *ssl_ocsp_update_responses(struct task *task, void *context,
                 * reinserted after the response is processed. */
                eb64_delete(&ocsp->next_update);
 
-               ++ocsp->refcount;
+               ocsp->refcount_instance++;
                ctx->cur_ocsp = ocsp;
                ocsp->last_update_status = OCSP_UPDT_UNKNOWN;
 
@@ -1299,7 +1313,7 @@ leave:
                ++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 +1342,7 @@ http_error:
        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 +1430,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;
@@ -1545,7 +1567,7 @@ static int cli_parse_show_ocspresponse(char **args, char *payload, struct appctx
                        HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock);
                        return cli_err(appctx, "Certificate ID or path does not match any certificate.\n");
                }
-               ++ocsp->refcount;
+               ocsp->refcount_instance++;
                HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock);
 
                ctx->ocsp = ocsp;
@@ -1646,7 +1668,7 @@ yield:
        free_trash_chunk(tmp);
        BIO_free(bio);
 
-       ++ocsp->refcount;
+       ocsp->refcount_instance++;
        ctx->ocsp = ocsp;
        HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock);
        return 0;
@@ -1655,6 +1677,14 @@ yield:
 #endif
 }
 
+static void cli_release_show_ocspresponse(struct appctx *appctx)
+{
+       struct show_ocspresp_cli_ctx *ctx = appctx->svcctx;
+
+       if (ctx)
+               ssl_sock_free_ocsp(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 +1945,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 },

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 926fc3b730c7ec5bfd36427c4c155f057d2921cb..7d4e79db3b45a66f2bbac83ba878a30f0676d589 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -1123,6 +1123,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)
@@ -1158,8 +1159,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;
 
@@ -1186,6 +1189,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;
@@ -1196,7 +1202,7 @@ static int ssl_sock_load_ocsp(const char *path, SSL_CTX *ctx, struct ckch_data *
 
                cb_arg->is_single = 1;
                cb_arg->s_ocsp = iocsp;
-               iocsp->refcount++;
+               iocsp->refcount_instance++;
 
                pkey = X509_get_pubkey(x);
                cb_arg->single_kt = EVP_PKEY_base_id(pkey);
@@ -1236,7 +1242,7 @@ static int ssl_sock_load_ocsp(const char *path, SSL_CTX *ctx, struct ckch_data *
                index = ssl_sock_get_ocsp_arg_kt_index(key_type);
                if (index >= 0 && !cb_arg->m_ocsp[index]) {
                        cb_arg->m_ocsp[index] = iocsp;
-                       iocsp->refcount++;
+                       iocsp->refcount_instance++;
                }
        }
        HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock);