Add Dependency Analysis Action and Dockerfile #1095

naveensrinivasan · 2023-02-23T23:31:55Z

New scorecard action #1070

Add workflow to publish dependency analysis Docker image
Add a new filter function to filter slices
Add a GetScorecardChecks function to get scorecard checks
Add a GetScore function to get score of a repo
Add a Validate function to validate token, owner, repo, commitSHA, and PR
Add a new action file for OSSF Scorecard dependency analysis

ossf-tests/vulpy#2 - python
ossf-tests/vulpy#1 - GitHub action
naveensrinivasan#23 - golang

Example of the GitHub action https://github.com/ossf-tests/vulpy/blob/master/.github/workflows/scorecard-dependency.yml

codecov · 2023-02-23T23:37:43Z

Codecov Report

Merging #1095 (c20c23d) into main (570a953) will decrease coverage by 23.30%.
The diff coverage is 17.66%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1095       +/-   ##
===========================================
- Coverage   62.94%   39.65%   -23.30%     
===========================================
  Files           4        5        +1     
  Lines         251      517      +266     
===========================================
+ Hits          158      205       +47     
- Misses         77      291      +214     
- Partials       16       21        +5

Impacted Files	Coverage Δ
dependency-analysis/main.go	`17.66% <17.66%> (ø)`

New scorecard action ossf#1070 - Add workflow to publish dependency analysis Docker image - Add a new filter function to filter slices - Add a GetScorecardChecks function to get scorecard checks - Add a GetScore function to get score of a repo - Add a Validate function to validate token, owner, repo, commitSHA, and PR - Add a new action file for OSSF Scorecard dependency analysis - Add structs for ScorecardResult, Check, DependencyDiff, and V Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>

- Update Apache License in `main_test.go` - Add `paralleltest` and `govet` comments - Change `_` to `//nolint:paralleltest` and `//nolint:govet` - Change `0644` to `0o644` - Remove test for invalid owner - Remove owner from `Validate` function - Remove a line from the `Vulnerability` struct [dependency-analysis/main_test.go] - Add Apache License to the file - Add `paralleltest` and `govet` comments - Change `_` to `//nolint:paralleltest` and `//nolint:govet` - Change `0644` to `0o644` - Remove test for invalid owner - Remove owner from `Validate` function [dependency-analysis/types.go] - Remove a line from the `Vulnerability` struct Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>

[dependency-analysis/README.md] - Enable the GitHub Dependency Graph API - Add a configuration input to specify the checks to run - Add a comment to the pull request with the results of the analysis - Update README with installation instructions Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>

dependency-analysis/README.md

dependency-analysis/action.yaml

.github/workflows/publish-dependency-image.yml

dependency-analysis/README.md

dependency-analysis/main.go

raghavkaul

Thanks for working on this, some suggestions for code cleanup.

Dockerfile-dependency-analysis

dependency-analysis/README.md

dependency-analysis/main.go

raghavkaul · 2023-02-27T15:49:51Z

dependency-analysis/main.go

