Conversation
AkakiAlice
requested review from
jasonish,
jufajardini and
victorjulien
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12285 +/- ##
==========================================
- Coverage 83.22% 83.22% -0.01%
==========================================
Files 912 915 +3
Lines 257311 257802 +491
==========================================
+ Hits 214154 214553 +399
- Misses 43157 43249 +92
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Thanks, what are the differences with the previous version ? I see CI is still red, cf the check rust build that you can reproduce locally by running |
| it matches all layers if a packet contains multiple VLAN layers. However, if a | ||
| specific layer is defined, it will only match that layer. | ||
|
|
||
| vlan.id uses :ref:`unsigned 16-bit integer <rules-integer-keywords>`. |
There was a problem hiding this comment.
Should we also mention here the max value, and the specification that defines that?
I think it is a good idea
| const DetectVlanIdData *a = smctx; | ||
| v->u8[0] = a->du16.mode; | ||
| v->u8[1] = a->layer; | ||
| v->u16[2] = a->du16.arg1; |
There was a problem hiding this comment.
There should also be v->u16[3] = a->du16.arg2; (used for ranges like vlan.id:100-200)
| #ifndef SURICATA_DETECT_VLAN_ID_H | ||
| #define SURICATA_DETECT_VLAN_ID_H | ||
|
|
||
| #define ANY_VLAN_LAYER INT8_MIN |
There was a problem hiding this comment.
This definition should be in rust
| @@ -0,0 +1,33 @@ | |||
| Vlan.id Keyword | |||
There was a problem hiding this comment.
CI is red because you have to add this file in doc/userguide/rules/index.rst
|
And last : Suggestion for even more expressivity : add support vlan.id: >100,all which matches only if all vlan layers match |
| arg2: 0, | ||
| mode: DetectUintMode::DetectUintModeEqual, | ||
| }, | ||
| layer: i8::MIN |
There was a problem hiding this comment.
Q: Why is the default layer i8::MIN when a layer < -3 is unacceptable to the parser? ref: https://github.com/OISF/suricata/pull/12285/files#diff-19017a83531856b891bc67909647f1bdbf986c4fd068b0e107c149a0158cdd11R52
|
Has a newer revision linked above |
3 participants
Ticket: #1065
Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.
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/1065
Describe changes:
Provide values to any of the below to override the defaults.
link to the pull request in the respective
_BRANCHvariable.SV_BRANCH=OISF/suricata-verify#2185
Previous PR= #12273