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

Decouple stream bypass from TLS encrypted bypass v4 #11831

Conversation

lukashino
Copy link
Contributor

Following up on #11801

Redmine ticket: https://redmine.openinfosecfoundation.org/issues/6788

Describe changes:
v4

  • rebased
  • changed SSH bypass defaults to hopefully be in sync with the previous settings

v3

  • added SSH app-layer option encryption-handling allowing to choose whether to continue inspection on SSH once it turns encrypted
  • added SV tests
  • minor docs updates

SV_BRANCH=OISF/suricata-verify#2047

Lukas Sismis and others added 4 commits September 24, 2024 23:22
Decouple app.protocols.tls.encryption-handling and stream.bypass.
There's no apparent reason why encrypted TLS bypass traffic should
depend on stream bypass, as these are unrelated features.

Ticket: 6788
Copy link

NOTE: This PR may contain new authors.

Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 8 lines in your changes missing coverage. Please review.

Project coverage is 79.06%. Comparing base (c55c7d6) to head (06cc6f9).
Report is 18 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11831      +/-   ##
==========================================
- Coverage   82.57%   79.06%   -3.51%     
==========================================
  Files         912      912              
  Lines      249357   249203     -154     
==========================================
- Hits       205918   197044    -8874     
- Misses      43439    52159    +8720     
Flag Coverage Δ
fuzzcorpus 60.60% <71.42%> (+0.03%) ⬆️
livemode 18.73% <32.14%> (+<0.01%) ⬆️
pcap 44.09% <71.42%> (-0.02%) ⬇️
suricata-verify ?
unittests 58.93% <67.85%> (-0.01%) ⬇️

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

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 22800

@lukashino lukashino changed the title Feat/decouple stream bypass from tls encrypted bypass v4 Decouple stream bypass from TLS encrypted bypass v4 Sep 25, 2024
@victorjulien
Copy link
Member

We also need an addition to the upgrade section, I think.

@victorjulien
Copy link
Member

Blocked by issues in the SV test

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

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

A couple inline comments...
I'm wondering if we should add a note in the upgrading section about this new option for SSL, of it too small to be needed there.

The TLS and SSH app layer parsers have the ability to stop processing
encrypted traffic after the initial handshake. By setting the
`app-layer.protocols.tls.encryption-handling` and
`app-layer.protocols.tls.encryption-handling` options to `bypass` Suricata
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`app-layer.protocols.tls.encryption-handling` options to `bypass` Suricata
`app-layer.protocols.ssh.encryption-handling` options to `bypass` Suricata

Comment on lines +936 to +941
# What to do when the encrypted communications start:
# - bypass: stop processing this flow as much as possible.
# Offload flow bypass to kernel or hardware if possible.
# - full: keep tracking and inspection as normal
#
# encryption-handling: bypass
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we indicate here what is the default?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the behavior here the same as we have for tls, or is there a difference?

@lukashino
Copy link
Contributor Author

goto #11886

@lukashino lukashino closed this Oct 6, 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.

5 participants