feat: optional "exact" ignore behaviour #58

craigfurman · 2021-08-03T10:23:28Z

Ready for review
Follows CONTRIBUTING rules
Reviewed by Snyk internal team

What does this PR do?

Add a flag to the ignore method signature to opt-in to "simple"
path-matching behaviour.

The following vulnerability path: ['foo', 'bar', 'baz'] will be
matched by the following ignore rules under this behaviour:

'foo > bar > baz'
'foo > *'
'*'

Under the default matching behaviour, there are some quirks that appear
specific to package manager use cases:

The first element of vulnerabilities paths is sliced off
If the path (minus the first element) "contains" the rule as a
substring, it will match. For example: the rule file.json will match
the path ["dir/file.json"].
Attempts to parse paths as packages, URLs, or git objects.

The Snyk infrastructure-as-code use-case breaks under these default
behaviours, but we still want to use the policy library in order to not
reimplement loading and parsing of the policy file.

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

Snyk IaC are adding support for .snyk ignore policies, but in order to adapt this feature to our domain we are proposing to use different semantics for the ignore rules. This doc contains the detail, but here is a summary:

Users can ignore all instances of an IaC issue:

version: v1.19.0
ignore:
  SNYK-CC-K8S-1:
    - '*':
        reason: None Given
        expires: 2021-08-26T08:40:35.249Z
        created: 2021-07-27T08:40:35.251Z

Ignores can be scoped to individual files:

version: v1.19.0
ignore:
  SNYK-CC-K8S-1:
    - 'staging/deployment.yaml > *':
        reason: None Given
        expires: 2021-08-26T08:40:35.249Z
        created: 2021-07-27T08:40:35.251Z

And finally, ignores can be scoped to individual instances in the IaC file's "resource path":

version: v1.19.0
ignore:
  SNYK-CC-K8S-1:
    - staging/deployment.yaml > [DocId:1] > spec > template > spec > containers[web] > securityContext > privileged':
        reason: None Given
        expires: 2021-08-26T08:40:35.249Z
        created: 2021-07-27T08:40:35.251Z

When integrating this policy library with the IaC flows in the Snyk CLI, I noticed some edge cases emerge. Most of them can be summarised as "inexact matching" occurring. For example, the path rule deployment.yaml would match any vulnerability in a file named deployment.yaml, even if it was in a subdirectory.

snyk/cli#2134 demonstrates our almost-complete CLI ignores feature. The last commit on that PR, snyk/cli@e851f9a, adds a failing test demonstrating the sort of edge-case I'm talking about, which is fixed when I npm-link in this PR branch.

What are the relevant tickets?

https://snyksec.atlassian.net/browse/CC-990

Screenshots

Additional questions

CLAassistant · 2021-08-03T10:23:35Z

All committers have signed the CLA.

craigfurman · 2021-08-03T10:26:51Z

lib/index.js

