Re: [PATCH 1/1] MINOR: sample: Add sample fetches for enhanced observability for TLS ClientHello

2025-01-09 Thread William Lallemand
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_extinteger
>  req.ssl_ver   integer
>  req_ssl_ver   integer
> +req.ssl_cipherlistbinary
> +req.ssl_sigalgs   binary
> +req.ssl_keyshare_groups   binary
> +req.ssl_supported_groups  binary
>  res.len   integer
>  res.payload(,)binary
>  res.payload_lv(,[,])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

Re: [PATCH 1/1] MINOR: sample: Add sample fetches for enhanced observability for TLS ClientHello

2025-01-06 Thread William Lallemand
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;