Mqtt rust keywords 4863 v2 #11374
Mqtt rust keywords 4863 v2 #11374
Conversation
So that MQTTTypeCode::CONNECT does not become c_o_n_n_e_c_t
As needed for MQTTTypeCode which accepts both CONNECT uppercase and unassigned lowercase
for easier later matching
Ticket: 4863 On the way, convert some keywords to use the first-class integer support. And imports in pure rust the support for multi-buffer.
There was a typo that version 3 was tested twice
as is done for other keywords
catenacyber
requested review from
jasonish,
jufajardini and
victorjulien
as code owners
|
|
||
|
|
||
| mqtt.flags | ||
| ---------- | ||
|
|
||
| Match on a combination of MQTT header flags, separated by commas (``,``). Flags may be prefixed by ``!`` to indicate negation, i.e. a flag prefixed by ``!`` must `not` be set to match. | ||
|
|
||
| mqtt.flags uses an :ref:`unsigned 8-bits integer <rules-integer-keywords>` |
There was a problem hiding this comment.
I'd consider this statement confusing unless also showing an example to illustrate that both negatable flag strings as well as numeric values can be used here.
Why would someone use the numeric value here? I thought the idea of the rule language is to abstract from literal byte values to semantic identifiers.
There was a problem hiding this comment.
Why would someone use the numeric value here? I thought the idea of the rule language is to abstract from literal byte values to semantic identifiers.
You can use either numeric or semantic identifier...
I am not sure it makes sense to use numeric, but it is available
There was a problem hiding this comment.
Got it. Maybe I was expecting an example like given for mqtt.type.
| let ctx = DetectUintData::<u8> { | ||
| arg1, | ||
| arg2, | ||
| mode: DetectUintMode::DetectUintModeBitmask, |
There was a problem hiding this comment.
Nice. This is surely powerful, but really needs documentation or examples. Took me a bit to get my head around it!
There was a problem hiding this comment.
There is https://docs.suricata.io/en/latest/rules/integer-keywords.html#bitmasks
Do you want more ?
There was a problem hiding this comment.
Oh I was referring to the Rust API (DetectUintMode and what tooling the Rust helpers provide). I think for common cases like negatable flags it would be helpful to see an example so the wheel will not be reinvented again ;)
First of all, thanks a lot for this porting work! It must have taken a lot of work to replicate, streamline and optimize things to make everything consistent with what you have planned for detection code in Rust! 🙌🏻 I have coarsely looked through the code and I must say that it's too much to really review in detail, in particular when unfamiliar with all the detection support that was established in the meantime. But I think I understand how the C concepts are mapped to Rust and how the interface works, while I am still stumped by some of the deeper Rust-isms (like any of the derive stuff!). It would be nice to get some documentation for all the helper code at some point so it's less likely to reinvent the wheel. Having noted that, I would approve these changes from my point of view; the code looks complete, the tests succeed and suricata-verify is also happy. |
|
ERROR: ERROR: QA failed on build_fetch. Pipeline 21348 |
|
ERROR: ERROR: QA failed on build_fetch. Pipeline 21349 |
Thanks Sascha
Thanks for the SV tests : as a matter of facts, they caught a bad copy paste from me where I put a keyword match function into another Continued in #11430 |
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/4683
Describe changes:
SV_BRANCH=OISF/suricata-verify#1920
#11316 with more fixing commits
@satta could you say if you approve ?