Decouple stream.bypass dependency from TLS encrypted bypass #9127

msdean · 2023-07-02T09:49:16Z

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

I have read the contributing guide lines at https://suricata.readthedocs.io/en/latest/devguide/codebase/contributing/contribution-process.html
I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
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=

victorjulien · 2023-07-02T10:30:25Z

Our git policy is to have a linear git history, so no merge branches.

msdean · 2023-07-02T10:36:27Z

I'm not sure I understand what you mean.

victorjulien · 2023-07-02T10:39:56Z

Note the "merge branch" here

github-actions · 2023-07-02T10:40:44Z

NOTE: This PR may contain new authors:

Dean Balandin <[email protected]>
msdean <[email protected]>

msdean · 2023-07-02T10:48:15Z

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?

github-actions · 2023-07-02T12:33:40Z

NOTE: This PR may contain new authors:

Dean Balandin <[email protected]>

jufajardini

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

msdean · 2023-07-04T08:02:01Z

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.

jufajardini · 2023-07-04T12:35:23Z

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.

Thanks! :) yeah, I noticed when going back to reference it that this part should probably be highlighted, somehow, it's not very visible...

github-actions · 2023-07-04T12:36:06Z

NOTE: This PR may contain new authors:

Dean Balandin <[email protected]>

jufajardini

Commit message related changes look good to me :)

msdean · 2023-08-02T07:26:52Z

Signed the contribution agreement today.
Can someone please review and approve the PR?
Thank you.

jufajardini · 2023-08-02T19:29:36Z

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.

victorjulien · 2023-08-05T09:22:10Z

Can you make sure the email used to sign the CLA matches the git author email?

victorjulien · 2023-08-05T09:22:34Z

git commit typo: "unrleated"

github-actions · 2023-08-09T16:26:49Z

NOTE: This PR may contain new authors:

Dean Balandin <[email protected]>

jufajardini · 2023-08-09T18:49:29Z

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 :)

msdean · 2023-08-13T13:51:03Z

rebased last commit to re-trigger CI

github-actions · 2023-08-14T23:40:32Z

NOTE: This PR may contain new authors:

Dean Balandin <[email protected]>

Dependabot is always getting flagged as a new author even tho it uses a consistent author of: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> But this doesn't work with plain grep. Fix by telling grep to treat the value as a fixed string instead of a regular expression.

Direction flag was checked against wrong field, leading to undefined behavior. Bug: OISF#6778.

Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2.24.0 to 3.24.1. - [Release notes](https://github.com/github/codeql-action/releases) - [Commits](github/codeql-action@v2.24.0...v3.24.1) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>

This commit improves the mqtt parsing of frames to handle multiple PDUs. Issue: 6592

Rework locking logic to avoid the following coverity warning. ** CID 1591966: Concurrent data access violations (MISSING_LOCK) /src/detect-engine-loader.c: 475 in DetectLoadersSync() 474 SCCtrlMutexLock(loader->tv->ctrl_mutex); >>> CID 1591966: Concurrent data access violations (MISSING_LOCK) >>> Accessing "loader->tv" without holding lock "DetectLoaderControl_.m". Elsewhere, "DetectLoaderControl_.tv" is written to with "DetectLoaderControl_.m" held 1 out of 1 times (1 of these accesses strongly imply that it is necessary). 475 pthread_cond_broadcast(loader->tv->ctrl_cond); 476 SCCtrlMutexUnlock(loader->tv->ctrl_mutex); The warning itself is harmless.

Ticket: 6617 So that rules with keyword like `filestore:to_server,flow` only store the files to server and not the ones to client... Directionality only worked with the default scope, ie the current file, and not the scope tx or scope flow. For non-default scope, tx or flow, both directions were stored whatever the directionality specified. For these non-default scopes, this commit keeps a default of both directions, but use only one direction if specified. Need to split flag FLOWFILE_STORE per direction, so that Suricata can retain this (optional) directional info from the filestore keyword. Fixes: 79499e4 ("app-layer: move files into transactions")

The runtime complexity of insertion sort is approx. O(h*n)^2 where h is the size of the HOME_NET and n is the number of ip only rules that use the HOME_NET. Replacing this with qsort significantly improves rule load time when a large HOME_NET is used in combination with a moderate amount of ip only rules.

When an interface with dots is used, per worker stats are nested by the dot-separated-components of the interface due to the usage of OutputStats2Json(). Prevent this by using OutputStats2Json() on a per-thread specific object and setting this object into the threads object using the json_object_set_new() which won't do the dot expansion. This was tested by creating an interface with dots in the name and checking the stats. ip link add name a.b.c type dummy With Suricata 7.0.2, sniffing on the a.b.c interface results in the following worker stats format: "threads": { "W#01-a": { "b": { "c": { "capture": { "kernel_packets": 0, After this fix, the output looks as follows: "threads": { "W#01-a.b.c": { "capture": { "kernel_packets": 0, Ticket: OISF#6732

Main purpose is to validate that the 30 of bond0.30 isn't expanded into a nested object during serialization.

No shared resource is being changed when the lock is held, it is immediately unlocked. So, remove it.

Ticket 6434

Also put "cocci" in the job name and install parallel so the script can actually run with concurrency.

If CONCURRENCY_LEVEL was set, the script would log a concurrency level even if the parallel command was not available. Not log if parallel is not available and set concurrency to 1.

msdean · 2024-02-19T12:28:25Z

catenacyber

Create a redmine ticket: https://redmine.openinfosecfoundation.org/issues/6788

catenacyber · 2024-02-19T14:41:30Z

Cool, could you rebase this PR, and mention this ticket number in the commit message ?

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.

…n.org/issues/6788

catenacyber · 2024-02-19T15:15:58Z

Sorry, I meant could you open a new PR with the rebased code ?

jufajardini · 2024-02-19T15:17:27Z

hey @msdean I think the rebasing picked too many commits here :P

msdean · 2024-02-19T15:20:33Z

hey @msdean I think the rebasing picked too many commits here :P

yea yea, I'm creating a new clean PR from master and cherry-picking the relevant commits.
I'll link it here and close this PR. Sorry for the mess.

msdean · 2024-02-19T15:25:14Z

New PR (closing this one):
#10464

msdean requested review from jufajardini and victorjulien as code owners July 2, 2023 09:49

msdean force-pushed the decouple_stream-bypass_from_tls-encrypted-bypass-v1 branch from f3fc42b to 8056b65 Compare July 2, 2023 10:46

msdean force-pushed the decouple_stream-bypass_from_tls-encrypted-bypass-v1 branch 2 times, most recently from adfe129 to d231273 Compare July 2, 2023 10:59

jufajardini reviewed Jul 3, 2023

View reviewed changes

msdean force-pushed the decouple_stream-bypass_from_tls-encrypted-bypass-v1 branch from d231273 to b1cdc2c Compare July 4, 2023 08:05

jufajardini approved these changes Jul 7, 2023

View reviewed changes

victorjulien added this to the 8.0 milestone Jul 11, 2023

msdean force-pushed the decouple_stream-bypass_from_tls-encrypted-bypass-v1 branch 2 times, most recently from de2859c to fd8a333 Compare August 7, 2023 07:22

msdean force-pushed the decouple_stream-bypass_from_tls-encrypted-bypass-v1 branch from fd8a333 to 6107332 Compare August 13, 2023 13:50

catenacyber added the needs ticket Needs (link to) redmine ticket label Aug 30, 2023

jasonish and others added 17 commits February 14, 2024 07:04

detect/tls.certs: fix direction handling

3c06457

Direction flag was checked against wrong field, leading to undefined behavior. Bug: OISF#6778.

mqtt: Improve frame parsing w/mult. PDUs

f9a20da

This commit improves the mqtt parsing of frames to handle multiple PDUs. Issue: 6592

ci: right sha for authors check

41a6211

stats: Add unittest for basic stats serialization

08db0f3

Main purpose is to validate that the 30 of bond0.30 isn't expanded into a nested object during serialization.

multi-tenant: remove futile mutex lock

7477307

No shared resource is being changed when the lock is held, it is immediately unlocked. So, remove it.

eve/stats: add description for common fields

5a1a32b

Ticket 6434

eve/stats: add description for applayer errors

1816e98

Ticket 6434

eve/stats: add description for expectations

8817514

Ticket 6434

eve/stats: add description for applayer flows

487ba82

Ticket 6434

github-ci: use all cpus for coccinelle checks

6198ea5

Also put "cocci" in the job name and install parallel so the script can actually run with concurrency.

cocci/run-check: log if parallel command is not found

f7114b7

If CONCURRENCY_LEVEL was set, the script would log a concurrency level even if the parallel command was not available. Not log if parallel is not available and set concurrency to 1.

msdean and others added 3 commits February 19, 2024 15:11

stream: decouple stream.bypass dependency from tls bypass

1c11a19

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.

userguide: update encrypted traffic bypass

6bddaef

Update documentation to reflect the new features and changes.

Rebase to master, redmine ticket https://redmine.openinfosecfoundatio…

84afef8

…n.org/issues/6788

msdean requested review from jasonish and a team as code owners February 19, 2024 15:14

msdean mentioned this pull request Feb 19, 2024

Decouple stream.bypass dependency from TLS encrypted bypass #10464

Closed

3 tasks

msdean closed this Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple stream.bypass dependency from TLS encrypted bypass #9127

Decouple stream.bypass dependency from TLS encrypted bypass #9127

msdean commented Jul 2, 2023 •

edited

Loading

victorjulien commented Jul 2, 2023

msdean commented Jul 2, 2023

victorjulien commented Jul 2, 2023

github-actions bot commented Jul 2, 2023

msdean commented Jul 2, 2023

github-actions bot commented Jul 2, 2023

jufajardini left a comment •

edited

Loading

msdean commented Jul 4, 2023

jufajardini commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

jufajardini left a comment

msdean commented Aug 2, 2023

jufajardini commented Aug 2, 2023

victorjulien commented Aug 5, 2023

victorjulien commented Aug 5, 2023

github-actions bot commented Aug 9, 2023

jufajardini commented Aug 9, 2023

msdean commented Aug 13, 2023 •

edited

Loading

github-actions bot commented Aug 14, 2023

msdean commented Feb 19, 2024

catenacyber commented Feb 19, 2024

catenacyber commented Feb 19, 2024

jufajardini commented Feb 19, 2024

msdean commented Feb 19, 2024 •

edited

Loading

msdean commented Feb 19, 2024

Decouple stream.bypass dependency from TLS encrypted bypass #9127

Decouple stream.bypass dependency from TLS encrypted bypass #9127

Conversation

msdean commented Jul 2, 2023 • edited Loading

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

victorjulien commented Jul 2, 2023

msdean commented Jul 2, 2023

victorjulien commented Jul 2, 2023

github-actions bot commented Jul 2, 2023

msdean commented Jul 2, 2023

github-actions bot commented Jul 2, 2023

jufajardini left a comment • edited Loading

Choose a reason for hiding this comment

msdean commented Jul 4, 2023

jufajardini commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

jufajardini left a comment

Choose a reason for hiding this comment

msdean commented Aug 2, 2023

jufajardini commented Aug 2, 2023

victorjulien commented Aug 5, 2023

victorjulien commented Aug 5, 2023

github-actions bot commented Aug 9, 2023

jufajardini commented Aug 9, 2023

msdean commented Aug 13, 2023 • edited Loading

github-actions bot commented Aug 14, 2023

msdean commented Feb 19, 2024

catenacyber commented Feb 19, 2024

catenacyber commented Feb 19, 2024

jufajardini commented Feb 19, 2024

msdean commented Feb 19, 2024 • edited Loading

msdean commented Feb 19, 2024

msdean commented Jul 2, 2023 •

edited

Loading

jufajardini left a comment •

edited

Loading

msdean commented Aug 13, 2023 •

edited

Loading

msdean commented Feb 19, 2024 •

edited

Loading