detect: add ldap operation keywords - v4 #12447
Conversation
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
AkakiAlice
requested review from
jasonish,
jufajardini and
victorjulien
as code owners
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
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`` |
There was a problem hiding this comment.
@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[])
There was a problem hiding this comment.
Also should we update schema.json to document the revert mapping ?
There was a problem hiding this comment.
I think @jasonish had some more thoughts about this.
There was a problem hiding this comment.
Need to document my thoughts more throuroughly somewhere, but I think its just a simplified version of what @lukashino had previous proposed. Looking at LDAP and the schema we see:
"ldap": {
"type": "object",
"optional": true,
"properties": {
"request": {
"type": "object",
"properties": {
"operation": {
"type": "string"
},
"message_id": {
"type": "integer"
},We could extend it out like:
"ldap": {
"type": "object",
"optional": true,
"properties": {
"request": {
"type": "object",
"properties": {
"operation": {
"type": "string"
"suricata": {
// A keyword that is a one to one mapping to this buffer.
"keyword": "ldap.operation",
// Related keywords that may also cover this field, if any.
"related_keywords": ["aaa", "bbb"]
}
},
"message_id": {
"type": "integer"
},There was a problem hiding this comment.
So, I understand that the PR is good as is, and we wait for a decision on what needs to be updated in the schema.
There was a problem hiding this comment.
Yeah, I think Victor was just poking me here so I'd write down some thoughts I had on the matter.
| } | ||
|
|
||
| unsafe extern "C" fn ldap_detect_request_free(_de: *mut c_void, ctx: *mut c_void) { | ||
| // Just unbox... |
There was a problem hiding this comment.
But rs_detect_u8_free just unboxes :-p
| } | ||
|
|
||
| #[no_mangle] | ||
| pub unsafe extern "C" fn ScDetectLdapRegister() { |
There was a problem hiding this comment.
git grep "fn Sc" | wc -l gives 7 functions to fix, including the template
We should do that in a next follow-up PR
There was a problem hiding this comment.
plus a CI check with this git grep?
|
Merged in #12462, thanks! |
|
Great work Alice, waiting on the next one :-) |
Ticket: #7453
Contribution style:
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
Our Contribution agreements:
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Changes (if applicable):
(including schema descriptions)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
Link to ticket: https://redmine.openinfosecfoundation.org/issues/7453
Description:
ldap.request.operation,ldap.responses.operationandldap.responses.countkeywords.Changes:
thread '<unnamed>' panicked at src/ldap/detect.rs:205:17: attempt to add with overflowSV_BRANCH=OISF/suricata-verify#2248
Previous PR= #12435