From a4da39a7798ef1391170627e62b9844d9b47bfdc Mon Sep 17 00:00:00 2001 From: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Date: Tue, 2 May 2023 17:42:32 -0700 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20[experimental]=20Create=20probes=20?= =?UTF-8?q?within=20findings=20(#2919)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * update Signed-off-by: laurentsimon * update Signed-off-by: laurentsimon * update Signed-off-by: laurentsimon * update Signed-off-by: laurentsimon * update Signed-off-by: laurentsimon * update Signed-off-by: laurentsimon * update Signed-off-by: laurentsimon * update Signed-off-by: laurentsimon * update Signed-off-by: laurentsimon * update Signed-off-by: laurentsimon * update Signed-off-by: laurentsimon * update Signed-off-by: laurentsimon --------- Signed-off-by: laurentsimon --- ...gitHubWorkflowPermissionsStepsNoWrite.yml} | 5 +- ...> gitHubWorkflowPermissionsTopNoWrite.yml} | 7 +- checks/evaluation/permissions/permissions.go | 66 ++++--- checks/permissions_test.go | 6 +- finding/finding.go | 159 ++++++++++++--- finding/finding_test.go | 102 +++++----- finding/probe/probe.go | 183 ++++++++++++++++++ finding/probe/probe_test.go | 109 +++++++++++ finding/probe/testdata/all-fields.yml | 16 ++ .../testdata/invalid-effort.yml} | 5 +- .../testdata/missing-id.yml} | 2 - finding/testdata/effort-high.yml | 3 +- finding/testdata/effort-low.yml | 3 +- finding/testdata/metadata-variables.yml | 7 +- pkg/common.go | 4 +- pkg/sarif.go | 2 +- 16 files changed, 546 insertions(+), 133 deletions(-) rename checks/evaluation/permissions/{GitHubWorkflowPermissionsStepsNoWrite.yml => gitHubWorkflowPermissionsStepsNoWrite.yml} (87%) rename checks/evaluation/permissions/{GitHubWorkflowPermissionsTopNoWrite.yml => gitHubWorkflowPermissionsTopNoWrite.yml} (85%) create mode 100644 finding/probe/probe.go create mode 100644 finding/probe/probe_test.go create mode 100644 finding/probe/testdata/all-fields.yml rename finding/{testdata/risk-high.yml => probe/testdata/invalid-effort.yml} (85%) rename finding/{testdata/risk-low.yml => probe/testdata/missing-id.yml} (90%) diff --git a/checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml b/checks/evaluation/permissions/gitHubWorkflowPermissionsStepsNoWrite.yml similarity index 87% rename from checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml rename to checks/evaluation/permissions/gitHubWorkflowPermissionsStepsNoWrite.yml index d4d81e22..171f8503 100644 --- a/checks/evaluation/permissions/GitHubWorkflowPermissionsStepsNoWrite.yml +++ b/checks/evaluation/permissions/gitHubWorkflowPermissionsStepsNoWrite.yml @@ -12,8 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +id: gitHubWorkflowPermissionsStepsNoWrite short: Checks that GitHub workflows do not have steps with dangerous write permissions -desc: This rule checks that GitHub workflows do not have steps with dangerous write permissions motivation: > Even with permissions default set to read, some scopes having write permissions in their steps brings incurs a risk to the project. By giving write permission to the Actions you call in jobs, an external Action you call could abuse them. Depending on the permissions, @@ -21,10 +21,9 @@ motivation: > For more information about the scopes and the vulnerabilities involved, see https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions. implementation: > - The rule is implemented by checking whether the `permissions` keyword is given non-write permissions for the following + The probe is implemented by checking whether the `permissions` keyword is given non-write permissions for the following scopes: `statuses`, `checks`, `security-events`, `deployments`, `contents`, `packages`, `actions`. Write permissions given to recognized packaging actions or commands are allowed and are considered an acceptable risk. -risk: Medium remediation: effort: High text: diff --git a/checks/evaluation/permissions/GitHubWorkflowPermissionsTopNoWrite.yml b/checks/evaluation/permissions/gitHubWorkflowPermissionsTopNoWrite.yml similarity index 85% rename from checks/evaluation/permissions/GitHubWorkflowPermissionsTopNoWrite.yml rename to checks/evaluation/permissions/gitHubWorkflowPermissionsTopNoWrite.yml index e1547592..65c89173 100644 --- a/checks/evaluation/permissions/GitHubWorkflowPermissionsTopNoWrite.yml +++ b/checks/evaluation/permissions/gitHubWorkflowPermissionsTopNoWrite.yml @@ -12,8 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +id: gitHubWorkflowPermissionsTopNoWrite short: Checks that GitHub workflows do not have default write permissions -desc: This rule checks that GitHub workflows do not have default write permissions motivation: > If no permissions are declared, a workflow's GitHub token's permissions default to write for all scopes. This include write permissions to push to the repository, to read encrypted secrets, etc. @@ -21,16 +21,15 @@ motivation: > implementation: > The rule is implemented by checking whether the `permissions` keyword is defined at the top of the workflow, and that no write permissions are given. -risk: High remediation: effort: Low text: - - Visit https://app.stepsecurity.io/secureworkflow/${{ repo }}/${{ workflow }}/${{ branch }}?enable=permissions + - Visit https://app.stepsecurity.io/secureworkflow/${{ metadata.repo }}/${{ metadata.workflow }}/${{ metadata.branch }}?enable=permissions - Tick the 'Restrict permissions for GITHUB_TOKEN' - Untick other options - "NOTE: If you want to resolve multiple issues at once, you can visit https://app.stepsecurity.io/securerepo instead." markdown: - - Visit [https://app.stepsecurity.io/secureworkflow](https://app.stepsecurity.io/secureworkflow/${{ repo }}/${{ workflow }}/${{ branch }}?enable=permissions). + - Visit [https://app.stepsecurity.io/secureworkflow](https://app.stepsecurity.io/secureworkflow/${{ metadata.repo }}/${{ metadata.workflow }}/${{ metadata.branch }}?enable=permissions). - Tick the 'Restrict permissions for GITHUB_TOKEN' - Untick other options - "NOTE: If you want to resolve multiple issues at once, you can visit [https://app.stepsecurity.io/securerepo](https://app.stepsecurity.io/securerepo) instead." diff --git a/checks/evaluation/permissions/permissions.go b/checks/evaluation/permissions/permissions.go index a661c372..ed8b0cbc 100644 --- a/checks/evaluation/permissions/permissions.go +++ b/checks/evaluation/permissions/permissions.go @@ -26,13 +26,18 @@ import ( ) //go:embed *.yml -var rules embed.FS +var probes embed.FS type permissions struct { topLevelWritePermissions map[string]bool jobLevelWritePermissions map[string]bool } +var ( + stepsNoWriteID = "gitHubWorkflowPermissionsStepsNoWrite" + topNoWriteID = "gitHubWorkflowPermissionsTopNoWrite" +) + // TokenPermissions applies the score policy for the Token-Permissions check. func TokenPermissions(name string, c *checker.CheckRequest, r *checker.TokenPermissionsData) checker.CheckResult { if r == nil { @@ -67,9 +72,9 @@ func applyScorePolicy(results *checker.TokenPermissionsData, c *checker.CheckReq dl := c.Dlogger //nolint:errcheck remediationMetadata, _ := remediation.New(c) - negativeRuleResults := map[string]bool{ - "GitHubWorkflowPermissionsStepsNoWrite": false, - "GitHubWorkflowPermissionsTopNoWrite": false, + negativeProbeResults := map[string]bool{ + stepsNoWriteID: false, + topNoWriteID: false, } for _, r := range results.TokenPermissions { @@ -77,14 +82,12 @@ func applyScorePolicy(results *checker.TokenPermissionsData, c *checker.CheckReq if r.File != nil { loc = &finding.Location{ Type: r.File.Type, - Value: r.File.Path, + Path: r.File.Path, LineStart: &r.File.Offset, } if r.File.Snippet != "" { loc.Snippet = &r.File.Snippet } - - loc.Value = r.File.Path } text, err := createText(r) @@ -112,7 +115,7 @@ func applyScorePolicy(results *checker.TokenPermissionsData, c *checker.CheckReq // We warn only for top-level. if *r.LocationType == checker.PermissionLocationTop { - warnWithRemediation(dl, msg, remediationMetadata, loc, negativeRuleResults) + warnWithRemediation(dl, msg, remediationMetadata, loc, negativeProbeResults) } else { dl.Debug(msg) } @@ -123,7 +126,7 @@ func applyScorePolicy(results *checker.TokenPermissionsData, c *checker.CheckReq } case checker.PermissionLevelWrite: - warnWithRemediation(dl, msg, remediationMetadata, loc, negativeRuleResults) + warnWithRemediation(dl, msg, remediationMetadata, loc, negativeProbeResults) // Group results by workflow name for score computation. if err := updateWorkflowHashMap(hm, r); err != nil { @@ -132,7 +135,7 @@ func applyScorePolicy(results *checker.TokenPermissionsData, c *checker.CheckReq } } - if err := reportDefaultFindings(results, c.Dlogger, negativeRuleResults); err != nil { + if err := reportDefaultFindings(results, c.Dlogger, negativeProbeResults); err != nil { return checker.InconclusiveResultScore, err } @@ -140,17 +143,18 @@ func applyScorePolicy(results *checker.TokenPermissionsData, c *checker.CheckReq } func reportDefaultFindings(results *checker.TokenPermissionsData, - dl checker.DetailLogger, negativeRuleResults map[string]bool, + dl checker.DetailLogger, negativeProbeResults map[string]bool, ) error { + // TODO(#2928): re-visit the need for NotApplicable outcome. // No workflow files exist. if len(results.TokenPermissions) == 0 { text := "no workflows found in the repository" - if err := reportFinding("GitHubWorkflowPermissionsStepsNoWrite", - text, finding.OutcomeNotApplicable, dl); err != nil { + if err := reportFinding(stepsNoWriteID, + text, finding.OutcomeNotAvailable, dl); err != nil { return err } - if err := reportFinding("GitHubWorkflowPermissionsTopNoWrite", - text, finding.OutcomeNotApplicable, dl); err != nil { + if err := reportFinding(topNoWriteID, + text, finding.OutcomeNotAvailable, dl); err != nil { return err } return nil @@ -158,12 +162,12 @@ func reportDefaultFindings(results *checker.TokenPermissionsData, // Workflow files found, report positive findings if no // negative findings were found. - // NOTE: we don't consider rule `GitHubWorkflowPermissionsTopNoWrite` + // NOTE: we don't consider probe `topNoWriteID` // because positive results are already reported. - found := negativeRuleResults["GitHubWorkflowPermissionsStepsNoWrite"] + found := negativeProbeResults[stepsNoWriteID] if !found { text := fmt.Sprintf("no %s write permissions found", checker.PermissionLocationJob) - if err := reportFinding("GitHubWorkflowPermissionsStepsNoWrite", + if err := reportFinding(stepsNoWriteID, text, finding.OutcomePositive, dl); err != nil { return err } @@ -172,8 +176,12 @@ func reportDefaultFindings(results *checker.TokenPermissionsData, return nil } -func reportFinding(rule, text string, o finding.Outcome, dl checker.DetailLogger) error { - f, err := finding.New(rules, rule) +func reportFinding(probe, text string, o finding.Outcome, dl checker.DetailLogger) error { + content, err := probes.ReadFile(probe + ".yml") + if err != nil { + return fmt.Errorf("%w", err) + } + f, err := finding.FromBytes(content, probe) if err != nil { return sce.WithMessage(sce.ErrScorecardInternal, err.Error()) } @@ -185,11 +193,15 @@ func reportFinding(rule, text string, o finding.Outcome, dl checker.DetailLogger } func createLogMsg(loct *checker.PermissionLocation) (*checker.LogMessage, error) { - Rule := "GitHubWorkflowPermissionsStepsNoWrite" + probe := stepsNoWriteID if loct == nil || *loct == checker.PermissionLocationTop { - Rule = "GitHubWorkflowPermissionsTopNoWrite" + probe = topNoWriteID } - f, err := finding.New(rules, Rule) + content, err := probes.ReadFile(probe + ".yml") + if err != nil { + return nil, fmt.Errorf("%w", err) + } + f, err := finding.FromBytes(content, probe) if err != nil { return nil, sce.WithMessage(sce.ErrScorecardInternal, err.Error()) @@ -201,19 +213,19 @@ func createLogMsg(loct *checker.PermissionLocation) (*checker.LogMessage, error) func warnWithRemediation(logger checker.DetailLogger, msg *checker.LogMessage, rem *remediation.RemediationMetadata, loc *finding.Location, - negativeRuleResults map[string]bool, + negativeProbeResults map[string]bool, ) { - if loc != nil && loc.Value != "" { + if loc != nil && loc.Path != "" { msg.Finding = msg.Finding.WithRemediationMetadata(map[string]string{ "repo": rem.Repo, "branch": rem.Branch, - "workflow": strings.TrimPrefix(loc.Value, ".github/workflows/"), + "workflow": strings.TrimPrefix(loc.Path, ".github/workflows/"), }) } logger.Warn(msg) // Record that we found a negative result. - negativeRuleResults[msg.Finding.Rule] = true + negativeProbeResults[msg.Finding.Probe] = true } func recordPermissionWrite(hm map[string]permissions, path string, diff --git a/checks/permissions_test.go b/checks/permissions_test.go index 066ed36f..189156f3 100644 --- a/checks/permissions_test.go +++ b/checks/permissions_test.go @@ -319,8 +319,8 @@ func TestGithubTokenPermissions(t *testing.T) { Error: nil, Score: checker.MaxResultScore, NumberOfWarn: 0, - NumberOfInfo: 2, // This is constant. - NumberOfDebug: 8, // This is 4 + (number of actions) + NumberOfInfo: 2, // This is constant. + NumberOfDebug: 8, // This is 4 + (number of actions) }, }, { @@ -488,7 +488,7 @@ func TestGithubTokenPermissionsLineNumber(t *testing.T) { logMessage.Finding.Location != nil && logMessage.Finding.Location.LineStart != nil && *logMessage.Finding.Location.LineStart == expectedLog.lineNumber && - logMessage.Finding.Location.Value == p && + logMessage.Finding.Location.Path == p && logType == checker.DetailWarn } if !scut.ValidateLogMessage(isExpectedLog, &dl) { diff --git a/finding/finding.go b/finding/finding.go index d9c2d72d..c2c23f38 100644 --- a/finding/finding.go +++ b/finding/finding.go @@ -16,10 +16,13 @@ package finding import ( "embed" + "errors" "fmt" "strings" - "github.com/ossf/scorecard/v4/rule" + "gopkg.in/yaml.v3" + + "github.com/ossf/scorecard/v4/finding/probe" ) // FileType is the type of a file. @@ -42,7 +45,7 @@ const ( // nolint: govet type Location struct { Type FileType `json:"type"` - Value string `json:"value"` + Path string `json:"path"` LineStart *uint `json:"lineStart,omitempty"` LineEnd *uint `json:"lineEnd,omitempty"` Snippet *string `json:"snippet,omitempty"` @@ -51,13 +54,33 @@ type Location struct { // Outcome is the result of a finding. type Outcome int +// TODO(#2928): re-visit the finding definitions. const ( + // NOTE: The additional '_' are intended for future use. + // This allows adding outcomes without breaking the values + // of existing outcomes. // OutcomeNegative indicates a negative outcome. OutcomeNegative Outcome = iota + _ + _ + _ + // OutcomeNotAvailable indicates an unavailable outcome, + // typically because an API call did not return an answer. + OutcomeNotAvailable + _ + _ + _ + // OutcomeError indicates an errors while running. + // The results could not be determined. + OutcomeError + _ + _ + _ // OutcomePositive indicates a positive outcome. OutcomePositive - // OutcomeNotApplicable indicates a non-applicable outcome. - OutcomeNotApplicable + _ + _ + _ // OutcomeNotSupported indicates a non-supported outcome. OutcomeNotSupported ) @@ -65,32 +88,92 @@ const ( // Finding represents a finding. // nolint: govet type Finding struct { - Rule string `json:"rule"` - Outcome Outcome `json:"outcome"` - Risk rule.Risk `json:"risk"` - Message string `json:"message"` - Location *Location `json:"location,omitempty"` - Remediation *rule.Remediation `json:"remediation,omitempty"` + Probe string `json:"probe"` + Outcome Outcome `json:"outcome"` + Message string `json:"message"` + Location *Location `json:"location,omitempty"` + Remediation *probe.Remediation `json:"remediation,omitempty"` } -// New creates a new finding. -func New(loc embed.FS, ruleID string) (*Finding, error) { - r, err := rule.New(loc, ruleID) +// AnonymousFinding is a finding without a corerpsonding probe ID. +type AnonymousFinding struct { + Finding + // Remove the probe ID from + // the structure until the probes are GA. + Probe string `json:"probe,omitempty"` +} + +var errInvalid = errors.New("invalid") + +// FromBytes creates a finding for a probe given its config file's content. +func FromBytes(content []byte, probeID string) (*Finding, error) { + p, err := probe.FromBytes(content, probeID) if err != nil { // nolint return nil, err } f := &Finding{ - Rule: ruleID, + Probe: p.ID, Outcome: OutcomeNegative, - Remediation: r.Remediation, - } - if r.Remediation != nil { - f.Risk = r.Risk + Remediation: p.Remediation, } return f, nil } +// New creates a new finding. +func New(loc embed.FS, probeID string) (*Finding, error) { + p, err := probe.New(loc, probeID) + if err != nil { + return nil, fmt.Errorf("%w", err) + } + + f := &Finding{ + Probe: p.ID, + Outcome: OutcomeNegative, + Remediation: p.Remediation, + } + return f, nil +} + +// NewWith create a finding with the desried location and outcome. +func NewWith(efs embed.FS, probeID, text string, loc *Location, + o Outcome, +) (*Finding, error) { + f, err := New(efs, probeID) + if err != nil { + return nil, fmt.Errorf("finding.New: %w", err) + } + + f = f.WithMessage(text).WithOutcome(o).WithLocation(loc) + return f, nil +} + +// NewWith create a negative finding with the desried location. +func NewNegative(efs embed.FS, probeID, text string, loc *Location, +) (*Finding, error) { + return NewWith(efs, probeID, text, loc, OutcomeNegative) +} + +// NewNotAvailable create a finding with a NotAvailable outcome and the desried location. +func NewNotAvailable(efs embed.FS, probeID, text string, loc *Location, +) (*Finding, error) { + return NewWith(efs, probeID, text, loc, OutcomeNotAvailable) +} + +// NewPositive create a positive finding with the desried location. +func NewPositive(efs embed.FS, probeID, text string, loc *Location, +) (*Finding, error) { + return NewWith(efs, probeID, text, loc, OutcomePositive) +} + +// Anonymize removes the probe ID and outcome +// from the finding. It is a temporary solution +// to integrate the code in the details without exposing +// too much information. +func (f *Finding) Anonymize() *AnonymousFinding { + return &AnonymousFinding{Finding: *f} +} + // WithMessage adds a message to an existing finding. // No copy is made. func (f *Finding) WithMessage(text string) *Finding { @@ -102,6 +185,13 @@ func (f *Finding) WithMessage(text string) *Finding { // No copy is made. func (f *Finding) WithLocation(loc *Location) *Finding { f.Location = loc + if f.Remediation != nil && f.Location != nil { + // Replace location data. + f.Remediation.Text = strings.Replace(f.Remediation.Text, + "${{ finding.location.path }}", f.Location.Path, -1) + f.Remediation.Markdown = strings.Replace(f.Remediation.Markdown, + "${{ finding.location.path }}", f.Location.Path, -1) + } return f } @@ -109,6 +199,8 @@ func (f *Finding) WithLocation(loc *Location) *Finding { // No copy is made. func (f *Finding) WithPatch(patch *string) *Finding { f.Remediation.Patch = patch + // NOTE: we will update the remediation section + // using patch information, e.g. ${{ patch.content }}. return f } @@ -131,16 +223,37 @@ func (f *Finding) WithRemediationMetadata(values map[string]string) *Finding { if f.Remediation != nil { // Replace all dynamic values. for k, v := range values { + // Replace metadata. f.Remediation.Text = strings.Replace(f.Remediation.Text, - fmt.Sprintf("${{ %s }}", k), v, -1) + fmt.Sprintf("${{ metadata.%s }}", k), v, -1) f.Remediation.Markdown = strings.Replace(f.Remediation.Markdown, - fmt.Sprintf("${{ %s }}", k), v, -1) + fmt.Sprintf("${{ metadata.%s }}", k), v, -1) } } return f } -// WorseThan compares outcomes. -func (o *Outcome) WorseThan(oo Outcome) bool { - return *o < oo +// UnmarshalYAML is a custom unmarshalling function +// to transform the string into an enum. +func (o *Outcome) UnmarshalYAML(n *yaml.Node) error { + var str string + if err := n.Decode(&str); err != nil { + return fmt.Errorf("decode: %w", err) + } + + switch n.Value { + case "Negative": + *o = OutcomeNegative + case "Positive": + *o = OutcomePositive + case "NotAvailable": + *o = OutcomeNotAvailable + case "NotSupported": + *o = OutcomeNotSupported + case "Error": + *o = OutcomeError + default: + return fmt.Errorf("%w: %q", errInvalid, str) + } + return nil } diff --git a/finding/finding_test.go b/finding/finding_test.go index f4586558..bda8e740 100644 --- a/finding/finding_test.go +++ b/finding/finding_test.go @@ -15,24 +15,21 @@ package finding import ( - "embed" "errors" + "os" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" - "github.com/ossf/scorecard/v4/rule" + "github.com/ossf/scorecard/v4/finding/probe" ) func errCmp(e1, e2 error) bool { return errors.Is(e1, e2) || errors.Is(e2, e1) } -//go:embed testdata/* -var testfs embed.FS - -func Test_New(t *testing.T) { +func Test_FromBytes(t *testing.T) { snippet := "some code snippet" patch := "some patch values" sline := uint(10) @@ -44,106 +41,92 @@ func Test_New(t *testing.T) { tests := []struct { name string id string + path string outcome *Outcome err error metadata map[string]string finding *Finding }{ - { - name: "risk high", - id: "testdata/risk-high", - outcome: &negativeOutcome, - finding: &Finding{ - Rule: "testdata/risk-high", - Outcome: OutcomeNegative, - Risk: rule.RiskHigh, - Remediation: &rule.Remediation{ - Text: "step1\nstep2 https://www.google.com/something", - Markdown: "step1\nstep2 [google.com](https://www.google.com/something)", - Effort: rule.RemediationEffortLow, - }, - }, - }, { name: "effort low", - id: "testdata/effort-low", + id: "effort-low", + path: "testdata/effort-low.yml", outcome: &negativeOutcome, finding: &Finding{ - Rule: "testdata/effort-low", + Probe: "effort-low", Outcome: OutcomeNegative, - Risk: rule.RiskHigh, - Remediation: &rule.Remediation{ + Remediation: &probe.Remediation{ Text: "step1\nstep2 https://www.google.com/something", Markdown: "step1\nstep2 [google.com](https://www.google.com/something)", - Effort: rule.RemediationEffortLow, + Effort: probe.RemediationEffortLow, }, }, }, { name: "effort high", - id: "testdata/effort-high", + id: "effort-high", + path: "testdata/effort-high.yml", outcome: &negativeOutcome, finding: &Finding{ - Rule: "testdata/effort-high", + Probe: "effort-high", Outcome: OutcomeNegative, - Risk: rule.RiskHigh, - Remediation: &rule.Remediation{ + Remediation: &probe.Remediation{ Text: "step1\nstep2 https://www.google.com/something", Markdown: "step1\nstep2 [google.com](https://www.google.com/something)", - Effort: rule.RemediationEffortHigh, + Effort: probe.RemediationEffortHigh, }, }, }, { name: "env variables", - id: "testdata/metadata-variables", + id: "metadata-variables", + path: "testdata/metadata-variables.yml", outcome: &negativeOutcome, metadata: map[string]string{"branch": "master", "repo": "ossf/scorecard"}, finding: &Finding{ - Rule: "testdata/metadata-variables", + Probe: "metadata-variables", Outcome: OutcomeNegative, - Risk: rule.RiskHigh, - Remediation: &rule.Remediation{ + Remediation: &probe.Remediation{ Text: "step1\nstep2 google.com/ossf/scorecard@master", Markdown: "step1\nstep2 [google.com/ossf/scorecard@master](google.com/ossf/scorecard@master)", - Effort: rule.RemediationEffortLow, + Effort: probe.RemediationEffortLow, }, }, }, { name: "patch", - id: "testdata/metadata-variables", + id: "metadata-variables", + path: "testdata/metadata-variables.yml", outcome: &negativeOutcome, metadata: map[string]string{"branch": "master", "repo": "ossf/scorecard"}, finding: &Finding{ - Rule: "testdata/metadata-variables", + Probe: "metadata-variables", Outcome: OutcomeNegative, - Risk: rule.RiskHigh, - Remediation: &rule.Remediation{ + Remediation: &probe.Remediation{ Text: "step1\nstep2 google.com/ossf/scorecard@master", Markdown: "step1\nstep2 [google.com/ossf/scorecard@master](google.com/ossf/scorecard@master)", - Effort: rule.RemediationEffortLow, + Effort: probe.RemediationEffortLow, Patch: &patch, }, }, }, { name: "location", - id: "testdata/metadata-variables", + id: "metadata-variables", + path: "testdata/metadata-variables.yml", outcome: &negativeOutcome, metadata: map[string]string{"branch": "master", "repo": "ossf/scorecard"}, finding: &Finding{ - Rule: "testdata/metadata-variables", + Probe: "metadata-variables", Outcome: OutcomeNegative, - Risk: rule.RiskHigh, - Remediation: &rule.Remediation{ + Remediation: &probe.Remediation{ Text: "step1\nstep2 google.com/ossf/scorecard@master", Markdown: "step1\nstep2 [google.com/ossf/scorecard@master](google.com/ossf/scorecard@master)", - Effort: rule.RemediationEffortLow, + Effort: probe.RemediationEffortLow, }, Location: &Location{ Type: FileTypeSource, - Value: "path/to/file.txt", + Path: "path/to/file.txt", LineStart: &sline, LineEnd: &eline, Snippet: &snippet, @@ -152,29 +135,29 @@ func Test_New(t *testing.T) { }, { name: "text", - id: "testdata/metadata-variables", + id: "metadata-variables", + path: "testdata/metadata-variables.yml", outcome: &negativeOutcome, metadata: map[string]string{"branch": "master", "repo": "ossf/scorecard"}, finding: &Finding{ - Rule: "testdata/metadata-variables", + Probe: "metadata-variables", Outcome: OutcomeNegative, - Risk: rule.RiskHigh, - Remediation: &rule.Remediation{ + Remediation: &probe.Remediation{ Text: "step1\nstep2 google.com/ossf/scorecard@master", Markdown: "step1\nstep2 [google.com/ossf/scorecard@master](google.com/ossf/scorecard@master)", - Effort: rule.RemediationEffortLow, + Effort: probe.RemediationEffortLow, }, Message: "some text", }, }, { - name: "outcome", - id: "testdata/metadata-variables", + name: "positive outcome", + id: "metadata-variables", + path: "testdata/metadata-variables.yml", outcome: &positiveOutcome, finding: &Finding{ - Rule: "testdata/metadata-variables", + Probe: "metadata-variables", Outcome: OutcomePositive, - Risk: rule.RiskHigh, Message: "some text", }, }, @@ -184,7 +167,12 @@ func Test_New(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - r, err := New(testfs, tt.id) + content, err := os.ReadFile(tt.path) + if err != nil { + t.Fatalf(err.Error()) + } + + r, err := FromBytes(content, tt.id) if err != nil || tt.err != nil { if !errCmp(err, tt.err) { t.Fatalf("unexpected error: %v", cmp.Diff(err, tt.err, cmpopts.EquateErrors())) diff --git a/finding/probe/probe.go b/finding/probe/probe.go new file mode 100644 index 00000000..1db07e8e --- /dev/null +++ b/finding/probe/probe.go @@ -0,0 +1,183 @@ +// Copyright 2023 OpenSSF Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package probe + +import ( + "embed" + "errors" + "fmt" + "strings" + + "gopkg.in/yaml.v3" +) + +var errInvalid = errors.New("invalid") + +// RemediationEffort indicates the estimated effort necessary to remediate a finding. +type RemediationEffort int + +const ( + // RemediationEffortNone indicates a no remediation effort. + RemediationEffortNone RemediationEffort = iota + // RemediationEffortLow indicates a low remediation effort. + RemediationEffortLow + // RemediationEffortMedium indicates a medium remediation effort. + RemediationEffortMedium + // RemediationEffortHigh indicates a high remediation effort. + RemediationEffortHigh +) + +// Remediation represents the remediation for a finding. +type Remediation struct { + // Patch for machines. + Patch *string `json:"patch,omitempty"` + // Text for humans. + Text string `json:"text"` + // Text in markdown format for humans. + Markdown string `json:"markdown"` + // Effort to remediate. + Effort RemediationEffort `json:"effort"` +} + +// nolint: govet +type yamlRemediation struct { + Text []string `yaml:"text"` + Markdown []string `yaml:"markdown"` + Effort RemediationEffort `yaml:"effort"` +} + +// nolint: govet +type yamlProbe struct { + ID string `yaml:"id"` + Short string `yaml:"short"` + Motivation string `yaml:"motivation"` + Implementation string `yaml:"implementation"` + Remediation yamlRemediation `yaml:"remediation"` +} + +// nolint: govet +type Probe struct { + ID string + Short string + Motivation string + Implementation string + Remediation *Remediation +} + +// FromBytes creates a probe from a file. +func FromBytes(content []byte, probeID string) (*Probe, error) { + r, err := parseFromYAML(content) + if err != nil { + return nil, err + } + + if err := validate(r, probeID); err != nil { + return nil, err + } + + return &Probe{ + ID: r.ID, + Short: r.Short, + Motivation: r.Motivation, + Implementation: r.Implementation, + Remediation: &Remediation{ + Text: strings.Join(r.Remediation.Text, "\n"), + Markdown: strings.Join(r.Remediation.Markdown, "\n"), + Effort: r.Remediation.Effort, + }, + }, nil +} + +// New create a new probe. +func New(loc embed.FS, probeID string) (*Probe, error) { + content, err := loc.ReadFile("def.yml") + if err != nil { + return nil, fmt.Errorf("%w", err) + } + return FromBytes(content, probeID) +} + +func validate(r *yamlProbe, probeID string) error { + if err := validateID(r.ID, probeID); err != nil { + return err + } + if err := validateRemediation(r.Remediation); err != nil { + return err + } + return nil +} + +func validateID(actual, expected string) error { + if actual != expected { + return fmt.Errorf("%w: ID: read '%v', expected '%v'", errInvalid, + actual, expected) + } + return nil +} + +func validateRemediation(r yamlRemediation) error { + switch r.Effort { + case RemediationEffortHigh, RemediationEffortMedium, RemediationEffortLow: + return nil + default: + return fmt.Errorf("%w: %v", errInvalid, fmt.Sprintf("remediation '%v'", r)) + } +} + +func parseFromYAML(content []byte) (*yamlProbe, error) { + r := yamlProbe{} + + err := yaml.Unmarshal(content, &r) + if err != nil { + return nil, fmt.Errorf("%w: %v", errInvalid, err) + } + return &r, nil +} + +// UnmarshalYAML is a custom unmarshalling function +// to transform the string into an enum. +func (r *RemediationEffort) UnmarshalYAML(n *yaml.Node) error { + var str string + if err := n.Decode(&str); err != nil { + return fmt.Errorf("%w: %v", errInvalid, err) + } + + // nolint:goconst + switch n.Value { + case "Low": + *r = RemediationEffortLow + case "Medium": + *r = RemediationEffortMedium + case "High": + *r = RemediationEffortHigh + default: + return fmt.Errorf("%w: effort:%q", errInvalid, str) + } + return nil +} + +// String stringifies the enum. +func (r *RemediationEffort) String() string { + switch *r { + case RemediationEffortLow: + return "Low" + case RemediationEffortMedium: + return "Medium" + case RemediationEffortHigh: + return "High" + default: + return "" + } +} diff --git a/finding/probe/probe_test.go b/finding/probe/probe_test.go new file mode 100644 index 00000000..2fe1a8a8 --- /dev/null +++ b/finding/probe/probe_test.go @@ -0,0 +1,109 @@ +// Copyright 2023 OpenSSF Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package probe + +import ( + "errors" + "os" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" +) + +func errCmp(e1, e2 error) bool { + return errors.Is(e1, e2) || errors.Is(e2, e1) +} + +func Test_FromBytes(t *testing.T) { + t.Parallel() + // nolint: govet + tests := []struct { + name string + id string + path string + err error + probe *Probe + }{ + { + name: "all fields set", + id: "all-fields", + path: "testdata/all-fields.yml", + probe: &Probe{ + ID: "all-fields", + Short: "short description", + Implementation: "impl1 impl2\n", + Motivation: "mot1 mot2\n", + Remediation: &Remediation{ + Text: "step1\nstep2 https://www.google.com/something", + Markdown: "step1\nstep2 [google.com](https://www.google.com/something)", + Effort: RemediationEffortLow, + }, + }, + }, + { + name: "mismatch probe ID", + id: "mismatch-id", + path: "testdata/all-fields.yml", + probe: &Probe{ + ID: "all-fields", + Short: "short description", + Implementation: "impl1 impl2\n", + Motivation: "mot1 mot2\n", + Remediation: &Remediation{ + Text: "step1\nstep2 https://www.google.com/something", + Markdown: "step1\nstep2 [google.com](https://www.google.com/something)", + Effort: RemediationEffortLow, + }, + }, + err: errInvalid, + }, + { + name: "missing id", + id: "missing-id", + path: "testdata/missing-id.yml", + err: errInvalid, + }, + { + name: "invalid effort", + id: "invalid-effort", + path: "testdata/invalid-effort.yml", + err: errInvalid, + }, + } + for _, tt := range tests { + tt := tt // Re-initializing variable so it is not changed while executing the closure below + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + content, err := os.ReadFile(tt.path) + if err != nil { + t.Fatalf(err.Error()) + } + + r, err := FromBytes(content, tt.id) + if err != nil || tt.err != nil { + if !errCmp(err, tt.err) { + t.Fatalf("unexpected error: %v", cmp.Diff(err, tt.err, cmpopts.EquateErrors())) + } + return + } + + if diff := cmp.Diff(*tt.probe, *r); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/finding/probe/testdata/all-fields.yml b/finding/probe/testdata/all-fields.yml new file mode 100644 index 00000000..c67a3251 --- /dev/null +++ b/finding/probe/testdata/all-fields.yml @@ -0,0 +1,16 @@ +id: all-fields +short: short description +motivation: > + mot1 + mot2 +implementation: > + impl1 + impl2 +remediation: + effort: Low + text: + - step1 + - step2 https://www.google.com/something + markdown: + - step1 + - step2 [google.com](https://www.google.com/something) diff --git a/finding/testdata/risk-high.yml b/finding/probe/testdata/invalid-effort.yml similarity index 85% rename from finding/testdata/risk-high.yml rename to finding/probe/testdata/invalid-effort.yml index 8f4ac0c9..0fc94744 100644 --- a/finding/testdata/risk-high.yml +++ b/finding/probe/testdata/invalid-effort.yml @@ -1,14 +1,13 @@ +id: invalid-effort short: short description -desc: description motivation: > line1 line2 implementation: > line1 line2 -risk: High remediation: - effort: Low + effort: invalid text: - step1 - step2 https://www.google.com/something diff --git a/finding/testdata/risk-low.yml b/finding/probe/testdata/missing-id.yml similarity index 90% rename from finding/testdata/risk-low.yml rename to finding/probe/testdata/missing-id.yml index 1c8e6cfc..7fb1325e 100644 --- a/finding/testdata/risk-low.yml +++ b/finding/probe/testdata/missing-id.yml @@ -1,12 +1,10 @@ short: short description -desc: description motivation: > line1 line2 implementation: > line1 line2 -risk: Low remediation: effort: Low text: diff --git a/finding/testdata/effort-high.yml b/finding/testdata/effort-high.yml index 237234d2..57a46402 100644 --- a/finding/testdata/effort-high.yml +++ b/finding/testdata/effort-high.yml @@ -1,12 +1,11 @@ +id: effort-high short: short description -desc: description motivation: > line1 line2 implementation: > line1 line2 -risk: High remediation: effort: High text: diff --git a/finding/testdata/effort-low.yml b/finding/testdata/effort-low.yml index 8f4ac0c9..7b390ecf 100644 --- a/finding/testdata/effort-low.yml +++ b/finding/testdata/effort-low.yml @@ -1,12 +1,11 @@ +id: effort-low short: short description -desc: description motivation: > line1 line2 implementation: > line1 line2 -risk: High remediation: effort: Low text: diff --git a/finding/testdata/metadata-variables.yml b/finding/testdata/metadata-variables.yml index c1307389..2fcb7c01 100644 --- a/finding/testdata/metadata-variables.yml +++ b/finding/testdata/metadata-variables.yml @@ -1,17 +1,16 @@ +id: metadata-variables short: short description -desc: description motivation: > line1 line2 implementation: > line1 line2 -risk: High remediation: effort: Low text: - step1 - - step2 google.com/${{ repo }}@${{ branch }} + - step2 google.com/${{ metadata.repo }}@${{ metadata.branch }} markdown: - step1 - - step2 [google.com/${{ repo }}@${{ branch }}](google.com/${{ repo }}@${{ branch }}) + - step2 [google.com/${{ metadata.repo }}@${{ metadata.branch }}](google.com/${{ metadata.repo }}@${{ metadata.branch }}) diff --git a/pkg/common.go b/pkg/common.go index c3a0ca2e..bd861d14 100644 --- a/pkg/common.go +++ b/pkg/common.go @@ -50,10 +50,10 @@ func nonStructuredResultString(d *checker.CheckDetail) string { func structuredResultString(d *checker.CheckDetail) string { var sb strings.Builder f := d.Msg.Finding - sb.WriteString(fmt.Sprintf("%s: %s severity: %s", typeToString(d.Type), f.Risk.String(), f.Message)) + sb.WriteString(fmt.Sprintf("%s: %s", typeToString(d.Type), f.Message)) if f.Location != nil { - sb.WriteString(fmt.Sprintf(": %s", f.Location.Value)) + sb.WriteString(fmt.Sprintf(": %s", f.Location.Path)) if f.Location.LineStart != nil { sb.WriteString(fmt.Sprintf(":%d", *f.Location.LineStart)) } diff --git a/pkg/sarif.go b/pkg/sarif.go index 8977f6d0..30f43d01 100644 --- a/pkg/sarif.go +++ b/pkg/sarif.go @@ -209,7 +209,7 @@ func generateDefaultConfig(risk string) string { func getPath(d *checker.CheckDetail) string { f := d.Msg.Finding if f != nil && f.Location != nil { - return f.Location.Value + return f.Location.Path } return d.Msg.Path }