suricata
suricata copied to clipboard
OISF
Decouple stream.bypass dependency from TLS encrypted bypass
Make sure these boxes are signed before submitting your Pull Request -- thank you.
- [x] I have read the contributing guide lines at https://suricata.readthedocs.io/en/latest/devguide/codebase/contributing/contribution-process.html
- [x] I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
- [x] I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)
Link to redmine ticket: No ticket, but have a forum discussion: https://forum.suricata.io/t/encrypted-tls-bypass-dependency-on-stream-bypass/3528
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=
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Note the "merge branch" here
Apologies, fixed now. Since it was a technical issue and not a code change due to a CR comment, I skipped the creation of a new branch/PR, is that ok?
documentation changes look good, provided that the code change is also approved. :)
commit messages are a bit different from our conventions (cf https://docs.suricata.io/en/latest/devguide/codebase/contributing/code-submission-process.html#commits, item 6).
We'd do something like this:
stream: decouple stream.bypass from TLS encrypt bypass
userguide: update encrypted traffic bypass
Got it, I went over the guide but missed that part, my bad. Will fix.
documentation changes look good, provided that the code change is also approved. :) commit messages are a bit different from our conventions (cf https://docs.suricata.io/en/latest/devguide/codebase/contributing/code-submission-process.html#commits, item 6). We'd do something like this:
stream: decouple stream.bypass from TLS encrypt bypassuserguide: update encrypted traffic bypassGot it, I went over the guide but missed that part, my bad. Will fix.
Thanks! :) yeah, I noticed when going back to reference it that this part should probably be highlighted, somehow, it's not very visible...
Signed the contribution agreement today. Can someone please review and approve the PR? Thank you.
Signed the contribution agreement today. Can someone please review and approve the PR? Thank you.
Hey there, I noticed that Victor tagged this with 8.0, so I imagine that he expects this one to be merged in 8, which doesn't have a branch, yet, I think.
Can you make sure the email used to sign the CLA matches the git author email?
CI failures seem unrelated, but I think it could be a good idea to rebase the code so we can see the CI all green again :)
Thanks @msdean for this investigation.
I think we will want a redmine ticket to track this
Did you notice that there is not only TLS, but also SSH that sets APP_LAYER_PARSER_BYPASS_READY and thus is affected by this PR ?
Did you notice that there is not only TLS, but also SSH that sets
APP_LAYER_PARSER_BYPASS_READYand thus is affected by this PR ?
I did not. Can you point me to where this happens? I can only find the 2 references in app-layer-ssl.c: https://github.com/OISF/suricata/blob/suricata-7.0.0/src/app-layer-ssl.c#L2183 and https://github.com/OISF/suricata/blob/suricata-7.0.0/src/app-layer-ssl.c#L2396
Not in src directory, in rust ;-) https://github.com/OISF/suricata/blob/suricata-7.0.0/rust/src/ssh/ssh.rs#L169
I see now, thanks.
So this brings me back to my original question about whether this coupling between stream.bypass and TLS-encrypted bypass, which as far as I can tell is not a real dependency. Looking at the code you pointed to, it seems that APP_LAYER_PARSER_BYPASS_READY is enabled when the SSH connection state is changed to 'Finished' (not a rust guy, but that's what it looks like), so I in this case as well - I don't see a reason for the coupling.
How would you advise me to proceed?
I see the point for your use case and that it is not allowed by current configuration options.
You need to enable stream.bypass to be able to bypass.
But by doing so, some bypasses are forced (depth, SSH)
When TLS bypass is still configurable
I guess the reason was that people wanting bypass wanted it always for depth and SSH, and not always for TLS...
Maybe StreamTcpBypassEnabled can use an enum argument reason (depth, ssh, tls...) and stream.bypass can be either a boolean (all or nothing) or an array of reasons we want the bypass for
Thoughts @jasonish about this configuration options ?
Am I hearing that enabling bypass is often too broad? And the ability to bypass on a protocol by protocol basis should be easier to configure? Unfortunately, I don't have many thoughts on that right now, but want to make sure I have the problem correctly identified.
Am I hearing that enabling bypass is often too broad?
I understand the same.
And the ability to bypass on a protocol by protocol basis should be easier to configure? Unfortunately, I don't have many thoughts on that right now, but want to make sure I have the problem correctly identified.
Exact, but it is not exactly "protocol", as depth and other stuff may trigger a bypass
Am I hearing that enabling bypass is often too broad? And the ability to bypass on a protocol by protocol basis should be easier to configure? Unfortunately, I don't have many thoughts on that right now, but want to make sure I have the problem correctly identified.
I'm not sure that's how I would phrase it, but I suppose that's correct. The way I see it is that its independent features being artificially coupled.
Basically stream.bypass is like a master switch for enabling any other bypass features, even though those features don't actually depend on it.
catenacyber
Create a redmine ticket: https://redmine.openinfosecfoundation.org/issues/6788
Cool, could you rebase this PR, and mention this ticket number in the commit message ?