Improve Trusty integration #3277

puerco · 2024-05-08T05:57:12Z

Summary

This PR surfaces the Trusty maliciousness data on the comment added by minder when analyzing dependencies. It also adds the required piping to add the data to the rest of the required low scored dependencies.

I've broken the Trusty evaluator to more scoped utility functions to make them more testable and added a few initial unit tests. It needs a little bit more mocking to write an integration test and the rest of the unit tests

I've simplified the comment template, we now have a single template instead of 3 and it is now pure markdown which is smaller. Here' is a screenshot of the output with malicious dependencies:

Demo PRs showing

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Added initial tests (up to 22% from 0 :) )

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

coveralls · 2024-05-08T07:00:46Z

coverage: 49.324% (+0.2%) from 49.135%
when pulling eeb2ada on puerco:malicious-deps
into daccbc1 on stacklok:main.

jhrozek

one minor comment, but looks good.

jhrozek · 2024-05-08T07:44:27Z

internal/engine/eval/trusty/trusty.go

+	// Classify all dependencies, tracking all that are malicious or scored low
+	for _, dep := range prDependencies.Deps {
+		if err := classifyDependency(ctx, &logger, e.client, ruleConfig, prSummaryHandler, dep); err != nil {
+			return fmt.Errorf("classifying dependency: %w", err)


Not blocking, but do you think we should error out the whole evaluation here? I wonder if we could classify a dependency as something like unknown instead.

Since this logic is responsible for blocking PRs introducing malware into the codebase, I would rather build a more resilient client to make the requests more robust and indeed fail if we can't get the trusty score instead of just letting it through as an unknown. WDYT?

The only two cases where this might fail are when there is an error talking to trusty or due to a misconfiguration in the profile.

lukehinds · 2024-05-08T13:09:17Z

There is more payload from trusty we could leverage here.

Provenance: sigstore or historical provenance. It might make sense to surface a threshold here for folks to set within the policy. This could be an float (I think?) between 1-10, or a simply bool cc: @therealnb , @yrobla

We also have deprecated or achieved available as fields to report in the same way as malicious

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

puerco · 2024-05-09T03:24:41Z

OK, I've surfaced the rest of the malicious data and the score components on the PR comment. The rule evaluator will now honor minimal scores for provenance and activity in the rule configuration.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

jhrozek · 2024-05-10T08:39:39Z

I really like the new format, but it seems like a high-scoring package is getting flagged now as well, see e.g. jakubtestorg/bad-python#193

jhrozek · 2024-05-10T08:44:41Z

internal/engine/eval/trusty/config.go

-	// summary score is used.
-	// If `evaluate_score` is set to something else (e.g. `provenance`)
-	// then that score is used, which comes from the details field.
-	EvaluateScore string `json:"evaluate_score" mapstructure:"evaluate_score"`


Oh this is probably why the "good" packages are now flagged as low scoring? Does it mean that everyone who deployed this profile with the old ruletype (before mindersec/minder-rules-and-profiles#111) would get all their deps flagged?

mmh interesting. Let me check why that one is getting flagged. The removal of EvaluateScore should not affect this one in particular as it has a high score and also a high provenance component.

I could not reproduce it (see here) it is weird because the profile is the same and the previous EvaluateScore is not used anymore. I'll keep looking

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

puerco · 2024-05-13T23:36:18Z

This is how its looking now. I will investigate Jakubs bugfind next but we can merge as it is and iterate on smaller PRs.

jhrozek · 2024-05-14T07:22:05Z

This is how its looking now. I will investigate Jakubs bugfind next but we can merge as it is and iterate on smaller PRs.

Let me try to reproduce again so I don't send you down the wrong path

jhrozek · 2024-05-14T12:20:58Z

I think there are two issues: The evaluator has hardcoded default configuration which should include also the two new attributes and additionally it seems like the condition when evaluating the scores are reversed - shouldn't we check that the score of the package is higher than the config? Check out the diff below:

diff --git a/internal/engine/eval/trusty/config.go b/internal/engine/eval/trusty/config.go
index 8b673b494..534427641 100644
--- a/internal/engine/eval/trusty/config.go
+++ b/internal/engine/eval/trusty/config.go
@@ -60,16 +60,22 @@ func defaultConfig() *config {
 		Action: pr_actions.ActionSummary,
 		EcosystemConfig: []ecosystemConfig{
 			{
-				Name:  "npm",
-				Score: 5.0,
+				Name:       "npm",
+				Score:      5.0,
+				Provenance: 5.0,
+				Activity:   5.0,
 			},
 			{
-				Name:  "pypi",
-				Score: 5.0,
+				Name:       "pypi",
+				Score:      5.0,
+				Provenance: 5.0,
+				Activity:   5.0,
 			},
 			{
-				Name:  "go",
-				Score: 5.0,
+				Name:       "go",
+				Score:      5.0,
+				Provenance: 5.0,
+				Activity:   5.0,
 			},
 		},
 	}
diff --git a/internal/engine/eval/trusty/trusty.go b/internal/engine/eval/trusty/trusty.go
index 634fdb915..53b4dcf6c 100644
--- a/internal/engine/eval/trusty/trusty.go
+++ b/internal/engine/eval/trusty/trusty.go
@@ -253,14 +253,14 @@ func classifyDependency(
 		}
 	}
 
-	if ecoConfig.Score <= packageScore {
+	if ecoConfig.Score > packageScore {
 		reasons = append(reasons, TRUSTY_LOW_SCORE)
 	}
-	if ecoConfig.Provenance <= descr["provenance"].(float64) {
+	if ecoConfig.Provenance > descr["provenance"].(float64) {
 		reasons = append(reasons, TRUSTY_LOW_PROVENANCE)
 	}
 
-	if ecoConfig.Activity <= descr["activity"].(float64) {
+	if ecoConfig.Activity > descr["activity"].(float64) {
 		reasons = append(reasons, TRUSTY_LOW_ACTIVITY)
 	}
 	if len(reasons) > 0 {

This PR surfaces the trusty malicious data to the comment added by minder when the trusty evaluator inspects a PR. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

This commit adds a few unit tests to some of the new utility functions handling the trusty evaluator. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

This commit drops the configuration getter from the trusty ecosystem config. As we now have access to the individual components we can write rules on each of them independent of each other. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

This commit implements the new trusty template which exposes all score components. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

This commit adds a simple test to ensure the comment template parses correctly. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

Signed-off-by: Adolfo García Veytia (puerco) <puerco@stacklok.com>

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

puerco · 2024-05-14T15:29:03Z

Ah good catch jakub, I've flippped the logic on the constants. Although those were not being used for the output anymore they were still being weighted to classify. I've pushed a change with the flipped comparisons.

evankanderson · 2024-05-14T17:32:13Z

it seems like the condition when evaluating the scores are reversed - shouldn't we check that the score of the package is higher than the config?

Should we have a test for this? It feels like the sort of bug that we could easily accidentally introduce again.

jhrozek · 2024-05-14T19:08:03Z

it seems like the condition when evaluating the scores are reversed - shouldn't we check that the score of the package is higher than the config?

Should we have a test for this? It feels like the sort of bug that we could easily accidentally introduce again.

We do have a smoke test, but alas, no way of running it locally yet. You are right that we should do better on the unit testing front - not having proper tests is mostly my fault, we do have reasonable tests for the OSV evaluator and the current Trusty evaluator code was meant to be a "quick hack" until we can generalize the PR dependency evaluators into common code and built both OSV and trusty evaluators atop them. We just haven't prioritized that work.

jhrozek

thank you for being patient with this long review!

JAORMX · 2024-05-15T08:31:54Z

I think we can merge this now. Let's start adding tests in further PRs

puerco · 2024-05-15T18:12:35Z

Should we have a test for this? It feels like the sort of bug that we could easily accidentally introduce again.
@evankanderson as part of this improvement I've been splitting the logic into more atomic functions to make them testable and to be able to mock the whole trusty evaluator. I'll strat sending incremental PRs to add more tests

puerco requested a review from a team as a code owner May 8, 2024 05:57

puerco changed the title ~~Malicious deps~~ Surface malicious dependencies from Trusty data May 8, 2024

yrobla previously approved these changes May 8, 2024

View reviewed changes

jhrozek previously approved these changes May 8, 2024

View reviewed changes

jhrozek reviewed May 8, 2024

View reviewed changes

puerco dismissed stale reviews from jhrozek and yrobla via ad5ca38 May 9, 2024 03:18

stacklokbot reviewed May 9, 2024

View reviewed changes

puerco force-pushed the malicious-deps branch from ad5ca38 to bfa9bec Compare May 9, 2024 03:20

stacklokbot reviewed May 9, 2024

View reviewed changes

puerco mentioned this pull request May 9, 2024

Trusty PR add provenance and activity mindersec/minder-rules-and-profiles#111

Merged

puerco force-pushed the malicious-deps branch from bfa9bec to f7dd320 Compare May 10, 2024 03:31

stacklokbot reviewed May 10, 2024

View reviewed changes

jhrozek reviewed May 10, 2024

View reviewed changes

puerco force-pushed the malicious-deps branch from f7dd320 to ab6ec59 Compare May 10, 2024 18:55

stacklokbot reviewed May 10, 2024

View reviewed changes

stacklokbot reviewed May 12, 2024

View reviewed changes

puerco force-pushed the malicious-deps branch from 80cef5a to 9adc7cf Compare May 13, 2024 22:57

stacklokbot reviewed May 13, 2024

View reviewed changes

puerco force-pushed the malicious-deps branch from 9adc7cf to 318c4dd Compare May 13, 2024 23:25

stacklokbot reviewed May 13, 2024

View reviewed changes

puerco changed the title ~~Surface malicious dependencies from Trusty data~~ Improve Trusty integration May 13, 2024

puerco added 10 commits May 14, 2024 09:19

Flag malicious deps in trusty PR comment

e0dd598

This PR surfaces the trusty malicious data to the comment added by minder when the trusty evaluator inspects a PR. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

Add tests for trusty utility funcs

1f4691f

This commit adds a few unit tests to some of the new utility functions handling the trusty evaluator. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

Add support for activity and provenance scores

c6257ca

Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

Surface score components and malcs deetz

2102f52

Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

Expose all trusty data with new tamplate

98c640b

This commit implements the new trusty template which exposes all score components. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

Drop noop alerts

e7e1b09

Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

Trusty: Add simple test for PR template

2035325

This commit adds a simple test to ensure the comment template parses correctly. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@stacklok.com>

Surface Deprecated and Archived packages in PR

edc029c

Signed-off-by: Adolfo García Veytia (puerco) <puerco@stacklok.com>

Flip blocking logic and base it on defaults

eeb2ada

Signed-off-by: Adolfo García Veytia (puerco) <puerco@stacklok.com>

puerco force-pushed the malicious-deps branch from 318c4dd to eeb2ada Compare May 14, 2024 15:26

stacklokbot reviewed May 14, 2024

View reviewed changes

jhrozek approved these changes May 14, 2024

View reviewed changes

JAORMX merged commit f1e2219 into mindersec:main May 15, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Trusty integration #3277

Improve Trusty integration #3277

puerco commented May 8, 2024

coveralls commented May 8, 2024 •

edited

Loading

jhrozek left a comment

jhrozek May 8, 2024

puerco May 9, 2024 •

edited

Loading

lukehinds commented May 8, 2024

stacklokbot left a comment

stacklokbot left a comment

stacklokbot left a comment

puerco commented May 9, 2024

stacklokbot left a comment

stacklokbot left a comment

jhrozek commented May 10, 2024

jhrozek May 10, 2024

puerco May 10, 2024

puerco May 14, 2024

stacklokbot left a comment

stacklokbot left a comment

stacklokbot left a comment

stacklokbot left a comment

stacklokbot left a comment

stacklokbot left a comment

stacklokbot left a comment

stacklokbot left a comment

puerco commented May 13, 2024

jhrozek commented May 14, 2024

jhrozek commented May 14, 2024

stacklokbot left a comment

stacklokbot left a comment

puerco commented May 14, 2024

evankanderson commented May 14, 2024

jhrozek commented May 14, 2024

jhrozek left a comment

JAORMX commented May 15, 2024

puerco commented May 15, 2024

Improve Trusty integration #3277

Improve Trusty integration #3277

Conversation

puerco commented May 8, 2024

Summary

Change Type

Testing

Review Checklist:

coveralls commented May 8, 2024 • edited Loading

jhrozek left a comment

Choose a reason for hiding this comment

jhrozek May 8, 2024

Choose a reason for hiding this comment

puerco May 9, 2024 • edited Loading

Choose a reason for hiding this comment

lukehinds commented May 8, 2024

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Invisible Unicode Characters Detected.

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Invisible Unicode Characters Detected.

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Mixed Scripts Detected.

puerco commented May 9, 2024

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Invisible Unicode Characters Detected.

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Mixed Scripts Detected.

jhrozek commented May 10, 2024

jhrozek May 10, 2024

Choose a reason for hiding this comment

puerco May 10, 2024

Choose a reason for hiding this comment

puerco May 14, 2024

Choose a reason for hiding this comment

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Invisible Unicode Characters Detected.

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Mixed Scripts Detected.

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Invisible Unicode Characters Detected.

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Mixed Scripts Detected.

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Invisible Unicode Characters Detected.

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Mixed Scripts Detected.

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Invisible Unicode Characters Detected.

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Mixed Scripts Detected.

puerco commented May 13, 2024

jhrozek commented May 14, 2024

jhrozek commented May 14, 2024

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Invisible Unicode Characters Detected.

stacklokbot left a comment

Choose a reason for hiding this comment

✅ No Mixed Scripts Detected.

puerco commented May 14, 2024

evankanderson commented May 14, 2024

jhrozek commented May 14, 2024

jhrozek left a comment

Choose a reason for hiding this comment

JAORMX commented May 15, 2024

puerco commented May 15, 2024

coveralls commented May 8, 2024 •

edited

Loading

puerco May 9, 2024 •

edited

Loading