From f84c598095fbcc55d7b6fc22ef8fc36345ea8406 Mon Sep 17 00:00:00 2001 From: Matt Jenkinson Date: Thu, 12 Nov 2026 14:18:33 +0000 Subject: [PATCH] feat: audit feedback writes from the apiserver Milo proxies requests to the feedback API without their bodies, so the apiserver now runs its own audit backend and is the record for writes to its API. - An audit-logging component adds the policy, a webhook config for activity's Vector in activity-system, and the apiserver flags and mounts. The core-control-plane overlay includes it. - Writes to posts, releases, changelog entries, votes, subscriptions and feedback sources are logged at RequestResponse, which the search indexer needs. Submissions, security reports, uploads and PostQuery are logged at Metadata. Reads are left to Milo. - Side effects are recorded as annotations on the request that caused them: subscribed, votes-moved-to, vote-granted and security-report. - A test evaluates the shipped policy with the apiserver's own evaluator. Part of datum-cloud/enhancements#943. Closes #11. --- README.md | 22 ++++++ .../audit-logging/audit-policy.yaml | 54 ++++++++++++++ .../audit-logging/audit-webhook.yaml | 26 +++++++ .../audit-logging/kustomization.yaml | 48 +++++++++++++ .../core-control-plane/kustomization.yaml | 3 + internal/apiserver/auditpolicy_test.go | 70 +++++++++++++++++++ internal/auditx/auditx.go | 28 ++++++++ internal/registry/post/storage.go | 2 + internal/registry/submission/storage.go | 6 +- internal/registry/submission/tosecurity.go | 2 + internal/registry/vote/subscription.go | 4 ++ 11 files changed, 264 insertions(+), 1 deletion(-) create mode 100644 config/components/audit-logging/audit-policy.yaml create mode 100644 config/components/audit-logging/audit-webhook.yaml create mode 100644 config/components/audit-logging/kustomization.yaml create mode 100644 internal/apiserver/auditpolicy_test.go create mode 100644 internal/auditx/auditx.go 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 }