diff --git a/README.md b/README.md index 2edfa31..5e465f2 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,28 @@ only in their owner's context, submissions and security reports are created only by their author (or logged by the Security Team), and `FeedbackSource` is platform-only. +## Audit + +Milo proxies requests to this API without their bodies, so the apiserver is +the audit source for its own writes (`config/components/audit-logging`, +included in the core-control-plane overlay): + +- Writes to posts, releases, changelog entries, votes, subscriptions and + feedback sources are logged at `RequestResponse`. The search indexer reads + posts, releases and changelog entries from `responseObject`. +- Submissions, security reports, uploads and `PostQuery` are logged at + `Metadata`, so private context, vulnerability details, files and draft + feedback never reach the audit log, activity or search. +- Reads aren't logged here; Milo's events cover them. +- Side effects are annotations on the request that caused them: + `feedback.miloapis.com/subscribed`, `votes-moved-to`, `vote-granted` and + `security-report`. + +Events go to activity's Vector webhook in `activity-system`, like Milo's. +The deployment supplies the webhook's CA as the `feedback-audit-webhook-ca` +ConfigMap, and drops Milo's own events for `feedback.miloapis.com` writes so +each write is recorded once. + ## Postgres The apiserver is the only component that connects to Postgres. Other diff --git a/config/components/audit-logging/audit-policy.yaml b/config/components/audit-logging/audit-policy.yaml new file mode 100644 index 0000000..c3661e4 --- /dev/null +++ b/config/components/audit-logging/audit-policy.yaml @@ -0,0 +1,54 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: feedback-audit-policy + namespace: feedback-system +data: + audit-policy.yaml: | + # The feedback apiserver is the audit source for writes to its API. Milo + # proxies requests here without their bodies, so it records them at + # Metadata only; the deployment drops Milo's events for + # feedback.miloapis.com writes so each write is recorded once, here. + # Reads stay with Milo's events. + apiVersion: audit.k8s.io/v1 + kind: Policy + omitStages: + - RequestReceived + - ResponseStarted + - Panic + rules: + - level: None + nonResourceURLs: + - /healthz* + - /livez* + - /readyz* + - /metrics + - /version + - /api + - /apis + - /apis/* + - /openapi + - /openapi/* + - level: None + verbs: [get, list, watch] + # Private content stays out of the audit log, activity and search: + # submissions (private context, triage), security reports + # (vulnerability details), uploads (file contents) and PostQuery + # (draft feedback in similarTo). Side effects show as annotations. + - level: Metadata + resources: + - group: feedback.miloapis.com + resources: + - submissions + - submissions/* + - securityreports + - securityreports/* + - attachmentuploads + - postqueries + # Everything else is written with its bodies. The search indexer reads + # posts, releases and changelog entries from responseObject. + - level: RequestResponse + verbs: [create, update, patch, delete, deletecollection] + resources: + - group: feedback.miloapis.com + - level: None diff --git a/config/components/audit-logging/audit-webhook.yaml b/config/components/audit-logging/audit-webhook.yaml new file mode 100644 index 0000000..f95dc34 --- /dev/null +++ b/config/components/audit-logging/audit-webhook.yaml @@ -0,0 +1,26 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: feedback-audit-webhook + namespace: feedback-system +data: + audit-webhook.yaml: | + # The Vector webhook in activity-system that Milo sends to, so feedback + # events reach the audit log, activity and search the same way. + apiVersion: v1 + kind: Config + clusters: + - name: activity-audit + cluster: + server: https://vector-sidecar.activity-system.svc.cluster.local:8080/events + certificate-authority: /etc/kubernetes/pki/audit-webhook/ca.crt + users: + - name: feedback-apiserver + user: + token: noop + contexts: + - name: activity-audit + context: + cluster: activity-audit + user: feedback-apiserver + current-context: activity-audit diff --git a/config/components/audit-logging/kustomization.yaml b/config/components/audit-logging/kustomization.yaml new file mode 100644 index 0000000..83075a4 --- /dev/null +++ b/config/components/audit-logging/kustomization.yaml @@ -0,0 +1,48 @@ +# Audit logging for the feedback apiserver: a policy and a webhook to +# activity's Vector. The deployer supplies the webhook's CA as the ConfigMap +# feedback-audit-webhook-ca (key ca.crt), e.g. from the cluster's trust +# bundle, or replaces feedback-audit-webhook. +apiVersion: kustomize.config.k8s.io/v1alpha1 +kind: Component + +resources: + - audit-policy.yaml + - audit-webhook.yaml + +patches: + - target: + group: apps + version: v1 + kind: Deployment + name: feedback-apiserver + patch: |- + - op: add + path: /spec/template/spec/containers/0/args/- + value: --audit-policy-file=/etc/feedback/audit/policy/audit-policy.yaml + - op: add + path: /spec/template/spec/containers/0/args/- + value: --audit-webhook-config-file=/etc/feedback/audit/webhook/audit-webhook.yaml + - op: add + path: /spec/template/spec/containers/0/args/- + value: --audit-webhook-mode=batch + - op: add + path: /spec/template/spec/containers/0/args/- + value: --audit-webhook-initial-backoff=10s + - op: add + path: /spec/template/spec/containers/0/volumeMounts/- + value: {name: audit-policy, mountPath: /etc/feedback/audit/policy, readOnly: true} + - op: add + path: /spec/template/spec/containers/0/volumeMounts/- + value: {name: audit-webhook, mountPath: /etc/feedback/audit/webhook, readOnly: true} + - op: add + path: /spec/template/spec/containers/0/volumeMounts/- + value: {name: audit-webhook-ca, mountPath: /etc/kubernetes/pki/audit-webhook, readOnly: true} + - op: add + path: /spec/template/spec/volumes/- + value: {name: audit-policy, configMap: {name: feedback-audit-policy}} + - op: add + path: /spec/template/spec/volumes/- + value: {name: audit-webhook, configMap: {name: feedback-audit-webhook}} + - op: add + path: /spec/template/spec/volumes/- + value: {name: audit-webhook-ca, configMap: {name: feedback-audit-webhook-ca, optional: true}} diff --git a/config/overlays/core-control-plane/kustomization.yaml b/config/overlays/core-control-plane/kustomization.yaml index dc79c35..d8710f4 100644 --- a/config/overlays/core-control-plane/kustomization.yaml +++ b/config/overlays/core-control-plane/kustomization.yaml @@ -4,6 +4,9 @@ kind: Kustomization resources: - ../../base +components: + - ../../components/audit-logging + images: - name: ghcr.io/milo-os/feedback newTag: latest diff --git a/internal/apiserver/auditpolicy_test.go b/internal/apiserver/auditpolicy_test.go new file mode 100644 index 0000000..2e01fe3 --- /dev/null +++ b/internal/apiserver/auditpolicy_test.go @@ -0,0 +1,70 @@ +package apiserver + +import ( + "os" + "testing" + + auditinternal "k8s.io/apiserver/pkg/apis/audit" + "k8s.io/apiserver/pkg/audit/policy" + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/authorization/authorizer" + "sigs.k8s.io/yaml" +) + +// TestAuditPolicy evaluates the shipped policy: writes are recorded with +// their bodies, except private kinds, which are recorded at Metadata; reads +// are left to Milo. +func TestAuditPolicy(t *testing.T) { + raw, err := os.ReadFile("../../config/components/audit-logging/audit-policy.yaml") + if err != nil { + t.Fatal(err) + } + var cm struct { + Data map[string]string `json:"data"` + } + if err := yaml.Unmarshal(raw, &cm); err != nil { + t.Fatal(err) + } + p, err := policy.LoadPolicyFromBytes([]byte(cm.Data["audit-policy.yaml"])) + if err != nil { + t.Fatal(err) + } + eval := policy.NewPolicyRuleEvaluator(p) + + u := &user.DefaultInfo{Name: "someone"} + res := func(verb, resource, sub string) authorizer.Attributes { + return authorizer.AttributesRecord{User: u, Verb: verb, APIGroup: group, APIVersion: "v1alpha1", + Resource: resource, Subresource: sub, Name: "x", ResourceRequest: true} + } + for _, c := range []struct { + attrs authorizer.Attributes + want auditinternal.Level + }{ + {res("create", "posts", ""), auditinternal.LevelRequestResponse}, + {res("patch", "posts", "status"), auditinternal.LevelRequestResponse}, + {res("delete", "posts", ""), auditinternal.LevelRequestResponse}, + {res("update", "releases", "status"), auditinternal.LevelRequestResponse}, + {res("create", "changelogentries", ""), auditinternal.LevelRequestResponse}, + {res("create", "votes", ""), auditinternal.LevelRequestResponse}, + {res("delete", "subscriptions", ""), auditinternal.LevelRequestResponse}, + {res("update", "feedbacksources", ""), auditinternal.LevelRequestResponse}, + {res("create", "submissions", ""), auditinternal.LevelMetadata}, + {res("update", "submissions", "status"), auditinternal.LevelMetadata}, + {res("create", "submissions", "review"), auditinternal.LevelMetadata}, + {res("create", "submissions", "to-security-report"), auditinternal.LevelMetadata}, + {res("create", "securityreports", ""), auditinternal.LevelMetadata}, + {res("patch", "securityreports", "status"), auditinternal.LevelMetadata}, + {res("create", "attachmentuploads", ""), auditinternal.LevelMetadata}, + {res("create", "postqueries", ""), auditinternal.LevelMetadata}, + {res("get", "posts", ""), auditinternal.LevelNone}, + {res("list", "submissions", ""), auditinternal.LevelNone}, + {res("get", "securityreports", ""), auditinternal.LevelNone}, + {res("get", "submissions", "attachment"), auditinternal.LevelNone}, + {authorizer.AttributesRecord{User: u, Verb: "get", Path: "/readyz"}, auditinternal.LevelNone}, + } { + got := eval.EvaluatePolicyRule(c.attrs).Level + if got != c.want { + t.Errorf("%s %s/%s: level %s, want %s", c.attrs.GetVerb(), c.attrs.GetResource(), c.attrs.GetSubresource(), got, c.want) + } + } +} diff --git a/internal/auditx/auditx.go b/internal/auditx/auditx.go new file mode 100644 index 0000000..5692677 --- /dev/null +++ b/internal/auditx/auditx.go @@ -0,0 +1,28 @@ +// Package auditx records a request's side effects as audit annotations, so +// the request's own audit event shows everything it changed without +// carrying private content. +package auditx + +import ( + "context" + + "k8s.io/apiserver/pkg/audit" +) + +// Annotation keys. +const ( + // Subscribed is the post a vote subscribed its voter to. + Subscribed = "feedback.miloapis.com/subscribed" + // VotesMovedTo is the post a merged post's votes moved to. + VotesMovedTo = "feedback.miloapis.com/votes-moved-to" + // VoteGranted is "/" when a review or a linked post gave the + // submitter a vote. + VoteGranted = "feedback.miloapis.com/vote-granted" + // SecurityReport is the report a submission became. + SecurityReport = "feedback.miloapis.com/security-report" +) + +// Annotate adds key=value to the request's audit event, if it has one. +func Annotate(ctx context.Context, key, value string) { + audit.AddAuditAnnotation(ctx, key, value) +} diff --git a/internal/registry/post/storage.go b/internal/registry/post/storage.go index 7facb27..049121d 100644 --- a/internal/registry/post/storage.go +++ b/internal/registry/post/storage.go @@ -15,6 +15,7 @@ import ( "k8s.io/apiserver/pkg/registry/rest" "k8s.io/klog/v2" + "go.miloapis.com/feedback/internal/auditx" "go.miloapis.com/feedback/internal/identity" "go.miloapis.com/feedback/internal/storage/pg" v1 "go.miloapis.com/feedback/pkg/apis/feedback/v1alpha1" @@ -176,6 +177,7 @@ func (h hooks) beforeWrite(ctx context.Context, op pg.Op, old, obj runtime.Objec if err := moveVotes(ctx, q, p.Name, to); err != nil { return apierrors.NewInternalError(fmt.Errorf("move votes to %s: %w", to, err)) } + auditx.Annotate(ctx, auditx.VotesMovedTo, to) if _, err := Retally(ctx, h.db, to, now); err != nil { return apierrors.NewInternalError(fmt.Errorf("retally %s: %w", to, err)) } diff --git a/internal/registry/submission/storage.go b/internal/registry/submission/storage.go index 7527e5a..2b44ca3 100644 --- a/internal/registry/submission/storage.go +++ b/internal/registry/submission/storage.go @@ -18,6 +18,7 @@ import ( "k8s.io/klog/v2" "go.miloapis.com/feedback/internal/attachments" + "go.miloapis.com/feedback/internal/auditx" "go.miloapis.com/feedback/internal/identity" "go.miloapis.com/feedback/internal/registry/attachmentupload" "go.miloapis.com/feedback/internal/registry/securityreport" @@ -216,7 +217,10 @@ func (h hooks) grant(ctx context.Context, owner string, sub *v1.Submission, post } _, err := h.o.Votes.Create(as, v, nil, &metav1.CreateOptions{}) switch { - case err == nil, apierrors.IsAlreadyExists(err): + case err == nil: + auditx.Annotate(ctx, auditx.VoteGranted, owner+"/"+post) + return nil + case apierrors.IsAlreadyExists(err): return nil case apierrors.IsInvalid(err): klog.InfoS("Not giving the submitter a vote", "submission", sub.Name, "post", post, "err", err) diff --git a/internal/registry/submission/tosecurity.go b/internal/registry/submission/tosecurity.go index 28d9cc5..2ee337b 100644 --- a/internal/registry/submission/tosecurity.go +++ b/internal/registry/submission/tosecurity.go @@ -12,6 +12,7 @@ import ( "k8s.io/apiserver/pkg/registry/rest" "go.miloapis.com/feedback/internal/attachments" + "go.miloapis.com/feedback/internal/auditx" "go.miloapis.com/feedback/internal/identity" "go.miloapis.com/feedback/internal/registry/securityreport" "go.miloapis.com/feedback/internal/storage/pg" @@ -88,6 +89,7 @@ func (r *ToSecurityReportREST) Create(ctx context.Context, name string, obj runt return err } rep := created.(*v1.SecurityReport) + auditx.Annotate(ctx, auditx.SecurityReport, rep.Name) if len(sub.Spec.Attachments) > 0 { if r.files == nil { return apierrors.NewServiceUnavailable("Attachments are not available, so this submission can't move yet.") diff --git a/internal/registry/vote/subscription.go b/internal/registry/vote/subscription.go index e15020a..0e73e9d 100644 --- a/internal/registry/vote/subscription.go +++ b/internal/registry/vote/subscription.go @@ -11,6 +11,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/storage/names" + "go.miloapis.com/feedback/internal/auditx" "go.miloapis.com/feedback/internal/storage/pg" v1 "go.miloapis.com/feedback/pkg/apis/feedback/v1alpha1" ) @@ -118,6 +119,9 @@ func (r *SubscriptionREST) Follow(ctx context.Context, postName string, reason v if apierrors.IsAlreadyExists(err) { return nil } + if err == nil { + auditx.Annotate(ctx, auditx.Subscribed, postName) + } return err }