➹ New Feature implementation request

While discussing #757, we found a few strange corner cases for how inheritance works in cascading scans, located in the logic for purging inherited values. Briefly, the problem looks like this:

Assume we have a scan with the label-based HookSelector defectdojo: false (to avoid running a DefectDojo hook that has this label). The scan then triggers a cascadingScan that overwrites the label-based selector with defectdojo: true. This cascaded scan then triggers another cascadingScan rule.

Given the currently expected behavior of cascading scans and inheritance, the updated labelSelector should be "purged", i.e., changes introduced by the first cascading scan should be reverted back to the state of the original scan. However, the current implementation will instead delete the label entirely due to limitations in "how far back" we can iterate to the originating scans.

@EndPositive made some suggestions on how this could be addressed, quoted here:

Option 1 - Retrieve root scan to create cascaded scan

Using ownerReferences, the Cascading Scans hook could recurse all the way back to the initial/root scan. Then merge the entries from the matched Cascading Rule.

Option 2 - Add "Engagement" CRD for global entries

We could add a global CRD where we define global entries that should be inherited to each scan within that engagement. Cascading Scans hook retains same behaviour as Option 1 but now retrieving the Engagement resource.

Option 3 - Specify inheritance per entry

For ultimate customization, users should be able to configure inheritance per entry. We could update our CRD's such that each env, initContainer, etc. contains an extra field persist which controls whether that entry should be passed on to cascaded scans. Kubernetes map types (e.g. labels) would be converted into structs.

I would add that as a novice SCB user, I was surprised that the purge functionality even existed when I was reviewing that PR - this was not what I expected the system to behave like at all. So, perhaps we should figure out if this whole purging thing should even take place by default. @EndPositive also had some ideas for specifying a larger set of possible inheritance behaviors, quoted below:

• merge: simply keeps adding matchers to each cascaded scan. Not sure if this has an actual use case, but this was default behavior for a long time.
• mergeAndPurge: required when the rule's entries should only be applied to the current scan. Most useful when you want to add environment variables to a cascaded scan, but not to the 'children' of that cascaded scan. Example would be scanner configuration enthronement variables, which only apply to the current scanner and not the child scanners. Same could be said about the hookSelector, for which you might want to disable DefectDojo import for a cascaded scan (like ncrack), but not for the 'children' of that cascaded scan (ncrack ssh -> remote system enumeration through).
• discard: when your initial scan has scanner specific entries that should not come over to cascaded scans, the cascaded scan can choose to ignore/discard those entries. E.g. you add environment variables to amass for private DNS authentication, but don't want to have these environment variables in cascaded nmap.
• discardTemporary: the initial scan specifies that DefectDojo should not run. However you still want to run DefectDojo for nmap findings. Think of amass -> nmap -> sslyze. nmap's cascading rule temporarily discard the initial scan's hook selector, and sslyze retrieves it again (if it enabled a merge option).

This issue is likely closer to an epic than a user story. @J12934 was also saying that the whole cascading scan thing being implemented as a hook instead of a core feature of the operator is more of a historical accident as well, and one that could be changed if someone wanted to invest some time into pulling the behavior into the operator itself. But that is a whole separate issue, which I just wanted to drop in here because it may influence how we think about these changes.

This may be a candidate for a version 4.0 of the SCB, as I am fairly sure that addressing these points will lead to a breaking change somewhere.