+// GetScorecardChecks returns the list of checks to run.
+// This uses the SCORECARD_CHECKS environment variable to get the path to the checks list.
+func GetScorecardChecks() ([]string, error) {
+	fileName := os.Getenv("SCORECARD_CHECKS")


Agree that it should be parameterized for testability; also wonder if filename is the right approach? If the user is configuring the DependencyDiff action, can their string environment variable be the comma-separated list of checks instead of the name of a file with the list of checks?

- Update Dockerfile for dependency analysis - Rename GetScore function to GetScorecardResult - Update README title and link to GitHub Dependency Review documentation - Add environment variable for list of checks - Add checks for valid repo - Filter out dependencies that are not added - Filter out dependencies that do [Dockerfile-dependency-analysis] - Rename file from `Dockerfile-dependency-analysis` to `Dependency-analysis.dockerfile` [dependency-analysis/main_test.go] - Change the name of the GetScore function to GetScorecardResult - Lower the minimum score required in the test from `got.Score < tt.score` to `got.Score <= tt.score` [dependency-analysis/README.md] - Change the title of the README from `OpenSSF Dependency Analysis` to `OpenSSF Scorecard Dependency Analysis` - Change the link to the GitHub Dependency Review documentation - Change the action name to `ossf/scorecard-action/dependency-analysis@main` [dependency-analysis/main.go] - Convert the PR number to an integer - Move the `octokit` initialization to a separate file - Add an environment variable to get the list of checks - Add a check for a valid repo - Convert the PR number to an integer - Add a function to get the HTML for vulnerabilities - Add a function to get the scorecard result - Filter out dependencies that are not added - Filter out dependencies that do [.github/workflows/publish-dependency-image.yml] - Change the file name for the Dockerfile from `Dockerfile-dependency-analysis` to `Dependency-analysis.dockerfile` Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>

azeemshaikh38

Question/comment on this design: #1070 (comment)

raghavkaul

Thanks, LGTM.

naveensrinivasan · 2023-02-28T19:15:58Z

Question/comment on this design: #1070 (comment)

We are continuing the discussion on the ☝️ thread

naveensrinivasan · 2023-03-03T14:09:46Z

Question/comment on this design: #1070 (comment)

A friendly ping

laurentsimon · 2023-03-06T22:45:02Z

.github/workflows/publish-dependency-image.yml

+      packages: write
+
+    steps:
+      - name: Checkout repository


you should be able to also create provenance for it using https://github.com/slsa-framework/slsa-github-generator/tree/main/internal/builders/container

laurentsimon · 2023-03-06T22:45:58Z

dependency-analysis/README.md

+## Usage
+The OpenSSF Dependency Analysis is a GitHub Action that can be easily incorporated into a workflow. 
+The workflow can be triggered on a pull request event. 
+The action will run on the latest commit on the default branch of the repository, and will create a comment on the pull request with the results of the analysis. 


maybe clarify what this means. Default branch of which repo?

laurentsimon · 2023-03-06T22:47:14Z

dependency-analysis/README.md

+The OpenSSF Dependency Analysis is a GitHub Action that can be easily incorporated into a workflow. 
+The workflow can be triggered on a pull request event. 
+The action will run on the latest commit on the default branch of the repository, and will create a comment on the pull request with the results of the analysis. 
+An example of the comment can be found [here](https://github.com/ossf-tests/vulpy/pull/2#issuecomment-1442310469).


are we showing the aggregate score in the summary for each result? Do users need to click to see it?
Are we creating a new comment for each run?

are we showing the aggregate score in the summary for each result

We aren't showing aggregate score. Do you think we should show the aggregate score?

Are we creating a new comment for each run?

We will not create results for reach run. Only if the user manually runs it only then we create another comment.

so we run it once for the first commit, and then only if the user asks to run?
Maybe add to the comment description to explain to users that they can re-run to update?

laurentsimon · 2023-03-06T22:49:03Z

dependency-analysis/README.md

+An example of the comment can be found [here](https://github.com/ossf-tests/vulpy/pull/2#issuecomment-1442310469).
+
+## Prerequisites
+The actions require enabling the [GitHub Dependency Review](https://docs.github.com/en/code-security/supply-chain-security/understanding-your-software-supply-chain/about-dependency-review) for the repository.


Is this really true? For go projects, unlikely. For other projects, is it? We don't need an exact dependency tag, so it should not be needed. Does GitHub API return dependency name / repo without enabling this feature?

Is this really true? For go projects, unlikely. For other projects, is it? We don't need an exact dependency tag, so it should not be needed. Does GitHub API return dependency name / repo without enabling this feature?

I have tested for go projects and here is python package https://github.com/ossf-tests/vulpy/pull/2/files. For teh GH API to return results it needs this feature to be tuned on.

Very interesting... and unexpected to me

laurentsimon · 2023-03-06T22:51:28Z

dependency-analysis/README.md

+### Configuration
+The action can be configured using the following inputs:
+
+- `SCORECARD_CHECKS`: This environment variable takes a file containing a list of checks to run. 


Do we need JSON format? I could imagine a single Action input that takes a comma-separated list check1, check2, check3. Action inputs are the standard way to pass parameters into Action, no?

Do you anticipate more complicated structure needed in the future?

We can give users the option to exclude certain repositories or checks for a particular repo, which we can make possible with the file option.

This will also give us the ability to extend the feature in the future. That's the idea.

We should have a config file in this case then. Standard way to pass parameters to an Action is not env variables. It's inputs.

laurentsimon · 2023-03-06T22:56:28Z

dependency-analysis/main.go

+		log.Fatal(err)
+	}
+
+	ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token})


OAuth tokens is required to access the BQtable? Can you add a comment why it's needed?

This is GH Client access.

ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token}) tc := oauth2.NewClient(context.Background(), ts) client := github.NewClient(tc)

I think from the code we should be able to make it being used in GH Client API. Unless you think it is still necessary.

auth token must bee needed to use the GITHUB_TOKEN. That makes sense now :-)

laurentsimon · 2023-03-06T23:00:15Z

dependency-analysis/main.go

