Re: [PATCH 0/2] openssl 1.1 async mode and engine support

[Willy Tarreau]

Hi Grant,

On Thu, Jan 19, 2017 at 11:27:43PM -0800, Grant Zhang wrote:
> Hi Willy,
> 
> Thank you very much for your review!
> 
> WRT engine configuration, I agree with your point of finer control over 
> which crypto ops get handled by hardware engine vs. software. It is possible
> to load/initialize engines using openssl configuration file, which
> is documented in https://github.com/01org/QAT_Engine (section title:
> using the openssl configuration file to load/initialize engines)
> 
> If we want more explicit control from haproxy on crypto operations handled
> by ssl engines, how about adding a "default_algorithms" parameter as part 
> of the ssl_engine config line, where default_algorithms specifies which 
> algorithms supplied by the engine should be used by default. Specify ALL 
> to make all algorithms supplied by the engine be used by default. 
> Something like the following:
> 
> # offload RSA and EC operations to qat engine
> ssl_engine qat default_algorithms RSA,EC

This is *exactly* what the second patch I sent does so I think we're
in line here. My limited understanding of the crypto engines made me
unsure whether we could do better or not.

> default_algorithms could ALL, RSA, DSA, EC, ... and they could be comma 
> seperated.
> All available values could be found at: 
> https://github.com/openssl/openssl/blob/master/crypto/engine/eng_fat.c#L54
> 
> Apparently "default_algorithms" is how openssl solve the engine config 
> problem: 
> https://github.com/openssl/openssl/blob/master/crypto/engine/eng_cnf.c#L125
> 
> In case there are multiple engines the config might be like:
> ssl_engine qat default_algorithms RSA
> ssl_engine dasync default_algorithms RAND
> 
> What do you think?

I didn't think it was possible to load multiple engines. Then that makes
total sense! Then this means that the "engine" in global_ssl should instead
become a list of (name,args*).

> I am working on V2 version of my patch to address your comments. Hopefully
> will send it out soon:-)

Great, thanks!

Willy

Re: [PATCH 0/2] openssl 1.1 async mode and engine support

[Grant Zhang]

Hi Willy,

Thank you very much for your review!

WRT engine configuration, I agree with your point of finer control over 
which crypto ops get handled by hardware engine vs. software. It is possible
to load/initialize engines using openssl configuration file, which
is documented in https://github.com/01org/QAT_Engine (section title:
using the openssl configuration file to load/initialize engines)

If we want more explicit control from haproxy on crypto operations handled
by ssl engines, how about adding a "default_algorithms" parameter as part 
of the ssl_engine config line, where default_algorithms specifies which 
algorithms supplied by the engine should be used by default. Specify ALL 
to make all algorithms supplied by the engine be used by default. 
Something like the following:

# offload RSA and EC operations to qat engine
ssl_engine qat default_algorithms RSA,EC

default_algorithms could ALL, RSA, DSA, EC, ... and they could be comma 
seperated.
All available values could be found at: 
https://github.com/openssl/openssl/blob/master/crypto/engine/eng_fat.c#L54

Apparently "default_algorithms" is how openssl solve the engine config problem: 
https://github.com/openssl/openssl/blob/master/crypto/engine/eng_cnf.c#L125

In case there are multiple engines the config might be like:
ssl_engine qat default_algorithms RSA
ssl_engine dasync default_algorithms RAND

What do you think?

Thanks,

Grant

I am working on V2 version of my patch to address your comments. Hopefully will
send it out soon:-)


