Re: Dynamically adding/deleting SSL certificates

[Aurélien Nephtali]

On Fri, Jun 1, 2018 at 11:13 AM, Aurélien Nephtali 
 wrote:
>
> We also need to agree on the payload format to use in the add command:
> only the PEM certificate is supported at the moment but when there
> will be OCSP + SCTL support it will become messy very quick.
> In my tests I am using something like "cert=[...] ocsp=[...]
> issuer=[...] sctl=[...]" but it is not pretty.
> I thought of using an INI file format but it is not very handy if you
> have to craft a file just for one operation.

Another idea would be to add a binary protocol to the CLI and distribute
a tool that would implement this protocol. The add command would be the
first to leverage this protocol to easily upload certificates and all
other stuff that may come with it.

The CLI parser would switch in binary parsing when receiving a special
command (or a special binary pattern).

Having two incompatible ways to speak to the software can be confusing
but as socat is required to speak to haproxy, using another tool may not
be that crazy.

-- 
Aurélien Nephtali

Re: Dynamically adding/deleting SSL certificates

[Aurélien Nephtali]

Hello Willy,

On Thu, May 31, 2018 at 9:02 PM, Willy Tarreau  wrote:
> Hi Aurélien,
>
> On Thu, May 31, 2018 at 11:01:44AM +0200, Aurélien Nephtali wrote:
>> Anyone has more comments, ideas or remarks regarding these patches ?
>
> Not on the patches themselves, but I think I mentioned elsewhere that
> we really need to start an important discussion on the subject of how
> to designate a certificate in general. Indeed :
>   - with crtlists (and even without) it's possible to have overlapping
> identifiers, the first matching one wins. There are even multiple
> key types for a same SNI.
>
>   - within a frontend there can be multiple bind lines. It's perfectly
> possible to use different sets of certs on two distinct lines, for
> example one for the public connections, and another set using the
> internal CA for the internal connections. Thus the frontend+SNI
> may not always be enough. And in fact this type of setup is not
> so rare, especially in places where you have content contributors
> who are required to present their client cert from outside but
> not from inside, where the security is much more relaxed, but
> the internal certs are exclusively used inside with a dedicated
> CA to avoid any accident.

I "addressed" this issue by allowing certificate updates ONLY if the
bind line has a name.
Currently, the only way to specify a particular bind line of a
frontend is using the luid but it is not very user friendly.
The new solution may not be perfect but it is easier to test the
feature this way.

