Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JA4 for TLS and QUIC -- v9 #10579

Closed
wants to merge 3 commits into from
Closed

JA4 for TLS and QUIC -- v9 #10579

wants to merge 3 commits into from

Conversation

satta
Copy link
Contributor

@satta satta commented Mar 4, 2024

Previous PR: #10341

Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/6379

Changes to previous PR:

  • Rebase against current master.
  • Removed raw JA4 plans from documentation.
  • Use ja4.ciphersuites.len() instead of separate counter.
  • Added comments for the reasoning behind some tests.
  • Removed stowaway #define carried over from older code.
  • Moved JA4_HEX_LEN into separate header to be used in detect and output code.
  • Reduce number of includes in JA4 detection implementation.
  • Do not allocate an empty JA3 hash when JA3 is disabled.
  • Restructure some code for better readability.
  • Pull out earlier feature tracking initialization into separate commit.
  • Change commit message prefix.
  • Moved JA3/4 enabled check into separate functions to improve readability.
  • Address overflow in ALPN extension parser code.
SV_BRANCH=pr/1682

satta added 2 commits March 4, 2024 19:29
This gives app layer code a chance to access feature
information.
Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 83.37029% with 75 lines in your changes are missing coverage. Please review.

Project coverage is 82.62%. Comparing base (c6c1eac) to head (9c6d91e).
Report is 30 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10579      +/-   ##
==========================================
- Coverage   82.67%   82.62%   -0.05%     
==========================================
  Files         922      927       +5     
  Lines      246969   247803     +834     
==========================================
+ Hits       204179   204753     +574     
- Misses      42790    43050     +260     
Flag Coverage Δ
fuzzcorpus 64.08% <63.27%> (+0.03%) ⬆️
suricata-verify 61.82% <83.14%> (+0.10%) ⬆️
unittests 62.16% <42.57%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@satta
Copy link
Contributor Author

satta commented Mar 7, 2024

Added back the ticket reference in the commit message.

@catenacyber catenacyber added the needs rebase Needs rebase to master label Mar 12, 2024
Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work Sascha :-)

  • CI : needs a rebase to fix the small conflict
  • Code : doing now
  • Commits segmentation : nice
  • Commit messages : ok for me
  • Git ID set : looks fine for me
  • CLA : you already contributed :-)
  • Doc update : Ok
  • Redmine ticket : ok
  • Rustfmt : ok for ja4.rs
  • Tests : Left remarks there
  • Dependencies added: none

@catenacyber
Copy link
Contributor

Compiling with --disable-ja4, I get "ja4": "", in my eve.json for quic and I do not want them

@satta
Copy link
Contributor Author

satta commented Mar 24, 2024

Compiling with --disable-ja4, I get "ja4": "", in my eve.json for quic and I do not want them

True, I just addressed this and also double-checked that for JA3.

@satta
Copy link
Contributor Author

satta commented Mar 26, 2024

New PR: #10725

@satta satta closed this Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master
Development

Successfully merging this pull request may close these issues.

3 participants