Skip to content

Lua ja3 7605 v6#13168

Closed
catenacyber wants to merge 3 commits intoOISF:masterfrom
catenacyber:lua-ja3-7605-v6
Closed

Lua ja3 7605 v6#13168
catenacyber wants to merge 3 commits intoOISF:masterfrom
catenacyber:lua-ja3-7605-v6

Conversation

@catenacyber
Copy link
Contributor

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

Describe changes:

  • lua: convert ja3 function into suricata.ja3 lib

SV_BRANCH=OISF/suricata-verify#2482

#13158 with review taken into account

so that future lua code can specify a direction
Since hooks, we do not need a specific SMTP buffer list id.
@codecov
Copy link

codecov bot commented May 5, 2025

Codecov Report

Attention: Patch coverage is 63.04348% with 51 lines in your changes missing coverage. Please review.

Project coverage is 83.04%. Comparing base (4e2f1de) to head (9cc6467).
Report is 22 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #13168      +/-   ##
==========================================
- Coverage   83.08%   83.04%   -0.04%     
==========================================
  Files         988      988              
  Lines      271850   271904      +54     
==========================================
- Hits       225874   225811      -63     
- Misses      45976    46093     +117     
Flag Coverage Δ
fuzzcorpus 61.34% <13.76%> (-0.09%) ⬇️
livemode 18.95% <7.24%> (-0.01%) ⬇️
pcap 44.78% <7.24%> (-0.03%) ⬇️
suricata-verify 64.80% <62.31%> (-0.03%) ⬇️
unittests 58.20% <13.04%> (-0.01%) ⬇️

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 25997

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 25998

@victorjulien victorjulien added this to the 8.0 milestone May 7, 2025
@jufajardini
Copy link
Contributor

Some doc comments incomming, sorry

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

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

Two typos, one place where I feel the explanation isn't so clear, and a suggestion for post merge:

Once the SV test gets also merged, I think we could add a reference in the docs here to the test, as a place to find a working Lua script that uses ja3 functions

end

``ja3.enable_ja3()`` will not enable ja3 if they are explicitly
disabled, so you should add ``requires: feature ja3;`` to your rule.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

Comment on lines +22 to +24
For use in rule matching, the rule may **hook** into a TLS or QUIC
transaction state if you want to match on only one of these protocols.
Or you should use need ``ja3`` or ``ja3s`` in your init script::
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you clarify what hook means here? Are these rule hooks? If so, I suggest adding a reference to that. If something else, maybe we need rewording or a longer explanation? (or a pointer to where to find more info)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you clarify what hook means here?

Not sure I can do it right, it comes from @jasonish doc about lua dns.

I am removing this as needs["ja3s"] = true looks more appropriate for ja3 lib

@victorjulien
Copy link
Member

Staging incl this PR is already running, so will merge this and @jufajardini 's comments will have to be addressed in a follow up patch.

@victorjulien
Copy link
Member

Merged in #13179, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants