feat: add vulnerability report verifier #1173

akashsinghal · 2023-11-15T01:14:26Z

Description

What this PR does / why we need it:

Adds a new vulnerability verifier for Sarif reports generated by Trivy + Grype, that can:
- enforce report content matches a schema (Sarif is default supported schema for now but it is extensible)
- enforce a MaximumAge duration (ex: '24h')
- enforce against existence of disallowedSeverity levels (ex: 'critical')
- enforce against existence of denylistCVEs (ex: CVE-2021-44228 log4shell)
- introduce a passthrough flag which will bypass all checks and append sarif content in verifier report
Adds new e2e test for verifier
Adds unit tests
Adds new notation-nested-validation Constraint + Constraint Template
- requires every artifact in the tree to be signed by notation
Adds new vulnerability-report-validation Constraint + Constraint Template
- at least one vulnerability report must be valid and signed by notation

Documentation PR here: ratify-project/ratify-web#35

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

Fixes #1096

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Helm Chart Change (any edit/addition/update that is necessary for changes merged to the main branch)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

Test A
Test B

Checklist:

Does the affected code have corresponding tests?
Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
Does this introduce breaking changes that would require an announcement or bumping the major version?
Do all new files have appropriate license header?

Post Merge Requirements

MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

codecov · 2023-11-15T01:42:39Z

Codecov Report

Attention: 37 lines in your changes are missing coverage. Please review.

Comparison is base (00f16de) 52.55% compared to head (3f22d04) 54.46%.

Files	Patch %	Lines
...rifier/vulnerabilityreport/vulnerability_report.go	88.68%	32 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1173      +/-   ##
==========================================
+ Coverage   52.55%   54.46%   +1.90%     
==========================================
  Files         101      103       +2     
  Lines        6349     6696     +347     
==========================================
+ Hits         3337     3647     +310     
- Misses       2690     2722      +32     
- Partials      322      327       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

susanshi

PR looks great overall! Left some clarification questions

CONTRIBUTING.md

Makefile

plugins/verifier/vulnerabilityreport/schemavalidation/schemavalidator.go

susanshi · 2023-11-23T05:51:51Z

plugins/verifier/vulnerabilityreport/vulnerability_report.go

+				Name:      input.Name,
+				IsSuccess: false,
+				Message:   fmt.Sprintf("vulnerability report validation failed: error validating maximum age:[%v]", err.Error()),
+			}, nil


Just to note VerifyReference can also return an error type, which might translate into a Plugin Error code. Tho i think it be cleaner return the validationError in the report, but we currently don't have a ERROR code that maps to TSG ( i will do the same in the SBOM verifier)

Do you think external plugins should manage and document their own Error Codes?

But we have no control on the 3rd party's plugins and cannot enforce the error code. Or maybe we can provide some common error codes to plugin developers?

plugins/verifier/vulnerabilityreport/vulnerability_report.go

test/bats/tests/config/vulnerabilityreport_config.json

plugins/verifier/vulnerabilityreport/vulnerability_report.go

yizha1 · 2023-11-23T13:38:17Z

Thanks @akashsinghal the video does help to understand the experience.

The workflow for users to enable vuln policy is

install Gatekeeper and Ratify
apply constraint
apply template
apply verifier

One question: If users want to enable for example signature validation or another policy like SBOM, will they apply a different constraint and template?

Will this PR include all the rules below?

Image should have vulnerability report
Evaluate the latest report, and the latest report should be less than maximumAge (Did you attach annotations so that you can check the time?)
Validate the signature of vulnerability report if signature validation is enabled ( It seems users need to enable it in the template)
No high and critical vuln --> the video demoed this one.
No CVE in denylist

plugins/verifier/vulnerabilityreport/vulnerability_report.go

akashsinghal · 2023-11-27T19:46:20Z

Thanks @akashsinghal the video does help to understand the experience.

The workflow for users to enable vuln policy is

install Gatekeeper and Ratify

apply constraint

apply template

apply verifier

One question: If users want to enable for example signature validation or another policy like SBOM, will they apply a different constraint and template?

Will this PR include all the rules below?

Image should have vulnerability report

Evaluate the latest report, and the latest report should be less than maximumAge (Did you attach annotations so that you can check the time?)

Validate the signature of vulnerability report if signature validation is enabled ( It seems users need to enable it in the template)

No high and critical vuln --> the video demoed this one.

No CVE in denylist

Thanks for taking a look @yizha1

One question: If users want to enable for example signature validation or another policy like SBOM, will they apply a different constraint and template?

If they want to enable signature validation at the image level then yes they will need to apply a separate policy. Same goes with SBOM.

Will this PR include all the rules below?

If the user wants to check ONLY for existence of report, they can apply Template + constraint and apply a vuln report verifier that does not specify any of the fields. This will essentially just do a schema check to make sure the report exists with a valid SARIF format.

Currently, the sample template at library/vulnerability-report-validation/template.yaml does not filter based on latest report. I can look into adding that. The "latest" report filtering can only be considered at rego evaluation since it's an aggregate report filter and NOT per report.

