From 1a1d9b175c44e95c454d1de509499d9ea028f981 Mon Sep 17 00:00:00 2001 From: AdamKorcz <44787359+AdamKorcz@users.noreply.github.com> Date: Tue, 23 Jan 2024 19:32:59 +0000 Subject: [PATCH] :book: Add documentation about probes and contributing (#3762) * :book: Add documentation about probes and contributing Signed-off-by: Adam Korczynski * change 'subdirectory' to 'directory' Signed-off-by: Adam Korczynski * fix 'golangci' typo Signed-off-by: Adam Korczynski * Added 'make fix-linter' to Makefile Signed-off-by: Adam Korczynski * Move commands to their own table Signed-off-by: Adam Korczynski * change 'problem' to 'supply-chain security risk' Signed-off-by: Adam Korczynski * Add sentence about what a finding is Signed-off-by: Adam Korczynski * remove sentence about running make rule locally Signed-off-by: Adam Korczynski * change 'supply-chain security risk' to 'heuristic' Signed-off-by: Adam Korczynski * Modify text on where to set remediation data Signed-off-by: Adam Korczynski * Add example Signed-off-by: Adam Korczynski * add line about discussing changes to the score in a GitHub issue Signed-off-by: Adam Korczynski --------- Signed-off-by: Adam Korczynski --- CONTRIBUTING.md | 29 ++++++++++++++++++++ Makefile | 5 ++++ probes/README.md | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+) create mode 100644 probes/README.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index eeec82bd..7953aa09 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -16,7 +16,10 @@ project. This document describes the contribution guidelines for the project. * [How to build scorecard locally](#how-to-build-scorecard-locally) * [PR Process](#pr-process) * [What to do before submitting a pull request](#what-to-do-before-submitting-a-pull-request) +* [Changing Score Results](#changing-score-results) +* [Linting](#linting) * [Permission for GitHub personal access tokens](#permission-for-github-personal-access-tokens) +* [Adding New Probes](#adding-new-probes) * [Where the CI Tests are configured](#where-the-ci-tests-are-configured) * [dailyscore-cronjob](#dailyscore-cronjob) * [Deploying the cron job](#deploying-the-cron-job) @@ -126,6 +129,9 @@ assumed to match the PR. For instance, if you have a bugfix in with a breaking change, it's generally encouraged to submit the bugfix separately, but if you must put them in one PR, you should mark the whole PR as breaking. +When a maintainer reviews your code, it is generally preferred to solve each individual +review with small fixes without rebasing, so the maintainer can assess each fix separately. + ## What to do before submitting a pull request Following the targets that can be used to test your changes locally. @@ -139,6 +145,25 @@ Make sure to signoff your commits before submitting a pull request. https://docs.pi-hole.net/guides/github/how-to-signoff/ +When developing locally, the following commands are useful to run regularly to check unit tests and linting. + +| Command | Description | Is called in the CI? | +| make unit-test | Runs unit tests only. `make all` will also run this. | yes | +| make check-linter | Checks linter issues only. `make all` will also run this. | yes | + +## Changing Score Results + +As a general rule of thumb, pull requests that change Scorecard score results will need a good reason to do so to get merged. +It is a good idea to discuss such changes in a GitHub issue before implementing them. + +## Linting + +Most linter issues can be fixed with `golangci-lint` with the following command: + +``` +make fix-linter +``` + ## Permission for GitHub personal access tokens The personal access token need the following scopes: @@ -168,6 +193,10 @@ Commit the changes, and submit a PR and scorecard would start scanning in subseq See [checks/write.md](checks/write.md). When you add new checks, you need to also update the docs. +## Adding New Probes + +See [probes/README.md](probes/README.md) for information about the probes. + ## Updating Docs A summary for each check needs to be included in the `README.md`. diff --git a/Makefile b/Makefile index 68decace..61f76940 100644 --- a/Makefile +++ b/Makefile @@ -94,6 +94,11 @@ check-linter: | $(GOLANGCI_LINT) # Run golangci-lint linter $(GOLANGCI_LINT) run -c .golangci.yml +fix-linter: ## Install and run golang linter, with fixes +fix-linter: | $(GOLANGCI_LINT) + # Run golangci-lint linter + $(GOLANGCI_LINT) run -c .golangci.yml --fix + add-projects: ## Adds new projects to ./cron/internal/data/projects.csv and ./cron/internal/data/gitlab-projects.csv add-projects: ./cron/internal/data/projects.csv | build-add-script # GitHub diff --git a/probes/README.md b/probes/README.md new file mode 100644 index 00000000..0c831484 --- /dev/null +++ b/probes/README.md @@ -0,0 +1,70 @@ +# Scorecard probes + +This directory contains all the Scorecard probes. + +A probe is an assessment of a focused, specific heuristic typically isolated to a particular ecosystem. For example, Scorecards fuzzing check consists of many different probes that assess particular ecosystems or aspects of fuzzing. + +Each probe has its own directory in `scorecard/probes`. The probes follow a camelcase naming convention that describe the exact heuristic a particular probe assesses. + +Probes can return multiple or a single finding, where a finding is a piece of data with an outcome, message, and optionally a location. Probes should be designed in such a way that a `finding.OutcomePositive` reflects a positive result, and `finding.OutcomeNegative` reflects a negative result. Scorecard has other `finding.Outcome` types available for other results; For example, the `finding.OutcomeNotAvailable` is often used for scenarios, where Scorecard cannot assess a project with a given probe. In addition, probes should also be named in such a way that they answer "yes" or "no", and where "yes" answers positively to the heuristic, and "no" answers negatively. For example, probes that check for SAST tools in the CI are called `toolXXXInstalled` so that `finding.OutcomePositive` reflects that it is positive to use the given tool, and that "yes" reflects what Scorecard considers the positive outcome. For some probes, this can be a bit trickier to do; The `notArchived` probe checks whether a project is archived, however, Scorecard considers archived projects to be negative, and the probe cannot be called `isArchived`. These naming conventions are not hard rules but merely guidelines. Note that probes do not do any formal evaluation such a scoring; This is left to the evaluation part once the outcomes have been produced by the probes. + +A probe consists of three files: + +- `def.yml`: The documentation of the probe. +- `impl.go`: The actual implementation of the probe. +- `impl_test.go`: The probes test. + +## Reusing code in probes + +When multiple probes use the same code, the reused code can be placed on `scorecard/probes/internal/utils` + +## How do I know which probes to add? + +In general, browsing through the Scorecard GitHub issues is the best way to find new probes to add. Requests for support for new tools, fuzzing engines or other heuristics can often be converted into specific probes. + +## Probe definition formatting + +Probe definitions can display links following standard markdown format. + +Probe definitions can display dynamic content. This requires modifications in `def.yml` and `impl.go` and in the evaluation steps. + +The following snippet in `def.yml` will display dynamic data provided by `impl.go`: + +```md +${{ metadata.dataToDisplay }} +``` + +And then in `impl.go` add the following metadata: + +```golang +f, err := finding.NewWith(fs, Probe, + "Message", nil, + finding.OutcomePositive) +f = f.WithRemediationMetadata(map[string]string{ + "dataToDisplay": "this is the text we will display", +}) +``` + +### Example +Consider a probe with following line in its `def.yml`: +``` +The project ${{ metadata.oss-fuzz-integration-status }} integrated into OSS-Fuzz. +``` + +and the probe sets the following metadata: +```golang +f, err := finding.NewWith(fs, Probe, + "Message", nil, + finding.OutcomePositive) +f = f.WithRemediationMetadata(map[string]string{ + "oss-fuzz-integration-status": "is", +}) +``` + +The probe will then output the following text: +``` +The project is integrated into OSS-Fuzz. +``` + +### Should the changes be in the probe or the evaluation? +The remediation data must be set in the probe. \ No newline at end of file