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 <al-hapr...@none.at> Date: Tuesday, June 20, 2023 at 3:51 PM To: Mariam John <john.mariam....@gmail.com>, haproxy@formilux.org <haproxy@formilux.org> Cc: eb...@haproxy.com <eb...@haproxy.com>, wlallem...@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([<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 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 > 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 > > syslog Slg_cust_fmt -wait Please can you create a dedicated test file for that feature so that the test can be exluded when the requierd OpenSSL is not used. I think the "openssl_version_atleast(1.1.1)" should be "3....." which is in the ssl_curves.vtc file. > 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 > + > /* string, returns the used cipher if front conn. transport layer is SSL. > * This function is also usable on backend conn if the fetch keyword 5th > * char is 'b'. > @@ -2174,6 +2214,9 @@ static struct sample_fetch_kw_list > sample_fetch_keywords = {ILH, { > { "ssl_bc_alpn", smp_fetch_ssl_fc_alpn, 0, > NULL, SMP_T_STR, SMP_USE_L5SRV }, > #endif > { "ssl_bc_cipher", smp_fetch_ssl_fc_cipher, 0, > NULL, SMP_T_STR, SMP_USE_L5SRV }, > +#if (HA_OPENSSL_VERSION_NUMBER >= 0x3000000fL) > + { "ssl_bc_curve", smp_fetch_ssl_fc_ec, 0, > NULL, SMP_T_STR, SMP_USE_L5SRV }, > +#endif > #if defined(OPENSSL_NPN_NEGOTIATED) && !defined(OPENSSL_NO_NEXTPROTONEG) > { "ssl_bc_npn", smp_fetch_ssl_fc_npn, 0, > NULL, SMP_T_STR, SMP_USE_L5SRV }, > #endif > @@ -2223,6 +2266,9 @@ static struct sample_fetch_kw_list > sample_fetch_keywords = {ILH, { > { "ssl_fc", smp_fetch_ssl_fc, 0, > NULL, SMP_T_BOOL, SMP_USE_L5CLI }, > { "ssl_fc_alg_keysize", smp_fetch_ssl_fc_alg_keysize, 0, > NULL, SMP_T_SINT, SMP_USE_L5CLI }, > { "ssl_fc_cipher", smp_fetch_ssl_fc_cipher, 0, > NULL, SMP_T_STR, SMP_USE_L5CLI }, > +#if (HA_OPENSSL_VERSION_NUMBER >= 0x3000000fL) > + { "ssl_fc_curve", smp_fetch_ssl_fc_ec, 0, > NULL, SMP_T_STR, SMP_USE_L5CLI }, > +#endif > { "ssl_fc_has_crt", smp_fetch_ssl_fc_has_crt, 0, > NULL, SMP_T_BOOL, SMP_USE_L5CLI }, > { "ssl_fc_has_early", smp_fetch_ssl_fc_has_early, 0, > NULL, SMP_T_BOOL, SMP_USE_L5CLI }, > { "ssl_fc_has_sni", smp_fetch_ssl_fc_has_sni, 0, > NULL, SMP_T_BOOL, SMP_USE_L5CLI }, Regards Alex
