René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"Rework inheritance for cascading scans","articleBody":"## ➹ New Feature implementation request\r\nWhile 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:\r\n\r\nAssume 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.\r\n\r\nGiven the currently expected behavior of cascading scans and inheritance, the updated labelSelector [should be \"purged\"](https://github.com/secureCodeBox/secureCodeBox/blob/1c500f84881fede57740c963bda98b8defa6f3ba/hooks/cascading-scans/hook/scan-helpers.ts#L144-L146), 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](https://github.com/secureCodeBox/secureCodeBox/blob/main/hooks/cascading-scans/hook/scan-helpers.ts#discussion_r739178568) due to limitations in \"how far back\" we can iterate to the originating scans.\r\n\r\n@EndPositive [made some suggestions on how this could be addressed](https://github.com/secureCodeBox/secureCodeBox/blob/main/hooks/cascading-scans/hook/scan-helpers.ts#discussion_r740009166), quoted here:\r\n\r\n\u003e ## Option 1 - Retrieve root scan to create cascaded scan\r\n\u003e 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.\r\n\u003e \r\n\u003e ## Option 2 - Add \"Engagement\" CRD for global entries\r\n\u003e 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.\r\n\u003e \r\n\u003e ## Option 3 - Specify inheritance per entry\r\n\u003e 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.\r\n\r\nI 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](https://github.com/secureCodeBox/secureCodeBox/blob/main/hooks/cascading-scans/hook/scan-helpers.ts#discussion_r738349922), quoted below:\r\n\r\n\u003e - `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.\r\n\u003e - `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 -\u003e remote system enumeration through).\r\n\u003e - `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`.\r\n\u003e - `discardTemporary`: the initial scan specifies that DefectDojo should not run. However you still want to run DefectDojo for `nmap` findings. Think of `amass` -\u003e `nmap` -\u003e `sslyze`. `nmap`'s cascading rule temporarily discard the initial scan's hook selector, and `sslyze` retrieves it again (if it enabled a merge option).\r\n\r\nThis 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.\r\n\r\nThis 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_.","author":{"url":"https://github.com/malexmave","@type":"Person","name":"malexmave"},"datePublished":"2021-11-01T08:30:41.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":1},"url":"https://github.com/789/secureCodeBox/issues/789"}