flow/output: log triggered exception policy - v1 #12683
Conversation
To accompany the Exception Policy stats, also add information about any Exception Policy triggered and for which target to the flow log event. Task OISF#6215
The stats for exception policies are only logged/ present when any of the exception policies are enabled (which means any value other than "auto" or "ignore" in IDS mode, or "ignore" in IPS mode). This wasn't clearly stated in the docs.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12683 +/- ##
==========================================
- Coverage 80.75% 80.71% -0.05%
==========================================
Files 934 935 +1
Lines 259594 259475 -119
==========================================
- Hits 209634 209431 -203
- Misses 49960 50044 +84
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
WARNING:
Pipeline 24919 |
| @@ -40,6 +40,15 @@ enum ExceptionPolicy { | |||
| * "tcp.reassembly_exception_policy.drop_packet" + 1 */ | |||
| #define EXCEPTION_POLICY_COUNTER_MAX_LEN 45 | |||
|
|
|||
| /** exception policy flags */ | |||
| #define EXCEPTION_DEFRAG_MEMCAP BIT_U16(1) | |||
There was a problem hiding this comment.
add a EXCEPTION_NONE BIT_U16(0) here?
There was a problem hiding this comment.
actually just 0, the other bit should start at offset 0
BIT_U16(0) is just 1 << 0 == 1
| case PKT_DROP_REASON_APPLAYER_ERROR: | ||
| return EXCEPTION_APPLAYER_ERROR; | ||
| default: | ||
| return BIT_U16(0); |
There was a problem hiding this comment.
then use that EXCEPTION_NONE here?
| void ExceptionPolicyApply(Packet *p, enum ExceptionPolicy policy, enum PacketDropReason drop_reason) | ||
| { | ||
| SCLogDebug("start: pcap_cnt %" PRIu64 ", policy %u", p->pcap_cnt, policy); | ||
| if (p->flow) { | ||
| p->flow->flags |= FLOW_TRIGGERED_EXCEPTION_POLICY; | ||
| p->flow->applied_exception_policy |= ExceptionPolicyFlag(drop_reason); |
There was a problem hiding this comment.
when would we have more than one? Or should this be a simple assignment instead?
In fact, if it is possible that this is already set, what do that mean?
There was a problem hiding this comment.
when would we have more than one? Or should this be a simple assignment instead?
Should it maybe be a counter, then?
And then if we have it >1, we check all flags that match?
In fact, if it is possible that this is already set, what do that mean?
This did cross my mind, but I wasn't sure this was a reasonable concern, as I didn't recall seeing such a scenario. (although probably because I didn't look for such situations when creating most tests/checks)
There was a problem hiding this comment.
Could applied_exception_policy itself work as the counter, maybe?
There was a problem hiding this comment.
for now, going with a different approach (inspired by the exception policy counters struct)
| @@ -480,6 +481,9 @@ typedef struct Flow_ | |||
| * has been set. */ | |||
| const struct SigGroupHead_ *sgh_toserver; | |||
|
|
|||
| /** which exception policy was applied, if any */ | |||
There was a problem hiding this comment.
by putting this between 2 pointers we're essentially use 8 bytes for this, which is a lot for something from which we just use 6 bits
There was a problem hiding this comment.
Sorry, I didn't realize that. >__<'
Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.
Changes (if applicable):
(including schema descriptions)
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/6215
Describe changes:
behavioral:
exception_policy_triggeredobject to flow eve logs, which is logged when an exception policy is triggeredimplementation-related:
uint16as we already plan on adding more targets, so auint8soon wouldn't be enoughsample output:
Minor:
Provide values to any of the below to override the defaults.
SV_BRANCH=OISF/suricata-verify#2325