+		url := strings.TrimPrefix(k, "https://")
+		scorecard, err := GetScorecardResult(url)
+		if err != nil {
+			if len(i.Vulnerabilities) > 0 {


Why are we treating Vulns as a special case?

I worry our data is inaccurate, ie we're using a dependency HEAD whereas a user is installing a particular package version. There are other tools that will do a better job at this than Scorecard, like dependabot.

I'm thinking we may even remove the vuln results entirely because of how poor our data may be. Otherwise users will be confused and unhappy that we're claiming there are vulns in a package if there are none.

OK. I agree. I can remove the Vulns.

laurentsimon · 2023-03-06T23:01:49Z

dependency-analysis/main.go

+	if checks.Repo.Name == "" {
+		return ""
+	}
+	sb := strings.Builder{}


maybe create a template file rather than hardcoded this layout in code. It' pretty hard to read otherwise

I agree. I can do that.

laurentsimon · 2023-03-06T23:03:52Z

dependency-analysis/main.go

+	if fileName == "" {
+		// default to critical and high severity checks
+		return []string{
+			"Dangerous-Workflow",


Should we instead be returning just the aggregate score and link to deps.dev for better UX? Or at least provide a link to deps.dev for users to see more results?

We can add the aggregate score but not every repo has the deps.dev and that is the reason not to include deps.dev.

laurentsimon · 2023-03-06T23:04:41Z

dependency-analysis/main_test.go

+				t.Errorf("GetScorecardChecks() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {


nit: use go-cmp package for deep comparison

- Remove depedencydiff as we are building something similar to this ossf/scorecard-action#1095 Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>

spencerschrock · 2023-08-03T18:12:29Z

Closing as this was implemented in a separate repo instead.
https://github.com/ossf/scorecard-dependencyanalysis

naveensrinivasan requested review from laurentsimon, raghavkaul and spencerschrock February 23, 2023 23:32

naveensrinivasan force-pushed the naveen/feat/dependency-analysis branch 3 times, most recently from 32cfd28 to b8b6508 Compare February 24, 2023 18:47

naveensrinivasan force-pushed the naveen/feat/dependency-analysis branch from b8b6508 to 6998dbe Compare February 24, 2023 18:50

spencerschrock reviewed Feb 27, 2023

View reviewed changes

raghavkaul requested changes Feb 27, 2023

View reviewed changes

naveensrinivasan requested review from spencerschrock and raghavkaul February 27, 2023 18:53

naveensrinivasan force-pushed the naveen/feat/dependency-analysis branch 3 times, most recently from 5f5499b to 0d4382c Compare February 27, 2023 19:04

naveensrinivasan force-pushed the naveen/feat/dependency-analysis branch from 0d4382c to c20c23d Compare February 27, 2023 19:13

azeemshaikh38 requested changes Feb 28, 2023

View reviewed changes

raghavkaul approved these changes Feb 28, 2023

View reviewed changes

laurentsimon reviewed Mar 6, 2023

View reviewed changes

azeemshaikh38 approved these changes Mar 10, 2023

View reviewed changes

naveensrinivasan mentioned this pull request May 23, 2023

⚠️ Remove dependencydiff files and related tests ossf/scorecard#3053

Closed

2 tasks

spencerschrock closed this Aug 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Dependency Analysis Action and Dockerfile #1095

Add Dependency Analysis Action and Dockerfile #1095

naveensrinivasan commented Feb 23, 2023

codecov bot commented Feb 23, 2023 •

edited

raghavkaul left a comment

raghavkaul Feb 27, 2023

azeemshaikh38 left a comment

raghavkaul left a comment

naveensrinivasan commented Feb 28, 2023

naveensrinivasan commented Mar 3, 2023

laurentsimon Mar 6, 2023

laurentsimon Mar 6, 2023

laurentsimon Mar 6, 2023

naveensrinivasan Mar 6, 2023

laurentsimon Mar 10, 2023

laurentsimon Mar 6, 2023

naveensrinivasan Mar 6, 2023

laurentsimon Mar 7, 2023

laurentsimon Mar 6, 2023

naveensrinivasan Mar 6, 2023

laurentsimon Mar 7, 2023

laurentsimon Mar 6, 2023

naveensrinivasan Mar 7, 2023

laurentsimon Mar 7, 2023

laurentsimon Mar 6, 2023

naveensrinivasan Mar 7, 2023

laurentsimon Mar 6, 2023

naveensrinivasan Mar 7, 2023

laurentsimon Mar 6, 2023

naveensrinivasan Mar 7, 2023

laurentsimon Mar 6, 2023

spencerschrock commented Aug 3, 2023

Add Dependency Analysis Action and Dockerfile #1095

Add Dependency Analysis Action and Dockerfile #1095

Conversation

naveensrinivasan commented Feb 23, 2023

codecov bot commented Feb 23, 2023 • edited

Codecov Report

raghavkaul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azeemshaikh38 left a comment

Choose a reason for hiding this comment

raghavkaul left a comment

Choose a reason for hiding this comment

naveensrinivasan commented Feb 28, 2023

naveensrinivasan commented Mar 3, 2023

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

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

spencerschrock commented Aug 3, 2023

codecov bot commented Feb 23, 2023 •

edited