diff --git a/README.md b/README.md index a2e0c2e..5edbb7a 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,7 @@ entries. It runs as an aggregated API server under Milo, like Early. Every resource in the design has a v1alpha1 type in `pkg/apis/feedback/v1alpha1` and a sample in `config/samples`, and the server -serves everything except `SecurityReport` and `SecurityAcknowledgement`. Storage for the rest is tracked as +serves every resource in the design. Storage for the rest is tracked as sub-issues of the epic. | Resource | Storage | State | @@ -28,7 +28,7 @@ sub-issues of the epic. | `PostQuery` | Postgres (not stored) | served | | `Vote`, `Subscription` | Postgres | served | | `Submission`, `AttachmentUpload` | Postgres, bucket | served | -| `SecurityReport`, `SecurityAcknowledgement` | Postgres | planned | +| `SecurityReport`, `SecurityAcknowledgement` | Postgres, bucket | served | ## Layout @@ -169,6 +169,35 @@ to the submitter and staff. Files live in `--attachments-bucket` lifecycle rule deleting `uploads/` after a day. Without a bucket, attachments are off and submissions without them still work. +## Security reports + +Private vulnerability reports, with their own role: only the Security Team +can list them or write their status. Feedback reviewers can't see them; +Milo IAM enforces that (see #10). + +- **Report:** reporters create reports in the user context (verified email, + same rate limit as submissions) and read their own, without + `status.internal`. Names are generated (`sec-…`). Attachments are + `AttachmentUpload`s with `purpose: SecurityReport`, stored under their own + prefix and served by `securityreports/{name}/attachment?ref=` to the + reporter and the Security Team. +- **Log:** the Security Team logs reports from email or GitHub in the + platform context with `spec.reporter` set. Those belong to the platform. +- **Work:** the Security Team writes `status` through the status + subresource. A phase change gets a timeline entry if the team didn't add + one; timeline entries can be added but never changed. `NotApplicable` and + `Duplicate` need a reason, `Duplicate` needs `internal.duplicateOf`, and + `credits` rewards need an amount. +- **Move:** staff send a misfiled submission to the security track with + `create` on `submissions/{name}/to-security-report`. In one transaction a + report owned by the submitter is created from it, its attachments are + copied, and the submission is deleted. +- **Acknowledge:** `SecurityAcknowledgement` lists reports in `Fixed` or + `Disclosed` whose reporter chose public credit, with only the credited + name, link, month fixed and severity. + +Reports are never embedded, indexed, synced or sent to the AI gateway. + ## Development ```bash diff --git a/internal/apiserver/apiserver.go b/internal/apiserver/apiserver.go index 632e16c..3c44257 100644 --- a/internal/apiserver/apiserver.go +++ b/internal/apiserver/apiserver.go @@ -26,6 +26,7 @@ import ( "go.miloapis.com/feedback/internal/registry/post" "go.miloapis.com/feedback/internal/registry/postquery" "go.miloapis.com/feedback/internal/registry/release" + "go.miloapis.com/feedback/internal/registry/securityreport" "go.miloapis.com/feedback/internal/registry/submission" "go.miloapis.com/feedback/internal/registry/vote" "go.miloapis.com/feedback/internal/storage/pg" @@ -259,8 +260,20 @@ func (c completedConfig) New(ctx context.Context) (*FeedbackServer, error) { } // Uploads: 20 per 10 minutes per user, per replica. v1alpha1Storage["attachmentuploads"] = attachmentupload.NewREST(files, ratelimit.NewLimiter(2, 20)) + reports, err := securityreport.NewREST(db, Scheme, securityreport.Options{ + Identities: ids, Files: files, Limit: sc.Limit, Window: sc.Window, + }) + if err != nil { + pool.Close() + return nil, fmt.Errorf("create SecurityReport storage: %w", err) + } + v1alpha1Storage["securityreports"] = reports.Store + v1alpha1Storage["securityreports/status"] = reports.Status + v1alpha1Storage["securityreports/attachment"] = reports.Attachment + v1alpha1Storage["securityacknowledgements"] = securityreport.NewAcknowledgementREST(db) + submissions, err := submission.NewREST(db, Scheme, submission.Options{ - Identities: ids, Files: files, Votes: votes, Subscriptions: subs, + Identities: ids, Files: files, Votes: votes, Subscriptions: subs, Reports: reports, Limit: sc.Limit, Window: sc.Window, }) if err != nil { @@ -271,9 +284,7 @@ func (c completedConfig) New(ctx context.Context) (*FeedbackServer, error) { v1alpha1Storage["submissions/status"] = submissions.Status v1alpha1Storage["submissions/review"] = submissions.Review v1alpha1Storage["submissions/attachment"] = submissions.Attachment - - // Other Postgres-backed resources (SecurityReport, ...) are added here as - // they are implemented. See datum-cloud/enhancements#943. + v1alpha1Storage["submissions/to-security-report"] = submissions.ToSecurity apiGroupInfo.VersionedResourcesStorageMap[feedbackv1alpha1.SchemeGroupVersion.Version] = v1alpha1Storage if err := genericServer.InstallAPIGroup(&apiGroupInfo); err != nil { diff --git a/internal/attachments/attachments.go b/internal/attachments/attachments.go index e1ddcf8..dd6e4a1 100644 --- a/internal/attachments/attachments.go +++ b/internal/attachments/attachments.go @@ -157,9 +157,10 @@ func (s *Store) Attach(ctx context.Context, purpose v1.AttachmentPurpose, owner, return out, nil } -// Move copies a parent's attachments to another parent and deletes the -// originals, e.g. when a submission becomes a security report. -func (s *Store) Move(ctx context.Context, from, to v1.AttachmentPurpose, fromParent, toParent string) error { +// CopyParent copies a parent's attachments to another parent, e.g. when a +// submission becomes a security report. Deleting the old parent deletes the +// originals. +func (s *Store) CopyParent(ctx context.Context, from, to v1.AttachmentPurpose, fromParent, toParent string) error { src := ParentPrefix(from, fromParent) it := s.bucket.List(&blob.ListOptions{Prefix: src}) for { @@ -173,9 +174,6 @@ func (s *Store) Move(ctx context.Context, from, to v1.AttachmentPurpose, fromPar if err := s.bucket.Copy(ctx, ParentPrefix(to, toParent)+strings.TrimPrefix(obj.Key, src), obj.Key, nil); err != nil { return err } - if err := s.bucket.Delete(ctx, obj.Key); err != nil { - return err - } } } diff --git a/internal/registry/attachmentupload/download.go b/internal/registry/attachmentupload/download.go new file mode 100644 index 0000000..da553a1 --- /dev/null +++ b/internal/registry/attachmentupload/download.go @@ -0,0 +1,83 @@ +package attachmentupload + +import ( + "context" + "errors" + "mime" + "net/http" + "slices" + "strconv" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apiserver/pkg/registry/rest" + "k8s.io/klog/v2" + + "go.miloapis.com/feedback/internal/attachments" + v1 "go.miloapis.com/feedback/pkg/apis/feedback/v1alpha1" +) + +// DownloadREST serves GET {resource}/{name}/attachment?ref=: one attached +// file, to whoever can read the parent. +type DownloadREST struct { + // Get reads the parent as the caller and returns its attachments. It + // must apply the parent's own access rules (scope, redaction). + Get func(ctx context.Context, name string) ([]v1.AttachmentReference, error) + NewFunc func() runtime.Object + Files *attachments.Store + Purpose v1.AttachmentPurpose + // Resource names the subresource in NotFound errors. + Resource schema.GroupResource +} + +var _ rest.Connecter = &DownloadREST{} + +func (r *DownloadREST) New() runtime.Object { return r.NewFunc() } +func (r *DownloadREST) Destroy() {} + +func (r *DownloadREST) NewConnectOptions() (runtime.Object, bool, string) { + return &v1.AttachmentOptions{}, false, "" +} + +func (r *DownloadREST) ConnectMethods() []string { return []string{http.MethodGet} } + +// Connect checks the caller can read the parent and that ref is attached to +// it, then returns a handler that sends the file. +func (r *DownloadREST) Connect(ctx context.Context, name string, opts runtime.Object, _ rest.Responder) (http.Handler, error) { + ref := opts.(*v1.AttachmentOptions).Ref + if ref == "" { + return nil, apierrors.NewBadRequest("set the ref query parameter to an attachment's uploadRef") + } + refs, err := r.Get(ctx, name) + if err != nil { + return nil, err + } + if !slices.ContainsFunc(refs, func(a v1.AttachmentReference) bool { return a.UploadRef == ref }) { + return nil, apierrors.NewNotFound(r.Resource, ref) + } + if r.Files == nil { + return nil, apierrors.NewServiceUnavailable("Attachments are not available.") + } + f, err := r.Files.Get(ctx, r.Purpose, name, ref) + if errors.Is(err, attachments.ErrNotFound) { + return nil, apierrors.NewNotFound(r.Resource, ref) + } + if err != nil { + klog.ErrorS(err, "Reading attachment failed", "resource", r.Resource, "name", name, "ref", ref) + return nil, apierrors.NewServiceUnavailable("Attachments are not available.") + } + return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { serveFile(w, f) }), nil +} + +// serveFile sends a file as a download that browsers won't render or run. +func serveFile(w http.ResponseWriter, f *attachments.File) { + h := w.Header() + h.Set("Content-Type", f.ContentType) + h.Set("Content-Length", strconv.Itoa(len(f.Data))) + h.Set("Content-Disposition", mime.FormatMediaType("attachment", map[string]string{"filename": f.Name})) + h.Set("X-Content-Type-Options", "nosniff") + h.Set("Content-Security-Policy", "sandbox; default-src 'none'") + h.Set("Cache-Control", "private, no-store") + _, _ = w.Write(f.Data) +} diff --git a/internal/registry/securityreport/acknowledgement.go b/internal/registry/securityreport/acknowledgement.go new file mode 100644 index 0000000..b84ca36 --- /dev/null +++ b/internal/registry/securityreport/acknowledgement.go @@ -0,0 +1,175 @@ +package securityreport + +import ( + "context" + "encoding/json" + "fmt" + "slices" + "strings" + "time" + + "github.com/jackc/pgx/v5" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/registry/rest" + + "go.miloapis.com/feedback/internal/storage/pg" + v1 "go.miloapis.com/feedback/pkg/apis/feedback/v1alpha1" +) + +// AcknowledgementREST serves SecurityAcknowledgement: a read-only view of +// reports in Fixed or Disclosed whose reporter chose public credit. Each +// holds the credited name, link, month and severity, and nothing else from +// the report. +type AcknowledgementREST struct { + db *pg.DB + table rest.TableConvertor +} + +var ( + _ rest.Getter = &AcknowledgementREST{} + _ rest.Lister = &AcknowledgementREST{} + _ rest.Scoper = &AcknowledgementREST{} + _ rest.SingularNameProvider = &AcknowledgementREST{} +) + +// NewAcknowledgementREST returns the view. +func NewAcknowledgementREST(db *pg.DB) *AcknowledgementREST { + return &AcknowledgementREST{db: db, table: ackTable{}} +} + +func (r *AcknowledgementREST) New() runtime.Object { return &v1.SecurityAcknowledgement{} } +func (r *AcknowledgementREST) NewList() runtime.Object { return &v1.SecurityAcknowledgementList{} } +func (r *AcknowledgementREST) Destroy() {} +func (r *AcknowledgementREST) NamespaceScoped() bool { return false } +func (r *AcknowledgementREST) GetSingularName() string { return "securityacknowledgement" } +func (r *AcknowledgementREST) ConvertToTable(ctx context.Context, obj, opts runtime.Object) (*metav1.Table, error) { + return r.table.ConvertToTable(ctx, obj, opts) +} + +const ackQuery = `SELECT name, created_at, object->'spec'->'credit', object->'status' FROM security_reports + WHERE phase IN ('Fixed', 'Disclosed') + AND coalesce((object->'spec'->'credit'->>'public')::boolean, false) + AND coalesce(object->'spec'->'credit'->>'name', '') <> ''` + +// Get returns one acknowledgement, named after its report. +func (r *AcknowledgementREST) Get(ctx context.Context, name string, _ *metav1.GetOptions) (runtime.Object, error) { + acks, err := r.query(ctx, ackQuery+" AND name = $1", name) + if err != nil { + return nil, err + } + if len(acks) == 0 { + return nil, apierrors.NewNotFound(v1.Resource("securityacknowledgements"), name) + } + return &acks[0], nil +} + +// List returns every acknowledgement, newest month first. The list is short, +// so it isn't paged; selectors aren't supported. +func (r *AcknowledgementREST) List(ctx context.Context, opts *metainternalversion.ListOptions) (runtime.Object, error) { + if opts != nil && ((opts.LabelSelector != nil && !opts.LabelSelector.Empty()) || + (opts.FieldSelector != nil && !opts.FieldSelector.Empty() && !isNameSelector(opts))) { + return nil, apierrors.NewBadRequest("securityacknowledgements support no selectors except metadata.name") + } + q, args := ackQuery, []any{} + if opts != nil && isNameSelector(opts) { + v, _ := opts.FieldSelector.RequiresExactMatch("metadata.name") + q, args = q+" AND name = $1", []any{v} + } + acks, err := r.query(ctx, q, args...) + if err != nil { + return nil, err + } + slices.SortFunc(acks, func(a, b v1.SecurityAcknowledgement) int { + if c := strings.Compare(b.Status.Month, a.Status.Month); c != 0 { + return c + } + return strings.Compare(a.Name, b.Name) + }) + return &v1.SecurityAcknowledgementList{Items: acks}, nil +} + +func isNameSelector(opts *metainternalversion.ListOptions) bool { + if opts.FieldSelector == nil { + return false + } + _, ok := opts.FieldSelector.RequiresExactMatch("metadata.name") + return ok && len(opts.FieldSelector.Requirements()) == 1 +} + +func (r *AcknowledgementREST) query(ctx context.Context, q string, args ...any) ([]v1.SecurityAcknowledgement, error) { + rows, err := r.db.Q(ctx).Query(ctx, q, args...) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + acks, err := pgx.CollectRows(rows, func(row pgx.CollectableRow) (v1.SecurityAcknowledgement, error) { + var ( + name string + created time.Time + rawCredit, raw []byte + credit v1.SecurityCredit + st v1.SecurityReportStatus + ) + if err := row.Scan(&name, &created, &rawCredit, &raw); err != nil { + return v1.SecurityAcknowledgement{}, err + } + if err := json.Unmarshal(rawCredit, &credit); err != nil { + return v1.SecurityAcknowledgement{}, fmt.Errorf("decode credit of %s: %w", name, err) + } + if err := json.Unmarshal(raw, &st); err != nil { + return v1.SecurityAcknowledgement{}, fmt.Errorf("decode status of %s: %w", name, err) + } + return v1.SecurityAcknowledgement{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Status: v1.SecurityAcknowledgementStatus{ + Name: credit.Name, URL: credit.URL, Month: fixedMonth(st, created), Severity: st.Severity, + }, + }, nil + }) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + return acks, nil +} + +// fixedMonth is when the report was fixed: its first Fixed timeline entry, +// else its first Disclosed one, else when it was received. +func fixedMonth(st v1.SecurityReportStatus, received time.Time) string { + for _, phase := range []v1.SecurityReportPhase{v1.SecurityReportPhaseFixed, v1.SecurityReportPhaseDisclosed} { + for _, e := range st.Timeline { + if e.Phase == phase { + return e.At.UTC().Format("2006-01") + } + } + } + return received.UTC().Format("2006-01") +} + +type ackTable struct{} + +func (ackTable) ConvertToTable(_ context.Context, obj runtime.Object, _ runtime.Object) (*metav1.Table, error) { + t := &metav1.Table{ColumnDefinitions: []metav1.TableColumnDefinition{ + {Name: "Name", Type: "string", Format: "name"}, + {Name: "Credit", Type: "string"}, + {Name: "Month", Type: "string"}, + {Name: "Severity", Type: "string"}, + {Name: "URL", Type: "string", Priority: 1}, + }} + add := func(a *v1.SecurityAcknowledgement) { + t.Rows = append(t.Rows, metav1.TableRow{ + Object: runtime.RawExtension{Object: a}, + Cells: []any{a.Name, a.Status.Name, a.Status.Month, string(a.Status.Severity), a.Status.URL}, + }) + } + switch o := obj.(type) { + case *v1.SecurityAcknowledgement: + add(o) + case *v1.SecurityAcknowledgementList: + for i := range o.Items { + add(&o.Items[i]) + } + } + return t, nil +} diff --git a/internal/registry/securityreport/securityreport_test.go b/internal/registry/securityreport/securityreport_test.go new file mode 100644 index 0000000..08a77d2 --- /dev/null +++ b/internal/registry/securityreport/securityreport_test.go @@ -0,0 +1,331 @@ +package securityreport + +import ( + "context" + "net/http/httptest" + "os" + "strings" + "testing" + "time" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/registry/rest" + + "go.miloapis.com/feedback/internal/attachments" + "go.miloapis.com/feedback/internal/identity" + "go.miloapis.com/feedback/internal/registry/attachmentupload" + "go.miloapis.com/feedback/internal/storage/pg" + "go.miloapis.com/feedback/internal/storage/pg/pgtest" + "go.miloapis.com/feedback/pkg/apis/feedback/install" + v1 "go.miloapis.com/feedback/pkg/apis/feedback/v1alpha1" +) + +func TestMain(m *testing.M) { os.Exit(pgtest.Main(m)) } + +var ( + ids = identity.Identities{Sync: []string{"sync"}, Controller: []string{"controller"}} + secTeam = pgtest.AsPlatform("sec@datum.net") + syncCtx = pgtest.AsPlatform("sync") + alice = pgtest.AsUser("alice") + bob = pgtest.AsUser("bob") +) + +type env struct { + db *pg.DB + r *REST + acks *AcknowledgementREST + files *attachments.Store + uploads *attachmentupload.REST +} + +func setup(t *testing.T) env { + t.Helper() + db := pgtest.Require(t, "security_reports") + scheme := runtime.NewScheme() + install.Install(scheme) + files, err := attachments.Open(context.Background(), "mem://") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = files.Close() }) + r, err := NewREST(db, scheme, Options{Identities: ids, Files: files, Limit: 3, Window: 10 * time.Minute}) + if err != nil { + t.Fatal(err) + } + return env{db, r, NewAcknowledgementREST(db), files, attachmentupload.NewREST(files, nil)} +} + +func spec() v1.SecurityReportSpec { + return v1.SecurityReportSpec{Title: "Viewer reads connector credentials", Description: "Steps...", + Credit: &v1.SecurityCredit{Public: true, Name: "Ada L.", URL: "https://example.com"}} +} + +func (e env) report(t *testing.T, ctx context.Context, s v1.SecurityReportSpec) *v1.SecurityReport { + t.Helper() + obj, err := e.r.Create(ctx, &v1.SecurityReport{Spec: s}, nil, &metav1.CreateOptions{}) + if err != nil { + t.Fatalf("report: %v", err) + } + return obj.(*v1.SecurityReport) +} + +func (e env) get(t *testing.T, ctx context.Context, name string) *v1.SecurityReport { + t.Helper() + obj, err := e.r.Get(ctx, name, &metav1.GetOptions{}) + if err != nil { + t.Fatalf("get %s: %v", name, err) + } + return obj.(*v1.SecurityReport) +} + +func (e env) setStatus(ctx context.Context, name string, f func(*v1.SecurityReportStatus)) (*v1.SecurityReport, error) { + obj, _, err := e.r.Status.Update(ctx, name, rest.DefaultUpdatedObjectInfo(nil, func(_ context.Context, _, old runtime.Object) (runtime.Object, error) { + r := old.DeepCopyObject().(*v1.SecurityReport) + f(&r.Status) + return r, nil + }), nil, nil, false, &metav1.UpdateOptions{}) + if err != nil { + return nil, err + } + return obj.(*v1.SecurityReport), nil +} + +func TestReporterFlow(t *testing.T) { + e := setup(t) + in := &v1.SecurityReport{ObjectMeta: metav1.ObjectMeta{Name: "chosen"}, Spec: spec(), + Status: v1.SecurityReportStatus{Phase: v1.SecurityReportPhaseFixed, Internal: &v1.SecurityReportInternal{Notes: "x"}}} + obj, err := e.r.Create(alice, in, nil, &metav1.CreateOptions{}) + if err != nil { + t.Fatal(err) + } + r := obj.(*v1.SecurityReport) + if !strings.HasPrefix(r.Name, "sec-") || r.Status.Phase != v1.SecurityReportPhaseReceived || + len(r.Status.Timeline) != 1 || r.Status.Internal != nil { + t.Errorf("created: %s %+v", r.Name, r.Status) + } + if _, err := e.r.Get(bob, r.Name, &metav1.GetOptions{}); !apierrors.IsNotFound(err) { + t.Errorf("bob reads alice's report: %v", err) + } + if got := e.get(t, secTeam, r.Name); got.Annotations[pg.OwnerAnnotation] != "alice" { + t.Errorf("owner: %v", got.Annotations) + } + + withReporter := spec() + withReporter.Reporter = &v1.ExternalReporter{Email: "x@example.com"} + unverified := request.WithUser(request.WithNamespace(context.Background(), ""), &user.DefaultInfo{Name: "carol", Extra: map[string][]string{ + pg.ParentKindExtraKey: {"User"}, pg.ParentNameExtraKey: {"carol"}, "iam.miloapis.com/emailverified": {"false"}}}) + badURL := spec() + badURL.Credit.URL = "javascript:alert(1)" + noName := spec() + noName.Credit.Name = "" + for name, c := range map[string]struct { + ctx context.Context + spec v1.SecurityReportSpec + want func(error) bool + }{ + "reporter from user": {alice, withReporter, apierrors.IsInvalid}, + "unverified": {unverified, spec(), apierrors.IsForbidden}, + "credit url": {alice, badURL, apierrors.IsInvalid}, + "public without name": {alice, noName, apierrors.IsInvalid}, + "org context": {pgtest.AsContext("Organization", "acme"), spec(), apierrors.IsForbidden}, + } { + if _, err := e.r.Create(c.ctx, &v1.SecurityReport{Spec: c.spec}, nil, &metav1.CreateOptions{}); !c.want(err) { + t.Errorf("%s: %v", name, err) + } + } + + r.Labels = map[string]string{"a": "b"} + if _, _, err := e.r.Update(alice, r.Name, rest.DefaultUpdatedObjectInfo(r), nil, nil, false, &metav1.UpdateOptions{}); !apierrors.IsForbidden(err) { + t.Errorf("reporter update: %v", err) + } + if _, _, err := e.r.Delete(alice, r.Name, nil, &metav1.DeleteOptions{}); !apierrors.IsForbidden(err) { + t.Errorf("reporter delete: %v", err) + } + if _, err := e.setStatus(alice, r.Name, func(st *v1.SecurityReportStatus) { st.Phase = v1.SecurityReportPhaseFixed }); err == nil { + t.Error("reporter status write allowed") + } + + // Rate limit: 3 per window, the first already counts. + e.report(t, alice, spec()) + e.report(t, alice, spec()) + if _, err := e.r.Create(alice, &v1.SecurityReport{Spec: spec()}, nil, &metav1.CreateOptions{}); !apierrors.IsTooManyRequests(err) { + t.Errorf("4th report: %v", err) + } +} + +func TestSecurityTeamStatus(t *testing.T) { + e := setup(t) + r := e.report(t, alice, spec()) + + out, err := e.setStatus(secTeam, r.Name, func(st *v1.SecurityReportStatus) { + st.Phase = v1.SecurityReportPhaseTriaged + st.Severity = v1.SeverityHigh + st.CVSSVector = "CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:H/I:N/A:N" + st.Internal = &v1.SecurityReportInternal{Assignee: "sec@datum.net", Notes: "repro'd"} + }) + if err != nil { + t.Fatal(err) + } + if tl := out.Status.Timeline; len(tl) != 2 || tl[1].Phase != v1.SecurityReportPhaseTriaged { + t.Errorf("timeline not appended: %+v", tl) + } + mine := e.get(t, alice, r.Name) + if mine.Status.Internal != nil || mine.Status.Severity != v1.SeverityHigh || len(mine.Status.Timeline) != 2 { + t.Errorf("reporter's view: %+v", mine.Status) + } + + // A note the team adds with the phase change is kept, not doubled. + out, err = e.setStatus(secTeam, r.Name, func(st *v1.SecurityReportStatus) { + st.Phase = v1.SecurityReportPhaseFixing + st.Timeline = append(st.Timeline, v1.SecurityTimelineEntry{Phase: v1.SecurityReportPhaseFixing, At: metav1.Now(), Note: "Fix in review"}) + }) + if err != nil || len(out.Status.Timeline) != 3 || out.Status.Timeline[2].Note != "Fix in review" { + t.Fatalf("timeline with note: %+v %v", out, err) + } + + for name, f := range map[string]func(*v1.SecurityReportStatus){ + "rewrite timeline": func(st *v1.SecurityReportStatus) { st.Timeline[0].Note = "edited" }, + "drop timeline": func(st *v1.SecurityReportStatus) { st.Timeline = st.Timeline[:1] }, + "bad severity": func(st *v1.SecurityReportStatus) { st.Severity = "spicy" }, + "n/a no reason": func(st *v1.SecurityReportStatus) { st.Phase = v1.SecurityReportPhaseNotApplicable }, + "dup no original": func(st *v1.SecurityReportStatus) { st.Phase, st.Reason = v1.SecurityReportPhaseDuplicate, "dup" }, + "credits": func(st *v1.SecurityReportStatus) { st.Reward = &v1.SecurityReward{Kind: v1.RewardKindCredits} }, + "advisory http": func(st *v1.SecurityReportStatus) { st.Advisory = &v1.SecurityAdvisory{URL: "http://x"} }, + } { + if _, err := e.setStatus(secTeam, r.Name, f); !apierrors.IsInvalid(err) { + t.Errorf("%s: %v", name, err) + } + } + if _, err := e.setStatus(syncCtx, r.Name, func(st *v1.SecurityReportStatus) { st.Severity = v1.SeverityLow }); !apierrors.IsInvalid(err) { + t.Errorf("sync writes status: %v", err) + } + if _, err := e.setStatus(secTeam, r.Name, func(st *v1.SecurityReportStatus) { + st.Reward = &v1.SecurityReward{Kind: v1.RewardKindCredits, Amount: "250.00", CreditGrantRef: &v1.CreditGrantReference{Name: "g"}} + }); err != nil { + t.Errorf("credits reward: %v", err) + } +} + +func TestExternalReports(t *testing.T) { + e := setup(t) + s := spec() + s.Reporter = &v1.ExternalReporter{Email: "researcher@example.com"} + s.Context.Client = v1.ClientEmail + r := e.report(t, secTeam, s) + got := e.get(t, secTeam, r.Name) + if got.Annotations[pg.OwnerAnnotation] != "" || got.Spec.Reporter.Email != "researcher@example.com" { + t.Errorf("external report: %+v", got) + } + var scope string + _ = e.db.Pool.QueryRow(context.Background(), "SELECT scope_type FROM security_reports WHERE name = $1", r.Name).Scan(&scope) + if scope != pg.ScopePlatform { + t.Errorf("scope %q", scope) + } + if _, err := e.r.Create(secTeam, &v1.SecurityReport{Spec: spec()}, nil, &metav1.CreateOptions{}); !apierrors.IsInvalid(err) { + t.Errorf("platform report without reporter: %v", err) + } + if _, err := e.r.Create(syncCtx, &v1.SecurityReport{Spec: s}, nil, &metav1.CreateOptions{}); !apierrors.IsForbidden(err) { + t.Errorf("sync logs a report: %v", err) + } + list, err := e.r.List(alice, &metainternalversion.ListOptions{}) + if err != nil || len(list.(*v1.SecurityReportList).Items) != 0 { + t.Errorf("alice sees external reports: %v %v", list, err) + } +} + +func TestAcknowledgements(t *testing.T) { + e := setup(t) + mk := func(ctx context.Context, public bool, phases ...v1.SecurityReportPhase) string { + s := spec() + s.Credit.Public = public + r := e.report(t, ctx, s) + for _, p := range phases { + if _, err := e.setStatus(secTeam, r.Name, func(st *v1.SecurityReportStatus) { + st.Phase, st.Severity = p, v1.SeverityMedium + st.Internal = &v1.SecurityReportInternal{Notes: "secret"} + }); err != nil { + t.Fatal(err) + } + } + return r.Name + } + fixed := mk(alice, true, v1.SecurityReportPhaseTriaged, v1.SecurityReportPhaseFixed) + disclosed := mk(bob, true, v1.SecurityReportPhaseFixed, v1.SecurityReportPhaseDisclosed) + private := mk(pgtest.AsUser("carol"), false, v1.SecurityReportPhaseFixed) + open := mk(pgtest.AsUser("dave"), true, v1.SecurityReportPhaseTriaged) + // The month comes from the first Fixed entry. + if _, err := e.db.Pool.Exec(context.Background(), `UPDATE security_reports + SET object = jsonb_set(object, '{status,timeline,2,at}', '"2026-03-04T00:00:00Z"') WHERE name = $1`, fixed); err != nil { + t.Fatal(err) + } + + obj, err := e.acks.List(pgtest.AsPlatform("datum-net"), &metainternalversion.ListOptions{}) + if err != nil { + t.Fatal(err) + } + items := obj.(*v1.SecurityAcknowledgementList).Items + if len(items) != 2 || items[0].Name != disclosed || items[1].Name != fixed { + t.Fatalf("acknowledgements: %+v", items) + } + a := items[1].Status + if a.Name != "Ada L." || a.URL != "https://example.com" || a.Month != "2026-03" || a.Severity != v1.SeverityMedium { + t.Errorf("acknowledgement: %+v", a) + } + if _, err := e.acks.Get(alice, private, &metav1.GetOptions{}); !apierrors.IsNotFound(err) { + t.Errorf("private credit: %v", err) + } + if _, err := e.acks.Get(alice, open, &metav1.GetOptions{}); !apierrors.IsNotFound(err) { + t.Errorf("unfixed report: %v", err) + } + if _, err := e.acks.Get(alice, fixed, &metav1.GetOptions{}); err != nil { + t.Errorf("get: %v", err) + } +} + +func TestAttachments(t *testing.T) { + e := setup(t) + upload := func(ctx context.Context, p v1.AttachmentPurpose) string { + t.Helper() + obj, err := e.uploads.Create(ctx, &v1.AttachmentUpload{Spec: v1.AttachmentUploadSpec{ + Purpose: p, Name: "poc.txt", ContentType: "text/plain", Data: []byte("curl ...")}}, nil, &metav1.CreateOptions{}) + if err != nil { + t.Fatal(err) + } + return obj.(*v1.AttachmentUpload).Status.UploadRef + } + wrongPurpose := upload(alice, v1.AttachmentPurposeSubmission) + ref := upload(alice, v1.AttachmentPurposeSecurityReport) + + s := spec() + s.Attachments = []v1.AttachmentReference{{UploadRef: wrongPurpose}} + if _, err := e.r.Create(alice, &v1.SecurityReport{Spec: s}, nil, &metav1.CreateOptions{}); !apierrors.IsInvalid(err) { + t.Errorf("submission upload on a report: %v", err) + } + s.Attachments = []v1.AttachmentReference{{UploadRef: ref}} + r := e.report(t, alice, s) + if a := r.Spec.Attachments[0]; a.Name != "poc.txt" || a.ContentType != "text/plain" { + t.Errorf("attachment: %+v", a) + } + for _, c := range []struct { + ctx context.Context + want bool + }{{alice, true}, {secTeam, true}, {bob, false}} { + h, err := e.r.Attachment.Connect(c.ctx, r.Name, &v1.AttachmentOptions{Ref: ref}, nil) + if (err == nil) != c.want { + t.Errorf("download: %v", err) + continue + } + if err == nil { + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest("GET", "/", nil)) + if w.Body.String() != "curl ..." { + t.Errorf("body %q", w.Body.String()) + } + } + } +} diff --git a/internal/registry/securityreport/storage.go b/internal/registry/securityreport/storage.go new file mode 100644 index 0000000..10c0633 --- /dev/null +++ b/internal/registry/securityreport/storage.go @@ -0,0 +1,257 @@ +package securityreport + +import ( + "context" + "errors" + "fmt" + "math" + "time" + + "github.com/jackc/pgx/v5" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/duration" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/storage/names" + "k8s.io/klog/v2" + + "go.miloapis.com/feedback/internal/attachments" + "go.miloapis.com/feedback/internal/identity" + "go.miloapis.com/feedback/internal/registry/attachmentupload" + "go.miloapis.com/feedback/internal/storage/pg" + v1 "go.miloapis.com/feedback/pkg/apis/feedback/v1alpha1" +) + +// Options configures SecurityReport storage. +type Options struct { + Identities identity.Identities + // Files stores attachments. Nil disables them. + Files *attachments.Store + // Limit reports per reporter to Limit per Window. Zero disables it. + Limit int + Window time.Duration +} + +// REST is SecurityReport storage and its subresources. +type REST struct { + *pg.Store + Status *pg.StatusStore + Attachment *attachmentupload.DownloadREST +} + +var gr = v1.Resource("securityreports") + +type movedKey struct{} + +// WithMovedFiles marks ctx as creating a report whose attachments the caller +// copies itself (to-security-report), so create doesn't look for uploads. +func WithMovedFiles(ctx context.Context) context.Context { + return context.WithValue(ctx, movedKey{}, true) +} + +// createScope puts a reporter's report in their user scope, and a report the +// Security Team logs for an external reporter in the platform scope. +func createScope(ctx context.Context) (pg.Scope, error) { + switch kind, name := pg.RequestContext(ctx); kind { + case pg.ScopeUser: + return pg.Scope{Type: pg.ScopeUser, Name: name}, nil + case pg.ScopePlatform: + return pg.Scope{Type: pg.ScopePlatform}, nil + default: + return pg.Scope{}, fmt.Errorf("report from the user context, not %s", kind) + } +} + +// NewREST returns SecurityReport storage. +func NewREST(db *pg.DB, typer runtime.ObjectTyper, o Options) (*REST, error) { + s := strategy{ObjectTyper: typer, NameGenerator: names.SimpleNameGenerator, ids: o.Identities, now: time.Now} + h := hooks{db: db, o: o} + store, err := pg.NewStore(db, pg.Kind{ + Table: "security_reports", + NewFunc: func() runtime.Object { return &v1.SecurityReport{} }, + NewListFunc: func() runtime.Object { return &v1.SecurityReportList{} }, + DefaultQualifiedResource: gr, + SingularQualifiedResource: v1.Resource("securityreport"), + CreateStrategy: s, UpdateStrategy: s, DeleteStrategy: s, + Scope: pg.UserOwned, + CreateScope: createScope, + Columns: func(obj runtime.Object) []pg.Column { + r := obj.(*v1.SecurityReport) + return []pg.Column{{Name: "phase", Value: string(r.Status.Phase)}, {Name: "severity", Value: string(r.Status.Severity)}} + }, + FieldColumns: map[string]string{"status.phase": "phase", "status.severity": "severity"}, + BeforeWrite: h.beforeWrite, + AfterWrite: h.afterWrite, + Decorate: func(ctx context.Context, objs []runtime.Object) error { + if kind, _ := pg.RequestContext(ctx); kind != pg.ScopePlatform { + for _, obj := range objs { + redact(obj.(*v1.SecurityReport)) + } + } + return nil + }, + TableConvertor: table{}, + }) + if err != nil { + return nil, err + } + return &REST{ + Store: store, + Status: pg.NewStatusStore(store, statusStrategy{s}), + Attachment: &attachmentupload.DownloadREST{ + Get: func(ctx context.Context, name string) ([]v1.AttachmentReference, error) { + obj, err := store.Get(ctx, name, &metav1.GetOptions{}) + if err != nil { + return nil, err + } + return obj.(*v1.SecurityReport).Spec.Attachments, nil + }, + NewFunc: func() runtime.Object { return &v1.SecurityReport{} }, + Files: o.Files, + Purpose: v1.AttachmentPurposeSecurityReport, + Resource: v1.Resource("securityreports/attachment"), + }, + }, nil +} + +type hooks struct { + db *pg.DB + o Options +} + +func (h hooks) beforeWrite(ctx context.Context, op pg.Op, _, obj runtime.Object) error { + kind, owner := pg.RequestContext(ctx) + if op != pg.OpCreate { + if kind != pg.ScopePlatform { + return apierrors.NewForbidden(gr, "", errors.New("security reports can't be changed after they are sent")) + } + return nil + } + r := obj.(*v1.SecurityReport) + sp := field.NewPath("spec") + if moved, _ := ctx.Value(movedKey{}).(bool); moved { + return nil + } + if kind == pg.ScopePlatform { + // The Security Team logging a report from email or GitHub. + if h.o.Identities.Of(ctx) != identity.Other { + return apierrors.NewForbidden(gr, "", errors.New("security reports are logged by the Security Team")) + } + var errs field.ErrorList + if r.Spec.Reporter == nil { + errs = append(errs, field.Required(sp.Child("reporter"), "reports logged in the platform context are for an external reporter")) + } + if len(r.Spec.Attachments) > 0 { + errs = append(errs, field.Forbidden(sp.Child("attachments"), "attach files to logged reports in the tracking issue")) + } + if len(errs) > 0 { + return apierrors.NewInvalid(v1.Kind("SecurityReport"), r.Name, errs) + } + return nil + } + + if r.Spec.Reporter != nil { + return apierrors.NewInvalid(v1.Kind("SecurityReport"), r.Name, field.ErrorList{ + field.Forbidden(sp.Child("reporter"), "you are the reporter; this is for reports the Security Team logs")}) + } + if identity.EmailUnverified(ctx) { + return apierrors.NewForbidden(gr, "", errors.New("verify your email address to report a vulnerability")) + } + if err := h.rateLimit(ctx, owner); err != nil { + return err + } + if len(r.Spec.Attachments) == 0 { + return nil + } + ap := sp.Child("attachments") + if h.o.Files == nil { + return apierrors.NewInvalid(v1.Kind("SecurityReport"), r.Name, field.ErrorList{field.Forbidden(ap, "attachments are not available")}) + } + refs := make([]string, len(r.Spec.Attachments)) + for i, a := range r.Spec.Attachments { + refs[i] = a.UploadRef + } + attached, err := h.o.Files.Attach(ctx, v1.AttachmentPurposeSecurityReport, owner, r.Name, refs) + if errors.Is(err, attachments.ErrNotFound) { + return apierrors.NewInvalid(v1.Kind("SecurityReport"), r.Name, field.ErrorList{ + field.Invalid(ap, refs, "every uploadRef must be one of your SecurityReport uploads from the last day")}) + } + if err != nil { + klog.ErrorS(err, "Attaching files to a security report failed") + return apierrors.NewServiceUnavailable("Attachments are not available; try again or send without them.") + } + r.Spec.Attachments = attached + return nil +} + +// rateLimit allows Limit reports per Window per reporter, counted in +// Postgres so every replica agrees. +func (h hooks) rateLimit(ctx context.Context, owner string) error { + if h.o.Limit <= 0 { + return nil + } + var oldest time.Time + err := h.db.Q(ctx).QueryRow(ctx, `SELECT created_at FROM security_reports + WHERE scope_type = $1 AND scope_name = $2 AND created_at > now() - make_interval(secs => $3) + ORDER BY created_at DESC OFFSET $4 LIMIT 1`, + pg.ScopeUser, owner, h.o.Window.Seconds(), h.o.Limit-1).Scan(&oldest) + if errors.Is(err, pgx.ErrNoRows) { + return nil + } + if err != nil { + return apierrors.NewInternalError(err) + } + return apierrors.NewTooManyRequests( + fmt.Sprintf("You can send %d reports every %s; try again later, or email security@datum.net.", h.o.Limit, duration.HumanDuration(h.o.Window)), + max(1, int(math.Ceil(time.Until(oldest.Add(h.o.Window)).Seconds())))) +} + +func (h hooks) afterWrite(ctx context.Context, op pg.Op, old, _ runtime.Object) error { + if op == pg.OpDelete && h.o.Files != nil { + r := old.(*v1.SecurityReport) + if len(r.Spec.Attachments) > 0 { + if err := h.o.Files.DeleteParent(ctx, v1.AttachmentPurposeSecurityReport, r.Name); err != nil { + klog.ErrorS(err, "Deleting a deleted report's attachments failed", "report", r.Name) + } + } + } + return nil +} + +type table struct{} + +func (table) ConvertToTable(_ context.Context, obj runtime.Object, _ runtime.Object) (*metav1.Table, error) { + t := &metav1.Table{ColumnDefinitions: []metav1.TableColumnDefinition{ + {Name: "Name", Type: "string", Format: "name"}, + {Name: "Owner", Type: "string", Priority: 1}, + {Name: "Phase", Type: "string"}, + {Name: "Severity", Type: "string"}, + {Name: "Title", Type: "string"}, + {Name: "Age", Type: "date"}, + }} + add := func(r *v1.SecurityReport) { + age := "" + if !r.CreationTimestamp.IsZero() { + age = duration.HumanDuration(time.Since(r.CreationTimestamp.Time)) + } + owner := r.Annotations[pg.OwnerAnnotation] + if owner == "" && r.Spec.Reporter != nil { + owner = "external" + } + t.Rows = append(t.Rows, metav1.TableRow{ + Object: runtime.RawExtension{Object: r}, + Cells: []any{r.Name, owner, string(r.Status.Phase), string(r.Status.Severity), r.Spec.Title, age}, + }) + } + switch o := obj.(type) { + case *v1.SecurityReport: + add(o) + case *v1.SecurityReportList: + t.ResourceVersion, t.Continue = o.ResourceVersion, o.Continue + for i := range o.Items { + add(&o.Items[i]) + } + } + return t, nil +} diff --git a/internal/registry/securityreport/strategy.go b/internal/registry/securityreport/strategy.go new file mode 100644 index 0000000..47cdc9b --- /dev/null +++ b/internal/registry/securityreport/strategy.go @@ -0,0 +1,243 @@ +// Package securityreport implements storage for SecurityReport and the +// SecurityAcknowledgement view. +// +// Who does what: +// - Reporters create reports in the user context and read their own, +// without status.internal. They can't change or withdraw them. +// - The Security Team works every report in the platform context: it +// writes status, and logs reports for people without a Datum account +// (spec.reporter set), which belong to the platform. Milo IAM keeps +// everyone else, feedback reviewers included, out. +// - feedback-sync and feedback-controller never write reports. +// +// Reports are never embedded, indexed, synced or sent to the AI gateway: +// nothing here calls those paths. +package securityreport + +import ( + "context" + "crypto/rand" + "encoding/hex" + "net/url" + "regexp" + "slices" + "time" + + "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/storage/names" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + + "go.miloapis.com/feedback/internal/identity" + v1 "go.miloapis.com/feedback/pkg/apis/feedback/v1alpha1" +) + +var ( + phases = []v1.SecurityReportPhase{ + v1.SecurityReportPhaseReceived, v1.SecurityReportPhaseTriaged, v1.SecurityReportPhaseFixing, + v1.SecurityReportPhaseFixed, v1.SecurityReportPhaseDisclosed, + v1.SecurityReportPhaseNotApplicable, v1.SecurityReportPhaseDuplicate, + } + severities = []v1.Severity{v1.SeverityCritical, v1.SeverityHigh, v1.SeverityMedium, v1.SeverityLow, v1.SeverityInformational} + rewards = []v1.RewardKind{v1.RewardKindNone, v1.RewardKindThanks, v1.RewardKindSwag, v1.RewardKindCredits} + clients = []v1.Client{v1.ClientWebsite, v1.ClientPortal, v1.ClientDatumctl, v1.ClientPatch, v1.ClientEmail, v1.ClientGitHub} + + amountRE = regexp.MustCompile(`^[0-9]+(\.[0-9]{1,2})?$`) + cvssRE = regexp.MustCompile(`^CVSS:[34]\.[01]/[A-Za-z:/]+$`) + emailRE = regexp.MustCompile(`^[^@\s]+@[^@\s]+\.[^@\s]+$`) +) + +func newName() string { + b := make([]byte, 6) + _, _ = rand.Read(b) + return "sec-" + hex.EncodeToString(b) +} + +type strategy struct { + runtime.ObjectTyper + names.NameGenerator + ids identity.Identities + now func() time.Time +} + +func (strategy) NamespaceScoped() bool { return false } + +// PrepareForCreate names the report (names are unique across reporters, so +// they are always generated) and starts its timeline. +func (s strategy) PrepareForCreate(_ context.Context, obj runtime.Object) { + r := obj.(*v1.SecurityReport) + r.Name, r.GenerateName = newName(), "" + r.Status = v1.SecurityReportStatus{ + Phase: v1.SecurityReportPhaseReceived, + Timeline: []v1.SecurityTimelineEntry{{Phase: v1.SecurityReportPhaseReceived, At: metav1.NewTime(s.now())}}, + } +} + +func (strategy) PrepareForUpdate(_ context.Context, obj, old runtime.Object) { + obj.(*v1.SecurityReport).Status = old.(*v1.SecurityReport).Status +} + +func (strategy) Validate(_ context.Context, obj runtime.Object) field.ErrorList { + return validateSpec(&obj.(*v1.SecurityReport).Spec) +} + +func (strategy) ValidateUpdate(_ context.Context, obj, old runtime.Object) field.ErrorList { + if !equality.Semantic.DeepEqual(obj.(*v1.SecurityReport).Spec, old.(*v1.SecurityReport).Spec) { + return field.ErrorList{field.Forbidden(field.NewPath("spec"), "is immutable")} + } + return nil +} + +func (strategy) WarningsOnCreate(context.Context, runtime.Object) []string { return nil } +func (strategy) WarningsOnUpdate(context.Context, runtime.Object, runtime.Object) []string { + return nil +} +func (strategy) AllowCreateOnUpdate(context.Context) bool { return false } +func (strategy) AllowUnconditionalUpdate(context.Context) bool { return true } +func (strategy) Canonicalize(runtime.Object) {} + +func validateSpec(s *v1.SecurityReportSpec) field.ErrorList { + p := field.NewPath("spec") + var errs field.ErrorList + errs = append(errs, text(p.Child("title"), s.Title, 200, true)...) + errs = append(errs, text(p.Child("description"), s.Description, 20000, true)...) + errs = append(errs, text(p.Child("affected"), s.Affected, 200, false)...) + errs = append(errs, text(p.Child("stepsToReproduce"), s.StepsToReproduce, 20000, false)...) + errs = append(errs, text(p.Child("impact"), s.Impact, 5000, false)...) + if len(s.Attachments) > 5 { + errs = append(errs, field.TooMany(p.Child("attachments"), len(s.Attachments), 5)) + } + seen := map[string]bool{} + for i, a := range s.Attachments { + ap := p.Child("attachments").Index(i).Child("uploadRef") + switch { + case a.UploadRef == "": + errs = append(errs, field.Required(ap, "")) + case seen[a.UploadRef]: + errs = append(errs, field.Duplicate(ap, a.UploadRef)) + } + seen[a.UploadRef] = true + } + if c := s.Credit; c != nil { + cp := p.Child("credit") + errs = append(errs, text(cp.Child("name"), c.Name, 100, c.Public)...) + if c.URL != "" { + if u, err := url.Parse(c.URL); err != nil || (u.Scheme != "https" && u.Scheme != "http") || u.Host == "" || len(c.URL) > 2048 { + errs = append(errs, field.Invalid(cp.Child("url"), c.URL, "must be an http(s) URL")) + } + } + } + if r := s.Reporter; r != nil { + rp := p.Child("reporter") + if r.Email == "" && r.GitHubLogin == "" { + errs = append(errs, field.Required(rp, "email or githubLogin")) + } + if r.Email != "" && !emailRE.MatchString(r.Email) { + errs = append(errs, field.Invalid(rp.Child("email"), r.Email, "must be an email address")) + } + } + if c := s.Context.Client; c != "" && !slices.Contains(clients, c) { + errs = append(errs, field.NotSupported(p.Child("context", "client"), c, clients)) + } + return errs +} + +func text(p *field.Path, v string, max int, required bool) field.ErrorList { + switch { + case v == "" && required: + return field.ErrorList{field.Required(p, "")} + case len(v) > max: + return field.ErrorList{field.TooLong(p, "", max)} + } + return nil +} + +// statusStrategy is the Security Team's status update. A phase change +// without a timeline entry for it gets one, so the reporter always sees it. +type statusStrategy struct { + strategy +} + +func (s statusStrategy) PrepareForUpdate(_ context.Context, obj, old runtime.Object) { + n, o := obj.(*v1.SecurityReport), old.(*v1.SecurityReport) + n.Spec = o.Spec + n.Labels, n.Annotations = o.Labels, o.Annotations + st := &n.Status + if st.Phase != o.Status.Phase { + if l := len(st.Timeline); l == 0 || st.Timeline[l-1].Phase != st.Phase { + st.Timeline = append(st.Timeline, v1.SecurityTimelineEntry{Phase: st.Phase, At: metav1.NewTime(s.now())}) + } + } +} + +func (s statusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { + sp := field.NewPath("status") + if s.ids.Of(ctx) != identity.Other { + return field.ErrorList{field.Forbidden(sp, "security report status is written by the Security Team")} + } + n, o := obj.(*v1.SecurityReport), old.(*v1.SecurityReport) + st := n.Status + var errs field.ErrorList + if !slices.Contains(phases, st.Phase) { + errs = append(errs, field.NotSupported(sp.Child("phase"), st.Phase, phases)) + } + if st.Severity != "" && !slices.Contains(severities, st.Severity) { + errs = append(errs, field.NotSupported(sp.Child("severity"), st.Severity, severities)) + } + if st.CVSSVector != "" && !cvssRE.MatchString(st.CVSSVector) { + errs = append(errs, field.Invalid(sp.Child("cvssVector"), st.CVSSVector, "must be a CVSS 3.x or 4.0 vector")) + } + switch st.Phase { + case v1.SecurityReportPhaseNotApplicable, v1.SecurityReportPhaseDuplicate: + errs = append(errs, text(sp.Child("reason"), st.Reason, 2000, true)...) + } + if st.Phase == v1.SecurityReportPhaseDuplicate { + dp := sp.Child("internal", "duplicateOf") + switch { + case st.Internal == nil || st.Internal.DuplicateOf == "": + errs = append(errs, field.Required(dp, "the original report")) + case st.Internal.DuplicateOf == n.Name: + errs = append(errs, field.Invalid(dp, n.Name, "a report can't duplicate itself")) + } + } + // The timeline is the reporter's record: entries can be added, not + // changed or removed. + tp := sp.Child("timeline") + if len(st.Timeline) < len(o.Status.Timeline) || !equality.Semantic.DeepEqual(st.Timeline[:len(o.Status.Timeline)], o.Status.Timeline) { + errs = append(errs, field.Forbidden(tp, "entries can be added but not changed or removed")) + } + for i, e := range st.Timeline { + if !slices.Contains(phases, e.Phase) { + errs = append(errs, field.NotSupported(tp.Index(i).Child("phase"), e.Phase, phases)) + } + errs = append(errs, text(tp.Index(i).Child("note"), e.Note, 2000, false)...) + } + if r := st.Reward; r != nil { + rp := sp.Child("reward") + if !slices.Contains(rewards, r.Kind) { + errs = append(errs, field.NotSupported(rp.Child("kind"), r.Kind, rewards)) + } + if r.Kind == v1.RewardKindCredits && !amountRE.MatchString(r.Amount) { + errs = append(errs, field.Invalid(rp.Child("amount"), r.Amount, "credits need an amount like 250.00")) + } + } + if a := st.Advisory; a != nil && a.URL != "" { + if u, err := url.Parse(a.URL); err != nil || u.Scheme != "https" { + errs = append(errs, field.Invalid(sp.Child("advisory", "url"), a.URL, "must be an https URL")) + } + } + return errs +} + +func (statusStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set { + return map[fieldpath.APIVersion]*fieldpath.Set{ + fieldpath.APIVersion(v1.SchemeGroupVersion.String()): fieldpath.NewSet(fieldpath.MakePathOrDie("spec")), + } +} + +// redact is what a reporter sees: everything but the internal notes. +func redact(r *v1.SecurityReport) { + r.Status.Internal = nil +} diff --git a/internal/registry/submission/review.go b/internal/registry/submission/review.go index 4bd5282..6a2e4f1 100644 --- a/internal/registry/submission/review.go +++ b/internal/registry/submission/review.go @@ -4,10 +4,6 @@ import ( "context" "errors" "fmt" - "mime" - "net/http" - "slices" - "strconv" "github.com/jackc/pgx/v5" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -16,9 +12,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" - "k8s.io/klog/v2" - "go.miloapis.com/feedback/internal/attachments" "go.miloapis.com/feedback/internal/identity" "go.miloapis.com/feedback/internal/registry/vote" "go.miloapis.com/feedback/internal/storage/pg" @@ -131,62 +125,3 @@ func validateReview(s *v1.SubmissionReviewSpec) field.ErrorList { } return errs } - -// AttachmentREST serves GET submissions/{name}/attachment?ref=: the file -// itself, to the submitter and staff. -type AttachmentREST struct { - store *pg.Store - files *attachments.Store -} - -var _ rest.Connecter = &AttachmentREST{} - -func (r *AttachmentREST) New() runtime.Object { return &v1.Submission{} } -func (r *AttachmentREST) Destroy() {} - -func (r *AttachmentREST) NewConnectOptions() (runtime.Object, bool, string) { - return &v1.AttachmentOptions{}, false, "" -} - -func (r *AttachmentREST) ConnectMethods() []string { return []string{http.MethodGet} } - -// Connect checks the caller can read the submission (the store scopes it to -// the submitter in the user context) and that ref is attached to it. -func (r *AttachmentREST) Connect(ctx context.Context, name string, opts runtime.Object, _ rest.Responder) (http.Handler, error) { - ref := opts.(*v1.AttachmentOptions).Ref - if ref == "" { - return nil, apierrors.NewBadRequest("set the ref query parameter to an attachment's uploadRef") - } - obj, err := r.store.Get(ctx, name, &metav1.GetOptions{}) - if err != nil { - return nil, err - } - sub := obj.(*v1.Submission) - if !slices.ContainsFunc(sub.Spec.Attachments, func(a v1.AttachmentReference) bool { return a.UploadRef == ref }) { - return nil, apierrors.NewNotFound(v1.Resource("submissions/attachment"), ref) - } - if r.files == nil { - return nil, apierrors.NewServiceUnavailable("Attachments are not available.") - } - f, err := r.files.Get(ctx, v1.AttachmentPurposeSubmission, name, ref) - if errors.Is(err, attachments.ErrNotFound) { - return nil, apierrors.NewNotFound(v1.Resource("submissions/attachment"), ref) - } - if err != nil { - klog.ErrorS(err, "Reading attachment failed", "submission", name, "ref", ref) - return nil, apierrors.NewServiceUnavailable("Attachments are not available.") - } - return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { serveFile(w, f) }), nil -} - -// serveFile sends a file as a download that browsers won't render or run. -func serveFile(w http.ResponseWriter, f *attachments.File) { - h := w.Header() - h.Set("Content-Type", f.ContentType) - h.Set("Content-Length", strconv.Itoa(len(f.Data))) - h.Set("Content-Disposition", mime.FormatMediaType("attachment", map[string]string{"filename": f.Name})) - h.Set("X-Content-Type-Options", "nosniff") - h.Set("Content-Security-Policy", "sandbox; default-src 'none'") - h.Set("Cache-Control", "private, no-store") - _, _ = w.Write(f.Data) -} diff --git a/internal/registry/submission/storage.go b/internal/registry/submission/storage.go index 1610543..7527e5a 100644 --- a/internal/registry/submission/storage.go +++ b/internal/registry/submission/storage.go @@ -19,6 +19,8 @@ import ( "go.miloapis.com/feedback/internal/attachments" "go.miloapis.com/feedback/internal/identity" + "go.miloapis.com/feedback/internal/registry/attachmentupload" + "go.miloapis.com/feedback/internal/registry/securityreport" "go.miloapis.com/feedback/internal/registry/vote" "go.miloapis.com/feedback/internal/storage/pg" v1 "go.miloapis.com/feedback/pkg/apis/feedback/v1alpha1" @@ -33,6 +35,8 @@ type Options struct { // their submission is approved or merged. Votes *vote.REST Subscriptions *vote.SubscriptionREST + // Reports receives submissions moved to the security track. + Reports *securityreport.REST // Limit submissions per user to Limit per Window. Zero disables it. Limit int Window time.Duration @@ -43,7 +47,8 @@ type REST struct { *pg.Store Status *pg.StatusStore Review *ReviewREST - Attachment *AttachmentREST + ToSecurity *ToSecurityReportREST + Attachment *attachmentupload.DownloadREST } var gr = v1.Resource("submissions") @@ -84,7 +89,20 @@ func NewREST(db *pg.DB, typer runtime.ObjectTyper, o Options) (*REST, error) { Store: store, Status: pg.NewStatusStore(store, statusStrategy{s}), Review: &ReviewREST{store: pg.NewStatusStore(store, reviewStrategy{s}), db: db, ids: o.Identities}, - Attachment: &AttachmentREST{store: store, files: o.Files}, + ToSecurity: &ToSecurityReportREST{store: store, reports: o.Reports, files: o.Files, db: db, ids: o.Identities}, + Attachment: &attachmentupload.DownloadREST{ + Get: func(ctx context.Context, name string) ([]v1.AttachmentReference, error) { + obj, err := store.Get(ctx, name, &metav1.GetOptions{}) + if err != nil { + return nil, err + } + return obj.(*v1.Submission).Spec.Attachments, nil + }, + NewFunc: func() runtime.Object { return &v1.Submission{} }, + Files: o.Files, + Purpose: v1.AttachmentPurposeSubmission, + Resource: v1.Resource("submissions/attachment"), + }, }, nil } diff --git a/internal/registry/submission/submission_test.go b/internal/registry/submission/submission_test.go index 82f2de8..59325ce 100644 --- a/internal/registry/submission/submission_test.go +++ b/internal/registry/submission/submission_test.go @@ -21,6 +21,7 @@ import ( "go.miloapis.com/feedback/internal/ratelimit" "go.miloapis.com/feedback/internal/registry/attachmentupload" "go.miloapis.com/feedback/internal/registry/post" + "go.miloapis.com/feedback/internal/registry/securityreport" "go.miloapis.com/feedback/internal/registry/vote" "go.miloapis.com/feedback/internal/storage/pg" "go.miloapis.com/feedback/internal/storage/pg/pgtest" @@ -51,7 +52,7 @@ type env struct { func setup(t *testing.T) env { t.Helper() - db := pgtest.Require(t, "posts", "votes", "subscriptions", "submissions") + db := pgtest.Require(t, "posts", "votes", "subscriptions", "submissions", "security_reports") scheme := runtime.NewScheme() install.Install(scheme) posts, err := post.NewREST(db, scheme, ids) @@ -449,3 +450,72 @@ func TestUploadRateLimit(t *testing.T) { t.Errorf("second upload: %v", err) } } + +func TestToSecurityReport(t *testing.T) { + e := setup(t) + reports, err := securityreport.NewREST(e.db, func() *runtime.Scheme { s := runtime.NewScheme(); install.Install(s); return s }(), + securityreport.Options{Identities: ids, Files: e.files}) + if err != nil { + t.Fatal(err) + } + e.s.ToSecurity.reports = reports + if _, err := e.db.Pool.Exec(context.Background(), "TRUNCATE security_reports"); err != nil { + t.Fatal(err) + } + + ref, err := e.upload(alice, "trace.txt", "text/plain", []byte("token=abc")) + if err != nil { + t.Fatal(err) + } + in := spec() + in.Attachments = []v1.AttachmentReference{{UploadRef: ref}} + s := e.submit(t, alice, in) + + move := func(ctx context.Context, name string) (*v1.SecurityReport, error) { + obj, err := e.s.ToSecurity.Create(ctx, name, &v1.SubmissionToSecurityReport{Affected: "connectors"}, nil, &metav1.CreateOptions{}) + if err != nil { + return nil, err + } + return obj.(*v1.SecurityReport), nil + } + if _, err := move(alice, s.Name); !apierrors.IsForbidden(err) { + t.Errorf("submitter moves: %v", err) + } + if _, err := move(controller, s.Name); !apierrors.IsForbidden(err) { + t.Errorf("controller moves: %v", err) + } + r, err := move(staff, s.Name) + if err != nil { + t.Fatal(err) + } + if r.Spec.Title != s.Spec.Title || r.Spec.Description != s.Spec.Body || r.Spec.Affected != "connectors" || + r.Spec.Context.Client != v1.ClientPortal || r.Annotations[pg.OwnerAnnotation] != "alice" || len(r.Spec.Attachments) != 1 { + t.Errorf("report: %+v", r) + } + if _, err := e.s.Get(staff, s.Name, &metav1.GetOptions{}); !apierrors.IsNotFound(err) { + t.Errorf("submission still there: %v", err) + } + if _, err := reports.Get(alice, r.Name, &metav1.GetOptions{}); err != nil { + t.Errorf("submitter can't read the report: %v", err) + } + h, err := reports.Attachment.Connect(alice, r.Name, &v1.AttachmentOptions{Ref: ref}, nil) + if err != nil { + t.Fatalf("attachment didn't move: %v", err) + } + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest("GET", "/", nil)) + if w.Body.String() != "token=abc" { + t.Errorf("moved file: %q", w.Body.String()) + } + if _, err := e.files.Get(context.Background(), v1.AttachmentPurposeSubmission, s.Name, ref); err != attachments.ErrNotFound { + t.Errorf("original file kept: %v", err) + } + + reviewed := e.submit(t, alice, spec()) + if _, err := e.review(staff, reviewed.Name, v1.SubmissionReviewSpec{Decision: v1.ReviewDecisionRejected, Reason: "no"}); err != nil { + t.Fatal(err) + } + if _, err := move(staff, reviewed.Name); !apierrors.IsConflict(err) { + t.Errorf("reviewed submission moves: %v", err) + } +} diff --git a/internal/registry/submission/tosecurity.go b/internal/registry/submission/tosecurity.go new file mode 100644 index 0000000..28d9cc5 --- /dev/null +++ b/internal/registry/submission/tosecurity.go @@ -0,0 +1,107 @@ +package submission + +import ( + "context" + "errors" + "fmt" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/registry/rest" + + "go.miloapis.com/feedback/internal/attachments" + "go.miloapis.com/feedback/internal/identity" + "go.miloapis.com/feedback/internal/registry/securityreport" + "go.miloapis.com/feedback/internal/storage/pg" + v1 "go.miloapis.com/feedback/pkg/apis/feedback/v1alpha1" +) + +// ToSecurityReportREST serves submissions/{name}/to-security-report: staff +// move a misfiled submission to the security track. In one transaction a +// SecurityReport owned by the submitter is created from it, its attachments +// are copied, and the submission is deleted (which deletes the originals). +type ToSecurityReportREST struct { + store *pg.Store + reports *securityreport.REST + files *attachments.Store + db *pg.DB + ids identity.Identities +} + +var _ rest.NamedCreater = &ToSecurityReportREST{} + +func (r *ToSecurityReportREST) New() runtime.Object { return &v1.SubmissionToSecurityReport{} } +func (r *ToSecurityReportREST) Destroy() {} + +// Create returns the new SecurityReport. +func (r *ToSecurityReportREST) Create(ctx context.Context, name string, obj runtime.Object, createValidation rest.ValidateObjectFunc, opts *metav1.CreateOptions) (runtime.Object, error) { + in, ok := obj.(*v1.SubmissionToSecurityReport) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("not a SubmissionToSecurityReport: %T", obj)) + } + if kind, _ := pg.RequestContext(ctx); kind != pg.ScopePlatform || r.ids.Of(ctx) != identity.Other { + return nil, apierrors.NewForbidden(gr, name, errors.New("submissions are moved to the security track by staff in the platform context")) + } + if opts != nil && len(opts.DryRun) > 0 { + return nil, apierrors.NewBadRequest("dry run isn't supported: moving copies attachments") + } + if createValidation != nil { + if err := createValidation(ctx, obj); err != nil { + return nil, err + } + } + if len(in.Affected) > 200 { + return nil, apierrors.NewInvalid(v1.Kind("SubmissionToSecurityReport"), name, field.ErrorList{ + field.TooLong(field.NewPath("affected"), "", 200)}) + } + + var out runtime.Object + err := r.db.InTx(ctx, func(ctx context.Context) error { + obj, err := r.store.Get(ctx, name, &metav1.GetOptions{}) + if err != nil { + return err + } + sub := obj.(*v1.Submission) + if sub.Status.Phase != v1.SubmissionPhasePendingReview { + return apierrors.NewConflict(gr, name, fmt.Errorf("already %s; only submissions waiting for review can move", sub.Status.Phase)) + } + owner := sub.Annotations[pg.OwnerAnnotation] + if owner == "" { + return apierrors.NewInternalError(fmt.Errorf("submission %s has no owner", name)) + } + + report := &v1.SecurityReport{Spec: v1.SecurityReportSpec{ + Title: sub.Spec.Title, + Description: sub.Spec.Body, + Affected: in.Affected, + Attachments: sub.Spec.Attachments, + Context: v1.SecurityReportContext{ + Client: sub.Spec.Context.Client, Organization: sub.Spec.Context.Organization, + ClientVersion: sub.Spec.Context.ClientVersion, + }, + }} + // The report belongs to the submitter, so they can follow it. + created, err := r.reports.Create(securityreport.WithMovedFiles(asUser(ctx, owner)), report, nil, &metav1.CreateOptions{}) + if err != nil { + return err + } + rep := created.(*v1.SecurityReport) + if len(sub.Spec.Attachments) > 0 { + if r.files == nil { + return apierrors.NewServiceUnavailable("Attachments are not available, so this submission can't move yet.") + } + if err := r.files.CopyParent(ctx, v1.AttachmentPurposeSubmission, v1.AttachmentPurposeSecurityReport, name, rep.Name); err != nil { + return apierrors.NewInternalError(fmt.Errorf("copy attachments: %w", err)) + } + } + if _, _, err := r.store.Delete(ctx, name, nil, &metav1.DeleteOptions{}); err != nil { + return err + } + // Return the staff view. + out, err = r.reports.Get(ctx, rep.Name, &metav1.GetOptions{}) + return err + }) + return out, err +} diff --git a/internal/storage/pg/kind.go b/internal/storage/pg/kind.go index 3c0d8dc..1c15312 100644 --- a/internal/storage/pg/kind.go +++ b/internal/storage/pg/kind.go @@ -32,6 +32,10 @@ type Kind struct { // Scope resolves which owner a request reads and writes. Scope ScopeFunc + // CreateScope, if set, resolves the owner of a new object instead of + // Scope. Security reports use it: staff read every report, but the ones + // they log for external reporters belong to the platform. + CreateScope ScopeFunc // Columns returns the extra column values for obj. Every call must return // the same column names in the same order. diff --git a/internal/storage/pg/store.go b/internal/storage/pg/store.go index f14db50..405e70c 100644 --- a/internal/storage/pg/store.go +++ b/internal/storage/pg/store.go @@ -80,7 +80,15 @@ func (s *Store) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set { } func (s *Store) scope(ctx context.Context) (Scope, error) { - sc, err := s.kind.Scope(ctx) + return s.resolve(ctx, s.kind.Scope) +} + +func (s *Store) createScope(ctx context.Context) (Scope, error) { + return s.resolve(ctx, s.kind.CreateScope) +} + +func (s *Store) resolve(ctx context.Context, fn ScopeFunc) (Scope, error) { + sc, err := fn(ctx) if err == nil { return sc, nil } @@ -310,7 +318,11 @@ func (s *Store) List(ctx context.Context, opts *metainternalversion.ListOptions) // Create implements rest.Creater. func (s *Store) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, opts *metav1.CreateOptions) (runtime.Object, error) { - sc, err := s.scope(ctx) + scopeFn := s.scope + if s.kind.CreateScope != nil { + scopeFn = s.createScope + } + sc, err := scopeFn(ctx) if err != nil { return nil, err }