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

Duplicate Rule Error for Sarif File #1076

Closed
akashsinghal opened this issue Jan 21, 2023 · 9 comments
Closed

Duplicate Rule Error for Sarif File #1076

akashsinghal opened this issue Jan 21, 2023 · 9 comments

Comments

@akashsinghal
Copy link

I've set up the ossf/scorecard-action to run for a project I'm working on. We started seeing errors from the action saying the outputted Sarif file is not valid (see below):

Error: Unable to upload "results.sarif" as it is not valid SARIF:
- instance.runs[2].tool.driver.rules contains duplicate item
Error: Unable to upload "results.sarif" as it is not valid SARIF:
- instance.runs[2].tool.driver.rules contains duplicate item
    at validateSarifFileSchema (/home/runner/work/_actions/github/codeql-action/a34ca99b4610d924e04c68db79e503e1f79f9f02/lib/upload-lib.js:189:15)
    at uploadFiles (/home/runner/work/_actions/github/codeql-action/a34ca99b4610d924e04c68db79e503e1f79f9f02/lib/upload-lib.js:238:9)
    at Object.uploadFromActions (/home/runner/work/_actions/github/codeql-action/a34ca99b4610d924e04c68db79e503e1f79f9f02/lib/upload-lib.js:132:18)
    at async run (/home/runner/work/_actions/github/codeql-action/a34ca99b4610d924e04c68db79e503e1f79f9f02/lib/upload-sarif-action.js:46:30)
    at async runWrapper (/home/runner/work/_actions/github/codeql-action/a34ca99b4610d924e04c68db79e503e1f79f9f02/lib/upload-sarif-action.js:68:9)

The workflow file we use is almost identical to the sample in the documentation: Here is the workflow file we use:

name: Scorecards supply-chain security
on:
  branch_protection_rule:
  schedule:
    # Weekly on Saturdays.
    - cron: '30 1 * * 6'
  push:
    branches: [ main ]
  workflow_dispatch:

permissions: read-all

jobs:
  analysis:
    name: Scorecards analysis
    runs-on: ubuntu-latest
    permissions:
      security-events: write
      id-token: write
      actions: read
      contents: read
    
    steps:
      - name: "Checkout code"
        uses: actions/checkout@93ea575cb5d8a053eaa0ac8fa3b40d7e05a33cc8 # tag=3.0.2
        with:
          persist-credentials: false

      - name: "Run analysis"
        uses: ossf/scorecard-action@e38b1902ae4f44df626f11ba0734b14fb91f8f86 # tag=v2.0.4
        with:
          results_file: results.sarif
          results_format: sarif
          repo_token: ${{ secrets.SCORECARD_READ_TOKEN }}
          publish_results: true

      - name: "Upload artifact"
        uses: actions/upload-artifact@3cea5372237819ed00197afe530f5a7ea3e805c8 # tag=v3.1.0
        with:
          name: SARIF file
          path: results.sarif
          retention-days: 5
      
      - name: "Upload to code-scanning"
        uses: github/codeql-action/upload-sarif@515828d97454b8354517688ddc5b48402b723750 # tag=v2.1.38
        with:
          sarif_file: results.sarif

Could I get some guidance on what the issue might be? As far as I understand, the rules are default and come preset with the OSSF action?

@akashsinghal
Copy link
Author

The duplicate rule is the Code-Review

