Skip to content

Websockets 2695 v7.1#10091

Closed
catenacyber wants to merge 4 commits intoOISF:masterfrom
catenacyber:websockets-2695-v7.1
Closed

Websockets 2695 v7.1#10091
catenacyber wants to merge 4 commits intoOISF:masterfrom
catenacyber:websockets-2695-v7.1

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

#10075 with

  • improved protocol detection (less tacky)
  • improved enumeration string derive (with its own ticket)
  • payload reassembly
  • payload decompression (cf RFC 7692)
  • some events when payload reassembly limit is reached

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
_ => #name::Unknown(v),
}
}
pub(crate) fn into_u(&self) -> #utype_str {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just from and into?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because they are standard keywords ?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe into_u8 and from_u8 to be a little more verbose given the name of the proc macro.

Also, any other enums we can implement this on to show its broader use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any other enums we can implement this on to show its broader use case?

For example http2.errorcode, http2.frametype (checking all http2-specific keywords)

_ => None
}
}
pub(crate) fn to_detect_ctx(s: &str) -> Option<DetectUintData<#utype_str>> {
Copy link
Member

Choose a reason for hiding this comment

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

I think this derive needs a new name as it does more than just to/from strings.

It is helpful when a derive generates the implementation of a trait, like AppLayerEvent as it gives you a place to document the interface that is being generated. AppLayerFrameType is another example.

Copy link
Member

Choose a reason for hiding this comment

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

More on using a trait. If we create a trait, StringEnumU8, then we should be able to get this method out of the derive, as I think derives should be the minimal amount of code needed.

We could then do something like:

impl From<&dyn StringEnumU8> for DetectUintData {
    type Error = ();
    fn try_from(value: &dyn MyTrait) -> Result<Self, Self::Error> {
    }
}

allowing anything implementing StringEnumU8 to be converted to a DetectUintData through standard interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok to make a trait.

Not getting your second comment
How can we get the method out of the derive ?

Copy link
Member

Choose a reason for hiding this comment

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

How can we get the method out of the derive ?

Just implement the From trait for DetectUintData alongside DetectUintData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I get what you mean.

But I do not really do impl From<&dyn StringEnumU8> for DetectUintData { because I resort to generic integer parsing if this is not an enum string...

So I think the prototype fn to_detect_ctx(s: &str) -> Option<DetectUintData<T>> is right as it takes a string as input

@suricata-qa
Copy link

ERROR:

ERROR: QA failed on SURI_TLPW1_files_sha256.

Pipeline 17182

@catenacyber catenacyber mentioned this pull request Dec 22, 2023
@catenacyber
Copy link
Contributor Author

Replaced by #10093

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