Re: [PATCH] MEDIUM: cli: Add multi-line mode support
Hi Aurélien, On Wed, Apr 18, 2018 at 09:36:14AM +0200, Aurélien Nephtali wrote: > > 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. I really don't like to mix this. It makes error control very hard, and will make it even harder to later add optional arguments to some commands, because they could optionally be looked up in the payload if the previous command used to support one. If we want to make some commands support getting one of its current arguments as payload (such as ocsp), let's do it explicitly for the command. For example it could be said that "set ssl ocsp-response" supports either one argument on the command line, or a payload. But making commands concatenate arguments and start of payload seems very wrong to me because we're mixing data of different semantics. Look at this totally made up example below. Let's say we implement support for payload to fill in ACLs. The following command is used to enter a list of new keywords that you want to detect in a query string as potentially dangerous actions on a login page : > add acl dangerous_words << + delete + clear + kill + close + remove + Then later someone proposes a patch for the ACLs to perform an atomic load after clearing existing ones, and suggests to append the optional keyword "clear" on the command line arguments. The command becomes : > add acl foo clear << + blah + foo + bar + zoo + As you can see this has no apparent effect on the first command above. But now the script used above to load the dangerous words sends them sorted in alphabetical order for whatever reason (eg: deduplication) : > add acl dangerous_words << + clear + delete + kill + close + remove + And suddenly this "clear" word appearing first in the payload is mistakenly used as an extension of the command line and silently destroys previous contents before loading the 4 remaining words. > 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. And this is exactly what prevents commands from being extended in the future : you know what you used to support as arguments because it was in the code, but you don't know what users used to feed via the payload so you have no way to avoid causing conflicts. I could make an analogy with HTTP headers vs payload. Each one has its own role and you never want to look up a cookie or accept-encoding in the payload of a POST for example. But at the same time it doesn't prevent specific applications from specifically implementing support for picking the information they need from the payload. It just doesn't happen this way in general. > > 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 > > $ Do you realize that as a *user* I don't understand how to safely and reliably use this ? It makes me feel like I just throw a random amount of words to the command and that it will figure by itself which ones are arguments and which ones are data to be loaded :-/ I ca
Re: Question regarding haproxy backend behaviour
Hi Thanks Igor/Moemen for your response. I hadn't considered frontend queuing, although I am not sure where to measure it. I have wound down the benchmark infrastructure for time being and it would take me some time to replicate it again for providing additional stats. In the meantime, I am attaching the sample logs of 200 lines for benchmarks from 1 of the haproxy server. Reading the logs however, I could see that both srv_queue and backend_queue are 0. One detail that you may notice reading the logs, that I had omitted earlier for sake of simplicity is that nginx_ssl_fe frontend is bound on 2 processes to split cpu load. So instead of this: frontend nginx_ssl_fe >> bind *:8443 ssl >> maxconn 10 >> bind-process 2 >> >> It has > bind-process 2 3 In these logs haproxy ssl_sess_id_router frontend is doing 21k frontend connections, and both processes of nginx_ssl_fe are doing approx 10k frontend connections for total of ~20k frontend connections. This is just one node there are 3 more nodes like this, making the frontend connections in the ssl_sess_id_router frontend ~63k and ~60k in all frontends for nginx_ssl_fe. The nginx is still handling only 32k connections from nginx_backend. Please let me know if you need more info. Thanks, Ayush Goyal On Tue, Apr 17, 2018 at 10:03 PM Moemen MHEDHBI wrote: > Hi > > On 16/04/2018 12:04, Igor Cicimov wrote: > > > > On Mon, 16 Apr 2018 6:09 pm Ayush Goyal wrote: > >> Hi Moemen, >> >> Thanks for your response. But I think I need to clarify a few things >> here. >> >> On Mon, Apr 16, 2018 at 4:33 AM Moemen MHEDHBI >> wrote: >> >>> Hi >>> >>> On 12/04/2018 19:16, Ayush Goyal wrote: >>> >>> Hi, >>> >>> I have a question regarding haproxy backend connection behaviour. We >>> have following setup: >>> >>> +-+ +---+ >>> | haproxy |>| nginx | >>> +-+ +---+ >>> >>> We use a haproxy cluster for ssl off-loading and then load balance >>> request to >>> nginx cluster. We are currently benchmarking this setup with 3 nodes for >>> haproxy >>> cluster and 1 nginx node. Each haproxy node has two frontend/backend >>> pair. First >>> frontend is a router for ssl connection which redistributes request to >>> the second >>> frontend in the haproxy cluster. The second frontend is for ssl >>> handshake and >>> routing requests to nginx servers. Our configuration is as follows: >>> >>> ``` >>> global >>> maxconn 10 >>> user haproxy >>> group haproxy >>> nbproc 2 >>> cpu-map 1 1 >>> cpu-map 2 2 >>> >>> defaults >>> mode http >>> option forwardfor >>> timeout connect 5s >>> timeout client 30s >>> timeout server 30s >>> timeout tunnel 30m >>> timeout client-fin 5s >>> >>> frontend ssl_sess_id_router >>> bind *:443 >>> bind-process 1 >>> mode tcp >>> maxconn 10 >>> log global >>> option tcp-smart-accept >>> option splice-request >>> option splice-response >>> default_backend ssl_sess_id_router_backend >>> >>> backend ssl_sess_id_router_backend >>> bind-process 1 >>> mode tcp >>> fullconn 5 >>> balance roundrobin >>> .. >>> option tcp-smart-connect >>> server lbtest01 :8443 weight 1 check send-proxy >>> server lbtest02 :8443 weight 1 check send-proxy >>> server lbtest03 :8443 weight 1 check send-proxy >>> >>> frontend nginx_ssl_fe >>> bind *:8443 ssl >>> maxconn 10 >>> bind-process 2 >>> option tcp-smart-accept >>> option splice-request >>> option splice-response >>> option forwardfor >>> reqadd X-Forwarded-Proto:\ https >>> timeout client-fin 5s >>> timeout http-request 8s >>> timeout http-keep-alive 30s >>> default_backend nginx_backend >>> >>> backend nginx_backend >>> bind-process 2 >>> balance roundrobin >>> http-reuse safe >>> option tcp-smart-connect >>> option splice-request >>> option splice-response >>> timeout tunnel 30m >>> timeout http-request 8s >>> timeout http-keep-alive 30s >>> server testnginx :80 weight 1 check >>> ``` >>> >>> The nginx node has nginx with 4 workers and 8192 max clients, therefore >>> the max >>> number of connection it can accept is 32768. >>> >>> For benchmark, we are generating ~3k new connections per second where >>> each >>> connection makes 1 http request and then holds the connection for next 30 >>> seconds. This results in a high established connection on the first >>> frontend, >>> ssl_sess_id_router, ~25k per haproxy node (Total ~77k connections on 3 >>> haproxy >>> nodes). The second frontend (nginx_ssl_fe) receives the same number of >>> connection on the frontend. On nginx node, we see that active connections >>> increase to ~32k. >>> >>> Our understanding is that haproxy should kee
Re: Dynamically adding/deleting SSL certificates
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, struct bind_conf *bind_conf, struct ssl_bind_conf *ssl_conf,
- char **sni_filter, int fcount, ch
Re: Fix building haproxy 1.8.5 with LibreSSL 2.6.4
Hi Emeric,
> Le 18 avr. 2018 à 14:21, Emeric Brun a écrit :
>
> On 04/16/2018 02:30 PM, Dmitry Sivachenko wrote:
>>
>>> On 07 Apr 2018, at 17:38, Emmanuel Hocdet wrote:
>>>
>>>
>>> I Andy
>>>
Le 31 mars 2018 à 16:43, Andy Postnikov a écrit :
I used to rework previous patch from Alpinelinux to build with latest
stable libressl
But found no way to run tests with openssl which is primary library as I
see
Is it possible to accept the patch upstream or get review on it?
>>>
>>>
>>> @@ -2208,7 +2223,7 @@
>>> #else
>>> cipher = SSL_CIPHER_find(ssl, cipher_suites);
>>> #endif
>>> - if (cipher && SSL_CIPHER_get_auth_nid(cipher) ==
>>> NID_auth_ecdsa) {
>>> + if (cipher && SSL_CIPHER_is_ECDSA(cipher)) {
>>> has_ecdsa = 1;
>>> break;
>>> }
>>>
>>> No, it’s a regression in lib compatibility.
>>>
>>
>>
>> Hello,
>>
>> it would be nice if you come to an acceptable solution and finally merge
>> LibreSSL support.
>> There were several attempts to propose LibreSSL support in the past and
>> every time discussion dies with no result.
>>
>> Thanks :)
>>
>>
>>
>
> What do you think Manu?
>
At least, regression should be fixed, it breaks openssl and boringssl build.
(SSL_CIPHER_get_auth_nid has been added in LibreSSL 2.7)
Otherwise the code only affects parts related to openssl, not boringssl.
++
Manu
Re: Fix building haproxy 1.8.5 with LibreSSL 2.6.4
On 04/16/2018 02:30 PM, Dmitry Sivachenko wrote:
>
>> On 07 Apr 2018, at 17:38, Emmanuel Hocdet wrote:
>>
>>
>> I Andy
>>
>>> Le 31 mars 2018 à 16:43, Andy Postnikov a écrit :
>>>
>>> I used to rework previous patch from Alpinelinux to build with latest
>>> stable libressl
>>> But found no way to run tests with openssl which is primary library as I see
>>> Is it possible to accept the patch upstream or get review on it?
>>>
>>>
>>
>>
>> @@ -2208,7 +2223,7 @@
>> #else
>> cipher = SSL_CIPHER_find(ssl, cipher_suites);
>> #endif
>> -if (cipher && SSL_CIPHER_get_auth_nid(cipher) ==
>> NID_auth_ecdsa) {
>> +if (cipher && SSL_CIPHER_is_ECDSA(cipher)) {
>> has_ecdsa = 1;
>> break;
>> }
>>
>> No, it’s a regression in lib compatibility.
>>
>
>
> Hello,
>
> it would be nice if you come to an acceptable solution and finally merge
> LibreSSL support.
> There were several attempts to propose LibreSSL support in the past and every
> time discussion dies with no result.
>
> Thanks :)
>
>
>
What do you think Manu?
R,
Emeric
Re: [PATCH] MEDIUM: cli: Add multi-line mode support
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.
