Re: [PATCH 0/1] Add 4 new sample fetches to get information from ClientHello message
Hello Mariam,
On Wed, Jan 29, 2025 at 05:59:37AM -0600, Mariam John wrote:
> Hello William,
>
> Thank you for your feedback on this PR and apologies for the delay. I
> addressed all the comments from
> your last review.:
> - included the SSL bind equivalent for each fetch and fixed a typo in the
> name of a fetch.
> - Undid the changes to buf-t.h
> - Updated test to support AWS-LC
> - Added a new function to do generic clienthello parsing that you can be used
> in every fetch in payload.c
>
> Thanks,
> Mariam.
Thanks Mariam, I made comments inline, in addition to the following comments.
Note that there are a lot of spaces/tabs problems in your patch, you should try
enable a way to differenciate them in
your editor. We are using tabs for indentation, and spaces for alignment, you
could look for examples here
https://www.haproxy.org/coding-style.html
Try to do a `git log -p` of your patches before submitting, it should show you
the trailing spaces and tabs in red.
If not, you must enable 'color.ui = auto' in your git configuration.
I didn't mention all spaces/tabs problems in my review, but I could fix the
remaining in your next iteration if that's
fine with you.
The patch is becoming quite big because of the changes with your new parsing
function, You should split this in multiple
parts to be more readable, it's quite difficult to understand the changes
currently, because the chunks are mixed up in
the patch.
You could proceed in this order:
1. a patch with your new fetches like you've done previously, without the
smp_client_hello_parse()
2. a patch with your new reg-test
3. a patch which introduces the new smp_client_hello_parse() function and uses
it in every fetches
On Wed, Jan 29, 2025 at 05:59:38AM -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
> ---
> doc/configuration.txt | 66 ++
> reg-tests/checks/tcp-check-client-hello.vtc | 82 +++
> src/payload.c | 697 ++--
> 3 files changed, 479 insertions(+), 366 deletions(-)
> create mode 100644 reg-tests/checks/tcp-check-client-hello.vtc
>
> diff --git a/doc/configuration.txt b/doc/configuration.txt
> index da9d8b540..ab39da283 100644
> --- a/doc/configuration.txt
> +++ b/doc/configuration.txt
> [...]
> +
> +req.ssl_supported_groups binary
> + Returns the binary form of the list of supported groups supported by the
> client
> + as reported in the TLS ClientHello and used for key exchange which can
> include
> + both elliptic curve and non-EC key exchange. 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.
> + Refer to "ssl_fc_eclist_bin" hich is the SSL bind equivalent that can be
> used
^ small typo there "which"
> + when the "ssl" option is specified.
> +
> + 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_supported_groups, be2hex(:,2),lower -m sub
> 0017 }
> + server fe3 ${htst_fe3_addr}:${htst_fe3_port}
> +
> req.ssl_st_ext : integer
>Returns 0 if the clie
[PATCH 0/1] Add 4 new sample fetches to get information from ClientHello message
Hello William, Thank you for your feedback on this PR and apologies for the delay. I addressed all the comments from your last review.: - included the SSL bind equivalent for each fetch and fixed a typo in the name of a fetch. - Undid the changes to buf-t.h - Updated test to support AWS-LC - Added a new function to do generic clienthello parsing that you can be used in every fetch in payload.c Thanks, Mariam. Mariam John (1): MINOR: sample: Add sample fetches for enhanced observability for TLS ClientHello doc/configuration.txt | 66 ++ reg-tests/checks/tcp-check-client-hello.vtc | 82 +++ src/payload.c | 697 ++-- 3 files changed, 479 insertions(+), 366 deletions(-) create mode 100644 reg-tests/checks/tcp-check-client-hello.vtc -- 2.39.3 (Apple Git-145)
[PATCH 0/1] Add 4 new sample fetches to get information from ClientHello message
Hello William,
Thank you for the quick review and feedback, appreciate it. I wasn't sure how
to send this patch
as a reply to the original patch I submitted but I hope I made the changes you
recommended. I made
the following changes:
- renamed `req.ssl_ciphers` to `req.ssl_cipherlist`
- I was not sure if any changes needed to be made to `req.ssl_keyshare_groups`.
This is usually a
subset of the list of curves available in the supported_groups and represents
the specific curve
chosen by the client from its list of supported groups to use for the current
key exchange. For
example, if the supported group = {0017:0018} in binary form {secp256r1,
secp384r1}, then the
keyshare group can be 0017.
- req.ssl_supported_groups & req.ssl_sigalgs - no changes
- Updated the test to fix all the errors. I had tested it before submitting the
original patch which
worked in my env but failed for other SSL flavors and platforms. Made the
required changes there.
This patch includes functionality to add sample fetches to get ciphers,
supported groups,
key shares and sigalgs from the ClientHello message. This will help enhance
observability and
help direct traffic to different backends based on different algorithms
supported by the client
for key exchange, for example.Included in this patch are the new sample fetch
implementation,
doc changes and regression test.
Thanks,
Mariam.
Mariam John (1):
MINOR: sample: Add sample fetches for enhanced observability for TLS
ClientHello
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
--
2.39.3 (Apple Git-145)
[PATCH 0/1] Add 4 new sample fetches to get information from ClientHello message
Hello William,
Thank you for the quick review and feedback, appreciate it. I wasn't sure how
to send this patch
as a reply to the original patch I submitted but I hope I made the changes you
recommended. I made
the following changes:
- renamed `req.ssl_ciphers` to `req.ssl_cipherlist`
- I was not sure if any changes needed to be made to `req.ssl_keyshare_groups`.
This is usually a
subset of the list of curves available in the supported_groups and represents
the specific curve
chosen by the client from its list of supported groups to use for the current
key exchange. For
example, if the supported group = {0017:0018} in binary form {secp256r1,
secp384r1}, then the
keyshare group can be 0017.
- req.ssl_supported_groups & req.ssl_sigalgs - no changes
- Updated the test to fix all the errors. I had tested it before submitting the
original patch which
worked in my env but failed for other SSL flavors and platforms. Made the
required changes there.
This patch includes functionality to add sample fetches to get ciphers,
supported groups,
key shares and sigalgs from the ClientHello message. This will help enhance
observability and
help direct traffic to different backends based on different algorithms
supported by the client
for key exchange, for example.Included in this patch are the new sample fetch
implementation,
doc changes and regression test.
Thanks,
Mariam.
Mariam John (1):
MINOR: sample: Add sample fetches for enhanced observability for TLS
ClientHello
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
--
2.39.3 (Apple Git-145)
[PATCH 0/1] Add 4 new sample fetches to get information from ClientHello message
This patch includes functionality to add sample fetches to get ciphers, supported groups, key shares and sigalgs from the ClientHello message. This will help enhance observability and help direct traffic to different backends based on different algorithms supported by the client for key exchange, for example.Included in this patch are the new sample fetch implementation, doc changes and regression test. Mariam John (1): MINOR: sample: Add sample fetches for enhanced observability for TLS ClientHello doc/configuration.txt | 61 ++ include/haproxy/buf-t.h | 2 + reg-tests/checks/tcp-check-client-hello.vtc | 79 +++ src/payload.c | 629 +++- 4 files changed, 770 insertions(+), 1 deletion(-) create mode 100644 reg-tests/checks/tcp-check-client-hello.vtc -- 2.39.3 (Apple Git-145)
