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.

Sign up for GitHub

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

Jump to bottom

Flow bytes pkts syntax/v3 #11889

Closed
wants to merge 2 commits into from
Closed

Flow bytes pkts syntax/v3 #11889

wants to merge 2 commits into from

Conversation

inashivb
Copy link
Member

@inashivb inashivb commented Oct 7, 2024 •
edited
Loading

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

SV_BRANCH=OISF/suricata-verify#2079

  1. Syntax comparisons:
master This PR
flow.pkts_toclient flow.pkts:toclient
flow.pkts_toserver flow.pkts:toserver
flow.bytes_toserver flow.bytes:toserver
flow.bytes_toclient flow.bytes:toclient
- flow.pkts:either
- flow.bytes:either
  1. Code advantages shared in the commit message of ac9c5d2

Previous PR: #11653
Changes since v1:

  • rebased and made PR description clearer so difference is easy to see as asked

Known issues:

  1. Doc update hasn't been done yet.
  2. Prefilter fn has not been updated yet so it may not work as intended.
  3. Tests for the parsing of arguments need to be done.

@inashivb
flow/pkts: make syntax cleaner and compact
ac9c5d2 
Currently, the syntax includes direction as a part of the keyword which
is against how usually keywords are done. By making direction as a
mandatory argument, it is possible to make the syntax cleaner and the
implementation more compact and easily extendable.
Pros:
- Registration table sees lesser entries
- If the options have to be extended, it can be done trivially
- In accordance w existing keyword implementations
@inashivb
flow/pkts: allow matching on either direction
e68868b 
For flow.bytes and flow.pkts keywords, allow matching in either
direction.

Feature 5646
@codecov Codecov
Copy link

codecov bot commented Oct 7, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 82.40000% with 22 lines in your changes missing coverage. Please review.

Project coverage is 82.60%. Comparing base (3f0512e) to head (e68868b).
Report is 5 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #11889      +/-   ##
==========================================
- Coverage   82.61%   82.60%   -0.01%     
==========================================
  Files         912      912              
  Lines      249354   249375      +21     
==========================================
+ Hits       205992   206008      +16     
- Misses      43362    43367       +5
Flag Coverage Δ
fuzzcorpus 60.59% <12.80%> (-0.04%) ⬇️
livemode 18.83% <12.80%> (-0.02%) ⬇️
pcap 44.05% <12.80%> (-0.04%) ⬇️
suricata-verify 62.02% <82.40%> (+0.02%) ⬆️
unittests 58.92% <12.80%> (-0.01%) ⬇️

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

@victorjulien victorjulien self-assigned this Oct 7, 2024
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 23002

@victorjulien
Copy link
Member 

detect-flow-pkts.c:44:17: warning: The left operand of '==' is a garbage value [core.UndefinedBinaryOperatorResult]
   44 |     if (df->dir == DETECT_FLOW_TO_SERVER) {
      |         ~~~~~~~ ^
1 warning generated.

@victorjulien
Copy link
Member

The existing keywords map 1:1 to the flow.bytes_toclient and friends fields from the eve record. Wonder if we should keep them as is.

victorjulien
victorjulien reviewed Oct 7, 2024
View reviewed changes
src/detect-flow-pkts.c
{
if (p->flow == NULL) {
return 0;
DetectFlow *df = SCCalloc(1, sizeof(DetectFlow));
Copy link
Member

Choose a reason for hiding this comment

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

no check after alloc

@inashivb
Copy link
Member Author

inashivb commented Oct 7, 2024

The existing keywords map 1:1 to the flow.bytes_toclient and friends fields from the eve record. Wonder if we should keep them as is.

Do you mean keep the rule language as-is? Sure! I just need a verdict on this so I can implement the either option as is better suited. :)

@inashivb inashivb mentioned this pull request Oct 8, 2024
Open
@inashivb inashivb closed this Oct 8, 2024
@inashivb inashivb deleted the flow-bytes-pkts-syntax/v3 branch October 8, 2024 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@victorjulien victorjulien victorjulien left review comments

Assignees

@victorjulien victorjulien

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@inashivb @suricata-qa @victorjulien