>   - at the moment cert file names are unique (one cert per file only),
> thus at first glance it could sound like files could be used as a
> unique identifier as is done with ACLs and maps. And it would
> ensure that the change can be made both on the FS and on the CLI.
> But with the ability to load certs from dirs, most of the time I
> fear that an agent connecting to a CLI will have no idea at all
> about the cert's original file name. The one adding a new cert
> will figure the name as it currently does to save it before
> reloading. But an agent designed to simply update a cert (eg:
> let's encrypt) could very well be totally unaware of its path.
>
>   - there are probably other unique ways to identify them that I'm
> not thinking about
>
> Thus I think it's important that a decision is taken on this and that
> we don't come back in 3 months saying "hmmm finally it was a bad idea,
> I cannot manage this setup with it".
>
> Ideas or experience feedback from anyone is welcome here.

We also need to agree on the payload format to use in the add command:
only the PEM certificate is supported at the moment but when there
will be OCSP + SCTL support it will become messy very quick.
In my tests I am using something like "cert=[...] ocsp=[...]
issuer=[...] sctl=[...]" but it is not pretty.
I thought of using an INI file format but it is not very handy if you
have to craft a file just for one operation.

The CLI default maximum line size may barely be enough to hold the
certificate but it will definitely not be if OSCP and stuff are used.
This can be addressed by supporting payload streaming like you mentioned before.

-- 
Aurélien Nephtali

Re: Dynamically adding/deleting SSL certificates

[Willy Tarreau]

Hi Aurélien,

On Thu, May 31, 2018 at 11:01:44AM +0200, Aurélien Nephtali wrote:
> Anyone has more comments, ideas or remarks regarding these patches ?

Not on the patches themselves, but I think I mentioned elsewhere that
we really need to start an important discussion on the subject of how
to designate a certificate in general. Indeed :
  - with crtlists (and even without) it's possible to have overlapping
identifiers, the first matching one wins. There are even multiple
key types for a same SNI.

  - within a frontend there can be multiple bind lines. It's perfectly
possible to use different sets of certs on two distinct lines, for
example one for the public connections, and another set using the
internal CA for the internal connections. Thus the frontend+SNI
may not always be enough. And in fact this type of setup is not
so rare, especially in places where you have content contributors
who are required to present their client cert from outside but
not from inside, where the security is much more relaxed, but
the internal certs are exclusively used inside with a dedicated
CA to avoid any accident.

  - at the moment cert file names are unique (one cert per file only),
thus at first glance it could sound like files could be used as a
unique identifier as is done with ACLs and maps. And it would
ensure that the change can be made both on the FS and on the CLI.
But with the ability to load certs from dirs, most of the time I
fear that an agent connecting to a CLI will have no idea at all
about the cert's original file name. The one adding a new cert
will figure the name as it currently does to save it before
reloading. But an agent designed to simply update a cert (eg:
let's encrypt) could very well be totally unaware of its path.

  - there are probably other unique ways to identify them that I'm
not thinking about

Thus I think it's important that a decision is taken on this and that
we don't come back in 3 months saying "hmmm finally it was a bad idea,
I cannot manage this setup with it".

Ideas or experience feedback from anyone is welcome here.

Thanks!
Willy

Re: Dynamically adding/deleting SSL certificates

[Aurélien Nephtali]

Hello Emeric, everyone,

On Wed, May 23, 2018 at 9:39 PM, Aurélien Nephtali
 wrote:
> Hello Emeric,
>
> On Tue, May 22, 2018 at 05:37:58PM +0200, Emeric Brun wrote:
>> Hi Auréline
>>
>> I see that you're using the domain to known the certificate to delete.
>>
>> If you take a look to crt-list, you will see that the identifier of the 
>> certificate
>> is customizable and is not necessarily the domain.
>>
>> I think to perform the adds/delete operation we should use the same 
>> identifiers than the crt-list option
>>
>
> Right, I forgot about these.
> It's now possible to use one of the identifiers (if any) of a
> certificate from a crt-list in the deletion.
>
> I added a refcount in the struct sni_ctx, added wildcard support that I
> forgot and corrected a typo in an error message.
>
> It's still RFC patches.
>

Anyone has more comments, ideas or remarks regarding these patches ?

Thanks !

-- 
Aurélien Nephtali

Re: Dynamically adding/deleting SSL certificates

[Aurélien Nephtali]

Hello Emeric,

On Tue, May 22, 2018 at 05:37:58PM +0200, Emeric Brun wrote:
> Hi Auréline
> 
> I see that you're using the domain to known the certificate to delete.
> 
> If you take a look to crt-list, you will see that the identifier of the 
> certificate
> is customizable and is not necessarily the domain.
> 
> I think to perform the adds/delete operation we should use the same 
> identifiers than the crt-list option
> 

Right, I forgot about these.
It's now possible to use one of the identifiers (if any) of a
certificate from a crt-list in the deletion.

I added a refcount in the struct sni_ctx, added wildcard support that I
forgot and corrected a typo in an error message.

It's still RFC patches.

-- 
Aurélien.
>From 9612ff9dcfda0611e5287ea0e9d9959d972e0061 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= 
Date: Wed, 18 Apr 2018 15:34:33 +0200
Subject: [PATCH 1/2] MINOR/WIP: ssl: Refactor ssl_sock_load_cert_file() into
 ssl_sock_load_cert2()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- ssl_sock_get_dh_from_file() -> ssl_sock_get_dh()
- ssl_sock_load_dh_params() takes a BIO * instead of a char *
- ssl_sock_load_cert_chain_file() -> ssl_sock_load_cert_chain() + takes a
  BIO * instead of a char *

Signed-off-by: Aurélien Nephtali 
---
 src/ssl_sock.c | 210 +
 1 file changed, 136 insertions(+), 74 deletions(-)

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 7a602ad57..eb0d43ded 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -2583,43 +2583,39 @@ static DH *ssl_get_tmp_dh(SSL *ssl, int export, int keylen)
 	return dh;
 }
 
-static DH * ssl_sock_get_dh_from_file(const char *filename)
+static DH * ssl_sock_get_dh(BIO *in)
 {
-	DH *dh = NULL;
-	BIO *in = BIO_new(BIO_s_file());
+	return PEM_read_bio_DHparams(in, NULL, NULL, NULL);
+}
 
-	if (in == NULL)
-		goto end;
+int ssl_sock_load_global_dh_param_from_file(const char *filename)
+{
+	BIO *in;
+	int ret = -1;
+
+	in = BIO_new(BIO_s_file());
+	if (!in)
+		return -1;
 
 	if (BIO_read_filename(in, filename) <= 0)
 		goto end;
 
-	dh = PEM_read_bio_DHparams(in, NULL, NULL, NULL);
+	global_dh = ssl_sock_get_dh(in);
+	if (global_dh)
+		ret = 0;
 
 end:
-if (in)
-BIO_free(in);
+	BIO_free(in);
 
-	return dh;
-}
-
-int ssl_sock_load_global_dh_param_from_file(const char *filename)
-{
-	global_dh = ssl_sock_get_dh_from_file(filename);
-
-	if (global_dh) {
-		return 0;
-	}
-
-	return -1;
+	return ret;
 }
 
 /* Loads Diffie-Hellman parameter from a file. Returns 1 if loaded, else -1
if an error occured, and 0 if parameter not found. */
-int ssl_sock_load_dh_params(SSL_CTX *ctx, const char *file)
+int ssl_sock_load_dh_params(SSL_CTX *ctx, BIO *in)
 {
 	int ret = -1;
-	DH *dh = ssl_sock_get_dh_from_file(file);
+	DH *dh = ssl_sock_get_dh(in);
 
 	if (dh) {
 		ret = 1;
@@ -3192,10 +3188,9 @@ static int ssl_sock_load_multi_cert(const char *path, struct bind_conf *bind_con
 /* Loads a certificate key and CA chain from a file. Returns 0 on error, -1 if
  * an early error happens and the caller must call SSL_CTX_free() by itelf.
  */
-static int ssl_sock_load_cert_chain_file(SSL_CTX *ctx, const char *file, struct bind_conf *s,
-	 struct ssl_bind_conf *ssl_conf, char **sni_filter, int fcount)
+static int ssl_sock_load_cert_chain(SSL_CTX *ctx, BIO *in, struct bind_conf *s,
+struct ssl_bind_conf *ssl_conf, char **sni_filter, int fcount)
 {
-	BIO *in;
 	X509 *x = NULL, *ca;
 	int i, err;
 	int ret = -1;
@@ -3211,14 +3206,6 @@ static int ssl_sock_load_cert_chain_file(SSL_CTX *ctx, const char *file, struct
 	STACK_OF(GENERAL_NAME) *names;
 #endif
 
-	in = BIO_new(BIO_s_file());
-	if (in == NULL)
-		goto end;
-
-	if (BIO_read_filename(in, file) <= 0)
-		goto end;
-
-
 	passwd_cb = SSL_CTX_get_default_passwd_cb(ctx);
 	passwd_cb_userdata = SSL_CTX_get_default_passwd_cb_userdata(ctx);
 
@@ -3308,44 +3295,110 @@ end:
 	if (x)
 		X509_free(x);
 
-	if (in)
-		BIO_free(in);
-
 	return ret;
 }
 
-static int ssl_sock_load_cert_file(const char *path, struct bind_conf *bind_conf, struct ssl_bind_conf *ssl_conf,
-   char **sni_filter, int fcount, char **err)
+struct ssl_input {
+const char *cert_path;
+struct chunk *cert_buf;
+};
+
+static int ssl_sock_load_cert2(struct ssl_input *si, struct bind_conf *bind_conf, struct ssl_bind_conf *ssl_conf,
+   char **sni_filter, int fcount, char **err)
 {
 	int ret;
 	SSL_CTX *ctx;
+	BIO *in;
+
+	if (!si->cert_path && (!si->cert_buf || !si->cert_buf->len))
+		return 1;
 
 	ctx = SSL_CTX_new(SSLv23_server_method());
 	if (!ctx) {
-		memprintf(err, "%sunable to allocate SSL context for cert '%s'.\n",
-		  err && *err ? *err : "", path);
+		if (si->cert_path)
+			memprintf(err, "%sunable to allocate SSL context for cert '%s'.\n",
+  err && *err ? *err : "", 

Re: Dynamically adding/deleting SSL certificates

[Emeric Brun]

Hi Auréline

On 05/18/2018 11:07 AM, Aurélien Nephtali wrote:
> Hello,
> 
> On Wed, Apr 18, 2018 at 9:34 PM, Aurélien Nephtali
>  wrote:
>> Hello,
>>
>> I have some patches to support dynamically loading and unloading PEM
>> certificates through the CLI. It is mainly a big refactoring of some
>> part of the SSL code (thanks Thierry for your patches, we came to the
>> same conclusion :) !).
>>
> 
> Here is an updated version of this feature. The changes are:
> - Use a payload in the CLI to pass the certificate
> - Change the way to specify on which listener the certificate is
> to be added/removed: using the "bind name".
>   If the listeners are not named, the only way to update their
> certificates is to do a global operation (using just the frontend name
> in the command).
> 
> One thing that should be discussed is what will be the command syntax
> when it will support more certificate options (OCSP, SCTL) ?
> I thought about sending something like an .ini file:
> 
> [certificate]
> a===
> 
> [ocsp]
> b===
> 
> etc...
> 
> but one needs to prepare these files: it may not be very handy for a
> one shot operation ?
> Plus, without streaming we're quickly limited by the payload size with
> the default value.
> 

I see that you're using the domain to known the certificate to delete.

If you take a look to crt-list, you will see that the identifier of the 
certificate
is customizable and is not necessarily the domain.

I think to perform the adds/delete operation we should use the same identifiers 
than the crt-list option

R,
Emeric

Re: Dynamically adding/deleting SSL certificates

[Aurélien Nephtali]

Hello,

On Wed, Apr 18, 2018 at 9:34 PM, Aurélien Nephtali
 wrote:
> Hello,
>
> I have some patches to support dynamically loading and unloading PEM
> certificates through the CLI. It is mainly a big refactoring of some
> part of the SSL code (thanks Thierry for your patches, we came to the
> same conclusion :) !).
>

Here is an updated version of this feature. The changes are:
- Use a payload in the CLI to pass the certificate
- Change the way to specify on which listener the certificate is
to be added/removed: using the "bind name".
  If the listeners are not named, the only way to update their
certificates is to do a global operation (using just the frontend name
in the command).

One thing that should be discussed is what will be the command syntax
when it will support more certificate options (OCSP, SCTL) ?
I thought about sending something like an .ini file:

[certificate]
a===

[ocsp]
b===

etc...

but one needs to prepare these files: it may not be very handy for a
one shot operation ?
Plus, without streaming we're quickly limited by the payload size with
the default value.

-- 
Aurélien Nephtali
From bfdd0e89ee79b996cd4a4b05afbde79e5919d8bc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= 
Date: Wed, 18 Apr 2018 15:34:33 +0200
Subject: [PATCH 1/2] MINOR/WIP: ssl: Refactor ssl_sock_load_cert_file() into
 ssl_sock_load_cert2()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- ssl_sock_get_dh_from_file() -> ssl_sock_get_dh()
- ssl_sock_load_dh_params() takes a BIO * instead of a char *
- ssl_sock_load_cert_chain_file() -> ssl_sock_load_cert_chain() + takes a
  BIO * instead of a char *

Signed-off-by: Aurélien Nephtali 
---
 src/ssl_sock.c | 210 +
 1 file changed, 136 insertions(+), 74 deletions(-)

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 7a602ad57..eb0d43ded 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -2583,43 +2583,39 @@ static DH *ssl_get_tmp_dh(SSL *ssl, int export, int keylen)
 	return dh;
 }
 
-static DH * ssl_sock_get_dh_from_file(const char *filename)
+static DH * ssl_sock_get_dh(BIO *in)
 {
-	DH *dh = NULL;
-	BIO *in = BIO_new(BIO_s_file());
+	return PEM_read_bio_DHparams(in, NULL, NULL, NULL);
+}
 
-	if (in == NULL)
-		goto end;
+int ssl_sock_load_global_dh_param_from_file(const char *filename)
+{
+	BIO *in;
+	int ret = -1;
+
+	in = BIO_new(BIO_s_file());
+	if (!in)
+		return -1;
 
 	if (BIO_read_filename(in, filename) <= 0)
 		goto end;
 
-	dh = PEM_read_bio_DHparams(in, NULL, NULL, NULL);
+	global_dh = ssl_sock_get_dh(in);
+	if (global_dh)
+		ret = 0;
 
 end:
-if (in)
-BIO_free(in);
+	BIO_free(in);
 
-	return dh;
-}
-
-int ssl_sock_load_global_dh_param_from_file(const char *filename)
-{
-	global_dh = ssl_sock_get_dh_from_file(filename);
-
-	if (global_dh) {
-		return 0;
-	}
-
-	return -1;
+	return ret;
 }
 
 /* Loads Diffie-Hellman parameter from a file. Returns 1 if loaded, else -1
if an error occured, and 0 if parameter not found. */
-int ssl_sock_load_dh_params(SSL_CTX *ctx, const char *file)
+int ssl_sock_load_dh_params(SSL_CTX *ctx, BIO *in)
 {
 	int ret = -1;
-	DH *dh = ssl_sock_get_dh_from_file(file);
+	DH *dh = ssl_sock_get_dh(in);
 
 	if (dh) {
 		ret = 1;
@@ -3192,10 +3188,9 @@ static int ssl_sock_load_multi_cert(const char *path, struct bind_conf *bind_con
 /* Loads a certificate key and CA chain from a file. Returns 0 on error, -1 if
  * an early error happens and the caller must call SSL_CTX_free() by itelf.
  */
-static int ssl_sock_load_cert_chain_file(SSL_CTX *ctx, const char *file, struct bind_conf *s,
-	 struct ssl_bind_conf *ssl_conf, char **sni_filter, int fcount)
+static int ssl_sock_load_cert_chain(SSL_CTX *ctx, BIO *in, struct bind_conf *s,
+struct ssl_bind_conf *ssl_conf, char **sni_filter, int fcount)
 {
-	BIO *in;
 	X509 *x = NULL, *ca;
 	int i, err;
 	int ret = -1;
@@ -3211,14 +3206,6 @@ static int ssl_sock_load_cert_chain_file(SSL_CTX *ctx, const char *file, struct
 	STACK_OF(GENERAL_NAME) *names;
 #endif
 
-	in = BIO_new(BIO_s_file());
-	if (in == NULL)
-		goto end;
-
-	if (BIO_read_filename(in, file) <= 0)
-		goto end;
-
-
 	passwd_cb = SSL_CTX_get_default_passwd_cb(ctx);
 	passwd_cb_userdata = SSL_CTX_get_default_passwd_cb_userdata(ctx);
 
@@ -3308,44 +3295,110 @@ end:
 	if (x)
 		X509_free(x);
 
-	if (in)
-		BIO_free(in);
-
 	return ret;
 }
 
-static int ssl_sock_load_cert_file(const char *path, struct bind_conf *bind_conf, struct ssl_bind_conf *ssl_conf,
-   char **sni_filter, int fcount, char **err)
+struct ssl_input {
+const char *cert_path;
+struct chunk *cert_buf;
+};
+
+static int ssl_sock_load_cert2(struct ssl_input *si, struct bind_conf *bind_conf, struct ssl_bind_conf *ssl_conf,
+   char **sni_filter, int 

Re: Dynamically adding/deleting SSL certificates

[Aurélien Nephtali]

Hello,

I have some patches to support dynamically loading and unloading PEM
certificates through the CLI. It is mainly a big refactoring of some
part of the SSL code (thanks Thierry for your patches, we came to the
same conclusion :) !).

When loading a PEM certificate, one also wants to load OCSP data/issuer
and/or SCTL. With the current implementation, all of these are fetched
from the filesystem. To have a full "dynamic load/unload" support, all
the functions that deal with these data need to be modified to use
buffers instead.

As a start, here is a draft of two patches that refactors some parts and
adds two new CLI commands:
- add ssl cert  
- del ssl cert  

Only loading a simple PEM is supported for now.
Support could be added gradually (PEM, then OCSP/issuer then SCTL) but
the main goal of this draft is to gather comments whether this is the
good way to go.

Warnings:

Until multi-lines support is added to the CLI, PEM certificates fed
through the CLI must not exceed 60 lines (CLI takes a maximum of 64
words and "add ssl cert" + frontend is 4 words).
The syntax is not very user-friendly as it requires a luid. I didn't
find a better way yet to specify for which 'listener' the add/del
operation has to be performed. Using a name could be better but it's an
optional value in the configuration.

Examples:

# echo "add ssl cert fe $(cat cert.pem | tr '\n' ' ') | socat /tmp/sock1 -
# echo "del ssl cert fe:2 example.com" | socat /tmp/sock1 -

Thanks !

-- 
Aurélien.
>From 46d5169a4dbd88d302c64b586748e22737bdadba Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= 
Date: Wed, 18 Apr 2018 15:34:33 +0200
Subject: [PATCH 1/2] MINOR/WIP: ssl: Refactor ssl_sock_load_cert_file() into
 ssl_sock_load_cert2()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- ssl_sock_get_dh_from_file() -> ssl_sock_get_dh()
- ssl_sock_load_dh_params() takes a BIO * instead of a char *
- ssl_sock_load_cert_chain_file() -> ssl_sock_load_cert_chain() + takes a
  BIO * instead of a char *

Signed-off-by: Aurélien Nephtali 
---
 src/ssl_sock.c | 210 +
 1 file changed, 136 insertions(+), 74 deletions(-)

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 23ad35b18..7543c7a69 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -2583,43 +2583,39 @@ static DH *ssl_get_tmp_dh(SSL *ssl, int export, int keylen)
 	return dh;
 }
 
-static DH * ssl_sock_get_dh_from_file(const char *filename)
+static DH * ssl_sock_get_dh(BIO *in)
 {
-	DH *dh = NULL;
-	BIO *in = BIO_new(BIO_s_file());
+	return PEM_read_bio_DHparams(in, NULL, NULL, NULL);
+}
 
-	if (in == NULL)
-		goto end;
+int ssl_sock_load_global_dh_param_from_file(const char *filename)
+{
+	BIO *in;
+	int ret = -1;
+
+	in = BIO_new(BIO_s_file());
+	if (!in)
+		return -1;
 
 	if (BIO_read_filename(in, filename) <= 0)
 		goto end;
 
-	dh = PEM_read_bio_DHparams(in, NULL, NULL, NULL);
+	global_dh = ssl_sock_get_dh(in);
+	if (global_dh)
+		ret = 0;
 
 end:
-if (in)
-BIO_free(in);
+	BIO_free(in);
 
-	return dh;
-}
-
-int ssl_sock_load_global_dh_param_from_file(const char *filename)
-{
-	global_dh = ssl_sock_get_dh_from_file(filename);
-
-	if (global_dh) {
-		return 0;
-	}
-
-	return -1;
+	return ret;
 }
 
 /* Loads Diffie-Hellman parameter from a file. Returns 1 if loaded, else -1
if an error occured, and 0 if parameter not found. */
-int ssl_sock_load_dh_params(SSL_CTX *ctx, const char *file)
+int ssl_sock_load_dh_params(SSL_CTX *ctx, BIO *in)
 {
 	int ret = -1;
-	DH *dh = ssl_sock_get_dh_from_file(file);
+	DH *dh = ssl_sock_get_dh(in);
 
 	if (dh) {
 		ret = 1;
@@ -3192,10 +3188,9 @@ static int ssl_sock_load_multi_cert(const char *path, struct bind_conf *bind_con
 /* Loads a certificate key and CA chain from a file. Returns 0 on error, -1 if
  * an early error happens and the caller must call SSL_CTX_free() by itelf.
  */
-static int ssl_sock_load_cert_chain_file(SSL_CTX *ctx, const char *file, struct bind_conf *s,
-	 struct ssl_bind_conf *ssl_conf, char **sni_filter, int fcount)
+static int ssl_sock_load_cert_chain(SSL_CTX *ctx, BIO *in, struct bind_conf *s,
+struct ssl_bind_conf *ssl_conf, char **sni_filter, int fcount)
 {
-	BIO *in;
 	X509 *x = NULL, *ca;
 	int i, err;
 	int ret = -1;
@@ -3211,14 +3206,6 @@ static int ssl_sock_load_cert_chain_file(SSL_CTX *ctx, const char *file, struct
 	STACK_OF(GENERAL_NAME) *names;
 #endif
 
-	in = BIO_new(BIO_s_file());
-	if (in == NULL)
-		goto end;
-
-	if (BIO_read_filename(in, file) <= 0)
-		goto end;
-
-
 	passwd_cb = SSL_CTX_get_default_passwd_cb(ctx);
 	passwd_cb_userdata = SSL_CTX_get_default_passwd_cb_userdata(ctx);
 
@@ -3308,44 +3295,110 @@ end:
 	if (x)
 		X509_free(x);
 
-	if (in)
-		BIO_free(in);
-
 	return ret;
 }
 
-static int ssl_sock_load_cert_file(const char *path, 

Re: Dynamically adding/deleting SSL certificates

[Thierry Fournier]

Hi aurelien,

I already look for adding dynamic certificates, and it is a real pain for me.
Note that I look for this one year ago, maybe something changed.
I look this development regarding only the basic usage: dynamically update of
RSA certificates, and I encountered some difficulties:

 - Certificates are also ECDHE and CURVES. I failed because I can’t test
   these kind of certificates.

 - ALPN and Cipher suites. If my memory is good, the cipher suite are stored
   only in the SSL context, and this context is hard or impossible to modify.

 - Certificates are linked with OCSP. No memories about theses.

 - Certificates are linked with SNI. SNI and, no SNI have no common parts
   of code for the certificate storage.

Some of these information are stored only in the SSL Context and I can’t find
any way to retrieve it for creating a new SSL context. I also fail to modify
existing context.

I suggest you to check the update way thought the code between coding the
socket interface.

My conclusion was a little refactoring of the SSL parts for making coherent
structs. Maybe storing some configuration with clear text and into the SSL
context ?

For information, I join my first patches of dynamic SSL certificates and
refactoring patch, before I stop this job because I do not have the requested
time for doing it cleanly.

The patches are provided as is. One serie is on the 1.8 branche, the other one
on the 1.6. Some of these patch are qualif, other ones are work in progress.
Sorry, but the WIP patch have french comments :-)

BR,
Thierry


1.6.9-based-0001-MINOR-ssl-export-the-function-ssl_sock_get_serial.patch
Description: Binary data


1.6.9-based-0002-BUG-MINOR-ssl-Check-malloc-return-code.patch
Description: Binary data


1.6.9-based-0003-BUG-MINOR-ssl-prevent-multiple-entries-for-the-same-.patch
Description: Binary data


1.6.9-based-0004-WIP-certifs-a-la-avol-e.patch
Description: Binary data


1.6.9-based-0005-wip2.patch
Description: Binary data


1.8-based-0001-MINOR-ssl-use-BIO-api-for-reading-certificates.patch
Description: Binary data


1.8-based-0002-MINOR-ssl-split-load-cert-in-two-parts.patch
Description: Binary data


1.8-based-0003-wip.patch
Description: Binary data


1.8-based-0004-wip-2.patch
Description: Binary data


> On 6 Mar 2018, at 15:14, Aurélien Nephtali  
> wrote:
> 
> Willy,
> 
> On Tue, Mar 6, 2018 at 2:30 PM, Willy Tarreau  wrote:
>> More or less. I'd rather return a 3rd case, like with do with samples :
>> "not sure yet" (need more data to decide). That allows the failure and
>> success cases to remain definitive.
> 
> Indeed, that what I was trying to say with: "to wait until a keyword
> handler says it is ready to process the command" :) : it will keep
> saying "need more data" until it can decide and say "OK I continued
> and processed the command" or "I couldn't process it".
> 
>> I just don't know exactly what is *currently* used. And we make it a
>> hard rule not to break existing deployments on purpose. People write
>> management scripts, utilities etc and purposely breaking their API is
>> really not fun at all. This is the *only* reason here.
> 
> And I fully agree with that, I want to add the feature without being
> too intrusive or being non retro-compatible.
> 
>> I think we're in sync on this. Please take a look at OCSP to see how it's
>> currently handled so that we don't have to imagine all possibly stupid
>> cases. Also take a look at {set|add} {acl|map} and I think that should
>> be all for now to get the whole picture of the compatiblity we have to
>> maintain.
>> 
> 
> I will do that and check if everything can fit in.
> 
> Thanks !
> 
> -- 
> Aurélien Nephtali
> 

Re: Dynamically adding/deleting SSL certificates

[Willy Tarreau]

Hello Aurélien,

On Tue, Mar 06, 2018 at 02:13:31PM +0100, Aurélien Nephtali wrote:
> > Probably that we could in fact extend the CLI syntax in a backwards 
> > compatible
> > way :
> >
> >[ ]* *
> >[optional body]
> >
> > Most commands don't use a body. Those using a body have to terminate it 
> > using
> > either its own representation known by the command, or an empty line.
> 
> Wouldn't it be easier/clearer to always require an empty line if there
> is a body ?

In theory I fully agree. The only thing is that I'm uncertain about the
format used by OCSP right now. I think it starts as an argument, I'm not
certain whether or not it continues on multiple lines or not.

> Even if it can be present but optional, an empty line would still
> signal the end of it.

If that doesn't break OCSP, I'm all in favor of this, at least for the
sake of avoiding layering violation : the CLI layer transports the body
without having to understand it and it's only the consumer which decides
if it's complete or not while decoding it.

> > "set ssl ocsp-response" takes a series of words in arguments, or a body.
> > Some commands might even simply support the concatenation of both, ie they
> > start immediately on the argument, detect the block is not completed, and
> > read the rest on the following lines till the end is found (either defined
> > by the data format or by the empty line). This simplifies the syntax of
> > long commands, to support both the first line as the first series of
> > arguments, or as the first line of the body.
> 
> Basically, it boils down to adding multi-lines support to the CLI, right ?

Looks like this indeed.

> The parser will have to be modified to wait until a keyword handler
> says it is ready to process the command if it has every argument it
> needs (continue) or some are missing (fail).

More or less. I'd rather return a 3rd case, like with do with samples :
"not sure yet" (need more data to decide). That allows the failure and
success cases to remain definitive.

> > E.g. we could write (just using maps as a simple example) :
> >
> >add map foo.txt foo1 bar1
> >
> > or:
> >
> >add map foo.txt
> >foo1 bar1
> >foo2 bar2
> >LF
> 
> [1]
> 
> >
> > For OCSP, we could have :
> >
> >set ssl ocsp-response line1
> >line2
> >line3
> >line4===
> >
> > or :
> >set ssl ocsp-response line1 line2 line3 line4===
> >
> > or :
> >set ssl ocsp-response
> >line1
> >line2
> >line3
> >line4===
> 
> Is there an advantage to allow these two syntaxes versus always the
> one in [1] ? Why not always requiring an empty line if multi-line is
> used ?

I just don't know exactly what is *currently* used. And we make it a
hard rule not to break existing deployments on purpose. People write
management scripts, utilities etc and purposely breaking their API is
really not fun at all. This is the *only* reason here. However since
I'm not sure about the current OCSP syntax, anything which still works
will be fine. For example, if it supports words passed as arguments
and doesn't use any extra line, we can simply make it accept either
an OCSP response passed as arguments, in which case there is no body,
or no argument at all in which case a body is expected, and terminated
by the empty line.

> That would still require cooperation from the handlers but it
> will stay simple: do I have enough data to proceed ?

That's why the "need more info" feedback from the handlers is needed,
and why (if possible), avoiding having to feed unterminated bodies to
these handlers "just in case" is better. Now that I'm thinking about
it, I don't see how the OCSP would currently consume extra lines, so
maybe it only works using arguments and we're already fine.

> In the case of ocsp-response, without an empty line, it would mean
> trying to decode the whole buffer at each new data - or maybe just
> what was added if base64dec() is modified to support partial data.
> Waiting for an empty line would simplify the logic by only decoding
> the buffer once - this is an example for ocsp-response but I think
> processing arguments once is easier than processing them at each new
> data.

I wholeheartly agree as long as we don't break existing ones ;-)
I'm even fine with not documenting the deprecated behaviours and only
document the new one to encourage migration.

> Requiring an empty line would also solve the case of commands with an
> unknown number of arguments. I do not know if there are some yet but
> they could then safely be added.

Indeed, like what I proposed for maps/acls.

> In the particular case of a PEM certificate, the handler would wait
> for an empty line and it would know it can treat what is after the
> known arguments as being the certificate without trying to guess it is
> complete - in the case where there would be no empty line.

Definitely!

I think we're in sync on this. Please take a look at OCSP to see how it's
currently handled so that we don't have to 

Re: Dynamically adding/deleting SSL certificates

[Aurélien Nephtali]

Hello Willy,

On Mon, Mar 5, 2018 at 8:37 PM, Willy Tarreau  wrote:
> Quotes could be part of some future statements and we'd
> possibly regret having used them if already used for this. For example we 
> could
> imagine one day uploading some JSON parts for certain things.

True, but it could also be addressed by switching the input mode to a
certain flavour (JSON, XML, whatever) but it is also true that
handling multiple lines also solves the problem and does not require
supporting quotes.

> Probably that we could in fact extend the CLI syntax in a backwards compatible
> way :
>
>[ ]* *
>[optional body]
>
> Most commands don't use a body. Those using a body have to terminate it using
> either its own representation known by the command, or an empty line.

Wouldn't it be easier/clearer to always require an empty line if there
is a body ?
Even if it can be present but optional, an empty line would still
signal the end of it.

> "set ssl ocsp-response" takes a series of words in arguments, or a body.
> Some commands might even simply support the concatenation of both, ie they
> start immediately on the argument, detect the block is not completed, and
> read the rest on the following lines till the end is found (either defined
> by the data format or by the empty line). This simplifies the syntax of
> long commands, to support both the first line as the first series of
> arguments, or as the first line of the body.

Basically, it boils down to adding multi-lines support to the CLI, right ?
The parser will have to be modified to wait until a keyword handler
says it is ready to process the command if it has every argument it
needs (continue) or some are missing (fail).

>
> E.g. we could write (just using maps as a simple example) :
>
>add map foo.txt foo1 bar1
>
> or:
>
>add map foo.txt
>foo1 bar1
>foo2 bar2
>LF

[1]

>
> For OCSP, we could have :
>
>set ssl ocsp-response line1
>line2
>line3
>line4===
>
> or :
>set ssl ocsp-response line1 line2 line3 line4===
>
> or :
>set ssl ocsp-response
>line1
>line2
>line3
>line4===

Is there an advantage to allow these two syntaxes versus always the
one in [1] ? Why not always requiring an empty line if multi-line is
used ? That would still require cooperation from the handlers but it
will stay simple: do I have enough data to proceed ?
In the case of ocsp-response, without an empty line, it would mean
trying to decode the whole buffer at each new data - or maybe just
what was added if base64dec() is modified to support partial data.
Waiting for an empty line would simplify the logic by only decoding
the buffer once - this is an example for ocsp-response but I think
processing arguments once is easier than processing them at each new
data.

Requiring an empty line would also solve the case of commands with an
unknown number of arguments. I do not know if there are some yet but
they could then safely be added.

In the particular case of a PEM certificate, the handler would wait
for an empty line and it would know it can treat what is after the
known arguments as being the certificate without trying to guess it is
complete - in the case where there would be no empty line.

Thanks !

-- 
Aurélien Nephtali

Re: Dynamically adding/deleting SSL certificates

[Aurélien Nephtali]

Hello Willy,

On Mon, Mar 5, 2018 at 7:25 PM, Willy Tarreau  wrote:
> I tend to think (first idea out of my head) that for such file types,
> we could very well consider that the command reads multiple lines and
> stops at the first empty line. That's very convenient to use in scripts
> and even by hand in copy-paste sessions. It would work with almost all
> of the data types we have to feed via the CLI, including the maps/acls.

It looks like a clean way to do.

> And a script writing there would just have to run grep -v "^$" to be
> save, which is pretty easy.
>
> In fact that's already the format used for the output : the output of
> each command is defined as running till the first empty line.
>
> I also thought about escaping end of lines with a backslash but that
> becomes very painful to place in scripts.

Yes, I also thought about that but discarded the idea since I wanted
something that could easily be used on the command line without major
data preprocessing.

> Just my two cents, I'm also interested in people's ideas regarding this.

Thanks for the comments, I will think about it and continue to monitor
other ideas!

-- 
Aurélien Nephtali

Re: Dynamically adding/deleting SSL certificates

[Willy Tarreau]

Hi Pieter,

On Mon, Mar 05, 2018 at 07:55:17PM +0100, PiBa-NL wrote:
> I would think the ocsp updates already does something similar with base64.

Yes, I thought about OCSP as well since I think it was the first one to
require large blocks. However OCSP contains a single binary record that
it totally makes sense to encode using base64. Regarding PEM, it's an
envelope containing multiple records which are already encoded. Thus the
format already contains delimiters.

> That would be usable for other binary files as well.?. Though i guess .pem
> is kinda readable already and not a binary file..

Exactly, it looks clean and the format specification mandates that readers
must ignore what is located between labels, and be tolerant regarding spacing,
which also implies that empty lines have no meaning for the encoding, thus
that we will not need to be able to send them as part of the uploaded data.

> Unless perhaps support for
> pfx files would get added some day.?. afaik those are in binary format..

I don't know pfx, so I can't say.

> root@server:/etc/haproxy# echo "set ssl ocsp-response $(/usr/bin/base64 -w
> 1 /etc/haproxy/star_mydomain_com.crt.ocsp)" | socat stdio
> unix-connect:/run/haproxy/admin.sock
> OCSP Response updated!
> 
> Not that i have a strong preference, but imho it would be nice to keep the
> way to call similar commands the the same.

I agree on this, which is also why I'm trying to think about other stuff that
we may have to upload. Using an end tag like "EOF" or any such thing would be
problematic for ACLs or maps as it would imply we would not be able to upload
this one as a pattern. Quotes could be part of some future statements and we'd
possibly regret having used them if already used for this. For example we could
imagine one day uploading some JSON parts for certain things.

In the end, PEM and base64 are not much different. We could possibly consider
that :
  - anything purely binary must absolutely be uploaded as base64 and that the
end is determined by the base64 stream

  - text-based multi-line blocks (like PEM) could be uploaded as-is as long
as they don't collide with our delimiters, and in this case we could use
an empty line to mark the end. At least this would work for maps, acls,
PEM and JSON, and even HTTP headers (if one day we want to implement a
debug mode allowing us to build a request from the CLI and send it to a
server, or if we want to modify some health check strings).

I'm not saying it's anything great nor awesome, but at the moment I can't
think about counter-examples that could painfully break it, nor be hard to
handle (even in Lua it shouldn't be hard, since Lua can register CLI keywords).

Probably that we could in fact extend the CLI syntax in a backwards compatible
way :

   [ ]* *
   [optional body]

Most commands don't use a body. Those using a body have to terminate it using
either its own representation known by the command, or an empty line.
"set ssl ocsp-response" takes a series of words in arguments, or a body.
Some commands might even simply support the concatenation of both, ie they
start immediately on the argument, detect the block is not completed, and
read the rest on the following lines till the end is found (either defined
by the data format or by the empty line). This simplifies the syntax of
long commands, to support both the first line as the first series of
arguments, or as the first line of the body.

E.g. we could write (just using maps as a simple example) :

   add map foo.txt foo1 bar1

or:

   add map foo.txt
   foo1 bar1
   foo2 bar2
   LF

For OCSP, we could have :

   set ssl ocsp-response line1
   line2
   line3
   line4===

or :
   set ssl ocsp-response line1 line2 line3 line4===

or :
   set ssl ocsp-response
   line1
   line2
   line3
   line4===

For PEM certs we'd probably never read from the arguments since we don't
yet support this (no need for backwards compatiblity).

Just my two cents.

Cheers,
Willy

Re: Dynamically adding/deleting SSL certificates

[PiBa-NL]

Hi,
Op 5-3-2018 om 19:25 schreef Willy Tarreau:

Hello Aurélien,

On Mon, Mar 05, 2018 at 03:34:11PM +0100, Aurélien Nephtali wrote:

Hello,

I'm working on a feature to add or delete SSL certificates without
reloading HAProxy and I'm facing a problem regarding the way to feed
the new certificates to the admin socket.

The certificates contain \n so the parser will trip on them and
incorrectly process the command.

Those are my ideas so far:

 - base64 the certificate content,
 - add a binary protocol to the socket to handle this special case
(intrusive, not the best idea),
 - add support for quotes.

(some months ago there was also an idea in
https://www.mail-archive.com/haproxy@formilux.org/msg23857.html)

What would be the best/upstreamable way to do ?

I tend to think (first idea out of my head) that for such file types,
we could very well consider that the command reads multiple lines and
stops at the first empty line. That's very convenient to use in scripts
and even by hand in copy-paste sessions. It would work with almost all
of the data types we have to feed via the CLI, including the maps/acls.

And a script writing there would just have to run grep -v "^$" to be
save, which is pretty easy.

In fact that's already the format used for the output : the output of
each command is defined as running till the first empty line.

I also thought about escaping end of lines with a backslash but that
becomes very painful to place in scripts.

Just my two cents, I'm also interested in people's ideas regarding this.

Thanks,
Willy

I would think the ocsp updates already does something similar with 
base64. That would be usable for other binary files as well.?. Though i 
guess .pem is kinda readable already and not a binary file.. Unless 
perhaps support for pfx files would get added some day.?. afaik those 
are in binary format..


root@server:/etc/haproxy# echo "set ssl ocsp-response $(/usr/bin/base64 
-w 1 /etc/haproxy/star_mydomain_com.crt.ocsp)" | socat stdio 
unix-connect:/run/haproxy/admin.sock

OCSP Response updated!

Not that i have a strong preference, but imho it would be nice to keep 
the way to call similar commands the the same.


Regards,

PiBa-NL (Pieter)

Re: Dynamically adding/deleting SSL certificates

[Willy Tarreau]

Hello Aurélien,

On Mon, Mar 05, 2018 at 03:34:11PM +0100, Aurélien Nephtali wrote:
> Hello,
> 
> I'm working on a feature to add or delete SSL certificates without
> reloading HAProxy and I'm facing a problem regarding the way to feed
> the new certificates to the admin socket.
> 
> The certificates contain \n so the parser will trip on them and
> incorrectly process the command.
> 
> Those are my ideas so far:
> 
> - base64 the certificate content,
> - add a binary protocol to the socket to handle this special case
> (intrusive, not the best idea),
> - add support for quotes.
> 
> (some months ago there was also an idea in
> https://www.mail-archive.com/haproxy@formilux.org/msg23857.html)
> 
> What would be the best/upstreamable way to do ?

I tend to think (first idea out of my head) that for such file types,
we could very well consider that the command reads multiple lines and
stops at the first empty line. That's very convenient to use in scripts
and even by hand in copy-paste sessions. It would work with almost all
of the data types we have to feed via the CLI, including the maps/acls.

And a script writing there would just have to run grep -v "^$" to be
save, which is pretty easy.

In fact that's already the format used for the output : the output of
each command is defined as running till the first empty line.

I also thought about escaping end of lines with a backslash but that
becomes very painful to place in scripts.

Just my two cents, I'm also interested in people's ideas regarding this.

Thanks,
Willy