RE: [PATCH 1/1] MEDIUM: ssl: new sample fetch method to get curve name

2023-06-21 Thread Mariam John
Thank you Alex and William for reviewing my PR. Appreciate your feedback. I 
will push out another patch with all the recommended changes soon. There are a 
few things that are not clear about the vtc tests but let me spend some time 
working on it first. This is my first big contribution to the project, so I am 
still learning the process. Appreciate your patience and guidance.

Thank you and have a good day.

Regards,
Mariam.

From: Aleksandar Lazic 
Date: Tuesday, June 20, 2023 at 3:51 PM
To: Mariam John , haproxy@formilux.org 

Cc: eb...@haproxy.com , wlallem...@haproxy.com 

Subject: [EXTERNAL] Re: [PATCH 1/1] MEDIUM: ssl: new sample fetch method to get 
curve name
Hi.

On 2023-06-20 (Di.) 18:50, Mariam John wrote:
> Adds a new sample fetch method to get the curve name used in the
> key agreement to enable better observability. In OpenSSLv3, the function
> `SSL_get_negotiated_group` returns the NID of the curve and from the NID,
> we get the curve name by passing the NID to OBJ_nid2sn. This was not
> available in v1.1.1. SSL_get_curve_name(), which returns the curve name
> directly was merged into OpenSSL master branch last week but will be available
> only in its next release.
> ---
>   doc/configuration.txt|  8 +
>   reg-tests/ssl/ssl_client_samples.vtc |  2 ++
>   reg-tests/ssl/ssl_curves.vtc |  4 +++
>   src/ssl_sample.c | 46 
>   4 files changed, 60 insertions(+)
>
> diff --git a/doc/configuration.txt b/doc/configuration.txt
> index 8bcfc3c06..d944ac132 100644
> --- a/doc/configuration.txt
> +++ b/doc/configuration.txt
> @@ -20646,6 +20646,10 @@ ssl_bc_cipher : string
> over an SSL/TLS transport layer. It can be used in a tcp-check or an
> http-check ruleset.
>
> +ssl_bc_curve : string
> +  Returns the name of the curve used in the key agreement when the outgoing
> +  connection was made over an SSL/TLS transport layer.
> +
>   ssl_bc_client_random : binary
> Returns the client random of the back connection when the incoming 
> connection
> was made over an SSL/TLS transport layer. It is useful to to decrypt 
> traffic
> @@ -20944,6 +20948,10 @@ ssl_fc_cipher : string
> Returns the name of the used cipher when the incoming connection was made
> over an SSL/TLS transport layer.
>
> +ssl_fc_curve : string
> +  Returns the name of the curve used in the key agreement when the incoming
> +  connection was made over an SSL/TLS transport layer.
> +
>   ssl_fc_cipherlist_bin([]) : binary
> Returns the binary form of the client hello cipher list. The maximum
> returned value length is limited by the shared capture buffer size

Please can you sort the key words in proper alphabetical order.

Please can you add "Require OpenSSL >= 3..." the right version simlar to
https://docs.haproxy.org/2.8/configuration.html#7.3.4-ssl_fc_server_handshake_traffic_secret
.


