Confirm all rpms in sboms have a known repo id #1135

simonbaird · 2024-09-11T21:26:26Z

Note that we can't enable this now, since the sboms that Konflux creates don't yet have repo ids, so I didn't add the one relevant check to the redhat collection in this PR.

Ref: EC-848

codecov-commenter · 2024-09-11T21:29:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b445a4f) to head (7ccc545).

Additional details and impacted files

@@            Coverage Diff             @@
##              main     #1135    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          115       117     +2     
  Lines         5894      6010   +116     
==========================================
+ Hits          5894      6010   +116

Files with missing lines	Coverage Δ
policy/lib/set_helpers.rego	`100.00% <100.00%> (ø)`
policy/lib/set_helpers_test.rego	`100.00% <100.00%> (ø)`
policy/release/rpm_repos.rego	`100.00% <100.00%> (ø)`
policy/release/rpm_repos_test.rego	`100.00% <100.00%> (ø)`

simonbaird · 2024-09-11T22:10:58Z

I might tinker with the annotations a little more, but I think it's ready for review.

zregvart

LGTM, one test case that could be added is for checking that the list appears in the error message, in addition to testing truncated_list_to_string, but that's just nitpicking...

policy/release/repo_ids.rego

simonbaird · 2024-09-13T16:22:06Z

I'll make a few changes. Moving to draft for a bit.

lcarva · 2024-09-13T16:43:59Z

policy/release/repo_ids.rego

+#   Checks that all RPMs listed in the SBOM have a valid and known
+#   repository id.
+#
+package policy.release.repo_ids


I wonder if this should be rpm_repo_ids since the repository terminology is very ambiguous.

Also not sure if ids need to be in the name. I can imagine this package doing something with rpm repos which is not necessarily related to their ids.

Revised in latest version. See if you like it.

lcarva · 2024-09-13T16:47:54Z

policy/release/repo_ids.rego

+# description: >-
+#   A list of valid and known repository ids should be available in the data.
+# custom:
+#   short_name: rpm_repo_ids_list_provided


The juxtaposition of package name and short name make this repetitive, repo_ids.rpm_repo_ids_list_provided. With my previous comment in mind, consider something like this rpm_repo.known_ids_list_provided.

Fixed I think.

policy/release/repo_ids.rego

lcarva · 2024-09-13T16:54:27Z

policy/release/repo_ids.rego

+#   solution: >-
+#     Ensure every rpm is from a known and accepted repository and that the data in the SBOM
+#     correctly records that.
+#   # Todo: Until the sbom generation is upated, this will always fail,


We should enable this when that change is released and there's a new Task users can upgrade to. Let's use effective_on to give users ~30 days to make the upgrade.

Will do in a future PR.

policy/release/repo_ids.rego

lcarva · 2024-09-13T17:03:04Z

policy/release/repo_ids.rego

+import data.lib
+
+# METADATA
+# title: Known repo id list provided


This can probably be merged with rpm_repo_ids_valid.

Still open to persuasion, but I think I preferred them separate.

policy/release/repo_ids.rego

lcarva · 2024-09-13T17:06:07Z

policy/release/repo_ids.rego

+}
+
+# Avoid including thousands of bad purls in the violation reason
+max_bad_purls := 10


This seems oddly specific. Let all the failures show up so users can tackle them as needed.

Are you worried about showing (say) 5000 violations?

Because every purl is malformed? That seems like a bigger problem.

My thinking is: if you do have this problem, it's better if we don't add a 9000 screen EC task log on top of problem pile. 😁

Fair point.

simonbaird · 2024-09-17T11:51:16Z

Still draft, but nearly there.

The one exception is coverage, since the coverage numbers only make sence if you run all the tests. I also changed the ordering of targets for the sake of tidiness. Quality-of-life tweak while doing some TDD for... Ref: https://issues.redhat.com/browse/EC-742

I want to use this in the next commit. (As mentioned in the comments, I think it can make some code a little more readable in some cases.)

simonbaird · 2024-09-17T18:05:28Z

The latest version has a different way of limiting the number of violations, as discussed post standup today.

simonbaird · 2024-09-17T18:09:28Z

Moved out of draft.

