Copilot commented on code in PR #12995:
URL: https://github.com/apache/trafficserver/pull/12995#discussion_r2969239952


##########
plugins/experimental/jax_fingerprint/ja4h/datasource.cc:
##########
@@ -0,0 +1,43 @@
+/** @file
+
+  @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 "datasource.h"
+
+#include <cctype>
+
+bool
+Datasource::_should_include_field(std::string_view name)
+{
+  constexpr std::string_view COOKIE{"cookie"};
+  constexpr std::string_view REFERER{"referer"};
+
+  if (name.length() == COOKIE.length()) {
+    if (std::equal(name.begin(), name.end(), COOKIE.begin(), [](char c1, char 
c2) { return std::tolower(c1) == c2; })) {
+      return false;
+    }
+  } else if (name.length() == REFERER.length()) {
+    if (std::equal(name.begin(), name.end(), REFERER.begin(), [](char c1, char 
c2) { return std::tolower(c1) == c2; })) {
+      return false;

Review Comment:
   `Datasource::_should_include_field()` uses `std::equal()` but this 
translation unit doesn't include `<algorithm>`, which can break compilation on 
toolchains that don't provide it transitively. Add the missing include (and 
consider casting to `unsigned char` before `std::tolower()` to avoid undefined 
behavior with signed `char`).



##########
doc/admin-guide/plugins/jax_fingerprint.en.rst:
##########
@@ -0,0 +1,182 @@
+.. 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:: ../../common.defs
+
+.. _admin-plugins-jax-fingerprint:
+
+JAx Fingerprint Plugin
+**********************
+
+Description
+===========
+
+The JAx Fingerprint plugin generates client fingerprints based on the JA4+ or 
JA3 algorithms designed by John Althouse.
+
+Fingerprints can be used for:
+
+* Client identification and tracking
+* Bot detection and mitigation
+* Security analytics and threat intelligence
+* Understanding client implementation patterns
+
+
+Plugin Configuration
+====================
+
+You can use the plugin as a global plugin, a remap plugin, or both.
+
+To use the plugin as a global plugin, add the following line to 
:file:`plugin.config`::
+
+    jax_fingerprint.so --standalone
+
+To use the plugin as a remap plugin, append the following line to a remap rule 
on :file:`remap.config`::
+
+    @plugin=jax_fingerprint.so @pparam=--standalone
+
+To use the plugin in a hybrid setup (both global and remap plugin), configure 
it in both :file:`plugin.config` and
+:file:`remap.config` without ``--standalone`` option.

Review Comment:
   The quick-start config examples omit the required `--method` option (e.g., 
`jax_fingerprint.so --standalone` and `@pparam=--standalone`), but the plugin 
errors out if `--method` isn't specified. Update these examples to include an 
explicit method so readers can copy/paste working configs.
   ```suggestion
       jax_fingerprint.so --standalone --method JA4
   
   To use the plugin as a remap plugin, append the following line to a remap 
rule on :file:`remap.config`::
   
       @plugin=jax_fingerprint.so @pparam=--standalone @pparam=--method 
@pparam=JA4
   
   To use the plugin in a hybrid setup (both global and remap plugin), 
configure it in both :file:`plugin.config` and
   :file:`remap.config` without the ``--standalone`` option but still 
specifying a ``--method``.
   ```



##########
plugins/experimental/jax_fingerprint/ja4h/ja4h_method.cc:
##########
@@ -0,0 +1,173 @@
+/** @file
+
+  @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 "ts/ts.h"
+
+#include "../plugin.h"
+#include "../context.h"
+#include "ja4h_method.h"
+#include "ja4h.h"
+#include "datasource.h"
+
+#include "openssl/sha.h"
+
+namespace ja4h_method
+{
+void on_request(JAxContext *, TSHttpTxn);
+
+struct Method method = {
+  "JA4H",
+  Method::Type::REQUEST_BASED,
+  nullptr,
+  on_request,
+};
+
+class TxnDatasource : public Datasource
+{
+public:
+  TxnDatasource(TSHttpTxn txnp);
+  ~TxnDatasource();
+  std::string_view get_method() override;
+  int              get_version() override;
+  bool             has_cookie_field() override;
+  bool             has_referer_field() override;
+  int              get_field_count() override;
+  std::string_view get_accept_language() override;
+  void             get_headers_hash(unsigned char out[32]) override;
+
+private:
+  TSHttpTxn _txn;
+  TSMBuffer _request = nullptr;
+  TSMLoc    _req_hdr = nullptr;
+};
+
+TxnDatasource::TxnDatasource(TSHttpTxn txnp) : _txn(txnp)
+{
+  TSHttpTxnClientReqGet(txnp, &(this->_request), &(this->_req_hdr));
+}
+
+TxnDatasource::~TxnDatasource()
+{
+  if (this->_request != nullptr) {
+    TSHandleMLocRelease(this->_request, TS_NULL_MLOC, this->_req_hdr);
+  }
+}
+
+std::string_view
+TxnDatasource::get_method()
+{
+  if (this->_request == nullptr) {
+    return "";
+  }
+
+  int         method_len;
+  const char *method = TSHttpHdrMethodGet(this->_request, this->_req_hdr, 
&method_len);
+
+  return {method, static_cast<size_t>(method_len)};
+}
+
+int
+TxnDatasource::get_version()
+{
+  if (TSHttpTxnClientProtocolStackContains(this->_txn, "h2")) {
+    return 2 << 16;
+  } else if (TSHttpTxnClientProtocolStackContains(this->_txn, "h3")) {
+    return 3 << 16;
+  } else {
+    return TSHttpHdrVersionGet(this->_request, this->_req_hdr);
+  }
+}
+
+bool
+TxnDatasource::has_cookie_field()
+{
+  TSMLoc mloc = TSMimeHdrFieldFind(this->_request, this->_req_hdr, 
TS_MIME_FIELD_COOKIE, TS_MIME_LEN_COOKIE);
+  if (mloc) {
+    TSHandleMLocRelease(this->_request, this->_req_hdr, mloc);
+  }
+  return mloc != TS_NULL_MLOC;

Review Comment:
   `TxnDatasource` ignores the return value of `TSHttpTxnClientReqGet()`, but 
most member functions unconditionally dereference `_request`/`_req_hdr` (e.g., 
`get_version()`, `has_cookie_field()`, `get_headers_hash()`). If 
`TSHttpTxnClientReqGet()` fails, this becomes a null dereference. Capture and 
check the return code in the constructor, and have all accessors short-circuit 
safely when request headers aren't available.



##########
plugins/experimental/jax_fingerprint/plugin.cc:
##########
@@ -0,0 +1,480 @@
+/** @file
+
+  @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 "plugin.h"
+#include "config.h"
+#include "context.h"
+#include "userarg.h"
+#include "method.h"
+#include "header.h"
+#include "log.h"
+
+#include "ja4/ja4_method.h"
+#include "ja4h/ja4h_method.h"
+#include "ja3/ja3_method.h"
+
+#include <ts/apidefs.h>
+#include <ts/ts.h>
+#include <ts/remap.h>
+#include <ts/remap_version.h>
+
+#include <getopt.h>
+
+#include <cstddef>
+#include <cstdint>
+#include <cstdio>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <string_view>
+#include <version>
+
+DbgCtl dbg_ctl{PLUGIN_NAME};
+
+namespace
+{
+
+} // end anonymous namespace
+
+static bool
+read_config_option(int argc, char const *argv[], PluginConfig &config)
+{
+  const struct option longopts[] = {
+    {"standalone",   no_argument,       nullptr, 's'},
+    {"method",       required_argument, nullptr, 'M'}, // JA4, JA4H, or JA3
+    {"mode",         required_argument, nullptr, 'm'}, // overwrite, keep, or 
append
+    {"header",       required_argument, nullptr, 'h'},
+    {"via-header",   required_argument, nullptr, 'v'},
+    {"log-filename", required_argument, nullptr, 'f'},
+    {"servernames",  required_argument, nullptr, 'S'},
+    {nullptr,        0,                 nullptr, 0  }
+  };
+
+  optind = 0;
+  int opt{0};
+  while ((opt = getopt_long(argc, const_cast<char *const *>(argv), "", 
longopts, nullptr)) >= 0) {
+    switch (opt) {
+    case '?':
+      Dbg(dbg_ctl, "Unrecognized command argument.");
+      break;
+    case 'M':
+      if (strcmp("JA4", optarg) == 0) {
+        config.method = ja4_method::method;
+      } else if (strcmp("JA4H", optarg) == 0) {
+        config.method = ja4h_method::method;
+      } else if (strcmp("JA3", optarg) == 0) {
+        config.method = ja3_method::method;
+      } else {
+        Dbg(dbg_ctl, "Unexpected method: %s", optarg);
+        return false;
+      }
+      break;
+    case 'm':
+      if (strcmp("overwrite", optarg) == 0) {
+        config.mode = Mode::OVERWRITE;
+      } else if (strcmp("keep", optarg) == 0) {
+        config.mode = Mode::KEEP;
+      } else if (strcmp("append", optarg) == 0) {
+        config.mode = Mode::APPEND;
+      } else {
+        Dbg(dbg_ctl, "Unexpected mode: %s", optarg);
+        return false;
+      }
+      break;
+    case 'h':
+      config.header_name = {optarg, strlen(optarg)};
+      break;
+    case 'v':
+      config.via_header_name = {optarg, strlen(optarg)};
+      break;
+    case 'f':
+      config.log_filename = {optarg, strlen(optarg)};
+      break;
+    case 's':
+      config.standalone = true;
+      break;
+    case 'S':
+      for (std::string_view input(optarg, strlen(optarg)); !input.empty();) {
+        auto pos = input.find(',');
+        config.servernames.emplace(input.substr(0, pos));
+        input.remove_prefix(pos == std::string_view::npos ? input.size() : pos 
+ 1);
+      }
+      break;
+    case 0:
+    case -1:
+      break;
+    default:
+      Dbg(dbg_ctl, "Unexpected options error.");
+      return false;
+    }
+  }
+
+  if (strcmp(config.method.name, "uninitialized") == 0) {
+    TSError("[%s] Method must be specified", PLUGIN_NAME);
+    return false;
+  }
+
+  Dbg(dbg_ctl, "JAx method is %s", config.method.name);
+  Dbg(dbg_ctl, "JAx mode is %d", static_cast<int>(config.mode));
+  Dbg(dbg_ctl, "JAx header is %s", !config.header_name.empty() ? 
config.header_name.c_str() : "DISABLED");
+  Dbg(dbg_ctl, "JAx via-header is %s", !config.via_header_name.empty() ? 
config.via_header_name.c_str() : "DISABLED");
+  Dbg(dbg_ctl, "JAx log file is %s", !config.log_filename.empty() ? 
config.log_filename.c_str() : "DISABLED");
+  Dbg(dbg_ctl, "JAx standalone mode  is %s", config.standalone ? "ENABLED" : 
"DISABLED");
+  for (auto &&servername : config.servernames) {
+    Dbg(dbg_ctl, "%s", servername.c_str());
+  }
+
+  return true;
+}
+
+void
+modify_headers(JAxContext *ctx, TSHttpTxn txnp, PluginConfig &config)
+{
+  if (!ctx->get_fingerprint().empty()) {
+    switch (config.mode) {
+    case Mode::KEEP:
+      if (!config.header_name.empty() && !has_header(txnp, 
config.header_name)) {
+        set_header(txnp, config.header_name, ctx->get_fingerprint());
+      }
+      if (!config.via_header_name.empty() && !has_header(txnp, 
config.via_header_name)) {
+        set_via_header(txnp, config.via_header_name);
+      }
+      break;
+    case Mode::OVERWRITE:
+      if (!config.header_name.empty()) {
+        set_header(txnp, config.header_name, ctx->get_fingerprint());
+      }
+      if (!config.via_header_name.empty()) {
+        set_via_header(txnp, config.via_header_name);
+      }
+      break;
+    case Mode::APPEND:
+      if (!config.header_name.empty()) {
+        append_header(txnp, config.header_name, ctx->get_fingerprint());
+      }
+      if (!config.via_header_name.empty()) {
+        append_via_header(txnp, config.via_header_name);
+      }
+      break;
+    default:
+      break;
+    }
+  } else {
+    Dbg(dbg_ctl, "No fingerprint attached to vconn!");
+    if (config.mode == Mode::OVERWRITE) {
+      if (!config.header_name.empty()) {
+        remove_header(txnp, config.header_name);
+      }
+      if (!config.via_header_name.empty()) {
+        remove_header(txnp, config.via_header_name);
+      }
+    }
+  }
+}
+
+int
+handle_client_hello(void *edata, PluginConfig &config)
+{
+  TSVConn     vconn = static_cast<TSVConn>(edata);
+  JAxContext *ctx   = get_user_arg(vconn, config);
+
+  if (!config.servernames.empty()) {
+    const char *servername;
+    int         servername_len;
+    servername = TSVConnSslSniGet(vconn, &servername_len);
+    if (servername != nullptr && servername_len > 0) {
+#ifdef __cpp_lib_generic_unordered_lookup
+      if (!config.servernames.contains(std::string_view(servername, 
servername_len))) {
+#else
+      if (!config.servernames.contains({servername, 
static_cast<size_t>(servername_len)})) {
+#endif
+        Dbg(dbg_ctl, "Server name %.*s is not in the server name set", 
servername_len, servername);
+        TSVConnReenable(vconn);
+        return TS_SUCCESS;
+      }
+    } else {
+      Dbg(dbg_ctl, "No SNI present but server name filtering is configured; 
skipping fingerprint generation");
+      TSVConnReenable(vconn);
+      return TS_SUCCESS;
+    }
+  }
+
+  if (nullptr == ctx) {
+    ctx = new JAxContext(config.method.name, TSNetVConnRemoteAddrGet(vconn));
+    set_user_arg(vconn, config, ctx);
+  }
+
+  if (config.method.on_client_hello) {
+    config.method.on_client_hello(ctx, vconn);
+  }
+
+  TSVConnReenable(vconn);
+
+  return TS_SUCCESS;
+}
+
+int
+handle_read_request_hdr(void *edata, PluginConfig &config)
+{
+  TSHttpTxn txnp = static_cast<TSHttpTxn>(edata);
+  TSHttpSsn ssnp = TSHttpTxnSsnGet(txnp);
+  if (ssnp == nullptr) {
+    Dbg(dbg_ctl, "Failed to get ssn object.");
+    return TS_SUCCESS;
+  }
+
+  TSVConn vconn = TSHttpSsnClientVConnGet(ssnp);
+  if (vconn == nullptr) {
+    Dbg(dbg_ctl, "Failed to get vconn object.");
+    return TS_SUCCESS;
+  }
+

Review Comment:
   `--servernames` filtering is only enforced in `handle_client_hello()`. For 
request-based methods (JA4H) no SSL client-hello hook is installed, so the 
configured servername allow-list is effectively ignored and 
fingerprints/headers/logging will still be generated for any SNI. Consider 
applying the same SNI check in `handle_read_request_hdr()` (you already have 
`vconn` there via `TSHttpSsnClientVConnGet`).



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to