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
>
> /* 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;