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 dependency from TLS encrypted bypass #10464

Conversation

msdean
Copy link

@msdean msdean commented Feb 19, 2024

Make sure these boxes are signed before submitting your Pull Request -- thank you.

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/6788
Link to forum discussion: https://forum.suricata.io/t/encrypted-tls-bypass-dependency-on-stream-bypass/3528
Link to previous PR: #9127

Describe changes:

  • 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 unrleated features.

Provide values to any of the below to override the defaults.

To use a pull request use a branch name like pr/N where N is the
pull request number.

Alternatively, SV_BRANCH may also be a link to an
OISF/suricata-verify pull-request.

SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=

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.
Update documentation to reflect the new features and changes.
Copy link

NOTE: This PR may contain new authors.

@@ -5472,17 +5472,13 @@ int StreamTcpPacket (ThreadVars *tv, Packet *p, StreamTcpThread *stt,
}

if (ssn->flags & STREAMTCP_FLAG_BYPASS) {
/* we can call bypass callback, if enabled */
if (StreamTcpBypassEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

bonus : you can make this function static as it is only used in src/stream-tcp.c

Finally, if ``encryption-handling`` is set to ``full``, Suricata will process the
flow as normal, without inspection limitations or bypass.
Finally, if ``encryption-handling`` is set to ``full``, Suricata will process
the flow as normal, without inspection limitations or bypass.
Copy link
Contributor

Choose a reason for hiding this comment

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

unneeded change

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 for the work.

CI : ✅
Code : good but there is the SSH case
Commits segmentation : ok, also ok to squash
Commit messages : ok, but there is the SSH case as well
Git ID set : looks fine for me
CLA : I do not have access
Doc update : thanks for it. Maybe there should be a note in doc/userguide/upgrade.rst as well
Redmine ticket : ok
Rustfmt : not needed
Tests : 🟠 could you add a SV test for this ?
Dependencies added: none

So, the major question is the behavior change for SSH which now gets automatically bypassed, even if this is not mentioned in doc or commit messages... What do you think about it ?

@jufajardini
Copy link
Contributor

So, the major question is the behavior change for SSH which now gets automatically bypassed, even if this is not mentioned in doc or commit messages... What do you think about it ?

If there's a change in behavior, then agreeing that we want a doc update.

@catenacyber
Copy link
Contributor

@msdean will you work further on this ?

@msdean
Copy link
Author

msdean commented Jul 14, 2024

Possibly at some point, but the request of adding an SV test is a bit much for me, as I don't really have the time at the moment to setup a dev environment and onboard to the codebase. I was hoping to get away with a small PR with just this change 😅.
If someone can pick up what's left that would be great, as I do think the change is the right way to go, but I'm currently content with using a fork with the fix for my needs, and obviously no expectations from the community to pick up my slack, I'm sure you guys have more pressing matters to attend do.
I do hope to get back to it and finish it up proper at some point, just don't see it happening any time soon unfortunately.

@catenacyber
Copy link
Contributor

Thanks for your answer, maybe we will pick it up.

So, the major question is the behavior change for SSH which now gets automatically bypassed, even if this is not mentioned in doc or commit messages... What do you think about it ?

Do you have an opinion about this ?

@msdean
Copy link
Author

msdean commented Jul 16, 2024

Thanks for your answer, maybe we will pick it up.

So, the major question is the behavior change for SSH which now gets automatically bypassed, even if this is not mentioned in doc or commit messages... What do you think about it ?

Do you have an opinion about this ?

I think it should either be documented or perhaps reworked in a way that doesn't affect SSH.

@lukashino
Copy link
Contributor

Followed up on this in #11801

@lukashino lukashino closed this Sep 19, 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.

4 participants