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

enricher: add RHCC enricher #1057

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

enricher: add RHCC enricher #1057

wants to merge 1 commit into from

Conversation

crozzy
Copy link
Contributor

@crozzy crozzy commented Sep 8, 2023

This change introduces an enricher who's purpose is to scope down the vulnerability report for layers that have RHCC packages. This approach helps to keep the index report unchanged and therefore state is less of an issue, it also builds on existing machinary.

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Attention: Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.

Project coverage is 55.47%. Comparing base (7088f7b) to head (0a9465c).

Files with missing lines Patch % Lines
enricher/rhcc/rhcc.go 87.50% 1 Missing and 1 partial ⚠️
rhel/rhcc/scanner.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1057      +/-   ##
==========================================
+ Coverage   55.41%   55.47%   +0.05%     
==========================================
  Files         282      283       +1     
  Lines       17890    17906      +16     
==========================================
+ Hits         9914     9933      +19     
+ Misses       6934     6931       -3     
  Partials     1042     1042              

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

@crozzy crozzy force-pushed the rhcc-enricher branch 5 times, most recently from 5aca24a to a6881c2 Compare September 14, 2023 22:58
@crozzy crozzy force-pushed the rhcc-enricher branch 2 times, most recently from 27eb16c to 9c10c91 Compare June 11, 2024 16:03
@crozzy
Copy link
Contributor Author

crozzy commented Jun 11, 2024

The result is something like this:

{
  "message/vnd.clair.map.layer; enricher=clair.rhcc": [
    {
      "2": "sha256:013d2d7b1d1f8a94ffb762e6960fb5f483c60e38bd8740b1ec1f1a557d3d1bbf",
      "4": "sha256:013d2d7b1d1f8a94ffb762e6960fb5f483c60e38bd8740b1ec1f1a557d3d1bbf",
      "6": "sha256:d93e847446533e9af99b246c8cebea2527bf3e9db82e45fb59e3d9f1443a7d2c",
      "8": "sha256:d93e847446533e9af99b246c8cebea2527bf3e9db82e45fb59e3d9f1443a7d2c"
    }
  ]
}

There are a couple of ways to do this, it could also be technically reversed. In that case we'd have to just consider binary packages (at the moment each RHCC layer has a source and a binary package describing it). I'm not sure what consumers of Clair would prefer.

@crozzy crozzy force-pushed the rhcc-enricher branch 2 times, most recently from eb59bba to 90a888a Compare June 11, 2024 18:35
@crozzy crozzy marked this pull request as ready for review June 11, 2024 18:40
@crozzy crozzy requested a review from a team as a code owner June 11, 2024 18:40
@crozzy crozzy requested review from hdonnay and removed request for a team June 11, 2024 18:40
Copy link
Member

@hdonnay hdonnay left a comment

Choose a reason for hiding this comment

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

Question about the approach

enricher/rhcc/rhcc.go Show resolved Hide resolved
@crozzy crozzy force-pushed the rhcc-enricher branch 2 times, most recently from f7156cc to c538f76 Compare June 12, 2024 15:20
enricher/rhcc/rhcc.go Outdated Show resolved Hide resolved
Copy link
Contributor

@RTann RTann left a comment

Choose a reason for hiding this comment

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

Haven't looked at the logic much yet, but I have a question. It was brought to my attention layers may be squashed, even if from the base image. For example podman build --squash-all. In that scenario, we will just see a single layer I believe, and I'm not sure how useful this would be in that scenario. I'm wondering what your thoughts are about this? I'm not saying to throw this out, but rather we may just need to make it clear to users that this may not be that useful for those images which are completely squashed

rhel/rhcc/rhcc.go Show resolved Hide resolved
@crozzy
Copy link
Contributor Author

crozzy commented Oct 10, 2024

Haven't looked at the logic much yet, but I have a question. It was brought to my attention layers may be squashed, even if from the base image. For example podman build --squash-all. In that scenario, we will just see a single layer I believe, and I'm not sure how useful this would be in that scenario. I'm wondering what your thoughts are about this? I'm not saying to throw this out, but rather we may just need to make it clear to users that this may not be that useful for those images which are completely squashed

If the image is squashed we should still be able to identify every RH layer that has composed the squashed layer but yeah, any client making decisions off of this information would need to be aware that it's fallible if the input has been manipulated. Clients could potentially be told to ignore vulnerabilities in RH layers that are squashed together with application layers.

This change introduces a new enricher that reports where rhcc packages
exist (if at all), it allows callers to discount vulnerabilities /
packages that come from the same layers. This approach
helps to keep the index report unchanged and therefore state is less
of an issue, it also builds on existing machinary.

Signed-off-by: crozzy <[email protected]>
func (e *Enricher) Name() string { return "rhcc" }

func (e *Enricher) Enrich(ctx context.Context, g driver.EnrichmentGetter, r *claircore.VulnerabilityReport) (string, []json.RawMessage, error) {
problematicPkgs := make(map[string]string)
Copy link
Contributor

Choose a reason for hiding this comment

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

what makes them problematic? 😄

for _, repoID := range e.RepositoryIDs {
repo := r.Repositories[repoID]
if repo.Name == rhcc.GoldRepo.Name {
problematicPkgs[id] = e.IntroducedIn.String()
Copy link
Contributor

Choose a reason for hiding this comment

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

should we break here, too? Actually, this doesn't happen in practice, but in theory: what if there were two environments for this package which relate back to the rhcc.GoldRepo and are in different layers? Which layer will we show (not sure if layer order is guaranteed when looking at the envs)? If this isn't even possible nor do we want to deal with it theoretically, shouold we just break out of the e.RepositoryIDs loop?


func Digest(name string) claircore.Digest {
h := sha256.New()
io.WriteString(h, name)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: panic on error here, too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants