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

Add basic support for PR vulnerability scanning #899

Merged
merged 10 commits into from
Sep 8, 2023
Merged

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Sep 8, 2023

This PR adds support for scanning pull requests for vulnerabilities. The
idea and PoC implementation was Luke's, I took his PoC code and wrapped it in
mediator's policy engine.

At the moment, only NPM is supported as far as package ecosystems go and
only OSV is supported as the package database but the rules are designed
to be pluggable.

Note that I didn't have much (any) time to read the code and prettify it after I had written the code.
I'm submitting the PR so that it can be useful for a mediator demo in the
near future. There are also almost no tests and several TODOs sprinkled
around, but the code as-is works and can be used to show the feature.

To test the feature, create the appropriate rule_type and the rule (see
examples/github/policies/pr_vuln_check.yaml), then enroll a repo
and open a PR with a vulnerability (see
jakubtestorg/bad-npm#5 for an example)

Fixes: #833
Fixes: #835
Fixes: #836
Fixes: #837
Related: #838 (the review comment is added, but I haven't tested the reviews past them being added..)

@jhrozek
Copy link
Contributor Author

jhrozek commented Sep 8, 2023

CC @lukehinds @JAORMX for the demo

@JAORMX
Copy link
Contributor

JAORMX commented Sep 8, 2023

Awesome, I'll test this out in the morning

@jhrozek
Copy link
Contributor Author

jhrozek commented Sep 8, 2023

Note that at the moment you need to submit the PR as a different identity that the one who enrolled repos in mediator - GH API won't let you review your own code. I'll address that in a follow up

)

// NewRuleEvaluator creates a new rule data evaluator
func NewRuleEvaluator(rt *pb.RuleType) (engif.Evaluator, error) {
func NewRuleEvaluator(rt *pb.RuleType, cli ghclient.RestAPI) (engif.Evaluator, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of this, but we can refactor it out later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

me neither, but at leat the client was already passed to the ingester, so we don't add a new parameter to the top-level policy eval, but just reuse a param that was pointed to the ingester.

internal/engine/eval/vulncheck/vulncheck.go Outdated Show resolved Hide resolved
Just defines a bunch of type in our protobuf API that would be used
later to handle pull requests.
Extens the entity handling machinery to handle pull request types and
the associated protobuf type so that we can pass around internal events
describing pull requests.
We'll need these methods later when we deal with PRs
Adds a new handler that at the moment only listens to PRs being opened,
extracts the needed information and passes that info by executing an
internal event.
…tity

Run with:
```
AUTH_TOKEN="sikrit" go run cmd/dev/main.go rule \
    --policy examples/github/policies/pr_vuln_check.yaml \
    --rule-type examples/github/rule-types/pr_vulnerability_check.yaml \
    --entity examples/github/pull_request.yaml
```
The PR diff ingester sorts out PR files into those that match a name for
a specific package ecosystem and returns a list of files for a given
ecosystem.
It was recommended during review that we keep all the policies in one
file.
@JAORMX
Copy link
Contributor

JAORMX commented Sep 8, 2023

I'm fine with merging this as is and iterating on top

@jhrozek
Copy link
Contributor Author

jhrozek commented Sep 8, 2023

I'm fine with merging this as is and iterating on top

great, thanks. btw the code in this PR should be quite self-contained (should..famous last words) so I hope it wouldn't break anything if merged. That said, @lukehinds would you prefer to land a large PR like this after Tuesday to keep main stable?

Copy link
Contributor

@lukehinds lukehinds left a comment

Choose a reason for hiding this comment

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

I am good to merge

@jhrozek jhrozek merged commit edbf3fd into mindersec:main Sep 8, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants