Feature: Improve test coverage for scorecard-action

[laurentsimon]

Things we should be able to test:

=== Unit tests: P0 =====

1. private repos
2. different trigger events: push, schedule, pull_request, workflow_dispatch, etc. Priority should be for push, schedule and pull request
3. different branches: we only support the default branch for push and schedule
4. forks (not supported), should be added to documentation Update docs #80
5. this problem Default branch is null for event of type schedule #106 (comment)
6. Capture the different threshold scores, e.g. Align the CII-Best-Practices requirements with the documentation #129

The entrypoint of the action is entrypoint.sh. It expects some variables to be set, see https://github.com/ossf/scorecard-action/blob/main/entrypoint.sh#L25. These variables are the one we need to change for each unit test.

The shell script is part of a dockerfile: the dockefile defines the GitHub action, so we can build an image and call it with different parameters to create our unit tests.

The file $GITHUB_EVENT_PATH should contain all the data we want, except for a GitHub API bug, see https://github.com/ossf/scorecard-action/blob/main/entrypoint.sh#L38. his means we may need to create severals repos for testing purposes until the issue is resolved.
#75 (review)

Here is an example of tests done by a contributor who filed an issue #75 (review)

=== Unit tests: P1 =====

1. Feature: add unit test for getEnabledChecks() function in cmd/root.go scorecard#1196 should test that command line does not change; and that the same checks are enabled with the same user's input

=== End-to-end tests: P2 ====
Run the workflow/upload the SARIF results to a repo and retrieve the results via an API. Verify the results shown are as expected. This is low priority, I think, because we already test SARIF generation in scorecard itself.

cc @azeemshaikh38 @naveensrinivasan anything missing?

[naveensrinivasan]

Discussed offline with @laurentsimon, Instead of writing tests for the shell script. I am proposing we move the shell script into a golang code, which would be easier to maintain and specifically write tests.

[azeemshaikh38]

@naveensrinivasan @laurentsimon fyi I have setup an e2e example test using ossf-tests/scorecard-check-binary-artifacts-e2e-4-binaries#1. Also, update the schedule frequency to run everyday - link. We need similar setups on different repo types to cover our bases such that every commit to scorecard-action gets tested.

PS: a CloudBuild trigger will automatically build and push a Docker image on every commit - gcr.io/openssf/scorecard-action.

[laurentsimon]

Thanks Azeem. How to we monitor the results of the runs?

[azeemshaikh38]

Ah, good point hadn't thought about that. Maybe we can choose to get notified about workflow run failures using this? If possible setup a Slack channel to get notified about such failures (we already do this for Scorecard cron job).

[laurentsimon]

I tend to look at more emails more than at slack. Which option do you think is better? Subscribing/watching the repos? Or something else?

[azeemshaikh38]

Email/Slack either of them works for me. Slack might be better (even though I don't look at Slack too often) for a few reasons:

• there is a dedicated channel and it's hard for it to get lost with other noise.
• folks can easily choose to join or leave a Slack channel so only interested parties get notified.
• since we don't need a quick turnaround time on failures, even if we notice the failures after a few days, that's ok since we only care about not releasing a broken build.

But again, either option works for me as long as we have a way to get alerted.

Watching the repos option is not scalable. We need a way to ensure that all failures get reported in a single place.

[laurentsimon]

let's try Slack then. Agreed on turn around.
I also like the idea of having other folks watch for problems if they want to :-) or even use a bot to mine the data in the future, aggregating all failures in one place.

[naveensrinivasan]

let's try Slack then. Agreed on turn around.
I also like the idea of having other folks watch for problems if they want to :-) or even use a bot to mine the data in the future, aggregating all failures in one place.

I agree, Slack works for everyone!

[laurentsimon]

fyi, I don't think we even need the docker image https://github.com/ossf-tests/scorecard-check-binary-artifacts-e2e-4-binaries/blob/28bd7c492bb963117b9476a506aca5872614750b/.github/workflows/scorecards.yml#L30, we could simply pin the action by @main directly from the action's repo.

I suppose the use of @main may make things harder to debug if they fail? I think it'd still be ok since the run's log contain the commit hash that was used... but probably cleaner with the docker image.

[azeemshaikh38]

Ah, I didn't realize we could do that. I understand docker better so I went with that :) Ok to move to @main if you are sure about its behavior.

[laurentsimon]

yes I think it should work. I use @branch-test when testing my PRs.

[azeemshaikh38]

SG, let's use @main then.

[azeemshaikh38]

I'm updating the scope of the issue here. We need these e2e tests in place before we can safely rollout the Golang scorecard-action.

@justaugustus @rohankh532 fyi.

[justaugustus]

@laurentsimon @azeemshaikh38 @naveensrinivasan -- Would you mind updating the issue description into the following sections:

• feature parity w/ entrypoint.sh
• new functionality

We should descope the Golang-based rollout to expressly the things that are required for feature parity with the current entrypoint and then take new functionality as follow-up actions.

[naveensrinivasan]

Yes, I was investigating into feature parity and realized one of the things missing/removed while refactoring.

scorecard-action/entrypoint.sh

Lines 36 to 53 in 56e3359

	## ============================== WARNING ======================================
	# https://docs.github.com/en/actions/learn-github-actions/environment-variables
	# export SCORECARD_PRIVATE_REPOSITORY="$(jq '.repository.private' $GITHUB_EVENT_PATH)"
	# export SCORECARD_DEFAULT_BRANCH="refs/heads/$(jq -r '.repository.default_branch' $GITHUB_EVENT_PATH)"
	#
	# The $GITHUB_EVENT_PATH file produces:
	# private: null
	# default_branch: null
	#
	# for trigger event `schedule`. This is a bug.
	# So instead we use the REST API to retrieve the data.
	#
	# Boolean inputs are strings https://github.com/actions/runner/issues/1483.
	# ===============================================================================
	curl -s -H "Authorization: Bearer $GITHUB_AUTH_TOKEN" https://api.github.com/repos/$GITHUB_REPOSITORY > repo_info.json
	export SCORECARD_PRIVATE_REPOSITORY="$(cat repo_info.json | jq -r '.private')"
	export SCORECARD_DEFAULT_BRANCH="refs/heads/$(cat repo_info.json | jq -r '.default_branch')"
	export SCORECARD_IS_FORK="$(cat repo_info.json | jq -r '.fork')"

I will take this up.

[justaugustus]

Another thing I'm thinking about but haven't handled is the event path.
When the action is running in production, it's already going to have a well-formed event path, so really the only time we need the env set is when we're attempting to mock a GitHub Action env.

I say that to say:
I think the way we're handling the environment intersperses test code w/ production.

For example:

❯ docker run -e GITHUB_REF=refs/heads/main \
           -e GITHUB_EVENT_NAME=branch_protection_rule \
           -e INPUT_RESULTS_FORMAT=sarif \
           -e INPUT_RESULTS_FILE=results.sarif \
           -e GITHUB_WORKSPACE=/ \
           -e INPUT_POLICY_FILE="/policy.yml" \
           -e INPUT_REPO_TOKEN=$(cat ~/.github-token) \
           -e GITHUB_REPOSITORY="kubernetes-sigs/release-sdk" \
           -e INPUT_PUBLISH_RESULTS="false" \
           -e GITHUB_EVENT_PATH="/repo.json" \
           -e SCORECARD_BIN="/scorecard-action" \
∙          scorecard-action
Event file: /repo.json
Event name: branch_protection_rule
Ref: refs/heads/main
Repository: kubernetes-sigs/release-sdk
Fork repository: false
Private repository: false
Publication enabled: false
Format: sarif
Policy file: /policy.yml
Default branch: refs/heads/main

