-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rust/sip: register parser for tcp v6 #9387
Conversation
Accepts valid characters as defined in RFC3261.
This patch lets the parser to work over tcp protocol, taking care of handling data before calling the request/response parsers.
This patch permits to set a direction when a new transaction is created in order to avoid 'signature shadowing' as reported by Eric Leblond in commit 5aaf507
This permits to log sip messages over tcp.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9387 +/- ##
==========================================
- Coverage 82.17% 78.35% -3.83%
==========================================
Files 968 968
Lines 274198 274139 -59
==========================================
- Hits 225331 214790 -10541
- Misses 48867 59349 +10482
Flags with carried forward coverage won't be shown. Click here to find out more. |
@@ -388,4 +569,22 @@ pub unsafe extern "C" fn rs_sip_register_parser() { | |||
} else { | |||
SCLogDebug!("Protocol detecter and parser disabled for SIP/UDP."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I'd change now I think is this line and the one further below.
Protocol detection and parsing disabled for UDP SIP
and likewise for the TCP. Or just bring correct spelling to both. Easy to do with in a staging commit though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK. I'm not sure if its suitable for 7.0.x as it is much like adding a new parser, but it is a reasonable enhancement to 7.0 as well. Will need to be discussed.
Why is the CI red ? |
Maybe you need to rebase the S-V PR ?.. |
if AppLayerParserStateIssetFlag(pstate, APP_LAYER_PARSER_EOF_TS) > 0 { | ||
return AppLayerResult::ok(); | ||
} else { | ||
return AppLayerResult::err(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we end up here ?
Commit |
I think commits should be squashed into |
@@ -50,6 +50,7 @@ vars: | |||
GENEVE_PORTS: 6081 | |||
VXLAN_PORTS: 4789 | |||
TEREDO_PORTS: 3544 | |||
SIP_PORTS: "[5060, 5061]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is meant to be used in signatures, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, looking good.
Here is my quick feedback :
CI : can you make it green ? (rebasing the S-V PR I guess)
Code : looks good
Commits segmentation : I think some commits can be squashed together
Commit messages :Could you put the redmine ticket reference in the main commit ?
Git ID set : looks fine for me
CLA : I do not have access, but you have already contributed
Doc update : Where should we document this change @jufajardini ? upgrade section ?
Redmine ticket : ok
Rustfmt : thanks 🙇
Tests : Looks noce, will comment on it
Dependencies added: none
Replaced by #9483 right ? |
Make sure these boxes are signed before submitting your Pull Request -- thank you.
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3351
Describe changes:
SV_BRANCH=OISF/suricata-verify#1341