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 -- v10 #10725

Closed
wants to merge 3 commits into from
Closed

JA4 for TLS and QUIC -- v10 #10725

wants to merge 3 commits into from

Conversation

satta
Copy link
Contributor

@satta satta commented Mar 26, 2024

Previous PR: #10579

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

Changes to previous PR:

  • Rebase against current master.
  • Reduce code duplication by moving ifdefs.
  • Avoid output of empty JSON objects when JA3/4 are disabled.
  • Avoid unnecessary allocation when JA4 is disabled.
  • Adjust wording in documentation and add some information about compile time activation/deactivation.
  • Remove unnecessary SCJA4 wrapper struct.
SV_BRANCH=pr/1682

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

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

Project coverage is 82.63%. Comparing base (7d937db) to head (84ec2de).
Report is 98 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10725      +/-   ##
==========================================
- Coverage   82.69%   82.63%   -0.06%     
==========================================
  Files         926      929       +3     
  Lines      247637   248060     +423     
==========================================
+ Hits       204790   204996     +206     
- Misses      42847    43064     +217     
Flag Coverage Δ
fuzzcorpus 64.12% <64.15%> (+0.07%) ⬆️
suricata-verify 61.83% <83.18%> (-0.15%) ⬇️
unittests 62.17% <43.85%> (-0.02%) ⬇️

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

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 : 🟢
  • Code : TODO
  • Commits segmentation : nice
  • Commit messages : ok for me
  • Git ID set : looks fine for me
  • CLA : you already contributed :-)
  • Doc update : Ok, maybe there could be only one file/section ja-keywords
  • Redmine ticket : ok
  • Rustfmt : ok for ja4.rs
  • Tests : 🟠 Left remarks there
  • Dependencies added: none

#ifndef HAVE_JA3
static int DetectJA3SetupNoSupport(DetectEngineCtx *a, Signature *b, const char *c)
{
SCLogError("no JA3 support built in");
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if this could be only one function instead of copy pasting 4 lines...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH I would consider having a generic function in some common file less understandable than this way. By keeping such small static functions in the same file, they are closer to where they are used (in plain developer sight), which I consider much more straightforward than having to dig them up in the IDE. FWIW, if I could even use anonymous functions in C for that, I would ;)

I think code duplication is only bad if it is nontrivial code (with a big impact if it's changed) that is being copied, which is not the case here unless the signature if the Setup function pointer would be to change -- but that would touch a lot of places anyway!

@catenacyber
Copy link
Contributor

This is good enough for me even if there are still some nits/questions

@catenacyber catenacyber added the needs rebase Needs rebase to master label Apr 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.

This now needs a rebase, you can take this as an opportunity to get some more nits better

@satta
Copy link
Contributor Author

satta commented Apr 12, 2024

This now needs a rebase, you can take this as an opportunity to get some more nits better

Will do!

@satta
Copy link
Contributor Author

satta commented Apr 12, 2024

maybe there could be only one file/section ja-keywords

Done!

@satta
Copy link
Contributor Author

satta commented Apr 12, 2024

Next PR: #10829

@satta satta closed this Apr 12, 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.

2 participants