Introduce scope limiter on cascading rules to enforce scope #805

EndPositive · 2021-11-10T15:05:40Z

Closes #761

Description

This PR introduces ScopeLimiter on CascadeSpec so to match expressions can be added to enforce scope on an engagement. In #761, this field was thought to be added to the CascadingRuleSpec, but I've decided against it because the scope is usually defined on an engagement, instead of on a specific rule. This results in the following scan spec:

apiVersion: "execution.securecodebox.io/v1"
kind: Scan
metadata:
  name: "nmap-scan"
  annotations:
    scope.cascading.securecodebox.io/domain: "secura.com" 
spec:
  cascades:
    scopeLimiter:
      allOf:
      - key: "scope.cascading.securecodebox.io/domains"
        operator: "In"
        values: ["{{attributes.hostname}}"]
  scanType: "nmap"
  parameters:
    - "-p-"
    - "136.144.234.6"

This scan results in a finding with the hostname 136-144-234-6.colo.transip.net, which I don't want to scan, because my scope is only the domain secura.com. With the PR, the scopeLimiter field is evaluated and cascading scans would not be triggered.

To ensure that you can define the selector independent of the finding format, I've added a new field scopeLimiterAliases in the ParseDefinitionSpec. This field allows you to define aliases per scanner. This field should be addable through values.yaml in each scanner (not yet implemented). The following alias could be added to nmap's parse definition.

scopeLimiterAliases:
  hostname: "{{attributes.hostname}}"

The scan definition would be:

scopeLimiter:
  allOf:
  - key: "scope.cascading.securecodebox.io/domains"
    operator: "In"
    values: ["{{$.hostname}}"]

Since a hostname might not be available for every parser, I've added the field ValidOnMissingRender so that an expression containing a mustache value which could not be rendered, resolves to true. This behavior is disabled by default. The following expression would result to true.

scopeLimiter:
  validOnMissingRender:  true
  allOf:
  - key: "scope.cascading.securecodebox.io/domains"
    operator: "In"
    values: ["{{invalid_field}}"]

The allOf, anyOf, and noneOf are independently supported. The following operator are supported: In, NotIn, Exists, DoesNotExist, Contains, DoesNotContain, InCIDR, NotInCIDR, SubdomainOf, NotSubdomainOf.

I'd like some feedback on this particular approach 😄.

Todo

Initial concept implementation
Create documentation PR
Rename field scanAnnotationSelector
Add values field for selectorAttributeMappings.

Checklist

Test your changes as thoroughly as possible before you commit them. Preferably, automate your test by unit/integration tests.
Make sure npm test runs for the whole project.
Make codeclimate checks happy

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Signed-off-by: Jop Zitman <jop.zitman@secura.com> # Conflicts: # hooks/cascading-scans/hook/hook.test.js

malexmave · 2021-11-12T10:38:22Z

Thanks for your effort already! Some initial thoughts on what you wrote:

I like the idea of shifting this from the CascadingScan definition to the Scan definition, makes a lot more sense there in principle, and it should work fine for single-stage cascading scans (i.e., a scan triggers a CascadingScan, and then you're done). In this case, a single generic cascading scan definition is sufficient for multiple engagements.

As soon as you have cascading scans that trigger additional cascading scans, it gets a bit more painful if you want to keep enforcing scope further down the chain. You'll need to write a custom cascading scan for each engagement after all, to specify the scope for further cascades in the ScanSpec, and you'll also need to start working with the hook selector features you recently contributed to avoid having cascading rules from different engagements trigger on the same scan, so at that point we're back to where we were before moving this to the Scan definition in terms of the complexity of setting things up. But since it is never worse than previously specified, and sometimes better, and it makes more intuitive sense, I think that moving it is a good thing.

One place where this new behavior may be worse than specifying it on a per-hook basis is this: If I have some complex engagement rules that could only be modelled as a nested boolean formula ("(A AND B) OR (C AND D)"), this presumably cannot be represented with the syntax you are building (or can you have an anyOf with allOf as a child?). With the hook-based notation, you could just have created two cascading rules to represent this, which would have only required a single run of the scanner. With the scan-based notation, you would have to run the scanner two separate times with different scope definitions. However, I am simply saying this here so that everyone is aware of this limitation - I still think that the scan-based notation is probably superior, since such complex engagement rules are probably uncommon.

How will this interact with inheritance on cascading scans? Standard inheritance behavior (turned off by default), or will this require anything more fancy?

The new logic will be in addition to the old one (invasive/non-invasive etc.) and constraints from both will be combined, correct?

Since a hostname might not be available for every parser, I've added the field ValidOnMissingRender so that an expression containing a mustache value which could not be rendered

Since the definition for what you want to match is specified on a single scan, which has a single parser (which presumably will either always or never have a specific field in the results), how can a case appear where the field is not defined? If the field is set to undefined / null / the empty string, that is one thing (which is why we have the $.HostOrIP helper), but shouldn't at least the rendering of mustache be either always or never successful?

The allOf, anyOf, and noneOf are independently supported.

What do you mean with "independently supported"?

The following operator are supported: In, NotIn, Exists, DoesNotExist, Contains, DoesNotContain, InCIDR, NotInCIDR.

What happened to the domain-based operator (subdomainOf)? Modeling this as "Contains" / "DoesNotContain" can be misleading, since you may have external service providers that include the domain as part of their domain name (securecodebox.io.services.azure.com or something like that). In that case, having an operator that is aware of the semantics of top/2nd level domains may be useful.

EndPositive · 2021-11-12T15:25:50Z

Hello again 😄,

For Secura, it makes sense to enforce scope throughout an entire engagement and thus force inherit the scope selector and not allow overriding it in cascading rules (i.e. no merge/perge issues). I think this tackles most of the points you discussed about moving the selector from hook to scan.

Some comments below:

cascading rules from different engagements trigger on the same scan,

I'm not really sure what you mean here. We have a default set of cascading rules and use labels to match rules per customer or engagement.

The new logic will be in addition to the old one (invasive/non-invasive etc.) and constraints from both will be combined, correct?

Definitely. The labels are static matches using Kubernetes-native label selectors. These are evaluated while retrieving the rules. The annotation selectors introduced in this PR are evaluated in "runtime" and match on the scan's annotations instead of on just the findings.

Since the definition for what you want to match is specified on a single scan, which has a single parser (which presumably will either always or never have a specific field in the results), how can a case appear where the field is not defined?

The idea for SelectorAttributeMappings is that you can use the same ScanAnnotationSelector template values for multiple different scanners. An example is that you might want to select based on IP, which Nmap gives in attributes.ip and sslyze in attributes.ip_address. The $.HostOrIP variables are far too generic and oblivious of the scantype/scanner. Furthermore, these are hardcoded.. The goal with the mappings is that it should always be rendered. If it cannot be rendered, the behaviour is defined by ValidOnMissingRender.

or can you have an anyOf with allOf as a child?

Nope, not sure if that would be possible to express in CRD.

What do you mean with "independently supported"?

Code is currently like below. To support more complex scenario's we could allow specifying allOf, anyOf, and noneOf in combination and evaluating them together somehow (maybe simple AND)?

if (scanAnnotationSelector.allOf !== undefined) return scanAnnotationSelector.allOf.every(validateRequirement);

if (scanAnnotationSelector.anyOf !== undefined) return scanAnnotationSelector.anyOf.some(validateRequirement);

if (scanAnnotationSelector.noneOf !== undefined) return !scanAnnotationSelector.noneOf.some(validateRequirement);

What happened to the domain-based operator (subdomainOf)?

I'll implement it once you like the approach 😄.

malexmave · 2021-11-12T16:46:06Z

For Secura, it makes sense to enforce scope throughout an entire engagement and thus force inherit the scope selector and not allow overriding it in cascading rules (i.e. no merge/perge issues).

Hmm, I see. In cases where you can be sure that all scans that should trigger cascading scans will always return a hostname / IP that you can match on, this should work fine. And I guess there aren't any scanners that you would want to trigger additional network scans that will not return this. So, makes sense. However, the thing you describe (not allowing overriding) would make the inheritance behavior of the scope limiters different from that of the other inherited values... We had discussed that can of worms in #789. This may be a point where some input by @J12934 may be useful, as he is usually our "consistency evangelist" 😁. (Or am I misunderstanding you somewhere?)

cascading rules from different engagements trigger on the same scan,
I'm not really sure what you mean here. We have a default set of cascading rules and use labels to match rules per customer or engagement.

tldr: Its another edge case that can show up, but its solvable with hook selectors and not necessary in 99% cases, so its fine the way you propose to do it. (You may notice that I think a lot about strange edge cases)

Long version: I was still thinking about cascading rules that specify different scopes for their sub-scans - either without inheritance, or with further limitations being added down the line, like saying "You can scan all these IPs, but please leave the following ports alone for follow-up scans". Or you may have one scan that discovers hosts (e.g., based on DNS), followed by an nmap on in-scope systems, followed by a more detailed scanner that operates on specific ports, and only certain ports are in-scope. You cannot specify the ports-in-scope on the initial scan, since there is no port information anywhere in its output and so specifying the port requirements would break the cascade unless you play tricks with your "aliasing" + "ignore this if templating fails" logic. This may be a use case where you would need to specify new scope limiters in your cascading scans ScanSpec, which would mean that you need per-engagement custom cascading rules.

The idea for SelectorAttributeMappings is that you can use the same ScanAnnotationSelector template values for multiple different scanners. An example is that you might want to select based on IP, which Nmap gives in attributes.ip and sslyze in attributes.ip_address.

I see, so its aliasing that is used in combination with the inherited scope limiters. Makes sense!

or can you have an anyOf with allOf as a child?
Nope, not sure if that would be possible to express in CRD.

Good, its a weird edge case anyway and not necessary. 😁

To support more complex scenario's we could allow specifying allOf, anyOf, and noneOf in combination and evaluating them together somehow (maybe simple AND)?

~~Hmm... Shouldn't any linear AND combination of allOf, anyOf and noneOf be expressible by a simple allOf with well-chosen matches inside of it?~~ After thinking about it: Nope, it shouldn't. Many are, but not all of them.

Combining all of them via AND may actually also resolve another problem I have with your code right now, which is that specifying all three operators is valid (in the sense that the cluster will accept a YAML file with them and not throw errors), but the code snippet you posted will just silently ignore most of them in favor of the first type it matches (i.e., if an allOf operator is present, anyOf and (more crucially) noneOf are ignored). ANDing them together would provide us with an intuitive and easy to document solution to this.

EndPositive · 2021-11-15T08:43:36Z

consistency evangelist

Hahah yeah I get you. I'm down to implement it anyways, so let me know.

in the sense that the cluster will accept a YAML file with them and not throw errors

Yeah I was actually wondering how to specify mutual exclusiveness in CRD, but I don't think it's possible. I'll implement it with AND.

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

malexmave · 2021-11-15T17:10:17Z

Just talked to @J12934. He is fine with the proposed inheritance behavior. :)

