From de6cfd169f5d398e19d9bdb6f565159744ad4b5c Mon Sep 17 00:00:00 2001 From: AdamKorcz Date: Tue, 26 Sep 2023 19:50:07 +0100 Subject: [PATCH 01/17] :seedling: convert Webhook check to probes Signed-off-by: AdamKorcz --- checker/check_result_test.go | 8 ++ checks/evaluation/webhooks.go | 44 +++--- checks/evaluation/webhooks_test.go | 136 +++++++----------- checks/webhook.go | 27 ++-- probes/entries.go | 4 + probes/webhooksWithoutTokenAuth/def.yml | 33 +++++ probes/webhooksWithoutTokenAuth/impl.go | 88 ++++++++++++ probes/webhooksWithoutTokenAuth/impl_test.go | 138 +++++++++++++++++++ 8 files changed, 353 insertions(+), 125 deletions(-) create mode 100644 probes/webhooksWithoutTokenAuth/def.yml create mode 100644 probes/webhooksWithoutTokenAuth/impl.go create mode 100644 probes/webhooksWithoutTokenAuth/impl_test.go diff --git a/checker/check_result_test.go b/checker/check_result_test.go index cf792232704..2a6f93b5434 100644 --- a/checker/check_result_test.go +++ b/checker/check_result_test.go @@ -127,6 +127,14 @@ func TestCreateProportionalScore(t *testing.T) { }, want: 5, }, + { + name: "2 and 5", + args: args{ + success: 2, + total: 5, + }, + want: 4, + }, } for _, tt := range tests { tt := tt diff --git a/checks/evaluation/webhooks.go b/checks/evaluation/webhooks.go index d7a5a761923..b0d111cd30b 100644 --- a/checks/evaluation/webhooks.go +++ b/checks/evaluation/webhooks.go @@ -15,47 +15,39 @@ package evaluation import ( - "fmt" - "github.com/ossf/scorecard/v4/checker" sce "github.com/ossf/scorecard/v4/errors" "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/webhooksWithoutTokenAuth" ) // Webhooks applies the score policy for the Webhooks check. -func Webhooks(name string, dl checker.DetailLogger, - r *checker.WebhooksData, +func Webhooks(name string, + findings []finding.Finding, dl checker.DetailLogger, ) checker.CheckResult { - if r == nil { - e := sce.WithMessage(sce.ErrScorecardInternal, "empty raw data") - return checker.CreateRuntimeErrorResult(name, e) + expectedProbes := []string{ + webhooksWithoutTokenAuth.Probe, } - if len(r.Webhooks) < 1 { - return checker.CreateMaxScoreResult(name, "no webhooks defined") + if !finding.UniqueProbesEqual(findings, expectedProbes) { + e := sce.WithMessage(sce.ErrScorecardInternal, "invalid probe results") + return checker.CreateRuntimeErrorResult(name, e) } - hasNoSecretCount := 0 - for _, hook := range r.Webhooks { - if !hook.UsesAuthSecret { - dl.Warn(&checker.LogMessage{ - Path: hook.Path, - Type: finding.FileTypeURL, - Text: "Webhook with no secret configured", - }) - hasNoSecretCount++ - } - } + totalWebhooks := findings[0].Values["totalWebhooks"] + webhooksWithoutSecret := findings[0].Values["webhooksWithoutSecret"] - if hasNoSecretCount == 0 { - return checker.CreateMaxScoreResult(name, fmt.Sprintf("all %d hook(s) have a secret configured", len(r.Webhooks))) + if findings[0].Outcome == finding.OutcomeNotApplicable { + return checker.CreateMaxScoreResult(name, "project does not have webhook") + } + if totalWebhooks == webhooksWithoutSecret { + return checker.CreateMinScoreResult(name, "no hook(s) have a secret configured") } - if len(r.Webhooks) == hasNoSecretCount { - return checker.CreateMinScoreResult(name, fmt.Sprintf("%d hook(s) do not have a secret configured", len(r.Webhooks))) + if webhooksWithoutSecret == 0 { + return checker.CreateMaxScoreResult(name, findings[0].Message) } return checker.CreateProportionalScoreResult(name, - fmt.Sprintf("%d/%d hook(s) with no secrets configured detected", - hasNoSecretCount, len(r.Webhooks)), hasNoSecretCount, len(r.Webhooks)) + findings[0].Message, webhooksWithoutSecret, totalWebhooks) } diff --git a/checks/evaluation/webhooks_test.go b/checks/evaluation/webhooks_test.go index 4a34f900f4e..0ff6bfebfa1 100644 --- a/checks/evaluation/webhooks_test.go +++ b/checks/evaluation/webhooks_test.go @@ -18,7 +18,7 @@ import ( "testing" "github.com/ossf/scorecard/v4/checker" - "github.com/ossf/scorecard/v4/clients" + "github.com/ossf/scorecard/v4/finding" scut "github.com/ossf/scorecard/v4/utests" ) @@ -32,105 +32,80 @@ func TestWebhooks(t *testing.T) { r *checker.WebhooksData } tests := []struct { - name string - args args - want checker.CheckResult - wantErr bool + name string + findings []finding.Finding + result scut.TestReturn }{ { - name: "r nil", - args: args{ - name: "test_webhook_check_pass", - dl: &scut.TestDetailLogger{}, + name: "no webhooks", + findings: []finding.Finding{ + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNotApplicable, + }, + }, + result: scut.TestReturn{ + Score: 10, }, - wantErr: true, }, { - name: "no webhooks", - args: args{ - name: "no webhooks", - dl: &scut.TestDetailLogger{}, - r: &checker.WebhooksData{}, + name: "1 webhook with no secret", + findings: []finding.Finding{ + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + }, }, - want: checker.CheckResult{ - Score: checker.MaxResultScore, + result: scut.TestReturn{ + Score: 0, }, }, { name: "1 webhook with secret", - args: args{ - name: "1 webhook with secret", - dl: &scut.TestDetailLogger{}, - r: &checker.WebhooksData{ - Webhooks: []clients.Webhook{ - { - Path: "https://github.com/owner/repo/settings/hooks/1234", - ID: 1234, - UsesAuthSecret: true, - }, + findings: []finding.Finding{ + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomePositive, + Values: map[string]int{ + "totalWebhooks": 1, + "webhooksWithoutSecret": 0, }, }, }, - want: checker.CheckResult{ + result: scut.TestReturn{ Score: 10, }, }, { - name: "1 webhook with no secret", - args: args{ - name: "1 webhook with no secret", - dl: &scut.TestDetailLogger{}, - r: &checker.WebhooksData{ - Webhooks: []clients.Webhook{ - { - Path: "https://github.com/owner/repo/settings/hooks/1234", - ID: 1234, - UsesAuthSecret: false, - }, + name: "2 webhooks one of which has secret", + findings: []finding.Finding{ + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 2, + "webhooksWithoutSecret": 1, }, }, }, - want: checker.CheckResult{ - Score: 0, + result: scut.TestReturn{ + Score: 5, }, }, { - name: "many webhooks with no secret and with secret", - args: args{ - name: "many webhooks with no secret and with secret", - dl: &scut.TestDetailLogger{}, - r: &checker.WebhooksData{ - Webhooks: []clients.Webhook{ - { - Path: "https://github.com/owner/repo/settings/hooks/1234", - ID: 1234, - UsesAuthSecret: false, - }, - { - Path: "https://github.com/owner/repo/settings/hooks/1111", - ID: 1111, - UsesAuthSecret: true, - }, - { - Path: "https://github.com/owner/repo/settings/hooks/4444", - ID: 4444, - UsesAuthSecret: true, - }, - { - Path: "https://github.com/owner/repo/settings/hooks/3333", - ID: 3333, - UsesAuthSecret: false, - }, - { - Path: "https://github.com/owner/repo/settings/hooks/2222", - ID: 2222, - UsesAuthSecret: false, - }, + name: "Five webhooks three of which have secrets", + findings: []finding.Finding{ + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 5, + "webhooksWithoutSecret": 2, }, }, }, - want: checker.CheckResult{ - Score: 6, + result: scut.TestReturn{ + Score: 4, }, }, } @@ -138,15 +113,10 @@ func TestWebhooks(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := Webhooks(tt.args.name, tt.args.dl, tt.args.r) - if tt.wantErr { - if got.Error == nil { - t.Errorf("Webhooks() error = %v, wantErr %v", got.Error, tt.wantErr) - } - } else { - if got.Score != tt.want.Score { - t.Errorf("Webhooks() = %v, want %v", got.Score, tt.want.Score) - } + dl := scut.TestDetailLogger{} + got := Webhooks(tt.name, tt.findings, &dl) + if !scut.ValidateTestReturn(t, tt.name, &tt.result, &got, &dl) { + t.Errorf("got %v, expected %v", got, tt.result) } }) } diff --git a/checks/webhook.go b/checks/webhook.go index 7a571aaced4..13f364e0cf1 100644 --- a/checks/webhook.go +++ b/checks/webhook.go @@ -15,12 +15,12 @@ package checks import ( - "os" - "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/checks/evaluation" "github.com/ossf/scorecard/v4/checks/raw" sce "github.com/ossf/scorecard/v4/errors" + "github.com/ossf/scorecard/v4/probes" + "github.com/ossf/scorecard/v4/probes/zrunner" ) const ( @@ -38,17 +38,6 @@ func init() { // WebHooks run Webhooks check. func WebHooks(c *checker.CheckRequest) checker.CheckResult { - // TODO: remove this check when v6 is released - _, enabled := os.LookupEnv("SCORECARD_EXPERIMENTAL") - if !enabled { - c.Dlogger.Warn(&checker.LogMessage{ - Text: "SCORECARD_EXPERIMENTAL is not set, not running the Webhook check", - }) - - e := sce.WithMessage(sce.ErrorUnsupportedCheck, "SCORECARD_EXPERIMENTAL is not set, not running the Webhook check") - return checker.CreateRuntimeErrorResult(CheckWebHooks, e) - } - rawData, err := raw.WebHook(c) if err != nil { e := sce.WithMessage(sce.ErrScorecardInternal, err.Error()) @@ -56,10 +45,16 @@ func WebHooks(c *checker.CheckRequest) checker.CheckResult { } // Set the raw results. - if c.RawResults != nil { - c.RawResults.WebhookResults = rawData + pRawResults := getRawResults(c) + pRawResults.WebhookResults = rawData + + // Evaluate the probes. + findings, err := zrunner.Run(pRawResults, probes.Webhook) + if err != nil { + e := sce.WithMessage(sce.ErrScorecardInternal, err.Error()) + return checker.CreateRuntimeErrorResult(CheckWebHooks, e) } // Return the score evaluation. - return evaluation.Webhooks(CheckWebHooks, c.Dlogger, &rawData) + return evaluation.Webhooks(CheckWebHooks, findings, c.Dlogger) } diff --git a/probes/entries.go b/probes/entries.go index c9d125f81e5..3baec254bdf 100644 --- a/probes/entries.go +++ b/probes/entries.go @@ -53,6 +53,7 @@ import ( "github.com/ossf/scorecard/v4/probes/toolDependabotInstalled" "github.com/ossf/scorecard/v4/probes/toolPyUpInstalled" "github.com/ossf/scorecard/v4/probes/toolRenovateInstalled" + "github.com/ossf/scorecard/v4/probes/webhooksWithoutTokenAuth" ) // ProbeImpl is the implementation of a probe. @@ -125,6 +126,9 @@ var ( BinaryArtifacts = []ProbeImpl{ freeOfUnverifiedBinaryArtifacts.Run, } + Webhook = []ProbeImpl{ + webhooksWithoutTokenAuth.Run, + } ) //nolint:gochecknoinits diff --git a/probes/webhooksWithoutTokenAuth/def.yml b/probes/webhooksWithoutTokenAuth/def.yml new file mode 100644 index 00000000000..08ba7db0e62 --- /dev/null +++ b/probes/webhooksWithoutTokenAuth/def.yml @@ -0,0 +1,33 @@ +# 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. + +id: webhooksWithoutTokenAuth +short: This check determines whether the webhook defined in the repository has a token configured to authenticate the origins of requests. +motivation: > + Webhooks without token authorization have the potential to make projects accessible to third-parties. +implementation: > + The probe checks all webhooks of a project and returns if it finds a single webhook without token authentication. +outcome: + - If the project has a single webhook without token authorization, the probe returns one OutcomeNegative (0). + - If the project does not have any webhooks without token authorization, the probe returns one OutcomePositive (1). +remediation: + effort: Low + text: + - Check whether your service supports token authentication. + - If there is support for token authentication, set the secret in the webhook configuration. + - If there is no support for token authentication, request the webhook service implement token authentication functionality. + markdown: + - Check whether your service supports token authentication. + - If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook). + - If there is no support for token authentication, request the webhook service implement token authentication functionality by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks). \ No newline at end of file diff --git a/probes/webhooksWithoutTokenAuth/impl.go b/probes/webhooksWithoutTokenAuth/impl.go new file mode 100644 index 00000000000..629d376cafd --- /dev/null +++ b/probes/webhooksWithoutTokenAuth/impl.go @@ -0,0 +1,88 @@ +// 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. + +// nolint:stylecheck +package webhooksWithoutTokenAuth + +import ( + "embed" + "fmt" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/internal/utils/uerror" +) + +//go:embed *.yml +var fs embed.FS + +const Probe = "webhooksWithoutTokenAuth" + +func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { + if raw == nil { + return nil, "", fmt.Errorf("%w: raw", uerror.ErrNil) + } + + r := raw.WebhookResults + totalWebhooks := len(r.Webhooks) + var webhooksWithoutSecret int + + var findings []finding.Finding + + if totalWebhooks == 0 { + f, err := finding.NewWith(fs, Probe, + "Repository has webhook without token authorization.", nil, + finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + return findings, Probe, nil + } + + for _, hook := range r.Webhooks { + if !hook.UsesAuthSecret { + webhooksWithoutSecret++ + } + } + + if webhooksWithoutSecret != 0 { + whWoS, tW := webhooksWithoutSecret, totalWebhooks + msg := fmt.Sprintf("Repository has %d webhooks out of %d without token authorization.", whWoS, tW) + f, err := finding.NewWith(fs, Probe, + msg, nil, finding.OutcomeNegative) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + f = f.WithValues(map[string]int{ + "totalWebhooks": totalWebhooks, + "webhooksWithoutSecret": webhooksWithoutSecret, + }) + findings = append(findings, *f) + } else { + f, err := finding.NewWith(fs, Probe, + fmt.Sprintf("All of the projects %d webhooks have token authorization.", totalWebhooks), nil, + finding.OutcomePositive) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + f = f.WithValues(map[string]int{ + "totalWebhooks": totalWebhooks, + "webhooksWithoutSecret": webhooksWithoutSecret, + }) + findings = append(findings, *f) + } + + return findings, Probe, nil +} diff --git a/probes/webhooksWithoutTokenAuth/impl_test.go b/probes/webhooksWithoutTokenAuth/impl_test.go new file mode 100644 index 00000000000..3276ce19c33 --- /dev/null +++ b/probes/webhooksWithoutTokenAuth/impl_test.go @@ -0,0 +1,138 @@ +// 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. + +// nolint:stylecheck +package webhooksWithoutTokenAuth + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/clients" + "github.com/ossf/scorecard/v4/finding" +) + +func Test_Run(t *testing.T) { + t.Parallel() + // nolint:govet + tests := []struct { + name string + raw *checker.RawResults + outcomes []finding.Outcome + err error + }{ + { + name: "No Webhooks", + raw: &checker.RawResults{ + WebhookResults: checker.WebhooksData{ + Webhooks: []clients.Webhook{}, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNotApplicable, + }, + }, + { + name: "Webhooks present with auth secret", + raw: &checker.RawResults{ + WebhookResults: checker.WebhooksData{ + Webhooks: []clients.Webhook{ + { + Path: "https://github.com/owner/repo/settings/hooks/1234", + ID: 1234, + UsesAuthSecret: true, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomePositive, + }, + }, + { + name: "Webhooks present without auth secret", + raw: &checker.RawResults{ + WebhookResults: checker.WebhooksData{ + Webhooks: []clients.Webhook{ + { + Path: "https://github.com/owner/repo/settings/hooks/1234", + ID: 1234, + UsesAuthSecret: false, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, + }, + }, + { + name: "Multiple webhooks present, one without auth secret", + raw: &checker.RawResults{ + WebhookResults: checker.WebhooksData{ + Webhooks: []clients.Webhook{ + { + Path: "https://github.com/owner/repo/settings/hooks/1234", + ID: 1234, + UsesAuthSecret: false, + }, + { + Path: "https://github.com/owner/repo/settings/hooks/12345", + ID: 1234, + UsesAuthSecret: true, + }, + { + Path: "https://github.com/owner/repo/settings/hooks/12346", + ID: 1234, + UsesAuthSecret: true, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, + }, + }, + } + 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() + + findings, s, err := Run(tt.raw) + if !cmp.Equal(tt.err, err, cmpopts.EquateErrors()) { + t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(tt.err, err, cmpopts.EquateErrors())) + } + if err != nil { + return + } + if diff := cmp.Diff(Probe, s); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(len(tt.outcomes), len(findings)); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + for i := range tt.outcomes { + outcome := &tt.outcomes[i] + f := &findings[i] + if diff := cmp.Diff(*outcome, f.Outcome); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + } + }) + } +} From 7a3d07323b64e615417db6d2cb6268a43e0dce0f Mon Sep 17 00:00:00 2001 From: AdamKorcz Date: Fri, 29 Sep 2023 12:31:09 +0100 Subject: [PATCH 02/17] Add test + nits Signed-off-by: AdamKorcz --- checks/evaluation/webhooks_test.go | 81 +++++++++++++++++++++++++ probes/webhooksWithoutTokenAuth/def.yml | 10 +-- 2 files changed, 86 insertions(+), 5 deletions(-) diff --git a/checks/evaluation/webhooks_test.go b/checks/evaluation/webhooks_test.go index 0ff6bfebfa1..4c2b4db0017 100644 --- a/checks/evaluation/webhooks_test.go +++ b/checks/evaluation/webhooks_test.go @@ -108,6 +108,87 @@ func TestWebhooks(t *testing.T) { Score: 4, }, }, + { + name: "Twelve webhooks none of which have secrets", + findings: []finding.Finding{ + { + Probe: "hasWebhooks", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasWebhooks", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasWebhooks", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasWebhooks", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasWebhooks", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasWebhooks", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasWebhooks", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasWebhooks", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasWebhooks", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasWebhooks", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasWebhooks", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasWebhooks", + Outcome: finding.OutcomePositive, + }, { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + }, { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + }, { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + }, { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + }, { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + }, { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + }, { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + }, { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + }, { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + }, { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + }, { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + }, { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + }, + }, + result: scut.TestReturn{ + Score: 0, + }, + }, } for _, tt := range tests { tt := tt diff --git a/probes/webhooksWithoutTokenAuth/def.yml b/probes/webhooksWithoutTokenAuth/def.yml index 08ba7db0e62..bca30ca24e6 100644 --- a/probes/webhooksWithoutTokenAuth/def.yml +++ b/probes/webhooksWithoutTokenAuth/def.yml @@ -13,20 +13,20 @@ # limitations under the License. id: webhooksWithoutTokenAuth -short: This check determines whether the webhook defined in the repository has a token configured to authenticate the origins of requests. +short: This check determines whether webhooks defined in the repository use a token to authenticate to remote servers. motivation: > Webhooks without token authorization have the potential to make projects accessible to third-parties. implementation: > - The probe checks all webhooks of a project and returns if it finds a single webhook without token authentication. + The probe checks all webhooks of a project and checks whether each uses token authentication. outcome: - - If the project has a single webhook without token authorization, the probe returns one OutcomeNegative (0). + - If the project has one or more webhooks without token authorization, the probe returns as many OutcomeNegative (0) as the project has webhooks without token authorization. - If the project does not have any webhooks without token authorization, the probe returns one OutcomePositive (1). remediation: effort: Low text: - Check whether your service supports token authentication. - - If there is support for token authentication, set the secret in the webhook configuration. - - If there is no support for token authentication, request the webhook service implement token authentication functionality. + - If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook). + - If there is no support for token authentication, request the webhook service implement token authentication functionality by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks). markdown: - Check whether your service supports token authentication. - If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook). From 18904d0915f38caa95e2bcaebf120d7c4b109e53 Mon Sep 17 00:00:00 2001 From: AdamKorcz Date: Tue, 14 Nov 2023 21:37:48 +0000 Subject: [PATCH 03/17] replace probe with OutcomeNotApplicable Signed-off-by: AdamKorcz --- checks/evaluation/webhooks_test.go | 73 ++----------------------- probes/webhooksWithoutTokenAuth/impl.go | 4 +- 2 files changed, 6 insertions(+), 71 deletions(-) diff --git a/checks/evaluation/webhooks_test.go b/checks/evaluation/webhooks_test.go index 4c2b4db0017..5c6f3947eea 100644 --- a/checks/evaluation/webhooks_test.go +++ b/checks/evaluation/webhooks_test.go @@ -112,77 +112,12 @@ func TestWebhooks(t *testing.T) { name: "Twelve webhooks none of which have secrets", findings: []finding.Finding{ { - Probe: "hasWebhooks", - Outcome: finding.OutcomePositive, - }, { - Probe: "hasWebhooks", - Outcome: finding.OutcomePositive, - }, { - Probe: "hasWebhooks", - Outcome: finding.OutcomePositive, - }, { - Probe: "hasWebhooks", - Outcome: finding.OutcomePositive, - }, { - Probe: "hasWebhooks", - Outcome: finding.OutcomePositive, - }, { - Probe: "hasWebhooks", - Outcome: finding.OutcomePositive, - }, { - Probe: "hasWebhooks", - Outcome: finding.OutcomePositive, - }, { - Probe: "hasWebhooks", - Outcome: finding.OutcomePositive, - }, { - Probe: "hasWebhooks", - Outcome: finding.OutcomePositive, - }, { - Probe: "hasWebhooks", - Outcome: finding.OutcomePositive, - }, { - Probe: "hasWebhooks", - Outcome: finding.OutcomePositive, - }, { - Probe: "hasWebhooks", - Outcome: finding.OutcomePositive, - }, { - Probe: "webhooksWithoutTokenAuth", - Outcome: finding.OutcomeNegative, - }, { - Probe: "webhooksWithoutTokenAuth", - Outcome: finding.OutcomeNegative, - }, { - Probe: "webhooksWithoutTokenAuth", - Outcome: finding.OutcomeNegative, - }, { - Probe: "webhooksWithoutTokenAuth", - Outcome: finding.OutcomeNegative, - }, { - Probe: "webhooksWithoutTokenAuth", - Outcome: finding.OutcomeNegative, - }, { - Probe: "webhooksWithoutTokenAuth", - Outcome: finding.OutcomeNegative, - }, { - Probe: "webhooksWithoutTokenAuth", - Outcome: finding.OutcomeNegative, - }, { - Probe: "webhooksWithoutTokenAuth", - Outcome: finding.OutcomeNegative, - }, { - Probe: "webhooksWithoutTokenAuth", - Outcome: finding.OutcomeNegative, - }, { - Probe: "webhooksWithoutTokenAuth", - Outcome: finding.OutcomeNegative, - }, { - Probe: "webhooksWithoutTokenAuth", - Outcome: finding.OutcomeNegative, - }, { Probe: "webhooksWithoutTokenAuth", Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 12, + "webhooksWithoutSecret": 12, + }, }, }, result: scut.TestReturn{ diff --git a/probes/webhooksWithoutTokenAuth/impl.go b/probes/webhooksWithoutTokenAuth/impl.go index 629d376cafd..65ae795f559 100644 --- a/probes/webhooksWithoutTokenAuth/impl.go +++ b/probes/webhooksWithoutTokenAuth/impl.go @@ -40,9 +40,9 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { var findings []finding.Finding - if totalWebhooks == 0 { + if len(r.Webhooks) == 0 { f, err := finding.NewWith(fs, Probe, - "Repository has webhook without token authorization.", nil, + "Repository does not have webhooks.", nil, finding.OutcomeNotApplicable) if err != nil { return nil, Probe, fmt.Errorf("create finding: %w", err) From ff38b16c6aad0f0807c1885fec2838991c43ee9b Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Tue, 28 Nov 2023 10:40:24 +0000 Subject: [PATCH 04/17] return one finding per webhook Signed-off-by: Adam Korczynski --- checks/evaluation/webhooks.go | 26 ++++- checks/evaluation/webhooks_test.go | 115 +++++++++++++++++-- probes/webhooksWithoutTokenAuth/def.yml | 2 +- probes/webhooksWithoutTokenAuth/impl.go | 57 +++++---- probes/webhooksWithoutTokenAuth/impl_test.go | 45 +++++++- 5 files changed, 192 insertions(+), 53 deletions(-) diff --git a/checks/evaluation/webhooks.go b/checks/evaluation/webhooks.go index b0d111cd30b..966e6c66c6f 100644 --- a/checks/evaluation/webhooks.go +++ b/checks/evaluation/webhooks.go @@ -15,6 +15,8 @@ package evaluation import ( + "fmt" + "github.com/ossf/scorecard/v4/checker" sce "github.com/ossf/scorecard/v4/errors" "github.com/ossf/scorecard/v4/finding" @@ -34,20 +36,34 @@ func Webhooks(name string, return checker.CreateRuntimeErrorResult(name, e) } + if len(findings) == 1 && findings[0].Outcome == finding.OutcomeNotApplicable { + return checker.CreateMaxScoreResult(name, "project does not have webhook") + } + + var webhooksWithoutSecret int + totalWebhooks := findings[0].Values["totalWebhooks"] - webhooksWithoutSecret := findings[0].Values["webhooksWithoutSecret"] - if findings[0].Outcome == finding.OutcomeNotApplicable { - return checker.CreateMaxScoreResult(name, "project does not have webhook") + for i := range findings { + f := &findings[i] + if f.Outcome == finding.OutcomeNegative { + webhooksWithoutSecret++ + } } + if totalWebhooks == webhooksWithoutSecret { return checker.CreateMinScoreResult(name, "no hook(s) have a secret configured") } if webhooksWithoutSecret == 0 { - return checker.CreateMaxScoreResult(name, findings[0].Message) + msg := fmt.Sprintf("All %d of the projects webhooks are configure with a secret", totalWebhooks) + return checker.CreateMaxScoreResult(name, msg) } + msg := fmt.Sprintf("%d out of the projects %d webhooks are configure without a secret", + webhooksWithoutSecret, + totalWebhooks) + return checker.CreateProportionalScoreResult(name, - findings[0].Message, webhooksWithoutSecret, totalWebhooks) + msg, totalWebhooks-webhooksWithoutSecret, totalWebhooks) } diff --git a/checks/evaluation/webhooks_test.go b/checks/evaluation/webhooks_test.go index 5c6f3947eea..71937877a90 100644 --- a/checks/evaluation/webhooks_test.go +++ b/checks/evaluation/webhooks_test.go @@ -67,8 +67,7 @@ func TestWebhooks(t *testing.T) { Probe: "webhooksWithoutTokenAuth", Outcome: finding.OutcomePositive, Values: map[string]int{ - "totalWebhooks": 1, - "webhooksWithoutSecret": 0, + "totalWebhooks": 1, }, }, }, @@ -83,8 +82,7 @@ func TestWebhooks(t *testing.T) { Probe: "webhooksWithoutTokenAuth", Outcome: finding.OutcomeNegative, Values: map[string]int{ - "totalWebhooks": 2, - "webhooksWithoutSecret": 1, + "totalWebhooks": 2, }, }, }, @@ -99,24 +97,121 @@ func TestWebhooks(t *testing.T) { Probe: "webhooksWithoutTokenAuth", Outcome: finding.OutcomeNegative, Values: map[string]int{ - "totalWebhooks": 5, - "webhooksWithoutSecret": 2, + "totalWebhooks": 5, + }, + }, + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 5, }, }, }, result: scut.TestReturn{ - Score: 4, + Score: 6, }, }, { - name: "Twelve webhooks none of which have secrets", + name: "One of 12 webhooks does not have secrets", findings: []finding.Finding{ { Probe: "webhooksWithoutTokenAuth", Outcome: finding.OutcomeNegative, Values: map[string]int{ - "totalWebhooks": 12, - "webhooksWithoutSecret": 12, + "totalWebhooks": 12, + }, + }, + }, + result: scut.TestReturn{ + Score: 9, + }, + }, + { + name: "12/12 webhooks do not have secrets", + findings: []finding.Finding{ + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 12, + }, + }, + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 12, + }, + }, + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 12, + }, + }, + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 12, + }, + }, + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 12, + }, + }, + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 12, + }, + }, + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 12, + }, + }, + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 12, + }, + }, + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 12, + }, + }, + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 12, + }, + }, + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 12, + }, + }, + { + Probe: "webhooksWithoutTokenAuth", + Outcome: finding.OutcomeNegative, + Values: map[string]int{ + "totalWebhooks": 12, }, }, }, diff --git a/probes/webhooksWithoutTokenAuth/def.yml b/probes/webhooksWithoutTokenAuth/def.yml index bca30ca24e6..6f0c0f2d25b 100644 --- a/probes/webhooksWithoutTokenAuth/def.yml +++ b/probes/webhooksWithoutTokenAuth/def.yml @@ -19,7 +19,7 @@ motivation: > implementation: > The probe checks all webhooks of a project and checks whether each uses token authentication. outcome: - - If the project has one or more webhooks without token authorization, the probe returns as many OutcomeNegative (0) as the project has webhooks without token authorization. + - If the project has any webhooks without token authorization, the probe returns as many OutcomeNegative (0) as the project has webhooks without token authorization. All findings include the value "totalWebhooks" which is the total number of webhooks that the project has. The finding also includes the path to the webhook. - If the project does not have any webhooks without token authorization, the probe returns one OutcomePositive (1). remediation: effort: Low diff --git a/probes/webhooksWithoutTokenAuth/impl.go b/probes/webhooksWithoutTokenAuth/impl.go index 65ae795f559..6a07ceb9e13 100644 --- a/probes/webhooksWithoutTokenAuth/impl.go +++ b/probes/webhooksWithoutTokenAuth/impl.go @@ -36,8 +36,6 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { r := raw.WebhookResults totalWebhooks := len(r.Webhooks) - var webhooksWithoutSecret int - var findings []finding.Finding if len(r.Webhooks) == 0 { @@ -52,36 +50,33 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { } for _, hook := range r.Webhooks { - if !hook.UsesAuthSecret { - webhooksWithoutSecret++ - } - } - - if webhooksWithoutSecret != 0 { - whWoS, tW := webhooksWithoutSecret, totalWebhooks - msg := fmt.Sprintf("Repository has %d webhooks out of %d without token authorization.", whWoS, tW) - f, err := finding.NewWith(fs, Probe, - msg, nil, finding.OutcomeNegative) - if err != nil { - return nil, Probe, fmt.Errorf("create finding: %w", err) + if hook.UsesAuthSecret { + msg := "Webhook with token authorization found." + f, err := finding.NewWith(fs, Probe, + msg, nil, finding.OutcomePositive) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + f = f.WithValues(map[string]int{ + "totalWebhooks": totalWebhooks, + }).WithLocation(&finding.Location{ + Path: hook.Path, + }) + findings = append(findings, *f) + } else { + msg := "Webhook without token authorization found." + f, err := finding.NewWith(fs, Probe, + msg, nil, finding.OutcomeNegative) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + f = f.WithValues(map[string]int{ + "totalWebhooks": totalWebhooks, + }).WithLocation(&finding.Location{ + Path: hook.Path, + }) + findings = append(findings, *f) } - f = f.WithValues(map[string]int{ - "totalWebhooks": totalWebhooks, - "webhooksWithoutSecret": webhooksWithoutSecret, - }) - findings = append(findings, *f) - } else { - f, err := finding.NewWith(fs, Probe, - fmt.Sprintf("All of the projects %d webhooks have token authorization.", totalWebhooks), nil, - finding.OutcomePositive) - if err != nil { - return nil, Probe, fmt.Errorf("create finding: %w", err) - } - f = f.WithValues(map[string]int{ - "totalWebhooks": totalWebhooks, - "webhooksWithoutSecret": webhooksWithoutSecret, - }) - findings = append(findings, *f) } return findings, Probe, nil diff --git a/probes/webhooksWithoutTokenAuth/impl_test.go b/probes/webhooksWithoutTokenAuth/impl_test.go index 3276ce19c33..e30ec9d8c36 100644 --- a/probes/webhooksWithoutTokenAuth/impl_test.go +++ b/probes/webhooksWithoutTokenAuth/impl_test.go @@ -53,7 +53,7 @@ func Test_Run(t *testing.T) { Webhooks: []clients.Webhook{ { Path: "https://github.com/owner/repo/settings/hooks/1234", - ID: 1234, + ID: 1, UsesAuthSecret: true, }, }, @@ -70,7 +70,7 @@ func Test_Run(t *testing.T) { Webhooks: []clients.Webhook{ { Path: "https://github.com/owner/repo/settings/hooks/1234", - ID: 1234, + ID: 1, UsesAuthSecret: false, }, }, @@ -87,24 +87,57 @@ func Test_Run(t *testing.T) { Webhooks: []clients.Webhook{ { Path: "https://github.com/owner/repo/settings/hooks/1234", - ID: 1234, + ID: 1, UsesAuthSecret: false, }, { Path: "https://github.com/owner/repo/settings/hooks/12345", - ID: 1234, + ID: 2, UsesAuthSecret: true, }, { Path: "https://github.com/owner/repo/settings/hooks/12346", - ID: 1234, + ID: 3, UsesAuthSecret: true, }, }, }, }, outcomes: []finding.Outcome{ - finding.OutcomeNegative, + finding.OutcomeNegative, finding.OutcomePositive, finding.OutcomePositive, + }, + }, + { + name: "Multiple webhooks present, two without auth secret", + raw: &checker.RawResults{ + WebhookResults: checker.WebhooksData{ + Webhooks: []clients.Webhook{ + { + Path: "https://github.com/owner/repo/settings/hooks/1234", + ID: 1, + UsesAuthSecret: false, + }, + { + Path: "https://github.com/owner/repo/settings/hooks/12345", + ID: 2, + UsesAuthSecret: true, + }, + { + Path: "https://github.com/owner/repo/settings/hooks/12346", + ID: 3, + UsesAuthSecret: true, + }, + { + Path: "https://github.com/owner/repo/settings/hooks/12346", + ID: 4, + UsesAuthSecret: false, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, finding.OutcomePositive, + finding.OutcomePositive, finding.OutcomeNegative, }, }, } From 7dd8812b3769bea7749467d8dde24f6b50d16f01 Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Tue, 28 Nov 2023 10:45:07 +0000 Subject: [PATCH 05/17] change wording in def.yml Signed-off-by: Adam Korczynski --- probes/webhooksWithoutTokenAuth/def.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/probes/webhooksWithoutTokenAuth/def.yml b/probes/webhooksWithoutTokenAuth/def.yml index 6f0c0f2d25b..630370603dc 100644 --- a/probes/webhooksWithoutTokenAuth/def.yml +++ b/probes/webhooksWithoutTokenAuth/def.yml @@ -13,7 +13,7 @@ # limitations under the License. id: webhooksWithoutTokenAuth -short: This check determines whether webhooks defined in the repository use a token to authenticate to remote servers. +short: This check determines whether the webhooks defined in the repository have tokens configured to authenticate the origins of requests. motivation: > Webhooks without token authorization have the potential to make projects accessible to third-parties. implementation: > From 8030d20a2840beab7152bb98a01c7c60a1b36d1f Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Tue, 28 Nov 2023 10:48:57 +0000 Subject: [PATCH 06/17] change wording in def.yml and checks.md Signed-off-by: Adam Korczynski --- docs/checks.md | 2 +- probes/webhooksWithoutTokenAuth/def.yml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/checks.md b/docs/checks.md index 9a8fa379866..8d049df23ed 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -678,7 +678,7 @@ This check determines whether the webhook defined in the repository has a token **Remediation steps** -- Check whether your service supports token authentication. +- Check if the service your webhooks is configured with supports secrets. - If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook). - If there is no support for token authentication, request the webhook service implement token authentication functionality by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks). diff --git a/probes/webhooksWithoutTokenAuth/def.yml b/probes/webhooksWithoutTokenAuth/def.yml index 630370603dc..d892bd6de39 100644 --- a/probes/webhooksWithoutTokenAuth/def.yml +++ b/probes/webhooksWithoutTokenAuth/def.yml @@ -24,10 +24,10 @@ outcome: remediation: effort: Low text: - - Check whether your service supports token authentication. + - Check if the service your webhooks is configured with supports secrets. - If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook). - If there is no support for token authentication, request the webhook service implement token authentication functionality by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks). markdown: - - Check whether your service supports token authentication. + - Check if the service your webhooks is configured with supports secrets. - If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook). - If there is no support for token authentication, request the webhook service implement token authentication functionality by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks). \ No newline at end of file From fb3bf0f3fa5fbff8077d7ddc5659de4a44c0c0b6 Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Tue, 28 Nov 2023 11:08:15 +0000 Subject: [PATCH 07/17] remove unused struct in test Signed-off-by: Adam Korczynski --- checks/evaluation/webhooks_test.go | 7 ------- probes/webhooksWithoutTokenAuth/impl.go | 2 +- probes/webhooksWithoutTokenAuth/impl_test.go | 4 ++-- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/checks/evaluation/webhooks_test.go b/checks/evaluation/webhooks_test.go index 71937877a90..849bcd519d4 100644 --- a/checks/evaluation/webhooks_test.go +++ b/checks/evaluation/webhooks_test.go @@ -17,7 +17,6 @@ package evaluation import ( "testing" - "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/finding" scut "github.com/ossf/scorecard/v4/utests" ) @@ -25,12 +24,6 @@ import ( // TestWebhooks tests the webhooks check. func TestWebhooks(t *testing.T) { t.Parallel() - //nolint:govet - type args struct { - name string - dl checker.DetailLogger - r *checker.WebhooksData - } tests := []struct { name string findings []finding.Finding diff --git a/probes/webhooksWithoutTokenAuth/impl.go b/probes/webhooksWithoutTokenAuth/impl.go index 6a07ceb9e13..4856b9cb420 100644 --- a/probes/webhooksWithoutTokenAuth/impl.go +++ b/probes/webhooksWithoutTokenAuth/impl.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// nolint:stylecheck +//nolint:stylecheck package webhooksWithoutTokenAuth import ( diff --git a/probes/webhooksWithoutTokenAuth/impl_test.go b/probes/webhooksWithoutTokenAuth/impl_test.go index e30ec9d8c36..18be86cb718 100644 --- a/probes/webhooksWithoutTokenAuth/impl_test.go +++ b/probes/webhooksWithoutTokenAuth/impl_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// nolint:stylecheck +//nolint:stylecheck package webhooksWithoutTokenAuth import ( @@ -28,7 +28,7 @@ import ( func Test_Run(t *testing.T) { t.Parallel() - // nolint:govet + //nolint:govet tests := []struct { name string raw *checker.RawResults From eab68730632dc530b694532c66df40b49478715e Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Tue, 28 Nov 2023 11:10:54 +0000 Subject: [PATCH 08/17] align checks.md with checks.yaml Signed-off-by: Adam Korczynski --- docs/checks/internal/checks.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 2ec49840581..7dabefcc5e8 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -826,7 +826,7 @@ checks: This check determines whether the webhook defined in the repository has a token configured to authenticate the origins of requests. remediation: - >- - Check whether your service supports token authentication. + Check if the service your webhooks is configured with supports secrets. - >- If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook). - >- From 147cb8bece648d51bf4fb119ecc2ca8a99eba243 Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Tue, 28 Nov 2023 11:19:06 +0000 Subject: [PATCH 09/17] bring back experimental for webhooks Signed-off-by: Adam Korczynski --- checks/webhook.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/checks/webhook.go b/checks/webhook.go index 13f364e0cf1..cc3284560f7 100644 --- a/checks/webhook.go +++ b/checks/webhook.go @@ -15,6 +15,8 @@ package checks import ( + "os" + "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/checks/evaluation" "github.com/ossf/scorecard/v4/checks/raw" @@ -38,6 +40,17 @@ func init() { // WebHooks run Webhooks check. func WebHooks(c *checker.CheckRequest) checker.CheckResult { + // TODO: remove this check when v6 is released + _, enabled := os.LookupEnv("SCORECARD_EXPERIMENTAL") + if !enabled { + c.Dlogger.Warn(&checker.LogMessage{ + Text: "SCORECARD_EXPERIMENTAL is not set, not running the Webhook check", + }) + + e := sce.WithMessage(sce.ErrorUnsupportedCheck, "SCORECARD_EXPERIMENTAL is not set, not running the Webhook check") + return checker.CreateRuntimeErrorResult(CheckWebHooks, e) + } + rawData, err := raw.WebHook(c) if err != nil { e := sce.WithMessage(sce.ErrScorecardInternal, err.Error()) From d8476bd4b1f448bd67e3af67c8f577cb01b73dd2 Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Tue, 28 Nov 2023 11:33:34 +0000 Subject: [PATCH 10/17] change 'token' to 'secret' in probe Signed-off-by: Adam Korczynski --- checks/evaluation/webhooks.go | 16 ++++---- checks/evaluation/webhooks_test.go | 38 +++++++++---------- probes/entries.go | 4 +- probes/webhooksWithoutSecret/def.yml | 33 ++++++++++++++++ .../impl.go | 4 +- .../impl_test.go | 2 +- probes/webhooksWithoutTokenAuth/def.yml | 33 ---------------- 7 files changed, 65 insertions(+), 65 deletions(-) create mode 100644 probes/webhooksWithoutSecret/def.yml rename probes/{webhooksWithoutTokenAuth => webhooksWithoutSecret}/impl.go (96%) rename probes/{webhooksWithoutTokenAuth => webhooksWithoutSecret}/impl_test.go (99%) delete mode 100644 probes/webhooksWithoutTokenAuth/def.yml diff --git a/checks/evaluation/webhooks.go b/checks/evaluation/webhooks.go index 966e6c66c6f..4e8e5b94f17 100644 --- a/checks/evaluation/webhooks.go +++ b/checks/evaluation/webhooks.go @@ -20,7 +20,7 @@ import ( "github.com/ossf/scorecard/v4/checker" sce "github.com/ossf/scorecard/v4/errors" "github.com/ossf/scorecard/v4/finding" - "github.com/ossf/scorecard/v4/probes/webhooksWithoutTokenAuth" + "github.com/ossf/scorecard/v4/probes/webhooksWithoutSecret" ) // Webhooks applies the score policy for the Webhooks check. @@ -28,7 +28,7 @@ func Webhooks(name string, findings []finding.Finding, dl checker.DetailLogger, ) checker.CheckResult { expectedProbes := []string{ - webhooksWithoutTokenAuth.Probe, + webhooksWithoutSecret.Probe, } if !finding.UniqueProbesEqual(findings, expectedProbes) { @@ -40,30 +40,30 @@ func Webhooks(name string, return checker.CreateMaxScoreResult(name, "project does not have webhook") } - var webhooksWithoutSecret int + var webhooksWithNoSecret int totalWebhooks := findings[0].Values["totalWebhooks"] for i := range findings { f := &findings[i] if f.Outcome == finding.OutcomeNegative { - webhooksWithoutSecret++ + webhooksWithNoSecret++ } } - if totalWebhooks == webhooksWithoutSecret { + if totalWebhooks == webhooksWithNoSecret { return checker.CreateMinScoreResult(name, "no hook(s) have a secret configured") } - if webhooksWithoutSecret == 0 { + if webhooksWithNoSecret == 0 { msg := fmt.Sprintf("All %d of the projects webhooks are configure with a secret", totalWebhooks) return checker.CreateMaxScoreResult(name, msg) } msg := fmt.Sprintf("%d out of the projects %d webhooks are configure without a secret", - webhooksWithoutSecret, + webhooksWithNoSecret, totalWebhooks) return checker.CreateProportionalScoreResult(name, - msg, totalWebhooks-webhooksWithoutSecret, totalWebhooks) + msg, totalWebhooks-webhooksWithNoSecret, totalWebhooks) } diff --git a/checks/evaluation/webhooks_test.go b/checks/evaluation/webhooks_test.go index 849bcd519d4..5e6c268b3b2 100644 --- a/checks/evaluation/webhooks_test.go +++ b/checks/evaluation/webhooks_test.go @@ -33,7 +33,7 @@ func TestWebhooks(t *testing.T) { name: "no webhooks", findings: []finding.Finding{ { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNotApplicable, }, }, @@ -45,7 +45,7 @@ func TestWebhooks(t *testing.T) { name: "1 webhook with no secret", findings: []finding.Finding{ { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, }, }, @@ -57,7 +57,7 @@ func TestWebhooks(t *testing.T) { name: "1 webhook with secret", findings: []finding.Finding{ { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomePositive, Values: map[string]int{ "totalWebhooks": 1, @@ -72,7 +72,7 @@ func TestWebhooks(t *testing.T) { name: "2 webhooks one of which has secret", findings: []finding.Finding{ { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 2, @@ -87,14 +87,14 @@ func TestWebhooks(t *testing.T) { name: "Five webhooks three of which have secrets", findings: []finding.Finding{ { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 5, }, }, { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 5, @@ -109,7 +109,7 @@ func TestWebhooks(t *testing.T) { name: "One of 12 webhooks does not have secrets", findings: []finding.Finding{ { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, @@ -124,84 +124,84 @@ func TestWebhooks(t *testing.T) { name: "12/12 webhooks do not have secrets", findings: []finding.Finding{ { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutTokenAuth", + Probe: "webhooksWithoutSecret", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, diff --git a/probes/entries.go b/probes/entries.go index 3baec254bdf..29fa98ef118 100644 --- a/probes/entries.go +++ b/probes/entries.go @@ -53,7 +53,7 @@ import ( "github.com/ossf/scorecard/v4/probes/toolDependabotInstalled" "github.com/ossf/scorecard/v4/probes/toolPyUpInstalled" "github.com/ossf/scorecard/v4/probes/toolRenovateInstalled" - "github.com/ossf/scorecard/v4/probes/webhooksWithoutTokenAuth" + "github.com/ossf/scorecard/v4/probes/webhooksWithoutSecret" ) // ProbeImpl is the implementation of a probe. @@ -127,7 +127,7 @@ var ( freeOfUnverifiedBinaryArtifacts.Run, } Webhook = []ProbeImpl{ - webhooksWithoutTokenAuth.Run, + webhooksWithoutSecret.Run, } ) diff --git a/probes/webhooksWithoutSecret/def.yml b/probes/webhooksWithoutSecret/def.yml new file mode 100644 index 00000000000..9af352233f1 --- /dev/null +++ b/probes/webhooksWithoutSecret/def.yml @@ -0,0 +1,33 @@ +# 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. + +id: webhooksWithoutSecret +short: This check determines whether the webhooks defined in the repository have secrets configured to authenticate the origins of requests. +motivation: > + Webhooks without secret authorization have the potential to make projects accessible to third-parties. +implementation: > + The probe checks all webhooks of a project and checks whether each uses secret authentication. +outcome: + - If the project has any webhooks without secret authorization, the probe returns as many OutcomeNegative (0) as the project has webhooks without secret authorization. All findings include the value "totalWebhooks" which is the total number of webhooks that the project has. The finding also includes the path to the webhook. + - If the project does not have any webhooks without secret authorization, the probe returns one OutcomePositive (1). +remediation: + effort: Low + text: + - Check if the service your webhooks is configured with supports secrets. + - If there is support for secret authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook). + - If there is no support for secret authentication, request the webhook service implement secret authentication functionality by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks). + markdown: + - Check if the service your webhooks is configured with supports secrets. + - If there is support for secret authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook). + - If there is no support for secret authentication, request the webhook service implement secret authentication functionality by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks). \ No newline at end of file diff --git a/probes/webhooksWithoutTokenAuth/impl.go b/probes/webhooksWithoutSecret/impl.go similarity index 96% rename from probes/webhooksWithoutTokenAuth/impl.go rename to probes/webhooksWithoutSecret/impl.go index 4856b9cb420..e568dff1615 100644 --- a/probes/webhooksWithoutTokenAuth/impl.go +++ b/probes/webhooksWithoutSecret/impl.go @@ -13,7 +13,7 @@ // limitations under the License. //nolint:stylecheck -package webhooksWithoutTokenAuth +package webhooksWithoutSecret import ( "embed" @@ -27,7 +27,7 @@ import ( //go:embed *.yml var fs embed.FS -const Probe = "webhooksWithoutTokenAuth" +const Probe = "webhooksWithoutSecret" func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { if raw == nil { diff --git a/probes/webhooksWithoutTokenAuth/impl_test.go b/probes/webhooksWithoutSecret/impl_test.go similarity index 99% rename from probes/webhooksWithoutTokenAuth/impl_test.go rename to probes/webhooksWithoutSecret/impl_test.go index 18be86cb718..2fc5a1d27eb 100644 --- a/probes/webhooksWithoutTokenAuth/impl_test.go +++ b/probes/webhooksWithoutSecret/impl_test.go @@ -13,7 +13,7 @@ // limitations under the License. //nolint:stylecheck -package webhooksWithoutTokenAuth +package webhooksWithoutSecret import ( "testing" diff --git a/probes/webhooksWithoutTokenAuth/def.yml b/probes/webhooksWithoutTokenAuth/def.yml deleted file mode 100644 index d892bd6de39..00000000000 --- a/probes/webhooksWithoutTokenAuth/def.yml +++ /dev/null @@ -1,33 +0,0 @@ -# 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. - -id: webhooksWithoutTokenAuth -short: This check determines whether the webhooks defined in the repository have tokens configured to authenticate the origins of requests. -motivation: > - Webhooks without token authorization have the potential to make projects accessible to third-parties. -implementation: > - The probe checks all webhooks of a project and checks whether each uses token authentication. -outcome: - - If the project has any webhooks without token authorization, the probe returns as many OutcomeNegative (0) as the project has webhooks without token authorization. All findings include the value "totalWebhooks" which is the total number of webhooks that the project has. The finding also includes the path to the webhook. - - If the project does not have any webhooks without token authorization, the probe returns one OutcomePositive (1). -remediation: - effort: Low - text: - - Check if the service your webhooks is configured with supports secrets. - - If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook). - - If there is no support for token authentication, request the webhook service implement token authentication functionality by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks). - markdown: - - Check if the service your webhooks is configured with supports secrets. - - If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook). - - If there is no support for token authentication, request the webhook service implement token authentication functionality by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks). \ No newline at end of file From 31183c6a7cdb08359c6847d4584e6880b832010f Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Mon, 4 Dec 2023 13:01:52 +0000 Subject: [PATCH 11/17] use checker.MinResultScore instead of 0 Signed-off-by: Adam Korczynski --- checks/evaluation/webhooks_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/checks/evaluation/webhooks_test.go b/checks/evaluation/webhooks_test.go index 5e6c268b3b2..fc2ab77027d 100644 --- a/checks/evaluation/webhooks_test.go +++ b/checks/evaluation/webhooks_test.go @@ -17,6 +17,7 @@ package evaluation import ( "testing" + "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/finding" scut "github.com/ossf/scorecard/v4/utests" ) @@ -209,7 +210,7 @@ func TestWebhooks(t *testing.T) { }, }, result: scut.TestReturn{ - Score: 0, + Score: checker.MinResultScore, }, }, } From 88943be22093fe892b1b33fa422d8a101b5b588b Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Mon, 4 Dec 2023 13:03:40 +0000 Subject: [PATCH 12/17] Change test name Signed-off-by: Adam Korczynski --- checks/evaluation/webhooks_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/checks/evaluation/webhooks_test.go b/checks/evaluation/webhooks_test.go index fc2ab77027d..f41f771e5c8 100644 --- a/checks/evaluation/webhooks_test.go +++ b/checks/evaluation/webhooks_test.go @@ -122,7 +122,7 @@ func TestWebhooks(t *testing.T) { }, }, { - name: "12/12 webhooks do not have secrets", + name: "Score should not drop below min score", findings: []finding.Finding{ { Probe: "webhooksWithoutSecret", From ed0aefb44ecf5e763d799230b5eae1b797c4d2f8 Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Mon, 4 Dec 2023 13:04:50 +0000 Subject: [PATCH 13/17] use checker.MinResultScore instead of 0 Signed-off-by: Adam Korczynski --- checks/evaluation/webhooks_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/checks/evaluation/webhooks_test.go b/checks/evaluation/webhooks_test.go index f41f771e5c8..7ae7e7b0f7c 100644 --- a/checks/evaluation/webhooks_test.go +++ b/checks/evaluation/webhooks_test.go @@ -51,7 +51,7 @@ func TestWebhooks(t *testing.T) { }, }, result: scut.TestReturn{ - Score: 0, + Score: checker.MinResultScore, }, }, { From f577c72e404b2c86b8ccc2671521f4d93cdea29f Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Mon, 4 Dec 2023 13:05:25 +0000 Subject: [PATCH 14/17] fix typo Signed-off-by: Adam Korczynski --- checks/evaluation/webhooks.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/checks/evaluation/webhooks.go b/checks/evaluation/webhooks.go index 4e8e5b94f17..6923c9572e0 100644 --- a/checks/evaluation/webhooks.go +++ b/checks/evaluation/webhooks.go @@ -56,11 +56,11 @@ func Webhooks(name string, } if webhooksWithNoSecret == 0 { - msg := fmt.Sprintf("All %d of the projects webhooks are configure with a secret", totalWebhooks) + msg := fmt.Sprintf("All %d of the projects webhooks are configured with a secret", totalWebhooks) return checker.CreateMaxScoreResult(name, msg) } - msg := fmt.Sprintf("%d out of the projects %d webhooks are configure without a secret", + msg := fmt.Sprintf("%d out of the projects %d webhooks are configured without a secret", webhooksWithNoSecret, totalWebhooks) From ffd61c06b8506f7e3c05a212d015649ddbacd4b8 Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Mon, 4 Dec 2023 13:07:36 +0000 Subject: [PATCH 15/17] Use checker.MaxResultScore instead of 10 Signed-off-by: Adam Korczynski --- checks/evaluation/webhooks_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/checks/evaluation/webhooks_test.go b/checks/evaluation/webhooks_test.go index 7ae7e7b0f7c..fb0bafc9bf4 100644 --- a/checks/evaluation/webhooks_test.go +++ b/checks/evaluation/webhooks_test.go @@ -39,7 +39,7 @@ func TestWebhooks(t *testing.T) { }, }, result: scut.TestReturn{ - Score: 10, + Score: checker.MaxResultScore, }, }, { @@ -66,7 +66,7 @@ func TestWebhooks(t *testing.T) { }, }, result: scut.TestReturn{ - Score: 10, + Score: checker.MaxResultScore, }, }, { From d59047ab9fdad0313b15e35ee35e6187c301d9a5 Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Mon, 4 Dec 2023 13:32:18 +0000 Subject: [PATCH 16/17] rename probe Signed-off-by: Adam Korczynski --- checks/evaluation/webhooks.go | 4 +- checks/evaluation/webhooks_test.go | 38 +++++++++---------- probes/entries.go | 4 +- .../def.yml | 2 +- .../impl.go | 4 +- .../impl_test.go | 2 +- 6 files changed, 27 insertions(+), 27 deletions(-) rename probes/{webhooksWithoutSecret => webhooksUseSecrets}/def.yml (99%) rename probes/{webhooksWithoutSecret => webhooksUseSecrets}/impl.go (97%) rename probes/{webhooksWithoutSecret => webhooksUseSecrets}/impl_test.go (99%) diff --git a/checks/evaluation/webhooks.go b/checks/evaluation/webhooks.go index 6923c9572e0..c0672608a92 100644 --- a/checks/evaluation/webhooks.go +++ b/checks/evaluation/webhooks.go @@ -20,7 +20,7 @@ import ( "github.com/ossf/scorecard/v4/checker" sce "github.com/ossf/scorecard/v4/errors" "github.com/ossf/scorecard/v4/finding" - "github.com/ossf/scorecard/v4/probes/webhooksWithoutSecret" + "github.com/ossf/scorecard/v4/probes/webhooksUseSecrets" ) // Webhooks applies the score policy for the Webhooks check. @@ -28,7 +28,7 @@ func Webhooks(name string, findings []finding.Finding, dl checker.DetailLogger, ) checker.CheckResult { expectedProbes := []string{ - webhooksWithoutSecret.Probe, + webhooksUseSecrets.Probe, } if !finding.UniqueProbesEqual(findings, expectedProbes) { diff --git a/checks/evaluation/webhooks_test.go b/checks/evaluation/webhooks_test.go index fb0bafc9bf4..69db1de7b4b 100644 --- a/checks/evaluation/webhooks_test.go +++ b/checks/evaluation/webhooks_test.go @@ -34,7 +34,7 @@ func TestWebhooks(t *testing.T) { name: "no webhooks", findings: []finding.Finding{ { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNotApplicable, }, }, @@ -46,7 +46,7 @@ func TestWebhooks(t *testing.T) { name: "1 webhook with no secret", findings: []finding.Finding{ { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, }, }, @@ -58,7 +58,7 @@ func TestWebhooks(t *testing.T) { name: "1 webhook with secret", findings: []finding.Finding{ { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomePositive, Values: map[string]int{ "totalWebhooks": 1, @@ -73,7 +73,7 @@ func TestWebhooks(t *testing.T) { name: "2 webhooks one of which has secret", findings: []finding.Finding{ { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 2, @@ -88,14 +88,14 @@ func TestWebhooks(t *testing.T) { name: "Five webhooks three of which have secrets", findings: []finding.Finding{ { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 5, }, }, { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 5, @@ -110,7 +110,7 @@ func TestWebhooks(t *testing.T) { name: "One of 12 webhooks does not have secrets", findings: []finding.Finding{ { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, @@ -125,84 +125,84 @@ func TestWebhooks(t *testing.T) { name: "Score should not drop below min score", findings: []finding.Finding{ { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, }, }, { - Probe: "webhooksWithoutSecret", + Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, Values: map[string]int{ "totalWebhooks": 12, diff --git a/probes/entries.go b/probes/entries.go index 29fa98ef118..4486ecd8913 100644 --- a/probes/entries.go +++ b/probes/entries.go @@ -53,7 +53,7 @@ import ( "github.com/ossf/scorecard/v4/probes/toolDependabotInstalled" "github.com/ossf/scorecard/v4/probes/toolPyUpInstalled" "github.com/ossf/scorecard/v4/probes/toolRenovateInstalled" - "github.com/ossf/scorecard/v4/probes/webhooksWithoutSecret" + "github.com/ossf/scorecard/v4/probes/webhooksUseSecrets" ) // ProbeImpl is the implementation of a probe. @@ -127,7 +127,7 @@ var ( freeOfUnverifiedBinaryArtifacts.Run, } Webhook = []ProbeImpl{ - webhooksWithoutSecret.Run, + webhooksUseSecrets.Run, } ) diff --git a/probes/webhooksWithoutSecret/def.yml b/probes/webhooksUseSecrets/def.yml similarity index 99% rename from probes/webhooksWithoutSecret/def.yml rename to probes/webhooksUseSecrets/def.yml index 9af352233f1..74db82cc2c5 100644 --- a/probes/webhooksWithoutSecret/def.yml +++ b/probes/webhooksUseSecrets/def.yml @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -id: webhooksWithoutSecret +id: webhooksUseSecrets short: This check determines whether the webhooks defined in the repository have secrets configured to authenticate the origins of requests. motivation: > Webhooks without secret authorization have the potential to make projects accessible to third-parties. diff --git a/probes/webhooksWithoutSecret/impl.go b/probes/webhooksUseSecrets/impl.go similarity index 97% rename from probes/webhooksWithoutSecret/impl.go rename to probes/webhooksUseSecrets/impl.go index e568dff1615..e21dfd766b8 100644 --- a/probes/webhooksWithoutSecret/impl.go +++ b/probes/webhooksUseSecrets/impl.go @@ -13,7 +13,7 @@ // limitations under the License. //nolint:stylecheck -package webhooksWithoutSecret +package webhooksUseSecrets import ( "embed" @@ -27,7 +27,7 @@ import ( //go:embed *.yml var fs embed.FS -const Probe = "webhooksWithoutSecret" +const Probe = "webhooksUseSecrets" func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { if raw == nil { diff --git a/probes/webhooksWithoutSecret/impl_test.go b/probes/webhooksUseSecrets/impl_test.go similarity index 99% rename from probes/webhooksWithoutSecret/impl_test.go rename to probes/webhooksUseSecrets/impl_test.go index 2fc5a1d27eb..eaf073d1f35 100644 --- a/probes/webhooksWithoutSecret/impl_test.go +++ b/probes/webhooksUseSecrets/impl_test.go @@ -13,7 +13,7 @@ // limitations under the License. //nolint:stylecheck -package webhooksWithoutSecret +package webhooksUseSecrets import ( "testing" From 2dc1b1e6f4d487dcd6afe334a5f533eae9a4fe09 Mon Sep 17 00:00:00 2001 From: Adam Korczynski Date: Tue, 5 Dec 2023 14:43:34 +0000 Subject: [PATCH 17/17] remove the 'totalWebhooks' value from findings Signed-off-by: Adam Korczynski --- checks/evaluation/webhooks.go | 2 +- checks/evaluation/webhooks_test.go | 111 ++++++++++++++++------------- probes/webhooksUseSecrets/def.yml | 2 +- probes/webhooksUseSecrets/impl.go | 9 +-- 4 files changed, 64 insertions(+), 60 deletions(-) diff --git a/checks/evaluation/webhooks.go b/checks/evaluation/webhooks.go index c0672608a92..37ed12c5e6b 100644 --- a/checks/evaluation/webhooks.go +++ b/checks/evaluation/webhooks.go @@ -42,7 +42,7 @@ func Webhooks(name string, var webhooksWithNoSecret int - totalWebhooks := findings[0].Values["totalWebhooks"] + totalWebhooks := len(findings) for i := range findings { f := &findings[i] diff --git a/checks/evaluation/webhooks_test.go b/checks/evaluation/webhooks_test.go index 69db1de7b4b..a698e732115 100644 --- a/checks/evaluation/webhooks_test.go +++ b/checks/evaluation/webhooks_test.go @@ -60,9 +60,6 @@ func TestWebhooks(t *testing.T) { { Probe: "webhooksUseSecrets", Outcome: finding.OutcomePositive, - Values: map[string]int{ - "totalWebhooks": 1, - }, }, }, result: scut.TestReturn{ @@ -75,9 +72,10 @@ func TestWebhooks(t *testing.T) { { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 2, - }, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, }, }, result: scut.TestReturn{ @@ -90,16 +88,22 @@ func TestWebhooks(t *testing.T) { { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 5, - }, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, }, { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 5, - }, }, }, result: scut.TestReturn{ @@ -112,9 +116,50 @@ func TestWebhooks(t *testing.T) { { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 12, - }, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, + }, + { + Probe: "webhooksUseSecrets", + Outcome: finding.OutcomePositive, }, }, result: scut.TestReturn{ @@ -127,86 +172,50 @@ func TestWebhooks(t *testing.T) { { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 12, - }, }, { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 12, - }, }, { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 12, - }, }, { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 12, - }, }, { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 12, - }, }, { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 12, - }, }, { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 12, - }, }, { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 12, - }, }, { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 12, - }, }, { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 12, - }, }, { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 12, - }, }, { Probe: "webhooksUseSecrets", Outcome: finding.OutcomeNegative, - Values: map[string]int{ - "totalWebhooks": 12, - }, }, }, result: scut.TestReturn{ diff --git a/probes/webhooksUseSecrets/def.yml b/probes/webhooksUseSecrets/def.yml index 74db82cc2c5..d7b81894568 100644 --- a/probes/webhooksUseSecrets/def.yml +++ b/probes/webhooksUseSecrets/def.yml @@ -19,7 +19,7 @@ motivation: > implementation: > The probe checks all webhooks of a project and checks whether each uses secret authentication. outcome: - - If the project has any webhooks without secret authorization, the probe returns as many OutcomeNegative (0) as the project has webhooks without secret authorization. All findings include the value "totalWebhooks" which is the total number of webhooks that the project has. The finding also includes the path to the webhook. + - If the project has any webhooks without secret authorization, the probe returns as many OutcomeNegative (0) as the project has webhooks without secret authorization and as many OutcomePositive as there are webhooks with secret authorization. All findings include the path to the webhook. - If the project does not have any webhooks without secret authorization, the probe returns one OutcomePositive (1). remediation: effort: Low diff --git a/probes/webhooksUseSecrets/impl.go b/probes/webhooksUseSecrets/impl.go index e21dfd766b8..08f87b14000 100644 --- a/probes/webhooksUseSecrets/impl.go +++ b/probes/webhooksUseSecrets/impl.go @@ -35,7 +35,6 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { } r := raw.WebhookResults - totalWebhooks := len(r.Webhooks) var findings []finding.Finding if len(r.Webhooks) == 0 { @@ -57,9 +56,7 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { if err != nil { return nil, Probe, fmt.Errorf("create finding: %w", err) } - f = f.WithValues(map[string]int{ - "totalWebhooks": totalWebhooks, - }).WithLocation(&finding.Location{ + f = f.WithLocation(&finding.Location{ Path: hook.Path, }) findings = append(findings, *f) @@ -70,9 +67,7 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { if err != nil { return nil, Probe, fmt.Errorf("create finding: %w", err) } - f = f.WithValues(map[string]int{ - "totalWebhooks": totalWebhooks, - }).WithLocation(&finding.Location{ + f = f.WithLocation(&finding.Location{ Path: hook.Path, }) findings = append(findings, *f)