Hi Marcin, I'm finally back to your patch set! Overall that looks fine, but I have some comments, mostly cosmetic.
> From b3a254b41411f22307a622250a6e95ac39fefee8 Mon Sep 17 00:00:00 2001 > From: Marcin Deranek <[email protected]> > Date: Mon, 12 Jul 2021 14:16:55 +0200 > Subject: [PATCH 1/5] MEDIUM: ssl: Capture more info from Client Hello (...) > diff --git a/include/haproxy/ssl_sock-t.h b/include/haproxy/ssl_sock-t.h > index 5acedcfc5..4b993894a 100644 > --- a/include/haproxy/ssl_sock-t.h > +++ b/include/haproxy/ssl_sock-t.h > @@ -202,8 +202,16 @@ struct ssl_sock_msg_callback { > /* This memory pool is used for capturing clienthello parameters. */ > struct ssl_capture { > unsigned long long int xxh64; > - unsigned char ciphersuite_len; > - char ciphersuite[VAR_ARRAY]; > + unsigned short int protocol_version; > + unsigned short int ciphersuite_len; > + unsigned short int extensions_len; > + unsigned short int ec_len; No need to mention "int" after "short" or "long", that's implicit. I guess you did it because of the "unsigned long long int" above but that is not needed. Since 2.4 we have shorter type names like "ushort", "uint", "uchar" etc that we're progressively trying to standardize the new code to, but they haven't slipped into older code yet. This could make sense here since the whole structure is being replaced. But that's not an obligation :-) > diff --git a/src/ssl_sock.c b/src/ssl_sock.c > index ba61243ac..2965a1446 100644 > --- a/src/ssl_sock.c > +++ b/src/ssl_sock.c > @@ -1652,7 +1652,16 @@ static void ssl_sock_parse_clienthello(struct > connection *conn, int write_p, int > struct ssl_capture *capture; > unsigned char *msg; > unsigned char *end; > - size_t rec_len; > + unsigned char *extensions_end; > + unsigned char *ec_start = NULL; > + unsigned char *ec_formats_start = NULL; > + unsigned char *list_end; > + unsigned short int protocol_version; > + unsigned short int extension_id; > + unsigned short int ec_len = 0; Same here for the type names. > @@ -1747,10 +1763,106 @@ static void ssl_sock_parse_clienthello(struct > connection *conn, int write_p, int > capture->xxh64 = XXH64(msg, rec_len, 0); > > /* Capture the ciphersuite. */ > - capture->ciphersuite_len = (global_ssl.capture_cipherlist < rec_len) ? > - global_ssl.capture_cipherlist : rec_len; > - memcpy(capture->ciphersuite, msg, capture->ciphersuite_len); > + capture->ciphersuite_len = MIN(global_ssl.capture_cipherlist, rec_len); > + capture->ciphersuite_offset = 0; > + memcpy(capture->data, msg, capture->ciphersuite_len); > + msg += rec_len; > + offset += capture->ciphersuite_len; > + > + /* Initialize other data */ > + capture->protocol_version = protocol_version; > + capture->extensions_len = 0; > + capture->extensions_offset = 0; > + capture->ec_len = 0; > + capture->ec_offset = 0; > + capture->ec_formats_len = 0; > + capture->ec_formats_offset = 0; Given the number of fields to preset to zero, you should instead replace the previous pool_alloc() call with a pool_zalloc(), it would be more future-proof and safer against the risk of accidentally missing one such field when more are added. > + /* Expect 2 bytes extension id + 2 bytes extension size */ > + msg += 2 + 2; > + if (msg + rec_len > extensions_end || msg + rec_len < msg) > + goto store_capture; > + if (extension_id == 0x000a) { > + /* Elliptic Curves */ Could you put a link here to the registry where you found this definition ? I guess it comes from an RFC or anything, but it does help when some extensions are needed or when dealing with interoperability issues, to figure if any other values need to be considered. > + else if (extension_id == 0x000b) { > + /* Elliptic Curves Point Formats */ Ditto. > + if (ec_start) { > + rec_len = MIN(global_ssl.capture_cipherlist - offset, ec_len); I'm not fond of using MIN() here as you're dealing with two different types (one signed int due to the subtract, and an unsigned short). It actually does the right thing but it's fragile. What about this: rec_len = ec_len; if (offset + rec_len > global_ssl.capture_cipherlist) rec_len = global_ssl.capture_cipherlist - offset; > + memcpy(capture->data + offset, ec_start, rec_len); > + capture->ec_offset = offset; > + capture->ec_len = rec_len; > + offset += rec_len; > + } > + if (ec_formats_start) { > + rec_len = MIN(global_ssl.capture_cipherlist - offset, > ec_formats_len); Same here. > +ssl_fc_cipherlist_bin([<exclude_grease>]) : binary > + Returns the binary form of the client hello cipher list. Setting > + <exlude_grease> to 1 will exclude GREASE (RFC8701) values from the output. I find that placing a lone boolean "1" here in the argument will complicate future extensions. I'm not necessarily fond of placing bare optional words either, but it's the fact that we're forcing a field for just one option, with a value that's not obvious especially if we add others in the future, when it could look like: ssl_fc_cipherlist_bin(0,,,1,2) Maybe a tiny variant which will leave the code as-is could be to just rename the argument to "filter_option" and mention in the doc that "filter_option" designates some optional filtering to perform on the input before processing it, and that for now only two values are permitted: - 0 : take the full list, including GREASE if any, this is the default - 1 : take the full list, excluding GREASE if any This way, depending on evolutions we may decide to either add new discrete values or make some of them combinations. However it will be important that the config parser rejects unsupported values. This should be the same for all of those which support this exclude_grease argument. > + The maximum returned value length is limited by the shared capture buffer > + size controlled by "tune.ssl.capture-cipherlist-size" setting. > > -ssl_fc_cipherlist_hex : string > + Example: > + http-request set-header X-SSL-JA3 %[ssl_fc_protocol_hello_id],\ > + %[ssl_fc_cipherlist_bin(1),be2dec(-,2)],\ > + %[ssl_fc_extlist_bin(1),be2dec(-,2)],\ > + %[ssl_fc_eclist_bin(1),be2dec(-,2)],\ > + %[ssl_fc_ecformats_bin,be2dec(-,1)] I thought we finally had an fmt-based "set-var" action, but looking through the doc, it seems that I was wrong. I've probably got too much used to rewriting it in todo lists :-( It would have made this slightly cleaner by not relying on a header in my opinion. But that's not a big deal. > + acl is_malware req.fhdr(x-ssl-ja3),digest(md5),hex,\ > + map(/path/to/file/with/malware-ja3.lst) -m found Actually you're exactly reimplementing an ACL from a map :-) This is equivalent to: acl is_malware req.fhdr(x-ssl-ja3),digest(md5),hex -f /path/to/file/with/malware-ja3.lst Be careful, there are a few more occurrences in your other examples. > + http-request set-header X-Malware True if is_malware > + http-request set-header X-Malware False if !is_malware This use case is interesting because it shows a limit with our use of maps and ACLs for which there is no "found" output type, and thus which does not allows to use Tim's "iif()" converter. We could have imagined something around: http-request set-header X-Malware %[req.fhdr(x-ssl-ja3),digest(md5),hex,map(/path/to/file/with/malware-ja3.lst),iif(True,False)] But that will not work there unless the file is a map and contains "1" as the value on each line. With that said it should work the opposite way (making 1 the default value and using others not to match): http-request set-header X-Malware %[req.fhdr(x-ssl-ja3),digest(md5),hex,map(/path/to/file/with/malware-ja3.lst,1),iif(False,True)] It can be fine to squeeze one more rule but it is far from being obvious, thus I think we ought to keep your version instead :-) > +ssl_fc_cipherlist_str([<exclude_grease>]) : string > + Returns the decoded text form of the client hello cipher list. Setting > + <exlude_grease> to 1 will exclude GREASE (RFC8701) values from the output. > + The maximum returned value length is limited by the shared capture buffer > + size controlled by "tune.ssl.capture-cipherlist-size" setting. Note that > + this sample-fetch is only available with OpenSSL >= 1.0.2. OK > + If the function > + is not enabled, this sample-fetch returns the hash like > + "ssl_fc_cipherlist_xxh". Hmm let's avoid this. Better simply fail to parse it or kindly mention "ssl_fc_cipherlist_str() requires OpenSSL >= 1.0.2" and be done with it. Otherwise it will result in users mistakenly using it for their hash, and getting breakage when upgrading. It looks convenient at first but it is not because we must tell users how not to shoot themselves in the foot. > ssl_fc_cipherlist_xxh : integer > - Returns a xxh64 of the cipher list. This hash can be return only is the > value > + Returns a xxh64 of the cipher list. This hash can return only if the value > "tune.ssl.capture-cipherlist-size" is set greater than 0, however the hash > - take in account all the data of the cipher list. > + take into account all the data of the cipher list. Just curious, why the full one requires OpenSSL >= 1.0.2 and not this one ? Maybe it's just the conversion to strings that only exists since 1.0.2 ? > diff --git a/include/haproxy/tools.h b/include/haproxy/tools.h > index e376a716e..3ccbfd356 100644 > --- a/include/haproxy/tools.h > +++ b/include/haproxy/tools.h > @@ -973,6 +973,8 @@ static inline unsigned long long rdtsc() > struct list; > int list_append_word(struct list *li, const char *str, char **err); > > +void exclude_grease(char *input, int len, struct buffer *output); I'm fine with having such a function in ssl_sample.c or something like this, but it should not be in tools with this name for the reason that "GREASE" is a generic term that is here applied to TLS, and what it does precisely is to deal with this. Maybe it's even just one aspect of greasing TLS fields, I don't know. Thus generally speaking please make sure it's as precise as possible (e.g. if RFC8701 mentions other parts supporting GREASE outside of the fields supported here), and move it in the SSL parts. > diff --git a/src/ssl_sample.c b/src/ssl_sample.c > index a21dcc16f..1eecf61bf 100644 > --- a/src/ssl_sample.c > +++ b/src/ssl_sample.c > @@ -1130,6 +1130,7 @@ smp_fetch_ssl_fc_sni(const struct arg *args, struct > sample *smp, const char *kw, > static int > smp_fetch_ssl_fc_cl_bin(const struct arg *args, struct sample *smp, const > char *kw, void *private) > { > + struct buffer *smp_trash; > struct connection *conn; > struct ssl_capture *capture; > SSL *ssl; > @@ -1143,10 +1144,20 @@ smp_fetch_ssl_fc_cl_bin(const struct arg *args, > struct sample *smp, const char * > if (!capture) > return 0; > > - smp->flags = SMP_F_VOL_TEST | SMP_F_CONST; > + if (args[0].data.sint) { You're adding arguments to this sample fetch, please add a comment above the function to enumerate them. As a general rule, whenever a sample fetch function or a converter makes use of arguments, please always enumerate them in comments before the function and indicate their various validity domains. When troubleshooting it's a real pain to have no idea what the function expects. And please also recall it in front of the parsing function (for the same reasons, especially as it helps seeing that an argument is not initialized or any such issue). > +static int > +smp_fetch_ssl_fc_ext_bin(const struct arg *args, struct sample *smp, const > char *kw, void *private) > +{ > + struct buffer *smp_trash; > + struct connection *conn; > + struct ssl_capture *capture; > + SSL *ssl; > + > + conn = objt_conn(smp->sess->origin); > + ssl = ssl_sock_get_ssl_object(conn); > + if (!ssl) > + return 0; > + > + capture = SSL_get_ex_data(ssl, ssl_capture_ptr_index); > + if (!capture) > + return 0; > + > + if (args[0].data.sint) { Same for this one. > + smp_trash = get_trash_chunk(); > + exclude_grease(capture->data + capture->extensions_offset, > capture->extensions_len, smp_trash); > + smp->data.u.str.area = smp_trash->area; > + smp->data.u.str.data = smp_trash->data; > + smp->flags = SMP_F_VOL_SESS; > + } > + else { > + smp->data.u.str.area = capture->data + > capture->extensions_offset; > + smp->data.u.str.data = capture->extensions_len; > + smp->flags = SMP_F_VOL_TEST | SMP_F_CONST; > + } > + > + smp->data.type = SMP_T_BIN; > + return 1; > +} > + > +static int > +smp_fetch_ssl_fc_ecl_bin(const struct arg *args, struct sample *smp, const > char *kw, void *private) And this one. > From 97077615b4eda025143dd00f48c18c962695986f Mon Sep 17 00:00:00 2001 > From: Marcin Deranek <[email protected]> > Date: Tue, 13 Jul 2021 14:05:24 +0200 > Subject: [PATCH 3/5] MINOR: sample: Add be2dec converter > > Add be2dec converter which allows to build JA3 compatible TLS > fingerprints by converting big-endian binary data into string > separated unsigned integers eg. > > http-request set-header X-SSL-JA3 %[ssl_fc_protocol_hello_id],\ > %[ssl_fc_cipherlist_bin(1),be2dec(-,2)],\ > %[ssl_fc_extlist_bin(1),be2dec(-,2)],\ > %[ssl_fc_eclist_bin(1),be2dec(-,2)],\ > %[ssl_fc_ecformats_bin,be2dec(-,1)] > --- > doc/configuration.txt | 12 +++++++ > reg-tests/converter/be2dec.vtc | 56 +++++++++++++++++++++++++++++++++ > src/sample.c | 57 ++++++++++++++++++++++++++++++++++ > 3 files changed, 125 insertions(+) > create mode 100644 reg-tests/converter/be2dec.vtc > > diff --git a/doc/configuration.txt b/doc/configuration.txt > index 450186ecb..babd33a12 100644 > --- a/doc/configuration.txt > +++ b/doc/configuration.txt > @@ -16092,6 +16092,18 @@ base64 > an SSL ID can be copied in a header). For base64url("URL and Filename > Safe Alphabet" (RFC 4648)) variant see "ub64enc". > > +be2dec(<separator>,<chunk_size>,[<truncate>]) > + Converts a binary input sample to a string containing an unsigned integer > + number per <chunk_size> input bytes. <separator> is put every <chunk_size> > + binary input bytes if specified. <truncate> flag indicates whatever binary > + input is truncated at <chunk_size> boundaries. <chunk_size> maximum value > is > + limited by the size of long long int (8 bytes). > + > + Example: > + bin(01020304050607),be2dec(:,2) # 258:772:1286:7 > + bin(01020304050607),be2dec(-,2,1) # 258-772-1286 > + bin(01020304050607),be2dec(,2,1) # 2587721286 > + This looks pretty versatile :-) I'm figuring that we could even use it to turn plain binary into IPv4 addresses: bin(7f000001),be2dec(.,1) # 127.0.0.1 > diff --git a/src/sample.c b/src/sample.c > index d02034cf0..5b7ad8b34 100644 > --- a/src/sample.c > +++ b/src/sample.c Args description plese :-) > +static int sample_conv_be2dec(const struct arg *args, struct sample *smp, > void *private) > +{ > + struct buffer *trash = get_trash_chunk(); > + const int last = args[2].data.sint ? smp->data.u.str.data - > args[1].data.sint + 1 : smp->data.u.str.data; (...) > + int max_size = trash->size - 2; > + int i; > + int start; > + int ptr = 0; > + unsigned long long number; > + char *pos; > + > + trash->data = 0; > + > + while (ptr < last && trash->data <= max_size) { > + start = trash->data; > + if (ptr) { > + /* Separator */ I guess you meant that you want to add a separator, but my interpretation here was that you found one (while the code disagreed). Please simply update the comment accordingly. > + memcpy(trash->area + trash->data, > args[0].data.str.area, args[0].data.str.data); > + trash->data += args[0].data.str.data; > + } > + else > + max_size -= args[0].data.str.data; > + /* Integer */ > + for (number = 0, i = 0; i < args[1].data.sint && ptr < > smp->data.u.str.data; i++) > + number = (number << 8) + (unsigned > char)smp->data.u.str.area[ptr++]; > + pos = ulltoa(number, trash->area + trash->data, trash->size - > trash->data); > + if (pos) That code above is compact and it's not immediately obvious there's a loop embedded inside. Please try to leave some blank lines around logical "blocks" (with "block" being something everyone sees a different definition for). I would personally skip one line before and after the loop to isolate it a little bit but it's a matter of taste. On Wed, Aug 18, 2021 at 11:51:56AM +0200, Marcin Deranek wrote: > From 0a26066b685d52dae818d2e369aa48c3119c4769 Mon Sep 17 00:00:00 2001 > From: Marcin Deranek <[email protected]> > Date: Tue, 13 Jul 2021 14:08:56 +0200 > Subject: [PATCH 4/5] MINOR: sample: Add be2hex converter (...) Here, whenever a sample fetch function or a converter makes use of arguments, please always enumerate them in comments before the function and indicate their various validity domains. When troubleshooting it's a real pain to have no idea what the function expects. And please also recall it in front of the parsing function (for the same reasons, especially as it helps seeing that an argument is not initialized or any such issue). > +static int sample_conv_be2hex(const struct arg *args, struct sample *smp, > void *private) > +{ > + struct buffer *trash = get_trash_chunk(); > + int chunk_size = args[1].data.sint; > + const int last = args[2].data.sint ? smp->data.u.str.data - chunk_size > + 1 : smp->data.u.str.data; > + int i; > + int max_size; > + int ptr = 0; > + unsigned char c; > + > + trash->data = 0; > + if (args[0].data.str.data == 0 && args[2].data.sint == 0) > + chunk_size = smp->data.u.str.data; > + max_size = trash->size - 2 * chunk_size; > + > + while (ptr < last && trash->data <= max_size) { > + if (ptr) { > + /* Separator */ I guess you meant that you want to add a separator, but my interpretation here was that you found one (while the code disagreed), similarly, please simply update the comment accordingly. > + memcpy(trash->area + trash->data, > args[0].data.str.area, args[0].data.str.data); > + trash->data += args[0].data.str.data; > + } > + else > + max_size -= args[0].data.str.data; > + /* Hex */ > + for (i = 0; i < chunk_size && ptr < smp->data.u.str.data; i++) { > + c = smp->data.u.str.area[ptr++]; > + trash->area[trash->data++] = hextab[(c >> 4) & 0xF]; > + trash->area[trash->data++] = hextab[c & 0xF]; > + } > + } > + > + smp->data.u.str = *trash; > + smp->data.type = SMP_T_STR; > + smp->flags &= ~SMP_F_CONST; > + return 1; > +} > + > From 5b48ad66e32340a73f832fbea8f80acae31ffd7a Mon Sep 17 00:00:00 2001 > From: Marcin Deranek <[email protected]> > Date: Tue, 13 Jul 2021 19:04:24 +0200 > Subject: [PATCH 5/5] MINOR: config: Deprecate tune.ssl.capture-cipherlist-size > > Deprecate tune.ssl.capture-cipherlist-size in favor of > tune.ssl.capture-buffer-size which better describes the purpose of the > setting. (...) Exact same comments as for be2dec. Thus overall it looks pretty good to me. I'm really sorry to have missed your first resend a month ago, I'll be more careful this time. Do not hesitate to Cc me with the next series! Thanks! Willy
