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([<filter_option>]) : 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} {
> txreq
> rxresp
> expect resp.status == 200
> + expect resp.http.x-ssl-fc-curve-name == "prime256v1"
> } -run
>
> # The backend tries to use the prime256v1 curve that is not accepted by the
> @@ -129,6 +132,7 @@ client c4 -connect ${h1_clearlst_sock} {
> txreq -url "/ecdhe-256"
> rxresp
> expect resp.status == 200
> + expect resp.http.x-ssl-fc-curve-name == "prime256v1"
> } -run
>
Be careful with the tests, they should pass with all supported
SSL libraries, which is quite difficult to do.
I pushed your patch in this branch so you could check the CI:
https://github.com/haproxy/haproxy/tree/feat-2165
I recommend you to run your own CI on a github fork, just activate
github action on your fork so all the reg-tests
are run with openssl 1.0.2, 1.1.1, 3.0, 3.1, libreSSL and quicTLS, doing
it manually is quite difficult.
You can create a new .vtc file for your test, and sets a required SSL
version in its header, we can't disable part of the tests in the .vtc
depending on the openssl version unfortunately.
Also, try to check the other ssl_bc_curves sample fetch in your test.
Don't hesitate to ask if you have difficulties with this part, it can be
bothersome.
> syslog Slg_cust_fmt -wait
> diff --git a/src/ssl_sample.c b/src/ssl_sample.c
> index 5aec97fef..d7a7a09f9 100644
> --- a/src/ssl_sample.c
> +++ b/src/ssl_sample.c
> @@ -1304,6 +1304,46 @@ smp_fetch_ssl_fc_is_resumed(const struct arg *args,
> struct sample *smp, const ch
> return 1;
> }
>
> +/*
> + * string, returns the EC curve used for key agreement on the
> + * front and backend connection.
> + *
> + * The function to get the curve name (SSL_get_negotiated_group) is only
> available
> + * in OpenSSLv3 onwards and not for previous versions.
> + */
> +#if (HA_OPENSSL_VERSION_NUMBER >= 0x3000000fL)
> +static int
> +smp_fetch_ssl_fc_ec(const struct arg *args, struct sample *smp, const char
> *kw, void *private)
> +{
> + struct connection *conn;
> + SSL *ssl;
> + int nid;
> +
> + if (obj_type(smp->sess->origin) == OBJ_TYPE_CHECK)
> + conn = (kw[4] == 'b') ? sc_conn(__objt_check(smp->sess->origin)->sc)
> : NULL;
> + else
> + conn = (kw[4] != 'b') ? objt_conn(smp->sess->origin) :
> + smp->strm ? sc_conn(smp->strm->scb) : NULL;
> +
> + ssl = ssl_sock_get_ssl_object(conn);
> + if (!ssl)
> + return 0;
> +
> + nid = SSL_get_negotiated_group(ssl);
> + if (!nid)
> + return 0;
> + smp->data.u.str.area = (char *)OBJ_nid2sn(nid);
> + if (!smp->data.u.str.area)
> + return 0;
> +
> + smp->data.type = SMP_T_STR;
> + smp->flags |= SMP_F_VOL_SESS | SMP_F_CONST;
> + smp->data.u.str.data = strlen(smp->data.u.str.area);
> +
> + return 1;
> +}
> +#endif
The code looks good to me, only the reg-tests part should be reworked IMO.
Regards,
--
William Lallemand
