detect: add vlan.id keyword - v6 #12301
Conversation
vlan.id matches on Virtual Local Area Network IDs It is an unsigned 16-bit integer Valid range for the default configuration = [1-4094] Supports prefiltering Ticket: OISF#1065
AkakiAlice
requested review from
jasonish,
jufajardini and
victorjulien
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12301 +/- ##
=========================================
Coverage ? 83.24%
=========================================
Files ? 914
Lines ? 257834
Branches ? 0
=========================================
Hits ? 214629
Misses ? 43205
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
CI : 🟢
Code : cool, just one nit
Commits segmentation : ok
Commit messages : nice
Git ID set : looks fine for me
CLA : you already contributed
Doc update : excellent
Redmine ticket : ok
Rustfmt : looks ok for vlan_id.rs
Tests : nice, thanks, added a remark there
Dependencies added: none
|
|
||
| pub const DETECT_VLAN_ID_ANY: i8 = i8::MIN; | ||
| pub const DETECT_VLAN_ID_ALL: i8 = i8::MAX; | ||
| pub const DETECT_VLAN_ID_COUNT: i8 = 100; |
There was a problem hiding this comment.
We may want to transfer src/decode-vlan.h:#define VLAN_MAX_LAYERS 3 here and use VLAN_MAX_LAYERS instead of hardcoded 3
There was a problem hiding this comment.
I defined VLAN_MAX_LAYERS in the vlan_id.rs file like this: pub const VLAN_MAX_LAYERS: i8 = 3;
but I get this error message:
decode.h:503:22: error: ‘VLAN_MAX_LAYERS’ undeclared here (not in a function)
503 | uint16_t vlan_id[VLAN_MAX_LAYERS];
And when I include rust.h in the decode.h file, this first error goes away, but I get several error messages similar to this one:
util-file.h:214:39: error: unknown type name ‘Flow’
214 | void FileDisableStoringForTransaction(Flow *f, const uint8_t direction, void *tx, uint64_t tx_id);
There was a problem hiding this comment.
I think you need to maintain 2 copies of the constant. One on the C side and the other one on the Rust side. The important thing is to avoid using magic numbers.
There was a problem hiding this comment.
Do not spend too much time on it if it gets too complex with include order.
Just do as Shivani said, and add some comments
| assert!(detect_parse_vlan_id("200abc").is_none()); | ||
| assert!(detect_parse_vlan_id("4096").is_none()); | ||
| assert!(detect_parse_vlan_id("600,abc").is_none()); | ||
| assert!(detect_parse_vlan_id("600,100").is_none()); |
There was a problem hiding this comment.
5, count and also 123,-4 are error cases not tested
There was a problem hiding this comment.
What do you mean by "test 1,2,3 errors" ?
There was a problem hiding this comment.
pass 1,2,3 as args to the fn detect_parse_vlan_id and see if that errors out
There was a problem hiding this comment.
yes assert!(detect_parse_vlan_id("1,2,3").is_none()); because parts.len() > 2
|
|
||
| vlan.id:300 # exactly 300 | ||
| vlan.id:<300,0 # smaller than 300 at layer 0 | ||
| vlan.id:>=200,1 # greater or equal than 200 at layer 1 |
There was a problem hiding this comment.
nit (missed this the other review x_x ):
| vlan.id:>=200,1 # greater or equal than 200 at layer 1 | |
| vlan.id:>=200,1 # greater than or equal to 200 at layer 1 |
| The id can be matched exactly, or compared using the ``op`` setting:: | ||
|
|
||
| vlan.id:300 # exactly 300 | ||
| vlan.id:<300,0 # smaller than 300 at layer 0 |
There was a problem hiding this comment.
| vlan.id:<300,0 # smaller than 300 at layer 0 | |
| vlan.id:<300,0 # less than 300 at layer 0 |
| Id values for vlan.id keyword: | ||
|
|
||
| ======== ================================================ | ||
| Value Description | ||
| ======== ================================================ | ||
| 1 - 4094 Valid range for vlan id | ||
| 0 - 3 Valid range of number of layers (with ``count``) | ||
| ======== ================================================ |
There was a problem hiding this comment.
We can use the .. table:: Sphinx directive and have neat table titles, here and for the following one:
| Id values for vlan.id keyword: | |
| ======== ================================================ | |
| Value Description | |
| ======== ================================================ | |
| 1 - 4094 Valid range for vlan id | |
| 0 - 3 Valid range of number of layers (with ``count``) | |
| ======== ================================================ | |
| .. table:: **Id values for vlan.id keyword** | |
| ======== ================================================ | |
| Value Description | |
| ======== ================================================ | |
| 1 - 4094 Valid range for vlan id | |
| 0 - 3 Valid range of number of layers (with ``count``) | |
| ======== ================================================ |
Ticket: #1065
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
Description:
Documentation changes:
idandlayer: line 20, line 29;count: line 77.vlan_id.rs changes:
#no_manglefrom the constants definitions: line 22;pub const: line 22;DETECT_VLAN_ID: line 22;.ok()?: line 35, line 54;count: line 46;ifto checkcountrange: line 55;i8::MAXandi8::MINwithDETECT_VLAN_ID_ALLandDETECT_VLAN_ID_ANY: line 45, line 52, line 103;allandcount: line 106, line 117.detect-vlan-id.c changes:
rust.h: line 21;ifchain withswitch-case: line 32.Commit message:
SV_BRANCH=OISF/suricata-verify#2194
Previous PR= #12290