{
  "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
  "version": "2.1.0",
  "runs": [
    {
      "automationDetails": {
        "id": "supply-chain/branch-protection/unknown-09 Mar 22 02:34 +0000"
      },
      "tool": {
        "driver": {
          "name": "Scorecard",
          "informationUri": "https://github.com/ossf/scorecard",
          "semanticVersion": "unknown",
          "rules": [
            {
              "id": "BranchProtectionID",
              "name": "Branch-Protection",
              "helpUri": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#branch-protection",
              "shortDescription": {
                "text": "Branch-Protection"
              },
              "fullDescription": {
                "text": "Determines if the default and release branches are protected with GitHub's branch protection settings."
              },
              "help": {
                "text": "Determines if the default and release branches are protected with GitHub's branch protection settings.",
                "markdown": "**Remediation (click \"Show more\" below)**:\n\n- Enable branch protection settings in your source hosting provider to avoid force pushes or deletion of your important branches.\n\n- For GitHub, check out the steps [here](https://docs.github.com/en/github/administering-a-repository/managing-a-branch-protection-rule).\n\n\n\n**Severity**: High\n\n\n\n**Details**:\n\nRisk: `High` (vulnerable to intentional malicious code injection)  \n\n\n\nThis check determines whether a project's default and release branches are\n\nprotected with GitHub's [branch protection](https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/about-protected-branches) settings. \n\nBranch protection allows maintainers to define rules that enforce\n\ncertain workflows for branches, such as requiring review or passing certain\n\nstatus checks before acceptance into a main branch, or preventing rewriting of\n\npublic history.\n\n\n\nNote: The following settings queried by the Branch-Protection check require an admin token: `DismissStaleReviews`, `EnforceAdmin`, and `StrictStatusCheck`. If\n\nthe provided token does not have admin access, the check will query the branch\n\nsettings accessible to non-admins and provide results based only on these settings.\n\nEven so, we recommend using a non-admin token, which provides a thorough enough\n\nresult to meet most user needs. \n\n\n\nDifferent types of branch protection protect against different risks:\n\n\n\n  - Require code review: requires at least one reviewer, which greatly\n\n    reduces the risk that a compromised contributor can inject malicious code.\n\n    Review also increases the likelihood that an unintentional vulnerability in\n\n    a contribution will be detected and fixed before the change is accepted.\n\n\n\n  - Prevent force push: prevents use of the `--force` command on public\n\n    branches, which overwrites code irrevocably. This protection prevents the\n\n    rewriting of public history without external notice.\n\n\n\n  - Require [status checks](https://docs.github.com/en/github/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/about-status-checks):\n\n    ensures that all required CI tests are met before a change is accepted. \n\n\n\nAlthough requiring code review can greatly reduce the chance that\n\nunintentional or malicious code enters the \"main\" branch, it is not feasible for\n\nall projects, such as those that don't have many active participants. For more\n\ndiscussion, see [Code Reviews](https://github.com/ossf/scorecard/blob/main/docs/checks.md#code-reviews).\n\n\n\nAdditionally, in some cases these rules will need to be suspended. For example,\n\nif a past commit includes illegal content such as child pornography, it may be\n\nnecessary to use a force push to rewrite the history rather than simply hide the\n\ncommit. \n\n\n\nThis test has tiered scoring. Each tier must be fully satisfied to achieve points at the next tier. For example, if you fulfill the Tier 3 checks but do not fulfill all the Tier 2 checks, you will not receive any points for Tier 3.\n\n\n\nNote: If Scorecard is run without an administrative access token, the requirements that specify “For administrators” are ignored.\n\n\n\nTier 1 Requirements (3/10 points):\n\n  - Prevent force push\n\n  - Prevent branch deletion\n\n  - For administrators: Include administrator for review\n\n\n\nTier 2 Requirements (6/10 points):\n\n  - Required reviewers >=1 \n\n  - For administrators: Strict status checks (require branches to be up-to-date before merging)\n\n\n\nTier 3 Requirements (8/10 points):\n\n  - Status checks defined\n\n\n\nTier 4 Requirements (9/10 points):\n\n  - Required reviewers >= 2\n\n\n\nTier 5 Requirements (10/10 points):\n\n  - For administrators: Dismiss stale reviews\n\n"
              },
              "defaultConfiguration": {
                "level": "error"
              },
              "properties": {
                "precision": "high",
                "problem.severity": "error",
                "security-severity": "7.0",
                "tags": [
                  "supply-chain",
                  "security",
                  "source-code",
                  "code-reviews"
                ]
              }
            }
          ]
        }
      },
      "results": [
        {
          "ruleId": "BranchProtectionID",
          "ruleIndex": 0,
          "message": {
            "text": "score is 2: branch protection is not maximal on development and all release branches:\nWarn: settings do not apply to administrators on branch 'main'\nWarn: status checks do not require up-to-date branches for 'main'\nWarn: number of required reviewers is 0 on branch 'main'\nWarn: Stale review dismissal disabled on branch 'main'\nClick Remediation section below to solve this issue"
          },
          "locations": [
            {
              "physicalLocation": {
                "region": {
                  "startLine": 1
                },
                "artifactLocation": {
                  "uri": "no file associated with this alert",
                  "uriBaseId": "%SRCROOT%"
                }
              }
            }
          ]
        }
      ]
    }
  ]
}