@@ -29,8 +29,13 @@ function defaultFilename() {
 }

 function attachMethods(policy) {
-  policy.filter = function (vulns, root) {
-    return filter(vulns, policy, root || path.dirname(policy.__filename));
+  policy.filter = function (vulns, root, simple = false) {


I'm not necessarily convinced that this interface is a good idea. A flag named after a behaviour that triggers a different code path is usually a bad smell. I'm not previously familiar with this part of our codebases, and as such I can't think of many other ideas at the moment.

One possible alternative would be to allow a function to be injected instead of a flag. Callers could then customise the actual filtering/matching behaviour using this function. One downside to that alternative is that in practice, that function would likely have to be duplicated in the CLI and registry code bases to support ignores in both of those contexts.

if we can't think of anything "better" than behaviour-influencing parameters, and we don't want to inject the whole match function for the reason in my comment above, maybe we should change this to an enum-string, e.g. matchStrategy = 'simple' | 'package-manager', with package-manager being the default. That way, future use-cases can be added without breaking this interface.

i'll b hone i love the strategy idea more since while reading this code... simple has no meaning... it's just a word right? but if we have it as a value for something that's called strategy and we pass matchStrategy around or whatever u call it that would make more sense right?

what do u mean duplicate the function in CLI and registry? why would we want to do it? are we duplicating the simple logic u introduced here?

@odinn1984 I totally agree, that variable name is not great. The concept I'm trying to express with "simple" is "exact matching of the path arrays, but also supporting *s". For example:

foo > bar matches ['foo', 'bar'].
foo > * matches ['foo', 'bar', 'baz']

This "relatively simple" behaviour is contrasted with the current default, which has a couple of package-manager-specific attributes:

Slicing the first element off the path before comparison

"fuzzy matching": If the path (minus the first element) "contains" the rule as a
substring, it will match. For example: the rule file.json will match
the path ["dir/file.json"].

parsing path elements as URLs or git objects and matching based on "normalised" forms of these

Perhaps the new, opt-in "simple" path can be named "exact", i.e. matchStrategy = 'exact', vs the default of matchStrategy = 'packageManager'. "exact" isn't strictly correct because of the *s, but perhaps it's still better than "simple". suggestions welcome!

re: duplicate the function, that refers to my other possible idea, which is to optionally inject a match function instead of a strategy enum. The upside to that is it solves our naming problem somewhat, by inverting the behaviour into the calling code instead of here, but the downside is that if we want to call this policy library both in the CLI (for local-exec scans, as IaC supports) and also in registry to support server-side ignores, we'd need to duplicate that matching function.

Does this make sense?

I've just pushed a commit to change the flag to a string, but I think the overall discussion about what interface to present is still open.

craigfurman · 2021-08-03T10:32:36Z

Oops did I just override a random assignment bot @joeholdcroft? Please unassign governance if so, sorry.

Add a flag to the `ignore` method signature to opt-in to "exact" path-matching behaviour. The following vulnerability path: `['foo', 'bar', 'baz']` will be matched by the following ignore rules under this behaviour: - 'foo > bar > baz' - 'foo > *' - '*' Under the default matching behaviour, there are some quirks that appear specific to package manager use cases: - The first element of vulnerabilities paths is sliced off - If the path (minus the first element) "contains" the rule as a substring, it will match. For example: the rule `file.json` will match the path `["dir/file.json"]`. The Snyk infrastructure-as-code use-case breaks under these default behaviours, but we still want to use the policy library in order to not reimplement loading and parsing of the policy file.

darscan · 2021-08-04T14:24:50Z

lib/filter/ignore.js

+  ignore,
+  vuln,
+  filtered,
+  matchStrategy = 'packageManager'


"packageManager" as a matchStrategy sounds weird no?

@darscan I can see that, yeah. But I'm not sure how else to name the set of characteristics that I've enumerated in #58 (comment) and the PR description. The new IaC code path could be paraphrased "match this like a file-qualified jsonpath", and the existing way could be paraphrased like "match this like a node in a package manager's graph".

suggestions welcome!

snyksec · 2021-08-05T15:31:28Z

🎉 This PR is included in version 1.22.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

This requires bumping the policy library to incorporate snyk/policy#58. This fixes some IaC ignores edge-cases, as expressed in a new test.

chore: run prettier on a test file

233321d

craigfurman commented Aug 3, 2021

View reviewed changes

craigfurman mentioned this pull request Aug 3, 2021

feat: IaC ignore support snyk/cli#2134

Merged

1 task

craigfurman marked this pull request as ready for review August 3, 2021 10:29

craigfurman requested review from a team and joeholdcroft and removed request for a team August 3, 2021 10:29

craigfurman requested a review from vasser August 3, 2021 10:34

craigfurman changed the title ~~feat: optinal "simple" ignore behaviour~~ feat: optional "simple" ignore behaviour Aug 3, 2021

craigfurman changed the title ~~feat: optional "simple" ignore behaviour~~ feat: optional "exact" ignore behaviour Aug 4, 2021

vasser approved these changes Aug 4, 2021

View reviewed changes

craigfurman force-pushed the feat/simple-match branch from 0489846 to cffc7a1 Compare August 4, 2021 13:24

darscan reviewed Aug 4, 2021

View reviewed changes

craigfurman merged commit 8c6f341 into master Aug 5, 2021

craigfurman deleted the feat/simple-match branch August 5, 2021 15:30

snyksec added the released label Aug 5, 2021

craigfurman pushed a commit to snyk/cli that referenced this pull request Aug 6, 2021

fix: match IaC ignore paths exactly

7dc4f7c

This requires bumping the policy library to incorporate snyk/policy#58. This fixes some IaC ignores edge-cases, as expressed in a new test.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: optional "exact" ignore behaviour #58

feat: optional "exact" ignore behaviour #58

craigfurman commented Aug 3, 2021 •

edited

CLAassistant commented Aug 3, 2021 •

edited

craigfurman Aug 3, 2021

craigfurman Aug 4, 2021 •

edited

odinn1984 Aug 4, 2021

craigfurman Aug 4, 2021 •

edited

craigfurman Aug 4, 2021

craigfurman commented Aug 3, 2021 •

edited

darscan Aug 4, 2021

craigfurman Aug 4, 2021 •

edited

snyksec commented Aug 5, 2021

feat: optional "exact" ignore behaviour #58

feat: optional "exact" ignore behaviour #58

Conversation

craigfurman commented Aug 3, 2021 • edited

What does this PR do?

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

What are the relevant tickets?

Screenshots

Additional questions

CLAassistant commented Aug 3, 2021 • edited

craigfurman Aug 3, 2021

Choose a reason for hiding this comment

craigfurman Aug 4, 2021 • edited

Choose a reason for hiding this comment

odinn1984 Aug 4, 2021

Choose a reason for hiding this comment

craigfurman Aug 4, 2021 • edited

Choose a reason for hiding this comment

craigfurman Aug 4, 2021

Choose a reason for hiding this comment

craigfurman commented Aug 3, 2021 • edited

darscan Aug 4, 2021

Choose a reason for hiding this comment

craigfurman Aug 4, 2021 • edited

Choose a reason for hiding this comment

snyksec commented Aug 5, 2021

craigfurman commented Aug 3, 2021 •

edited

CLAassistant commented Aug 3, 2021 •

edited

craigfurman Aug 4, 2021 •

edited

craigfurman Aug 4, 2021 •

edited

craigfurman commented Aug 3, 2021 •

edited

craigfurman Aug 4, 2021 •

edited