From de39061cc59e0c24fb17a3953efb501777ef6bd0 Mon Sep 17 00:00:00 2001 From: naveen <172697+naveensrinivasan@users.noreply.github.com> Date: Mon, 3 Jan 2022 01:43:38 +0000 Subject: [PATCH] :seedling: Refactor vulnerabilities client --- Makefile | 5 +- checker/check_request.go | 13 +++-- checks/vulnerabilities.go | 63 +++----------------- clients/mockclients/vulnerabilities.go | 66 +++++++++++++++++++++ clients/vulnerabilities.go | 80 ++++++++++++++++++++++++++ cmd/root.go | 6 +- cmd/serve.go | 3 +- cron/worker/main.go | 10 +++- e2e/vulnerabilities_test.go | 23 ++++---- pkg/scorecard.go | 21 ++++--- 10 files changed, 202 insertions(+), 88 deletions(-) create mode 100644 clients/mockclients/vulnerabilities.go create mode 100644 clients/vulnerabilities.go diff --git a/Makefile b/Makefile index cb4ca337..849f89ed 100644 --- a/Makefile +++ b/Makefile @@ -111,7 +111,7 @@ cron/data/metadata.pb.go: cron/data/metadata.proto | $(PROTOC) protoc --go_out=../../../ cron/data/metadata.proto generate-mocks: ## Compiles and generates all mocks using mockgen. -generate-mocks: clients/mockclients/repo_client.go clients/mockclients/repo.go clients/mockclients/cii_client.go +generate-mocks: clients/mockclients/repo_client.go clients/mockclients/repo.go clients/mockclients/cii_client.go checks/mockclients/vulnerabilities.go clients/mockclients/repo_client.go: clients/repo_client.go # Generating MockRepoClient $(MOCKGEN) -source=clients/repo_client.go -destination=clients/mockclients/repo_client.go -package=mockrepo -copyright_file=clients/mockclients/license.txt @@ -121,6 +121,9 @@ clients/mockclients/repo.go: clients/repo.go clients/mockclients/cii_client.go: clients/cii_client.go # Generating MockCIIClient $(MOCKGEN) -source=clients/cii_client.go -destination=clients/mockclients/cii_client.go -package=mockrepo -copyright_file=clients/mockclients/license.txt +checks/mockclients/vulnerabilities.go: clients/vulnerabilities.go + # Generating MockCIIClient + $(MOCKGEN) -source=clients/vulnerabilities.go -destination=clients/mockclients/vulnerabilities.go -package=mockrepo -copyright_file=clients/mockclients/license.txt generate-docs: ## Generates docs generate-docs: validate-docs docs/checks.md diff --git a/checker/check_request.go b/checker/check_request.go index 5ac24906..6e802591 100644 --- a/checker/check_request.go +++ b/checker/check_request.go @@ -22,12 +22,13 @@ import ( // CheckRequest struct encapsulates all data to be passed into a CheckFn. type CheckRequest struct { - Ctx context.Context - RepoClient clients.RepoClient - CIIClient clients.CIIBestPracticesClient - OssFuzzRepo clients.RepoClient - Dlogger DetailLogger - Repo clients.Repo + Ctx context.Context + RepoClient clients.RepoClient + CIIClient clients.CIIBestPracticesClient + OssFuzzRepo clients.RepoClient + Dlogger DetailLogger + Repo clients.Repo + VulnerabilitiesClient clients.VulnerabilitiesClient // UPGRADEv6: return raw results instead of scores. RawResults *RawResults } diff --git a/checks/vulnerabilities.go b/checks/vulnerabilities.go index 19602f9f..cd71159e 100644 --- a/checks/vulnerabilities.go +++ b/checks/vulnerabilities.go @@ -15,45 +15,25 @@ package checks import ( - "bytes" - "encoding/json" "fmt" - "net/http" "strings" "github.com/ossf/scorecard/v3/checker" + "github.com/ossf/scorecard/v3/clients" sce "github.com/ossf/scorecard/v3/errors" ) const ( // CheckVulnerabilities is the registered name for the OSV check. CheckVulnerabilities = "Vulnerabilities" - osvQueryEndpoint = "https://api.osv.dev/v1/query" ) -type osvQuery struct { - Commit string `json:"commit"` -} - -type osvResponse struct { - Vulns []struct { - ID string `json:"id"` - } `json:"vulns"` -} - -// Vulnerabilities cheks for vulnerabilities in api.osv.dev. -type Vulnerabilities interface { - HasUnfixedVulnerabilities(c *checker.CheckRequest) checker.CheckResult -} -type vulns struct{} - //nolint:gochecknoinits func init() { - v := &vulns{} - registerCheck(CheckVulnerabilities, v.HasUnfixedVulnerabilities) + registerCheck(CheckVulnerabilities, HasUnfixedVulnerabilities) } -func (resp *osvResponse) getVulnerabilities() []string { +func getVulnerabilities(resp *clients.VulnerabilitiesResponse) []string { ids := make([]string, 0, len(resp.Vulns)) for _, vuln := range resp.Vulns { ids = append(ids, vuln.ID) @@ -61,13 +41,8 @@ func (resp *osvResponse) getVulnerabilities() []string { return ids } -// NewVulnerabilities creates a new Vulnerabilities check. -func NewVulnerabilities() Vulnerabilities { - return &vulns{} -} - // HasUnfixedVulnerabilities runs Vulnerabilities check. -func (v *vulns) HasUnfixedVulnerabilities(c *checker.CheckRequest) checker.CheckResult { +func HasUnfixedVulnerabilities(c *checker.CheckRequest) checker.CheckResult { commits, err := c.RepoClient.ListCommits() if err != nil { e := sce.WithMessage(sce.ErrScorecardInternal, "Client.Repositories.ListCommits") @@ -78,38 +53,14 @@ func (v *vulns) HasUnfixedVulnerabilities(c *checker.CheckRequest) checker.Check return checker.CreateInconclusiveResult(CheckVulnerabilities, "no commits found") } - query, err := json.Marshal(&osvQuery{ - Commit: commits[0].SHA, - }) + resp, err := c.VulnerabilitiesClient.HasUnfixedVulnerabilities(c.Ctx, commits[0].SHA) if err != nil { - e := sce.WithMessage(sce.ErrScorecardInternal, "json.Marshal") - return checker.CreateRuntimeErrorResult(CheckVulnerabilities, e) - } - - req, err := http.NewRequestWithContext(c.Ctx, http.MethodPost, osvQueryEndpoint, bytes.NewReader(query)) - if err != nil { - e := sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("http.NewRequestWithContext: %v", err)) - return checker.CreateRuntimeErrorResult(CheckVulnerabilities, e) - } - - // Use our own http client as the one from CheckRequest adds GitHub tokens to the headers. - httpClient := &http.Client{} - resp, err := httpClient.Do(req) - if err != nil { - e := sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("httpClient.Do: %v", err)) - return checker.CreateRuntimeErrorResult(CheckVulnerabilities, e) - } - defer resp.Body.Close() - - var osvResp osvResponse - decoder := json.NewDecoder(resp.Body) - if err := decoder.Decode(&osvResp); err != nil { - e := sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("decoder.Decode: %v", err)) + e := sce.WithMessage(sce.ErrScorecardInternal, "VulnerabilitiesClient.HasUnfixedVulnerabilities") return checker.CreateRuntimeErrorResult(CheckVulnerabilities, e) } // TODO: take severity into account. - vulnIDs := osvResp.getVulnerabilities() + vulnIDs := getVulnerabilities(&resp) if len(vulnIDs) > 0 { c.Dlogger.Warn3(&checker.LogMessage{ Text: fmt.Sprintf("HEAD is vulnerable to %s", strings.Join(vulnIDs, ", ")), diff --git a/clients/mockclients/vulnerabilities.go b/clients/mockclients/vulnerabilities.go new file mode 100644 index 00000000..04e5fad4 --- /dev/null +++ b/clients/mockclients/vulnerabilities.go @@ -0,0 +1,66 @@ +// Copyright 2021 Security Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +// Code generated by MockGen. DO NOT EDIT. +// Source: clients/vulnerabilities.go + +// Package mockrepo is a generated GoMock package. +package mockrepo + +import ( + context "context" + reflect "reflect" + + gomock "github.com/golang/mock/gomock" + clients "github.com/ossf/scorecard/v3/clients" +) + +// MockVulnerabilitiesClient is a mock of VulnerabilitiesClient interface. +type MockVulnerabilitiesClient struct { + ctrl *gomock.Controller + recorder *MockVulnerabilitiesClientMockRecorder +} + +// MockVulnerabilitiesClientMockRecorder is the mock recorder for MockVulnerabilitiesClient. +type MockVulnerabilitiesClientMockRecorder struct { + mock *MockVulnerabilitiesClient +} + +// NewMockVulnerabilitiesClient creates a new mock instance. +func NewMockVulnerabilitiesClient(ctrl *gomock.Controller) *MockVulnerabilitiesClient { + mock := &MockVulnerabilitiesClient{ctrl: ctrl} + mock.recorder = &MockVulnerabilitiesClientMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockVulnerabilitiesClient) EXPECT() *MockVulnerabilitiesClientMockRecorder { + return m.recorder +} + +// HasUnfixedVulnerabilities mocks base method. +func (m *MockVulnerabilitiesClient) HasUnfixedVulnerabilities(context context.Context, commit string) (clients.OSVResponse, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HasUnfixedVulnerabilities", context, commit) + ret0, _ := ret[0].(clients.OSVResponse) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// HasUnfixedVulnerabilities indicates an expected call of HasUnfixedVulnerabilities. +func (mr *MockVulnerabilitiesClientMockRecorder) HasUnfixedVulnerabilities(context, commit interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasUnfixedVulnerabilities", reflect.TypeOf((*MockVulnerabilitiesClient)(nil).HasUnfixedVulnerabilities), context, commit) +} diff --git a/clients/vulnerabilities.go b/clients/vulnerabilities.go new file mode 100644 index 00000000..b82c22a2 --- /dev/null +++ b/clients/vulnerabilities.go @@ -0,0 +1,80 @@ +// Copyright 2021 Security Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package clients + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + + "github.com/ossf/scorecard/v3/errors" +) + +const osvQueryEndpoint = "https://api.osv.dev/v1/query" + +type osvQuery struct { + Commit string `json:"commit"` +} + +// VulnerabilitiesClient cheks for vulnerabilities in api.osv.dev. +type VulnerabilitiesClient interface { + HasUnfixedVulnerabilities(context context.Context, commit string) (VulnerabilitiesResponse, error) +} + +// VulnerabilitiesResponse is the response from the OSV API. +type VulnerabilitiesResponse struct { + Vulns []struct { + ID string `json:"id"` + } `json:"vulns"` +} + +type vulns struct{} + +// DefaultVulnerabilitiesClient is a new Vulnerabilities client. +func DefaultVulnerabilitiesClient() VulnerabilitiesClient { + return vulns{} +} + +// HasUnfixedVulnerabilities runs Vulnerabilities check. +func (v vulns) HasUnfixedVulnerabilities(ctx context.Context, commit string) (VulnerabilitiesResponse, error) { + query, err := json.Marshal(&osvQuery{ + Commit: commit, + }) + if err != nil { + return VulnerabilitiesResponse{}, errors.WithMessage(err, "failed to marshal query") + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, osvQueryEndpoint, bytes.NewReader(query)) + if err != nil { + return VulnerabilitiesResponse{}, errors.WithMessage(err, "failed to create request") + } + + // Use our own http client as the one from CheckRequest adds GitHub tokens to the headers. + httpClient := &http.Client{} + resp, err := httpClient.Do(req) + if err != nil { + return VulnerabilitiesResponse{}, errors.WithMessage(err, "failed to send request") + } + defer resp.Body.Close() + + var osvResp VulnerabilitiesResponse + decoder := json.NewDecoder(resp.Body) + if err := decoder.Decode(&osvResp); err != nil { + return VulnerabilitiesResponse{}, errors.WithMessage(err, "failed to decode response") + } + + return osvResp, nil +} diff --git a/cmd/root.go b/cmd/root.go index 82f9d83b..e433c1da 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -193,7 +193,7 @@ func scorecardCmd(cmd *cobra.Command, args []string) { // nolint: errcheck defer logger.Sync() // Flushes buffer, if any. - repoURI, repoClient, ossFuzzRepoClient, ciiClient, repoType, err := getRepoAccessors(ctx, uri, logger) + repoURI, repoClient, ossFuzzRepoClient, ciiClient, vulnsClient, repoType, err := getRepoAccessors(ctx, uri, logger) if err != nil { log.Panic(err) } @@ -228,7 +228,7 @@ func scorecardCmd(cmd *cobra.Command, args []string) { log.Panicf("only json format is supported") } - repoResult, err := pkg.RunScorecards(ctx, repoURI, raw, enabledChecks, repoClient, ossFuzzRepoClient, ciiClient) + repoResult, err := pkg.RunScorecards(ctx, repoURI, raw, enabledChecks, repoClient, ossFuzzRepoClient, ciiClient, vulnsClient) if err != nil { log.Panic(err) } @@ -425,6 +425,7 @@ func getRepoAccessors(ctx context.Context, uri string, logger *zap.Logger) ( repoClient clients.RepoClient, ossFuzzRepoClient clients.RepoClient, ciiClient clients.CIIBestPracticesClient, + vulnerabilityClient clients.VulnerabilitiesClient, repoType string, err error) { var localRepo, githubRepo clients.Repo @@ -442,6 +443,7 @@ func getRepoAccessors(ctx context.Context, uri string, logger *zap.Logger) ( repo = githubRepo repoClient = githubrepo.CreateGithubRepoClient(ctx, logger) ciiClient = clients.DefaultCIIBestPracticesClient() + vulnerabilityClient = clients.DefaultVulnerabilitiesClient() ossFuzzRepoClient, err = githubrepo.CreateOssFuzzRepoClient(ctx, logger) return } diff --git a/cmd/serve.go b/cmd/serve.go index 126ad0dd..a2a0457d 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -66,13 +66,14 @@ var serveCmd = &cobra.Command{ ctx := r.Context() repoClient := githubrepo.CreateGithubRepoClient(ctx, logger) ossFuzzRepoClient, err := githubrepo.CreateOssFuzzRepoClient(ctx, logger) + vulnsClient := clients.DefaultVulnerabilitiesClient() if err != nil { sugar.Error(err) rw.WriteHeader(http.StatusInternalServerError) } defer ossFuzzRepoClient.Close() ciiClient := clients.DefaultCIIBestPracticesClient() - repoResult, err := pkg.RunScorecards(ctx, repo, false, checks.AllChecks, repoClient, ossFuzzRepoClient, ciiClient) + repoResult, err := pkg.RunScorecards(ctx, repo, false, checks.AllChecks, repoClient, ossFuzzRepoClient, ciiClient, vulnsClient) if err != nil { sugar.Error(err) rw.WriteHeader(http.StatusInternalServerError) diff --git a/cron/worker/main.go b/cron/worker/main.go index 77f0a1e2..3be30651 100644 --- a/cron/worker/main.go +++ b/cron/worker/main.go @@ -52,7 +52,9 @@ func processRequest(ctx context.Context, batchRequest *data.ScorecardBatchRequest, checksToRun checker.CheckNameToFnMap, bucketURL, bucketURL2 string, checkDocs docs.Doc, repoClient clients.RepoClient, ossFuzzRepoClient clients.RepoClient, - ciiClient clients.CIIBestPracticesClient, logger *zap.Logger) error { + ciiClient clients.CIIBestPracticesClient, + vulnsClient clients.VulnerabilitiesClient, + logger *zap.Logger) error { filename := data.GetBlobFilename( fmt.Sprintf("shard-%07d", batchRequest.GetShardNum()), batchRequest.GetJobTime().AsTime()) @@ -83,7 +85,8 @@ func processRequest(ctx context.Context, continue } repo.AppendMetadata(repo.Metadata()...) - result, err := pkg.RunScorecards(ctx, repo, false, checksToRun, repoClient, ossFuzzRepoClient, ciiClient) + result, err := pkg.RunScorecards(ctx, repo, false, checksToRun, + repoClient, ossFuzzRepoClient, ciiClient, vulnsClient) if errors.Is(err, sce.ErrRepoUnreachable) { // Not accessible repo - continue. continue @@ -190,6 +193,7 @@ func main() { repoClient := githubrepo.CreateGithubRepoClient(ctx, logger) ciiClient := clients.BlobCIIBestPracticesClient(ciiDataBucketURL) ossFuzzRepoClient, err := githubrepo.CreateOssFuzzRepoClient(ctx, logger) + vulnsClient := clients.DefaultVulnerabilitiesClient() if err != nil { panic(err) } @@ -222,7 +226,7 @@ func main() { } if err := processRequest(ctx, req, checksToRun, bucketURL, bucketURL2, checkDocs, - repoClient, ossFuzzRepoClient, ciiClient, logger); err != nil { + repoClient, ossFuzzRepoClient, ciiClient, vulnsClient, logger); err != nil { logger.Warn(fmt.Sprintf("error processing request: %v", err)) // Nack the message so that another worker can retry. subscriber.Nack() diff --git a/e2e/vulnerabilities_test.go b/e2e/vulnerabilities_test.go index c59a9bf8..7def2480 100644 --- a/e2e/vulnerabilities_test.go +++ b/e2e/vulnerabilities_test.go @@ -22,6 +22,7 @@ import ( "github.com/ossf/scorecard/v3/checker" "github.com/ossf/scorecard/v3/checks" + "github.com/ossf/scorecard/v3/clients" "github.com/ossf/scorecard/v3/clients/githubrepo" scut "github.com/ossf/scorecard/v3/utests" ) @@ -37,10 +38,11 @@ var _ = Describe("E2E TEST:Vulnerabilities", func() { dl := scut.TestDetailLogger{} req := checker.CheckRequest{ - Ctx: context.Background(), - RepoClient: repoClient, - Repo: repo, - Dlogger: &dl, + Ctx: context.Background(), + RepoClient: repoClient, + VulnerabilitiesClient: clients.DefaultVulnerabilitiesClient(), + Repo: repo, + Dlogger: &dl, } expected := scut.TestReturn{ Error: nil, @@ -50,7 +52,7 @@ var _ = Describe("E2E TEST:Vulnerabilities", func() { NumberOfDebug: 0, } - result := checks.NewVulnerabilities().HasUnfixedVulnerabilities(&req) + result := checks.HasUnfixedVulnerabilities(&req) // UPGRADEv2: to remove. // Old version. Expect(result.Error).Should(BeNil()) @@ -69,10 +71,11 @@ var _ = Describe("E2E TEST:Vulnerabilities", func() { dl := scut.TestDetailLogger{} checkRequest := checker.CheckRequest{ - Ctx: context.Background(), - RepoClient: repoClient, - Repo: repo, - Dlogger: &dl, + Ctx: context.Background(), + RepoClient: repoClient, + VulnerabilitiesClient: clients.DefaultVulnerabilitiesClient(), + Repo: repo, + Dlogger: &dl, } expected := scut.TestReturn{ Error: nil, @@ -81,7 +84,7 @@ var _ = Describe("E2E TEST:Vulnerabilities", func() { NumberOfInfo: 0, NumberOfDebug: 0, } - result := checks.NewVulnerabilities().HasUnfixedVulnerabilities(&checkRequest) + result := checks.HasUnfixedVulnerabilities(&checkRequest) // UPGRADEv2: to remove. // Old version. Expect(result.Error).Should(BeNil()) diff --git a/pkg/scorecard.go b/pkg/scorecard.go index 42764051..9169c2a9 100644 --- a/pkg/scorecard.go +++ b/pkg/scorecard.go @@ -30,14 +30,16 @@ import ( func runEnabledChecks(ctx context.Context, repo clients.Repo, raw *checker.RawResults, checksToRun checker.CheckNameToFnMap, repoClient clients.RepoClient, ossFuzzRepoClient clients.RepoClient, ciiClient clients.CIIBestPracticesClient, + vulnsClient clients.VulnerabilitiesClient, resultsCh chan checker.CheckResult) { request := checker.CheckRequest{ - Ctx: ctx, - RepoClient: repoClient, - OssFuzzRepo: ossFuzzRepoClient, - CIIClient: ciiClient, - Repo: repo, - RawResults: raw, + Ctx: ctx, + RepoClient: repoClient, + OssFuzzRepo: ossFuzzRepoClient, + CIIClient: ciiClient, + VulnerabilitiesClient: vulnsClient, + Repo: repo, + RawResults: raw, } wg := sync.WaitGroup{} for checkName, checkFn := range checksToRun { @@ -78,7 +80,8 @@ func RunScorecards(ctx context.Context, checksToRun checker.CheckNameToFnMap, repoClient clients.RepoClient, ossFuzzRepoClient clients.RepoClient, - ciiClient clients.CIIBestPracticesClient) (ScorecardResult, error) { + ciiClient clients.CIIBestPracticesClient, + vulnsClient clients.VulnerabilitiesClient) (ScorecardResult, error) { if err := repoClient.InitRepo(repo); err != nil { // No need to call sce.WithMessage() since InitRepo will do that for us. //nolint:wrapcheck @@ -104,9 +107,9 @@ func RunScorecards(ctx context.Context, } resultsCh := make(chan checker.CheckResult) if raw { - go runEnabledChecks(ctx, repo, &ret.RawResults, checksToRun, repoClient, ossFuzzRepoClient, ciiClient, resultsCh) + go runEnabledChecks(ctx, repo, &ret.RawResults, checksToRun, repoClient, ossFuzzRepoClient, ciiClient, vulnsClient, resultsCh) } else { - go runEnabledChecks(ctx, repo, nil, checksToRun, repoClient, ossFuzzRepoClient, ciiClient, resultsCh) + go runEnabledChecks(ctx, repo, nil, checksToRun, repoClient, ossFuzzRepoClient, ciiClient, vulnsClient, resultsCh) } for result := range resultsCh {