detect/proto: check ipproto enabled setting first

[inashivb]

Previous PR: #14713

Link to ticket: https://redmine.openinfosecfoundation.org/issues/8205

Changes since v3:

• fixed docs yaml key
• rebased on top of latest main

SV_BRANCH=OISF/suricata-verify#2893

Note: This should NOT affect any existing setups and configs. If you see that happening, it should be reported as a bug.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.15%. Comparing base (2cf9a32) to head (501af81).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #14715   +/-   ##
=======================================
  Coverage   82.14%   82.15%           
=======================================
  Files        1007     1007           
  Lines      263194   263194           
=======================================
+ Hits       216210   216228   +18     
+ Misses      46984    46966   -18     

Flag	Coverage Δ
fuzzcorpus	60.19% <100.00%> (ø)
livemode	18.85% <100.00%> (-0.02%)	⬇️
pcap	44.61% <100.00%> (+<0.01%)	⬆️
suricata-verify	65.35% <100.00%> (-0.02%)	⬇️
unittests	59.36% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[suricata-qa]

Information: QA ran without warnings.

Pipeline = 29335

[victorjulien]

Note: This should NOT affect any existing setups and configs. If you see that happening, it should be reported as a bug.

If this is correct, why do we see the SV tests fail w/o this PR?

[inashivb]

Note: This should NOT affect any existing setups and configs. If you see that happening, it should be reported as a bug.

If this is correct, why do we see the SV tests fail w/o this PR?

Great question. I should have clarified that s-v tests specifically use different combinations to show behaviors when we have inconsistent settings. Such settings are not shipped by us in suricata.yaml by default.
e.g. for dns (as done in tests) by default we have (ref: https://github.com/OISF/suricata/blob/main/suricata.yaml.in#L1082):

    dns:
      tcp:
        enabled: yes
        detection-ports:
          dp: 53
      udp:
        enabled: yes
        detection-ports:
          dp: 53

but the tests go on to show the bug as defined in the ticket and commit message in case a global enabled was also set up in a combination with specific tcp and udp settings.

So, in conclusion, maybe my claim is wrong. It is based on the assumption that one would only use protocol enabling setting as shipped with default suricata.yaml. One could also use those inconsistent settings which would indeed lead to a change in behavior.

[victorjulien]

Right, so I would classify this as a behavior change, even if it is one at a low risk of affecting many users. Still I would say it requires at least an upgrade note.

[victorjulien]

Are we warning on settings that are internally inconsistent?

[inashivb]

Are we warning on settings that are internally inconsistent?

No. Maybe I misunderstood and thought that we decided to:

1. look for specific ipproto setting
2. if not found, silently fallback to global setting as the default

I can do warnings as well. That'll require reading all settings irrespective of a need for fallback. Coming up in the next rev.

Thank you! 🙇🏽‍♀️

[victorjulien]

I think for example global enabled but then all ipprotos disabled should warn.

Same for reverse.

[catenacyber]

See inline like

Still I would say it requires at least an upgrade note.

[inashivb]

I think for example global enabled but then all ipprotos disabled should warn.

These generic fn calls are per ipproto supported by the respective applayer parser so, going with:

• global enabled but ipproto in view disabled -> warn
• global disabled but ipproto in view enabled -> warn