MEDIUM: ssl: add ocsp-update.timeout global option

The httpclient used to fetch OCSP responses during automatic updates
only has a connect timeout (5s), no server timeout: if a response
starts but is never fully delivered, the request can hang forever.
This task and its httpclient are shared by the whole ocsp-update
queue, so a single stuck request blocks every other certificate
waiting behind it, effectively stalling all further OCSP updates.

Add a global "ocsp-update.timeout <timeout>" option and apply it with
httpclient_set_timeout() before each update request is sent. This
bounds the total time spent on one OCSP fetch, from the connection
attempt to the full response, and lets a stuck or overly slow
responder be retried later following the usual
ocsp-update.mindelay/maxdelay rules instead of blocking the queue
indefinitely.

The value is parsed with parse_time_err() using TIME_UNIT_MS, the same
helper and default unit used by every other timeout keyword in the
codebase (e.g. "timeout connect", "tcp-ut"), so it accepts a bare
number in milliseconds or any value suffixed with an explicit unit.
It defaults to 10s (SSL_OCSP_UPDATE_HTTP_TIMEOUT, now expressed in
milliseconds).

Can be backported to stable branches that already carry the other
ocsp-update.* global options.

Signed-off-by: Alex Szakaly <alex.szakaly@gmail.com>

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 638eb81f9aed6f1473f61ea0e7e179cc5aecf3a8..509825923be6506867ddf77d162693b9237a801e 100644 (file)
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -3163,6 +3163,16 @@ ocsp-update.mode [ on | off ]
   See option "ocsp-update" for more information about the auto update
   mechanism.
 
+ocsp-update.timeout <timeout>
+  Sets the timeout applied to the request used to fetch an OCSP response
+  during an automatic update. This timeout covers the whole request, from
+  the connection attempt to the reception of the full response. If it is
+  reached, the update is considered a failure and is logged as such. The
+  timeout value is specified in milliseconds by default, but can be in any
+  other unit if the number is suffixed by the unit, as explained at the top
+  of this document. It defaults to 10s. See option "ocsp-update" for more
+  information about the auto update mechanism.
+
 pidfile <pidfile>
   Writes PIDs of all daemons into file <pidfile> when daemon mode or writes PID
   of master process into file <pidfile> when master-worker mode. This option is

diff --git a/include/haproxy/ssl_ocsp-t.h b/include/haproxy/ssl_ocsp-t.h
index 1f3bbc41b7f953b1b66ca1d1292ac74a067b8175..a6ca0287135c03f50de9109cf531383311fe6dc9 100644 (file)
--- a/include/haproxy/ssl_ocsp-t.h
+++ b/include/haproxy/ssl_ocsp-t.h
@@ -37,6 +37,7 @@ extern int ocsp_ex_index;
 #define SSL_OCSP_UPDATE_DELAY_MIN 5*60  /* 5 minutes */
 #define SSL_OCSP_UPDATE_MARGIN 60   /* 1 minute */
 #define SSL_OCSP_HTTP_ERR_REPLAY 60 /* 1 minute */
+#define SSL_OCSP_UPDATE_HTTP_TIMEOUT 10000 /* 10 seconds */
 
 #if defined(HAVE_SSL_OCSP)
 /*

diff --git a/include/haproxy/ssl_sock-t.h b/include/haproxy/ssl_sock-t.h
index 6904e187ecae4193b8c28f55727811491a5a47dd..512f8831de5cd41ca6afe49aa442180b968b60c3 100644 (file)
--- a/include/haproxy/ssl_sock-t.h
+++ b/include/haproxy/ssl_sock-t.h
@@ -333,6 +333,7 @@ struct global_ssl {
        struct {
                unsigned int delay_max;
                unsigned int delay_min;
+               int http_timeout; /* in milliseconds */
                int mode; /* default mode used for ocsp auto-update (off, on) */
                int disable;
        } ocsp_update;

