Skip to content
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

5912 rfb skip v3 #9064

Closed
wants to merge 3 commits into from
Closed

5912 rfb skip v3 #9064

wants to merge 3 commits into from

Conversation

satta
Copy link
Contributor

@satta satta commented Jun 22, 2023

Previous PR: #9041

Describe changes to previous PR:

  • Reintroduce test for error state.
  • Add more debug validations for cases where there must be a tx.
  • Remove unused Eq Derive.
  • Squash changes into fewer, more sensible commits.
  • Set event when state machine gets confused by parties talking out of order.
  • Also add new rule file rfb-events.rules to rules/Makefile.am.

We only try to parse a small subset of what is possible in
RFB. Currently we only understand some standard auth schemes
and stop parsing when the server-client handshake is complete.
Since in IPS mode returning an error from the parser causes
drops that are likely uncalled for, we do not want to return
errors when we simply do not understand what happens in the
traffic.
@satta satta requested review from jasonish and a team as code owners June 22, 2023 21:10
@satta satta mentioned this pull request Jun 22, 2023
@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Merging #9064 (23189c8) into master (643e674) will decrease coverage by 0.04%.
The diff coverage is 35.89%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9064      +/-   ##
==========================================
- Coverage   82.35%   82.31%   -0.04%     
==========================================
  Files         969      969              
  Lines      273655   273721      +66     
==========================================
- Hits       225359   225325      -34     
- Misses      48296    48396     +100     
Flag Coverage Δ
fuzzcorpus 64.53% <33.91%> (-0.05%) ⬇️
suricata-verify 60.59% <24.34%> (-0.06%) ⬇️
unittests 62.89% <7.69%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

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

Thanks Sascha.

Could you just add a redmine ticket for this and reference it in the main commit message ?
Otherwise, all good for me.

CI : green
Code : a few questions
Commits segmentation : ok for me
Commit messages : still missing a ticket reference in the main commit
Git ID set : ok
CLA : ok
Doc update : none needed
Redmine ticket : needed, right ?
Rustfmt : ok
Tests : ok
Dependencies added: none

@satta
Copy link
Contributor Author

satta commented Jun 26, 2023

Thanks Sascha.

Could you just add a redmine ticket for this and reference it in the main commit message ?

The redmine ticket was linked in the first original PR: #5912

I'll keep it tracked across PR iterations next time 👍🏻 -- and also open a new PR to include it in the main commit message. Thanks!

@satta satta mentioned this pull request Jun 26, 2023
@satta
Copy link
Contributor Author

satta commented Jun 26, 2023

New PR: #9071

@satta satta closed this Jun 26, 2023
@satta satta deleted the 5912-rfb-skip-v3 branch July 13, 2023 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants