[PATCH v2 0/4] add set server ssl command
Hello, This patchset is an attempt to add a new command for configure ssl on server at runtime: - the first patch adds the possibility to observe the change on a `show servers state`. - the two next ones are only here to prepare the last one to add the command. I added them separatly to facilitate the review. `ssl_sock_prepare_srv_ctx` protection is not mandatory but I found it safer while writing my patch. - the last one is adding the new command. I'm not 100% sure of the consequences of`prepare_srv` and `destroy_srv` but from what I read and tested, it seems ok. --- changed in v2: - patch1/4: reorder parameters to match format string - patch3/4: reorder includes, error introduced while splitting my patch. --- William Dauchy (4): MINOR: cli/proxy: add `srv_use_ssl` to `show servers state` MINOR: ssl: protect ssl_sock_prepare_srv_ctx from double ctx allocation MINOR: ssl: create common ssl_ctx init MINOR: cli/ssl: configure ssl on server at runtime doc/management.txt | 4 +++ include/haproxy/server-t.h | 3 ++- include/haproxy/server.h | 2 ++ src/cfgparse-ssl.c | 32 +++--- src/proxy.c| 5 ++-- src/server.c | 55 +- src/ssl_sock.c | 16 --- 7 files changed, 81 insertions(+), 36 deletions(-) -- 2.28.0
[PATCH v2 4/4] MINOR: cli/ssl: configure ssl on server at runtime
in the context of a progressive migration, we want to be able to
activate SSL ciphering on outgoing connections to the server at runtime
without reloading.
This patch adds a `set server ssl` command to allow that:
- call common `srv_init_sslctx` from previous commit rework
- call `prepare_srv` to init ctx when enabling ssl
- call `destroy_srv` to deinit ctx when disabling ssl
- update doc
Signed-off-by: William Dauchy
---
doc/management.txt | 3 +++
include/haproxy/server.h | 1 +
src/server.c | 33 -
3 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/doc/management.txt b/doc/management.txt
index 66c973278..c616a0abb 100644
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -1846,6 +1846,9 @@ set server / fqdn
Change a server's FQDN to the value passed in argument. This requires the
internal run-time DNS resolver to be configured and enabled for this server.
+set server / ssl [ on | off ]
+ This option configures SSL ciphering on outgoing connections to the server.
+
set severity-output [ none | number | string ]
Change the severity output format of the stats socket connected to for the
duration of the current session.
diff --git a/include/haproxy/server.h b/include/haproxy/server.h
index 64951374b..09938fcc1 100644
--- a/include/haproxy/server.h
+++ b/include/haproxy/server.h
@@ -56,6 +56,7 @@ int srv_init_addr(void);
struct server *cli_find_server(struct appctx *appctx, char *arg);
struct server *new_server(struct proxy *proxy);
void srv_init_sslctx(struct server *s);
+void srv_set_ssl(struct server *s, signed char use_ssl);
/* functions related to server name resolution */
int snr_update_srv_status(struct server *s, int has_no_ip);
diff --git a/src/server.c b/src/server.c
index 74f829674..92bc128fc 100644
--- a/src/server.c
+++ b/src/server.c
@@ -1226,6 +1226,28 @@ void srv_init_sslctx(struct server *s)
s->ssl_ctx.methods.max =
global_ssl.connect_default_sslmethods.max;
}
+/* Configure ssl on server .
+ * do nothing if there is no change to apply
+ *
+ * Must be called with the server lock held.
+ */
+void srv_set_ssl(struct server *s, signed char use_ssl)
+{
+ if (s->use_ssl == use_ssl)
+ return;
+
+ s->use_ssl = use_ssl;
+ if (s->use_ssl == 1) {
+ srv_init_sslctx(s);
+
+ if (xprt_get(XPRT_SSL) && xprt_get(XPRT_SSL)->prepare_srv)
+ xprt_get(XPRT_SSL)->prepare_srv(s);
+ } else {
+ if (xprt_get(XPRT_SSL) && xprt_get(XPRT_SSL)->destroy_srv)
+ xprt_get(XPRT_SSL)->destroy_srv(s);
+ s->xprt = xprt_get(XPRT_RAW);
+ }
+}
/* Note: must not be declared as its list will be overwritten.
* Please take care of keeping this list alphabetically sorted, doing so helps
@@ -4402,10 +4424,19 @@ static int cli_parse_set_server(char **args, char
*payload, struct appctx *appct
if (warning)
cli_msg(appctx, LOG_WARNING, warning);
}
+ else if (strcmp(args[3], "ssl") == 0) {
+ if (strcmp(args[4], "on") == 0)
+ srv_set_ssl(sv, 1);
+ else if (strcmp(args[4], "off") == 0)
+ srv_set_ssl(sv, 0);
+ else
+ cli_err(appctx, "'set server ssl' expects 'on' or
'off'.\n");
+ cli_msg(appctx, LOG_NOTICE, "server ssl setting updated.\n");
+ }
else {
cli_err(appctx,
"'set server ' only supports 'agent', 'health',
'state',"
- " 'weight', 'addr', 'fqdn' and 'check-port'.\n");
+ " 'weight', 'addr', 'fqdn', 'check-port' and 'ssl'.\n");
}
out_unlock:
HA_SPIN_UNLOCK(SERVER_LOCK, >lock);
--
2.28.0
[PATCH v2 2/4] MINOR: ssl: protect ssl_sock_prepare_srv_ctx from double ctx allocation
this will be useful if we want to be able to call it at runtime through
the CLI. Not 100% mandatory but might be a good protection for future
use.
Signed-off-by: William Dauchy
---
src/ssl_sock.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index aa9061a6b..0ef7a912b 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -4408,6 +4408,10 @@ int ssl_sock_prepare_srv_ctx(struct server *srv)
if (srv->use_ssl == 1)
srv->xprt = _sock;
+ /* avoid to leak another ctx if ctx is already allocated */
+ if (srv->ssl_ctx.ctx)
+ return cfgerr;
+
ctx = SSL_CTX_new(SSLv23_client_method());
if (!ctx) {
ha_alert("config : %s '%s', server '%s': unable to allocate ssl
context.\n",
@@ -4714,15 +4718,21 @@ int ssl_sock_prepare_bind_conf(struct bind_conf
*bind_conf)
void ssl_sock_free_srv_ctx(struct server *srv)
{
#ifdef TLSEXT_TYPE_application_layer_protocol_negotiation
- if (srv->ssl_ctx.alpn_str)
+ if (srv->ssl_ctx.alpn_str) {
free(srv->ssl_ctx.alpn_str);
+ srv->ssl_ctx.alpn_str = NULL;
+ }
#endif
#ifdef OPENSSL_NPN_NEGOTIATED
- if (srv->ssl_ctx.npn_str)
+ if (srv->ssl_ctx.npn_str) {
free(srv->ssl_ctx.npn_str);
+ srv->ssl_ctx.npn_str = NULL;
+ }
#endif
- if (srv->ssl_ctx.ctx)
+ if (srv->ssl_ctx.ctx) {
SSL_CTX_free(srv->ssl_ctx.ctx);
+ srv->ssl_ctx.ctx = NULL;
+ }
}
/* Walks down the two trees in bind_conf and frees all the certs. The pointer
may
--
2.28.0
[PATCH v2 3/4] MINOR: ssl: create common ssl_ctx init
so we can reuse it later
Signed-off-by: William Dauchy
---
include/haproxy/server.h | 1 +
src/cfgparse-ssl.c | 32 +++-
src/server.c | 22 ++
3 files changed, 26 insertions(+), 29 deletions(-)
diff --git a/include/haproxy/server.h b/include/haproxy/server.h
index f15b7057d..64951374b 100644
--- a/include/haproxy/server.h
+++ b/include/haproxy/server.h
@@ -55,6 +55,7 @@ int srv_set_addr_via_libc(struct server *srv, int *err_code);
int srv_init_addr(void);
struct server *cli_find_server(struct appctx *appctx, char *arg);
struct server *new_server(struct proxy *proxy);
+void srv_init_sslctx(struct server *s);
/* functions related to server name resolution */
int snr_update_srv_status(struct server *s, int has_no_ip);
diff --git a/src/cfgparse-ssl.c b/src/cfgparse-ssl.c
index d22ae96fb..747f7d392 100644
--- a/src/cfgparse-ssl.c
+++ b/src/cfgparse-ssl.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
#include
@@ -1352,19 +1353,7 @@ static int srv_parse_check_sni(char **args, int
*cur_arg, struct proxy *px, stru
static int srv_parse_check_ssl(char **args, int *cur_arg, struct proxy *px,
struct server *newsrv, char **err)
{
newsrv->check.use_ssl = 1;
- if (global_ssl.connect_default_ciphers && !newsrv->ssl_ctx.ciphers)
- newsrv->ssl_ctx.ciphers =
strdup(global_ssl.connect_default_ciphers);
-#if (HA_OPENSSL_VERSION_NUMBER >= 0x10101000L)
- if (global_ssl.connect_default_ciphersuites &&
!newsrv->ssl_ctx.ciphersuites)
- newsrv->ssl_ctx.ciphersuites =
strdup(global_ssl.connect_default_ciphersuites);
-#endif
- newsrv->ssl_ctx.options |= global_ssl.connect_default_ssloptions;
- newsrv->ssl_ctx.methods.flags |=
global_ssl.connect_default_sslmethods.flags;
- if (!newsrv->ssl_ctx.methods.min)
- newsrv->ssl_ctx.methods.min =
global_ssl.connect_default_sslmethods.min;
- if (!newsrv->ssl_ctx.methods.max)
- newsrv->ssl_ctx.methods.max =
global_ssl.connect_default_sslmethods.max;
-
+ srv_init_sslctx(newsrv);
return 0;
}
@@ -1536,22 +1525,7 @@ static int srv_parse_sni(char **args, int *cur_arg,
struct proxy *px, struct ser
static int srv_parse_ssl(char **args, int *cur_arg, struct proxy *px, struct
server *newsrv, char **err)
{
newsrv->use_ssl = 1;
- if (global_ssl.connect_default_ciphers && !newsrv->ssl_ctx.ciphers)
- newsrv->ssl_ctx.ciphers =
strdup(global_ssl.connect_default_ciphers);
-#if (HA_OPENSSL_VERSION_NUMBER >= 0x10101000L)
- if (global_ssl.connect_default_ciphersuites &&
!newsrv->ssl_ctx.ciphersuites)
- newsrv->ssl_ctx.ciphersuites =
strdup(global_ssl.connect_default_ciphersuites);
-#endif
- newsrv->ssl_ctx.options |= global_ssl.connect_default_ssloptions;
- newsrv->ssl_ctx.methods.flags |=
global_ssl.connect_default_sslmethods.flags;
-
- if (!newsrv->ssl_ctx.methods.min)
- newsrv->ssl_ctx.methods.min =
global_ssl.connect_default_sslmethods.min;
-
- if (!newsrv->ssl_ctx.methods.max)
- newsrv->ssl_ctx.methods.max =
global_ssl.connect_default_sslmethods.max;
-
-
+ srv_init_sslctx(newsrv);
return 0;
}
diff --git a/src/server.c b/src/server.c
index b1656d5ce..74f829674 100644
--- a/src/server.c
+++ b/src/server.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -1205,6 +1206,27 @@ void srv_compute_all_admin_states(struct proxy *px)
}
}
+/* Common function to init ssl_ctx
+ */
+void srv_init_sslctx(struct server *s)
+{
+ if (global_ssl.connect_default_ciphers && !s->ssl_ctx.ciphers)
+ s->ssl_ctx.ciphers = strdup(global_ssl.connect_default_ciphers);
+#if (HA_OPENSSL_VERSION_NUMBER >= 0x10101000L)
+ if (global_ssl.connect_default_ciphersuites && !s->ssl_ctx.ciphersuites)
+ s->ssl_ctx.ciphersuites =
strdup(global_ssl.connect_default_ciphersuites);
+#endif
+ s->ssl_ctx.options |= global_ssl.connect_default_ssloptions;
+ s->ssl_ctx.methods.flags |= global_ssl.connect_default_sslmethods.flags;
+
+ if (!s->ssl_ctx.methods.min)
+ s->ssl_ctx.methods.min =
global_ssl.connect_default_sslmethods.min;
+
+ if (!s->ssl_ctx.methods.max)
+ s->ssl_ctx.methods.max =
global_ssl.connect_default_sslmethods.max;
+}
+
+
/* Note: must not be declared as its list will be overwritten.
* Please take care of keeping this list alphabetically sorted, doing so helps
* all code contributors.
--
2.28.0
[PATCH v2 1/4] MINOR: cli/proxy: add `srv_use_ssl` to `show servers state`
The aim is to be able to hot change `ssl` parameter for each server.
Signed-off-by: William Dauchy
---
doc/management.txt | 1 +
include/haproxy/server-t.h | 3 ++-
src/proxy.c| 5 +++--
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/doc/management.txt b/doc/management.txt
index adbad95d3..66c973278 100644
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -2401,6 +2401,7 @@ show servers state []
srv_fqdn:Server FQDN.
srv_port:Server port.
srvrecord: DNS SRV record associated to this SRV.
+ srv_use_ssl: use ssl for server connections.
show sess
Dump all known sessions. Avoid doing this on slow connections as this can
diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h
index decc5e292..0d94a5947 100644
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -122,7 +122,8 @@ enum srv_initaddr {
"srv_f_forced_id "\
"srv_fqdn " \
"srv_port " \
-"srvrecord"
+"srvrecord " \
+"srv_use_ssl"
#define SRV_STATE_FILE_MAX_FIELDS 20
#define SRV_STATE_FILE_NB_FIELDS_VERSION_1 19
diff --git a/src/proxy.c b/src/proxy.c
index 18cdf426e..fe7f43594 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -1930,14 +1930,15 @@ static int dump_servers_state(struct stream_interface
*si)
"%d %s %s "
"%d %d %d %d %ld "
"%d %d %d %d %d "
-"%d %d %s %u %s"
+"%d %d %s %u "
+"%s %d"
"\n",
px->uuid, px->id,
srv->puid, srv->id, srv_addr,
srv->cur_state, srv->cur_admin,
srv->uweight, srv->iweight, (long int)srv_time_since_last_change,
srv->check.status, srv->check.result,
srv->check.health, srv->check.state, srv->agent.state,
bk_f_forced_id, srv_f_forced_id,
srv->hostname ? srv->hostname : "-", srv->svc_port,
-srvrecord ? srvrecord : "-");
+srvrecord ? srvrecord : "-", srv->use_ssl);
} else {
/* show servers conn */
int thr;
--
2.28.0
Re: [PATCH 3/4] MINOR: ssl: create common ssl_ctx init
Hi Tim, Thank you for your answer. On Sun, Oct 4, 2020 at 12:46 PM Tim Düsterhus wrote: > This commit fails to build if USE_OPENSSL is not defined. good catch, I wrongly split my patch. fixed in v2. -- William
Re: [PATCH 1/4] MINOR: cli/proxy: add `srv_use_ssl` to `show servers state`
Hi Tim, Thanks for your answer. On Sun, Oct 4, 2020 at 12:38 PM Tim Düsterhus wrote: > > - srvrecord ? srvrecord : "-"); > > + srvrecord ? srvrecord : "-", > > srv->use_ssl); > > But here you don't. From what I am seeing the line breaks of the > parameters match the line breaks of the format string. it's not entirely true, e.g for srvrecord, but I can't certainly fix that to be more coherent. will send an update in v2. -- William
Re: [PATCH 3/4] MINOR: ssl: create common ssl_ctx init
William, Am 03.10.20 um 23:15 schrieb William Dauchy: > so we can reuse it later > > Signed-off-by: William Dauchy > --- > include/haproxy/server.h | 1 + > src/cfgparse-ssl.c | 31 ++- > src/server.c | 21 + > 3 files changed, 24 insertions(+), 29 deletions(-) > This commit fails to build if USE_OPENSSL is not defined. Best regards Tim Düsterhus
Re: [PATCH 1/4] MINOR: cli/proxy: add `srv_use_ssl` to `show servers state`
William,
Am 03.10.20 um 23:15 schrieb William Dauchy:
> index 18cdf426e..fffd841f8 100644
> --- a/src/proxy.c
> +++ b/src/proxy.c
> @@ -1930,14 +1930,15 @@ static int dump_servers_state(struct stream_interface
> *si)
>"%d %s %s "
>"%d %d %d %d %ld "
>"%d %d %d %d %d "
> - "%d %d %s %u %s"
> + "%d %d %s %u %s "
> + "%d"
Here you add a new line within the format string.
>"\n",
>px->uuid, px->id,
>srv->puid, srv->id, srv_addr,
>srv->cur_state, srv->cur_admin,
> srv->uweight, srv->iweight, (long int)srv_time_since_last_change,
>srv->check.status, srv->check.result,
> srv->check.health, srv->check.state, srv->agent.state,
>bk_f_forced_id, srv_f_forced_id,
> srv->hostname ? srv->hostname : "-", srv->svc_port,
> - srvrecord ? srvrecord : "-");
> + srvrecord ? srvrecord : "-", srv->use_ssl);
But here you don't. From what I am seeing the line breaks of the
parameters match the line breaks of the format string.
> } else {
> /* show servers conn */
> int thr;
>
Best regards
Tim Düsterhus