{
                     "id": "CodeReviewID",
                     "name": "Code-Review",
                     "helpUri": "https://github.com/ossf/scorecard/blob/376f465c111c39c6a5ad7408e8896cd790cb5219/docs/checks.md#code-review",
                     "shortDescription": {
                        "text": "Code-Review"
                     },
                     "fullDescription": {
                        "text": "Determines if the project requires code review before pull requests (aka merge requests) are merged."
                     },
                     "help": {
                        "text": "Determines if the project requires code review before pull requests (aka merge requests) are merged.",
                        "markdown": "**Remediation (click \"Show more\" below)**:\n\n- If the project has only one contributor, or does not have enough reviewers to practically require that all contributions be reviewed, try to recruit more maintainers to the project who will be willing to review others' work. Ideally at least some of these people will be from different organizations (see [Contributors](checks.md#contributors)). If the project has very limited utility, consider expanding its intended utility so more people will be interested in improving it, and make that larger scope clear to potential contributors.\n\n- Follow security best practices by performing strict code reviews for every new pull request / merge request.\n\n- Make \"code reviews\" mandatory in your repository configuration. ([Instructions for GitHub.](https://docs.github.com/en/github/administering-a-repository/about-protected-branches#require-pull-request-reviews-before-merging))\n\n- Enforce the rule for administrators / code owners as well. ([Instructions for GitHub.](https://docs.github.com/en/github/administering-a-repository/about-protected-branches#include-administrators))\n\n\n\n**Severity**: High\n\n\n\n**Details**:\n\nRisk: `High` (unintentional vulnerabilities or possible injection of malicious\n\ncode)\n\n\n\nThis check determines whether the project requires code review before pull\n\nrequests (merge requests) are merged.\n\n\n\nReviews detect various unintentional problems, including vulnerabilities that\n\ncan be fixed immediately before they are merged, which improves the quality of\n\nthe code. Reviews may also detect or deter an attacker trying to insert\n\nmalicious code (either as a malicious contributor or as an attacker who has\n\nsubverted a contributor's account), because a reviewer might detect the\n\nsubversion.\n\n\n\nThe check first tries to detect whether [Branch-Protection](checks.md#branch-protection) is enabled on the\n\ndefault branch with at least one required reviewer. If this fails, the check\n\ndetermines whether the most recent (~30) commits have a Github-approved review\n\nor if the merger is different from the committer (implicit review). It also\n\nperforms a similar check for reviews using\n\n[Prow](https://github.com/kubernetes/test-infra/tree/master/prow#readme) (labels\n\n\"lgtm\" or \"approved\") and [Gerrit](https://www.gerritcodereview.com/) (\"Reviewed-on\" and \"Reviewed-by\").\n\n\n\nNote: Requiring reviews for all changes is infeasible for some projects, such as\n\nthose with only one active participant. Even a project with multiple active\n\ncontributors may not have enough active participation to be able to require\n\nreview of all proposed changes. Projects with a small number of active\n\nparticipants instead sometimes aim for a review of a\n\npercentage of proposals (e.g., \"at least half of all proposed changes are\n\nreviewed\").\n\n\n\nRequiring review does not eliminate all risks. The other reviewers might fail to\n\nnotice unintentional vulnerabilities or malicious code, be colluding with a\n\nmalicious developer, or even be the same person (using a \"[sock\n\npuppet](https://en.wikipedia.org/wiki/Sock_puppet_account)\" account).\n\n"
                     },
                     "defaultConfiguration": {
                        "level": "error"
                     },
                     "properties": {
                        "precision": "high",
                        "problem.severity": "error",
                        "security-severity": "7.0",
                        "tags": [
                           "supply-chain",
                           "security",
                           "source-code",
                           "code-reviews"
                        ]
                     }
                  },
                  {
                     "id": "CodeReviewID",
                     "name": "Code-Review",
                     "helpUri": "https://github.com/ossf/scorecard/blob/376f465c111c39c6a5ad7408e8896cd790cb5219/docs/checks.md#code-review",
                     "shortDescription": {
                        "text": "Code-Review"
                     },
                     "fullDescription": {
                        "text": "Determines if the project requires code review before pull requests (aka merge requests) are merged."
                     },
                     "help": {
                        "text": "Determines if the project requires code review before pull requests (aka merge requests) are merged.",
                        "markdown": "**Remediation (click \"Show more\" below)**:\n\n- If the project has only one contributor, or does not have enough reviewers to practically require that all contributions be reviewed, try to recruit more maintainers to the project who will be willing to review others' work. Ideally at least some of these people will be from different organizations (see [Contributors](checks.md#contributors)). If the project has very limited utility, consider expanding its intended utility so more people will be interested in improving it, and make that larger scope clear to potential contributors.\n\n- Follow security best practices by performing strict code reviews for every new pull request / merge request.\n\n- Make \"code reviews\" mandatory in your repository configuration. ([Instructions for GitHub.](https://docs.github.com/en/github/administering-a-repository/about-protected-branches#require-pull-request-reviews-before-merging))\n\n- Enforce the rule for administrators / code owners as well. ([Instructions for GitHub.](https://docs.github.com/en/github/administering-a-repository/about-protected-branches#include-administrators))\n\n\n\n**Severity**: High\n\n\n\n**Details**:\n\nRisk: `High` (unintentional vulnerabilities or possible injection of malicious\n\ncode)\n\n\n\nThis check determines whether the project requires code review before pull\n\nrequests (merge requests) are merged.\n\n\n\nReviews detect various unintentional problems, including vulnerabilities that\n\ncan be fixed immediately before they are merged, which improves the quality of\n\nthe code. Reviews may also detect or deter an attacker trying to insert\n\nmalicious code (either as a malicious contributor or as an attacker who has\n\nsubverted a contributor's account), because a reviewer might detect the\n\nsubversion.\n\n\n\nThe check first tries to detect whether [Branch-Protection](checks.md#branch-protection) is enabled on the\n\ndefault branch with at least one required reviewer. If this fails, the check\n\ndetermines whether the most recent (~30) commits have a Github-approved review\n\nor if the merger is different from the committer (implicit review). It also\n\nperforms a similar check for reviews using\n\n[Prow](https://github.com/kubernetes/test-infra/tree/master/prow#readme) (labels\n\n\"lgtm\" or \"approved\") and [Gerrit](https://www.gerritcodereview.com/) (\"Reviewed-on\" and \"Reviewed-by\").\n\n\n\nNote: Requiring reviews for all changes is infeasible for some projects, such as\n\nthose with only one active participant. Even a project with multiple active\n\ncontributors may not have enough active participation to be able to require\n\nreview of all proposed changes. Projects with a small number of active\n\nparticipants instead sometimes aim for a review of a\n\npercentage of proposals (e.g., \"at least half of all proposed changes are\n\nreviewed\").\n\n\n\nRequiring review does not eliminate all risks. The other reviewers might fail to\n\nnotice unintentional vulnerabilities or malicious code, be colluding with a\n\nmalicious developer, or even be the same person (using a \"[sock\n\npuppet](https://en.wikipedia.org/wiki/Sock_puppet_account)\" account).\n\n"
                     },
                     "defaultConfiguration": {
                        "level": "error"
                     },
                     "properties": {
                        "precision": "high",
                        "problem.severity": "error",
                        "security-severity": "7.0",
                        "tags": [
                           "supply-chain",
                           "security",
                           "source-code",
                           "code-reviews"
                        ]
                     }
                  },

