-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 #9127
Decouple stream.bypass dependency from TLS encrypted bypass #9127
Conversation
Our git policy is to have a linear git history, so no merge branches. |
I'm not sure I understand what you mean. |
NOTE: This PR may contain new authors:
|
f3fc42b
to
8056b65
Compare
adfe129
to
d231273
Compare
NOTE: This PR may contain new authors:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
d231273
to
b1cdc2c
Compare
Thanks! :) yeah, I noticed when going back to reference it that this part should probably be highlighted, somehow, it's not very visible... |
NOTE: This PR may contain new authors:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit message related changes look good to me :)
Signed the contribution agreement today. |
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? |
git commit typo: "unrleated" |
de2859c
to
fd8a333
Compare
NOTE: This PR may contain new authors:
|
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 :) |
fd8a333
to
6107332
Compare
rebased last commit to re-trigger CI |
NOTE: This PR may contain new authors:
|
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
Ticket 6434
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.
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 ? |
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.
Sorry, I meant could you open a new PR with the rebased code ? |
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. |
New PR (closing this one): |
Make sure these boxes are signed before submitting your Pull Request -- thank you.
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:
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
whereN
is thepull request number.
Alternatively,
SV_BRANCH
may also be a link to anOISF/suricata-verify pull-request.