library/notation-nested-validation/template.yaml

library/vulnerability-report-validation/template.yaml

plugins/verifier/vulnerabilityreport/schemavalidation/schemavalidator.go

binbin-li · 2023-11-28T08:12:46Z

plugins/verifier/vulnerabilityreport/vulnerability_report.go

+				Name:      input.Name,
+				IsSuccess: false,
+				Message:   fmt.Sprintf("vulnerability report validation failed: error validating maximum age:[%v]", err.Error()),
+			}, nil


But we have no control on the 3rd party's plugins and cannot enforce the error code. Or maybe we can provide some common error codes to plugin developers?

plugins/verifier/vulnerabilityreport/vulnerability_report.go

susanshi

Looks good to me, thanks!

binbin-li

lgtm once the ci passes

akashsinghal self-assigned this Nov 15, 2023

akashsinghal mentioned this pull request Nov 15, 2023

feat: add vulnerability scanner [PROTOTYPE] #1123

Closed

12 tasks

akashsinghal force-pushed the akashsinghal/vulnReportVerifier branch from 1630608 to 80de33a Compare November 15, 2023 01:38

feat: add vulnerability report verifier

9275cb5

akashsinghal force-pushed the akashsinghal/vulnReportVerifier branch from 17194ef to 9275cb5 Compare November 20, 2023 19:03

akashsinghal marked this pull request as ready for review November 20, 2023 23:07

akashsinghal requested review from binbin-li, jimmyraywv, luisdlp, susanshi and toddysm as code owners November 20, 2023 23:07

susanshi reviewed Nov 23, 2023

View reviewed changes

binbin-li reviewed Nov 27, 2023

View reviewed changes

merge main

8eb1acd

akashsinghal added 2 commits November 27, 2023 22:42

address comments; add timestamp to all reports

748e7b4

address more comments

5b807ed

binbin-li reviewed Nov 28, 2023

View reviewed changes

akashsinghal added 8 commits November 28, 2023 18:33

update schema validator

90e2218

switch to artifact type

665c853

add support for latest report filtering

dfebb1e

small indent

8b521b8

fix test

10b6a48

rename notary project sig

c55d840

add comments to vuln report template

8168fb5

add comments to nested notation template

d219788

susanshi previously approved these changes Nov 30, 2023

View reviewed changes

binbin-li previously approved these changes Nov 30, 2023

View reviewed changes

override new go package license

c1b3f4e

akashsinghal dismissed stale reviews from binbin-li and susanshi via c1b3f4e November 30, 2023 17:47

akashsinghal added 3 commits November 30, 2023 17:48

Merge branch 'main' into akashsinghal/vulnReportVerifier

9b70878

update to make create time stamp configurable

d88e13e

rename disallowedSeverity to disallowedSeverities

497c82e

akashsinghal force-pushed the akashsinghal/vulnReportVerifier branch from 7242c1c to 497c82e Compare November 30, 2023 22:51

akashsinghal added 3 commits November 30, 2023 23:51

add rego policy library for rego policy provider

c44187a

small typo

8966de2

add license header

3f22d04

susanshi approved these changes Dec 1, 2023

View reviewed changes

binbin-li approved these changes Dec 1, 2023

View reviewed changes

akashsinghal merged commit 60bc25f into ratify-project:main Dec 1, 2023
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add vulnerability report verifier #1173

feat: add vulnerability report verifier #1173

akashsinghal commented Nov 15, 2023 •

edited

Loading

codecov bot commented Nov 15, 2023 •

edited

Loading

susanshi left a comment

susanshi Nov 23, 2023

akashsinghal Nov 28, 2023

binbin-li Nov 28, 2023

yizha1 commented Nov 23, 2023 •

edited by akashsinghal

Loading

akashsinghal commented Nov 27, 2023 •

edited

Loading

binbin-li Nov 28, 2023

susanshi left a comment

binbin-li left a comment

feat: add vulnerability report verifier #1173

feat: add vulnerability report verifier #1173

Conversation

akashsinghal commented Nov 15, 2023 • edited Loading

Description

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Type of change

How Has This Been Tested?

Checklist:

Post Merge Requirements

codecov bot commented Nov 15, 2023 • edited Loading

Codecov Report

susanshi left a comment

Choose a reason for hiding this comment

susanshi Nov 23, 2023

Choose a reason for hiding this comment

akashsinghal Nov 28, 2023

Choose a reason for hiding this comment

binbin-li Nov 28, 2023

Choose a reason for hiding this comment

yizha1 commented Nov 23, 2023 • edited by akashsinghal Loading

akashsinghal commented Nov 27, 2023 • edited Loading

binbin-li Nov 28, 2023

Choose a reason for hiding this comment

susanshi left a comment

Choose a reason for hiding this comment

binbin-li left a comment

Choose a reason for hiding this comment

akashsinghal commented Nov 15, 2023 •

edited

Loading

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

codecov bot commented Nov 15, 2023 •

edited

Loading

yizha1 commented Nov 23, 2023 •

edited by akashsinghal

Loading

akashsinghal commented Nov 27, 2023 •

edited

Loading