[Scenario] Trigger bucket capacity improvement

[LaurenceJJones]

          > I believe it doesn't matter what you set capacity it will default to -1 but like you said can be an improvement if it cant be omitted

It bombs out (does not start) if I set it to trigger but capacity is set to non-0. Bug? Feature? I'm not sure:

time="14-10-2023 08:50:21" level=fatal msg="reload handler failure: unable to init crowdsec: while loading scenarios: scenario loading failed: loading of crowdsecurity/WHATEVER failed : invalid bucket from /etc/crowdsec/scenariosWHATEVER.yaml : trigger bucket must have 0 capacity"

Originally posted by @adam-ah in https://github.com/crowdsecurity/crowdsec/issues/2534#issuecomment-1764074547

We should improve this by allowing the user not to set a capacity, if they intentionally put it as trigger then we should default the capacity instead of failing to run

[github-actions[bot]]

@LaurenceJJones: Thanks for opening an issue, it is currently awaiting triage.

In the meantime, you can:

1. Check Crowdsec Documentation to see if your issue can be self resolved.
2. You can also join our Discord.
3. Check Releases to make sure your agent is on the latest version.

Details

I am a bot created to help the [crowdsecurity](https://github.com/crowdsecurity) developers manage community feedback and contributions. You can check out my [manifest file](https://github.com/crowdsecurity/crowdsec/blob/master/.github/governance.yml) to understand my behavior and what I can do. If you want to use this for your project, you can check out the [BirthdayResearch/oss-governance-bot](https://github.com/BirthdayResearch/oss-governance-bot) repository.

[github-actions[bot]]

@LaurenceJJones: There are no 'kind' label on this issue. You need a 'kind' label to start the triage process.

• /kind feature
• /kind enhancement
• /kind bug
• /kind packaging

Details

I am a bot created to help the [crowdsecurity](https://github.com/crowdsecurity) developers manage community feedback and contributions. You can check out my [manifest file](https://github.com/crowdsecurity/crowdsec/blob/master/.github/governance.yml) to understand my behavior and what I can do. If you want to use this for your project, you can check out the [BirthdayResearch/oss-governance-bot](https://github.com/BirthdayResearch/oss-governance-bot) repository.

[LaurenceJJones]

Closing due to more investigation, user can omit the key example

type: trigger
name: crowdsecurity/CVE-2023-23397
description: "Detect CVE-2023-23397 from sysmon events"
filter: |
  evt.Meta.service == 'sysmon' && evt.Parsed.EventID == '1' &&
  evt.Parsed.ParentImage endsWith "\\svchost.exe" &&
  evt.Parsed.Image endsWith "\\rundll32.exe" &&
  evt.Parsed.CommandLine contains "C:\\windows\\system32\\davclnt.dll,DavSetCookie" &&
  evt.Parsed.CommandLine matches '://\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}' &&
  (not (evt.Parsed.CommandLine contains "://10." ||
        evt.Parsed.CommandLine contains "://192.168." ||
        evt.Parsed.CommandLine contains "://172.16." ||
        evt.Parsed.CommandLine contains "://172.17." ||
        evt.Parsed.CommandLine contains "://172.18." ||
        evt.Parsed.CommandLine contains "://172.19." ||
        evt.Parsed.CommandLine contains "://172.20." ||
        evt.Parsed.CommandLine contains "://172.21." ||
        evt.Parsed.CommandLine contains "://172.22." ||
        evt.Parsed.CommandLine contains "://172.23." ||
        evt.Parsed.CommandLine contains "://172.24." ||
        evt.Parsed.CommandLine contains "://172.25." ||
        evt.Parsed.CommandLine contains "://172.26." ||
        evt.Parsed.CommandLine contains "://172.27." ||
        evt.Parsed.CommandLine contains "://172.28." ||
        evt.Parsed.CommandLine contains "://172.29." ||
        evt.Parsed.CommandLine contains "://172.30." ||
        evt.Parsed.CommandLine contains "://172.31." ||
        evt.Parsed.CommandLine contains "://127." ||
        evt.Parsed.CommandLine contains "://169.254."))
labels:
 notification: true
 os: windows
scope:
  type: user_account
  expression: evt.Parsed.User

You could argue that we should ignore these errors, however, the team has decided that it better to error instead of bypassing it as the user could have a unexpected outcome

[adam-ah]

The current one is surprising and allows creating illogical / uninterpretable setups. These are the only two logical approaches:

• drop the trigger concept, a bucket with 0 capacity does exactly the same (why have another keyword??)
• enforce omitting capacity on trigger: it is not 0, but uninterpretable: trigger does not have a capacity, so any capacity setting should bomb out

All other combinations (including the current one, such as enforcing a specific value; or a new one, such as ignoring the value) are not correct and you couldn't argue that they are.

The only explanation for the current setup is that someone somewhere created a not-too-logical PR, and someone else somewhere else approved it, because a unit test passed; but not because it was the best approach, but because PRs tend to get approved even if they aren't that great (unfortunately, this is the all-too-common faulty reasoning that leaves most illogical things in software).

EDIT OK, I checked, it was indeed the reason: 3 years ago there was the usual "it's too big let's just merge it" kind of change coming in, changing over 500 files: https://github.com/crowdsecurity/crowdsec/pull/482 Good luck arguing that :)

[blotus]

Hello,

• drop the trigger concept, a bucket with 0 capacity does exactly the same (why have another keyword??)

I feel it would make things confusing: yes, a trigger bucket can be seen as a shortcut for a leaky bucket with a 0 capacity (and no leakspeed by extension), but currently, leaky buckets require a capacity > 0 and having a leakspeed (which does not make sense in the context of trigger buckets, as there is nothing to remove from the bucket).

While we could handle specifically the case of a leaky bucket having a 0 capacity (and allowing no leakspeed in this case), it could lead to unexpected behaviors from the user POV, and having a dedicated keyword for this makes things much easier to understand (when looking at a scenario, if you see trigger, you know you don't have to look for a capacity in the file, while if we were to allow doing trigger-like buckets with a leaky, you would need to find another value to actually understand how the scenario works)

• enforce omitting capacity on trigger: it is not 0, but uninterpretable: trigger does not have a capacity, so any capacity setting should bomb out

Explicitly refusing a capacity: 0 for a trigger bucket would be a breaking change, and we try to avoid them (AFAIK, we never had one in the scenarios) : we have no idea if someone out there has not set it explicitly in a custom scenario, and functionally, it would be the same behavior as currently, just much more restrictive (and from my point of view, the argument is not strong enough to justify a breaking change). Internally, a bucket (no matter the type) will always default to a 0 capacity if not set in the scenario file. If you explicitly create a trigger bucket with a capacity of anything other than 0, crowdsec will refuse to load the scenario because it is invalid.

And for completeness' sake, to sum up the behavior of the various buckets regarding the capacity:

• leaky: It must have a capacity > 0, so you have to set the value in the scenario file.
• trigger: It must have a capacity of 0, so you can either omit the capacity or set it to 0. Any other value will return an error.
• conditional: Capacity can be whatever you want, but crowdsec will warn you if it is not -1 (a special value telling crowdsec the bucket cannot overflow purely based on the number of events), as we consider that there are use cases where you want to overflow on either the condition or the number of events still in the bucket (To be fair, this one does allow to have a 0 capacity, but i'd be inclined to consider this a bug that we need to fix as it makes little to no sense in this case).
• bayesian: Capacity must be -1, as this bucket is designed to compute some probabilities based on the events previously poured, overflowing based on capacity should never happen.
• counter: Capacity must be -1, as this bucket is intended to count things over time, so it cannot overflow "naturally". It will always overflow at set intervals of time.