Re: Using different sources when connecting to a server

[Aurélien Nephtali]

Hello Baptiste,

On Wed, Jul 4, 2018 at 1:07 PM, Baptiste  wrote:
> Hi Aurélien,
>
> My 2 cents.
>
>> I'm trying to add a feature which allows HAProxy to use more than one
>> source when connecting to a server of a backend. The main reason is to
>> avoid duplicating the 'server' lines to reach more than 64k connections
>> from HAProxy to one server.
>
>
> Cool!
>
>>
>> So far I thought of two ways:
>> - each time the 'source' keyword is encountered on a 'server' line,
>>   duplicate the original 'struct server' and fill 'conn_src' with
>>   the correct source informations. It's easy to implement but does
>>   not scale at all. In fact it mimics the multiple 'server' lines.
>>   The big advantage is that it can use all existing features that
>>   deal with 'struct server' (balance keyword, for example).
>> - use a list of 'struct conn_src' in 'struct server' and 'struct
>>   proxy' and choose the best source (using round-robbin, leastconn,
>>   etc...) when a connection is about to get established.
>
>
> I also prefer the second option.
> So we would have 2 LBing algorithm? One to choose the server and one to
> choose the source IP to use?

It depends. Considering this feature could be (only ?) useful to address the 64k
maximum connections, maybe hardcoding a leastconn algorithm is enough.

>>
>> The config. syntax would look like this:
>>
>> server srv 127.0.0.1:9000 source 127.0.0.2 source 127.0.0.3 source
>> 127.0.0.4 source 127.0.0.5 source 127.0.0.6 source 127.0.1.0/24
>>
>> Not using ip1,ip2,ip/cidr,... avoids confusion when using keywords like
>> usesrc, interface, etc...
>
>
> Sure, but at least, I don't want to set 255 source for a "source
> 10.0.0.0/24", so please confirm you'll still allow CIDR notation.

Yes, look at the last 'source' from my config. line example. What I found
tedious is to use something like this:

server srv 127.0.0.1:9000 source
127.0.0.2,127.0.0.3,127.0.0.4,127.0.1.0/24 usesrc clientip,client
[...]

>>
>> Checks to the server would be done from each source but it can be very
>> slow to cover the whole range.
>
>
> I would make this optional. From a pure LBing safety point of view, I
> understand the requirement.
> That said, in some cases, we may not want to run tens or hundreds of health
> checks per second.
> I see different options:
> - check from all source IP
> - check from the host IP address (as of no source is configured)
> - check from one source IP per source subnet
>
>>
>> The main problem I see is how to efficiently store all sources for each
>> server. Using the CIDR syntax can quickly allow millions of sources to
>> be used and if we want to use algorithms like 'leastconn', we need to
>> remember how many connections are still active on a particular source
>> (using round-robbin + an index into the range would otherwise have been
>> one solution)
>> I have some ideas but I would like to know the preferred way.
>
>
> Well, storing a 32 bit hash of  and counting on this
> pattern (and automatically eject server source+dest IP which have reached
> 64K concurrent connections).

Using a leastconn algorithm with very long connections will quickly fill the
list/tree with entries with a counter of 1.

>
> I have a question: what would be the impact on "retries" ? At first, we
> could use it as of today. But later, we may want to retry from a different
> source IP.

-- 
Aurélien Nephtali

Using different sources when connecting to a server

[Aurélien Nephtali]

Hello,

I'm trying to add a feature which allows HAProxy to use more than one
source when connecting to a server of a backend. The main reason is to
avoid duplicating the 'server' lines to reach more than 64k connections
from HAProxy to one server.

So far I thought of two ways:
- each time the 'source' keyword is encountered on a 'server' line,
  duplicate the original 'struct server' and fill 'conn_src' with
  the correct source informations. It's easy to implement but does
  not scale at all. In fact it mimics the multiple 'server' lines.
  The big advantage is that it can use all existing features that
  deal with 'struct server' (balance keyword, for example).
- use a list of 'struct conn_src' in 'struct server' and 'struct
  proxy' and choose the best source (using round-robbin, leastconn,
  etc...) when a connection is about to get established.

The config. syntax would look like this:

server srv 127.0.0.1:9000 source 127.0.0.2 source 127.0.0.3 source 127.0.0.4 
source 127.0.0.5 source 127.0.0.6 source 127.0.1.0/24

Not using ip1,ip2,ip/cidr,... avoids confusion when using keywords like
usesrc, interface, etc...

Checks to the server would be done from each source but it can be very
slow to cover the whole range.

The main problem I see is how to efficiently store all sources for each
server. Using the CIDR syntax can quickly allow millions of sources to
be used and if we want to use algorithms like 'leastconn', we need to
remember how many connections are still active on a particular source
(using round-robbin + an index into the range would otherwise have been
one solution)
I have some ideas but I would like to know the preferred way.

Thanks.

-- 
Aurélien Nephtali

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

[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?= <aurelien.nepht...@corp.ovh.com>
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 <aurelien.nepht...@corp.ovh.com>
---
 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_p

Re: Dynamically adding/deleting SSL certificates

[Aurélien Nephtali]

Hello,

On Wed, Apr 18, 2018 at 9:34 PM, Aurélien Nephtali
<aurelien.nepht...@gmail.com> 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?= <aurelien.nepht...@corp.ovh.com>
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 <aurelien.nepht...@corp.ovh.com>
---
 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

Re: [PATCH] MEDIUM: cli: Add multi-line mode support

[Aurélien Nephtali]

Willy,

On Thu, Apr 26, 2018 at 12:32 PM, Willy Tarreau <w...@1wt.eu> wrote:
> Thanks for this. All of this looks OK to me. Please just let me know if
> you want me to merge them now or if you expect other adjustments.

I think it's OK for me.

Thanks !

-- 
Aurélien Nephtali

Re: [PATCH] MEDIUM: cli: Add multi-line mode support

[Aurélien Nephtali]

Hello Willy,

Sorry for the delay.

Here are the three amended patches.

Changes:
- update the documentation
- add some comments regarding the detection of the payload pattern
  and the input that is zero terminated
- use appctx->chunk to gather data without using the trash
- allow semicolons in a payload (NEW)
- make "add map" payload parsing clearer
- fix "set ssl ocsp-response" payload parsing
- don't skip whitespaces at the beginning of the payload (the less it
  is modified the better it is, I think)

"0001-wip.patch" is an incremental patch for the review.

Thanks.

-- 
Aurélien.
From 1c7b61400e1647cb0e9dd783375f24336352f464 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Wed, 18 Apr 2018 13:26:46 +0200
Subject: [PATCH 1/3] MEDIUM: cli: Add payload support
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In order to use arbitrary data in the CLI (multiple lines or group of words
that must be considered as a whole, for example), it is now possible to add a
payload to the commands. To do so, the first line needs to end with a special
pattern: <<\n. Everything that follows will be left untouched by the CLI parser
and will be passed to the commands parsers.

Per-command support will need to be added to take advantage of this
feature.

Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
 doc/management.txt |  12 +++
 include/proto/applet.h |   4 +-
 include/proto/cli.h|   1 -
 include/types/applet.h |   6 +-
 include/types/cli.h|   2 +-
 src/cache.c|   2 +-
 src/cli.c  | 281 +++--
 src/dns.c  |   2 +-
 src/hlua.c |   2 +-
 src/map.c  |  12 +--
 src/proto_http.c   |   2 +-
 src/proxy.c|  16 +--
 src/server.c   |  20 ++--
 src/ssl_sock.c |   6 +-
 src/stats.c|   6 +-
 src/stick_table.c  |   2 +-
 src/stream.c   |   6 +-
 17 files changed, 237 insertions(+), 145 deletions(-)

diff --git a/doc/management.txt b/doc/management.txt
index 4b6901851..bca0fd469 100644
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -1298,6 +1298,18 @@ delimiter to mark an end of output for each command, and takes care of ensuring
 that no command can emit an empty line on output. A script can thus easily
 parse the output even when multiple commands were pipelined on a single line.
 