One other point he raised: engagement.scope/domain as an annotation name is violating the policies of Kubernetes, which says that if you do something that looks like a domain, you must own that domain. He also raised the point that if you want scope definition to be immutable, but you are using annotations to store information about them, then these annotations by default fall under the standard annotation inheritance rules (i.e., they can be modified or deleted).

He proposes scope.cascadingscans.securecodebox.io/* as the namespace for scope annotations (this also makes clear that the scope is only defined for cascading scans, and does not enforce anything on the first scan, i.e., if you set a scope of example.com on an "nmap google.com" scan, it will not prevent the scan). This would also allow you to solve one problem with another and define special handling for these annotations if you really want to make sure that these annotations are inherited without modifications (if that is the behavior you want to go for).

EndPositive · 2021-11-16T08:58:31Z

Awesome. I'll take a look at annotation handling. Should ScanAnnotationSelector also work for annotations that are not in that namespace? If not, should we consider renaming this field?

malexmave · 2021-11-16T09:11:40Z

Hmm. An argument could be made that it should be called scopeSelector or scopeLimiter or something like that. Is there any reasonable situation in which you would want to use the ScanAnnotationSelector to select on non-scope-related annotations? I can't come up with one on short notice, but maybe you know one.

If there isn't, and if you want to force inheritance on scope-related annotations no matter if other annotations are inherited, it probably makes sense to:

Rename the selector to something with "scope" in the name
Check in the cascading hook that it only matches on annotations in the scope namespace (and throw an error and stop execution if it doesn't, because this should be considered a syntax error)
Always inherit all annotations in that namespace, no matter if other annotations are inherited
Potentially prevent new annotations from being added to that namespace in the cascading definitions (not sure about this - do you think this would be necessary / a good idea? It's the only way to enforce immutability)

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

EndPositive · 2021-11-16T10:48:32Z

@malexmave implemented your proposal. See the test scope annotations should be completely immutable, showcases behavior really well. Currently, any scope annotations added by a cascading rule's scanLabels are silently discarded. Should we throw errors for this?

Still thinking about a better name for scanAnnotationSelector.

malexmave · 2021-11-16T10:50:18Z

Currently, any scope annotations added by a cascading rule's scanLabels are silently discarded. Should we throw errors for this?

I would argue that "failing loudly" (stopping execution and printing an error message) would be the right move here, as silently discarding information will lead to impossible-to-track-down errors for users.

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

…ppings to ScopeLimiterAliases Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

EndPositive · 2021-11-17T17:46:41Z

@malexmave @J12934 ready for review

malexmave · 2021-11-18T09:14:04Z

I will review this soon. Just a quick point so that we can avoid having the headache again: Can you make sure the helm docs are up to date so that we don't have the issue again where every new pull request runs the "Update helm docs" action? 😁

The resulting changes from the current state of the PR can be seen in 0b5c09b (I pushed it to a branch in our repo to run the CI).

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Signed-off-by: Max Maass <max.maass@iteratec.com>

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Compatibility with NodeJS 14.X. Signed-off-by: Max Maass <max.maass@iteratec.com>

Signed-off-by: Max Maass <max.maass@iteratec.com>

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

malexmave

After a fruitful and long set of discussions, debugging, testing and changes, this PR is finally ready to merge 🎉. Thanks again @EndPositive for the great work, long discussions and many changes you implemented.

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>

J12934 · 2021-12-08T10:09:08Z

Updated the branch & then resolved the merge issues in the unit tests. Seems to be working now.
Will merge as soon as the other checks are passing and completed.

We should really tackle the code complexity in the cascading hook soon. The test file is already at 2800 lines of code 😬
Does anybody have a good overview over the current hooks code and ideas on how it could be refactored to be simpler?

EndPositive · 2021-12-08T10:28:10Z

@J12934, I think it would be a good idea to refactor the test file to get rid of the snapshot testing. Also, we should probably group the hook tests in the same manner as the scope limiter tests.

Regarding the complexity of the hook itself. Maybe that should be combined with #789..

Jop Zitman added 2 commits November 10, 2021 15:30

Add reverse-matches

14ec455

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Merge remote-tracking branch 'upstream/main' into reverse-matches

bae68d1

Signed-off-by: Jop Zitman <jop.zitman@secura.com> # Conflicts: # hooks/cascading-scans/hook/hook.test.js

malexmave added cascading-scans CRD Improvements or additions to CRDs enhancement New feature or request labels Nov 12, 2021

Jop Zitman added 3 commits November 15, 2021 12:16

AND together anyOf, allOf, and noneOf

cf49cb3

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Implement subdomainOf operator

86899c9

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Implement "global" validation/error handling

f207d10

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Jop Zitman added 2 commits November 16, 2021 11:40

Select only annotations with domain scope.cascading.securecodebox.io/

2aef704

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Enforce immutability of scope.cascading.securecodebox.io/ annotations

9c5f93c

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Jop Zitman added 4 commits November 16, 2021 12:09

Loudly fail if new scope annotations are defined in a cascading rule

da3eeae

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Rename ScanAnnotationSelector to ScopeLimiter and SelectorAttributeMa…

869f4d2 < 8000 /code>
…ppings to ScopeLimiterAliases Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Add scopeLimiterAliases to scanner values.yaml

2ff80be

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Remove unused import

8751a3d

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

EndPositive changed the title ~~Introduce reverse matches on cascading rules to enforce scope~~ Introduce scope limiter on cascading rules to enforce scope Nov 17, 2021

EndPositive mentioned this pull request Nov 17, 2021

Scope limiter documentation secureCodeBox/documentation#155

Merged

Jop Zitman added 2 commits November 17, 2021 18:44

Make scope limiter actually run in cluster

013c2e2

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

JSON stringify logs

59d9a28

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

EndPositive marked this pull request as ready for review November 17, 2021 17:46

Jop Zitman and others added 13 commits November 29, 2021 14:31

Add more Contains tests

b56f3ff

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Add In tests

c90379d

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Add trailing delimiter to internal split

ff081c0

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Rename keyinobjectlist -> pickValues and list -> asList

e4f0324

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Add failing test case for trailing comma

2500c0c

Signed-off-by: Max Maass <max.maass@iteratec.com>

Add more tests for trailing comma in list

edf7161

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Rename asList to getValues

b2285c2

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

Use replace with RegEx instead of replaceAll

051b3ae

Compatibility with NodeJS 14.X. Signed-off-by: Max Maass <max.maass@iteratec.com>

Support comma with trailing space for split

d415288

Signed-off-by: Max Maass <max.maass@iteratec.com>

Rename getValues to asList

2a3733d

Signed-off-by: Max Maass <max.maass@iteratec.com>

Rename pickValues to getValues

5c5dc6f

Signed-off-by: Max Maass <max.maass@iteratec.com>

Add more test cases to increase coverage

31110aa

Signed-off-by: Max Maass <max.maass@iteratec.com>

Make Values field required

e1e797e

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

malexmave previously approved these changes Nov 30, 2021

View reviewed changes

Test lists in alias

af68075

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

EndPositive dismissed malexmave’s stale review via af68075 December 1, 2021 09:20

Merge branch 'main' into reverse-matches

42c3d20

Signed-off-by: Jop Zitman <jop.zitman@secura.com>

malexmave previously approved these changes Dec 1, 2021

View reviewed changes

malexmave mentioned this pull request Dec 1, 2021

🚧 [Consistency] hostname vs. host vs. name vs. location in finding format #853

Closed

Merge branch 'main' into reverse-matches

27d6e60

Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>

J12934 dismissed malexmave’s stale review via 27d6e60 December 7, 2021 11:49

J12934 added 3 commits December 7, 2021 12:54

Merge branch 'main' into reverse-matches

cad9840

Update use of getCascadingScans function

4e906b3

Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>

Update snapshots

2dabb0f

Signed-off-by: Jannik Hollenbach <jannik.hollenbach@iteratec.com>

J12934 approved these changes Dec 8, 2021

View reviewed changes

J12934 merged commit 23f35e0 into secureCodeBox:main Dec 8, 2021

EndPositive deleted the reverse-matches branch December 8, 2021 10:28

rfelber mentioned this pull request Feb 22, 2022

Check API Reference Docs for Correctness: CascadingRule #1918

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce scope limiter on cascading rules to enforce scope #805

Introduce scope limiter on cascading rules to enforce scope #805

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Introduce scope limiter on cascading rules to enforce scope #805

Introduce scope limiter on cascading rules to enforce scope #805

Uh oh!

Conversation

Uh oh!

Description

Todo

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants