Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Use lockfile scalibr interface #1330

Merged
merged 148 commits into from
Nov 1, 2024

Conversation

another-rex
Copy link
Collaborator

@another-rex another-rex commented Oct 18, 2024

This PR contains all the code required to move to osv-scalibr while making the existing code compile and pass all tests (container tests not passing because of a bug in the scalibr alpine extractor).

Changes not mentioned in the following list will be split off in separate PRs which should land before this PR.

Those are:

Changes in this PR:

  • Fixture changes:
    • Scalibr Python requirements.txt extractor currently doesn't support packages without versions, so added some version strings to the test files
  • Image package required quite a bit of reworking to successfully update.
    • Add the ability to iterate through a directory via the pathtree library
    • Support scalibr FS interface for Layers
    • Add conversion code to convert inventories from osv-scalibr back to v1's lockfile and Inventory
      • This is done to minimize snapshot changes. Followup PRs should remove this conversion
  • Add internal/lockfilescalibr package:
    • errors.go adds common extraction errors we want to translate.
    • translation.go adds helper functions and translation logic between osv-scanner v1 extractor names, and osv-scalibr extractor names.

Changes in followup PRs:

  • Delete lockfiles package and migrate everything to use osv-scalibr extractors
  • Remove conversion code in image

@another-rex another-rex marked this pull request as ready for review October 28, 2024 00:09
@another-rex
Copy link
Collaborator Author

This PR is ready for review, but the snapshots tests will be updated to the updated version to make the test pass once google/osv-scalibr#241 is merged.

We also currently use a pseudo version of osv-scalibr until a release is made with all the latest changes.

pkg/osvscanner/osvscanner.go Outdated Show resolved Hide resolved
internal/image/layer.go Outdated Show resolved Hide resolved
// causing artifact extractors to double extract elements here.
// So let's skip all these directories for now.
// See (b/364536788)
if strings.HasPrefix(file.virtualPath, "/usr/local/") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why skip scanning if /usr/local/ ?

/usr/local generally has stuff not installed by OS package managers right? it's everything else under /usr that's problematic wrt double scanning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we found a lot of OS packages get installed here. @hogo6002 IIRC you found a lot of go compiler binaries here?

Copy link
Contributor

@hogo6002 hogo6002 Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we found a lot of go compiler binaries there
Update: not usr/local, it was usr/lib

Copy link
Contributor

@hogo6002 hogo6002 Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example:
GO-2023-2375 has been found in the actual project binary.
image
But it also shows 20 times in /usr/lib
image
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah gotcha, I'll update this path. I think I found some npm packages in /usr/local though. I'll double check

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking https://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard

It looks like we should ignore the entire /usr directory. I think everything in there is written by os package managers, since it is suppose to be read only.

internal/lockfilescalibr/translation.go Outdated Show resolved Hide resolved
internal/lockfilescalibr/translation.go Show resolved Hide resolved
@another-rex another-rex force-pushed the use-lockfile-scalibr-interface branch from cb55375 to 60daa0c Compare October 28, 2024 04:35
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome! I won't pretend that I understand all of it, but it's exciting to see 😄

Scalibr Python requirements.txt extractor currently doesn't support packages without versions, so added some version strings to the test files

that defeats the point of those test fixtures, and makes this a breaking change (oh wait, we're already doing one of those 🙈)

I'm guessing it's not an easy fix, but it'd nice if we could note somewhere that ideally these should be removed once support is added


I'm not sure if there's anything else like that - if there is, maybe would it be useful if we created like a "post v2 follow-up" issue where we could just chuck this stuff into so we don't forget, and then we can come back after the majors out to properly split/link things with proper issues? (probably not worth it though if this is the only one)

internal/image/image.go Outdated Show resolved Hide resolved
"github.com/google/osv-scalibr/extractor"
"github.com/google/osv-scalibr/extractor/filesystem/os/apk"
"github.com/google/osv-scalibr/extractor/filesystem/os/dpkg"
scalibrosv "github.com/google/osv-scalibr/extractor/filesystem/osv"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bro 🤜

// TODO: Currently osv-scalibr does not correctly annotate OS packages
// causing artifact extractors to double extract elements here.
// So let's skip all these directories for now.
// See (b/364536788)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this bug number should be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is just reference for us to keep track.

another-rex and others added 6 commits November 1, 2024 11:58
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Xueqin Cui <[email protected]>
Co-authored-by: Michael Kedar <[email protected]>
Co-authored-by: Gareth Jones <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Resolves google#1316 

Update the --docker flag to:
- Only accept one image to scan at a time (to make displaying results
easier)
- Call new image scanning function internally.
- Acts like a convenience function for 
```
docker save <image-name> > img-name.tar && osv-scanner --experimental-oci-image=img.name.tar
```

TODO: 
- [x] Add an ACCEPTANCE test which uses docker to pull down a stable
image.
- [x] Include a docker pull first, as docker save only saves images
already on device and does not pull images online.
@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 77.60252% with 71 lines in your changes missing coverage. Please review.

Project coverage is 68.68%. Comparing base (1638434) to head (d1e9eb6).

Files with missing lines Patch % Lines
internal/image/layer.go 42.85% 29 Missing and 3 partials ⚠️
internal/lockfilescalibr/translation.go 82.35% 8 Missing and 4 partials ⚠️
internal/image/extractor.go 82.50% 6 Missing and 1 partial ⚠️
pkg/osvscanner/osvscanner.go 90.00% 4 Missing and 3 partials ⚠️
internal/image/scan.go 94.52% 2 Missing and 2 partials ⚠️
...lescalibr/language/osv/osvscannerjson/extractor.go 0.00% 4 Missing ⚠️
...ckfilescalibr/language/java/pomxmlnet/extractor.go 0.00% 3 Missing ⚠️
...alibr/language/javascript/nodemodules/extractor.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##               v2    #1330      +/-   ##
==========================================
- Coverage   68.72%   68.68%   -0.05%     
==========================================
  Files         187      188       +1     
  Lines       17884    18075     +191     
==========================================
+ Hits        12290    12414     +124     
- Misses       4917     4977      +60     
- Partials      677      684       +7     

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

@another-rex another-rex merged commit b15b566 into google:v2 Nov 1, 2024
13 checks passed
another-rex added a commit that referenced this pull request Nov 29, 2024
…ncy resolution (#1331)

Part of #1330

No functional change is made compared to the version in
`internal/manifest`, just switched to use the osv-scalibr interface.

Extractors moved to lockfilescalibr as a temporary staging ground before
moving to osv-scalibr.
another-rex added a commit that referenced this pull request Nov 29, 2024
This PR contains all the code required to move to osv-scalibr while
making the existing code compile and pass all tests (container tests not
passing because of a bug in the scalibr alpine extractor).

Changes not mentioned in the following list will be split off in
separate PRs which should land before this PR.

Those are:
- [x] #1337
- [x] #1331
- [x] #1338
- [x] #1341
- [x] #1345

Changes in this PR:
- Fixture changes:
- Scalibr Python requirements.txt extractor currently doesn't support
packages without versions, so added some version strings to the test
files
- Image package required quite a bit of reworking to successfully
update.
- Add the ability to iterate through a directory via the pathtree
library
  - Support scalibr FS interface for Layers
- Add conversion code to convert inventories from osv-scalibr back to
v1's lockfile and Inventory
- This is done to minimize snapshot changes. Followup PRs should remove
this conversion
- Add `internal/lockfilescalibr` package:
  - `errors.go` adds common extraction errors we want to translate.
- `translation.go` adds helper functions and translation logic between
osv-scanner v1 extractor names, and osv-scalibr extractor names.

Changes in followup PRs:
- Delete lockfiles package and migrate everything to use osv-scalibr
extractors
- Remove conversion code in image

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Gareth Jones <[email protected]>
Co-authored-by: Xueqin Cui <[email protected]>
Co-authored-by: Michael Kedar <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants