Skip to content

Comments

Websockets 2695 v8#10093

Closed
catenacyber wants to merge 4 commits intoOISF:masterfrom
catenacyber:websockets-2695-v8
Closed

Websockets 2695 v8#10093
catenacyber wants to merge 4 commits intoOISF:masterfrom
catenacyber:websockets-2695-v8

Conversation

@catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/2695
https://redmine.openinfosecfoundation.org/issues/6647

Describe changes:

  • app-layer: websockets protocol support
  • rust : derive for protocol enumerations strings
  • enip: register on default port 44818 also for TCP (as is done on UDP)
  • http2: add newer settings, including the one for websocket over HTTP/2 cf https://www.rfc-editor.org/rfc/rfc8441#section-3
SV_BRANCH=pr/1550

OISF/suricata-verify#1550 justrebased and force-pushed

#10091 with

  • trait for derive and hopefully fixed compilation with older rust

I think this is good enough for a first version even if there may be improvements (that can happen in later tickets) :

  • payload logging : waiting for feedback wether this is only for alerts, or also for websocket event
  • using latest HTTP/1 transaction in a single rule, instead of splitting rule and using flowbits
  • support websockets over HTTP/2 : need pcap
    This is a big one as websockets over HTTP/2 only use a single HTTP/2 stream and not the whole TCP connection which keeps having newer regular HTTP/2 streams

if no config option is found,
as is done for udp

Ticket: 6304
Ticket: 6647

Allows keywords using integers to use strings in signature
parsing based on a rust enumeration with a derive.
Including the one for websocket over HTTP/2
@catenacyber
Copy link
Contributor Author

Maybe I should change the fin keyword to be the complete first byte, and implement https://redmine.openinfosecfoundation.org/issues/6648 to march on the reserved bits (like the one that got used for compression) (can still be matched now with frames and byte_extract I guess)

@suricata-qa
Copy link

ERROR:

ERROR: QA failed on SURI_TLPW1_files_sha256.

Pipeline 17183

fn from_str(s: &str) -> Option<Self> where Self: Sized;

/// Get a detect context for integer keyword.
fn to_detect_ctx(s: &str) -> Option<DetectUintData<T>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you should now be able to do something like:

impl<T> TryFrom<&dyn Enum<T>> for DetectUintData<T> {
    type Error = ();

    fn try_from(value: &dyn Enum<T>) -> Result<Self, Self::Error> {
        todo!()
    }
}

instead of having this method as part of the trait.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively naming.. This is an enum specifically for use in detection with a direct mapping from a name to an integer value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you should now be able to do something like:

impl<T> TryFrom<&dyn Enum<T>> for DetectUintData<T> {
    type Error = ();

    fn try_from(value: &dyn Enum<T>) -> Result<Self, Self::Error> {
        todo!()
    }
}

instead of having this method as part of the trait.

So I think the prototype fn to_detect_ctx(s: &str) -> Option<DetectUintData<T>> is right as it takes a string as input, and resorts to generic integer parsing if this is not an enum string

This is an enum specifically for use in detection with a direct mapping from a name to an integer value?

The enum is also used in logging (not only for alerts). Was it your question ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just seems out of place, with respect to naming. We have a rather generic trait named Enum, but then we have this method to_detect_ctx that doesn't even take &self.

Some more comments in next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the trait can be better named indeed.

method to_detect_ctx that doesn't even take &self.

But I do not want to use the enumeration : I just want a unique association between integers and strings that goes both ways, and some helper functions around that.
Is derive+trait+enum the best way to do so ? Or do you see another way ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the gist I posted that has the from traits, maybe a generic helper function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thans Jason, tried in d5f1bf2

@catenacyber catenacyber mentioned this pull request Jan 2, 2024
@catenacyber
Copy link
Contributor Author

Replaced by #10104

@catenacyber catenacyber closed this Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants