internal/worker: create issues for GitHub security advisories
worker.CreateIssues now creates issues for security advisories
that need them.
Change-Id: I99fa8dc12d6b26681d89e0a861d21cd1ff14f042
Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/384334
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
diff --git a/internal/worker/store/fire_store.go b/internal/worker/store/fire_store.go
index 9921b6a..a4c20ff 100644
--- a/internal/worker/store/fire_store.go
+++ b/internal/worker/store/fire_store.go
@@ -280,6 +280,21 @@
return tx.t.Set(tx.s.ghsaRecordRef(r.GHSA.ID), r)
}
+// GetGHSARecord implements Transaction.GetGHSARecord.
+func (tx *fsTransaction) GetGHSARecord(id string) (_ *GHSARecord, err error) {
+ defer derrors.Wrap(&err, "GetGHSARecord(%s)", id)
+
+ docsnap, err := tx.t.Get(tx.s.ghsaRecordRef(id))
+ if status.Code(err) == codes.NotFound {
+ return nil, nil
+ }
+ var gr GHSARecord
+ if err := docsnap.DataTo(&gr); err != nil {
+ return nil, err
+ }
+ return &gr, nil
+}
+
// GetGHSARecords implements Transaction.GetGHSARecords.
func (tx *fsTransaction) GetGHSARecords() (_ []*GHSARecord, err error) {
defer derrors.Wrap(&err, "GetGHSARecords()")
diff --git a/internal/worker/store/mem_store.go b/internal/worker/store/mem_store.go
index a52f1b0..283c092 100644
--- a/internal/worker/store/mem_store.go
+++ b/internal/worker/store/mem_store.go
@@ -170,7 +170,7 @@
return nil
}
-// SetGHSARecord implements Transaction.SetGHSARecord.n
+// SetGHSARecord implements Transaction.SetGHSARecord.
func (tx *memTransaction) SetGHSARecord(r *GHSARecord) error {
if _, ok := tx.ms.ghsaRecords[r.GHSA.ID]; !ok {
return fmt.Errorf("GHSARecord %s does not exist", r.GHSA.ID)
@@ -179,7 +179,15 @@
return nil
}
-// GetGHSARecords returns all the GHSARecords in the database.
+// GetGHSARecord implements Transaction.GetGHSARecord.
+func (tx *memTransaction) GetGHSARecord(id string) (*GHSARecord, error) {
+ if r, ok := tx.ms.ghsaRecords[id]; ok {
+ return r, nil
+ }
+ return nil, fmt.Errorf("GHSARecord %s does not exist", id)
+}
+
+// GetGHSARecords implements Transaction.GetGHSARecords.
func (tx *memTransaction) GetGHSARecords() ([]*GHSARecord, error) {
var recs []*GHSARecord
for _, r := range tx.ms.ghsaRecords {
diff --git a/internal/worker/store/store.go b/internal/worker/store/store.go
index 7d05d4a..80440c1 100644
--- a/internal/worker/store/store.go
+++ b/internal/worker/store/store.go
@@ -63,6 +63,12 @@
History []*CVERecordSnapshot
}
+func (r *CVERecord) GetID() string { return r.ID }
+func (r *CVERecord) GetPrettyID() string { return r.ID }
+func (r *CVERecord) GetUnit() string { return r.Module }
+func (r *CVERecord) GetIssueReference() string { return r.IssueReference }
+func (r *CVERecord) GetIssueCreatedAt() time.Time { return r.IssueCreatedAt }
+
// Validate returns an error if the CVERecord is not valid.
func (r *CVERecord) Validate() error {
if r.ID == "" {
@@ -179,6 +185,26 @@
TriageState TriageState
// TriageStateReason is an explanation of TriageState.
TriageStateReason string
+ // IssueReference is a reference to the GitHub issue that was filed.
+ // E.g. golang/vulndb#12345.
+ // Set only after a GitHub issue has been successfully created.
+ IssueReference string
+ // IssueCreatedAt is the time when the issue was created.
+ // Set only after a GitHub issue has been successfully created.
+ IssueCreatedAt time.Time
+}
+
+func (r *GHSARecord) GetID() string { return r.GHSA.ID }
+func (r *GHSARecord) GetUnit() string { return r.GHSA.Vulns[0].Package }
+func (r *GHSARecord) GetIssueReference() string { return r.IssueReference }
+func (r *GHSARecord) GetIssueCreatedAt() time.Time { return r.IssueCreatedAt }
+
+func (r *GHSARecord) GetPrettyID() string {
+ // The GHSA ID in the Identifiers list is more human-readable.
+ if len(r.GHSA.Identifiers) > 0 {
+ return r.GHSA.Identifiers[0].Value
+ }
+ return r.GHSA.ID
}
// A Store is a storage system for the CVE database.
@@ -236,6 +262,10 @@
// an error if no such record exists.
SetGHSARecord(*GHSARecord) error
+ // GetGHSARecord returns a single GHSARecord by ID.
+ // If not found, it returns (nil, nil).
+ GetGHSARecord(id string) (*GHSARecord, error)
+
// GetGHSARecords returns all the GHSARecords in the database.
GetGHSARecords() ([]*GHSARecord, error)
}
diff --git a/internal/worker/store/store_test.go b/internal/worker/store/store_test.go
index 35dd90b..d21b8e2 100644
--- a/internal/worker/store/store_test.go
+++ b/internal/worker/store/store_test.go
@@ -248,6 +248,20 @@
if diff := cmp.Diff(gs, got); diff != "" {
t.Errorf("mismatch (-want, +got):\n%s", diff)
}
+
+ // Retrieve one record by ID.
+ var got0 *GHSARecord
+ err = s.RunTransaction(ctx, func(ctx context.Context, tx Transaction) error {
+ var err error
+ got0, err = tx.GetGHSARecord(gs[0].GetID())
+ return err
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ if got, want := got0, gs[0]; !cmp.Equal(got, want) {
+ t.Errorf("got %+v, want %+v", got, want)
+ }
}
func createCVERecords(t *testing.T, ctx context.Context, s Store, crs []*CVERecord) {
diff --git a/internal/worker/update_test.go b/internal/worker/update_test.go
index b3e4c6a..a3bf32d 100644
--- a/internal/worker/update_test.go
+++ b/internal/worker/update_test.go
@@ -353,7 +353,23 @@
func createCVERecords(t *testing.T, s store.Store, crs []*store.CVERecord) {
err := s.RunTransaction(context.Background(), func(_ context.Context, tx store.Transaction) error {
for _, cr := range crs {
- if err := tx.CreateCVERecord(cr); err != nil {
+ copy := *cr
+ if err := tx.CreateCVERecord(©); err != nil {
+ return err
+ }
+ }
+ return nil
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+}
+
+func createGHSARecords(t *testing.T, s store.Store, grs []*store.GHSARecord) {
+ err := s.RunTransaction(context.Background(), func(_ context.Context, tx store.Transaction) error {
+ for _, gr := range grs {
+ copy := *gr
+ if err := tx.CreateGHSARecord(©); err != nil {
return err
}
}
diff --git a/internal/worker/worker.go b/internal/worker/worker.go
index 794cea5..a1049d8 100644
--- a/internal/worker/worker.go
+++ b/internal/worker/worker.go
@@ -215,53 +215,30 @@
ctx = event.Start(ctx, "CreateIssues")
defer event.End(ctx)
+ if err := createCVEIssues(ctx, st, ic, limit); err != nil {
+ return err
+ }
+ return createGHSAIssues(ctx, st, ic, limit)
+}
+
+func createCVEIssues(ctx context.Context, st store.Store, ic issues.Client, limit int) (err error) {
+ defer derrors.Wrap(&err, "createCVEIssues(destination: %s)", ic.Destination())
+
needsIssue, err := st.ListCVERecordsWithTriageState(ctx, store.TriageStateNeedsIssue)
if err != nil {
return err
}
- log.Infof(ctx, "CreateIssues starting; destination: %s, total needing issue: %d",
+ log.Infof(ctx, "createCVEIssues starting; destination: %s, total needing issue: %d",
ic.Destination(), len(needsIssue))
- numCreated := int64(0)
+ numCreated := 0
for _, cr := range needsIssue {
- if limit > 0 && int(numCreated) >= limit {
+ if limit > 0 && numCreated >= limit {
break
}
- if cr.IssueReference != "" || !cr.IssueCreatedAt.IsZero() {
- log.With(
- "CVE", cr.ID,
- "IssueReference", cr.IssueReference,
- "IssueCreatedAt", cr.IssueCreatedAt,
- ).Errorf(ctx, "%s: triage state is NeedsIssue but issue field(s) non-zero; skipping", cr.ID)
- continue
- }
- body, err := newBody(cr)
+ ref, err := createIssue(ctx, cr, ic, newCVEBody)
if err != nil {
- log.With(
- "CVE", cr.ID,
- "IssueReference", cr.IssueReference,
- "IssueCreatedAt", cr.IssueCreatedAt,
- ).Errorf(ctx, "%s: triage state is NeedsIssue but could not generate body; skipping: %v", cr.ID, err)
- continue
- }
-
- // Create the issue.
- iss := &issues.Issue{
- Title: fmt.Sprintf("x/vulndb: potential Go vuln in %s: %s", cr.Module, cr.ID),
- Body: body,
- }
- if err := issueRateLimiter.Wait(ctx); err != nil {
return err
}
- num, err := ic.CreateIssue(ctx, iss)
- if err != nil {
- return fmt.Errorf("creating issue for %s: %w", cr.ID, err)
- }
- // If we crashed here, we would have filed an issue without recording
- // that fact in the DB. That can lead to duplicate issues, but nothing
- // worse (we won't miss a CVE).
- // TODO(https://go.dev/issue/49733): look for the issue title to avoid duplications.
- ref := ic.Reference(num)
- log.With("CVE", cr.ID).Infof(ctx, "created issue %s for %s", ref, cr.ID)
// Update the CVERecord in the DB with issue information.
err = st.RunTransaction(ctx, func(ctx context.Context, tx store.Transaction) error {
@@ -280,11 +257,12 @@
}
numCreated++
}
- log.With("limit", limit).Infof(ctx, "CreateIssues done: %d created", numCreated)
+ log.With("limit", limit).Infof(ctx, "createCVEIssues done: %d created", numCreated)
return nil
}
-func newBody(cr *store.CVERecord) (string, error) {
+func newCVEBody(sr storeRecord) (string, error) {
+ cr := sr.(*store.CVERecord)
var b strings.Builder
if cr.CVE == nil {
return "", fmt.Errorf("cannot create body for CVERecord with nil CVE")
@@ -317,6 +295,112 @@
return b.String(), nil
}
+func createGHSAIssues(ctx context.Context, st store.Store, ic issues.Client, limit int) (err error) {
+ defer derrors.Wrap(&err, "createGHSAIssues(destination: %s)", ic.Destination())
+
+ sas, err := getGHSARecords(ctx, st)
+ if err != nil {
+ return err
+ }
+ var needsIssue []*store.GHSARecord
+ for _, sa := range sas {
+ if sa.TriageState == store.TriageStateNeedsIssue {
+ needsIssue = append(needsIssue, sa)
+ }
+ }
+
+ log.Infof(ctx, "createGHSAIssues starting; destination: %s, total needing issue: %d",
+ ic.Destination(), len(needsIssue))
+ numCreated := 0
+ for _, gr := range needsIssue {
+ if limit > 0 && numCreated >= limit {
+ break
+ }
+ ref, err := createIssue(ctx, gr, ic, newGHSABody)
+ if err != nil {
+ return err
+ }
+ // Update the GHSARecord in the DB with issue information.
+ err = st.RunTransaction(ctx, func(ctx context.Context, tx store.Transaction) error {
+ r, err := tx.GetGHSARecord(gr.GetID())
+ if err != nil {
+ return err
+ }
+ r.TriageState = store.TriageStateIssueCreated
+ r.IssueReference = ref
+ r.IssueCreatedAt = time.Now()
+ return tx.SetGHSARecord(r)
+ })
+ if err != nil {
+ return err
+ }
+ numCreated++
+ }
+ log.With("limit", limit).Infof(ctx, "createGHSAIssues done: %d created", numCreated)
+ return nil
+}
+
+func newGHSABody(sr storeRecord) (string, error) {
+ sa := sr.(*store.GHSARecord).GHSA
+
+ var b strings.Builder
+ intro := fmt.Sprintf(
+ "In GitHub Security Advisory [%s](%s), there is a vulnerability in the Go package or module %s.",
+ sr.GetPrettyID(), sa.Permalink, sa.Vulns[0].Package)
+ if err := issueTemplate.Execute(&b, issueTemplateData{
+ Intro: intro,
+ }); err != nil {
+ return "", err
+ }
+ return b.String(), nil
+}
+
+type storeRecord interface {
+ GetID() string
+ GetPrettyID() string
+ GetUnit() string
+ GetIssueReference() string
+ GetIssueCreatedAt() time.Time
+}
+
+func createIssue(ctx context.Context, r storeRecord, ic issues.Client, newBody func(storeRecord) (string, error)) (ref string, err error) {
+ id := r.GetID()
+ defer derrors.Wrap(&err, "createIssue(%s)", id)
+
+ if r.GetIssueReference() != "" || !r.GetIssueCreatedAt().IsZero() {
+ log.With(
+ "ID", id,
+ "IssueReference", r.GetIssueReference(),
+ "IssueCreatedAt", r.GetIssueCreatedAt(),
+ ).Errorf(ctx, "%s: triage state is NeedsIssue but issue field(s) non-zero; skipping", id)
+ return "", nil
+ }
+ body, err := newBody(r)
+ if err != nil {
+ log.With("ID", id).Errorf(ctx, "%s: triage state is NeedsIssue but could not generate body; skipping: %v", id, err)
+ return "", nil
+ }
+ // Create the issue.
+ iss := &issues.Issue{
+ Title: fmt.Sprintf("x/vulndb: potential Go vuln in %s: %s", r.GetUnit(), r.GetPrettyID()),
+ Body: body,
+ }
+ if err := issueRateLimiter.Wait(ctx); err != nil {
+ return "", err
+ }
+ num, err := ic.CreateIssue(ctx, iss)
+ if err != nil {
+ return "", fmt.Errorf("creating issue for %s: %w", id, err)
+ }
+ // If we crashed here, we would have filed an issue without recording
+ // that fact in the DB. That can lead to duplicate issues, but nothing
+ // worse (we won't miss a CVE).
+ // TODO(https://go.dev/issue/49733): look for the issue title to avoid duplications.
+ ref = ic.Reference(num)
+ log.With("ID", id).Infof(ctx, "created issue %s for %s", ref, id)
+ return ref, nil
+}
+
type issueTemplateData struct {
Intro string
Report string
@@ -327,9 +411,11 @@
var issueTemplate = template.Must(template.New("issue").Parse(`
{{- .Intro}}
+{{if (and .Pre .Report) -}}
{{.Pre}}
{{.Report}}
{{.Pre}}
+{{- end}}
See [doc/triage.md](https://github.com/golang/vulndb/blob/master/doc/triage.md) for instructions on how to triage this report.
`))
diff --git a/internal/worker/worker_test.go b/internal/worker/worker_test.go
index c6a3fcb..5ed3cd8 100644
--- a/internal/worker/worker_test.go
+++ b/internal/worker/worker_test.go
@@ -9,6 +9,7 @@
import (
"context"
+ "fmt"
"math"
"os"
"sort"
@@ -137,32 +138,71 @@
},
}
createCVERecords(t, mstore, crs)
+ grs := []*store.GHSARecord{
+ {
+ GHSA: &ghsa.SecurityAdvisory{
+ ID: "g1",
+ Vulns: []*ghsa.Vuln{{Package: "p1"}},
+ },
+ TriageState: store.TriageStateNeedsIssue,
+ },
+ {
+ GHSA: &ghsa.SecurityAdvisory{
+ ID: "g2",
+ Vulns: []*ghsa.Vuln{{Package: "p2"}},
+ },
+ TriageState: store.TriageStateNoActionNeeded,
+ },
+ {
+ GHSA: &ghsa.SecurityAdvisory{
+ ID: "g3",
+ Vulns: []*ghsa.Vuln{{Package: "p3"}},
+ },
+ TriageState: store.TriageStateIssueCreated,
+ },
+ }
+ createGHSARecords(t, mstore, grs)
if err := CreateIssues(ctx, mstore, ic, 0); err != nil {
t.Fatal(err)
}
- var wants []*store.CVERecord
+ var wantCVERecords []*store.CVERecord
for _, r := range crs {
copy := *r
- wants = append(wants, ©)
+ wantCVERecords = append(wantCVERecords, ©)
}
- wants[0].TriageState = store.TriageStateIssueCreated
- wants[0].IssueReference = "inMemory#1"
+ wantCVERecords[0].TriageState = store.TriageStateIssueCreated
+ wantCVERecords[0].IssueReference = "inMemory#1"
- gotRecs := mstore.CVERecords()
- if len(gotRecs) != len(wants) {
- t.Fatalf("wrong number of records: got %d, want %d", len(gotRecs), len(wants))
+ gotCVERecs := mstore.CVERecords()
+ if len(gotCVERecs) != len(wantCVERecords) {
+ t.Fatalf("wrong number of records: got %d, want %d", len(gotCVERecs), len(wantCVERecords))
}
- for _, want := range wants {
- got := gotRecs[want.ID]
+ for _, want := range wantCVERecords {
+ got := gotCVERecs[want.ID]
if !cmp.Equal(got, want, cmpopts.IgnoreFields(store.CVERecord{}, "IssueCreatedAt")) {
- t.Errorf("got %+v\nwant %+v", got, want)
+ t.Errorf("\ngot %+v\nwant %+v", got, want)
}
}
+
+ var wantGHSARecs []*store.GHSARecord
+ for _, r := range grs {
+ copy := *r
+ wantGHSARecs = append(wantGHSARecs, ©)
+ }
+ wantGHSARecs[0].TriageState = store.TriageStateIssueCreated
+ wantGHSARecs[0].IssueReference = "inMemory#2"
+
+ gotGHSARecs := getGHSARecordsSorted(t, mstore)
+ fmt.Printf("%+v\n", gotGHSARecs[0])
+ if diff := cmp.Diff(wantGHSARecs, gotGHSARecs,
+ cmpopts.IgnoreFields(store.GHSARecord{}, "IssueCreatedAt")); diff != "" {
+ t.Errorf("mismatch (-want, +got):\n%s", diff)
+ }
}
-func TestNewBody(t *testing.T) {
+func TestNewCVEBody(t *testing.T) {
r := &store.CVERecord{
ID: "ID1",
Module: "a.Module",
@@ -175,7 +215,7 @@
},
},
}
- got, err := newBody(r)
+ got, err := newCVEBody(r)
if err != nil {
t.Fatal(err)
}
@@ -197,6 +237,31 @@
}
}
+func TestNewGHSABody(t *testing.T) {
+ r := &store.GHSARecord{
+ GHSA: &ghsa.SecurityAdvisory{
+ ID: "G1_blah",
+ Identifiers: []ghsa.Identifier{{Type: "GHSA", Value: "G1"}},
+ Permalink: "https://github.com/permalink/to/G1",
+ Description: "a description",
+ Vulns: []*ghsa.Vuln{{Package: "aPackage"}},
+ },
+ }
+ got, err := newGHSABody(r)
+ if err != nil {
+ t.Fatal(err)
+ }
+ want := `In GitHub Security Advisory [G1](https://github.com/permalink/to/G1), there is a vulnerability in the Go package or module aPackage.
+
+
+
+See [doc/triage.md](https://github.com/golang/vulndb/blob/master/doc/triage.md) for instructions on how to triage this report.
+`
+ if diff := cmp.Diff(unindent(want), got); diff != "" {
+ t.Errorf("mismatch (-want, +got):\n%s", diff)
+ }
+}
+
// unindent removes leading whitespace from s.
// It first finds the line beginning with the fewest space and tab characters.
// It then removes that many characters from every line.
@@ -258,13 +323,7 @@
if gotStats != wantStats {
t.Errorf("\ngot %+v\nwant %+v", gotStats, wantStats)
}
- gotRecords, err := getGHSARecords(ctx, mstore)
- if err != nil {
- t.Fatal(err)
- }
- sort.Slice(gotRecords, func(i, j int) bool {
- return gotRecords[i].GHSA.ID < gotRecords[j].GHSA.ID
- })
+ gotRecords := getGHSARecordsSorted(t, mstore)
if diff := cmp.Diff(wantRecords, gotRecords); diff != "" {
t.Errorf("mismatch (-want, +got):\n%s", diff)
}
@@ -301,6 +360,16 @@
}
+func getGHSARecordsSorted(t *testing.T, st store.Store) []*store.GHSARecord {
+ t.Helper()
+ rs, err := getGHSARecords(context.Background(), st)
+ if err != nil {
+ t.Fatal(err)
+ }
+ sort.Slice(rs, func(i, j int) bool { return rs[i].GHSA.ID < rs[j].GHSA.ID })
+ return rs
+}
+
func fakeListFunc(sas []*ghsa.SecurityAdvisory) GHSAListFunc {
return func(_ context.Context, since time.Time) ([]*ghsa.SecurityAdvisory, error) {
var rs []*ghsa.SecurityAdvisory
