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

IDS: Add checkbox for exception-policy #7271

Closed
wants to merge 6 commits into from

Conversation

mimugmail
Copy link
Member

Hi,

Suricata 7 added exeception policies:
https://docs.suricata.io/en/latest/configuration/exception-policies.html#exception-policies

When just upgrading to 24.1.2 there is no entry in suricata.yml which seems to activate this feature:
https://forum.suricata.io/t/my-traffic-gets-blocked-after-upgrading-to-suricata-7/3745

Two confirmations that when adding it to custom.yml it seems to work again in Suricata 6 behavior:
https://forum.opnsense.org/index.php?topic=38961.0
https://forum.opnsense.org/index.php?topic=38944.0

There might arise more problems e.g. when running openvpn on port 443 as it also does not speak https.

Nonetheless, in the official documentation it states that the default is ignore but it only seems to act like v6 when adding it explicitly, so maybe we can also set it to enabled by default, or just activate it in suricata.yml without a knob and let the decision up to you guys :)
https://docs.suricata.io/en/latest/configuration/exception-policies.html#exception-policies

@AdSchellevis AdSchellevis self-assigned this Feb 22, 2024
@AdSchellevis
Copy link
Member

defaults in their documentation seem to be rather convoluted this release, not sure what happened there, but for now I prefer setting these values to their advertised defaults to prevent issues and additional user interaction. If at one point we do want to offer access to these parameters, we should add all possible options instead of a checkbox anyway.

For reference, the sample yaml also seems to suggest ignore as being default (an not needed to set):
https://github.com/OISF/suricata/blob/69f0e85785e03ef344e56c16929a9e803c2894ab/suricata.yaml.in#L858-L862

I'll close this ticket with a commit to set it to the default, next it might also be a good idea to compare differences between our default yaml and theirs, but that's probably for a next minor release.

@kulikov-a
Copy link
Member

@mimugmail @AdSchellevis Hi
im afraid that setting the error-policy: ignore (only app layer behavior fallback) will not fix possible flow or defrag drops (since the master-switch switched all policies to drop)?
OISF/suricata@0d92890
if the goal is to temporarily return to the previous behavior (before adding all the policies options to the GUI), maybe it would be more logical to set the master switch to ignore (exception-policy: ignore)?

@AdSchellevis
Copy link
Member

@kulikov-a I'm not sure that's needed now as we also added ce87c2f and 0d676c7 to ignore midstream exceptions. Are there know cases where this 9b82093 doesn't fix the current issues?

@kulikov-a
Copy link
Member

@AdSchellevis

Are there know cases where this 9b82093 doesn't fix the current issues?

no actually. just a precaution in case the memcap hit

AdSchellevis added a commit that referenced this pull request Feb 25, 2024
@AdSchellevis
Copy link
Member

@kulikov-a Maybe we should go in phases then, 4cf6870 aligns the suricata.yaml to upstream, 36b2b66 sets it to ignore as suggested as a precaution. Both should probably go in to a minor release, but not the next one....

@kulikov-a
Copy link
Member

@AdSchellevis sounds logical ) Thanks!

fichtner pushed a commit that referenced this pull request Feb 27, 2024
…olicy to it's advertised default. closes #7271 and #7276

(cherry picked from commit 9b82093)
fichtner pushed a commit that referenced this pull request Feb 27, 2024
fichtner pushed a commit that referenced this pull request Mar 12, 2024
fichtner pushed a commit that referenced this pull request Mar 12, 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.

None yet

3 participants