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 - v3 #12435

Closed

Conversation

AkakiAlice
Copy link
Contributor

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:

  • Add support for negative indexing
  • Implement ldap.responses.count
  • Separate commit messages

SV_BRANCH=OISF/suricata-verify#2243
Previous PR= #12343

ldap.request.operation matches on Lightweight Directory Access Protocol request operations
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
It is an unsigned 8-bit integer
Doesn't support prefiltering

Ticket: OISF#7453
ldap.responses.count matches on the number of LDAP responses
It is an unsigned 32-bit integer
Doesn't support prefiltering

Ticket: OISF#7453
Copy link

codecov bot commented Jan 20, 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 (c207cfc) to head (793e6b1).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12435      +/-   ##
==========================================
+ Coverage   80.62%   80.63%   +0.01%     
==========================================
  Files         920      921       +1     
  Lines      258697   258921     +224     
==========================================
+ Hits       208566   208793     +227     
+ Misses      50131    50128       -3     
Flag Coverage Δ
fuzzcorpus 56.76% <22.66%> (-0.05%) ⬇️
livemode 19.40% <22.66%> (+<0.01%) ⬆️
pcap 44.23% <23.11%> (-0.02%) ⬇️
suricata-verify 63.29% <83.85%> (+0.04%) ⬆️
unittests 58.47% <22.66%> (-0.05%) ⬇️

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

@catenacyber
Copy link
Contributor

Could you check CI failures :

panicked at src/ldap/detect.rs:205:17:

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, there is at least CI to fix

CI : some failures ⚠️
Code : checking
Commits segmentation : ok, the rustfmt commit could come first
Commit messages : good, we can add the json schema field that map to the added keywords
Git ID set : looks fine for me
CLA : you already contributed
Doc update : good
Redmine ticket : ok
Rustfmt : good :-)
Tests : nice, thanks
Dependencies added: none

}

impl ProtocolOp {
pub fn to_u8(&self) -> u8 {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jasonish do you see a rustier way ?

use std::str::FromStr;

#[derive(Debug, PartialEq)]
enum LdapIndex {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did I create a ticket to make this struct generic ? (not specific to ldap)

LdapIndex::Index(idx) => {
let index = if idx < 0 {
// negative values for backward indexing.
tx.responses.len() + idx as usize
Copy link
Contributor

Choose a reason for hiding this comment

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

@jasonish ideas on how to make this ?

I see
((tx.responses.len() as i32) + idx) as usize

@AkakiAlice
Copy link
Contributor Author

Replaced by: #12447

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