Hello Mariam, On Thu, Jan 02, 2025 at 06:16:28PM -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_ciphers: 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 > groups > - 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
Indeed this is really useful, It totally makes sense from a feature point of view, I think we can improve your patch a little bit before integration but I'd be happy to have these fetches! We already have some clienthello parsing when called with the SSL binds, some of the ssl_fc_* fetches use a capture system which stores some extensions in the struct ssl_capture. This is done in src/ssl_sock.c in the ssl_sock_parse_clienthello() function. It's done with an SSL context, but only because the callback system is providing it, the clienthello is also parsed manually in there. In the future we could maybe do the clienthello parsing in the same function rather than implementing 2 differents methods for TCP binds and SSL binds, but don't worry about that for now. What's interesting in there is how we store the data, and we should try to provide the same output and stay consistent with the naming: - req.ssl_ciphers has its SSL bind equivalent which is ssl_fc_cipherlist_bin, maybe we could call the new fetch req.ssl_cipherlist. Both provides the raw binary ciphers as stored in the clienthello, and is limited by the cipherlist len in the clienthello. - req.ssl_sigalgs has its ssl_fc_sigalgs_bin equivalent, so we are good with the naming, it also uses the sigalgs len and everything is raw, should be good! - req.ssl_keyshare_groups don't have any equivalent, data is processed and this is not the raw data, but only the group field in each KeyShareEntry. The output generated looks like ssl_fc_eclist_bin. - req.ssl_supported_groups have a SSL binds equivalent, which is ssl_fc_eclist_bin, maybe we can call your new fetch req.ssl_fc_eclist? Well that might be confusing and using the TLS extension name is probably better... I pushed your patch on our CI and there are some failures, you should try to clone the HAProxy repository on your github account in order to check if your reg-test is working correctly: https://github.com/haproxy/haproxy/actions/runs/12635483567/job/35205484779 > diff --git a/doc/configuration.txt b/doc/configuration.txt > index 76b622bce..c1ec84a67 100644 > --- a/doc/configuration.txt > +++ b/doc/configuration.txt > [...] > +req.ssl_ssl_sigalgs binary ^ typo there > + 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. I think you should mention the SSL bind equivalent keyword for each fetch, that would help users to find how to do it. > 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. > /* 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..00e5c2f23 > --- /dev/null > +++ b/reg-tests/checks/tcp-check-client-hello.vtc >From the failure I'm seeing on the CI, the reg-tests lacks a check on the SSL >feature, it also should not be started on older OpenSSL (1.0.2), and does not seem to pass on wolfssl, libressl. 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. > @@ -0,0 +1,79 @@ > +varnishtest "Health checks: test enhanced observability of TLS ClientHello" > +feature ignore_unknown_macro > + feature cmd "$HAPROXY_PROGRAM -cc 'feature(OPENSSL) && !ssllib_name_startswith(wolfSSL) && !ssllib_name_startswith(LibreSSL) && openssl_version_atleast(1.1.1)'" I think something like this could probably be enough to fix most of the CI problems. > +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}" > + > [...] > > +/* 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; > + } In my opinion 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 > + smp->data.type = SMP_T_BIN; > + smp->data.u.str.area = (char *)&smp->data.u.str.bin; > + 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; > +} Otherwise the implementation seems good to me, I'll check in the future if we could use the same parsing functions for the TCP bind and the SSL ones, but that's not urgent for now. Regards, -- William Lallemand
