Hello Mariam,
On Thu, Jan 09, 2025 at 02:36:09AM -0600, Mariam John wrote:
> Subject: [PATCH 1/1] MINOR: sample: Add sample fetches for enhanced
> observability for TLS ClientHello
> Add new sample fetches to get the ciphers, supported groups, key shares and
> signature algorithms
> that the client supports during a TLS handshake as part of the contents of a
> TLS ClientHello.
> Currently we can get the following contents of the ClientHello message:
> SNI(req_ssl_sni) and
> TLS protocol version(req_ssl_ver). The following new sample fetches will be
> added to get the
> following contents of the ClientHello message exchanged during the TLS
> handshake (supported by
> the client):
> - req.ssl_cipherlist: Returns the binary form of the list of symmetric cipher
> options
> - req.ssl_sigalgs: Returns the binary form of the list of signature algorithms
> - req.ssl_keyshare_groups: Return the binary format of the list of key share
> group
> - req.ssl_supported_groups: Returns the binary form of the list of supported
> groups used in key exchange
>
> This added functionality would allow routing with fine granularity pending
> the capabilities the client
> indicates in the ClientHello. For example, this would allow the ability to
> enable TLS passthrough or
> TLS termination based on the supported groups detected in the ClientHello
> message. Another usage is to
> take client key shares into consideration when deciding which of the client
> supported groups should be
> used for groups considered to have 'equal security level' as well as enabling
> fine grain selection of
> certificate types(beyond the RSA vs ECC distinction). All of that is relevant
> in the context of rapidly
> upcoming PQC operation modes.
>
> Fixes: #2532
That's better regarding the CI, but I think you missed my inline comments in my
previous mail!
See below for my inline comments:
> ---
> doc/configuration.txt | 61 ++
> include/haproxy/buf-t.h | 2 +
> reg-tests/checks/tcp-check-client-hello.vtc | 84 +++
> src/payload.c | 629 +++++++++++++++++++-
> 4 files changed, 775 insertions(+), 1 deletion(-)
> create mode 100644 reg-tests/checks/tcp-check-client-hello.vtc
>
> diff --git a/doc/configuration.txt b/doc/configuration.txt
> index 76b622bce..6d5ea4a7d 100644
> --- a/doc/configuration.txt
> +++ b/doc/configuration.txt
> @@ -25030,6 +25030,10 @@ req_ssl_sni
> string
> req.ssl_st_ext integer
> req.ssl_ver integer
> req_ssl_ver integer
> +req.ssl_cipherlist binary
> +req.ssl_sigalgs binary
> +req.ssl_keyshare_groups binary
> +req.ssl_supported_groups binary
> res.len integer
> res.payload(<offset>,<length>) binary
> res.payload_lv(<offset1>,<length>[,<offset2>]) binary
> @@ -25234,6 +25238,63 @@ req_ssl_sni : string (deprecated)
> use_backend bk_allow if { req.ssl_sni -f allowed_sites }
> default_backend bk_sorry_page
>
> +req.ssl_cipherlist binary
> + Returns the binary form of the list of symmetric cipher options supported
> by
> + the client as reported in the contents of a TLS ClientHello. Note that this
> + only applies to raw contents found in the request buffer and not to
> contents
> + deciphered via an SSL data layer, so this will not work with "bind" lines
> + having the "ssl" option.
> +
I think you should mention the SSL bind equivalent keyword for each fetch when
it exists, that would help users to find
how to do it, when they use the "ssl" option.
> + Examples :
> + # Wait for a client hello for at most 5 seconds
> + tcp-request inspect-delay 5s
> + tcp-request content accept if { req.ssl_hello_type 1 }
> + use-server fe3 if { req.ssl_cipherlist,be2hex(:,2),lower -m sub
> 1302:009f }
> + server fe3 ${htst_fe3_addr}:${htst_fe3_port}
> +
> +req.ssl_ssl_sigalgs binary
Typo there, "ssl_ssl".
> + Returns the binary form of the list of signature algorithms supported by
> the
> + client as reported in the TLS ClientHello. This is available as a client
> hello
> + extension. Note that this only applies to raw contents found in the request
> + buffer and not to contents deciphered via an SSL data layer, so this will
> not
> + work with "bind" lines having the "ssl" option.
> +
> + Examples :
> + # Wait for a client hello for at most 5 seconds
> + tcp-request inspect-delay 5s
> + tcp-request content accept if { req.ssl_hello_type 1 }
> + use-server fe4 if { req.ssl_sigalgs,be2hex(:,2),lower -m sub 0403:0805 }
> + server fe4 ${htst_fe4_addr}:${htst_fe4_port}
> +
> [...]
> diff --git a/include/haproxy/buf-t.h b/include/haproxy/buf-t.h
> index 5c59b0aaf..59472da84 100644
> --- a/include/haproxy/buf-t.h
> +++ b/include/haproxy/buf-t.h
> @@ -31,11 +31,13 @@
> #include <haproxy/api-t.h>
>
> /* Structure defining a buffer's head */
> +#define buffer_bin_size 16 /* Space to hold up to 8 code points, e.g
> for key share groups */
> struct buffer {
> size_t size; /* buffer size in bytes */
> char *area; /* points to <size> bytes */
> size_t data; /* amount of data after head including
> wrapping */
> size_t head; /* start offset of remaining data relative
> to area */
> + uint8_t bin[buffer_bin_size]; /* space to hold binary data, used to
> collect non-consecutive data */
> };
>
I'd rather not change this structure, this is a generic API which is used
everywhere, it could have consequences from a
perfomance point of view on other parts of the code. You should be able to
store everything in the area buffer, which is
big enough, or just use an intermediate buffer. You probably don't need it, see
my comment on get_trash_chunk at the end
of the file.
> /* A buffer may be in 3 different states :
> diff --git a/reg-tests/checks/tcp-check-client-hello.vtc
> b/reg-tests/checks/tcp-check-client-hello.vtc
> new file mode 100644
> index 000000000..56c94b543
> --- /dev/null
> +++ b/reg-tests/checks/tcp-check-client-hello.vtc
> @@ -0,0 +1,84 @@
> +#REQUIRE_OPTION=OPENSSL
> +#REGTEST_TYPE=slow
> +#EXCLUDE_TARGETS=osx,generic
> +
> +varnishtest "Health checks: test enhanced observability of TLS ClientHello"
> +feature cmd "$HAPROXY_PROGRAM -cc 'feature(OPENSSL) &&
> ssllib_name_startswith(OpenSSL) && openssl_version_atleast(1.1.1)'"
Something like that would be better to support AWS-LC:
feature cmd "$HAPROXY_PROGRAM -cc 'feature(OPENSSL) &&
!ssllib_name_startswith(wolfSSL) && !ssllib_name_startswith(LibreSSL) &&
openssl_version_atleast(1.1.1)'"
That's better than the previous patch, however on AWS-LC it fails because of
'tune.ssl.default-dh-param', but you
probably don't need this keyword if you don't try to use older DH ciphers. We
really try to make an effort with AWS-LC
so all reg-tests are working with this library.
> +feature ignore_unknown_macro
> +
> +syslog S_ok -level notice {
> + recv
> + expect ~ "[^:\\[ ]\\[${h1_pid}\\]: Health check for server be[0-9]+/srv
> succeeded, reason: Layer6 check passed.+check duration: [[:digit:]]+ms,
> status: 1/1 UP."
> + recv
> + expect ~ "[^:\\[ ]\\[${h1_pid}\\]: Health check for server be[0-9]+/srv
> succeeded, reason: Layer6 check passed.+check duration: [[:digit:]]+ms,
> status: 1/1 UP."
> + recv
> + expect ~ "[^:\\[ ]\\[${h1_pid}\\]: Health check for server be[0-9]+/srv
> succeeded, reason: Layer6 check passed.+check duration: [[:digit:]]+ms,
> status: 1/1 UP."
> +} -start
> +
> +haproxy htst -conf {
> + global
> + tune.ssl.default-dh-param 2048
> + log 127.0.0.1:514 local2
You probably don't need the dh-param as well as the log line.
> + ssl-default-bind-options ssl-min-ver TLSv1.2 ssl-max-ver TLSv1.3
> +
> + defaults
> + log global
> + option tcplog
> + timeout client "${HAPROXY_TEST_TIMEOUT-5s}"
> + timeout server "${HAPROXY_TEST_TIMEOUT-5s}"
> + timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
> +
> [...]
> diff --git a/src/payload.c b/src/payload.c
> index 6a536d719..9762deab7 100644
> --- a/src/payload.c
> +++ b/src/payload.c
> @@ -522,6 +522,629 @@ smp_fetch_req_ssl_ver(const struct arg *args, struct
> sample *smp, const char *kw
> return 0;
> }
> [...]
> +
> +/* Extract the supported group that may be presented in a TLS client hello
> handshake
> + * message. The format of the message is the following (cf RFC5246 +
> RFC6066) :
> + * TLS frame :
> + * - uint8 type = 0x16 (Handshake)
> + * - uint16 version >= 0x0301 (TLSv1)
> + * - uint16 length (frame length)
> + * - TLS handshake :
> + * - uint8 msg_type = 0x01 (ClientHello)
> + * - uint24 length (handshake message
> length)
> + * - ClientHello :
> + * - uint16 client_version >= 0x0301 (TLSv1)
> + * - uint8 Random[32] (4 first ones are timestamp)
> + * - SessionID :
> + * - uint8 session_id_len (0..32) (SessionID len in
> bytes)
> + * - uint8 session_id[session_id_len]
> + * - CipherSuite :
> + * - uint16 cipher_len >= 2 (Cipher length in
> bytes)
> + * - uint16 ciphers[cipher_len/2]
> + * - CompressionMethod :
> + * - uint8 compression_len >= 1 (# of supported
> methods)
> + * - uint8 compression_methods[compression_len]
> + * - optional client_extension_len (in bytes)
> + * - optional sequence of ClientHelloExtensions (as many bytes as
> above):
> + * - uint16 extension_type = 0 for server_name
> + * - uint16 extension_len
> + * - opaque extension_data[extension_len]
> + * - uint16 server_name_list_len (# of bytes here)
> + * - opaque server_names[server_name_list_len bytes]
> + * - uint8 name_type = 0 for host_name
> + * - uint16 name_len
> + * - opaque hostname[name_len bytes]
> + */
> +static int
> +smp_fetch_ssl_supported_groups(const struct arg *args, struct sample *smp,
> const char *kw, void *private)
> +{
> + int hs_len, ext_len, bleft;
> + struct channel *chn;
> + unsigned char *data;
> +
> + if (!smp->strm)
> + goto not_ssl_hello;
> +
> + /* meaningless for HTX buffers */
> + if (IS_HTX_STRM(smp->strm))
> + goto not_ssl_hello;
> +
> + chn = ((smp->opt & SMP_OPT_DIR) == SMP_OPT_DIR_RES) ? &smp->strm->res
> : &smp->strm->req;
> + bleft = ci_data(chn);
> + data = (unsigned char *)ci_head(chn);
> +
> + /* Check for SSL/TLS Handshake */
> + if (!bleft)
> + goto too_short;
> + if (*data != 0x16)
> + goto not_ssl_hello;
> +
> + /* Check for SSLv3 or later (SSL version >= 3.0) in the record layer*/
> + if (bleft < 3)
> + goto too_short;
> + if (data[1] < 0x03)
> + goto not_ssl_hello;
> +
> + if (bleft < 5)
> + goto too_short;
> + hs_len = (data[3] << 8) + data[4];
> + if (hs_len < 1 + 3 + 2 + 32 + 1 + 2 + 2 + 1 + 1 + 2 + 2)
> + goto not_ssl_hello; /* too short to have an extension */
> +
> + data += 5; /* enter TLS handshake */
> + bleft -= 5;
> + /* Check for a complete client hello starting at <data> */
> + if (bleft < 1)
> + goto too_short;
> + if (data[0] != 0x01) /* msg_type = Client Hello */
> + goto not_ssl_hello;
> +
> + /* Check the Hello's length */
> + if (bleft < 4)
> + goto too_short;
> + hs_len = (data[1] << 16) + (data[2] << 8) + data[3];
> + if (hs_len < 2 + 32 + 1 + 2 + 2 + 1 + 1 + 2 + 2)
> + goto not_ssl_hello; /* too short to have an extension */
> +
> + /* We want the full handshake here */
> + if (bleft < hs_len)
> + goto too_short;
> +
> + data += 4;
> + /* Start of the ClientHello message */
> + if (data[0] < 0x03 || data[1] < 0x01) /* TLSv1 minimum */
> + goto not_ssl_hello;
> +
> + ext_len = data[34]; /* session_id_len */
> + if (ext_len > 32 || ext_len > (hs_len - 35)) /* check for correct
> session_id len */
> + goto not_ssl_hello;
> +
> + /* Jump to cipher suite */
> + hs_len -= 35 + ext_len;
> + data += 35 + ext_len;
> +
> + if (hs_len < 4 || /* minimum one cipher
> */
> + (ext_len = (data[0] << 8) + data[1]) < 2 || /* minimum 2 bytes
> for a cipher */
> + ext_len > hs_len)
> + goto not_ssl_hello;
> +
> + /* Jump to the compression methods */
> + hs_len -= 2 + ext_len;
> + data += 2 + ext_len;
> +
> + if (hs_len < 2 || /* minimum one compression
> method */
> + data[0] < 1 || data[0] > hs_len) /* minimum 1 bytes for a
> method */
> + goto not_ssl_hello;
> +
> +
> + /* Jump to the extensions */
> + hs_len -= 1 + data[0];
> + data += 1 + data[0];
> +
> + if (hs_len < 2 || /* minimum one extension list
> length */
> + (ext_len = (data[0] << 8) + data[1]) > hs_len - 2) /* list too
> long */
> + goto not_ssl_hello;
> +
I feel like we are doing this for every smp_fetch_ssl_* fetch in payload.c, we
will probably copy/paste bugs for every
new fetch, so you should probably have a function which is doing the generic
clienthello parsing that you can call in
every fetch, and then only the extension parsing would be done manually in the
fetch.
> + hs_len = ext_len; /* limit ourselves to the extension length */
> + data += 2; /* Now 'data' points to the first content byte of an
> extension */
> +
> + while (hs_len >= 4) {
> +
> + int ext_type, grp_len;
> +
> + ext_type = (data[0] << 8) + data[1]; /* Extension type */
> + ext_len = (data[2] << 8) + data[3]; /* Extension length */
> +
> + if (ext_len > hs_len - 4) /* Extension too long */
> + goto not_ssl_hello;
> +
> + if (ext_type == 10) { /* Supported groups extension type ID is
> 10dec */
> + if (ext_len < 2) /* need at least one entry of 2 bytes in
> the list length */
> + goto not_ssl_hello;
> +
> + grp_len = (data[4] << 8) + data[5]; /* Supported group list
> length */
> + if (grp_len < 2 || grp_len > hs_len - 6)
> + goto not_ssl_hello; /* at least 2 bytes per
> supported group */
> +
> + smp->data.type = SMP_T_BIN;
> + smp->data.u.str.area = (char *)data + 6;
> + smp->data.u.str.data = grp_len;
> + smp->flags = SMP_F_VOL_SESS | SMP_F_CONST;
> +
> + return 1;
> +
> + }
> + hs_len -= 4 + ext_len;
> + data += 4 + ext_len;
> + }
> + /* supported groups not found */
> + goto not_ssl_hello;
> +
> + too_short:
> + smp->flags = SMP_F_MAY_CHANGE;
> +
> + not_ssl_hello:
> +
> + return 0;
> +}
> +
> [...]
> +
> +/* Extract the key shares that may be presented in a TLS client hello
> handshake message.
> + * The format of the message is the following (cf RFC5246 + RFC6066) :
> + * TLS frame :
> + * - uint8 type = 0x16 (Handshake)
> + * - uint16 version >= 0x0301 (TLSv1)
> + * - uint16 length (frame length)
> + * - TLS handshake :
> + * - uint8 msg_type = 0x01 (ClientHello)
> + * - uint24 length (handshake message
> length)
> + * - ClientHello :
> + * - uint16 client_version >= 0x0301 (TLSv1)
> + * - uint8 Random[32] (4 first ones are timestamp)
> + * - SessionID :
> + * - uint8 session_id_len (0..32) (SessionID len in
> bytes)
> + * - uint8 session_id[session_id_len]
> + * - CipherSuite :
> + * - uint16 cipher_len >= 2 (Cipher length in
> bytes)
> + * - uint16 ciphers[cipher_len/2]
> + * - CompressionMethod :
> + * - uint8 compression_len >= 1 (# of supported
> methods)
> + * - uint8 compression_methods[compression_len]
> + * - optional client_extension_len (in bytes)
> + * - optional sequence of ClientHelloExtensions (as many bytes as
> above):
> + * - uint16 extension_type = 0 for server_name
> + * - uint16 extension_len
> + * - opaque extension_data[extension_len]
> + * - uint16 server_name_list_len (# of bytes here)
> + * - opaque server_names[server_name_list_len bytes]
> + * - uint8 name_type = 0 for host_name
> + * - uint16 name_len
> + * - opaque hostname[name_len bytes]
> + */
> +static int
> +smp_fetch_ssl_keyshare_groups(const struct arg *args, struct sample *smp,
> const char *kw, void *private)
> +{
> + int hs_len, ext_len, bleft, readPosition, numberOfKeyshares;
> + struct channel *chn;
> + unsigned char *data;
> + unsigned char *dataPointer;
> +
> + if (!smp->strm)
> + goto not_ssl_hello;
> +
> + /* meaningless for HTX buffers */
> + if (IS_HTX_STRM(smp->strm))
> + goto not_ssl_hello;
> +
> + chn = ((smp->opt & SMP_OPT_DIR) == SMP_OPT_DIR_RES) ? &smp->strm->res
> : &smp->strm->req;
> + bleft = ci_data(chn);
> + data = (unsigned char *)ci_head(chn);
> +
> + /* Check for SSL/TLS Handshake */
> + if (!bleft)
> + goto too_short;
> + if (*data != 0x16)
> + goto not_ssl_hello;
> +
> + /* Check for SSLv3 or later (SSL version >= 3.0) in the record layer*/
> + if (bleft < 3)
> + goto too_short;
> + if (data[1] < 0x03)
> + goto not_ssl_hello;
> +
> + if (bleft < 5)
> + goto too_short;
> + hs_len = (data[3] << 8) + data[4];
> + if (hs_len < 1 + 3 + 2 + 32 + 1 + 2 + 2 + 1 + 1 + 2 + 2)
> + goto not_ssl_hello; /* too short to have an extension */
> +
> + data += 5; /* enter TLS handshake */
> + bleft -= 5;
> +
> + /* Check for a complete client hello starting at <data> */
> + if (bleft < 1)
> + goto too_short;
> + if (data[0] != 0x01) /* msg_type = Client Hello */
> + goto not_ssl_hello;
> +
> + /* Check the Hello's length */
> + if (bleft < 4)
> + goto too_short;
> + hs_len = (data[1] << 16) + (data[2] << 8) + data[3];
> + if (hs_len < 2 + 32 + 1 + 2 + 2 + 1 + 1 + 2 + 2)
> + goto not_ssl_hello; /* too short to have an extension */
> +
> + /* We want the full handshake here */
> + if (bleft < hs_len)
> + goto too_short;
> +
> + data += 4;
> + /* Start of the ClientHello message */
> + if (data[0] < 0x03 || data[1] < 0x01) /* TLSv1 minimum */
> + goto not_ssl_hello;
> +
> + ext_len = data[34]; /* session_id_len */
> + if (ext_len > 32 || ext_len > (hs_len - 35)) /* check for correct
> session_id len */
> + goto not_ssl_hello;
> +
> + /* Jump to cipher suite */
> + hs_len -= 35 + ext_len;
> + data += 35 + ext_len;
> +
> + if (hs_len < 4 || /* minimum one cipher
> */
> + (ext_len = (data[0] << 8) + data[1]) < 2 || /* minimum 2 bytes
> for a cipher */
> + ext_len > hs_len)
> + goto not_ssl_hello;
> +
> + /* Jump to the compression methods */
> + hs_len -= 2 + ext_len;
> + data += 2 + ext_len;
> +
> + if (hs_len < 2 || /* minimum one compression
> method */
> + data[0] < 1 || data[0] > hs_len) /* minimum 1 bytes for a
> method */
> + goto not_ssl_hello;
> +
> + /* Jump to the extensions */
> + hs_len -= 1 + data[0];
> + data += 1 + data[0];
> +
> + if (hs_len < 2 || /* minimum one extension list
> length */
> + (ext_len = (data[0] << 8) + data[1]) > hs_len - 2) /* list too
> long */
> + goto not_ssl_hello;
> +
> + hs_len = ext_len; /* limit ourselves to the extension length */
> + data += 2; /* Now 'data' points to the first content byte of an
> extension */
> +
> + while (hs_len >= 4) {
> +
> + int ext_type, keyshare_len;
> +
> + ext_type = (data[0] << 8) + data[1]; /* Extension type */
> + ext_len = (data[2] << 8) + data[3]; /* Extension length */
> +
> + if (ext_len > hs_len - 4) /* Extension too long */
> + goto not_ssl_hello;
> +
> + if (ext_type == 51) { /* Keyshare extension type ID is 51dec */
> + if (ext_len < 2) /* need at least one entry of 2 bytes in
> the list length */
> + goto not_ssl_hello;
> +
> + keyshare_len = (data[4] << 8) + data[5]; /* Client keyshare length
> */
> + if (keyshare_len < 2 || keyshare_len > hs_len - 6)
> + goto not_ssl_hello; /* at least 2 bytes per keyshare */
> + dataPointer = data + 6; /* start of keyshare entries */
> + readPosition = 0;
> + numberOfKeyshares = 0;
> + while (readPosition < keyshare_len) {
> + /* Get the binary value of the keyshare group and
> move the offset to the end of the related keyshare */
> + memmove(&smp->data.u.str.bin[2*numberOfKeyshares],
> &dataPointer[readPosition], 2);
> + numberOfKeyshares++;
> + if (2*numberOfKeyshares > buffer_bin_size)
> + goto not_ssl_hello; /* not enough space to
> store all keyshare groups --> something is wrong */
> + readPosition += ((int)dataPointer[readPosition+2] <<
> 8) + (int)dataPointer[readPosition+3] + 4;
> + }
> + smp->data.type = SMP_T_BIN;
> + smp->data.u.str.area = (char *)&smp->data.u.str.bin;
You don't need this str.bin array, you can just do a get_trash_chunk(), copy
the data inside and assign it to
data.u.str like it's done in some of the fetches in src/ssl_sample.c for
example.
> + smp->data.u.str.data = 2*numberOfKeyshares;
> + smp->flags = SMP_F_VOLATILE | SMP_F_CONST;
> +
> + return 1;
> + }
> + hs_len -= 4 + ext_len;
> + data += 4 + ext_len;
> + }
> + /* keyshare groups not found */
> + goto not_ssl_hello;
> +
> + too_short:
> + smp->flags = SMP_F_MAY_CHANGE;
> + not_ssl_hello:
> + return 0;
> +}
--
William Lallemand