For all components in all sboms that are rpms, require they have a repository_id value in their purl, and require that the repository_id value is in in the big list of known repository_ids. Includes a bit of extra work to limit the number of violations produced, since I'm expecting there to be either none of many hundreds. As mentioned in a Todo in the comments, this is not yet added to the redhat collection, but it will be added soon in an upcoming PR. Ref: https://issues.redhat.com/browse/EC-848

simonbaird · 2024-09-17T22:38:19Z

Things not in this PR:

Adding the new rule to the 'redhat' collection. I'm planning to do that later while adding an effective_on date in the future.
Using the 'item' (correction 'term') mechanism to be able to create fine grained exclude directives. This is a good idea, but I think it can be done in a new PR and in a new story.

zregvart

Would have loved to have seen term on a per package basis, but this is okay for now and that can be added in a followup

simonbaird force-pushed the repoid-checks branch 4 times, most recently from 35a11a7 to 4e28682 Compare September 11, 2024 22:10

simonbaird marked this pull request as ready for review September 11, 2024 22:11

zregvart reviewed Sep 12, 2024

View reviewed changes

policy/release/repo_ids.rego Outdated Show resolved Hide resolved

policy/release/repo_ids.rego Outdated Show resolved Hide resolved

simonbaird marked this pull request as draft September 13, 2024 16:22

lcarva reviewed Sep 13, 2024

View reviewed changes

simonbaird force-pushed the repoid-checks branch from 4e28682 to cd28756 Compare September 17, 2024 11:32

simonbaird force-pushed the repoid-checks branch from cd28756 to 883f4e7 Compare September 17, 2024 18:01

simonbaird added 2 commits September 17, 2024 14:04

Add to_array helper similar to to_set

26792e1

I want to use this in the next commit. (As mentioned in the comments, I think it can make some code a little more readable in some cases.)

simonbaird force-pushed the repoid-checks branch from 883f4e7 to ed2aef0 Compare September 17, 2024 18:04

simonbaird marked this pull request as ready for review September 17, 2024 18:07

simonbaird force-pushed the repoid-checks branch 2 times, most recently from 10574f9 to 7e78f51 Compare September 17, 2024 21:47

simonbaird force-pushed the repoid-checks branch from 7e78f51 to 7ccc545 Compare September 17, 2024 21:53

zregvart approved these changes Sep 18, 2024

View reviewed changes

simonbaird merged commit 0c4f17b into enterprise-contract:main Sep 18, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Confirm all rpms in sboms have a known repo id #1135

Confirm all rpms in sboms have a known repo id #1135

simonbaird commented Sep 11, 2024 •

edited

Loading

codecov-commenter commented Sep 11, 2024 •

edited

Loading

simonbaird commented Sep 11, 2024

zregvart left a comment

simonbaird commented Sep 13, 2024

lcarva Sep 13, 2024

simonbaird Sep 17, 2024

lcarva Sep 13, 2024

simonbaird Sep 17, 2024

lcarva Sep 13, 2024

simonbaird Sep 17, 2024

lcarva Sep 13, 2024

simonbaird Sep 17, 2024

lcarva Sep 13, 2024

simonbaird Sep 13, 2024

lcarva Sep 13, 2024

simonbaird Sep 17, 2024

lcarva Sep 17, 2024

simonbaird commented Sep 17, 2024

simonbaird commented Sep 17, 2024

simonbaird commented Sep 17, 2024

simonbaird commented Sep 17, 2024 •

edited

Loading

zregvart left a comment

Confirm all rpms in sboms have a known repo id #1135

Confirm all rpms in sboms have a known repo id #1135

Conversation

simonbaird commented Sep 11, 2024 • edited Loading

codecov-commenter commented Sep 11, 2024 • edited Loading

Codecov Report

simonbaird commented Sep 11, 2024

zregvart left a comment

Choose a reason for hiding this comment

simonbaird commented Sep 13, 2024

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

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

simonbaird commented Sep 17, 2024

simonbaird commented Sep 17, 2024

simonbaird commented Sep 17, 2024

simonbaird commented Sep 17, 2024 • edited Loading

zregvart left a comment

Choose a reason for hiding this comment

simonbaird commented Sep 11, 2024 •

edited

Loading

codecov-commenter commented Sep 11, 2024 •

edited

Loading

simonbaird commented Sep 17, 2024 •

edited

Loading