[PATCH v2 0/4] add set server ssl command

2020-10-04 Thread William Dauchy
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

2020-10-04 Thread William Dauchy
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

2020-10-04 Thread William Dauchy
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

2020-10-04 Thread William Dauchy
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`

2020-10-04 Thread William Dauchy
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

2020-10-04 Thread William Dauchy
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`

2020-10-04 Thread William Dauchy
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

2020-10-04 Thread Tim Düsterhus
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`

2020-10-04 Thread Tim Düsterhus
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