refactor(report): Replacing `source_location` in `github` report when scanning an image #5999

Maxim-Durand · 2024-01-24T20:28:36Z

Description

Relates to #5998

The goal is to change the source_location of the manifest in SBOM report so it provides useful information when displayed in Github Dependency.

The current solution is to use <image_registry>/<image_name>:<tag>@sha256:<image_hash> format.

Before changes

As you can see instead of specifying the image scanned, it only show Python.

After changes

The report now shows the image name and tag as the source_location for all packages.

"manifests": {
    "Python": {
      "name": "python-pkg",
      "file": {
        "source_location": "redacted_registry/accesspod:latest"
      },

Checklist

I've read the guidelines for contributing to this repository.
I've followed the conventions in the PR title.
I've added tests that prove my fix is effective or that my feature works.
I've updated the documentation with the relevant information (if needed).
I've added usage information (if the PR introduces new options)
I've included a "before" and "after" example to the description (if the PR is a user interface change).

Maxim-Durand · 2024-01-24T20:31:11Z

pkg/report/github/github.go

-				SrcLocation: result.Target,
+			if report.ArtifactType == "container_image" {
+				// Replacing `source_location` by the image name and tag
+				image_reference := strings.Join(report.Metadata.RepoTags, ", ")


Using report.ArtifactName instead of report.Metadata.RepoTags would miss the image tag.

Could we use "RepoDigests" instead, as that included both the tag and digest? That would address the comment I just put on the PR discussion?

Just to confirm using RepoDigests would work I did some testing and it seems to be working fine.

Each time you scan your image with a new sha it will close all the previously open alerts for that image and only keep the latest version open.
But you're still able to see the previously closed alerts by simply using the is:closed filter and searching for a specific manifest works fine too.

A mix of suggestion at #5999 (comment) and yours would give us the longest form possible storing all information:

<image_registry>/<image_name>:<tag>@sha256:<image_hash> - <filePath_to_package>

I tested it and although it's very long, filtering seems to work too (the image_hash is fake).

Important
One big drawback of using DmitriyLewen@8431664 is that since each vulnerable package has its own source_location set using its FilePath, it means there is no more grouping in Github.
So, if you scan an image with 10 vulnerable packages, Github will show 10 different manifest instead of 1.
To me this defeats the purpose, and that's why I would advise to only use <image_registry>/<image_name>:<tag>@sha256:<image_hash>.

Yeah, showing the package location (while having those disadvantages) will help with investigations about what actually needs to be fixed/updated

DmitriyLewen

Hello @Maxim-Durand
Thanks for your work!

I left some comments. Take a look, please.

Can you also update tests, please

DmitriyLewen · 2024-01-25T05:10:26Z

pkg/report/github/github.go

@@ -105,8 +105,17 @@ func (w Writer) Write(ctx context.Context, report types.Report) error {
 		manifest.Name = string(result.Type)
 		// show path for language-specific packages only
 		if result.Class == types.ClassLangPkg {
-			manifest.File = &File{
-				SrcLocation: result.Target,


We aggregate some packages to 1 result -

trivy/pkg/fanal/applier/docker.go

Lines 263 to 291 in fb36c4e

func aggregate(detail *ftypes.ArtifactDetail) {

var apps []ftypes.Application

aggregatedApps := map[ftypes.LangType]*ftypes.Application{

ftypes.PythonPkg: {Type: ftypes.PythonPkg},

ftypes.CondaPkg: {Type: ftypes.CondaPkg},

ftypes.GemSpec: {Type: ftypes.GemSpec},

ftypes.NodePkg: {Type: ftypes.NodePkg},

ftypes.Jar: {Type: ftypes.Jar},

}

for _, app := range detail.Applications {

a, ok := aggregatedApps[app.Type]

if !ok {

apps = append(apps, app)

continue

}

a.Libraries = append(a.Libraries, app.Libraries...)

}

for _, app := range aggregatedApps {

if len(app.Libraries) > 0 {

apps = append(apps, *app)

}

}

// Overwrite Applications

detail.Applications = apps

}

But we can find path from Results.Packages.FilePath:

{ "Target": "Python", "Class": "lang-pkgs", "Type": "python-pkg", "Packages": [ { "Name": "pip", "Version": "23.2.1", "Licenses": [ "MIT" ], "Layer": { "DiffID": "sha256:a7d483b6066e2ed6c9cd1d5890a93326e3f192ef6e537c046296053fa547a844" }, "FilePath": "usr/local/lib/python3.12/site-packages/pip-23.2.1.dist-info/METADATA" },

Maybe we just use FilePath for these packages?

Although I agree with you it's an interesting change as it would make the manifest more precise.

Nonetheless, I think the full FilePath would be too much to render in Github Dependency and would bloat the UI.
I can do a test with that change if you want and show the results here ?

Otherwise we could try to extract important values from the FilePath and adding them to result.Target, but that seems complex and prone to errors.

For other languages we already use filePath (filePath is stored in target):

➜ 5999 trivy -d fs -f github . | jq .manifests ... { "foo/bar/requirements.txt": { "name": "pip", "file": { "source_location": "foo/bar/requirements.txt" }, "resolved": { "click": { "package_url": "pkg:pypi/[email protected]", "relationship": "direct", "scope": "runtime" } } } }

And we didn't get issues about UI.

I can do a test with that change if you want and show the results here ?

Yes please. If you have time, check it out.

So I have a few things to report:

As you pointed out trivy aggregates multiple packages in the same "App" (for instance Python or Node) into one result.
The issue is that in the sbom format, the source_location is set at the manifest level not at the package level (resolved key in sbom format). Meaning, if we keep the default trivy aggregation then we have to use only one of all the packages FilePath. How to define that value is either arbitrary, or we need some sort of heuristic ?

In terms of UI it isn't as bloated as I expected, so I think you're right and it's ok to use.

The source_location used in the above screenshot is simply the FilePath of the last package in result.Packages so it's not even correct.

In the case trivy is scanning an image I think it's still pretty useless to replace the manifest's source_location with this FilePath value and we should instead reference the image's name and tag.

Meaning, if we keep the default trivy aggregation then we have to use only one of all the packages FilePath. How to define that value is either arbitrary, or we need some sort of heuristic ?

We have already seen cases where it is necessary to remove aggregation, but this requires large changes and we are not sure about it.
I created a small function to get filePath for each package - DmitriyLewen@8431664
Can you take a look?

In the case trivy is scanning an image I think it's still pretty useless to replace the manifest's source_location with this FilePath value and we should instead reference the image's name and tag.

This makes sense for UI. But what worries me is case where user only knows that image contains vulnerable package, but doesn't have filePath to find and update that package. It would be great to add filePath somehow.
e.g. we can use <image_name>:<tag> - <filePath> format for manifest map key. Is it possible to see gsbom file after uploading to GitHub?

Not that I can see in Github Dependency UI no, see screenshot below to see how it looks.

So user can only get package metadata when taking snapshot, right?

If they use trivy then there's no issue as the user will now be able to find the file path for each package by parsing the JSON report before sending it to GitHub.

But if they use trivy-action then the report is sent automatically and the user doesn't have access to the original report hence he wouldn't be able to parse it.
Maybe if the user then export it from GitHub, the metadata tag with each package file path will still be there, I'll test that.

This is what I got from github export to SBOM for the same package as in #5999 (comment):

{ "SPDXID": "SPDXRef-pip-pillow-10.1.0", "name": "pip:pillow", "versionInfo": "10.1.0", "downloadLocation": "NOASSERTION", "filesAnalyzed": false, "licenseConcluded": "CAL-1.0", "supplier": "NOASSERTION", "externalRefs": [ { "referenceCategory": "PACKAGE-MANAGER", "referenceLocator": "pkg:pypi/[email protected]", "referenceType": "purl" } ] },

As you can see the metadata tag doesn't show up in the exported SBOM.

Important
Conclusion: users of trivy-action would currently have no way of finding out the filePath of each package when using the github format.

As this issue only involves trivy-action and not trivy itself anymore, I think we should continue this discussion once this PR is merged to find a solution for trivy-action users at aquasecurity/trivy-action#286
(I would recommend to add an option for upload as an artifact so that user can easily fetch the report).

I created aquasecurity/trivy-action#307 to make sure users of trivy-action will know where to find the missing details from Github Dependency.

@DmitriyLewen and @knqyf263 do you agree with this compromise ?
If so, can you review this PR code ?

pkg/report/github/github.go

Co-authored-by: DmitriyLewen <[email protected]>

…ag as source_location

RichardoC · 2024-02-01T13:29:07Z

This will be very handy, to give an example I do nightly scans of "nightly" containers, and the most recently release of my project kube-audit-rest and it's not possible to tell them apart which means I'm not sure if something's a regression or not

Given aquasecurity/trivy-action#279 (comment) I think it should also include the digest e.g. example.com/my-image:my-tag@sha256-fake-hash so that it's possible to tell which version of an image was scanned, as the image could have been retagged

Below is an example screenshot showing how it's not possible to tell them apart currently

…t for source_location of an image scan

RichardoC · 2024-02-03T14:38:20Z

pkg/report/github/github.go

+				image_with_hash := strings.Join(report.Metadata.RepoDigests, ", ")
+				_, image_hash, found := strings.Cut(image_with_hash, "@")
+				if found {
+					image_reference += "@" + image_hash


Why are you doing this rather than just using RepoDigests directly? Mind adding a comment?

RepoDigests looks like <registry>/<image_name>@sha256:<image_hash>.
Whereas RepoTag looks like looks like <registry>/<image_name>:<image_tag>.

So if only using RepoDigests it means loosing the <image_tag>.

Good question, I added a comment.

…ation

RichardoC

Thank you for fixing this!

DmitriyLewen

Hello @Maxim-Durand
Sorry for delay.

Left comments. Take a look, please.

pkg/report/github/github_test.go

pkg/report/github/github.go

Co-authored-by: DmitriyLewen <[email protected]>

DmitriyLewen

@Maxim-Duran
Thank you very much for your work, discussion, opinions and patience.

LGTM. I think we choosed the best solution for this problem.

@knqyf263 I approved this PR. Take a look, please.

replacing source_location in github report when scanning an image

458a30b

Maxim-Durand commented Jan 24, 2024

View reviewed changes

Maxim-Durand mentioned this pull request Jan 24, 2024

Changing manifest source_location when sending SBOM of an image to Github Dependency aquasecurity/trivy-action#300

Closed

DmitriyLewen reviewed Jan 25, 2024

View reviewed changes

Update pkg/report/github/github.go

054833e

Co-authored-by: DmitriyLewen <[email protected]>

This comment was marked as resolved.

Sign in to view

Maxim-Durand changed the title ~~fix(image): Replacing source_location in github report when scanning an image~~ refactor(template): Replacing source_location in github report when scanning an image Jan 26, 2024

added test for image export to github using the full image_name and t…

3c2d602

…ag as source_location

Using <image_registry>/<image_name>:<tag>@sha256:<image_hash> forma…

b65ef00

…t for source_location of an image scan

Maxim-Durand changed the title ~~refactor(template): Replacing source_location in github report when scanning an image~~ refactor(report): Replacing source_location in github report when scanning an image Feb 3, 2024

Maxim-Durand marked this pull request as ready for review February 3, 2024 12:27

Maxim-Durand requested a review from knqyf263 as a code owner February 3, 2024 12:27

Maxim-Durand requested review from RichardoC and DmitriyLewen February 3, 2024 12:32

RichardoC reviewed Feb 3, 2024

View reviewed changes

added comments about RepoDigests and RepoTag parsing and concaten…

077f52f

…ation

Maxim-Durand requested a review from RichardoC February 5, 2024 13:33

adding "source_location" of each package into their metadata field

dffe603

RichardoC approved these changes Feb 7, 2024

View reviewed changes

Maxim-Durand mentioned this pull request Feb 12, 2024

docs(report): improve documentation around Using Trivy to generate SBOM and sending it to Github aquasecurity/trivy-action#307

Merged

DmitriyLewen reviewed Feb 19, 2024

View reviewed changes

pkg/report/github/github_test.go Outdated Show resolved Hide resolved

pkg/report/github/github.go Outdated Show resolved Hide resolved

Apply suggestions from DmitriyLewen review

c43fd98

Co-authored-by: DmitriyLewen <[email protected]>

Maxim-Durand requested a review from DmitriyLewen February 19, 2024 14:55

lint fix

fc53d37

DmitriyLewen approved these changes Feb 22, 2024

View reviewed changes

knqyf263 added this pull request to the merge queue Feb 22, 2024

Merged via the queue into aquasecurity:main with commit 388f476 Feb 22, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(report): Replacing `source_location` in `github` report when scanning an image #5999

refactor(report): Replacing `source_location` in `github` report when scanning an image #5999

Maxim-Durand commented Jan 24, 2024 •

edited

Loading

Maxim-Durand Jan 24, 2024 •

edited

Loading

RichardoC Feb 1, 2024 •

edited

Loading

Maxim-Durand Feb 2, 2024

Maxim-Durand Feb 2, 2024 •

edited

Loading

RichardoC Feb 3, 2024

DmitriyLewen left a comment

DmitriyLewen Jan 25, 2024

Maxim-Durand Jan 25, 2024

DmitriyLewen Jan 26, 2024

Maxim-Durand Jan 26, 2024 •

edited

Loading

DmitriyLewen Jan 29, 2024

Maxim-Durand Feb 8, 2024

DmitriyLewen Feb 9, 2024 •

edited

Loading

Maxim-Durand Feb 9, 2024 •

edited

Loading

Maxim-Durand Feb 9, 2024 •

edited

Loading

Maxim-Durand Feb 13, 2024

This comment was marked as resolved.

RichardoC commented Feb 1, 2024

RichardoC Feb 3, 2024

Maxim-Durand Feb 5, 2024 •

edited

Loading

RichardoC left a comment

DmitriyLewen left a comment

DmitriyLewen left a comment

	func aggregate(detail *ftypes.ArtifactDetail) {
	var apps []ftypes.Application

	aggregatedApps := map[ftypes.LangType]*ftypes.Application{
	ftypes.PythonPkg: {Type: ftypes.PythonPkg},
	ftypes.CondaPkg: {Type: ftypes.CondaPkg},
	ftypes.GemSpec: {Type: ftypes.GemSpec},
	ftypes.NodePkg: {Type: ftypes.NodePkg},
	ftypes.Jar: {Type: ftypes.Jar},
	}

	for _, app := range detail.Applications {
	a, ok := aggregatedApps[app.Type]
	if !ok {
	apps = append(apps, app)
	continue
	}
	a.Libraries = append(a.Libraries, app.Libraries...)
	}

	for _, app := range aggregatedApps {
	if len(app.Libraries) > 0 {
	apps = append(apps, *app)
	}
	}

	// Overwrite Applications
	detail.Applications = apps
	}

refactor(report): Replacing source_location in github report when scanning an image #5999

refactor(report): Replacing source_location in github report when scanning an image #5999

Conversation

Maxim-Durand commented Jan 24, 2024 • edited Loading

Description

Before changes

After changes

Checklist

Maxim-Durand Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

RichardoC Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maxim-Durand Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitriyLewen 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

Maxim-Durand Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitriyLewen Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Maxim-Durand Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Maxim-Durand Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

RichardoC commented Feb 1, 2024

Choose a reason for hiding this comment

Maxim-Durand Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

RichardoC left a comment

Choose a reason for hiding this comment

DmitriyLewen left a comment

Choose a reason for hiding this comment

DmitriyLewen left a comment

Choose a reason for hiding this comment

refactor(report): Replacing `source_location` in `github` report when scanning an image #5999

refactor(report): Replacing `source_location` in `github` report when scanning an image #5999

Maxim-Durand commented Jan 24, 2024 •

edited

Loading

Maxim-Durand Jan 24, 2024 •

edited

Loading

RichardoC Feb 1, 2024 •

edited

Loading

Maxim-Durand Feb 2, 2024 •

edited

Loading

Maxim-Durand Jan 26, 2024 •

edited

Loading

DmitriyLewen Feb 9, 2024 •

edited

Loading

Maxim-Durand Feb 9, 2024 •

edited

Loading

Maxim-Durand Feb 9, 2024 •

edited

Loading

Maxim-Durand Feb 5, 2024 •

edited

Loading