> On Jan 19, 2017, at 10:38, Willy Tarreau  wrote:
> 
> Hi Grant,
> 
> sorry for the longer delay than what I expected.
> 
> I rebased your work on top of the latest development tree so that it's
> easier to review since it's where it's ultimately going to get merged.
> 
> First I have a question regarding the engines configuration. I still have
> a pretty similar patch lying around that I never merged because I was not
> happy with having to enable all algorithms or none on the engine. I had
> tested some low performance crypto cards in the past and they were speeding
> up some operations and slowing down others.
> 
> Even today, if I test on my 4-core home PC with a Skylake CPU, aes-128-gcm
> gives me 218 Gbps of bandwidth which no hardware accelerator will be able
> to do given that this requires a huge amount of PCI bandwidth.
> 
> Thus ideally I'd like to be able to delegate the heavy stuff to an engine
> and let the CPU deal with the cheap stuff. That's mostly symmetric vs
> asymmetric, but I had cut it around all the ENGINE_register_*() functions
> that openssl exposes. I don't know the level of granularity we have with
> the engines (which is also why I never merged this patch).
> 
> I remember for example a crypto card where operations were not faster but
> they would at least offload the CPU, and in the end it was faster to let
> the card sign with an RSA key and run the DH parts on the CPU so that both
> could work more or less in parallel. Also it's unclear to me whether or not
> it is possible to use multiple engines. Eg, I remember that some engines
> can provide random, which can be convenient if a card doesn't, or if some
> algos are not supported by the card and only the randoms are desired.
> 
> Thus whatever information you can find in this area would be useful. I'm
> appending the two patches I used to work on by then (it was 4 years ago
> likely for version 1.5 but you'll see how similar they are to yours so
> you'll easily understand).
> 
> Now let's go to the patches below :-)
> 
>> From 0961d9807d9af10c841824bc1a5a4d72b02f Mon Sep 17 00:00:00 2001
>> From: Grant Zhang 
>> Date: Sat, 14 Jan 2017 01:42:14 +
>> Subject: RFC: add openssl engine support
>> 
>> Global configuration parameter "ssl_engine" may be used to specify
>> openssl engine.
>> ---
> 
> For this one you can happily steal the doc part from my initial patch, it
> should match.
> 
> (...)
>> diff --git a/src/ssl_sock.c b/src/ssl_sock.c
>> index 95f979c..9d07f9c 100644
>> --- a/src/ssl_sock.c
>> +++ b/src/ssl_sock.c
>> @@ -56,6 +56,8 @@
>> #include 
>> #include 
>> 
>> +#include 
>> +
>> #include 
>> #include 
>> #include 
>> @@ -135,6 +137,7 @@ static struct xprt_ops ssl_sock;
>> static struct {
>>  char *crt_base; /* base directory path for certificates */
>>  char *ca_base;  /* base directory path for CAs and CRLs */
>> +char *engine;   /* openssl engine to use */
>> 
>>  char *listen_default_ciphers;
>>  char *connect_default_ciphers;
>> @@ -263,6 +266,42 @@ static forceinline void ssl_sock_dump_errors(struct 
>> connection *conn)
>>  }
>> }
>> 
>> +static ENGINE *engine;
>> +
>> +void ssl_init_engine(const char *engine_id)
>> +{
>> +OpenSSL_add_all_algorithms();
>> +ENGINE_load_builtin_engines();
>> +RAND_set_rand_method(NULL);
>> +
>> +/* grab the structural reference to the engine */
>> +engine = 

Dynamically adding server's to backend

[Michał]

Hello,

What do you think about making possible to add servers to backend at
runtime via CLI/socket? Changing addresses and wieghts of servers is ok,
because we can use "placeholders", but it's not the best solution and
requires to configure many placeholders when it comes to autoscaling.

This problem solves reloading haproxy with safe loading certificates to
encrypted volume which takes some time and in dynamically changing world of
microservices - it just kills whole load balancing solutions with endless
reloads.

I'm not sure how it will work, so the only question is if this feature is
welcome in haproxy or you don't want to add it now. I will try to do it
easiest way possible and check if any algorithms can break after adding
server at runtime, so if you are interested in such patch - I will report
how it works and if there is such need - what we should do to support it if
there's no such possibility to merge this feature in current state.

Michał

Propagating agent-check weight change to tracking servers

[Michał]

Hello,

We use track's in haproxy to minimize check traffic in some situations and
after my last patch we are probably going to switch to agent-checks for
live management of weights and statuses. One problem I see now - track
don't propagate weight setting to trackers, so if we set agent-check on
track we can manage status only.

My first PoC solution works good, so I thought about introducing something
like agent-track or track-agent directive set on backends (or maybe it
should be default, non-configurable behaviour) to propagate agent-check
responses from main check to all tracking backends. Both default behaviour
and directive approach are small changes in code, but a little bigger in
functionality.

In my opinion if we set agent-check on backend which is tracked by others -
it should propagate agent-check weight response to those tracking backend
and set weight on them too. Configurable or not - it will be good feature.

Michał

Re: HAProxy Lua Map.end & reserved keywords

[Thierry Fournier]

Hi. Just a quick message: I don't forget this bug, I'm just just very busy.

Thierry


Le 12 janvier 2017 10:54:42 AM "Robin H. Johnson" 
 a écrit :



On Wed, Jan 11, 2017 at 12:17:26PM +0100, Willy Tarreau wrote:

On Mon, Jan 09, 2017 at 08:47:17PM +, Robin H. Johnson wrote:
> Maybe Willy would considering changing the name of the matches to 'prefix'
> & 'suffix' instead of 'beg' & 'end', and just keep beg/end as legacy.
Another approach would be to have simple rules when mapping a namespace
to another one. For example we could easily imagine that all "map_something"
in haproxy are mapped to their exact equivalent name in Lua (thus we would
have "map_end" instead of "map.end"), or that we could prefix all these
words by "_" which would give "map._end" which is not much different from
the haproxy keyword "map_end" and easy enough to remember.

This is actually why I thought of Map.match_str, with a potential idea
that at some point it MIGHT become a bitfield for additional behaviors
(eg the output types per
https://cbonte.github.io/haproxy-dconv/1.6/configuration.html#7.3.1-map).

The underscore is not sufficiently unique I think, but my knowledge of
Lua is nowhere near as extensive as I'd like.

--
Robin Hugh Johnson
E-Mail : robb...@orbis-terrarum.net
Home Page  : http://www.orbis-terrarum.net/?l=people.robbat2
ICQ#   : 30269588 or 41961639
GnuPG FP   : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85

Re: [PATCH 0/2] openssl 1.1 async mode and engine support

[Willy Tarreau]

Hi Grant,

sorry for the longer delay than what I expected.

I rebased your work on top of the latest development tree so that it's
easier to review since it's where it's ultimately going to get merged.

First I have a question regarding the engines configuration. I still have
a pretty similar patch lying around that I never merged because I was not
happy with having to enable all algorithms or none on the engine. I had
tested some low performance crypto cards in the past and they were speeding
up some operations and slowing down others.

Even today, if I test on my 4-core home PC with a Skylake CPU, aes-128-gcm
gives me 218 Gbps of bandwidth which no hardware accelerator will be able
to do given that this requires a huge amount of PCI bandwidth.

Thus ideally I'd like to be able to delegate the heavy stuff to an engine
and let the CPU deal with the cheap stuff. That's mostly symmetric vs
asymmetric, but I had cut it around all the ENGINE_register_*() functions
that openssl exposes. I don't know the level of granularity we have with
the engines (which is also why I never merged this patch).

I remember for example a crypto card where operations were not faster but
they would at least offload the CPU, and in the end it was faster to let
the card sign with an RSA key and run the DH parts on the CPU so that both
could work more or less in parallel. Also it's unclear to me whether or not
it is possible to use multiple engines. Eg, I remember that some engines
can provide random, which can be convenient if a card doesn't, or if some
algos are not supported by the card and only the randoms are desired.

Thus whatever information you can find in this area would be useful. I'm
appending the two patches I used to work on by then (it was 4 years ago
likely for version 1.5 but you'll see how similar they are to yours so
you'll easily understand).

Now let's go to the patches below :-)

> From 0961d9807d9af10c841824bc1a5a4d72b02f Mon Sep 17 00:00:00 2001
> From: Grant Zhang 
> Date: Sat, 14 Jan 2017 01:42:14 +
> Subject: RFC: add openssl engine support
> 
> Global configuration parameter "ssl_engine" may be used to specify
> openssl engine.
> ---

For this one you can happily steal the doc part from my initial patch, it
should match.

(...)
> diff --git a/src/ssl_sock.c b/src/ssl_sock.c
> index 95f979c..9d07f9c 100644
> --- a/src/ssl_sock.c
> +++ b/src/ssl_sock.c
> @@ -56,6 +56,8 @@
>  #include 
>  #include 
>  
> +#include 
> +
>  #include 
>  #include 
>  #include 
> @@ -135,6 +137,7 @@ static struct xprt_ops ssl_sock;
>  static struct {
>   char *crt_base; /* base directory path for certificates */
>   char *ca_base;  /* base directory path for CAs and CRLs */
> + char *engine;   /* openssl engine to use */
>  
>   char *listen_default_ciphers;
>   char *connect_default_ciphers;
> @@ -263,6 +266,42 @@ static forceinline void ssl_sock_dump_errors(struct 
> connection *conn)
>   }
>  }
>  
> +static ENGINE *engine;
> +
> +void ssl_init_engine(const char *engine_id)
> +{
> + OpenSSL_add_all_algorithms();
> + ENGINE_load_builtin_engines();
> + RAND_set_rand_method(NULL);
> +
> + /* grab the structural reference to the engine */
> + engine = ENGINE_by_id(engine_id);
> + if (engine  == NULL) {
> + Alert("Engine %s: failed to get structural reference\n", 
> engine_id);

Please mention "SSL-Engine" so that there's absolutely no ambiguity
when a user meets such an error.

> + exit(-1);

Please don't exit() directly from functions, it makes it impossible to
run validity checks on the configs. Instead simply return an error code
that the caller will check. Specifically we're used to have return codes
having some flags like ERR_FATAL, ERR_ABORT or ERR_WARN depending on the
criticity of the error, that's very convenient.

> + }
> +
> + if (!ENGINE_init(engine)) {
> + /* the engine couldn't initialise, release it */
> + Alert("Engine %s: failed to initialize\n", engine_id);
> + ENGINE_free(engine);

Here on the error unrolling, better add labels and goto the end of the
function. That makes it easier to validate that nothing is missing (eg
if we later insert something in the middle) of these calls.

> + return;
> + }
> +
> + if (ENGINE_set_default(engine, ENGINE_METHOD_ALL) == 0) {
> + Alert("Engine %s: ENGINE_set_default failed\n", engine_id);
> + ENGINE_finish(engine);
> + ENGINE_free(engine);

Same here.

> + return;
> + }
> +
> + /* release the functional reference from ENGINE_init() */
> + ENGINE_finish(engine);
> +
> + /* release the structural reference from ENGINE_by_id() */
> + ENGINE_free(engine);
> +}
> +
>  /*
>   *  This function returns the number of seconds  elapsed
>   *  since the Epoch, 1970-01-01 00:00:00 + (UTC) and the
> @@ -6365,6 +6404,26 

Re: Status code "-1" in logs

[Andrew Smalley]

Hello John

Thank you for your clarification,

I guess its an easy mistake to make when you see a 503 and assume its the
error when I knew you were talking about the "-1" issue.



Regards

Andrew Smalley

Loadbalancer.org Ltd.



On 19 January 2017 at 00:24, Skarbek, John  wrote:

> Hey Andrew,
>
> On January 18, 2017 at 16:11:55, Andrew Smalley (asmal...@loadbalancer.org)
> wrote:
>
> Hello John
>
> The problem is you are getting a 503 error or no servers available.
>
>503  when no server was available to handle the request, or in response to
> monitoring requests which match the "monitor fail" condition
>
>
> Just for clarification here, the position of 503 is in the spot where the
> size of the data transfer is.  So unless there's some awkward parsing, this
> 503 is not the error code in this case.  I don't have custom log
> configurations, so I'm sitting here assuming that the -1 is where the
> status code ought to be.  5 positions of timing sperated by a '/', all of
> which look kinda legit considering the termination state.  Followed by a
> status code, in my case -1, followed by 'bytes read', in my case 503.
>
>
>
> And the CDNN is actually two errors CD and NN
>
>   CD   The client unexpectedly aborted during data transfer. This can be
>   caused by a browser crash, by an intermediate equipment between the
>   client and haproxy which decided to actively break the connection,
>   by network routing issues between the client and haproxy, or by a
>   keep-alive session between the server and the client terminated 
> first
>   by the client.
>
>NN   No cookie was provided by the client, none was inserted in the
>   response. For instance, this can be in insert mode with "postonly"
>   set on a GET request.
>
> More information could be provided with a valid configuration
>
> I hope this helps?
>
> I took the information from the Documents available here
> http://www.haproxy.org/download/1.8/doc/configuration.txt
> 
>
> Regards
>
> Andrew Smalley
>
> Loadbalancer.org
> 
>  Ltd.
>
>
>
> On 18 January 2017 at 21:04, Skarbek, John  wrote:
>
>> Good Morning,
>>
>> I was spying on my logs and something out of the ordinary popped out at
>> me. We are getting a status code of -1. The status CDNN is odd enough as
>> it is… Why would this be?
>>
>> Jan 18 13:47:18 example.com 
>> 
>>  Jan 18 20:47:18 haproxy[23541]: 10.0.0.1:24550 
>> 
>>  [18/Jan/2017:20:47:16.412] fe~ be/10.1.0.1:3001 
>> 
>>  282/0/1490/-1/1824 -1 503 - - CDNN 2296/96/12/4/0 0/0 "GET /healthcheck 
>> HTTP/1.1"
>> Jan 18 13:44:01 example.com 
>> 
>>  Jan 18 20:44:01 haproxy[23445]: 10.0.0.1:2650 
>> 
>>  [18/Jan/2017:20:43:59.295] fe~ be/10.1.0.1:3001 
>> 
>>  501/0/1349/-1/2079 -1 503 - - CDNN 2249/86/6/1/0 0/0 "GET /healthcheck 
>> HTTP/1.1"
>>
>>
>
>