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

detect: add ldap operation keywords - v4 #12447

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

AkakiAlice
Copy link
Contributor

@AkakiAlice AkakiAlice commented Jan 21, 2025

Ticket: #7453

Contribution style:

Our Contribution agreements:

Changes (if applicable):

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

Description:

  • Implement ldap.request.operation , ldap.responses.operation and ldap.responses.count keywords.

Changes:

SV_BRANCH=OISF/suricata-verify#2248
Previous PR= #12435

ldap.request.operation matches on Lightweight Directory Access Protocol request operations
This keyword maps to the eve field ldap.request.operation
It is an unsigned 8-bit integer
Doesn't support prefiltering

Ticket: OISF#7453
ldap.responses.operation matches on Lightweight Directory Access Protocol response operations
This keyword maps to the eve field ldap.responses[].operation
It is an unsigned 8-bit integer
Doesn't support prefiltering

Ticket: OISF#7453
ldap.responses.count matches on the number of LDAP responses
This keyword maps to the eve field len(ldap.responses[])
It is an unsigned 32-bit integer
Doesn't support prefiltering

Ticket: OISF#7453
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

Attention: Patch coverage is 83.11111% with 38 lines in your changes missing coverage. Please review.

Project coverage is 80.63%. Comparing base (95e8427) to head (575ec24).

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #12447    +/-   ##
========================================
  Coverage   80.63%   80.63%            
========================================
  Files         920      921     +1     
  Lines      258704   258928   +224     
========================================
+ Hits       208595   208776   +181     
- Misses      50109    50152    +43     
Flag Coverage Δ
fuzzcorpus 56.76% <22.66%> (-0.05%) ⬇️
livemode 19.40% <22.66%> (+<0.01%) ⬆️
pcap 44.27% <23.11%> (-0.06%) ⬇️
suricata-verify 63.28% <83.85%> (+0.01%) ⬆️
unittests 58.48% <22.66%> (-0.04%) ⬇️

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 :-)

CI : 🟢
Code : good
Commits segmentation : ok thanks
Commit messages : good
Git ID set : looks fine for me
CLA : you already contributed
Doc update : good, cc @jufajardini
Redmine ticket : ok
Rustfmt : good :-)
Tests : nice, thanks
Dependencies added: none


ldap.request.operation uses :ref:`unsigned 8-bit integer <rules-integer-keywords>`.

This keyword maps to the eve field ``ldap.request.operation``
Copy link
Contributor

Choose a reason for hiding this comment

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

@lukashino what do you think about this ? cf your work on #11951

Especially, is the log field written the correct way ?
Here this is a simple case, but what about the 2 others with arrays : ldap.responses[].operation and len(ldap.responses[])

Copy link
Contributor

Choose a reason for hiding this comment

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

Also should we update schema.json to document the revert mapping ?

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.

2 participants