> diff --git a/reg-tests/ssl/ssl_client_samples.vtc 
> b/reg-tests/ssl/ssl_client_samples.vtc
> index 5a84e4b25..1f078ea98 100644
> --- a/reg-tests/ssl/ssl_client_samples.vtc
> +++ b/reg-tests/ssl/ssl_client_samples.vtc
> @@ -46,6 +46,7 @@ haproxy h1 -conf {
>   http-response add-header x-ssl-s_serial %[ssl_c_serial,hex]
>   http-response add-header x-ssl-key_alg %[ssl_c_key_alg]
>   http-response add-header x-ssl-version %[ssl_c_version]
> +http-response add-header x-ssl-curve-name %[ssl_fc_curve]
>
>   bind "${tmpdir}/ssl.sock" ssl crt ${testdir}/common.pem ca-file 
> ${testdir}/ca-auth.crt verify optional crt-ignore-err all crl-file 
> ${testdir}/crl-auth.pem
>
> @@ -69,6 +70,7 @@ client c1 -connect ${h1_clearlst_sock} {
>   expect resp.http.x-ssl-s_serial == "02"
>   expect resp.http.x-ssl-key_alg == "rsaEncryption"
>   expect resp.http.x-ssl-version == "1"
> +expect resp.http.x-ssl-curve-name == "X25519"
>   } -run
>
>
> diff --git a/reg-tests/ssl/ssl_curves.vtc b/reg-tests/ssl/ssl_curves.vtc
> index 5cc70df14..3dbe47c4d 100644
> --- a/reg-tests/ssl/ssl_curves.vtc
> +++ b/reg-tests/ssl/ssl_curves.vtc
> @@ -75,6 +75,7 @@ haproxy h1 -conf {
>   listen ssl1-lst
>   bind "${tmpdir}/ssl1.sock" ssl crt ${testdir}/common.pem ca-file 
> ${testdir}/set_cafile_rootCA.crt verify optional curves P-256:P-384
>   server s1 ${s1_addr}:${s1_port}
> +http-response add-header x-ssl-fc-curve-name %[ssl_fc_curve]
>
>   # The prime256v1 curve, which is used by default by a backend when no
>   # 'curves' or 'ecdhe' option is specified, is not allowed on this 
> listener
> @@ -98,6 +99,7 @@ haproxy h1 -conf {
>
>   bind "${tmpdir}/ssl-ecdhe-256.sock" ssl crt ${testdir}/common.pem 
> ca-file ${testdir}/set_cafile_rootCA.crt verify optional ecdhe prime256v1
>   server s1 ${s1_addr}:${s1_port}
> +http-response add-header x-ssl-fc-curve-name %[ssl_fc_curve]
>
>   } -start
>
> @@ -105,6 +107,7 @@ client c1 -connect ${h1_clearlst_sock} {
> txreq
> 

Re: [PATCH 0/1] Implement new sample fetch method to get the curve name used in key agreement

2023-06-21 Thread William Lallemand
Hello Mariam,

On Tue, Jun 20, 2023 at 11:50:51AM -0500, Mariam John wrote:
> This is an implementation of feature request 
> [#2165](https://github.com/haproxy/haproxy/issues/2165),
> to get the EC curve name used during the key agreement in OpenSSL. This patch 
> includes the following
> changes:
> - new sample fetch methods `ssl_fc_curve` and `ssl_bc_curve`, to get the 
> curve name
> - doc changes to add description for the new sample fetch methods
> - updates the existing regression tests to test the new sample fetch methods
> 
> This uses the function `SSL_get_negotiated_group` method available from the 
> OpenSSLv3 release. 
> 

Thanks for your contribution, review below.


On Tue, Jun 20, 2023 at 11:50:52AM -0500, Mariam John wrote:
> Adds a new sample fetch method to get the curve name used in the
> key agreement to enable better observability. In OpenSSLv3, the function
> `SSL_get_negotiated_group` returns the NID of the curve and from the NID,
> we get the curve name by passing the NID to OBJ_nid2sn. This was not
> available in v1.1.1. SSL_get_curve_name(), which returns the curve name
> directly was merged into OpenSSL master branch last week but will be available
> only in its next release.
> ---
>  doc/configuration.txt|  8 +
>  reg-tests/ssl/ssl_client_samples.vtc |  2 ++
>  reg-tests/ssl/ssl_curves.vtc |  4 +++
>  src/ssl_sample.c | 46 
>  4 files changed, 60 insertions(+)
> 
> diff --git a/doc/configuration.txt b/doc/configuration.txt
> index 8bcfc3c06..d944ac132 100644
> --- a/doc/configuration.txt
> +++ b/doc/configuration.txt
> @@ -20646,6 +20646,10 @@ ssl_bc_cipher : string
>over an SSL/TLS transport layer. It can be used in a tcp-check or an
>http-check ruleset.
>  
> +ssl_bc_curve : string
> +  Returns the name of the curve used in the key agreement when the outgoing
> +  connection was made over an SSL/TLS transport layer.
> +
>  ssl_bc_client_random : binary
>Returns the client random of the back connection when the incoming 
> connection
>was made over an SSL/TLS transport layer. It is useful to to decrypt 
> traffic
> @@ -20944,6 +20948,10 @@ ssl_fc_cipher : string
>Returns the name of the used cipher when the incoming connection was made
>over an SSL/TLS transport layer.
>  
> +ssl_fc_curve : string
> +  Returns the name of the curve used in the key agreement when the incoming
> +  connection was made over an SSL/TLS transport layer.
> +

As Lukas mentioned, be careful with the alphabetical order in the
configuration and the required OpenSSL version.

>  ssl_fc_cipherlist_bin([]) : binary
>Returns the binary form of the client hello cipher list. The maximum
>returned value length is limited by the shared capture buffer size
> diff --git a/reg-tests/ssl/ssl_client_samples.vtc 
> b/reg-tests/ssl/ssl_client_samples.vtc
> index 5a84e4b25..1f078ea98 100644
> --- a/reg-tests/ssl/ssl_client_samples.vtc
> +++ b/reg-tests/ssl/ssl_client_samples.vtc
> @@ -46,6 +46,7 @@ haproxy h1 -conf {
>  http-response add-header x-ssl-s_serial %[ssl_c_serial,hex]
>  http-response add-header x-ssl-key_alg %[ssl_c_key_alg]
>  http-response add-header x-ssl-version %[ssl_c_version]
> +http-response add-header x-ssl-curve-name %[ssl_fc_curve]
>  
>  bind "${tmpdir}/ssl.sock" ssl crt ${testdir}/common.pem ca-file 
> ${testdir}/ca-auth.crt verify optional crt-ignore-err all crl-file 
> ${testdir}/crl-auth.pem
>  
> @@ -69,6 +70,7 @@ client c1 -connect ${h1_clearlst_sock} {
>  expect resp.http.x-ssl-s_serial == "02"
>  expect resp.http.x-ssl-key_alg == "rsaEncryption"
>  expect resp.http.x-ssl-version == "1"
> +expect resp.http.x-ssl-curve-name == "X25519"
>  } -run
>  

The ssl_client_samples.vtc file is dedicated to samples used in Client
Authentication, so I don't think it should be here.


> diff --git a/reg-tests/ssl/ssl_curves.vtc b/reg-tests/ssl/ssl_curves.vtc
> index 5cc70df14..3dbe47c4d 100644
> --- a/reg-tests/ssl/ssl_curves.vtc
> +++ b/reg-tests/ssl/ssl_curves.vtc
> @@ -75,6 +75,7 @@ haproxy h1 -conf {
>  listen ssl1-lst
>  bind "${tmpdir}/ssl1.sock" ssl crt ${testdir}/common.pem ca-file 
> ${testdir}/set_cafile_rootCA.crt verify optional curves P-256:P-384
>  server s1 ${s1_addr}:${s1_port}
> +http-response add-header x-ssl-fc-curve-name %[ssl_fc_curve]
>  
>  # The prime256v1 curve, which is used by default by a backend when no
>  # 'curves' or 'ecdhe' option is specified, is not allowed on this 
> listener
> @@ -98,6 +99,7 @@ haproxy h1 -conf {
>  
>  bind "${tmpdir}/ssl-ecdhe-256.sock" ssl crt ${testdir}/common.pem 
> ca-file ${testdir}/set_cafile_rootCA.crt verify optional ecdhe prime256v1
>  server s1 ${s1_addr}:${s1_port}
> +http-response add-header x-ssl-fc-curve-name %[ssl_fc_curve]
>  
>  } -start
>  
> @@ -105,6 +107,7 @@ client c1 -connect ${h1_clearlst_sock} {
>

Re: Performance cost of using Lua for service discovery

2023-06-21 Thread Thomas Pedoussaut
Hi,
For something similar, I in fact use the admin socket to haproxy to add drain 
and remove servers from backend definitions. Very light and doesn't use CPU on 
the haproxy machine.

On 21 June 2023 02:29:42 GMT+02:00, Diffie  wrote:
>Hello!
>
>I had a question about the performance of Lua in HAProxy. I am experimenting 
>with using a Lua script as a way of dynamically updating server ports. This 
>script would be used instead of HAProxy's native support for DNS SRV records 
>for service discovery (SRV records are not ideal for my use case, and I 
>believe you have been trying to move away from DNS in the core codebase, based 
>on release notes I've read on this mailing list -- please correct me if that 
>is wrong).
>
>So, I am thinking of using `core.register_task()` to run a Lua function at 
>startup. This function will talk to some simple web server over HTTP, which 
>will return a list of server ids/ip addrs/ports. Lua would then update the 
>port(s) of any servers with the relevant id using :set_addr(). The server 
>count could have a ceiling of ~1k servers. This whole thing would run in a 
>loop, and sleep every 10 seconds or so. 
>
>My question is, does the above approach sound like it could affect the 
>performance of HAProxy in some significant way (any more than a DNS SRV 
>lookup)? Also, in general, is the performance impact of core.register_task() 
>less than a standard core.register_service()? I ask since register_task() 
>would have a fixed schedule (e.g., every 10s), whereas a service could be hit 
>with every incoming request.
>
>Thanks!
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.