Skip to content

Commit

Permalink
✨ Add custom remediation for workflow permissions/pinned dependencies (
Browse files Browse the repository at this point in the history
…#1885)

* draft

* update

* updates

* updates

* updates

* updates

* updates

* updates
  • Loading branch information
laurentsimon committed May 6, 2022
1 parent 22694dc commit 8c97d46
Show file tree
Hide file tree
Showing 8 changed files with 323 additions and 63 deletions.
25 changes: 19 additions & 6 deletions checker/check_result.go
Expand Up @@ -93,6 +93,18 @@ type CheckResult struct {
Reason string `json:"-"` // A sentence describing the check result (score, etc)
}

// Remediation represents a remediation.
type Remediation struct {
// Code snippet for humans.
Snippet string
// Diff for machines.
Diff string
// Help text for humans.
HelpText string
// Help text in markdown format for humans.
HelpMarkdown string
}

// CheckDetail contains information for each detail.
type CheckDetail struct {
Msg LogMessage
Expand All @@ -103,12 +115,13 @@ type CheckDetail struct {
// This allows updating the definition easily.
// nolint:govet
type LogMessage struct {
Text string // A short string explaining why the detail was recorded/logged.
Path string // Fullpath to the file.
Type FileType // Type of file.
Offset uint // Offset in the file of Path (line for source/text files).
EndOffset uint // End of offset in the file, e.g. if the command spans multiple lines.
Snippet string // Snippet of code
Text string // A short string explaining why the detail was recorded/logged.
Path string // Fullpath to the file.
Type FileType // Type of file.
Offset uint // Offset in the file of Path (line for source/text files).
EndOffset uint // End of offset in the file, e.g. if the command spans multiple lines.
Snippet string // Snippet of code
Remediation *Remediation // Remediation information, if any.
}

// CreateProportionalScore creates a proportional score.
Expand Down
67 changes: 39 additions & 28 deletions checks/permissions.go
Expand Up @@ -82,6 +82,11 @@ func TokenPermissions(c *checker.CheckRequest) checker.CheckResult {
data := permissionCbData{
workflows: make(map[string]permissions),
}

if err := remdiationSetup(c); err != nil {
createResultForLeastPrivilegeTokens(data, err)
}

err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{
Pattern: ".github/workflows/*",
CaseSensitive: false,
Expand Down Expand Up @@ -157,22 +162,24 @@ func validatePermission(permissionKey permission, permissionValue *actionlint.Pe
if strings.EqualFold(val, "write") {
if isPermissionOfInterest(permissionKey, ignoredPermissions) {
dl.Warn(&checker.LogMessage{
Path: path,
Type: checker.FileTypeSource,
Offset: lineNumber,
Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val),
// TODO: set Snippet.
Path: path,
Type: checker.FileTypeSource,
Offset: lineNumber,
Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val),
Snippet: val,
Remediation: createWorkflowPermissionRemediation(path),
})
recordPermissionWrite(pPermissions, permissionKey)
} else {
// Only log for debugging, otherwise
// it may confuse users.
dl.Debug(&checker.LogMessage{
Path: path,
Type: checker.FileTypeSource,
Offset: lineNumber,
Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val),
// TODO: set Snippet.
Path: path,
Type: checker.FileTypeSource,
Offset: lineNumber,
Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val),
Snippet: val,
Remediation: createWorkflowPermissionRemediation(path),
})
}
return nil
Expand Down Expand Up @@ -243,22 +250,24 @@ func validatePermissions(permissions *actionlint.Permissions, permLevel, path st
lineNumber := fileparser.GetLineNumber(permissions.All.Pos)
if !strings.EqualFold(val, "read-all") && val != "" {
dl.Warn(&checker.LogMessage{
Path: path,
Type: checker.FileTypeSource,
Offset: lineNumber,
Text: fmt.Sprintf("%s permissions set to '%v'", permLevel, val),
// TODO: set Snippet.
Path: path,
Type: checker.FileTypeSource,
Offset: lineNumber,
Text: fmt.Sprintf("%s permissions set to '%v'", permLevel, val),
Snippet: val,
Remediation: createWorkflowPermissionRemediation(path),
})
recordAllPermissionsWrite(pdata, permLevel, path)
return nil
}

dl.Info(&checker.LogMessage{
Path: path,
Type: checker.FileTypeSource,
Offset: lineNumber,
Text: fmt.Sprintf("%s permissions set to '%v'", permLevel, val),
// TODO: set Snippet.
Path: path,
Type: checker.FileTypeSource,
Offset: lineNumber,
Text: fmt.Sprintf("%s permissions set to '%v'", permLevel, val),
Snippet: val,
Remediation: createWorkflowPermissionRemediation(path),
})
} else /* scopeIsSet == true */ if err := validateMapPermissions(permissions.Scopes,
permLevel, path, dl, getWritePermissionsMap(pdata, path, permLevel), ignoredPermissions); err != nil {
Expand All @@ -273,10 +282,11 @@ func validateTopLevelPermissions(workflow *actionlint.Workflow, path string,
// Check if permissions are set explicitly.
if workflow.Permissions == nil {
dl.Warn(&checker.LogMessage{
Path: path,
Type: checker.FileTypeSource,
Offset: checker.OffsetDefault,
Text: fmt.Sprintf("no %s permission defined", topLevelPermission),
Path: path,
Type: checker.FileTypeSource,
Offset: checker.OffsetDefault,
Text: fmt.Sprintf("no %s permission defined", topLevelPermission),
Remediation: createWorkflowPermissionRemediation(path),
})
recordAllPermissionsWrite(pdata, topLevelPermission, path)
return nil
Expand All @@ -296,10 +306,11 @@ func validatejobLevelPermissions(workflow *actionlint.Workflow, path string,
// so only top-level read-only permissions need to be declared.
if job.Permissions == nil {
dl.Debug(&checker.LogMessage{
Path: path,
Type: checker.FileTypeSource,
Offset: fileparser.GetLineNumber(job.Pos),
Text: fmt.Sprintf("no %s permission defined", jobLevelPermission),
Path: path,
Type: checker.FileTypeSource,
Offset: fileparser.GetLineNumber(job.Pos),
Text: fmt.Sprintf("no %s permission defined", jobLevelPermission),
Remediation: createWorkflowPermissionRemediation(path),
})
recordAllPermissionsWrite(pdata, jobLevelPermission, path)
continue
Expand Down
13 changes: 9 additions & 4 deletions checks/pinned_dependencies.go
Expand Up @@ -63,6 +63,10 @@ func PinnedDependencies(c *checker.CheckRequest) checker.CheckResult {
}
*/

if remErr := remdiationSetup(c); remErr != nil {
return checker.CreateRuntimeErrorResult(CheckPinnedDependencies, remErr)
}

// GitHub actions.
actionScore, actionErr := isGitHubActionsWorkflowPinned(c)
if actionErr != nil {
Expand Down Expand Up @@ -702,10 +706,11 @@ var validateGitHubActionWorkflow fileparser.DoWhileTrueOnFileContent = func(
if !match {
dl.Warn(&checker.LogMessage{
Path: pathfn, Type: checker.FileTypeSource,
Offset: uint(execAction.Uses.Pos.Line),
EndOffset: uint(execAction.Uses.Pos.Line), // `Uses` always span a single line.
Snippet: execAction.Uses.Value,
Text: fmt.Sprintf("%s action not pinned by hash", owner),
Offset: uint(execAction.Uses.Pos.Line),
EndOffset: uint(execAction.Uses.Pos.Line), // `Uses` always span a single line.
Snippet: execAction.Uses.Value,
Text: fmt.Sprintf("%s action not pinned by hash", owner),
Remediation: createWorkflowPinningRemediation(pathfn),
})
}

Expand Down
8 changes: 4 additions & 4 deletions checks/raw/binary_artifact.go
Expand Up @@ -95,10 +95,10 @@ var checkBinaryFileContent fileparser.DoWhileTrueOnFileContent = func(path strin
exists1 := binaryFileTypes[t.Extension]
if exists1 {
*pfiles = append(*pfiles, checker.File{
Path: path,
Type: checker.FileTypeBinary,
Offset: checker.OffsetDefault,
})
Path: path,
Type: checker.FileTypeBinary,
Offset: checker.OffsetDefault,
})
return true, nil
}

Expand Down
89 changes: 89 additions & 0 deletions checks/remediations.go
@@ -0,0 +1,89 @@
// Copyright 2022 Security 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 checks

import (
"errors"
"fmt"
"strings"
"sync"

"github.com/ossf/scorecard/v4/checker"
"github.com/ossf/scorecard/v4/clients"
)

var (
remediationBranch string
remediationRepo string
remediationOnce *sync.Once
remediationSetupErr error
)

var (
workflowText = "update your workflow using https://app.stepsecurity.io/secureworkflow/%s/%s/%s?enable=%s"
//nolint
workflowMarkdown = "update your workflow using [https://app.stepsecurity.io](https://app.stepsecurity.io/secureworkflow/%s/%s/%s?enable=%s)"
)

//nolint:gochecknoinits
func init() {
remediationOnce = new(sync.Once)
}

func remdiationSetup(c *checker.CheckRequest) error {
remediationOnce.Do(func() {
// Get the branch for remediation.
b, err := c.RepoClient.GetDefaultBranch()
if err != nil && !errors.Is(err, clients.ErrUnsupportedFeature) {
remediationSetupErr = err
return
}
if b.Name != nil {
remediationBranch = *b.Name
uri := c.Repo.URI()
parts := strings.Split(uri, "/")
if len(parts) != 3 {
remediationSetupErr = fmt.Errorf("%w: %s", errInvalidArgLength, uri)
return
}
remediationRepo = fmt.Sprintf("%s/%s", parts[1], parts[2])
}
})

return remediationSetupErr
}

func createWorkflowPermissionRemediation(filepath string) *checker.Remediation {
return createWorkflowRemediation(filepath, "permissions")
}

func createWorkflowPinningRemediation(filepath string) *checker.Remediation {
return createWorkflowRemediation(filepath, "pin")
}

func createWorkflowRemediation(path, t string) *checker.Remediation {
p := strings.TrimPrefix(path, ".github/workflows/")
if remediationBranch == "" || remediationRepo == "" {
return nil
}

text := fmt.Sprintf(workflowText, remediationRepo, p, remediationBranch, t)
markdown := fmt.Sprintf(workflowMarkdown, remediationRepo, p, remediationBranch, t)

return &checker.Remediation{
HelpText: text,
HelpMarkdown: markdown,
}
}
16 changes: 14 additions & 2 deletions pkg/sarif.go
Expand Up @@ -64,7 +64,8 @@ type location struct {
PhysicalLocation physicalLocation `json:"physicalLocation"`
//nolint
// This is optional https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#location-object.
Message *text `json:"message,omitempty"`
Message *text `json:"message,omitempty"`
HasRemediation bool `json:"-"`
}

//nolint
Expand Down Expand Up @@ -300,6 +301,12 @@ func detailsToLocations(details []checker.CheckDetail,
Message: &text{Text: d.Msg.Text},
}

// Add remediaiton information
if d.Msg.Remediation != nil {
loc.Message.Text = fmt.Sprintf("%s\nRemediation tip: %s", loc.Message.Text, d.Msg.Remediation.HelpMarkdown)
loc.HasRemediation = true
}

// Set the region depending on the file type.
loc.PhysicalLocation.Region = detailToRegion(&d)
locs = append(locs, loc)
Expand Down Expand Up @@ -428,12 +435,17 @@ func createSARIFRule(checkName, checkID, descURL, longDesc, shortDesc, risk stri
}

func createSARIFCheckResult(pos int, checkID, message string, loc *location) result {
t := fmt.Sprintf("%s\nClick Remediation section below to solve this issue", message)
if loc.HasRemediation {
t = fmt.Sprintf("%s\nClick Remediation section below for further remediation help", message)
}

return result{
RuleID: checkID,
// https://github.com/microsoft/sarif-tutorials/blob/main/docs/2-Basics.md#level
// Level: scoreToLevel(minScore, score),
RuleIndex: pos,
Message: text{Text: fmt.Sprintf("%s\nClick Remediation section below to solve this issue", message)},
Message: text{Text: t},
Locations: []location{*loc},
}
}
Expand Down

0 comments on commit 8c97d46

Please sign in to comment.