Trusty: Support blocking PRs through reviews (#3392)

This commit adds to minder the capability to request changes in PRs when minder finds something odd using trusty data. We now also introduce a new setting to disable blocking on malicious deps (all power to the users, but why would you want that?!). Signed-off-by: Adolfo García Veytia (puerco) <[email protected]>
mindersec · May 23, 2024 · 9ffe6bb · 9ffe6bb
1 parent 2749c31
commit 9ffe6bb
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 35 deletions.
diff --git a/internal/engine/eval/trusty/actions.go b/internal/engine/eval/trusty/actions.go
@@ -26,7 +26,10 @@ import (
 	template "text/template"
 	"unicode"
 
+	"github.com/google/go-github/v61/github"
+
 	"github.com/stacklok/minder/internal/constants"
+	"github.com/stacklok/minder/internal/engine/eval/pr_actions"
 	pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
 	provifv1 "github.com/stacklok/minder/pkg/providers/v1"
 )
@@ -140,7 +143,12 @@ type dependencyAlternatives struct {
 	Dependency *pb.Dependency
 
 	// Reason captures the reason why a package was flagged
-	Reasons     []RuleViolationReason
+	Reasons []RuleViolationReason
+
+	// BlockPR will cause the PR to be blocked as requesting changes when true
+	BlockPR bool
+
+	// trustyReply is the complete response from trusty for this package
 	trustyReply *Reply
 }
 
@@ -154,19 +162,11 @@ type summaryPrHandler struct {
 	commentTemplate     *template.Template
 }
 
-func (sph *summaryPrHandler) trackAlternatives(
-	dep *pb.PrDependencies_ContextualDependency,
-	violationReasons []RuleViolationReason,
-	trustyReply *Reply,
-) {
-	sph.trackedAlternatives = append(sph.trackedAlternatives, dependencyAlternatives{
-		Dependency:  dep.Dep,
-		Reasons:     violationReasons,
-		trustyReply: trustyReply,
-	})
+func (sph *summaryPrHandler) trackAlternatives(dep dependencyAlternatives) {
+	sph.trackedAlternatives = append(sph.trackedAlternatives, dep)
 }
 
-func (sph *summaryPrHandler) submit(ctx context.Context) error {
+func (sph *summaryPrHandler) submit(ctx context.Context, ruleConfig *config) error {
 	if len(sph.trackedAlternatives) == 0 {
 		return nil
 	}
@@ -176,11 +176,41 @@ func (sph *summaryPrHandler) submit(ctx context.Context) error {
 		return fmt.Errorf("could not generate summary: %w", err)
 	}
 
-	_, err = sph.cli.CreateIssueComment(ctx, sph.pr.GetRepoOwner(), sph.pr.GetRepoName(), int(sph.pr.GetNumber()), summary)
-	if err != nil {
-		return fmt.Errorf("could not create comment: %w", err)
+	action := ruleConfig.Action
+
+	// Check all the tracked dependencies. If any of them call for the PR
+	// to be blocked, set the review action to REQUEST_CHANGES
+	var reviewAction string = "COMMENT"
+	for _, d := range sph.trackedAlternatives {
+		if d.BlockPR {
+			reviewAction = "REQUEST_CHANGES"
+			break
+		}
 	}
 
+	switch action {
+	case pr_actions.ActionReviewPr:
+		_, err = sph.cli.CreateReview(
+			ctx, sph.pr.GetRepoOwner(), sph.pr.GetRepoName(), int(sph.pr.GetNumber()),
+			&github.PullRequestReviewRequest{
+				NodeID:   new(string),
+				CommitID: &sph.pr.CommitSha,
+				Body:     &summary,
+				Event:    github.String(reviewAction),
+				Comments: []*github.DraftReviewComment{},
+			},
+		)
+		if err != nil {
+			return fmt.Errorf("submitting pr summary: %w", err)
+		}
+	case pr_actions.ActionSummary:
+		_, err = sph.cli.CreateIssueComment(ctx, sph.pr.GetRepoOwner(), sph.pr.GetRepoName(), int(sph.pr.GetNumber()), summary)
+		if err != nil {
+			return fmt.Errorf("could not create comment: %w", err)
+		}
+	case pr_actions.ActionComment, pr_actions.ActionCommitStatus, pr_actions.ActionProfileOnly:
+		return fmt.Errorf("pull request action not supported")
+	}
 	return nil
 }
 

diff --git a/internal/engine/eval/trusty/config.go b/internal/engine/eval/trusty/config.go
@@ -47,35 +47,41 @@ type ecosystemConfig struct {
 	// Activity is the minimal activity score that minder needs to find to
 	// consider the package as trustworthy.
 	Activity float64 `json:"activity" mapstructure:"activity"`
+
+	// AllowMalicious disables blocking PRs introducing malicious dependencies
+	AllowMalicious bool `json:"allow_malicious" mapstructure:"allow_malicious"`
 }
 
-// config is the configuration for the vulncheck evaluator
+// config is the configuration for the trusty evaluator
 type config struct {
 	Action          pr_actions.Action `json:"action" mapstructure:"action" validate:"required"`
 	EcosystemConfig []ecosystemConfig `json:"ecosystem_config" mapstructure:"ecosystem_config" validate:"required"`
 }
 
 func defaultConfig() *config {
 	return &config{
-		Action: pr_actions.ActionSummary,
+		Action: pr_actions.ActionReviewPr,
 		EcosystemConfig: []ecosystemConfig{
 			{
-				Name:       "npm",
-				Score:      5.0,
-				Provenance: 5.0,
-				Activity:   5.0,
+				Name:           "npm",
+				Score:          5.0,
+				Provenance:     5.0,
+				Activity:       5.0,
+				AllowMalicious: false,
 			},
 			{
-				Name:       "pypi",
-				Score:      5.0,
-				Provenance: 5.0,
-				Activity:   5.0,
+				Name:           "pypi",
+				Score:          5.0,
+				Provenance:     5.0,
+				Activity:       5.0,
+				AllowMalicious: false,
 			},
 			{
-				Name:       "go",
-				Score:      5.0,
-				Provenance: 5.0,
-				Activity:   5.0,
+				Name:           "go",
+				Score:          5.0,
+				Provenance:     5.0,
+				Activity:       5.0,
+				AllowMalicious: false,
 			},
 		},
 	}

diff --git a/internal/engine/eval/trusty/trusty.go b/internal/engine/eval/trusty/trusty.go
@@ -121,8 +121,8 @@ func (e *Evaluator) Eval(ctx context.Context, pol map[string]any, res *engif.Res
 		return nil
 	}
 
-	if err := submitSummary(ctx, prSummaryHandler); err != nil {
-		logger.Err(err).Msgf("Failed Generating PR Summary: %s", err.Error())
+	if err := submitSummary(ctx, prSummaryHandler, ruleConfig); err != nil {
+		logger.Err(err).Msgf("Failed generating PR summary: %s", err.Error())
 		return fmt.Errorf("submitting pull request summary: %w", err)
 	}
 
@@ -160,7 +160,7 @@ func parseRuleConfig(pol map[string]any) (*config, error) {
 		return nil, fmt.Errorf("failed to parse config: %w", err)
 	}
 
-	if ruleConfig.Action != pr_actions.ActionSummary {
+	if ruleConfig.Action != pr_actions.ActionSummary && ruleConfig.Action != pr_actions.ActionReviewPr {
 		return nil, fmt.Errorf("action %s is not implemented", ruleConfig.Action)
 	}
 
@@ -169,8 +169,8 @@ func parseRuleConfig(pol map[string]any) (*config, error) {
 
 // submitSummary submits the pull request summary. It will return an error if
 // something fails.
-func submitSummary(ctx context.Context, prSummary *summaryPrHandler) error {
-	if err := prSummary.submit(ctx); err != nil {
+func submitSummary(ctx context.Context, prSummary *summaryPrHandler, ruleConfig *config) error {
+	if err := prSummary.submit(ctx, ruleConfig); err != nil {
 		return fmt.Errorf("failed to submit summary: %w", err)
 	}
 	return nil
@@ -236,6 +236,9 @@ func classifyDependency(
 	// Check all the policy violations
 	reasons := []RuleViolationReason{}
 
+	// shouldBlockPR indicates if the PR should beblocked based on this dep
+	shouldBlockPR := false
+
 	ecoConfig := getEcosystemConfig(logger, ruleConfig, dep)
 	if ecoConfig == nil {
 		return
@@ -249,6 +252,10 @@ func classifyDependency(
 			Str("malicious", "true").
 			Msgf("malicious dependency")
 
+		if !ecoConfig.AllowMalicious {
+			shouldBlockPR = true
+		}
+
 		reasons = append(reasons, TRUSTY_MALICIOUS_PKG)
 	}
 
@@ -288,7 +295,13 @@ func classifyDependency(
 			Float64("threshold", ecoConfig.Score).
 			Msgf("the dependency has lower score than threshold or is malicious, tracking")
 
-		prSummary.trackAlternatives(dep, reasons, resp)
+		//prSummary.trackAlternatives(dep, reasons, resp)
+		prSummary.trackAlternatives(dependencyAlternatives{
+			Dependency:  dep.Dep,
+			Reasons:     reasons,
+			BlockPR:     shouldBlockPR,
+			trustyReply: resp,
+		})
 	} else {
 		logger.Debug().
 			Str("dependency", dep.Dep.Name).