License checker feature #501

josieang · 2023-08-21T05:32:17Z

TODO:

all the tests
license allowlist behaviour as detailed in the thread License checker feature #501 (comment)
license table view

josieang · 2023-08-21T05:35:10Z

cmd/osv-scanner/main.go

+				Name:  "experimental-all-packages",
+				Usage: "when json output is selected, prints all packages",
+			},
+			&cli.BoolFlag{


I think in the json output when the licenses is specified without an allowlist, we want it to print all packages (and their licenses) by default. If that's the case, then I'm not sure how the all-packages and licenses flags should interact.

Now that we do have a all-packages flag, maybe licenses (without allowlist) shouldn't print all packages by default, and just fill in the field for packages that already exists? If someone wants full license info for everything, pass the all-packages flag.

@oliverchang thoughts?

Just to be sure I understand the alternatives here, is it:

--licenses (without --all-packages) will just ignore all Unknown licenses?

--licenses implies --all-packages ?

This is complicated by the behaviour for when an allow-list is passed, because to be consistent with the vulnerability scanning behaviour, it makes more sense for --licenses <ALLOWLIST> to not imply --all-packages.

Thinking about this a bit more, all packages should in fact have a license right? (as opposed to vulnerabilities). We just consider "Unknown" to be a license that's worthwhile reporting too, because from a license compliance perspective it's just as important.

From that perspective, 2. above seems to make more sense to me. Not necessarily because it implies --all-packages for the sake of it, but rather that we do have something worthwhile to report for all packages when it comes to licenses.

okay i see.

This is complicated by the behaviour for when an allow-list is passed, because to be consistent with the vulnerability scanning behaviour it makes more sense for --licenses to not imply --all-packages.

Thinking about this, I think rather than thinking that --licenses implies --all-packages, we can think of how --licenses with an empty allowlist means every license a violation, which means all packages should be printed in the json output.

This is the solution I'm tending towards:

--all-packages has no effect on the table output. It will always be only the vulnerabilities in the vulns table, and if licenses is specified (regardless of allowlist), all the licenses and their counts in addition to the vulns table.

in json output --licenses=<allowlist> prints only packages with license violations or packages with vulns. If allowlist is empty then all packages are violating licenses (the licenses field and license_violations field will be populated for every package)

in json output --licenses=<allowlist> with --all-packages prints all packages whether they have a vuln or license violation or not. The license_violations field is populated for every package that has a violation, license field populated for every package.

thoughts?

The JSON output makes sense to me.

Can you clarify what happens in the table output when --licenses is passed in with an allowlist?

Would the packages that are violating the allowlist be shown individually? or only a list of violating licenses in a summary format?

I was imagining that it would be the same as if there was no allowlist specified, it would have a a row per license and a column with count. If we did want to do something, we could add a column in the table when an allowlist is specified like 'violation' column with value either true or false.

and to clarify, all licenses, including the UNKNOWN license would be in that table whether allowlist is specified or not.

How would a user know what package violated the allowlist without having to parse the JSON?

they wouldn't.

my thinking was listing all individual licenses, or all license violations here may clutter the view. But you do make a good point as the vulns table shows the individual vulns, but this table would only shows a summary. it doesn't mirror the behaviour of the vulns perfectly.

we did discuss in the meeting adding a summary/all-packages flag to toggle this, but iirc we concluded that we will wait for feedback first and add flags or change the table format in response to user feedback.

did you have a solution in mind?

sorry! my bad my memory is wrong. For the table view we agreed to list a summary when allowlist is empty, and violations when allowlist isn't empty.

pkg/osvscanner/osvscanner.go

pkg/osvscanner/vulnerability_result.go

pkg/models/licenses.go

pkg/osvscanner/osvscanner.go

another-rex

Added some more comments!

internal/output/markdowntable.go

internal/output/table.go

pkg/depsdev/license.go

pkg/osvscanner/osvscanner.go

pkg/osvscanner/vulnerability_result.go

josieang · 2023-08-31T09:03:05Z

I'm getting this error in my tests, the difference seems to be the CVSS severity value of the first vuln. Looking at https://api.osv.dev/v1/vulns/CVE-2022-37434 it looks like the CVSS is reported by the API.

    --- FAIL: TestRun/#03 (2.55s)
        main_test.go:443: stdout
             got:
            Scanning dir ./fixtures/sbom-insecure/
            Scanned /Users/josieang/fork/osv-scanner/cmd/osv-scanner/fixtures/sbom-insecure/alpine.cdx.xml as CycloneDX SBOM and found 15 packages
            Scanned /Users/josieang/fork/osv-scanner/cmd/osv-scanner/fixtures/sbom-insecure/postgres-stretch.cdx.xml as CycloneDX SBOM and found 136 packages
            +-------------------------------------+------+-----------+--------------------------------+------------------------------------+-------------------------------------------------+
            | OSV URL                             | CVSS | ECOSYSTEM | PACKAGE                        | VERSION                            | SOURCE                                          |
            +-------------------------------------+------+-----------+--------------------------------+------------------------------------+-------------------------------------------------+
            | https://osv.dev/CVE-2022-37434      | 9.8  | Alpine    | zlib                           | 1.2.10-r2                          | fixtures/sbom-insecure/alpine.cdx.xml           |
            | https://osv.dev/DLA-3022-1          |      | Debian    | dpkg                           | 1.18.25                            | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GHSA-v95c-p5hm-xq8f | 6    | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GO-2022-0274        |      |           |                                |                                    |                                                 |
            | https://osv.dev/GHSA-f3fp-gc8g-vw66 | 5.9  | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GHSA-g2j6-57v7-gm8c | 6.1  | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GHSA-m8cg-xc2p-r3fc | 2.5  | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GHSA-vpvm-3wq2-2wvm | 7    | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GHSA-p782-xgp4-8hr8 | 5.3  | Go        | golang.org/x/sys               | v0.0.0-20210817142637-7d9622a276b7 | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GO-2022-0493        |      |           |                                |                                    |                                                 |
            | https://osv.dev/DLA-3012-1          |      | Debian    | libxml2                        | 2.9.4+dfsg1-2.2+deb9u6             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/DLA-3008-1          |      | Debian    | openssl                        | 1.1.0l-1~deb9u5                    | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/DLA-3051-1          |      | Debian    | tzdata                         | 2021a-0+deb9u3                     | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            +-------------------------------------+------+-----------+--------------------------------+------------------------------------+-------------------------------------------------+

             want:
            Scanning dir ./fixtures/sbom-insecure/
            Scanned %%/fixtures/sbom-insecure/alpine.cdx.xml as CycloneDX SBOM and found 15 packages
            Scanned %%/fixtures/sbom-insecure/postgres-stretch.cdx.xml as CycloneDX SBOM and found 136 packages
            +-------------------------------------+------+-----------+--------------------------------+------------------------------------+-------------------------------------------------+
            | OSV URL                             | CVSS | ECOSYSTEM | PACKAGE                        | VERSION                            | SOURCE                                          |
            +-------------------------------------+------+-----------+--------------------------------+------------------------------------+-------------------------------------------------+
            | https://osv.dev/CVE-2022-37434      |      | Alpine    | zlib                           | 1.2.10-r2                          | fixtures/sbom-insecure/alpine.cdx.xml           |
            | https://osv.dev/DLA-3022-1          |      | Debian    | dpkg                           | 1.18.25                            | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GHSA-v95c-p5hm-xq8f | 6    | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GO-2022-0274        |      |           |                                |                                    |                                                 |
            | https://osv.dev/GHSA-f3fp-gc8g-vw66 | 5.9  | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GHSA-g2j6-57v7-gm8c | 6.1  | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GHSA-m8cg-xc2p-r3fc | 2.5  | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GHSA-vpvm-3wq2-2wvm | 7    | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GHSA-p782-xgp4-8hr8 | 5.3  | Go        | golang.org/x/sys               | v0.0.0-20210817142637-7d9622a276b7 | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/GO-2022-0493        |      |           |                                |                                    |                                                 |
            | https://osv.dev/DLA-3012-1          |      | Debian    | libxml2                        | 2.9.4+dfsg1-2.2+deb9u6             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/DLA-3008-1          |      | Debian    | openssl                        | 1.1.0l-1~deb9u5                    | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            | https://osv.dev/DLA-3051-1          |      | Debian    | tzdata                         | 2021a-0+deb9u3                     | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
            +-------------------------------------+------+-----------+--------------------------------+------------------------------------+-------------------------------------------------+
            ```

osv.BatchedQuery We are planning to create deps.dev queries from these packages. Rather than use the osv.BatchedQuery type to construct deps.dev queries from, create a new type Package that includes package information and source information.

This makes the licenses available in the json output only. Making it available in table form and filtering by an allowlist is left as a TODO.

"no vulnerabilities found" to "no issues found"

between summary vs violations table

I don't know a lot about SARIF so maybe these are required but that would be surprising to me whereas trimming these off make it a bit consistent for editors and such. Obviously if these are required then lets close this PR

this is in preparation for the license scanning feature. the queries are structured around making requests to the osv API, we also will want to make requests to the deps.dev api. #501

maybe this will help with the failing test

codecov-commenter · 2023-11-01T22:48:22Z

Codecov Report

Merging #501 (288bef9) into main (4aed961) will decrease coverage by 0.04%.
The diff coverage is 78.89%.

@@            Coverage Diff             @@
##             main     #501      +/-   ##
==========================================
- Coverage   78.82%   78.78%   -0.04%     
==========================================
  Files          78       79       +1     
  Lines        5511     5742     +231     
==========================================
+ Hits         4344     4524     +180     
- Misses        990     1031      +41     
- Partials      177      187      +10

Files	Coverage Δ
internal/output/markdowntable.go	`100.00% <100.00%> (+27.27%)`	⬆️
pkg/reporter/table_reporter.go	`100.00% <100.00%> (ø)`
pkg/osvscanner/vulnerability_result.go	`81.33% <92.50%> (+2.90%)`	⬆️
pkg/models/results.go	`77.58% <61.53%> (-4.64%)`	⬇️
cmd/osv-scanner/main.go	`82.35% <79.31%> (+0.09%)`	⬆️
internal/output/table.go	`81.39% <90.00%> (+5.63%)`	⬆️
pkg/osvscanner/osvscanner.go	`54.34% <74.57%> (+1.21%)`	⬆️
pkg/depsdev/license.go	`57.89% <57.89%> (ø)`

... and 1 file with indirect coverage changes

another-rex

Looks pretty good! Added some comments.

cmd/osv-scanner/json

cmd/osv-scanner/main.go

cmd/osv-scanner/osv-scanner

cmd/osv-scanner/main.go

cmd/osv-scanner/main_test.go

internal/output/table.go

pkg/osvscanner/osvscanner.go

pkg/osvscanner/vulnerability_result_internal_test.go

josieang · 2023-11-03T02:43:33Z

Ah I see, looks like we might need to update the linter and the go version in the checks. errors.Join was added in go 1.20.

I want to use errors.Join in the following PR: - License checker feature google#501 It is a method added in go 1.20.

I want to use errors.Join in the following PR: - License checker feature #501 It is a method added in go 1.20.

cmd/osv-scanner/main.go

another-rex

LGTM!

oliverchang · 2023-11-22T08:03:19Z

cmd/osv-scanner/main.go

+		case errors.Is(err, osvscanner.OnlyUncalledVulnerabilitiesFoundErr):
+			// TODO: Discuss whether to have a different exit code
+			// now that running call analysis is not default.
+			return 0b0010 // 2


I wonder if we're complicating the return codes here by having so many cases and different error types. If users do need to distinguish these, this should be done through another mechanism (e.g. the JSON output or models.VulnerabilityResults output).

I suggest we just return 1 if there is any error at all.

@G-Rath @another-rex

yyeaahhh I'd say for now best to keep it simple and find out if people have a strong need for them - as you've eluded to @oliverchang, I think as well the scanner will grow to a point that fine-grain errors should be handled via e.g. JSON just because of all the different facets

I'm happy to return 0 or 1 here and sync with @another-rex so that these changes can be done alongside his changes to make call analysis return 0 or 1 as well.

I may not get to implementing error details in the models.VulnerabilityResults output soon but I'll file an issue for this.

Thanks! Note that I don't think #675 is necessary -- all the data is there in models.VulnerabilityResults already.

oliverchang · 2023-11-22T08:06:55Z

pkg/osvscanner/osvscanner.go


-	return vulnerabilityResults, nil
+		switch {
+		case !vuln && !onlyUncalledVuln && !licenseViolation:


Returning to this :) Sorry I didn't get a chance to review this earlier.

Similar to my other comment, this is getting very complicated. I suggest we just fold this into a single VulnerabilitiesFoundErr if there are any errors (including license errors).

If a user wants to distinguish between these errors more, they can just look at the models.VulnerabilityResults result.

@another-rex

josieang commented Aug 21, 2023

View reviewed changes

G-Rath reviewed Aug 21, 2023

View reviewed changes

pkg/osvscanner/osvscanner.go Outdated Show resolved Hide resolved

G-Rath reviewed Aug 21, 2023

View reviewed changes

pkg/osvscanner/vulnerability_result.go Outdated Show resolved Hide resolved

josieang commented Aug 21, 2023

View reviewed changes

pkg/models/licenses.go Outdated Show resolved Hide resolved

josieang commented Aug 21, 2023

View reviewed changes

pkg/osvscanner/osvscanner.go Outdated Show resolved Hide resolved

josieang commented Aug 21, 2023

View reviewed changes

pkg/osvscanner/osvscanner.go Outdated Show resolved Hide resolved

another-rex reviewed Aug 24, 2023

View reviewed changes

Spiritaine approved these changes Aug 26, 2023

View reviewed changes

josieang force-pushed the licenses branch 2 times, most recently from 6b582a8 to e741c98 Compare October 26, 2023 20:58

josieang mentioned this pull request Oct 27, 2023

Refactor package scanning to produce packages instead of queries #614

Merged

josieang and others added 15 commits October 26, 2023 23:27

Allow all packages to be printed when using json format

0f16554

Implement licenses deps.dev API call

80cb689

This makes the licenses available in the json output only. Making it available in table form and filtering by an allowlist is left as a TODO.

Address intial review comments

ba44d84

Implement table view

be89683

Implement allowlisting licenses in json view

1d47444

Address another-rex's comments

eeabba5

Address comment from another-rex that I missed

6b1e0e4

add tests for buildVulnerabilityResults

067e84d

fix some bugs, add a LicenseViolationErr variable, change wording from

6b1ba5f

"no vulnerabilities found" to "no issues found"

Return license violation errors

df4936c

Add simple summary license table tests

1648605

Add tests for flattening licenses

4a9a5bb

add config field to the results to help the table reporters to toggle

647bc42

between summary vs violations table

add test for licenses and all-packages flags with json output

9b9705d

josieang force-pushed the licenses branch from e741c98 to 1ff92f9 Compare October 27, 2023 07:16

obey the linter

a738691

josieang added 4 commits November 2, 2023 08:54

Merge branch 'main' into licenses-new

e0fdca5

upgrade grpc to non vulnerable version

8308616

fix spacing of table in unit test

9c9ea75

maybe this will help with the failing test

make source paths as relative to the working directory

7675a78

another-rex requested changes Nov 3, 2023

View reviewed changes

josieang added 2 commits November 3, 2023 13:38

respond to another-rex's comments

199cda2

Merge branch 'main' into licenses-new

8b708d8

josieang added a commit to josieang/osv-scanner that referenced this pull request Nov 3, 2023

upgrade version of Go

e8aa96a

I want to use errors.Join in the following PR: - License checker feature google#501 It is a method added in go 1.20.

josieang mentioned this pull request Nov 3, 2023

upgrade version of Go in GitHub checks #637

Merged

add a markdown table test

b87803c

josieang added a commit that referenced this pull request Nov 3, 2023

upgrade version of Go in GitHub checks (#637)

4aed961

I want to use errors.Join in the following PR: - License checker feature #501 It is a method added in go 1.20.

Merge branch 'main' into licenses-new

11fa7cf

josieang requested a review from another-rex November 3, 2023 05:07

another-rex reviewed Nov 3, 2023

View reviewed changes

cmd/osv-scanner/main.go Outdated Show resolved Hide resolved

Rename the flags back

288bef9

josieang requested a review from another-rex November 3, 2023 05:14

another-rex approved these changes Nov 3, 2023

View reviewed changes

josieang merged commit a85d675 into google:main Nov 5, 2023

josieang mentioned this pull request Nov 21, 2023

Support license checks #477

Closed

oliverchang reviewed Nov 22, 2023

View reviewed changes

josieang mentioned this pull request Nov 23, 2023

consider putting details about errors in models.VulnerabilityResults #675

Open

oliverchang mentioned this pull request Nov 23, 2023

Simplify return codes #676

Closed

BrewTestBot mentioned this pull request Dec 6, 2023

osv-scanner 1.5.0 Homebrew/homebrew-core#156557

Merged

another-rex mentioned this pull request Dec 6, 2023

refactor: switch golang.org/x/exp/slices usages to stdlib #690

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

License checker feature #501

License checker feature #501

josieang commented Aug 21, 2023 •

edited

Loading

josieang Aug 21, 2023

another-rex Aug 22, 2023

oliverchang Aug 28, 2023

josieang Aug 28, 2023 •

edited

Loading

another-rex Aug 28, 2023

josieang Aug 28, 2023 •

edited

Loading

josieang Aug 28, 2023

another-rex Aug 28, 2023

josieang Aug 28, 2023

josieang Aug 29, 2023

another-rex left a comment

josieang commented Aug 31, 2023

codecov-commenter commented Nov 1, 2023 •

edited

Loading

another-rex left a comment

josieang commented Nov 3, 2023

another-rex left a comment

oliverchang Nov 22, 2023

G-Rath Nov 23, 2023

josieang Nov 23, 2023

josieang Nov 23, 2023

oliverchang Nov 23, 2023

oliverchang Nov 22, 2023

License checker feature #501

License checker feature #501

Conversation

josieang commented Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josieang Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josieang Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

another-rex left a comment

Choose a reason for hiding this comment

josieang commented Aug 31, 2023

codecov-commenter commented Nov 1, 2023 • edited Loading

Codecov Report

another-rex left a comment

Choose a reason for hiding this comment

josieang commented Nov 3, 2023

another-rex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josieang commented Aug 21, 2023 •

edited

Loading

josieang Aug 28, 2023 •

edited

Loading

josieang Aug 28, 2023 •

edited

Loading

codecov-commenter commented Nov 1, 2023 •

edited

Loading