You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2026/10/07 00:39:44 UTC
[PR] Add TSClientHello::get_extension_types_raw for JA4 [trafficserver]
bneradt opened a new pull request, #13809:
URL: https://github.com/apache/trafficserver/pull/13809
On OpenSSL builds, TSClientHello::get_extension_types() only reports
the extensions OpenSSL recognizes. GREASE values and unknown types such
as ALPS (0x4469) or ECH on older OpenSSL never make it into OpenSSL's
parsed ClientHello table, so they cannot be recovered through any
OpenSSL ClientHello accessor. As a result, jax_fingerprint computes a
JA4 for Chrome of t13d1515h2 instead of the t13d1516h2 that Wireshark
and other JA4 implementations report for the same connection.
This patch records the extension types from the raw ClientHello
message on OpenSSL, using a message callback that runs before the
ClientHello callback and is removed once the handshake completes. A
new additive plugin API, TSClientHello::get_extension_types_raw(),
exposes that list in wire order, GREASE included. BoringSSL already
walks the raw extensions buffer, so its raw and existing lists are the
same. The semantics of get_extension_types() are unchanged so that
existing plugins, such as the jax_fingerprint JA3 method, keep
producing the same fingerprints.
The jax_fingerprint JA4 method now uses the raw list. A Catch2 test
covers the ClientHello parser and a new autest sends a Chrome-shaped
ClientHello and verifies the resulting JA4.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6151049831
Correction to my earlier reply: `_raw_extension_types = {}` resolves to the `initializer_list` assignment, which keeps the existing buffer (confirmed with g++ and clang: capacity stays 50). `_clear()` now uses `std::vector<int>().swap(_raw_extension_types)`, which does release it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "Copilot (via GitHub) via github" <gi...@trafficserver.apache.org>.
Copilot commented on code in PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#discussion_r4476592000
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -595,6 +595,24 @@ ssl_client_hello_callback(const SSL_CLIENT_HELLO *client_hello)
return CLIENT_HELLO_SUCCESS;
}
+#if HAVE_SSL_CTX_SET_CLIENT_HELLO_CB
+// OpenSSL's ClientHello API drops extensions it doesn't recognize, including
+// GREASE. Record the extension types from the raw ClientHello message, which
+// this callback receives before ssl_client_hello_callback runs.
+static void
+ssl_msg_callback(int write_p, int /* version ATS_UNUSED */, int content_type, const void *buf, size_t len, SSL *ssl,
+ void * /* arg ATS_UNUSED */)
+{
+ auto const *msg = static_cast<const uint8_t *>(buf);
+ if (write_p || content_type != SSL3_RT_HANDSHAKE || len == 0 || msg[0] != SSL3_MT_CLIENT_HELLO) {
+ return;
+ }
+ if (TLSSNISupport *snis = TLSSNISupport::getInstance(ssl); snis != nullptr) {
+ snis->on_client_hello_message(msg, len);
+ }
Review Comment:
on_client_hello_message() documents and requires the *full* handshake message (including the 4-byte header), but ssl_msg_callback forwards the msg callback buffer directly. If OpenSSL ever invokes the msg callback with record-level handshake fragments (or otherwise not the complete ClientHello), this will systematically fail parsing, increment the failure metric, and negate the intended fix for fragmented ClientHellos. Consider adding explicit reassembly keyed off the handshake length field (buffer until 4+body_len), or ensure via API guarantees that this callback always receives complete handshake messages before calling on_client_hello_message().
##########
src/iocore/net/TLSSNISupport.cc:
##########
@@ -138,6 +141,111 @@ TLSSNISupport::on_client_hello(ClientHello &client_hello)
}
}
+void
+TLSSNISupport::on_client_hello_message(const uint8_t *msg, size_t len)
+{
+ _reset_raw_extension_types();
+
+ auto const count =
+ parse_client_hello_extension_types(msg, len, _fast_raw_extension_storage.data(), _fast_raw_extension_storage.size());
+ if (!count) {
+ Metrics::Counter::increment(ssl_rsb.client_hello_raw_parse_failure);
+ Dbg(dbg_ctl_ssl_sni, "Could not parse the extensions of the raw ClientHello");
+ return;
+ }
+ if (*count <= _fast_raw_extension_storage.size()) {
+ _raw_extension_types = _fast_raw_extension_storage.data();
+ } else {
+ _slow_raw_extension_storage = std::make_unique<int[]>(*count);
+ parse_client_hello_extension_types(msg, len, _slow_raw_extension_storage.get(), *count);
Review Comment:
The second call to parse_client_hello_extension_types() ignores its return value. If it were to ever fail (e.g., due to unexpected callback buffer semantics), _raw_extension_types would be set to storage that may be only partially populated. Capture and validate the returned optional (e.g., ink_assert / early-return on failure) so the state remains consistent and failures are surfaced deterministically.
##########
tests/uranium_tests/pluginTest/jax_fingerprint/test_jax_fingerprint_raw_extensions.py:
##########
@@ -0,0 +1,56 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import re
+import sys
+
+import pytest
+
+from tools.uranium.services import ATSFactory, ServiceFactory, wait_for_file_lines
+
+
+def test_jax_fingerprint_raw_extensions(ats_factory: ATSFactory, services: ServiceFactory) -> None:
+ """Include ALPS but not GREASE in complete and fragmented Chrome fingerprints.
+
Review Comment:
This docstring is a bit misleading relative to the behavior under test: the ClientHello being sent includes GREASE extension *types*, but the JA4 computation drops GREASE values by design. Consider rewording to clarify that the test verifies the raw extension-type list includes unknown types like ALPS (and that GREASE is present on the wire but excluded from the computed fingerprint).
##########
tests/uranium_tests/pluginTest/jax_fingerprint/chrome_client_hello.py:
##########
@@ -0,0 +1,169 @@
+#!/usr/bin/env python3
+'''
+Send a Chrome-shaped TLS ClientHello and report the JA4 fingerprint it should produce.
+
+The ClientHello carries GREASE cipher suites and extensions as well as the
+ALPS (0x4469) extension, which OpenSSL does not recognize. A fingerprint
+computed from the extensions OpenSSL reports would miss all of these.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import argparse
+import hashlib
+import os
+import socket
+import struct
+import sys
+
+SERVER_NAME = b'jax.server.test'
+
+GREASE_CIPHER = 0x0A0A
+CIPHERS = [
+ GREASE_CIPHER, 0x1301, 0x1302, 0x1303, 0xC02B, 0xC02F, 0xC02C, 0xC030, 0xCCA9, 0xCCA8, 0xC013, 0xC014, 0x009C, 0x009D, 0x002F,
+ 0x0035
+]
+
+SIGNATURE_ALGORITHMS = [0x0403, 0x0804, 0x0401, 0x0503, 0x0805, 0x0501, 0x0806, 0x0601]
+
+EXT_SERVER_NAME = 0x0000
+EXT_ALPN = 0x0010
+EXT_ALPS = 0x4469
+FIRST_GREASE_EXTENSION = 0x2A2A
+LAST_GREASE_EXTENSION = 0x9A9A
+
+
+def is_grease(value: int) -> bool:
+ """Identify a reserved GREASE value.
+
+ :param value: Unsigned 16-bit cipher or extension identifier.
+ """
+ return (value & 0x0F0F) == 0x0A0A and (value >> 8) == (value & 0xFF)
+
+
+def u8_prefixed(data: bytes) -> bytes:
+ """Prefix bytes with their one-byte length.
+
+ :param data: Payload of at most 255 bytes.
+ """
+ return struct.pack('!B', len(data)) + data
+
+
+def u16_prefixed(data: bytes) -> bytes:
+ """Prefix bytes with their network-order two-byte length.
+
+ :param data: Payload of at most 65535 bytes.
+ """
+ return struct.pack('!H', len(data)) + data
+
+
+def u16_list(values: list[int]) -> bytes:
+ """Encode a list of network-order unsigned integers.
+
+ :param values: Unsigned 16-bit identifiers to concatenate.
+ """
+ return b''.join(struct.pack('!H', v) for v in values)
+
+
+def extensions() -> list[tuple[int, bytes]]:
+ '''Return the ClientHello extensions, in Chrome's wire order.'''
+ server_name = u16_prefixed(b'\x00' + u16_prefixed(SERVER_NAME))
+ alpn = u16_prefixed(u8_prefixed(b'h2') + u8_prefixed(b'http/1.1'))
+ groups = u16_prefixed(u16_list([0x1A1A, 0x001D, 0x0017, 0x0018]))
+ key_share = u16_prefixed(u16_list([0x1A1A]) + u16_prefixed(b'\x00') + u16_list([0x001D]) + u16_prefixed(os.urandom(32)))
+ supported_versions = u8_prefixed(u16_list([0x3A3A, 0x0304, 0x0303]))
+ return [
+ (FIRST_GREASE_EXTENSION, b''),
+ (EXT_SERVER_NAME, server_name),
+ (0x0017, b''), # extended_master_secret
+ (0xFF01, b'\x00'), # renegotiation_info
+ (0x000A, groups), # supported_groups
+ (0x000B, u8_prefixed(b'\x00')), # ec_point_formats
+ (0x0023, b''), # session_ticket
+ (EXT_ALPN, alpn),
+ (0x0005, b'\x01\x00\x00\x00\x00'), # status_request
+ (0x000D, u16_prefixed(u16_list(SIGNATURE_ALGORITHMS))), # signature_algorithms
+ (0x0012, b''), # signed_certificate_timestamp
+ (0x0033, key_share), # key_share
+ (0x002D, u8_prefixed(b'\x01')), # psk_key_exchange_modes
+ (0x002B, supported_versions), # supported_versions
+ (0x001B, u8_prefixed(u16_list([0x0002]))), # compress_certificate
+ (EXT_ALPS, u16_prefixed(u8_prefixed(b'h2'))), # application_settings
+ (LAST_GREASE_EXTENSION, b'\x00'),
+ (0x0015, bytes(16)), # padding
+ ]
+
+
+def client_hello_records(fragment_size: int = 0) -> bytes:
+ '''Return TLS records holding the ClientHello.
+
+ :param fragment_size: When non-zero, split the ClientHello across records
+ of at most this many bytes.
+ '''
+ body = struct.pack('!H', 0x0303) + os.urandom(32) + u8_prefixed(os.urandom(32))
+ body += u16_prefixed(u16_list(CIPHERS))
+ body += u8_prefixed(b'\x00')
+ body += u16_prefixed(b''.join(struct.pack('!HH', t, len(d)) + d for t, d in extensions()))
+ handshake = b'\x01' + struct.pack('!I', len(body))[1:] + body
+ size = fragment_size or len(handshake)
+ fragments = [handshake[i:i + size] for i in range(0, len(handshake), size)]
+ return b''.join(struct.pack('!BHH', 0x16, 0x0301, len(f)) + f for f in fragments)
+
+
+def truncated_sha256(values: list[str]) -> str:
+ """Hash comma-separated fields using JA4's truncated SHA-256 format.
+
+ :param values: Ordered hexadecimal fingerprint fields.
+ """
+ return hashlib.sha256(','.join(values).encode()).hexdigest()[:12]
+
+
+def expected_ja4() -> str:
+ '''Return the JA4 fingerprint computed from every extension on the wire.
+
+ The c section hashes the signature algorithms per the JA4 specification.
+ '''
+ ciphers = [c for c in CIPHERS if not is_grease(c)]
+ types = [t for t, _ in extensions() if not is_grease(t)]
+ hashed_types = [t for t in types if t not in (EXT_SERVER_NAME, EXT_ALPN)]
+
+ part_a = f't13d{len(ciphers):02d}{len(types):02d}h2'
+ part_b = truncated_sha256(sorted(f'{c:04x}' for c in ciphers))
+ sigalgs = ','.join(f'{a:04x}' for a in SIGNATURE_ALGORITHMS)
+ part_c = hashlib.sha256(f"{','.join(sorted(f'{t:04x}' for t in hashed_types))}_{sigalgs}".encode()).hexdigest()[:12]
+ return f'{part_a}_{part_b}_{part_c}'
+
+
+def main() -> int:
+ parser = argparse.ArgumentParser(description=__doc__)
+ parser.add_argument('port', type=int, help='The TLS port to send the ClientHello to.')
+ parser.add_argument(
+ '--fragment-size', type=int, default=0, help='Split the ClientHello across records of at most this many bytes.')
+ args = parser.parse_args()
+
+ with socket.create_connection(('127.0.0.1', args.port), timeout=5) as sock:
+ sock.sendall(client_hello_records(args.fragment_size))
+ # Wait for the ServerHello so that the ClientHello has been processed.
+ response = sock.recv(5)
Review Comment:
The script closes the TCP connection immediately after reading 5 bytes of the server response, which can provoke server-side TLS errors/noise depending on logging configuration and timing (potentially making the subsequent diagnostics assertions flaky). To make the test more robust, perform a cleaner shutdown (e.g., read and discard more of the ServerHello/handshake or send/attempt TLS close_notify semantics if practical for this raw-socket approach) before closing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add TSClientHello::get_extension_types_raw for JA4 [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6067727265
[approve ci autest 1]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6151271683
Pushed as three commits so the delta is easy to review: 206466e is unchanged from the version you approved, e90054f releases the vector with `swap` (the actual leak fix), and 4160a70 adds a `proxy.process.ssl.client_hello_raw_parse_failure` counter for when the raw ClientHello can't be parsed and `get_extension_types()` falls back to OpenSSL's list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#discussion_r4262205046
##########
src/iocore/net/TLSSNISupport.cc:
##########
@@ -138,6 +142,84 @@ TLSSNISupport::on_client_hello(ClientHello &client_hello)
}
}
+void
+TLSSNISupport::on_client_hello_message(const uint8_t *msg, size_t len)
+{
+ _has_raw_extension_types = parse_client_hello_extension_types(msg, len, _raw_extension_types);
+ if (!_has_raw_extension_types) {
+ Metrics::Counter::increment(ssl_rsb.client_hello_raw_parse_failure);
+ Dbg(dbg_ctl_ssl_sni, "Could not parse the extensions of the raw ClientHello");
+ }
+}
+
+bool
+TLSSNISupport::parse_client_hello_extension_types(const uint8_t *msg, size_t msg_len, std::vector<int> &types)
+{
+ types.clear();
+
+ size_t offset = 0;
+ size_t end = 0;
+
+ auto read_u8 = [&](size_t &value) {
+ if (offset + 1 > end) {
+ return false;
+ }
+ value = msg[offset];
+ offset += 1;
+ return true;
+ };
+ auto read_u16 = [&](size_t &value) {
+ if (offset + 2 > end) {
+ return false;
+ }
+ value = (msg[offset] << 8) | msg[offset + 1];
+ offset += 2;
+ return true;
+ };
+ auto skip = [&](size_t n) {
+ if (offset + n > end) {
+ return false;
+ }
+ offset += n;
+ return true;
+ };
+
+ // Handshake header: msg_type(1) length(3).
+ if (msg_len < 4 || msg[0] != SSL3_MT_CLIENT_HELLO) {
+ return false;
+ }
+ size_t const body_len = (msg[1] << 16) | (msg[2] << 8) | msg[3];
+ if (4 + body_len > msg_len) {
+ return false;
+ }
+ offset = 4;
+ end = 4 + body_len;
+
+ // legacy_version(2) random(32) session_id<0..32> cipher_suites<2..2^16-2> compression_methods<1..2^8-1>
+ size_t len = 0;
+ if (!skip(2 + 32) || !read_u8(len) || !skip(len) || !read_u16(len) || !skip(len) || !read_u8(len) || !skip(len)) {
+ return false;
+ }
+
Review Comment:
Fixed in 574706f: the parser now rejects a legacy session id longer than 32 bytes, an empty or odd-length cipher suite list, and an empty compression method list. Each case has a unit test.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -248,6 +248,24 @@ ssl_client_hello_callback(const SSL_CLIENT_HELLO *client_hello)
return CLIENT_HELLO_SUCCESS;
}
+#if HAVE_SSL_CTX_SET_CLIENT_HELLO_CB
+// OpenSSL's ClientHello API drops extensions it doesn't recognize, including
+// GREASE. Record the extension types from the raw ClientHello message, which
+// this callback receives before ssl_client_hello_callback runs.
+static void
+ssl_msg_callback(int write_p, int /* version ATS_UNUSED */, int content_type, const void *buf, size_t len, SSL *ssl,
+ void * /* arg ATS_UNUSED */)
+{
+ auto const *msg = static_cast<const uint8_t *>(buf);
+ if (write_p || content_type != SSL3_RT_HANDSHAKE || len == 0 || msg[0] != SSL3_MT_CLIENT_HELLO) {
+ return;
+ }
+ if (TLSSNISupport *snis = TLSSNISupport::getInstance(ssl); snis != nullptr) {
+ snis->on_client_hello_message(msg, len);
+ }
Review Comment:
This doesn't apply to handshake messages. For `SSL3_RT_HANDSHAKE`, OpenSSL calls the message callback once per complete, reassembled handshake message (from `tls_get_message_body()`), not per record payload. Record-level calls use `SSL3_RT_HEADER`, which the callback ignores. To confirm, 574706f adds an autest case that sends the ClientHello split across 100-byte records: it produces the same JA4 as the single-record ClientHello and leaves `proxy.process.ssl.client_hello_raw_parse_failure` at 0.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -1025,6 +1043,10 @@ ssl_callback_info(const SSL *ssl, int where, int ret)
}
if (where & SSL_CB_HANDSHAKE_DONE) {
// handshake is complete
+#if HAVE_SSL_CTX_SET_CLIENT_HELLO_CB
+ // The raw ClientHello has been recorded. Stop the per record message callbacks.
+ SSL_set_msg_callback(const_cast<SSL *>(ssl), nullptr);
Review Comment:
Per maskit's review, we're not handling TLS 1.2 client renegotiation here: the plugin API doesn't support it, HTTP/2 forbids it, and TLS 1.3 removed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub) via github" <gi...@trafficserver.apache.org>.
bneradt commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6641336043
@bryancall: thanks. Both points are addressed at 88a558f77c, rebased on master.
- **`<span>`**: `parse_client_hello_extension_types()` now takes `int *types, size_t capacity` instead of a `std::span`, and `TLSSNISupport.h` no longer includes `<span>`. I folded this into f395c365d4, "Keep raw ClientHello extension types off the heap". The fixed 32-entry buffer and the heap fallback are unchanged.
- **Reading back the heap list**: `TLSSNISupport` has a new protected `_get_raw_extension_types(size_t &count)`, which `RecycledTLSSNISupport` exposes. On every recycle, the recycle test now checks that all 40 types match in wire order, and that `_clear()` leaves no list recorded. I checked that dropping the second parse into the heap buffer now fails the test (160 failed assertions), and that the leak checks still pass.
At 88a558f77c, in an ASan build, all 97 `test_net` ctest cases and 75 urtests (`jax_fingerprint`, `ja4`, `ja3`, `client_hello`, `tls_hooks` and `sni`) pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub) via github" <gi...@trafficserver.apache.org>.
bneradt commented on code in PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#discussion_r4482916579
##########
tests/uranium_tests/pluginTest/jax_fingerprint/test_jax_fingerprint_raw_extensions.py:
##########
@@ -0,0 +1,56 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import re
+import sys
+
+import pytest
+
+from tools.uranium.services import ATSFactory, ServiceFactory, wait_for_file_lines
+
+
+def test_jax_fingerprint_raw_extensions(ats_factory: ATSFactory, services: ServiceFactory) -> None:
+ """Include ALPS but not GREASE in complete and fragmented Chrome fingerprints.
+
Review Comment:
The docstring already says this: the fingerprints include ALPS and exclude GREASE. The client sends GREASE on the wire, and `expected_ja4()` in `chrome_client_hello.py` drops it the same way JA4 does. No change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#discussion_r4225059722
##########
tests/gold_tests/pluginTest/jax_fingerprint/jax_fingerprint_raw_extensions.test.py:
##########
@@ -0,0 +1,65 @@
+'''
+Verify that jax_fingerprint JA4 counts every ClientHello extension on the wire.
+
+OpenSSL's ClientHello API omits GREASE and extension types it does not
+recognize, such as ALPS. JA4 must still count and hash them.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import sys
+
+sys.path.insert(0, Test.TestDirectory)
+from chrome_client_hello import expected_ja4
+
+Test.Summary = __doc__
+Test.SkipUnless(Condition.PluginExists('jax_fingerprint.so'))
+
+# Wireshark's JA4 for this Chrome ClientHello: 15 non-GREASE ciphers and 16
+# non-GREASE extensions, including ALPS.
+CHROME_JA4 = 't13d1516h2_8daaf6152771_e5627efa2ab1'
+assert expected_ja4() == CHROME_JA4, expected_ja4()
+
+ts = Test.MakeATSProcess('ts', enable_tls=True)
+ts.addDefaultSSLFiles()
+ts.Disk.ssl_multicert_yaml.AddLines(
+ '''
+ssl_multicert:
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+'''.split('\n'))
+ts.Disk.records_config.update(
+ {
+ 'proxy.config.ssl.server.cert.path': ts.Variables.SSLDir,
+ 'proxy.config.ssl.server.private_key.path': ts.Variables.SSLDir,
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'jax_fingerprint',
+ })
+ts.Disk.plugin_config.AddLine('jax_fingerprint.so --method JA4')
+ts.Disk.traffic_out.Content += Testers.ContainsExpression(
+ f'Fingerprint: {CHROME_JA4}', 'JA4 should count GREASE and ALPS as they appear on the wire.')
Review Comment:
JA4 deliberately excludes GREASE extensions (as `expected_ja4()` does at line 114), so this failure message incorrectly says they are counted. Only the unknown ALPS extension affects this expected fingerprint; please make the diagnostic reflect that distinction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub) via github" <gi...@trafficserver.apache.org>.
bneradt commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6609627656
Thanks for the review, @bryancall. I rebased on master and added a sixth commit, d34ea08df8 "Clarify raw ClientHello docs and test diagnostics", for your non-blocking notes. The first five commits are unchanged apart from the rebase.
Addressed:
- **Counter wording.** `ssl.en.rst` now says `client_hello_raw_parse_failure` counts malformed ClientHellos, which OpenSSL also rejects (most of them before any plugin sees the ClientHello), so it does not point to missing fingerprints.
- **Upgrade note.** `upgrading.en.rst` now says GREASE on its own doesn't change the fingerprints, since they drop it; only unrecognized non-GREASE types such as ALPS do. It also says `ja3_fingerprint` still reads OpenSSL's list, so on OpenSSL its JA3 can now differ from the one `jax_fingerprint` logs for the same client.
- **Stale comment.** The `get_extension_types()` comment in `include/ts/apidefs.h.in` now describes the raw, in-order list and the fallback to OpenSSL's list.
- **Direct assert.** `test_jax_fingerprint_raw_extensions.py` now asserts `Expected JA4:` on the client's output directly, so if the helper's JA4 drifts, the failure names the JA4 instead of a missing ServerHello.
Not addressed:
- **`swap` and counter coverage.** Dropping the `swap` in `_clear()` only leaves the old vector's capacity allocated on a recycled VC. Nothing observable changes, so an end-to-end test can't catch it. Covering the increment would need a hand-built malformed ClientHello whose handshake fails, just to read a counter. The parser's rejection paths are already covered in `test_TLSSNISupport.cc`, so I left that out of this PR.
- **Pinning `ja4_fingerprint`'s JA4.** `ja4_fingerprint` gets its extension list from the same `get_extension_types()` call that the new `jax_fingerprint` test pins. Pinning its JA4 too would mostly retest the same path through a second plugin. I kept this PR to the API change and the one consumer test.
Tested locally on the rebased head (Fedora container, `ci-fedora-urtest` preset with unit tests on): 101/101 matching ctest tests (`test_net`, `test_jax`, the ja3/ja4 unit tests), all 21 jax/ja3/ja4 fingerprint urtests, the docs build with no warnings, and the format check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6070159780
Thanks, agreed. I reworked the patch that way:
- `get_extension_types()` now returns the raw ClientHello extension list on OpenSSL, falling back to `SSL_client_hello_get1_extensions_present` if the raw message doesn't parse. `TSRawExtensionTypeList`, `get_extension_types_raw()`, `raw_begin()`/`raw_end()`, the extra `InkAPI.cc` iterators and the `ja4/tls_client_hello_summary.cc` change are gone.
- `upgrading.en.rst` now has a release note that JA3/JA4 values on OpenSSL change for clients sending extensions OpenSSL doesn't recognize.
- `TSClientHello.en.rst` notes that `TSClientHelloExtensionGet()` still can't return the contents of those extensions on OpenSSL.
The message callback, the parser and both tests are unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6099917574
Good catch, thanks. `TLSSNISupport::_clear()` now releases `_raw_extension_types` (`= {}`) and resets `_has_raw_extension_types`, so the buffer is freed when `SSLNetVConnection::clear()` recycles the VC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub) via github" <gi...@trafficserver.apache.org>.
bneradt commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6642150711
[approve ci urtest 3]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub) via github" <gi...@trafficserver.apache.org>.
bneradt commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6752342024
[approve ci format]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add TSClientHello::get_extension_types_raw for JA4 [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6069758432
Instead of adding a second API, could we fix `get_extension_types()` itself on OpenSSL?
On OpenSSL the existing list is wrong for fingerprinting in general, not just for JA4. Any fingerprint that counts or hashes extension types expects every type on the wire except GREASE. BoringSSL's `get_extension_types()` already gives that, because it walks the raw extensions buffer. The description keeps the old semantics so that JA3 output doesn't change. But on OpenSSL that output is off in the same way: it misses ALPS for Chrome, and it differs from the same plugin on BoringSSL. jax_fingerprint filters GREASE itself, so nothing depends on OpenSSL leaving GREASE out. The doc for `get_extension_types()` also already promises "a consistent interface regardless of the SSL library in use", which OpenSSL doesn't meet today.
If the OpenSSL branch of `ClientHello::begin()`/`end()` returns `_raw_extension_types` when the raw message parsed, and falls back to `SSL_client_hello_get1_extensions_present` otherwise, every method picks up the fix with no plugin changes. The PR could then drop `TSRawExtensionTypeList`, `get_extension_types_raw()`, `raw_begin()`/`raw_end()`, the two `InkAPI.cc` iterators, and the change to `ja4/tls_client_hello_summary.cc`. The message callback, the parser, and both tests stay as they are.
This changes behavior that shipped in 10.2.0, since the API came from PR 12914 and was backported to 10.2.x. It only affects OpenSSL builds, and it only adds entries. JA3 values on OpenSSL will change for clients that send extensions OpenSSL doesn't recognize, so it should get a release note.
Separately, and not related to JA3, JA4 or jax_fingerprint: one small doc request. On OpenSSL, `TSClientHelloExtensionGet()` still only returns extensions OpenSSL recognizes, because `SSL_client_hello_get0_ext` reads the same parsed table. After this change the type list can include ALPS while `TSClientHelloExtensionGet(ch, 0x4469, ...)` returns `TS_ERROR`. jax_fingerprint isn't affected, because it never fetches the contents of those extensions. A note in `TSClientHello.en.rst` would just tell other plugin authors about the mismatch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#discussion_r4263110743
##########
src/iocore/net/unit_tests/test_TLSSNISupport.cc:
##########
@@ -0,0 +1,226 @@
+/** @file
+
+ Catch based unit tests for parsing extension types from a raw ClientHello.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "tscore/ink_inet.h"
+#include "iocore/net/TLSSNISupport.h"
+
+#include <catch2/catch_test_macros.hpp>
+
+#include <cstdint>
+#include <vector>
+
+namespace
+{
+
+struct Extension {
+ uint16_t type;
+ std::vector<uint8_t> data;
+};
+
+void
+put_u16(std::vector<uint8_t> &out, size_t value)
+{
+ out.push_back(static_cast<uint8_t>(value >> 8));
+ out.push_back(static_cast<uint8_t>(value));
+}
+
+// The lengths of the variable length fields that precede the extensions.
+struct Lengths {
+ size_t session_id = 32;
+ size_t cipher_suites = 4;
+ size_t compression_methods = 1;
+};
+
+std::vector<uint8_t>
+make_client_hello_body(std::vector<Extension> const &extensions, bool with_extensions_block = true, Lengths lengths = {})
+{
+ std::vector<uint8_t> body;
+
+ put_u16(body, 0x0303);
+ body.insert(body.end(), 32, 0xAB);
+
+ body.push_back(static_cast<uint8_t>(lengths.session_id));
+ body.insert(body.end(), lengths.session_id, 0xCD);
+
+ // By default, GREASE and TLS_AES_128_GCM_SHA256.
+ put_u16(body, lengths.cipher_suites);
+ for (size_t i = 0; i < lengths.cipher_suites; ++i) {
+ body.push_back(i < 2 ? 0x0A : (i % 2 == 0 ? 0x13 : 0x01));
+ }
+
+ // By default, the null compression method.
+ body.push_back(static_cast<uint8_t>(lengths.compression_methods));
+ body.insert(body.end(), lengths.compression_methods, 0);
+
+ if (with_extensions_block) {
+ std::vector<uint8_t> block;
+ for (auto const &ext : extensions) {
+ put_u16(block, ext.type);
+ put_u16(block, ext.data.size());
+ block.insert(block.end(), ext.data.begin(), ext.data.end());
+ }
+ put_u16(body, block.size());
+ body.insert(body.end(), block.begin(), block.end());
+ }
+ return body;
+}
+
+std::vector<uint8_t>
+make_handshake_message(std::vector<uint8_t> const &body)
+{
+ std::vector<uint8_t> msg{SSL3_MT_CLIENT_HELLO, static_cast<uint8_t>(body.size() >> 16), static_cast<uint8_t>(body.size() >> 8),
+ static_cast<uint8_t>(body.size())};
+ msg.insert(msg.end(), body.begin(), body.end());
+ return msg;
+}
+
+std::vector<int>
+parse(std::vector<uint8_t> const &msg, bool expect_success = true)
+{
+ std::vector<int> types{-1};
+ bool const parsed = TLSSNISupport::parse_client_hello_extension_types(msg.data(), msg.size(), types);
+ REQUIRE(parsed == expect_success);
+ return types;
+}
+
+} // namespace
+
+TEST_CASE("Raw ClientHello extension types are listed in wire order", "[TLSSNISupport]")
+{
+ // GREASE, server_name, ALPS (0x4469), ECH (0xfe0d), an empty extension, and GREASE again.
+ std::vector<Extension> const extensions{
+ {0x1A1A, {} },
+ {0x0000, {0x00, 0x00} },
+ {0x4469, {0x00, 0x03, 0x02, 'h'}},
+ {0xFE0D, {0x01, 0x02, 0x03} },
+ {0x0017, {} },
+ {0xCACA, {0x00} },
+ };
+
+ auto const types = parse(make_handshake_message(make_client_hello_body(extensions)));
+ CHECK(types == std::vector<int>{0x1A1A, 0x0000, 0x4469, 0xFE0D, 0x0017, 0xCACA});
+}
+
+TEST_CASE("Raw ClientHello duplicate extension types are preserved", "[TLSSNISupport]")
+{
+ auto const types = parse(make_handshake_message(make_client_hello_body({
+ {0x000A, {}},
+ {0x000A, {}},
+ })));
+ CHECK(types == std::vector<int>{0x000A, 0x000A});
+}
+
+TEST_CASE("Raw ClientHello without an extensions block", "[TLSSNISupport]")
+{
+ auto const types = parse(make_handshake_message(make_client_hello_body({}, false)));
+ CHECK(types.empty());
+}
+
+TEST_CASE("Raw ClientHello with an empty extensions block", "[TLSSNISupport]")
+{
+ auto const types = parse(make_handshake_message(make_client_hello_body({})));
+ CHECK(types.empty());
+}
+
+TEST_CASE("Malformed raw ClientHellos are rejected", "[TLSSNISupport]")
+{
+ std::vector<Extension> const extensions{
+ {0x0000, {0x00, 0x00} },
+ {0x002B, {0x02, 0x03, 0x04}},
+ };
+ auto const good = make_handshake_message(make_client_hello_body(extensions));
+
+ SECTION("empty message")
+ {
+ CHECK(parse({}, false).empty());
+ }
+
+ SECTION("not a ClientHello")
+ {
+ auto msg = good;
+ msg[0] = SSL3_MT_SERVER_HELLO;
+ CHECK(parse(msg, false).empty());
+ }
+
+ SECTION("handshake length longer than the message")
+ {
+ auto msg = good;
+ msg.pop_back();
+ CHECK(parse(msg, false).empty());
+ }
+
+ SECTION("truncated before the extensions block")
+ {
+ auto body = make_client_hello_body({}, false);
+ body.resize(30);
+ CHECK(parse(make_handshake_message(body), false).empty());
+ }
+
+ SECTION("legacy session id longer than 32 bytes")
+ {
+ CHECK(parse(make_handshake_message(make_client_hello_body(extensions, true, {.session_id = 33})), false).empty());
+ }
+
+ SECTION("no cipher suites")
+ {
+ CHECK(parse(make_handshake_message(make_client_hello_body(extensions, true, {.cipher_suites = 0})), false).empty());
+ }
+
+ SECTION("odd cipher suites length")
+ {
+ CHECK(parse(make_handshake_message(make_client_hello_body(extensions, true, {.cipher_suites = 3})), false).empty());
+ }
+
+ SECTION("no compression methods")
+ {
+ CHECK(parse(make_handshake_message(make_client_hello_body(extensions, true, {.compression_methods = 0})), false).empty());
+ }
+
+ SECTION("extension data longer than the extensions block")
+ {
+ auto body = make_client_hello_body(extensions);
+ // Grow the last extension's declared length by one byte.
+ body[body.size() - 4] += 1;
Review Comment:
This does hit the length field. The last extension is `0x002B` with three data bytes, so the body ends with `type_hi type_lo len_hi len_lo d0 d1 d2`. Counting from the end, `size - 1` to `size - 3` are the data bytes and `size - 4` is `len_lo`. Incrementing it changes the declared length from 3 to 4, which runs past the extensions block, and the section asserts that the parse fails.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub) via github" <gi...@trafficserver.apache.org>.
bneradt commented on code in PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#discussion_r4482924014
##########
src/iocore/net/TLSSNISupport.cc:
##########
@@ -138,6 +141,111 @@ TLSSNISupport::on_client_hello(ClientHello &client_hello)
}
}
+void
+TLSSNISupport::on_client_hello_message(const uint8_t *msg, size_t len)
+{
+ _reset_raw_extension_types();
+
+ auto const count =
+ parse_client_hello_extension_types(msg, len, _fast_raw_extension_storage.data(), _fast_raw_extension_storage.size());
+ if (!count) {
+ Metrics::Counter::increment(ssl_rsb.client_hello_raw_parse_failure);
+ Dbg(dbg_ctl_ssl_sni, "Could not parse the extensions of the raw ClientHello");
+ return;
+ }
+ if (*count <= _fast_raw_extension_storage.size()) {
+ _raw_extension_types = _fast_raw_extension_storage.data();
+ } else {
+ _slow_raw_extension_storage = std::make_unique<int[]>(*count);
+ parse_client_hello_extension_types(msg, len, _slow_raw_extension_storage.get(), *count);
Review Comment:
Good catch. Fixed in 3c5ce08fe3: the second parse into the overflow storage now checks the stored count. If it differs from the first pass, it counts a raw parse failure and resets the raw list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub) via github" <gi...@trafficserver.apache.org>.
bneradt commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6636173494
@maskit, @bryancall: thanks, both are addressed in two new commits at the end of the branch, rebased on master. Head is 1e4ba0aa78.
**No allocation in the common handshake** (3d20e0efb9, "Keep raw ClientHello extension types off the heap")
- `parse_client_hello_extension_types()` now validates the whole message, writes the first `types.size()` extension types into a caller-provided `std::span`, and returns the total count. It returns `std::nullopt` if the message is malformed.
- `on_client_hello_message()` parses into a fixed `std::array` of 32, the same `MAX_EXTENSIONS_FOR_FAST_PATH` that `ja4/tls_client_hello_summary.h` uses. Only when the count is more than 32 does it allocate an exact-size `std::unique_ptr<int[]>` and parse a second time into it. So the common path makes one pass and no allocation, and the list still has no cap.
- `_clear()`, `bind()`, and the start of each `on_client_hello_message()` all release the heap fallback through `_reset_raw_extension_types()`.
- The element type is `int`, not `uint16_t` as in the jax summary. On OpenSSL, `ExtensionIdIterator` walks an `int *` because that is what `SSL_client_hello_get1_extensions_present()` returns. Supporting a second element type would need a second pointer in the iterator, and `TSClientHello::TSExtensionTypeList::Iterator` reserves only 24 bytes for it. The `static_assert` in `InkAPI.cc` caught that, so the iterator and the public API are unchanged.
- There is also a new unit test that passes a buffer shorter than the extension count and checks that the count is still right.
**Recycle test** (1e4ba0aa78, "Test that _clear() releases the raw extension list")
This is Bryan's `RecycledTLSSNISupport` test as written: 40 extensions, so it goes through the heap fallback. I checked it under ASan with `ENABLE_ASAN=ON`, Fedora 44, OpenSSL 3:
| `_clear()` | `test_net '[TLSSNISupport]'` |
|---|---|
| calls `_reset_raw_extension_types()` (this PR) | all 7 cases pass, no leaks |
| clears only the pointer and count, leaving the heap buffer | `LeakSanitizer: 640 byte(s) leaked in 4 allocation(s)` from `on_client_hello_message()` |
**Local runs at 1e4ba0aa78** (ASan build): all 97 `test_net` ctest cases pass. 75 urtests matching `jax_fingerprint or ja4 or ja3 or client_hello or tls_hooks or sni` pass, including `test_jax_fingerprint_raw_extensions`.
The docs only describe what `get_extension_types()` returns, which hasn't changed, so they needed no edits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#discussion_r4252467839
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -1025,6 +1043,10 @@ ssl_callback_info(const SSL *ssl, int where, int ret)
}
if (where & SSL_CB_HANDSHAKE_DONE) {
// handshake is complete
+#if HAVE_SSL_CTX_SET_CLIENT_HELLO_CB
+ // The raw ClientHello has been recorded. Stop the per record message callbacks.
+ SSL_set_msg_callback(const_cast<SSL *>(ssl), nullptr);
Review Comment:
ATS permits TLS 1.2 client renegotiation when `proxy.config.ssl.allow_client_renegotiation` is enabled. Disabling this callback after the initial handshake means a later ClientHello is not recorded, while `_has_raw_extension_types` remains true; the renegotiation hook therefore exposes the first ClientHello's extension list. Keep the message callback active for connections on which renegotiation is allowed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#discussion_r4238420411
##########
tests/gold_tests/pluginTest/jax_fingerprint/jax_fingerprint_raw_extensions.test.py:
##########
@@ -0,0 +1,65 @@
+'''
+Verify that jax_fingerprint JA4 counts every ClientHello extension on the wire.
+
+OpenSSL's ClientHello API omits GREASE and extension types it does not
+recognize, such as ALPS. JA4 must still count and hash them.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import sys
+
+sys.path.insert(0, Test.TestDirectory)
+from chrome_client_hello import expected_ja4
+
+Test.Summary = __doc__
+Test.SkipUnless(Condition.PluginExists('jax_fingerprint.so'))
+
+# Wireshark's JA4 for this Chrome ClientHello: 15 non-GREASE ciphers and 16
+# non-GREASE extensions, including ALPS.
+CHROME_JA4 = 't13d1516h2_8daaf6152771_e5627efa2ab1'
+assert expected_ja4() == CHROME_JA4, expected_ja4()
+
+ts = Test.MakeATSProcess('ts', enable_tls=True)
+ts.addDefaultSSLFiles()
+ts.Disk.ssl_multicert_yaml.AddLines(
+ '''
+ssl_multicert:
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+'''.split('\n'))
+ts.Disk.records_config.update(
+ {
+ 'proxy.config.ssl.server.cert.path': ts.Variables.SSLDir,
+ 'proxy.config.ssl.server.private_key.path': ts.Variables.SSLDir,
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'jax_fingerprint',
+ })
+ts.Disk.plugin_config.AddLine('jax_fingerprint.so --method JA4')
+ts.Disk.traffic_out.Content += Testers.ContainsExpression(
+ f'Fingerprint: {CHROME_JA4}', 'JA4 should count GREASE and ALPS as they appear on the wire.')
Review Comment:
Fixed: the message now says JA4 should count the ALPS extension, which OpenSSL does not recognize. I corrected the test docstring the same way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#discussion_r4262983014
##########
src/iocore/net/unit_tests/test_TLSSNISupport.cc:
##########
@@ -0,0 +1,226 @@
+/** @file
+
+ Catch based unit tests for parsing extension types from a raw ClientHello.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "tscore/ink_inet.h"
+#include "iocore/net/TLSSNISupport.h"
+
+#include <catch2/catch_test_macros.hpp>
+
+#include <cstdint>
+#include <vector>
+
+namespace
+{
+
+struct Extension {
+ uint16_t type;
+ std::vector<uint8_t> data;
+};
+
+void
+put_u16(std::vector<uint8_t> &out, size_t value)
+{
+ out.push_back(static_cast<uint8_t>(value >> 8));
+ out.push_back(static_cast<uint8_t>(value));
+}
+
+// The lengths of the variable length fields that precede the extensions.
+struct Lengths {
+ size_t session_id = 32;
+ size_t cipher_suites = 4;
+ size_t compression_methods = 1;
+};
+
+std::vector<uint8_t>
+make_client_hello_body(std::vector<Extension> const &extensions, bool with_extensions_block = true, Lengths lengths = {})
+{
+ std::vector<uint8_t> body;
+
+ put_u16(body, 0x0303);
+ body.insert(body.end(), 32, 0xAB);
+
+ body.push_back(static_cast<uint8_t>(lengths.session_id));
+ body.insert(body.end(), lengths.session_id, 0xCD);
+
+ // By default, GREASE and TLS_AES_128_GCM_SHA256.
+ put_u16(body, lengths.cipher_suites);
+ for (size_t i = 0; i < lengths.cipher_suites; ++i) {
+ body.push_back(i < 2 ? 0x0A : (i % 2 == 0 ? 0x13 : 0x01));
+ }
+
+ // By default, the null compression method.
+ body.push_back(static_cast<uint8_t>(lengths.compression_methods));
+ body.insert(body.end(), lengths.compression_methods, 0);
+
+ if (with_extensions_block) {
+ std::vector<uint8_t> block;
+ for (auto const &ext : extensions) {
+ put_u16(block, ext.type);
+ put_u16(block, ext.data.size());
+ block.insert(block.end(), ext.data.begin(), ext.data.end());
+ }
+ put_u16(body, block.size());
+ body.insert(body.end(), block.begin(), block.end());
+ }
+ return body;
+}
+
+std::vector<uint8_t>
+make_handshake_message(std::vector<uint8_t> const &body)
+{
+ std::vector<uint8_t> msg{SSL3_MT_CLIENT_HELLO, static_cast<uint8_t>(body.size() >> 16), static_cast<uint8_t>(body.size() >> 8),
+ static_cast<uint8_t>(body.size())};
+ msg.insert(msg.end(), body.begin(), body.end());
+ return msg;
+}
+
+std::vector<int>
+parse(std::vector<uint8_t> const &msg, bool expect_success = true)
+{
+ std::vector<int> types{-1};
+ bool const parsed = TLSSNISupport::parse_client_hello_extension_types(msg.data(), msg.size(), types);
+ REQUIRE(parsed == expect_success);
+ return types;
+}
+
+} // namespace
+
+TEST_CASE("Raw ClientHello extension types are listed in wire order", "[TLSSNISupport]")
+{
+ // GREASE, server_name, ALPS (0x4469), ECH (0xfe0d), an empty extension, and GREASE again.
+ std::vector<Extension> const extensions{
+ {0x1A1A, {} },
+ {0x0000, {0x00, 0x00} },
+ {0x4469, {0x00, 0x03, 0x02, 'h'}},
+ {0xFE0D, {0x01, 0x02, 0x03} },
+ {0x0017, {} },
+ {0xCACA, {0x00} },
+ };
+
+ auto const types = parse(make_handshake_message(make_client_hello_body(extensions)));
+ CHECK(types == std::vector<int>{0x1A1A, 0x0000, 0x4469, 0xFE0D, 0x0017, 0xCACA});
+}
+
+TEST_CASE("Raw ClientHello duplicate extension types are preserved", "[TLSSNISupport]")
+{
+ auto const types = parse(make_handshake_message(make_client_hello_body({
+ {0x000A, {}},
+ {0x000A, {}},
+ })));
+ CHECK(types == std::vector<int>{0x000A, 0x000A});
+}
+
+TEST_CASE("Raw ClientHello without an extensions block", "[TLSSNISupport]")
+{
+ auto const types = parse(make_handshake_message(make_client_hello_body({}, false)));
+ CHECK(types.empty());
+}
+
+TEST_CASE("Raw ClientHello with an empty extensions block", "[TLSSNISupport]")
+{
+ auto const types = parse(make_handshake_message(make_client_hello_body({})));
+ CHECK(types.empty());
+}
+
+TEST_CASE("Malformed raw ClientHellos are rejected", "[TLSSNISupport]")
+{
+ std::vector<Extension> const extensions{
+ {0x0000, {0x00, 0x00} },
+ {0x002B, {0x02, 0x03, 0x04}},
+ };
+ auto const good = make_handshake_message(make_client_hello_body(extensions));
+
+ SECTION("empty message")
+ {
+ CHECK(parse({}, false).empty());
+ }
+
+ SECTION("not a ClientHello")
+ {
+ auto msg = good;
+ msg[0] = SSL3_MT_SERVER_HELLO;
+ CHECK(parse(msg, false).empty());
+ }
+
+ SECTION("handshake length longer than the message")
+ {
+ auto msg = good;
+ msg.pop_back();
+ CHECK(parse(msg, false).empty());
+ }
+
+ SECTION("truncated before the extensions block")
+ {
+ auto body = make_client_hello_body({}, false);
+ body.resize(30);
+ CHECK(parse(make_handshake_message(body), false).empty());
+ }
+
+ SECTION("legacy session id longer than 32 bytes")
+ {
+ CHECK(parse(make_handshake_message(make_client_hello_body(extensions, true, {.session_id = 33})), false).empty());
+ }
+
+ SECTION("no cipher suites")
+ {
+ CHECK(parse(make_handshake_message(make_client_hello_body(extensions, true, {.cipher_suites = 0})), false).empty());
+ }
+
+ SECTION("odd cipher suites length")
+ {
+ CHECK(parse(make_handshake_message(make_client_hello_body(extensions, true, {.cipher_suites = 3})), false).empty());
+ }
+
+ SECTION("no compression methods")
+ {
+ CHECK(parse(make_handshake_message(make_client_hello_body(extensions, true, {.compression_methods = 0})), false).empty());
+ }
+
+ SECTION("extension data longer than the extensions block")
+ {
+ auto body = make_client_hello_body(extensions);
+ // Grow the last extension's declared length by one byte.
+ body[body.size() - 4] += 1;
Review Comment:
`body[body.size() - 4]` indexes into the last extension’s *type* field (type_lo), not its declared length, so this mutation won’t necessarily make the extensions block inconsistent and can cause the test to pass parsing when it should fail. Update the test to increment the correct byte(s) of the last extension length field (i.e., the length bytes immediately preceding the last extension’s data). This is required because it can make this unit test section fail to validate the intended error case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub) via github" <gi...@trafficserver.apache.org>.
bneradt commented on code in PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#discussion_r4482915088
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -595,6 +595,24 @@ ssl_client_hello_callback(const SSL_CLIENT_HELLO *client_hello)
return CLIENT_HELLO_SUCCESS;
}
+#if HAVE_SSL_CTX_SET_CLIENT_HELLO_CB
+// OpenSSL's ClientHello API drops extensions it doesn't recognize, including
+// GREASE. Record the extension types from the raw ClientHello message, which
+// this callback receives before ssl_client_hello_callback runs.
+static void
+ssl_msg_callback(int write_p, int /* version ATS_UNUSED */, int content_type, const void *buf, size_t len, SSL *ssl,
+ void * /* arg ATS_UNUSED */)
+{
+ auto const *msg = static_cast<const uint8_t *>(buf);
+ if (write_p || content_type != SSL3_RT_HANDSHAKE || len == 0 || msg[0] != SSL3_MT_CLIENT_HELLO) {
+ return;
+ }
+ if (TLSSNISupport *snis = TLSSNISupport::getInstance(ssl); snis != nullptr) {
+ snis->on_client_hello_message(msg, len);
+ }
Review Comment:
OpenSSL calls the message callback for a handshake message from `tls_get_message_body()`, after it has reassembled the whole message, header included, so the callback never sees a record-level fragment. `test_jax_fingerprint_raw_extensions.py` covers this: it also sends the ClientHello split across 100-byte records, then checks that both fingerprints include ALPS and that `client_hello_raw_parse_failure` is still 0. No change.
##########
tests/uranium_tests/pluginTest/jax_fingerprint/chrome_client_hello.py:
##########
@@ -0,0 +1,169 @@
+#!/usr/bin/env python3
+'''
+Send a Chrome-shaped TLS ClientHello and report the JA4 fingerprint it should produce.
+
+The ClientHello carries GREASE cipher suites and extensions as well as the
+ALPS (0x4469) extension, which OpenSSL does not recognize. A fingerprint
+computed from the extensions OpenSSL reports would miss all of these.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import argparse
+import hashlib
+import os
+import socket
+import struct
+import sys
+
+SERVER_NAME = b'jax.server.test'
+
+GREASE_CIPHER = 0x0A0A
+CIPHERS = [
+ GREASE_CIPHER, 0x1301, 0x1302, 0x1303, 0xC02B, 0xC02F, 0xC02C, 0xC030, 0xCCA9, 0xCCA8, 0xC013, 0xC014, 0x009C, 0x009D, 0x002F,
+ 0x0035
+]
+
+SIGNATURE_ALGORITHMS = [0x0403, 0x0804, 0x0401, 0x0503, 0x0805, 0x0501, 0x0806, 0x0601]
+
+EXT_SERVER_NAME = 0x0000
+EXT_ALPN = 0x0010
+EXT_ALPS = 0x4469
+FIRST_GREASE_EXTENSION = 0x2A2A
+LAST_GREASE_EXTENSION = 0x9A9A
+
+
+def is_grease(value: int) -> bool:
+ """Identify a reserved GREASE value.
+
+ :param value: Unsigned 16-bit cipher or extension identifier.
+ """
+ return (value & 0x0F0F) == 0x0A0A and (value >> 8) == (value & 0xFF)
+
+
+def u8_prefixed(data: bytes) -> bytes:
+ """Prefix bytes with their one-byte length.
+
+ :param data: Payload of at most 255 bytes.
+ """
+ return struct.pack('!B', len(data)) + data
+
+
+def u16_prefixed(data: bytes) -> bytes:
+ """Prefix bytes with their network-order two-byte length.
+
+ :param data: Payload of at most 65535 bytes.
+ """
+ return struct.pack('!H', len(data)) + data
+
+
+def u16_list(values: list[int]) -> bytes:
+ """Encode a list of network-order unsigned integers.
+
+ :param values: Unsigned 16-bit identifiers to concatenate.
+ """
+ return b''.join(struct.pack('!H', v) for v in values)
+
+
+def extensions() -> list[tuple[int, bytes]]:
+ '''Return the ClientHello extensions, in Chrome's wire order.'''
+ server_name = u16_prefixed(b'\x00' + u16_prefixed(SERVER_NAME))
+ alpn = u16_prefixed(u8_prefixed(b'h2') + u8_prefixed(b'http/1.1'))
+ groups = u16_prefixed(u16_list([0x1A1A, 0x001D, 0x0017, 0x0018]))
+ key_share = u16_prefixed(u16_list([0x1A1A]) + u16_prefixed(b'\x00') + u16_list([0x001D]) + u16_prefixed(os.urandom(32)))
+ supported_versions = u8_prefixed(u16_list([0x3A3A, 0x0304, 0x0303]))
+ return [
+ (FIRST_GREASE_EXTENSION, b''),
+ (EXT_SERVER_NAME, server_name),
+ (0x0017, b''), # extended_master_secret
+ (0xFF01, b'\x00'), # renegotiation_info
+ (0x000A, groups), # supported_groups
+ (0x000B, u8_prefixed(b'\x00')), # ec_point_formats
+ (0x0023, b''), # session_ticket
+ (EXT_ALPN, alpn),
+ (0x0005, b'\x01\x00\x00\x00\x00'), # status_request
+ (0x000D, u16_prefixed(u16_list(SIGNATURE_ALGORITHMS))), # signature_algorithms
+ (0x0012, b''), # signed_certificate_timestamp
+ (0x0033, key_share), # key_share
+ (0x002D, u8_prefixed(b'\x01')), # psk_key_exchange_modes
+ (0x002B, supported_versions), # supported_versions
+ (0x001B, u8_prefixed(u16_list([0x0002]))), # compress_certificate
+ (EXT_ALPS, u16_prefixed(u8_prefixed(b'h2'))), # application_settings
+ (LAST_GREASE_EXTENSION, b'\x00'),
+ (0x0015, bytes(16)), # padding
+ ]
+
+
+def client_hello_records(fragment_size: int = 0) -> bytes:
+ '''Return TLS records holding the ClientHello.
+
+ :param fragment_size: When non-zero, split the ClientHello across records
+ of at most this many bytes.
+ '''
+ body = struct.pack('!H', 0x0303) + os.urandom(32) + u8_prefixed(os.urandom(32))
+ body += u16_prefixed(u16_list(CIPHERS))
+ body += u8_prefixed(b'\x00')
+ body += u16_prefixed(b''.join(struct.pack('!HH', t, len(d)) + d for t, d in extensions()))
+ handshake = b'\x01' + struct.pack('!I', len(body))[1:] + body
+ size = fragment_size or len(handshake)
+ fragments = [handshake[i:i + size] for i in range(0, len(handshake), size)]
+ return b''.join(struct.pack('!BHH', 0x16, 0x0301, len(f)) + f for f in fragments)
+
+
+def truncated_sha256(values: list[str]) -> str:
+ """Hash comma-separated fields using JA4's truncated SHA-256 format.
+
+ :param values: Ordered hexadecimal fingerprint fields.
+ """
+ return hashlib.sha256(','.join(values).encode()).hexdigest()[:12]
+
+
+def expected_ja4() -> str:
+ '''Return the JA4 fingerprint computed from every extension on the wire.
+
+ The c section hashes the signature algorithms per the JA4 specification.
+ '''
+ ciphers = [c for c in CIPHERS if not is_grease(c)]
+ types = [t for t, _ in extensions() if not is_grease(t)]
+ hashed_types = [t for t in types if t not in (EXT_SERVER_NAME, EXT_ALPN)]
+
+ part_a = f't13d{len(ciphers):02d}{len(types):02d}h2'
+ part_b = truncated_sha256(sorted(f'{c:04x}' for c in ciphers))
+ sigalgs = ','.join(f'{a:04x}' for a in SIGNATURE_ALGORITHMS)
+ part_c = hashlib.sha256(f"{','.join(sorted(f'{t:04x}' for t in hashed_types))}_{sigalgs}".encode()).hexdigest()[:12]
+ return f'{part_a}_{part_b}_{part_c}'
+
+
+def main() -> int:
+ parser = argparse.ArgumentParser(description=__doc__)
+ parser.add_argument('port', type=int, help='The TLS port to send the ClientHello to.')
+ parser.add_argument(
+ '--fragment-size', type=int, default=0, help='Split the ClientHello across records of at most this many bytes.')
+ args = parser.parse_args()
+
+ with socket.create_connection(('127.0.0.1', args.port), timeout=5) as sock:
+ sock.sendall(client_hello_records(args.fragment_size))
+ # Wait for the ServerHello so that the ClientHello has been processed.
+ response = sock.recv(5)
Review Comment:
The fingerprint is logged from the ClientHello hook, before ATS sends the ServerHello, so it is in the log before the client reads the first response byte. The test waits for both fingerprint lines rather than racing them, and the client closing mid-handshake does not produce an ERROR or FATAL line in diags.log, which is what the test checks. No change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6070808527
Thanks for the rework. One problem: `_raw_extension_types` leaks on OpenSSL.
`SSLNetVConnection` is allocated from `ClassAllocator<SSLNetVConnection, false>`, so its destructor never runs on free. Every allocation placement-news a fresh object over the recycled memory. `TLSSNISupport::_clear()` resets `hints_from_sni` and `_sni_server_name` but not the new vector. `bind()` calls `clear()`, which keeps the capacity, but the next placement-new overwrites the vector's pointer. So every inbound TLS handshake on OpenSSL leaks the vector's buffer, whether or not a plugin reads the list.
Could `_clear()` release it too, the same way it resets `_sni_server_name`? For example:
```cpp
_raw_extension_types = {};
_has_raw_extension_types = false;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#discussion_r4262112299
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -248,6 +248,24 @@ ssl_client_hello_callback(const SSL_CLIENT_HELLO *client_hello)
return CLIENT_HELLO_SUCCESS;
}
+#if HAVE_SSL_CTX_SET_CLIENT_HELLO_CB
+// OpenSSL's ClientHello API drops extensions it doesn't recognize, including
+// GREASE. Record the extension types from the raw ClientHello message, which
+// this callback receives before ssl_client_hello_callback runs.
+static void
+ssl_msg_callback(int write_p, int /* version ATS_UNUSED */, int content_type, const void *buf, size_t len, SSL *ssl,
+ void * /* arg ATS_UNUSED */)
+{
+ auto const *msg = static_cast<const uint8_t *>(buf);
+ if (write_p || content_type != SSL3_RT_HANDSHAKE || len == 0 || msg[0] != SSL3_MT_CLIENT_HELLO) {
+ return;
+ }
+ if (TLSSNISupport *snis = TLSSNISupport::getInstance(ssl); snis != nullptr) {
+ snis->on_client_hello_message(msg, len);
+ }
Review Comment:
This assumes the msg callback receives a complete ClientHello handshake message starting at `SSL3_MT_CLIENT_HELLO`. In OpenSSL, `SSL_CTX_set_msg_callback` is commonly invoked with TLS record payloads (which may contain partial handshake messages, a handshake message split across records, or multiple handshake messages in one record). In those cases `on_client_hello_message(msg, len)` will frequently fail to parse (and permanently lose the ClientHello types), producing incorrect fingerprints and incrementing `client_hello_raw_parse_failure` for valid handshakes.\n\nFix: parse the handshake framing robustly in the callback by (a) iterating over all handshake messages in the provided buffer (type + 3-byte length + body), and (b) buffering across callbacks when a ClientHello is fragmented. A simple approach is to store an in-progress buffer per connection (e.g., on TLSSNISupport) until the full ClientHello length is available, then parse and optionally disable further msg callbacks fo
r that SSL once captured.
##########
src/iocore/net/TLSSNISupport.cc:
##########
@@ -138,6 +142,84 @@ TLSSNISupport::on_client_hello(ClientHello &client_hello)
}
}
+void
+TLSSNISupport::on_client_hello_message(const uint8_t *msg, size_t len)
+{
+ _has_raw_extension_types = parse_client_hello_extension_types(msg, len, _raw_extension_types);
+ if (!_has_raw_extension_types) {
+ Metrics::Counter::increment(ssl_rsb.client_hello_raw_parse_failure);
+ Dbg(dbg_ctl_ssl_sni, "Could not parse the extensions of the raw ClientHello");
+ }
+}
+
+bool
+TLSSNISupport::parse_client_hello_extension_types(const uint8_t *msg, size_t msg_len, std::vector<int> &types)
+{
+ types.clear();
+
+ size_t offset = 0;
+ size_t end = 0;
+
+ auto read_u8 = [&](size_t &value) {
+ if (offset + 1 > end) {
+ return false;
+ }
+ value = msg[offset];
+ offset += 1;
+ return true;
+ };
+ auto read_u16 = [&](size_t &value) {
+ if (offset + 2 > end) {
+ return false;
+ }
+ value = (msg[offset] << 8) | msg[offset + 1];
+ offset += 2;
+ return true;
+ };
+ auto skip = [&](size_t n) {
+ if (offset + n > end) {
+ return false;
+ }
+ offset += n;
+ return true;
+ };
+
+ // Handshake header: msg_type(1) length(3).
+ if (msg_len < 4 || msg[0] != SSL3_MT_CLIENT_HELLO) {
+ return false;
+ }
+ size_t const body_len = (msg[1] << 16) | (msg[2] << 8) | msg[3];
+ if (4 + body_len > msg_len) {
+ return false;
+ }
+ offset = 4;
+ end = 4 + body_len;
+
+ // legacy_version(2) random(32) session_id<0..32> cipher_suites<2..2^16-2> compression_methods<1..2^8-1>
+ size_t len = 0;
+ if (!skip(2 + 32) || !read_u8(len) || !skip(len) || !read_u16(len) || !skip(len) || !read_u8(len) || !skip(len)) {
+ return false;
+ }
+
Review Comment:
The parser claims to accept only a “well formed ClientHello” (per header/docs), but it currently accepts several structurally invalid values as successful parses, which can lead to returning extension types for malformed ClientHellos (and potentially masking real parse failures). Concrete gaps here:\n- `session_id` length is not validated to be <= 32.\n- `cipher_suites` length is not validated to be >= 2 and an even number of bytes.\n- `compression_methods` length is not validated to be >= 1.\n\nFix: add explicit checks for these TLS ClientHello invariants before `skip(len)` calls. Also add unit tests covering these malformed-length cases (e.g., odd cipher_suites length, compression_methods length 0, session_id length 33) to ensure `parse_client_hello_extension_types()` returns false.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "maskit (via GitHub) via github" <gi...@trafficserver.apache.org>.
maskit commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6783101923
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub) via github" <gi...@trafficserver.apache.org>.
bneradt merged PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bryancall (via GitHub) via github" <gi...@trafficserver.apache.org>.
bryancall commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6748851143
[approve ci doxygen]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] List every ClientHello extension type on OpenSSL [trafficserver]
Posted by "bneradt (via GitHub) via github" <gi...@trafficserver.apache.org>.
bneradt commented on PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#issuecomment-6672339850
[approve ci urtest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org