This is a contrived example based on

scorecard-action/Dockerfile

Lines 15 to 23 in 56e3359

	# Testing: docker run -e GITHUB_REF=refs/heads/main \
	# -e GITHUB_EVENT_NAME=branch_protection_rule \
	# -e INPUT_RESULTS_FORMAT=sarif \
	# -e INPUT_RESULTS_FILE=results.sarif \
	# -e GITHUB_WORKSPACE=/ \
	# -e INPUT_POLICY_FILE="/policy.yml" \
	# -e INPUT_REPO_TOKEN=$GITHUB_AUTH_TOKEN \
	# -e GITHUB_REPOSITORY="ossf/scorecard" \
	# laurentsimon/scorecard-action:latest

Of note are the following things:

           -e GITHUB_REPOSITORY="kubernetes-sigs/release-sdk" \
           -e INPUT_PUBLISH_RESULTS="false" \
           -e GITHUB_EVENT_PATH="/repo.json" \
           -e SCORECARD_BIN="/scorecard-action" \

GITHUB_EVENT_PATH was non-fork.json copied into the container and as a result GITHUB_REPOSITORY was not respected (which makes sense, but isn't obvious).

Just braindumping at the moment...

[justaugustus]

I will take this up.

@naveensrinivasan -- Mind if I continue working on it while you list some of the expectations in the issue description?

I've got some local work already and between that and #120, I'm worried that all three of us will work on the same thing and do it in a slightly different way.

[naveensrinivasan]

I will take this up.

@naveensrinivasan -- Mind if I continue working on it while you list some of the expectations in the issue description?

I've got some local work already and between that and #120, I'm worried that all three of us will work on the same thing and do it in a slightly different way.

Sounds good to me. I can start investigating missing features. Would that work?

[justaugustus]

Sounds good to me. I can start investigating missing features. Would that work?

Perfect! Appreciate it, Naveen!

[naveensrinivasan]

Unit tests

1. Use the GitHub API to address this issue

1. • 🐛 Fix null default branch/private fields #75 (review)
2. • this problem Default branch is null for event of type schedule #106 (comment)

This is something @justaugustus is implementing.

2. We only support the default branch for push and schedule

This now only checks for pull_request. Do we need to check for schedule?

• Unit tests missing for this.

scorecard-action/options/options.go

Lines 154 to 160 in 56e3359

	if strings.Contains(os.Getenv(o.GithubEventName), "pull_request") &&
	os.Getenv(o.GithubRef) == o.DefaultBranch {
	fmt.Printf("%s not supported with %s event.\n", os.Getenv(o.GithubRef), os.Getenv(o.GithubEventName))
	fmt.Printf("Only the default branch %s is supported.\n", o.DefaultBranch)
	return errOnlyDefaultBranchSupported
	}

3. private repos

• What do we need to test here?

4. different trigger events: push, schedule, pull_request, workflow_dispatch, etc. Priority should be for push, schedule and pull request

• What do we need to test here?

@laurentsimon Could you please create a separate issue for each one of these and add some details so that it is easier to track?

e2e tests

What would need to be covered in the e2e? We would need to have specifics. Having a separate issue would be helpful with specifics.

[azeemshaikh38]

So I have created a label golang action rollout to track this work. I have split it out into 3 issues:

• Feature: Improve test coverage for scorecard-action #79 (this one) - to setup unit tests and e2e test infrastructure.
• Feature: Feature-parity of Golang action  #132 - to achieve feature parity of Golang Action with current bash action.
• Feature: Scorecard badges #133 - to track work on Scorecard badges

Hope that helps.

[azeemshaikh38]

@naveensrinivasan for your question on e2e tests:

We need to setup repos (or use existing ones) in ossf-tests org for varying scenarios (different trigger conditions, private repos, non-main default branch name etc.) and implement #79 (comment) so that failures result in notification on a Slack channel.

[laurentsimon]

@naveensrinivasan does @azeemshaikh38 reply answer the question?

[naveensrinivasan]

@naveensrinivasan does @azeemshaikh38 reply answer the question?

Yes.

[naveensrinivasan]

#142

[naveensrinivasan]

Closing it as it has been addressed.