RepoMirrors
/
haproxy
mirror of http://git.haproxy.org/git/haproxy.git/
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Remi Tricot-Le Breton 2024-02-07 16:38:43 +01:00 committed by William Lallemand
parent 23cab33b67
commit befebf8b51
5 changed files with 75 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
include/haproxy/ssl_ocsp-t.h
View File

@ -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;

1
include/haproxy/ssl_ocsp.h
View File

@ -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,

19
src/ssl_ckch.c
View File

@ -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;
/* 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
}
/*

54
src/ssl_ocsp.c
View File

@ -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 @@ 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 +1342,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 +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 @@ static int cli_io_handler_show_ocspresponse(struct appctx *appctx)
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 @@ 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(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 },

12
src/ssl_sock.c
View File

@ -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);