diff --git a/src/ssl_ocsp.c b/src/ssl_ocsp.c
index 9191d5bdfab3e8caf7fe29d050dadca5826f7e67..42f7d87613d0731e4ddf992e5c610f0c11263fc6 100644 (file)
--- a/src/ssl_ocsp.c
+++ b/src/ssl_ocsp.c
@@ -63,6 +63,7 @@
 #include <haproxy/task.h>
 #include <haproxy/ticks.h>
 #include <haproxy/time.h>
+#include <haproxy/tools.h>
 #include <haproxy/trace.h>
 #include <haproxy/ssl_trace.h>
 
@@ -1401,6 +1402,16 @@ static struct task *ssl_ocsp_update_responses(struct task *task, void *context,
                        goto leave;
                }
 
+               /* Without an explicit timeout, the httpclient's server side
+                * timeout stays unset (see httpclient_set_timeout()) and the
+                * request can hang forever if the response is never fully
+                * delivered to us, even after the underlying connection is
+                * done. Since this task and its httpclient are shared by the
+                * whole ocsp-update queue, such a hang blocks every other
+                * certificate behind it as well.
+                */
+               httpclient_set_timeout(hc, global_ssl.ocsp_update.http_timeout);
+
                /* if the ocsp_update.http_proxy option was set */
                if (ocsp_update_dst) {
                        hc->options |= HTTPCLIENT_O_HTTPPROXY;
@@ -2086,6 +2097,40 @@ static int ssl_parse_global_ocsp_update_disable(char **args, int section_type, s
        return 0;
 }
 
+static int ssl_parse_global_ocsp_timeout(char **args, int section_type, struct proxy *curpx,
+                                         const struct proxy *defpx, const char *file, int line,
+                                         char **err)
+{
+       const char *res;
+       unsigned int value = 0;
+
+       if (*(args[1]) == 0) {
+               memprintf(err, "'%s' expects an integer argument.", args[0]);
+               return -1;
+       }
+
+       res = parse_time_err(args[1], &value, TIME_UNIT_MS);
+       if (res == PARSE_TIME_OVER) {
+               memprintf(err, "timer overflow in argument <%s> to <%s>, maximum value is 2147483647 ms (~24.8 days).",
+                         args[1], args[0]);
+               return -1;
+       }
+       else if (res == PARSE_TIME_UNDER) {
+               memprintf(err, "timer underflow in argument <%s> to <%s>, minimum non-null value is 1 ms.",
+                         args[1], args[0]);
+               return -1;
+       }
+       else if (res) {
+               memprintf(err, "unsupported character '%c' in '%s' (wants an integer delay).",
+                         *res, args[0]);
+               return -1;
+       }
+
+       global_ssl.ocsp_update.http_timeout = value;
+
+       return 0;
+}
+
 static int ocsp_update_parse_global_http_proxy(char **args, int section_type, struct proxy *curpx,
                                         const struct proxy *defpx, const char *file, int line,
                                         char **err)
@@ -2170,6 +2215,7 @@ static struct cfg_kw_list cfg_kws = {ILH, {
        { CFG_GLOBAL, "ocsp-update.mindelay", ssl_parse_global_ocsp_mindelay },
        { CFG_GLOBAL, "ocsp-update.mode", ssl_parse_global_ocsp_update_mode },
        { CFG_GLOBAL, "ocsp-update.httpproxy", ocsp_update_parse_global_http_proxy },
+       { CFG_GLOBAL, "ocsp-update.timeout", ssl_parse_global_ocsp_timeout },
 #endif
        { 0, NULL, NULL },
 }};

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index aa5f0f0ee084312898043ec40c48ceba38a7f641..1f971d717e7e1eece1679a0a24dc1636673302bc 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -150,6 +150,7 @@ struct global_ssl global_ssl = {
 #ifdef HAVE_SSL_OCSP
        .ocsp_update.delay_max = SSL_OCSP_UPDATE_DELAY_MAX,
        .ocsp_update.delay_min = SSL_OCSP_UPDATE_DELAY_MIN,
+       .ocsp_update.http_timeout = SSL_OCSP_UPDATE_HTTP_TIMEOUT,
        .ocsp_update.mode = SSL_SOCK_OCSP_UPDATE_OFF,
        .ocsp_update.disable = 0,
 #endif