diff --git a/checker/raw_result.go b/checker/raw_result.go index a1c8189c1a8c..860619da1341 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -339,88 +339,50 @@ func PermissionLocationToString(l PermissionLocation) string { } } -// PermissionAlertType represents a permission type. -type PermissionAlertType int +// PermissionType represents a permission type. +type PermissionType int const ( - // PermissionAlertTypeUndeclared an undecleared permission. - PermissionAlertTypeUndeclared = iota - // PermissionAlertTypeWrite is a permission set to `write`. - PermissionAlertTypeWrite + // PermissionTypeUndefined an undecleared permission. + PermissionTypeUndefined = iota + // PermissionTypeWrite is a permission set to `write` for a permission we consider potentially dangerous. + PermissionTypeWrite + // PermissionTypeRead is a permission set to `read`. + PermissionTypeRead + // PermissionTypeNone is a permission set to `none`. + PermissionTypeNone + // PermissionTypeOther is for other kinds of alerts, mostly to support debug messages. + // TODO: remove it once we have implemented severity (#1874). + PermissionTypeOther ) -// PermissionAlertTypeToString stringifies a PermissionAlertType. -func PermissionAlertTypeToString(t PermissionAlertType) string { +// PermissionTypeToString stringifies a PermissionType. +func PermissionTypeToString(t PermissionType) string { switch t { - case PermissionAlertTypeUndeclared: - return "undeclared" + case PermissionTypeUndefined: + return "undefined" - case PermissionAlertTypeWrite: + case PermissionTypeWrite: return "write" + case PermissionTypeRead: + return "read" + case PermissionTypeNone: + return "none" default: - return "" + return "unknown" } } // TokenPermission defines a token permission alert. //nolint type TokenPermission struct { + Type PermissionType Job *WorkflowJob Remediation *Remediation LocationType *PermissionLocation - AlertType *PermissionAlertType Name *string Value *string File *File - Log Log -} - -// LogLevel represents a log level. -// Note: may be removed once all checks are migrated. -type LogLevel int - -const ( - // LogLevelUnknown is unknown level. - // TODO: remove after migrating all checks. - LogLevelUnknown = iota - // LogLevelDebug is debug log. - LogLevelDebug - // LogLevelInfo is info log. - LogLevelInfo - // LogLevelWarn is warn log. - LogLevelWarn -) - -// Log represent a log message. -type Log struct { - Msg string - Level LogLevel -} - -// DetailToRawLog converts a CheckDetail to a raw log. -// Note: may be removed once all checks are migrated. -func DetailToRawLog(d *CheckDetail) Log { - switch d.Type { - case DetailDebug: - return Log{ - Msg: d.Msg.Text, - Level: LogLevelDebug, - } - case DetailInfo: - return Log{ - Msg: d.Msg.Text, - Level: LogLevelInfo, - } - case DetailWarn: - return Log{ - Msg: d.Msg.Text, - Level: LogLevelWarn, - } - default: - return Log{ - Msg: d.Msg.Text, - Level: LogLevelUnknown, - } - } + Msg *string } diff --git a/checks/evaluation/permissions.go b/checks/evaluation/permissions.go index 893be72b6176..5dc8654ed49b 100644 --- a/checks/evaluation/permissions.go +++ b/checks/evaluation/permissions.go @@ -15,6 +15,8 @@ package evaluation import ( + "fmt" + "github.com/ossf/scorecard/v4/checker" sce "github.com/ossf/scorecard/v4/errors" ) @@ -26,7 +28,10 @@ func TokenPermissions(name string, dl checker.DetailLogger, r *checker.TokenPerm return checker.CreateRuntimeErrorResult(name, e) } - score, details := calculateScore(r) + score, err := calculateScore(r, dl) + if err != nil { + return checker.CreateRuntimeErrorResult(name, err) + } if score != checker.MaxResultScore { return checker.CreateResultWithScore(name, @@ -37,7 +42,7 @@ func TokenPermissions(name string, dl checker.DetailLogger, r *checker.TokenPerm "tokens are read-only in GitHub workflows") } -func calculateScore(results *checker.TokenPermissionsData, dl checker.DetailLogger) int { +func calculateScore(results *checker.TokenPermissionsData, dl checker.DetailLogger) (int, error) { // See list https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/. // Note: there are legitimate reasons to use some of the permissions like checks, deployments, etc. // in CI/CD systems https://docs.travis-ci.com/user/github-oauth-scopes/. @@ -53,25 +58,57 @@ func calculateScore(results *checker.TokenPermissionsData, dl checker.DetailLogg msg.Type = r.File.Type msg.Snippet = r.File.Snippet } - if r.Log.Msg != "" { - msg.Text = r.Log.Msg + text, err := createMessage(r) + if err != nil { + return checker.MinResultScore, err } + msg.Text = text - switch r.Log.Level { - case checker.LogLevelDebug: + switch r.Type { + case checker.PermissionTypeOther: dl.Debug(&msg) - case checker.LogLevelInfo: + case checker.PermissionTypeNone, checker.PermissionTypeRead: dl.Info(&msg) - case checker.LogLevelWarn: - + case checker.PermissionTypeWrite: + dl.Warn(&msg) // TODO: construct a hash map indexed by workflow file. } } // TODO: use the hash map to compute the score. - return 10 + return int(score) - 1, nil +} + +func createMessage(t checker.TokenPermission) (string, error) { + // By default, use the message already present. + if t.Msg != nil { + return *t.Msg, nil + } + + // Ensure there's no implementation bug. + if t.LocationType == nil { + return "", sce.WithMessage(sce.ErrScorecardInternal, "locationType is nil") + } + + // Use a different message depending on the type. + switch t.Type { + case checker.PermissionTypeUndefined: + return fmt.Sprintf("no %s permission defined", + checker.PermissionLocationToString(*t.LocationType)), nil + + default: + if t.Name == nil || t.Value == nil { + return "", sce.WithMessage(sce.ErrScorecardInternal, + fmt.Sprintf("nil fields: %v, %v", + t.Name, t.Value)) + } + + return fmt.Sprintf("%s '%v' permission set to '%v'", + checker.PermissionLocationToString(*t.LocationType), + *t.Name, *t.Value), nil + } } /* diff --git a/checks/raw/permissions.go b/checks/raw/permissions.go index 82204f7978de..410e38cc352c 100644 --- a/checks/raw/permissions.go +++ b/checks/raw/permissions.go @@ -130,15 +130,6 @@ var validateGitHubActionTokenPermissions fileparser.DoWhileTrueOnFileContent = f return false, err } - // Extract the logs. - logs := dl.Flush() - for _, l := range logs { - pdata.results.TokenPermissions = append(pdata.results.TokenPermissions, - checker.TokenPermission{ - Log: checker.DetailToRawLog(&l), - }) - } - // TODO(laurent): 2. Identify github actions that require write and add checks. // TODO(laurent): 3. Read a few runs and ensures they have the same permissions. @@ -170,11 +161,7 @@ func validatePermission(permissionKey permission, permissionValue *actionlint.Pe Name: &key, Value: &val, Remediation: createWorkflowPermissionRemediation(path), - Log: checker.Log{ - Level: checker.LogLevelWarn, - Msg: fmt.Sprintf("%s '%v' permission set to '%v'", - checker.PermissionLocationToString(permLoc), key, val), - }, + Type: checker.PermissionTypeWrite, // TODO: Job }) recordPermissionWrite(pPermissions, permissionKey) @@ -192,11 +179,8 @@ func validatePermission(permissionKey permission, permissionValue *actionlint.Pe LocationType: &permLoc, Name: &key, Value: &val, - Log: checker.Log{ - Level: checker.LogLevelDebug, - Msg: fmt.Sprintf("%s '%v' permission set to '%v'", - checker.PermissionLocationToString(permLoc), key, val), - }, + // It's a write but not considered dangerous. + Type: checker.PermissionTypeOther, // TODO: Job }) } @@ -214,16 +198,22 @@ func validatePermission(permissionKey permission, permissionValue *actionlint.Pe LocationType: &permLoc, Name: &key, Value: &val, - Log: checker.Log{ - Level: checker.LogLevelInfo, - Msg: fmt.Sprintf("%s '%v' permission set to '%v'", - checker.PermissionLocationToString(permLoc), key, val), - }, + Type: typeOfPermission(val), // TODO: Job }) return nil } +func typeOfPermission(val string) checker.PermissionType { + switch val { + case "read", "read-all": + return checker.PermissionTypeRead + case "none": + return checker.PermissionTypeNone + } + return checker.PermissionTypeOther +} + func validateMapPermissions(scopes map[string]*actionlint.PermissionScope, permLoc checker.PermissionLocation, path string, pdata *permissionCbData, pPermissions map[permission]bool, ignoredPermissions map[permission]bool, @@ -276,12 +266,8 @@ func validatePermissions(permissions *actionlint.Permissions, permLoc checker.Pe Offset: checker.OffsetDefault, }, LocationType: &permLoc, - Log: checker.Log{ - Level: checker.LogLevelInfo, - Msg: fmt.Sprintf("%s permissions set to 'none'", - checker.PermissionLocationToString(permLoc)), - }, - Value: &none, + Type: checker.PermissionTypeNone, + Value: &none, // TODO: Job, etc. }) } @@ -300,11 +286,7 @@ func validatePermissions(permissions *actionlint.Permissions, permLoc checker.Pe LocationType: &permLoc, Value: &val, Remediation: createWorkflowPermissionRemediation(path), - Log: checker.Log{ - Level: checker.LogLevelWarn, - Msg: fmt.Sprintf("%s permissions set to '%v'", - checker.PermissionLocationToString(permLoc), val), - }, + Type: checker.PermissionTypeWrite, // TODO: Job }) @@ -322,11 +304,7 @@ func validatePermissions(permissions *actionlint.Permissions, permLoc checker.Pe }, LocationType: &permLoc, Value: &val, - Log: checker.Log{ - Level: checker.LogLevelInfo, - Msg: fmt.Sprintf("%s permissions set to '%v'", - checker.PermissionLocationToString(permLoc), val), - }, + Type: typeOfPermission(val), // TODO: Job }) } else /* scopeIsSet == true */ if err := validateMapPermissions(permissions.Scopes, @@ -342,7 +320,6 @@ func validateTopLevelPermissions(workflow *actionlint.Workflow, path string, // Check if permissions are set explicitly. if workflow.Permissions == nil { var permLoc checker.PermissionLocation = checker.PermissionLocationTop - var alertType checker.PermissionAlertType = checker.PermissionAlertTypeUndeclared pdata.results.TokenPermissions = append(pdata.results.TokenPermissions, checker.TokenPermission{ File: &checker.File{ @@ -351,12 +328,8 @@ func validateTopLevelPermissions(workflow *actionlint.Workflow, path string, Offset: checker.OffsetDefault, }, LocationType: &permLoc, - AlertType: &alertType, + Type: checker.PermissionTypeUndefined, Remediation: createWorkflowPermissionRemediation(path), - Log: checker.Log{ - Level: checker.LogLevelWarn, - Msg: fmt.Sprintf("no %s permission defined", checker.PermissionLocationToString(permLoc)), - }, // TODO: Job }) @@ -378,7 +351,6 @@ func validatejobLevelPermissions(workflow *actionlint.Workflow, path string, // so only top-level read-only permissions need to be declared. if job.Permissions == nil { var permLoc checker.PermissionLocation = checker.PermissionLocationJob - var alertType checker.PermissionAlertType = checker.PermissionAlertTypeUndeclared pdata.results.TokenPermissions = append(pdata.results.TokenPermissions, checker.TokenPermission{ File: &checker.File{ @@ -387,11 +359,8 @@ func validatejobLevelPermissions(workflow *actionlint.Workflow, path string, Offset: fileparser.GetLineNumber(job.Pos), }, LocationType: &permLoc, - AlertType: &alertType, - Log: checker.Log{ - Level: checker.LogLevelDebug, - Msg: fmt.Sprintf("no %s permission defined", checker.PermissionLocationToString(permLoc)), - }, + Type: checker.PermissionTypeOther, + Msg: stringPointer(fmt.Sprintf("no %s permission defined", jobLevelPermission)), // TODO: Job }) @@ -476,10 +445,8 @@ func isSARIFUploadAction(workflow *actionlint.Workflow, fp string, pdata *permis Offset: fileparser.GetLineNumber(uses.Pos), // TODO: set Snippet. }, - Log: checker.Log{ - Level: checker.LogLevelDebug, - Msg: "codeql SARIF upload workflow detected", - }, + Type: checker.PermissionTypeOther, + Msg: stringPointer("codeql SARIF upload workflow detected"), // TODO: Job }) @@ -494,10 +461,9 @@ func isSARIFUploadAction(workflow *actionlint.Workflow, fp string, pdata *permis Type: checker.FileTypeSource, Offset: checker.OffsetDefault, }, - Log: checker.Log{ - Level: checker.LogLevelDebug, - Msg: "not a codeql upload SARIF workflow", - }, + Type: checker.PermissionTypeOther, + Msg: stringPointer("not a codeql upload SARIF workflow"), + // TODO: Job }) @@ -523,10 +489,8 @@ func isCodeQlAnalysisWorkflow(workflow *actionlint.Workflow, fp string, pdata *p Type: checker.FileTypeSource, Offset: fileparser.GetLineNumber(uses.Pos), }, - Log: checker.Log{ - Level: checker.LogLevelDebug, - Msg: "codeql workflow detected", - }, + Type: checker.PermissionTypeOther, + Msg: stringPointer("codeql workflow detected"), // TODO: Job }) @@ -542,15 +506,17 @@ func isCodeQlAnalysisWorkflow(workflow *actionlint.Workflow, fp string, pdata *p Type: checker.FileTypeSource, Offset: checker.OffsetDefault, }, - Log: checker.Log{ - Level: checker.LogLevelDebug, - Msg: "not a codeql workflow", - }, + Type: checker.PermissionTypeOther, + Msg: stringPointer("not a codeql workflow"), }) return false } +func stringPointer(s string) *string { + return &s +} + // A packaging workflow using GitHub's supported packages: // https://docs.github.com/en/packages. func requiresPackagesPermissions(workflow *actionlint.Workflow, fp string, dl checker.DetailLogger) bool {