ja4_fingerprint: Add an option to disable logging#12938
ja4_fingerprint: Add an option to disable logging#12938maskit wants to merge 1 commit intoapache:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a --nologging runtime option to the experimental ja4_fingerprint plugin so operators can disable creation/writing of the plugin’s text log file, and updates the admin documentation to reflect available options.
Changes:
- Add
--nologgingoption parsing and conditional log file creation / write behavior in the plugin. - Move the debug (
Dbg) fingerprint message out of the file-log write helper so it always logs when debug tags are enabled. - Update plugin documentation to describe
--preserveand--nologging, and adjust logging description/limitations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| plugins/experimental/ja4_fingerprint/plugin.cc | Adds --nologging option and guards log file creation and log_fingerprint() calls. |
| doc/admin-guide/plugins/ja4_fingerprint.en.rst | Documents plugin options and updates logging/limitations text. |
| .. option:: --preserve | ||
|
|
||
| This option controls whether the plugin preserves any existing JA4 header. If the option is specified the plugin keep the header | ||
| intact. If the option is not speficied, the plugins appends a generated fingerprint to the existing header value. |
There was a problem hiding this comment.
Typo in documentation: "speficied" should be "specified".
| intact. If the option is not speficied, the plugins appends a generated fingerprint to the existing header value. | |
| intact. If the option is not specified, the plugins appends a generated fingerprint to the existing header value. |
| This option controls whether the plugin preserves any existing JA4 header. If the option is specified the plugin keep the header | ||
| intact. If the option is not speficied, the plugins appends a generated fingerprint to the existing header value. |
There was a problem hiding this comment.
The --preserve option description has several grammar issues and is hard to read as a single long line (e.g., "the plugin keep", "the plugins appends"). Please fix the wording and wrap the text consistently with the rest of this RST file.
| This option controls whether the plugin preserves any existing JA4 header. If the option is specified the plugin keep the header | |
| intact. If the option is not speficied, the plugins appends a generated fingerprint to the existing header value. | |
| This option controls whether the plugin preserves any existing JA4 header. | |
| If the option is specified, the plugin keeps the header intact. If the | |
| option is not specified, the plugin appends a generated fingerprint to the | |
| existing header value. |
|
|
||
| .. option:: --nologging | ||
|
|
||
| This option disables log output. |
There was a problem hiding this comment.
"This option disables log output" is ambiguous now that debug logging (Dbg) still emits fingerprints when debug tags are enabled. Please clarify that --nologging disables writing to ja4_fingerprint.log (text log), and whether debug output is still expected.
| This option disables log output. | |
| This option disables writing fingerprints to the :file:`ja4_fingerprint.log` text log file. It does not affect Traffic Server debug | |
| logging (``Dbg``); if relevant debug tags are enabled, fingerprints may still appear in debug logs. |
| The plugin writes to ``ja4_fingerprint.log`` in the Traffic Server log | ||
| directory (typically ``/var/log/trafficserver/``). | ||
| directory (typically ``/var/log/trafficserver/``) if the feature is not disabled. |
There was a problem hiding this comment.
"if the feature is not disabled" is unclear—this appears to refer specifically to --nologging. Consider rephrasing to explicitly mention --nologging, and also update earlier sections (e.g., the "Plugin Behavior" bullet that says it always "Log to File") to reflect the conditional logging behavior.
| // The VCONN_CLOSE handler is now responsible for freeing the resource. | ||
| TSUserArgSet(ssl_vc, *get_user_arg_index(), static_cast<void *>(data.release())); | ||
| } |
There was a problem hiding this comment.
TSUserArgSet() stores a JA4_data* on the vconn, but other hooks treat this user arg as a std::string* (see TSUserArgGet() usage in handle_read_request_hdr() and the delete in handle_vconn_close()). Deleting/using a JA4_data* as a std::string* is undefined behavior even if the std::string is the first member. Please make the stored type consistent (e.g., store JA4_data* and update the later casts/deletes, or store only a std::string* and adjust log_fingerprint() accordingly).
| if (!global_nologging_enabled) { | ||
| if (!create_log_file()) { | ||
| TSError("[%s] Failed to create log.", PLUGIN_NAME); | ||
| return; | ||
| } else { | ||
| Dbg(dbg_ctl, "Created log file."); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new --nologging behavior isn't covered by existing tests. Since there are already gold tests for this plugin, please add a test case that runs with ja4_fingerprint.so --nologging and asserts (1) no ja4_fingerprint.log is created/written and (2) JA4 headers are still injected.
|
I also started working on adding options to change header names, and now I feel like it may be nice if the default behavior is different. How about output nothing by default, and have these 3 options to enable each output, instead of introducing It would be an incompatible change though. |
This adds
--nologgingoption to ja4_fingerprint plugin. If it's specified, the plugin does not make a log file.Current documentation says that the plugin does not have any options, but there is
--preserveactually. I added documentation for it as well.