+Some commands may take an optional payload. To add one to a command, the first
+line needs to end with the "<<\n" pattern. The next lines will be treated as
+the payload and can contain as many lines as needed. To validate a command with
+a payload, it needs to end with an empty line.
+
+Limitations do exist: the length of the whole buffer passed to the CLI must
+not be greater than tune.bfsize and the pattern "<<" must not be glued to the
+last word of the line.
+
+When entering a paylod while in interactive mode, the prompt will change from
+"> " to "+ ".
+
 It is important to understand that when multiple haproxy processes are started
 on the same sockets, any process may pick up the request and will output its
 own stats.
diff --git a/include/proto/applet.h b/include/proto/applet.h
index cdd4d90f0..7cc2c0ad1 100644
--- a/include/proto/applet.h
+++ b/include/proto/applet.h
@@ -43,11 +43,13 @@ static int inline appctx_res_wakeup(struct appctx *appctx);
 
 /* Initializes all required fields for a new appctx. Note that it does the
  * minimum acceptable initialization for an appctx. This means only the
- * 3 integer states st0, st1, st2 are zeroed.
+ * 3 integer states st0, st1, st2 and the chunk used to gather unfinished
+ * commands are zeroed
  */
 static inline void appctx_init(struct appctx *appctx, unsigned long thread_mask)
 {
 	appctx->st0 = appctx->st1 = appctx->st2 = 0;
+	appctx->chunk = NULL;
 	appctx->io_release = NULL;
 	appctx->thread_mask = thread_mask;
 	appctx->state = APPLET_SLEEPING;
diff --git a/include/proto/cli.h b/include/proto/cli.h
index d5feb862a..da80af7d3 100644
--- a/include/proto/cli.h
+++ b/include/proto/cli.h
@@ -24,7 +24,6 @@
 #define _PROTO_CLI_H
 
 
-struct cli_kw* cli_find_kw(char **args);
 void cli_register_kw(struct cli_kw_list *kw_list);
 
 int cli_has_level(struct appctx *appctx, int level);
diff --git a/include/types/applet.h b/include/types/applet.h
index 89c318c1d..b0715866e 100644
--- a/include/types/applet.h
+++ b/include/types/applet.h
@@ -50,6 +50,9 @@ struct applet {
 #define APPLET_WOKEN_UP 0x02  /* applet was running and requested to woken up again */
 #define APPLET_WANT_DIE 0x04  /* applet was running and requested to die */
 
+#define APPCTX_CLI_ST1_PROMPT  (1 << 0)
+#define APPCTX_CLI_ST1_PAYLOAD (1 << 1)
+
 /* Context o

Re: [PATCH] MEDIUM: cli: Add multi-line mode support

[Aurélien Nephtali]

Hello Willy,

On Fri, Apr 20, 2018 at 05:12:22PM +0200, Willy Tarreau wrote:
> Hi Aurélien,
>
> It seems to me that some places in the parser look for "<<" anywhere on the
> line (mainly the strstr() which even skips trailing spaces/tabs), and some
> parts of the logic only expect it at the end.
>
> I'm personally perfectly fine with being strict at the beginning and doing
> as you documented, ie using "<<\n" as the delimiter so that users don't get
> used to start to put it anywhere. I noticed anyway that it's not recognized
> as the delimiter if not at the end, but the logic seems to check for it.

That's right. The logic in cli_io_handler() looks for the pattern at the end of
the first line received. In cli_parse_request(), before the
tokenization, it will
rescan the input to look for the pattern again only if it knows it is there (the
flag APPCTX_CLI_ST1_PAYLOAD is set). I thought about storing its position
to avoid this second lookup but felt using an extra field for that
would not be that
good considering the parsing is not something done millions of times per
second. I don't like doing/storing things multiple times but here I think it's a
fair trade-off.
I will add some comments since I can see how it can be disturbing to see two
different logics.

> > + p = appctx->chunk->str;
> > + end = p + appctx->chunk->len;
> > +
> > + /* look for a payload */
> > + if (appctx->st1 & APPCTX_CLI_ST1_PAYLOAD) {
> > + payload = strstr(p, PAYLOAD_PATTERN);
> > + end = payload;
> > + /* skip the pattern */
> > + payload += strlen(PAYLOAD_PATTERN);
> > + /* skip whitespaces */
> > + payload += strspn(payload, " \t");
> > + }
>
> Be extremely careful with the str* functions in haproxy. Due to being used
> to process buffers in-place, most of the time our strings are *not* zero-
> terminated, which is why they're often put in chunks made of (str,len),
> or the new immediate strings (ist) also made of (ptr,len). The internal
> API is not very rich regarding this so some functions are implemented
> like strnistr() and others using ist like istist() which does the same
> as strstr() but on ist strings. It's among the things we have to do to
> unify the internal strings API as for now many operations are simply
> open-coded. At the very least, wherever the chunk is filled, a comment
> should indicate that the trailing zero is assumed to be present in the
> rest of the code, because the risk of the code being changed without
> keeping it is high.

I took extra care to be sure I was dealing only with C-strings because
even if it's not very fun to parse strings in C, it's even worse when
the are not zero terminated (hello nginx) and you always end up needing
a str-like() function the internal API does not provide. I will also
add comments.

> > + chunk_appendf(appctx->chunk, "%s", trash.str);
>
> Given that this will result in appctx->chunk always being equal to trash.str,
> I think you should take a look at cli_io_handler() to check if it still makes
> sense at all to use the trash there. Maybe you should simply rely on this
> allocated chunk all the time and save this copy and formatting.

Mmh, yes, I think we can read input data into appctx->chunk and drop the trash
variable.

>
> > From 63439f2b089613973f72a37dd5dda17f45f26545 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= 
> > Date: Wed, 18 Apr 2018 14:04:47 +0200
> > Subject: [PATCH 2/3] MINOR: map: Add payload support to "add map"
> (...)
> > diff --git a/src/map.c b/src/map.c
> > index d02a0255c..64222dc2e 100644
> > --- a/src/map.c
> > +++ b/src/map.c
> > + const char *end = payload + strlen(payload);
> > +
> > + while (payload < end) {
> (...)
> > + /* value */
> > + payload += strspn(payload, " \t");
> > + value = payload;
> > + l = strcspn(value, "\n");
> > + value[l] = 0;
> > + payload += l + 1;
>
> I think this one is not exactly good, as a string ending in "123\0" will
> cause payload to point past the \0. While there's theorically nothing
> wrong with this, and we know that on all supported architecture, ~0 is
> already not a valid pointer so this will not cause payload to become < end,
> it can at least be confusing when debugging. I'd rather do :
>
> payload += l;
> if (*payload)
> payload++;

Yes, 'payload' will go past the end of the string and that is why I
use 'end' and stop
dereferencing 'payload' after the increment but I totally see why it
can confuse people.

>
> > From 1b7f6c40b010521a8b55103c984d3c8f305e818c Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= 
> > Date: Wed, 18 Apr 2018 14:04:58 +0200
> > Subject: [PATCH 3/3] MINOR: ssl: Add payload support to "set ssl
> (...)
> > --- a/src/ssl_sock.c
> > +++ b/src/ssl_sock.c
> > @@ -8565,16 +8565,27 @@ static int cli_parse_set_ocspresponse(char **args, 
> > char *payload, struct appctx
> >  {
> >  #if (defined SSL_CTRL_SET_TLSEXT_STATUS_REQ_CB && !defined 

Re: [PATCH] MEDIUM: cli: Add multi-line mode support

[Aurélien Nephtali]

Hello Willy,

On Thu, Apr 19, 2018 at 8:51 AM, Willy Tarreau <w...@1wt.eu> wrote:

> Hoping this helps,

Yes, I hope it does too.

I guess I was sent off-track by the will to make something very flexible
(at least in my mind) despite your multiple good examples, sorry about
that.

Anyway, here are three patches that, I hope, will do what is needed:
- add a way to specify an optional payload by using << at the end of
  the first line of a command
- add payload support to "add map"
- add payload support to "set ssl ocsp-response"

More commands could benefit from using a payload but for a start I only
did these two.

Thanks for all your time, I really appreciated it.

-- 
Aurélien Nephtali
>From f7b1a17afff1d054d02300535f03c7921e8ef7df Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Wed, 18 Apr 2018 13:26:46 +0200
Subject: [PATCH 1/3] MEDIUM: cli: Add payload support
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In order to use arbitrary data in the CLI (multiple lines or group of words
that must be considered as a whole, for example), it is now possible to add a
payload to the commands. To do so, the first line needs to end with a special
pattern: <<\n. Everything that follows will be left untouched by the CLI parser
and will be passed to the commands parsers.

Per-command support will need to be added to take advantage of this
feature.

Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
 doc/management.txt |  17 
 include/proto/applet.h |   4 +-
 include/proto/cli.h|   1 -
 include/types/applet.h |   6 +-
 include/types/cli.h|   2 +-
 src/cache.c|   2 +-
 src/cli.c  | 228 ++---
 src/dns.c  |   2 +-
 src/hlua.c |   2 +-
 src/map.c  |  12 +--
 src/proto_http.c   |   2 +-
 src/proxy.c|  16 ++--
 src/server.c   |  20 ++---
 src/ssl_sock.c |   6 +-
 src/stats.c|   6 +-
 src/stick_table.c  |   2 +-
 src/stream.c   |   6 +-
 17 files changed, 205 insertions(+), 129 deletions(-)

diff --git a/doc/management.txt b/doc/management.txt
index 4b6901851..5fe0d2e8e 100644
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -1298,6 +1298,23 @@ delimiter to mark an end of output for each command, and takes care of ensuring
 that no command can emit an empty line on output. A script can thus easily
 parse the output even when multiple commands were pipelined on a single line.
 
+Some commands may take an optional payload. To add one to a command, the first
+line needs to end with the "<<\n" pattern. The next lines will be treated as
+the payload and can contain as many lines as needed. To validate a command with
+a payload, it needs to end with an empty line.
+
+Limitations do exist: the length of the whole buffer passed to the CLI must
+not be greater than tune.bfsize and the pattern "<<" must not be glued to the
+last word of the line.
+
+When entering a paylod while in interactive mode, the prompt will change from
+"> " to "+ ".
+
+Example:
+
+# echo -e "set ssl ocsp-response <<\n$(base64 ocsp.der)\n" | \
+	socat /tmp/sock1 -
+
 It is important to understand that when multiple haproxy processes are started
 on the same sockets, any process may pick up the request and will output its
 own stats.
diff --git a/include/proto/applet.h b/include/proto/applet.h
index cdd4d90f0..7cc2c0ad1 100644
--- a/include/proto/applet.h
+++ b/include/proto/applet.h
@@ -43,11 +43,13 @@ static int inline appctx_res_wakeup(struct appctx *appctx);
 
 /* Initializes all required fields for a new appctx. Note that it does the
  * minimum acceptable initialization for an appctx. This means only the
- * 3 integer states st0, st1, st2 are zeroed.
+ * 3 integer states st0, st1, st2 and the chunk used to gather unfinished
+ * commands are zeroed
  */
 static inline void appctx_init(struct appctx *appctx, unsigned long thread_mask)
 {
 	appctx->st0 = appctx->st1 = appctx->st2 = 0;
+	appctx->chunk = NULL;
 	appctx->io_release = NULL;
 	appctx->thread_mask = thread_mask;
 	appctx->state = APPLET_SLEEPING;
diff --git a/include/proto/cli.h b/include/proto/cli.h
index d5feb862a..da80af7d3 100644
--- a/include/proto/cli.h
+++ b/include/proto/cli.h
@@ -24,7 +24,6 @@
 #define _PROTO_CLI_H
 
 
-struct cli_kw* cli_find_kw(char **args);
 void cli_register_kw(struct cli_kw_list *kw_list);
 
 int cli_has_level(struct appctx *appctx, int level);
diff --git a/include/types/applet.h b/include/types/applet.h
index 89c318c1d..b0715866e 100644
--- a/include/types/applet.h
+++ b/include/types/applet.h
@@ -50,6 +50,9 @@ struct applet {
 #define APPLET_WOKEN_UP 0x02  /* applet was running and requested to wo

[PATCH] BUG/MINOR: pattern: Add a missing HA_SPIN_INIT()

[Aurélien Nephtali]

Hello,

Here is a small patch to add a missing spinlock init.

Thanks.

-- 
Aurélien Nephtali


0001-BUG-MINOR-pattern-Add-a-missing-HA_SPIN_INIT-in-pat_.patch
Description: Binary data

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 <frontend[:luid]> 
- del ssl cert <frontend[:luid]> 

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?= <aurelien.nepht...@corp.ovh.com>
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 <aurelien.nepht...@corp.ovh.com>
---
 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);

Re: [PATCH] MEDIUM: cli: Add multi-line mode support

[Aurélien Nephtali]

Willy,

On Wed, Apr 18, 2018 at 08:36:05AM +0200, Willy Tarreau wrote:
> > The problem is: the main parser (cli_parse_request()) does not have any
> > idea of what is an argument and how many there should be so it can't
> > tell if the command syntax is valid or not before passing it to the
> > commands parser.
> 
> OK, I thought it was tokenized in the cli parser.

The tokenization is done in cli_split_args() which is called by parsers
WHEN they think they have all they need (after they called
cli_resync_args()).

> 
> > What I did is allow the user to use << where he wants
> > and then the commands parsers will do their jobs and if they need more
> > data they will look for them in what is called 'payload'.
> 
> OK
> 
> > I guess your
> > point was to use << as a strict delimiter of arguments and payload.
> 
> Not necessarily. By the way, shell supports < My worries come from your example above which splits the "set server"
> command and some of you comments which seem to imply that the arguments
> are looked up in the payload part.

Well, they are looked up in the payload part but only if the command
parser is missing its mandadory ones.
Let's use the "set server" keywords as an example. This command does not
usually takes a payload but it's what I wanted to add: "multi-lines for
everyone". But maybe the real point should be to add "payload for some" ?
Here would be valid ways of using "set server":

# set server <<
bk/fe state ready
# set server bk/fe state ready
# set server bk/fe <<
state
ready
# set server <<
bk/fe
state
ready

The parser will first call cli_resync_args().
What that will do is consume the payload in order to reach
its number of mandatory arguments. Internally the list of arguments will
become: 'set' 'server 'bk/fe' 'state' 'ready' and the payload will be
set to NULL. Then the parser will request tokenization (through
cli_split_args()) and do its job.

There is no "real" split between arguments and payload. It will be
command parsers that will decide what is the final payload according to
what they need.
It could feel convoluted but that's the way I found to:
- add multi-lines support to *everything* without knowing wether a command
  supports multi-lines or how many arguments it needs (optional or
  mandatory)
- being able to use << where the user wants

> > > Then we can indeed improve ocsp-response, map/acl to support taking 
> > > entries
> > > either as arguments or as payload or both. In fact I think that there is 
> > > no
> > > need for doing something very different from what we see in shell already 
> > > :
> > > the command's args are argv[0..N] and the payload is in stdin. Then many
> > > commands decide where to pick their data from
> > 
> > Yes, that is what I did but I also allowed the << pattern to be loosely
> > placed at the user conveniance.
> 
> OK then that's fine. It's just not the first impression I got when reading
> it, and the commands you changed seemed to continue to use args even for
> the payload part. Please keep in mind the example I proposed regarding
> "add map" with HTTP status codes and reasons, to be sure that once your
> change is applied, it finally becomes possible to add "404 Not Found" as
> a payload where only the "add map" command decides how to parse that line
> (ie: decide that 404 is the key and the rest is the value, which implies that
> it's *not* tokenized). If this works easily, I'm quite confident we'll be on
> a good track.
> 

Well, it already does that :) :

$ cat tst3 | socat /tmp/sock1 -

$ socat /tmp/sock1 -
show map #-1
0x7e6ad0 test.com bk
0x7f5260 200 OK
0x7f52a0 203 Non-Authoritative Information
0x82e3b0 204 No Content
0x82e4b0 301 Moved Permanently
0x82e5b0 302 Found
0x82e6b0 303 See Other
0x82e7b0 400 Bad Request

$ cat tst3
add map <<
#-1
100 Continue
200 OK
203 Non-Authoritative Information
204 No Content
301 Moved Permanently
302 Found
303 See Other
400 Bad Request

$

As you can see, I purposedly set a mandatory argument after the << to be
sure we're on the same page regarding what I did with the command parser
looking into the "payload" (I call payload everything that is after the
<<) and to wether or not this is the way to go :).

-- 
Aurélien.

Re: [PATCH] MEDIUM: cli: Add multi-line mode support

[Aurélien Nephtali]

Hello Willy,

On Wed, Apr 18, 2018 at 07:38:24AM +0200, Willy Tarreau wrote:
> Hi Aurélien,
> 
> On Tue, Apr 17, 2018 at 09:06:35PM +0200, Aurélien Nephtali wrote:
> > Example:
> > If this command is entered:
> > set server bk/fe <<
> > state ready
> > 
> > Three arguments and a payload will be tagged. This command requires (in
> > some cases) seven arguments (including the keywords) so a call to
> > cli_resync_args(ci, 7) will extract what can be found from the payload.
> > Once a parser has done everything it can to ensure it has access to its
> > arguments, it can call cli_split_args() to do the proper tokenization
> > and modify the input string to insert null bytes.
> 
> I'm confused, I thought we agreed *not* to mix arguments and payload, like
> in the map example I provided, but now you seem to imply that extra args
> are looked up in the payload part, while here you seem to have stuck to
> this design, and your "add map" example seems to do exactly the case which
> makes it useless (ie no way to enter spaces in the value because of the
> undesired tokenizing).
> 

The problem is: the main parser (cli_parse_request()) does not have any
idea of what is an argument and how many there should be so it can't
tell if the command syntax is valid or not before passing it to the
commands parser. What I did is allow the user to use << where he wants
and then the commands parsers will do their jobs and if they need more
data they will look for them in what is called 'payload'. I guess your
point was to use << as a strict delimiter of arguments and payload.

> > For now three commands was modified to take advantage of the multi-line
> > mode: "set ssl ocsp-response" and "add map"/"add acl".
> > 
> > The number of arguments has been increased to 8192 (from 64) and the
> > array to hold the pointers gets its memory from a pool. I chose 8192
> > since it's the maximum number of arguments possible with an input buffer
> > of 16384 (default tune.bufsize) (16384 / 2 if all arguments are one-char
> > words). Would computing the max value at runtime be better ?
> 
> That's exactly what scares me in fact. For me there's no reason to increase
> the argument count, the parser should parse a command's argument and decide
> that what follows '<<' is a raw payload (we can debate whether it starts on
> the next line or immediately after '<<' or if it's only '<<\n' which starts
> a payload but for not it's a detail). The command should then get the pointer
> to that payload as a payload pointer and take care of its payload itself.

Ok, I can remove that part.

> 
> Then we can indeed improve ocsp-response, map/acl to support taking entries
> either as arguments or as payload or both. In fact I think that there is no
> need for doing something very different from what we see in shell already :
> the command's args are argv[0..N] and the payload is in stdin. Then many
> commands decide where to pick their data from

Yes, that is what I did but I also allowed the << pattern to be loosely
placed at the user conveniance.

> By the way, please better split the patch in several parts :
>   - one for the infrastructure changes (CLI parser in general and its
> repercussions all over the code)
>   - one per modified subsystem to benefit from it (eg: adding payload
> support to ocsp should be one patch, adding payload support to map/acl
> should be another one)

Sure.

-- 
Aurélien.

Re: [PATCH] MEDIUM: cli: Add multi-line mode support

[Aurélien Nephtali]

Hello,

Here is another attempt to add multi-lines support to the CLI.

To enter a multi-lines command, a special pattern (<<) has to be used at
the end of the first line. Once in this mode, everything else is
gathered as it and the command is considered terminated on the reception
of an empty line. The command is then parsed and passed to a parser if a
valid keyword is found. This mode is not persistent across commands.

Technical part:
---

Parsing consists of "tagging" the position of each word BEFORE the
multi-lines pattern or until the end of the buffer if there is only one
line. This way, the input buffer is untouched and it is the parser
responsabilty to decide if it needs tokenization of its arguments. To do
this, there are two new functions: cli_resync_args() and
cli_split_args().

The main difficulty is that the parser does not know how many arguments
takes a command and if it accepts a payload (what is found after the
multi-lines pattern), that is why it only tags the input. If a parser
needs N arguments (mandatory or optional) it can call
cli_resync_args(ci, N) to try to get them from the payload. If there is
no payload (or not enough data to reach N args) the missing slots are
filled with the empty string). The list of arguments always ends with a
pointer on an empty string.

Example:
If this command is entered:
set server bk/fe <<
state ready

Three arguments and a payload will be tagged. This command requires (in
some cases) seven arguments (including the keywords) so a call to
cli_resync_args(ci, 7) will extract what can be found from the payload.
Once a parser has done everything it can to ensure it has access to its
arguments, it can call cli_split_args() to do the proper tokenization
and modify the input string to insert null bytes.

For now three commands was modified to take advantage of the multi-line
mode: "set ssl ocsp-response" and "add map"/"add acl".

The number of arguments has been increased to 8192 (from 64) and the
array to hold the pointers gets its memory from a pool. I chose 8192
since it's the maximum number of arguments possible with an input buffer
of 16384 (default tune.bufsize) (16384 / 2 if all arguments are one-char
words). Would computing the max value at runtime be better ?

A field, 'arg_count', is left unused by the parsers (but filled) in
'struct cli_input'. As a next step, instead of checking if the n-th
argument is not an empty string, it could be a good idea to convert all
parsers to check this field so they can know how many arguments they can
access to.

--
Aurélien.
>From 37b3c2f86bc77f6fd9a9eb416b43c1183cd81351 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Sun, 15 Apr 2018 18:38:51 +0200
Subject: [PATCH] MEDIUM: cli: Add multi-line mode support
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It is now possible to write commands spanning on multiple lines. To do
so, a special pattern (<<) must be entered at the end of the first line.

Example:

# echo -e "set ssl ocsp-response <<\n$(base64 ocsp.der)\n" | \
   socat /tmp/sock1 -

All commands support this mode and some of them ("set ssl ocsp-response"
and "add map"/"add acl") have seen their syntax enhanced to take advantage
of it (i.e.: accept base64 on multiple lines or accept multiple key/value
pairs).

Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
 doc/management.txt|  24 
 include/common/defaults.h |   4 +-
 include/proto/applet.h|   4 +-
 include/proto/cli.h   |   3 +-
 include/types/applet.h|   6 +-
 include/types/cli.h   |  22 ++-
 src/cache.c   |   2 +-
 src/cli.c | 344 +++---
 src/dns.c |   9 +-
 src/hlua.c|   8 +-
 src/map.c | 188 +
 src/proto_http.c  |  15 +-
 src/proxy.c   |  66 ++---
 src/server.c  | 131 +++---
 src/ssl_sock.c|  42 --
 src/stats.c   |  39 --
 src/stick_table.c |  19 +--
 src/stream.c  |  27 ++--
 18 files changed, 647 insertions(+), 306 deletions(-)

diff --git a/doc/management.txt b/doc/management.txt
index 4b6901851..f01c87e95 100644
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -1298,6 +1298,30 @@ delimiter to mark an end of output for each command, and takes care of ensuring
 that no command can emit an empty line on output. A script can thus easily
 parse the output even when multiple commands were pipelined on a single line.
 
+It is also possible to send commands spanning on multiple lines. This is done by
+ending the first line with the "<<" pattern. The next lines will be treated as
+being part of the comman

Re: [PATCH] BUG/MINOR: cli: Ensure appctx->ctx.cli.err is always set when using CLI_ST_PRINT_FREE

[Aurélien Nephtali]

Hello Willy (not being rude this time :p),

On Mon, Apr 16, 2018 at 05:01:18PM +0200, Willy Tarreau wrote:
> I agree on the principle, but memprintf(, "foo") will set err to NULL
> if there's no more memory. And I personally care a lot about staying rock
> solid even under harsh memory conditions, because it's always when you have
> the most visitors on your site that you have the least memory left and you
> don't want so many witnesses of your lack of RAM. That's why I'm thinking
> that the "out of memory" error message could more or less serve as a real
> indicator of what happened and as a motivation for developers never to use
> it by default (while "internal error" could be tempting on a lazy day).
> 

Here are the two patches with the changes you proposed.

Thanks !

-- 
Aurélien.
>From 29719bded4ff2b96cb4ac258373c01f0be18428b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Mon, 16 Apr 2018 18:50:19 +0200
Subject: [PATCH 1/2] BUG/MINOR: cli: Guard against NULL messages when using
 CLI_ST_PRINT_FREE
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Some error paths (especially those followed when running out of memory)
can set the error message to NULL. In order to avoid a crash, use a
generic message ("Out of memory") when this case arises.

It should be backported to 1.8.

Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
 src/cli.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/cli.c b/src/cli.c
index 018d508d3..965709ec8 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -625,14 +625,20 @@ static void cli_io_handler(struct appctx *appctx)
 else
 	si_applet_cant_put(si);
 break;
-			case CLI_ST_PRINT_FREE:
-if (cli_output_msg(res, appctx->ctx.cli.err, LOG_ERR, cli_get_severity_output(appctx)) != -1) {
+			case CLI_ST_PRINT_FREE: {
+const char *msg = appctx->ctx.cli.err;
+
+if (!msg)
+	msg = "Out of memory.\n";
+
+if (cli_output_msg(res, msg, LOG_ERR, cli_get_severity_output(appctx)) != -1) {
 	free(appctx->ctx.cli.err);
 	appctx->st0 = CLI_ST_PROMPT;
 }
 else
 	si_applet_cant_put(si);
 break;
+			}
 			case CLI_ST_CALLBACK: /* use custom pointer */
 if (appctx->io_handler)
 	if (appctx->io_handler(appctx)) {
-- 
2.11.0

>From a9b9825d3b6257ccd42d2b56827e23fa91d4768c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Mon, 16 Apr 2018 19:02:42 +0200
Subject: [PATCH 2/2] MINOR: cli: Ensure the CLI always outputs an error when
 it should
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When using the CLI_ST_PRINT_FREE state, always output something back
if the faulty function did not fill the 'err' variable.
The map/acl code could lead to a crash whereas the SSL code was silently
failing.

Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
 src/map.c  | 38 --
 src/ssl_sock.c |  5 +
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/src/map.c b/src/map.c
index 9313dc87e..7953c2a0b 100644
--- a/src/map.c
+++ b/src/map.c
@@ -723,15 +723,21 @@ static int cli_parse_set_map(char **args, struct appctx *appctx, void *private)
 return 1;
 			}
 
-			/* Try to delete the entry. */
+			/* Try to modify the entry. */
 			err = NULL;
 			HA_SPIN_LOCK(PATREF_LOCK, >ctx.map.ref->lock);
 			if (!pat_ref_set_by_id(appctx->ctx.map.ref, ref, args[4], )) {
 HA_SPIN_UNLOCK(PATREF_LOCK, >ctx.map.ref->lock);
-if (err)
+if (err) {
 	memprintf(, "%s.\n", err);
-appctx->ctx.cli.err = err;
-appctx->st0 = CLI_ST_PRINT_FREE;
+	appctx->ctx.cli.err = err;
+	appctx->st0 = CLI_ST_PRINT_FREE;
+}
+else {
+	appctx->ctx.cli.severity = LOG_ERR;
+	appctx->ctx.cli.msg = "Failed to update an entry.\n";
+	appctx->st0 = CLI_ST_PRINT;
+}
 return 1;
 			}
 			HA_SPIN_UNLOCK(PATREF_LOCK, >ctx.map.ref->lock);
@@ -744,10 +750,16 @@ static int cli_parse_set_map(char **args, struct appctx *appctx, void *private)
 			HA_SPIN_LOCK(PATREF_LOCK, >ctx.map.ref->lock);
 			if (!pat_ref_set(appctx->ctx.map.ref, args[3], args[4], )) {
 HA_SPIN_UNLOCK(PATREF_LOCK, >ctx.map.ref->lock);
-if (err)
+if (err) {
 	memprintf(, "%s.\n", err);
-appctx->ctx.cli.err = err;
-appctx->st0 = CLI_ST_PRINT_FREE;
+	appctx->ctx.cli.err = err;
+	appctx->st0 = CLI_ST_PRINT_FREE;
+}
+else {
+	appctx->ctx.cli.severity = LOG_ERR;
+	appctx->ctx.cli.msg = "Failed to update an entry.\n";
+	appctx->st0 = CLI_ST_PRINT;
+}
 return 1;
 			}
 	

Re: [PATCH] BUG/MINOR: cli: Ensure appctx->ctx.cli.err is always set when using CLI_ST_PRINT_FREE

[Aurélien Nephtali]

On Mon, Apr 16, 2018 at 4:19 PM, Willy Tarreau <w...@1wt.eu> wrote:
> Hi Aurélien,
>
> On Sun, Apr 15, 2018 at 09:58:49AM +0200, Aurélien Nephtali wrote:
>> Hello,
>>
>> Here is a small patch to fix a potential crash when using
>> CLI_ST_PRINT_FREE in an error path in the 'map' code.
>> The problematic part is in the 'add' feature but all other usages have
>> ben modified the same way to be consistent.
>
> Interesting one. In fact, while it does provide a friendlier error message
> to the user, the real issue in my opinion is in the cli_io_handler() where
> it handles CLI_ST_PRINT_FREE, where it's not defensive enough against a
> NULL in appctx->ctx.cli.err. And even with your patch this situation can
> arise if an out of memory condition happens in the final memprintf() of
> the map code.
>
> Thus what I'd suggest would be instead to check for NULL there and to fall
> back to a generic "out of memory" error message (if that makes sense, maybe
> other situations may lead to this, I don't know) as a first patch,

This was my first idea, using "Internal error" or something like that
but I had the feeling it was covering some cases that should be
properly handled.
As it's "internal code" I bet on the fact that it should not happen.

>From what I saw briefly all errors paths fill 'err' but I may have
overlooked some cases.

> then another one which is just a small improvement to make error messages more
> relevant for map and ocsp (which is exactly what your patch does).
>
> I'm just having a small comment below :
>
>> - if (err)
>> + if (err) {
>>   memprintf(, "%s.\n", err);
>> - appctx->ctx.cli.err = err;
>> - appctx->st0 = CLI_ST_PRINT_FREE;
>> +appctx->ctx.cli.err = err;
>> +appctx->st0 = CLI_ST_PRINT_FREE;
>> +}
>> +else {
>> + appctx->ctx.cli.severity = LOG_ERR;
>> + appctx->ctx.cli.msg = "Failed to 
>> update an entry.\n";
>> + appctx->st0 = CLI_ST_PRINT;
>> +}
>>   return 1;
>
> Please be careful above, as you can see, the lines are filled with spaces,
> maybe the code was copy-pasted there (it's the same at other locations).
>

Arg!#@ sorry, I thought I got them all.. My indent rules were reset at
some point and these lines slipped through.
I usually have a rule to show extra tabs when I should use spaces but
not the inverse :).

-- 
Aurélien Nephtali

[PATCH] BUG/MINOR: cli: Ensure appctx->ctx.cli.err is always set when using CLI_ST_PRINT_FREE

[Aurélien Nephtali]

Hello,

Here is a small patch to fix a potential crash when using
CLI_ST_PRINT_FREE in an error path in the 'map' code.
The problematic part is in the 'add' feature but all other usages have
ben modified the same way to be consistent.

It also adds a missing error message in the SSL code when the loading of
an OCSP response failed. The error path always sets the 'err' variable
but it's a good idea to handle the case where it does not in the event
that future code forgot to fill it.

-- 
Aurélien.
>From e91962c02cdc0ae48ff59066cdc0f22e1063b496 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Sun, 15 Apr 2018 07:39:54 +0200
Subject: [PATCH] BUG/MINOR: cli: Ensure appctx->ctx.cli.err is always set when
 using CLI_ST_PRINT_FREE
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In the CLI, appctx->ctx.cli.err must point to a valid string when using
the state CLI_ST_PRINT_FREE otherwise a crash will happen.

In the map code, one error path does not fill the 'err' variable
(pat_ref_add() -> pat_ref_push()) and in the SSL code it is correct but
it lacks a default error message if the function called does not fill
'err'.

In both cases only use CLI_ST_PRINT_FREE if 'err' is not NULL otherwise
use a static default message.

It should be backported to 1.8.

Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
 src/map.c  | 38 --
 src/ssl_sock.c |  5 +
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/src/map.c b/src/map.c
index 9313dc87e..6cf6e4a87 100644
--- a/src/map.c
+++ b/src/map.c
@@ -723,15 +723,21 @@ static int cli_parse_set_map(char **args, struct appctx *appctx, void *private)
 return 1;
 			}
 
-			/* Try to delete the entry. */
+			/* Try to modify the entry. */
 			err = NULL;
 			HA_SPIN_LOCK(PATREF_LOCK, >ctx.map.ref->lock);
 			if (!pat_ref_set_by_id(appctx->ctx.map.ref, ref, args[4], )) {
 HA_SPIN_UNLOCK(PATREF_LOCK, >ctx.map.ref->lock);
-if (err)
+if (err) {
 	memprintf(, "%s.\n", err);
-appctx->ctx.cli.err = err;
-appctx->st0 = CLI_ST_PRINT_FREE;
+appctx->ctx.cli.err = err;
+appctx->st0 = CLI_ST_PRINT_FREE;
+}
+else {
+	appctx->ctx.cli.severity = LOG_ERR;
+	appctx->ctx.cli.msg = "Failed to update an entry.\n";
+	appctx->st0 = CLI_ST_PRINT;
+}
 return 1;
 			}
 			HA_SPIN_UNLOCK(PATREF_LOCK, >ctx.map.ref->lock);
@@ -744,10 +750,16 @@ static int cli_parse_set_map(char **args, struct appctx *appctx, void *private)
 			HA_SPIN_LOCK(PATREF_LOCK, >ctx.map.ref->lock);
 			if (!pat_ref_set(appctx->ctx.map.ref, args[3], args[4], )) {
 HA_SPIN_UNLOCK(PATREF_LOCK, >ctx.map.ref->lock);
-if (err)
+if (err) {
 	memprintf(, "%s.\n", err);
-appctx->ctx.cli.err = err;
-appctx->st0 = CLI_ST_PRINT_FREE;
+appctx->ctx.cli.err = err;
+appctx->st0 = CLI_ST_PRINT_FREE;
+}
+else {
+	appctx->ctx.cli.severity = LOG_ERR;
+	appctx->ctx.cli.msg = "Failed to update an entry.\n";
+	appctx->st0 = CLI_ST_PRINT;
+}
 return 1;
 			}
 			HA_SPIN_UNLOCK(PATREF_LOCK, >ctx.map.ref->lock);
@@ -829,10 +841,16 @@ static int cli_parse_add_map(char **args, struct appctx *appctx, void *private)
 			ret = pat_ref_add(appctx->ctx.map.ref, args[3], NULL, );
 		HA_SPIN_UNLOCK(PATREF_LOCK, >ctx.map.ref->lock);
 		if (!ret) {
-			if (err)
+			if (err) {
 memprintf(, "%s.\n", err);
-			appctx->ctx.cli.err = err;
-			appctx->st0 = CLI_ST_PRINT_FREE;
+appctx->ctx.cli.err = err;
+appctx->st0 = CLI_ST_PRINT_FREE;
+			}
+			else {
+appctx->ctx.cli.severity = LOG_ERR;
+appctx->ctx.cli.msg = "Failed to add an entry.\n";
+appctx->st0 = CLI_ST_PRINT;
+			}
 			return 1;
 		}
 
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 8151cb381..23ad35b18 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -8588,6 +8588,11 @@ static int cli_parse_set_ocspresponse(char **args, struct appctx *appctx, void *
 			appctx->ctx.cli.err = err;
 			appctx->st0 = CLI_ST_PRINT_FREE;
 		}
+		else {
+			appctx->ctx.cli.severity = LOG_ERR;
+			appctx->ctx.cli.msg = "Failed to update OCSP response.\n";
+			appctx->st0 = CLI_ST_PRINT;
+		}
 		return 1;
 	}
 	appctx->ctx.cli.severity = LOG_INFO;
-- 
2.11.0

Re: [PATCH] MEDIUM: cli: Add multi-line mode support

[Aurélien Nephtali]

y to
> the next command so that there is no need to do this :
>
>   > multiline;add map http_status
>   + 200 OK
>   + 206 Partial Content
>   + 302 Found
>   + 303 See Other
>   + 304 Not Modified
>   +
>   > multiline
>
> It could thus be shortened (eg: "ml") or even be a different character,
> though I don't really see which one.
>
> Or probably much better, we could end a command with a character indicating
> that a payload follows, a bit a-la "<< EOF" we're used to see in shell.
> Maybe just using "<<" at the end of a line could be sufficient to indicate
> that a payload follows and that it must be ended by an empty line. Eg:
>
>   > add map http_status <<
>   + 200 OK
>   + 206 Partial Content
>   + 302 Found
>   + 303 See Other
>   + 304 Not Modified
>   +
>   >

Using a special pattern is a good idea. Let's start with "<<" and
tweak it later if it collides with what can be found in a payload.

>
> The benefit here is that it's still the parser which knows whether or
> not a payload follows, regardless of the command, and can simply pass
> a flag to the command saying "you have a payload to read".

Yes, in fact that's exactly what I did at first but I really thought
it was too ambitious and subject to compatibility-breaking (again due
to my lack of background on the CLI/HAProxy usage).

I realize I wanted to be too generic to be sure I would not break
anything but I ended doing something too light.

>
> While typing this I was also thinking that we could also put this as a
> flag in the CLI keywords registration (ie takes a payload or not) but
> I'm not convinced this would be a good idea because it would mean that
> the user (or script) would have to know whether or not the command
> expects a payload, and could quickly get out of sync on the first mistake.
>
> I think that it is reasonable for a first implementation to have some
> technical limitations, such as supporting only a single-buffer payload
> (ie not implement the streaming mode first). It can be documented that
> no more than "tune.bufsize" bytes may be sent at once in a single
> command, and scripts feeding large data chunks will simply have to cut
> them into pieces (eg: ACLs and maps). This will allow you to make the
> parser wait for the empty line when seeing that a multi-line block is
> being sent, and simply pass the pointer to this block to the command,
> doing everything at once (which is less complex). And this will not
> prevent us from removing this limitation later if/once it really
> becomes annoying.
>

Ok, I'll do that in a way it will be easy to enhance.

> Just a very minor cosmetic comments below :
>
> > diff --git a/include/types/applet.h b/include/types/applet.h
> > index 89c318c1d..9789e202a 100644
> > --- a/include/types/applet.h
> > +++ b/include/types/applet.h
> > @@ -57,8 +57,11 @@ struct appctx {
> >   /* 3 unused bytes here */
> >   unsigned short state;  /* Internal appctx state */
> >   unsigned int st0;  /* CLI state for stats, session state for 
> > peers */
> > - unsigned int st1;  /* prompt for stats, session error for 
> > peers */
> > + unsigned int st1;  /* prompt/multiline for stats, session 
> > error for peers */
> > +#define APPCTX_CLI_ST1_PROMPT(1 << 0)
> > +#define APPCTX_CLI_ST1_MULTILINE (1 << 1)
>
> Please don't put #define in the middle of the structs, they're harder to
> find later when reading the code, and it's harder also to make other
> defines depend on them. Better place them at the top of the file with the
> other ones. Simply mention in st1 that it makes use of APPCTX_CLI_ST1_*
> and that's fine.

Sure.

-- 
Aurélien Nephtali

Re: [PATCH] MEDIUM: cli: Add multi-line mode support

[Aurélien Nephtali]

Hello,

On Fri, Mar 23, 2018 at 09:43:50PM +0100, Aurélien Nephtali wrote:
> Hello,
> 
> This patch adds multi-line mode support to the CLI.

[...]

Please consider reviewing the attached patch instead as it fixes a
compiler warning.

-- 
Aurélien.
>From 53356f83dab6483512d2db1fae87abd24beca4c0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Thu, 15 Mar 2018 15:44:19 +0100
Subject: [PATCH] MEDIUM: cli: Add multi-line mode support
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Add a new command to toggle multi-line mode: multiline.
Once activated, all commands must end with an empty line.

The multi-line mode enables line feeds to be used anywhere in the
request as long it stays syntactically correct once reassembled.
This mode should be particularly useful for scripts or when inputing
data that can contain line feeds.

Examples:
$ echo -e "multiline;set ssl ocsp-response $(base64 ocsp.der)\n" | socat
/tmp/sock1 -
$ echo -e "multiline ; show\nstat\n" | socat /tmp/sock1 -

Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
 doc/management.txt |  23 +++
 include/proto/applet.h |   4 +-
 include/types/applet.h |   5 +-
 src/cli.c  | 165 +++--
 src/ssl_sock.c |  20 +-
 5 files changed, 168 insertions(+), 49 deletions(-)

diff --git a/doc/management.txt b/doc/management.txt
index 1488862e7..dd52cf82a 100644
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -1563,6 +1563,29 @@ help
   Print the list of known keywords and their basic usage. The same help screen
   is also displayed for unknown commands.
 
+multiline
+  Toggle the multi-line mode. When enabled, this mode allows commands to be split
+  across multiple lines. To signal the end of a multi-line request, an empty
+  line must be sent. The multi-line mode can also be used with the interactive
+  mode: if 'prompt' was enabled beforehand, the CLI becomes interactive with
+  multi-line support. The prompt will also change from '> ' to '+ ' to emphasize
+  that an empty line is needed to finish and parse the command.
+
+  Example:
+$ echo -e "multiline;set server\nTEST/A\nstate ready\n" | socat /tmp/sock1 -
+$ socat /tmp/sock1 -
+prompt
+
+> multiline
+
+> set
++ server TEST/A
++ state
++ ready
++
+
+>
+
 prompt
   Toggle the prompt at the beginning of the line and enter or leave interactive
   mode. In interactive mode, the connection is not closed after a command
diff --git a/include/proto/applet.h b/include/proto/applet.h
index cdd4d90f0..7cc2c0ad1 100644
--- a/include/proto/applet.h
+++ b/include/proto/applet.h
@@ -43,11 +43,13 @@ static int inline appctx_res_wakeup(struct appctx *appctx);
 
 /* Initializes all required fields for a new appctx. Note that it does the
  * minimum acceptable initialization for an appctx. This means only the
- * 3 integer states st0, st1, st2 are zeroed.
+ * 3 integer states st0, st1, st2 and the chunk used to gather unfinished
+ * commands are zeroed
  */
 static inline void appctx_init(struct appctx *appctx, unsigned long thread_mask)
 {
 	appctx->st0 = appctx->st1 = appctx->st2 = 0;
+	appctx->chunk = NULL;
 	appctx->io_release = NULL;
 	appctx->thread_mask = thread_mask;
 	appctx->state = APPLET_SLEEPING;
diff --git a/include/types/applet.h b/include/types/applet.h
index 89c318c1d..9789e202a 100644
--- a/include/types/applet.h
+++ b/include/types/applet.h
@@ -57,8 +57,11 @@ struct appctx {
 	/* 3 unused bytes here */
 	unsigned short state;  /* Internal appctx state */
 	unsigned int st0;  /* CLI state for stats, session state for peers */
-	unsigned int st1;  /* prompt for stats, session error for peers */
+	unsigned int st1;  /* prompt/multiline for stats, session error for peers */
+#define APPCTX_CLI_ST1_PROMPT(1 << 0)
+#define APPCTX_CLI_ST1_MULTILINE (1 << 1)
 	unsigned int st2;  /* output state for stats, unused by peers  */
+	struct chunk *chunk;   /* used to store unfinished commands */
 	struct applet *applet; /* applet this context refers to */
 	void *owner;   /* pointer to upper layer's entity (eg: stream interface) */
 	struct act_rule *rule; /* rule associated with the applet. */
diff --git a/src/cli.c b/src/cli.c
index ada45a865..cc76a3127 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -73,6 +73,7 @@ static const char stats_sock_usage_msg[] =
 	"Unknown command. Please enter one of the following commands only :\n"
 	"  help   : this message\n"
 	"  prompt : toggle interactive mode with prompt\n"
+	"  multiline  : toggle multi-line mode\n"
 	"  quit   : disconnect\n"
 	"";
 
@@ -90,7 +91,7 @@ static struct cli_kw_list cli_keyw

[PATCH] MEDIUM: cli: Add multi-line mode support

[Aurélien Nephtali]

Hello,

This patch adds multi-line mode support to the CLI.
To keep a backward compatible behaviour, this mode is only available
when enabled  with the 'multiline' command.
Once enabled, _ALL_ commands (except 'quit') must end with an empty line (LF).
This mode can also be used with pipelined commands or in interactive
mode if the 'prompt' command was issued beforehand.

When interactive mode and multi-line mode are both enabled, the prompt
will change from '\n> ' to '+ ' to have a visual feedback that the
current request is not complete.

A second patch is attached: it adds a Perl script (and some other
files used for the tests) to test the CLI with different use cases.
I don't know if it can be merged as it but it can still be used to
validate that nothing was broken while adding the multi-line mode.
Simply launch: perl tests/cli.pl to run the tests.

Examples:

===
$ echo -e "multiline;set ssl ocsp-response $(base64 ocsp.der)\n" |
socat /tmp/sock1 -

OCSP Response updated!

$
===
$ echo -e "multiline;set\nserver\nbk/srv\nstate\nready\n" | socat /tmp/sock1 -


$
===

--
Aurélien Nephtali


0001-MEDIUM-cli-Add-multi-line-mode-support.patch
Description: Binary data


0001-TESTS-Add-a-script-to-test-the-CLI.patch
Description: Binary data

[PATCH] BUG/MINOR: cli: Fix a crash when sending a command with too many arguments

[Aurélien Nephtali]

Hello,

This patch fixes a crash when the CLI is fed with too many arguments:

$ seq -s ' ' 0 64 | socat /tmp/sock1 -

-- 
Aurélien Nephtali
From 09033c7d2cf1119ef3f6590fcf0c662bfaebf612 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Fri, 16 Mar 2018 10:11:06 +0100
Subject: [PATCH] BUG/MINOR: cli: Fix a crash when sending a command with too
 many arguments
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This bug was introduced in 48bcfdab2 ("MEDIUM: dumpstat: make the CLI
parser understand the backslash as an escape char").

This should be backported to 1.8.

Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
 src/cli.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/cli.c b/src/cli.c
index 84b6229b7..96ad9a445 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -416,7 +416,7 @@ static int cli_parse_request(struct appctx *appctx, char *line)
 
 	/* unescape '\' */
 	arg = 0;
-	while (*args[arg] != '\0') {
+	while (arg <= MAX_STATS_ARGS && *args[arg] != '\0') {
 		j = 0;
 		for (i=0; args[arg][i] != '\0'; i++) {
 			if (args[arg][i] == '\\') {
-- 
2.11.0

[PATCH] BUG/MINOR: cli: Ensure all command outputs end with a LF

[Aurélien Nephtali]

Hello,

This patch adds some missing LF to the outputs of some commands.
It may break some scripts that rely on these broken outputs but it also
breaks the parsing of pipelined commands.

-- 
Aurélien.
>From 058559ce78d05c87e7c19fd7866a52ccce608a39 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Thu, 15 Mar 2018 21:48:50 +0100
Subject: [PATCH] BUG/MINOR: cli: Ensure all command outputs end with a LF
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Since 200b0fac ("MEDIUM: Add support for updating TLS ticket keys via
socket"), 4147b2ef ("MEDIUM: ssl: basic OCSP stapling support."),
4df59e9 ("MINOR: cli: add socket commands and config to prepend
informational messages with severity") and 654694e1 ("MEDIUM: stats/cli:
add support for "set table key" to enter values"), commands
'set ssl tls-key', 'set ssl ocsp-response', 'set severity-output' and
'set table' do not always send an extra LF at the end of their outputs.

This is required as mentioned in doc/management.txt:

"Since multiple commands may be issued at once, haproxy uses the empty
line as a delimiter to mark an end of output for each command"

Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
 src/cli.c | 2 +-
 src/ssl_sock.c| 4 ++--
 src/stick_table.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/cli.c b/src/cli.c
index 84b6229b7..7bffbdd56 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -1145,7 +1145,7 @@ static int cli_parse_set_severity_output(char **args, struct appctx *appctx, voi
 		return 0;
 
 	appctx->ctx.cli.severity = LOG_ERR;
-	appctx->ctx.cli.msg = "one of 'none', 'number', 'string' is a required argument";
+	appctx->ctx.cli.msg = "one of 'none', 'number', 'string' is a required argument\n";
 	appctx->st0 = CLI_ST_PRINT;
 	return 1;
 }
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index daf428247..5acf38f60 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -8554,7 +8554,7 @@ static int cli_parse_set_tlskeys(char **args, struct appctx *appctx, void *priva
 	}
 	ssl_sock_update_tlskey_ref(ref, );
 	appctx->ctx.cli.severity = LOG_INFO;
-	appctx->ctx.cli.msg = "TLS ticket key updated!";
+	appctx->ctx.cli.msg = "TLS ticket key updated!\n";
 	appctx->st0 = CLI_ST_PRINT;
 	return 1;
 
@@ -8591,7 +8591,7 @@ static int cli_parse_set_ocspresponse(char **args, struct appctx *appctx, void *
 		return 1;
 	}
 	appctx->ctx.cli.severity = LOG_INFO;
-	appctx->ctx.cli.msg = "OCSP Response updated!";
+	appctx->ctx.cli.msg = "OCSP Response updated!\n";
 	appctx->st0 = CLI_ST_PRINT;
 	return 1;
 #else
diff --git a/src/stick_table.c b/src/stick_table.c
index fe26e3121..73c70d3e2 100644
--- a/src/stick_table.c
+++ b/src/stick_table.c
@@ -3334,7 +3334,7 @@ static int table_prepare_data_request(struct appctx *appctx, char **args)
 {
 	if (appctx->ctx.table.action != STK_CLI_ACT_SHOW && appctx->ctx.table.action != STK_CLI_ACT_CLR) {
 		appctx->ctx.cli.severity = LOG_ERR;
-		appctx->ctx.cli.msg = "content-based lookup is only supported with the \"show\" and \"clear\" actions";
+		appctx->ctx.cli.msg = "content-based lookup is only supported with the \"show\" and \"clear\" actions\n";
 		appctx->st0 = CLI_ST_PRINT;
 		return 1;
 	}
-- 
2.11.0

[PATCH 3/3] CLEANUP: cli: Fix a typo in the 'set rate-limit' usage

[Aurélien Nephtali]

Hello,

The attached patch fixes a typo in the usage message of 'set
rate-limit'.

-- 
Aurélien.
>From bb62cb61291fe07eee3ce2cbc92dfcadc30d7622 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Sun, 11 Mar 2018 16:55:02 +0100
Subject: [PATCH 3/3] CLEANUP: cli: Fix a typo in the 'set rate-limit' usage
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The correct keyword is 'ssl-sessions' (vs. 'ssl-session').
The typo was introduced in 45c742be05 ('REORG: cli: move the "set
rate-limit" functions to their own parser').

Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
 src/cli.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/cli.c b/src/cli.c
index 65914451..3cae0f31 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -1184,7 +1184,7 @@ static int cli_parse_set_ratelimit(char **args, struct appctx *appctx, void *pri
 			"   - 'connections global' to set the per-process maximum connection rate\n"
 			"   - 'sessions global' to set the per-process maximum session rate\n"
 #ifdef USE_OPENSSL
-			"   - 'ssl-session global' to set the per-process maximum SSL session rate\n"
+			"   - 'ssl-sessions global' to set the per-process maximum SSL session rate\n"
 #endif
 			"   - 'http-compression global' to set the per-process maximum compression speed in kB/s\n";
 		appctx->st0 = CLI_ST_PRINT;
-- 
2.11.0

[PATCH 2/3] CLEANUP: cli: Remove a leftover debug message

[Aurélien Nephtali]

Hello,

The attached patch removes a printf() that was probably used for
debugging purposes.

-- 
Aurélien.
>From 220e631e6f209ed51af772b12f7f60a1a1ce5857 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Sat, 10 Mar 2018 20:59:56 +0100
Subject: [PATCH 2/3] CLEANUP: cli: Remove a leftover debug message
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This printf() was added in f886e3478d ("MINOR: cli: Add a command to
send listening sockets.").

Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
 src/cli.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/cli.c b/src/cli.c
index fbd26464..65914451 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -1416,7 +1416,6 @@ static int _getsocks(char **args, struct appctx *appctx, void *private)
 iov.iov_len = curoff;
 if (sendmsg(fd, , 0) != curoff) {
 	ha_warning("Failed to transfer sockets\n");
-	printf("errno %d\n", errno);
 	goto out;
 }
 /* Wait for an ack */
-- 
2.11.0

[PATCH 1/3] CLEANUP: ssl: Remove a duplicated #include

[Aurélien Nephtali]

Hello,

The attached patch removes a duplicated #include in the SSL code.

-- 
Aurélien.
>From 3955f2cd7f820e46710c1e610d4aaa2eace92c88 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Tue, 20 Feb 2018 19:23:07 +0100
Subject: [PATCH 1/3] CLEANUP: ssl: Remove a duplicated #include
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

openssl/x509.h is included twice since commit fc0421fde ("MEDIUM: ssl:
add support for SNI and wildcard certificates").

Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
 src/ssl_sock.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index e3db4e17..e2cde197 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -43,7 +43,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
-- 
2.11.0

[PATCH] MINOR/BUG: cli: Fix a crash when passing a negative or too large value to "show fd"

[Aurélien Nephtali]

Hello,

Here is a small patch to fix a crash that occurs when trying to "show
fd" on a negative (or too large) fd value:

$ echo "show fd -1" | socat /tmp/sock1 stdio
$ echo "show fd " | socat /tmp/sock1 stdio

-- 
Aurélien.
>From 7a1a8fe760a50cf3afdd630090468b4657a6de9d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
Date: Fri, 9 Mar 2018 18:51:16 +0100
Subject: [PATCH] MINOR/BUG: cli: Fix a crash when passing a negative or too
 large value to "show fd"
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This bug is present since 7a4a0ac71d ("MINOR: cli: add a new "show fd"
command").

This should be backported to 1.8.

Signed-off-by: Aurélien Nephtali <aurelien.nepht...@corp.ovh.com>
---
 src/cli.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/cli.c b/src/cli.c
index fbd264640..51efbc445 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -772,7 +772,7 @@ static int cli_io_handler_show_fd(struct appctx *appctx)
/* we have two inner loops here, one for the proxy, the other one for
 * the buffer.
 */
-   while (fd < global.maxsock) {
+   while (fd >= 0 && fd < global.maxsock) {
struct fdtab fdt;
struct listener *li = NULL;
struct server *sv = NULL;
-- 
2.11.0

Re: Dynamically adding/deleting SSL certificates

[Aurélien Nephtali]

Hello Willy,

On Mon, Mar 5, 2018 at 8:37 PM, Willy Tarreau <w...@1wt.eu> 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 <w...@1wt.eu> 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

Dynamically adding/deleting SSL certificates

[Aurélien Nephtali]

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 ?

Thanks !

-- 
Aurélien Nephtali