app-layer: websockets protocol support#10075
Conversation
Ticket: 2695 Introduces a device EnumStringU8 to ease the use of enumerations in protocols : logging the string out of the u8, and for detection, parsing the u8 out of the string
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #10075 +/- ##
==========================================
- Coverage 82.39% 82.34% -0.05%
==========================================
Files 972 978 +6
Lines 271406 271861 +455
==========================================
+ Hits 223624 223864 +240
- Misses 47782 47997 +215
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
WARNING:
Pipeline 17144 |
| if let Some(xorkey) = tx.pdu.mask { | ||
| js.set_uint("mask", xorkey.into())?; | ||
| } | ||
| if let Some(val) = web_socket_opcode_string(tx.pdu.opcode) { |
There was a problem hiding this comment.
Would it be more idiomatic if the derive was done as a From or FromStr implementation?
There was a problem hiding this comment.
So rather ToString and FromStr ?
You are my reference for what is idiomatic in rust :-p
Is there a performance cost to do type conversion from integer to enum ?
There was a problem hiding this comment.
Oh, I misread this.. So it essentially goes from u8 -> WebSocketOpCode -> &str? My first feeling is the derive macro does seem like a little overkill since WebSocketOpcode doesn't seem to be used at all in the code, other than behind the derive macro? But that aside..
I think the derived code should implement a trait, or an impl block rather than a make function. Normally you might add a to_str() method...
impl WebSocketOpCode {
fn to_str(&self) -> 'static &str {
...
}
// Or as a direct replacement. Call like WebSocketOpcode::to_str()
fn to_str() -> Option<&'static str> {
}
}
Of course this assumes that you have a constructed WebSocketOpcode already, which it probably makes sense that tx.pdu.opcode might be an instance of, which could be done with an implementation of
impl From<u8> for WebSocketOpcode
I guess it feels odd that this enum exists only to generate some bare functions, but is never actually used?
There was a problem hiding this comment.
So it essentially goes from u8 -> WebSocketOpCode -> &str?
Nope, not using WebSocketOpCode itself
the derive macro does seem like a little overkill
I want to code only once the match between integer value and string...
How do I achieve that without overkill ?
And without risking a typo if I code 2 functions (one stringer and one from str)
Of course this assumes that you have a constructed WebSocketOpcode already
I do not
which it probably makes sense that tx.pdu.opcode might be an instance of
It does not seem to fit for me asWebSocketOpcode enum has a limited number of values, and I still want opcode 3 that is unknown to be parsed...
Can/should I improve my enum ? Like adding a case Unknown(u8) ?
I guess it feels odd that this enum exists only to generate some bare functions, but is never actually used?
I agree.
Sum up :
- Can/should I improve my enum ? Like adding a case
Unknown(u8)? - Is there a batter way than derive macro ?
There was a problem hiding this comment.
Can/should I improve my enum ? Like adding a case
Unknown(u8)?
This makes sense.
Is there a batter way than derive macro ?
Implement the methods on the enum, like from_str and to_str?
This looks interesting as well: https://github.com/Peternator7/strum
There was a problem hiding this comment.
There is no standard to_str right ?
Should I use https://doc.rust-lang.org/std/string/trait.ToString.html ? That is to_string(&self) -> String so that allocates even if it is a static string... I think not
There was a problem hiding this comment.
There is no standard
to_strright ?Should I use https://doc.rust-lang.org/std/string/trait.ToString.html ? That is
to_string(&self) -> Stringso that allocates even if it is a static string... I think not
No there isn't, but that doesn't mean you can implement it/derive it directly on the WebSocketOpCode.
There was a problem hiding this comment.
Strum does not seem to work for displaying the value of a case Unknown(u8)
cf code in https://docs.rs/strum_macros/0.25.3/strum_macros/derive.Display.html where
Color::Blue(10)does only display blue without the 10
Ah, I didn't look at it that closely.
There was a problem hiding this comment.
Why did you not use strum for AppLayerEvent derive ?
Looks quite similar
Yeah, they do. derive(AppLayerEvent) was added to implement the AppLayerEvent trait we use in some generic functions and has some additional Suri only methods like get_event_info.
There was a problem hiding this comment.
You can check next version of the PR ;-)
|
Replaced by #10091 |
3 participants
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/2695
Describe changes:
OISF/suricata-verify#1550 justrebased and force-pushed
#10074 with greener CI
Draft : what should be done for protocol detection ?
I hackingly register the probing parser on port 1
TODO: