BUG/MEDIUM: ssl_ckch: Rework 'commit ssl cert' to handle full buffer cases
When changes on a certificate are commited, a trash buffer is used to create
the response. Once done, the message is copied in the response buffer.
However, if the buffer is full, there is no way to retry and the message is
lost. The same issue may happen with the error message. It is a design issue
of cli_io_handler_commit_cert() function.
To fix it, the function was reworked. First, the error message is now part
of the service context. This way, if we cannot push the error message in the
reponse buffer, we may retry later. To do so, a dedicated state was created
(CERT_ST_ERROR). Then, the success message is also handled in a dedicated
state (CERT_ST_SUCCESS). This way we are able to retry to push it if
necessary. Finally, the dot displayed for each updated CKCH instance is now
immediatly pushed in the response buffer, and before the update. This way,
we are able to retry too if necessary.
This patch should fix the issue #1725. It must be backported as far as
2.2. But massive refactoring was performed in 2.6. So, for the 2.5 and
below, the patch must be adapted.
(cherry picked from commit 9d56e248a63b053b7c78ca9075fa5ce713599941)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit a684730eff43ce898733d5443a5c7948daa2cefe)
[cf: As expected, the patch was highly adapted]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit dbf5eebcdf2588f59db45b5b3a12a2e44e4aab58)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
diff --git a/include/haproxy/applet-t.h b/include/haproxy/applet-t.h
index a3cf3de..014e01e 100644
--- a/include/haproxy/applet-t.h
+++ b/include/haproxy/applet-t.h
@@ -175,6 +175,7 @@
int flags; /* non-zero if "dict" dump requested */
} cfgpeers;
struct {
+ char *err;
struct ckch_store *old_ckchs;
struct ckch_store *new_ckchs;
struct ckch_inst *next_ckchi;
diff --git a/include/haproxy/ssl_sock-t.h b/include/haproxy/ssl_sock-t.h
index 9839011..974b836 100644
--- a/include/haproxy/ssl_sock-t.h
+++ b/include/haproxy/ssl_sock-t.h
@@ -106,7 +106,9 @@
SETCERT_ST_INIT = 0,
SETCERT_ST_GEN,
SETCERT_ST_INSERT,
+ SETCERT_ST_SUCCESS,
SETCERT_ST_FIN,
+ SETCERT_ST_ERROR,
};
#if (HA_OPENSSL_VERSION_NUMBER < 0x1010000fL)
diff --git a/src/ssl_ckch.c b/src/ssl_ckch.c
index 77f4ecb..113ab4b 100644
--- a/src/ssl_ckch.c
+++ b/src/ssl_ckch.c
@@ -1241,17 +1241,14 @@
/* release function of the `set ssl cert' command, free things and unlock the spinlock */
static void cli_release_commit_cert(struct appctx *appctx)
{
- struct ckch_store *new_ckchs;
+ struct ckch_store *new_ckchs = appctx->ctx.ssl.new_ckchs;
HA_SPIN_UNLOCK(CKCH_LOCK, &ckch_lock);
- if (appctx->st2 != SETCERT_ST_FIN) {
- /* free every new sni_ctx and the new store, which are not in the trees so no spinlock there */
- new_ckchs = appctx->ctx.ssl.new_ckchs;
-
- /* if the allocation failed, we need to free everything from the temporary list */
+ /* free every new sni_ctx and the new store, which are not in the trees so no spinlock there */
+ if (new_ckchs)
ckch_store_free(new_ckchs);
- }
+ ha_free(&appctx->ctx.ssl.err);
}
/*
@@ -1261,30 +1258,24 @@
{
struct stream_interface *si = appctx->owner;
int y = 0;
- char *err = NULL;
int errcode = 0;
int retval = 0;
struct ckch_store *old_ckchs, *new_ckchs = NULL;
struct ckch_inst *ckchi, *ckchis;
- struct buffer *trash = alloc_trash_chunk();
struct sni_ctx *sc0, *sc0s;
struct crtlist_entry *entry;
- if (trash == NULL)
- goto error;
if (unlikely(si_ic(si)->flags & (CF_WRITE_ERROR|CF_SHUTW)))
- goto error;
+ goto end;
while (1) {
switch (appctx->st2) {
case SETCERT_ST_INIT:
/* This state just print the update message */
- chunk_printf(trash, "Committing %s", ckchs_transaction.path);
- if (ci_putchk(si_ic(si), trash) == -1) {
- si_rx_room_blk(si);
+ chunk_printf(&trash, "Committing %s", ckchs_transaction.path);
+ if (ci_putchk(si_ic(si), &trash) == -1)
goto yield;
- }
appctx->st2 = SETCERT_ST_GEN;
/* fallthrough */
case SETCERT_ST_GEN:
@@ -1299,9 +1290,6 @@
old_ckchs = appctx->ctx.ssl.old_ckchs;
new_ckchs = appctx->ctx.ssl.new_ckchs;
- if (!new_ckchs)
- continue;
-
/* get the next ckchi to regenerate */
ckchi = appctx->ctx.ssl.next_ckchi;
/* we didn't start yet, set it to the first elem */
@@ -1314,25 +1302,34 @@
char **sni_filter = NULL;
int fcount = 0;
+ /* save the next ckchi to compute in case of yield */
+ appctx->ctx.ssl.next_ckchi = ckchi;
+
/* it takes a lot of CPU to creates SSL_CTXs, so we yield every 10 CKCH instances */
if (y >= 10) {
- /* save the next ckchi to compute */
- appctx->ctx.ssl.next_ckchi = ckchi;
+ si_rx_endp_more(si);
goto yield;
}
+ /* display one dot per new instance */
+ if (ci_putstr(si_ic(si), ".") == -1)
+ goto yield;
+
if (ckchi->crtlist_entry) {
sni_filter = ckchi->crtlist_entry->filters;
fcount = ckchi->crtlist_entry->fcount;
}
+ appctx->ctx.ssl.err = NULL;
if (ckchi->is_server_instance)
- errcode |= ckch_inst_new_load_srv_store(new_ckchs->path, new_ckchs, &new_inst, &err);
+ errcode |= ckch_inst_new_load_srv_store(new_ckchs->path, new_ckchs, &new_inst, &appctx->ctx.ssl.err);
else
- errcode |= ckch_inst_new_load_store(new_ckchs->path, new_ckchs, ckchi->bind_conf, ckchi->ssl_conf, sni_filter, fcount, &new_inst, &err);
+ errcode |= ckch_inst_new_load_store(new_ckchs->path, new_ckchs, ckchi->bind_conf, ckchi->ssl_conf, sni_filter, fcount, &new_inst, &appctx->ctx.ssl.err);
- if (errcode & ERR_CODE)
+ if (errcode & ERR_CODE) {
+ appctx->st2 = SETCERT_ST_ERROR;
goto error;
+ }
/* if the previous ckchi was used as the default */
if (ckchi->is_default)
@@ -1343,8 +1340,10 @@
/* Create a new SSL_CTX and link it to the new instance. */
if (new_inst->is_server_instance) {
retval = ssl_sock_prepare_srv_ssl_ctx(ckchi->server, new_inst->ctx);
- if (retval)
+ if (retval) {
+ appctx->st2 = SETCERT_ST_ERROR;
goto error;
+ }
}
/* create the link to the crtlist_entry */
@@ -1354,15 +1353,15 @@
/* this iterate on the newly generated SNIs in the new instance to prepare their SSL_CTX */
list_for_each_entry_safe(sc0, sc0s, &new_inst->sni_ctx, by_ckch_inst) {
if (!sc0->order) { /* we initialized only the first SSL_CTX because it's the same in the other sni_ctx's */
- errcode |= ssl_sock_prepare_ctx(ckchi->bind_conf, ckchi->ssl_conf, sc0->ctx, &err);
- if (errcode & ERR_CODE)
+ appctx->ctx.ssl.err = NULL;
+ errcode |= ssl_sock_prepare_ctx(ckchi->bind_conf, ckchi->ssl_conf, sc0->ctx, &appctx->ctx.ssl.err);
+ if (errcode & ERR_CODE) {
+ appctx->st2 = SETCERT_ST_ERROR;
goto error;
+ }
}
}
-
- /* display one dot per new instance */
- chunk_appendf(trash, ".");
/* link the new ckch_inst to the duplicate */
LIST_APPEND(&new_ckchs->ckch_inst, &new_inst->by_ckchs);
y++;
@@ -1375,9 +1374,6 @@
old_ckchs = appctx->ctx.ssl.old_ckchs;
new_ckchs = appctx->ctx.ssl.new_ckchs;
- if (!new_ckchs)
- continue;
-
/* get the list of crtlist_entry in the old store, and update the pointers to the store */
LIST_SPLICE(&new_ckchs->crtlist_entry, &old_ckchs->crtlist_entry);
list_for_each_entry(entry, &new_ckchs->crtlist_entry, by_ckch_store) {
@@ -1440,6 +1436,12 @@
/* Replace the old ckchs by the new one */
ckch_store_free(old_ckchs);
ebst_insert(&ckchs_tree, &new_ckchs->node);
+ appctx->ctx.ssl.old_ckchs = appctx->ctx.ssl.new_ckchs = NULL;
+ appctx->st2 = SETCERT_ST_SUCCESS;
+ /* fallthrough */
+ case SETCERT_ST_SUCCESS:
+ if (ci_putstr(si_ic(si), "\nSuccess!\n") == -1)
+ goto yield;
appctx->st2 = SETCERT_ST_FIN;
/* fallthrough */
case SETCERT_ST_FIN:
@@ -1448,38 +1450,23 @@
ckchs_transaction.old_ckchs = NULL;
ckchs_transaction.path = NULL;
goto end;
+
+ case SETCERT_ST_ERROR:
+ error:
+ chunk_printf(&trash, "\n%sFailed!\n", appctx->ctx.ssl.err);
+ if (ci_putchk(si_ic(si), &trash) == -1)
+ goto yield;
+ appctx->st2 = SETCERT_ST_FIN;
+ break;
}
}
end:
-
- chunk_appendf(trash, "\n");
- if (errcode & ERR_WARN)
- chunk_appendf(trash, "%s", err);
- chunk_appendf(trash, "Success!\n");
- if (ci_putchk(si_ic(si), trash) == -1)
- si_rx_room_blk(si);
- free_trash_chunk(trash);
/* success: call the release function and don't come back */
return 1;
+
yield:
- /* store the state */
- if (ci_putchk(si_ic(si), trash) == -1)
- si_rx_room_blk(si);
- free_trash_chunk(trash);
- si_rx_endp_more(si); /* let's come back later */
+ si_rx_room_blk(si);
return 0; /* should come back */
-
-error:
- /* spin unlock and free are done in the release function */
- if (trash) {
- chunk_appendf(trash, "\n%sFailed!\n", err);
- if (ci_putchk(si_ic(si), trash) == -1)
- si_rx_room_blk(si);
- free_trash_chunk(trash);
- }
- free(err);
- /* error: call the release function and don't come back */
- return 1;
}
/*