@akashsinghal akashsinghal changed the title Duplicate Ruler Error for Sarif File Duplicate Rule Error for Sarif File Jan 21, 2023
@laurentsimon
Copy link
Contributor

mhh, that's interesting, we've never seen this error before. Can you link to a run of the Action to help us see ur logs?

@akashsinghal
Copy link
Author

@laurentsimon
Copy link
Contributor

laurentsimon commented Jan 26, 2023

So you're using the latest version, which is good. I was not able to reproduce on my end. From the logs, it seems that the CITestsID is absent, and CodeReviewID appears twice. Other repos that use the same version of the Action run just fine, so something else is going on.

Would you be able to add the following workflow to your repo and trigger it manually: https://github.com/laurentsimon/scorecard-action-test-2/blob/main/.github/workflows/test.yml

I'd like to see the logs for anything suspicious. If we're at least able to reproduce the SARIF problems but not find the root cause, I'l make a build that logs more. Will take a bit of fiddling around.

If you have another repo where you can reproduce the problem, I'd use that instead and I could troubleshoot in the repo instead.

@akashsinghal
Copy link
Author

akashsinghal commented Jan 26, 2023

Interestingly, this problem doesn't occur for forked repos with the exact same workflow files. Unfortunately, we have stricter governance standards for merging to our main branch and updating the workflow might be challenging. I want to try running it on the non default branch but the current workflow we have doesn't seem to allow it? Is there a specific setting to enable to run on a different branch?

@laurentsimon
Copy link
Contributor

I think you may need to commit it to the branch you control and use "on: push" trigger. Then push to the branch. Would that work?

@akashsinghal
Copy link
Author

I was able to create a separate branch and updated the existing workflow file to run on Pull Request trigger. (from the scorecard code, it seems like it will only run if it's not default branch or its a pull request). the workflow was successful from the new branch. So it seems to be isolated to the main branch: https://github.com/deislabs/ratify/actions/runs/4019713495

@laurentsimon
Copy link
Contributor

I see. I need to be able to reproduce the problem to troubleshoot it. Any other ideas?
Anything special about your repo?

@akashsinghal
Copy link
Author

Thanks for taking a look. I'm wondering if there's some special setting/rule configured for our default main branch which is causing a conflict. I'll reach out to an owner to view the rules and update here with any more questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants