diff --git a/README.md b/README.md index 5edbb7a..2edfa31 100644 --- a/README.md +++ b/README.md @@ -46,6 +46,42 @@ config/milo/ Milo IAM ProtectedResources, Roles, discovery config/samples/ example resources ``` +## Access + +`config/milo` registers the service with Milo: + +- `iam/`: a `ProtectedResource` per kind (users act on their own `User`; + staff act platform-wide) and roles: + + | Role | For | + | --- | --- | + | `feedback.miloapis.com-user` | every signed-in user, in their own user context | + | `feedback.miloapis.com-public-reader` | the website: visible posts, releases, changelog, acknowledgements | + | `feedback.miloapis.com-viewer` | staff read access, including hidden posts | + | `feedback.miloapis.com-reviewer` | the feedback queue: submissions, reviews, hiding posts | + | `feedback.miloapis.com-editor`, `-admin` | managing `FeedbackSource`s | + | `feedback.miloapis.com-security-team` | security reports; no other role can see them | + +- `rbac/`: Kubernetes RBAC in Milo for the service's own components, which + authenticate with client certificates: + `system:feedback-sync@feedback.miloapis.com`, + `system:feedback-controller@feedback.miloapis.com` and + `system:feedback-apiserver@feedback.miloapis.com`. Each is limited to its + own reads and writes; none needs `system:masters`. +- `discovery/`: everything users touch appears in the platform and user + contexts; `FeedbackSource` only in the platform context. +- `bindings/` (optional): the `feedback-security-team` group with the + security-team role, and the reviewer role for `staff-users`. + +Users get the user role by adding it to `inheritedRoles` of Milo's +`iam-user-self-manage` role. That role is shared with other services, so +the deployment patches it and keeps their entries. + +The apiserver enforces the contexts too: votes and subscriptions change +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. + ## Postgres The apiserver is the only component that connects to Postgres. Other @@ -64,8 +100,10 @@ migrating at the same time. Add a migration as the next numbered file in ## Who writes what `feedback-sync` and `feedback-controller` are recognised by username -(`--sync-users`, `--controller-users`; defaults are the `feedback-system` -service accounts). On top of Milo's IAM: +(`--sync-users`, `--controller-users`). The defaults are the common names of +the client certificates they present to Milo: +`system:feedback-sync@feedback.miloapis.com` and +`system:feedback-controller@feedback.miloapis.com`. On top of Milo's IAM: - Only `feedback-sync` creates and deletes posts, and writes releases and changelog entries. @@ -74,9 +112,11 @@ service accounts). On top of Milo's IAM: history, `highestNotified` and part-shipped records; the server computes vote counts. Fields a caller doesn't own are ignored, like spec on a status update. Anyone else gets Forbidden on post status. -- Staff can change `spec.hidden` and metadata. Hidden posts are NotFound - outside the platform context and never listed on releases or changelog - entries. +- Staff can change `spec.hidden` and metadata. Hidden posts are visible only + to callers allowed `get` on `posts/hidden` (staff, sync, the controller), + in any context; everyone else, the website included, gets NotFound. They + are never listed on releases or changelog entries or returned by + `PostQuery`. - `Release.status.posts` and `ChangelogEntry.status.posts` are derived from the posts table on every read. - Votes on a deleted post are kept for `--vote-retention` (30 days) in case diff --git a/config/milo/bindings/kustomization.yaml b/config/milo/bindings/kustomization.yaml new file mode 100644 index 0000000..3056556 --- /dev/null +++ b/config/milo/bindings/kustomization.yaml @@ -0,0 +1,26 @@ +# Who gets which role, platform-wide. Optional: a deployment includes this +# component, or binds the roles to its own groups instead. +# +# - feedback-security-team gets the security-team role on reports. +# - staff-users get the reviewer role on everything else. PolicyBindings +# select one kind each, so there is one file per kind. +# +# Users get feedback.miloapis.com-user on their own User by adding it to +# inheritedRoles of Milo's iam-user-self-manage role. That role is shared +# with other services, so the deployment patches it, keeping their entries. +apiVersion: kustomize.config.k8s.io/v1alpha1 +kind: Component + +resources: + - security-team-group.yaml + - security-team-securityreports.yaml + - security-team-securityacknowledgements.yaml + - staff-reviewer-feedbacksources.yaml + - staff-reviewer-posts.yaml + - staff-reviewer-postqueries.yaml + - staff-reviewer-releases.yaml + - staff-reviewer-changelogentries.yaml + - staff-reviewer-votes.yaml + - staff-reviewer-subscriptions.yaml + - staff-reviewer-submissions.yaml + - staff-reviewer-securityacknowledgements.yaml diff --git a/config/milo/bindings/security-team-group.yaml b/config/milo/bindings/security-team-group.yaml new file mode 100644 index 0000000..0a015a5 --- /dev/null +++ b/config/milo/bindings/security-team-group.yaml @@ -0,0 +1,7 @@ +# Members are added with GroupMemberships. Keep it to the people who handle +# vulnerability reports: membership grants every report's contents. +apiVersion: iam.miloapis.com/v1alpha1 +kind: Group +metadata: + name: feedback-security-team + namespace: milo-system diff --git a/config/milo/bindings/security-team-securityacknowledgements.yaml b/config/milo/bindings/security-team-securityacknowledgements.yaml new file mode 100644 index 0000000..0327441 --- /dev/null +++ b/config/milo/bindings/security-team-securityacknowledgements.yaml @@ -0,0 +1,17 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: feedback-security-team-securityacknowledgements + namespace: milo-system +spec: + roleRef: + name: feedback.miloapis.com-security-team + namespace: milo-system + subjects: + - kind: Group + name: feedback-security-team + namespace: milo-system + resourceSelector: + resourceKind: + apiGroup: feedback.miloapis.com + kind: SecurityAcknowledgement diff --git a/config/milo/bindings/security-team-securityreports.yaml b/config/milo/bindings/security-team-securityreports.yaml new file mode 100644 index 0000000..ae8cd6d --- /dev/null +++ b/config/milo/bindings/security-team-securityreports.yaml @@ -0,0 +1,17 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: feedback-security-team-securityreports + namespace: milo-system +spec: + roleRef: + name: feedback.miloapis.com-security-team + namespace: milo-system + subjects: + - kind: Group + name: feedback-security-team + namespace: milo-system + resourceSelector: + resourceKind: + apiGroup: feedback.miloapis.com + kind: SecurityReport diff --git a/config/milo/bindings/staff-reviewer-changelogentries.yaml b/config/milo/bindings/staff-reviewer-changelogentries.yaml new file mode 100644 index 0000000..2e535d9 --- /dev/null +++ b/config/milo/bindings/staff-reviewer-changelogentries.yaml @@ -0,0 +1,17 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: feedback-staff-reviewer-changelogentries + namespace: milo-system +spec: + roleRef: + name: feedback.miloapis.com-reviewer + namespace: milo-system + subjects: + - kind: Group + name: staff-users + namespace: milo-system + resourceSelector: + resourceKind: + apiGroup: feedback.miloapis.com + kind: ChangelogEntry diff --git a/config/milo/bindings/staff-reviewer-feedbacksources.yaml b/config/milo/bindings/staff-reviewer-feedbacksources.yaml new file mode 100644 index 0000000..20d5950 --- /dev/null +++ b/config/milo/bindings/staff-reviewer-feedbacksources.yaml @@ -0,0 +1,17 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: feedback-staff-reviewer-feedbacksources + namespace: milo-system +spec: + roleRef: + name: feedback.miloapis.com-reviewer + namespace: milo-system + subjects: + - kind: Group + name: staff-users + namespace: milo-system + resourceSelector: + resourceKind: + apiGroup: feedback.miloapis.com + kind: FeedbackSource diff --git a/config/milo/bindings/staff-reviewer-postqueries.yaml b/config/milo/bindings/staff-reviewer-postqueries.yaml new file mode 100644 index 0000000..0c8c50f --- /dev/null +++ b/config/milo/bindings/staff-reviewer-postqueries.yaml @@ -0,0 +1,17 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: feedback-staff-reviewer-postqueries + namespace: milo-system +spec: + roleRef: + name: feedback.miloapis.com-reviewer + namespace: milo-system + subjects: + - kind: Group + name: staff-users + namespace: milo-system + resourceSelector: + resourceKind: + apiGroup: feedback.miloapis.com + kind: PostQuery diff --git a/config/milo/bindings/staff-reviewer-posts.yaml b/config/milo/bindings/staff-reviewer-posts.yaml new file mode 100644 index 0000000..089972d --- /dev/null +++ b/config/milo/bindings/staff-reviewer-posts.yaml @@ -0,0 +1,17 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: feedback-staff-reviewer-posts + namespace: milo-system +spec: + roleRef: + name: feedback.miloapis.com-reviewer + namespace: milo-system + subjects: + - kind: Group + name: staff-users + namespace: milo-system + resourceSelector: + resourceKind: + apiGroup: feedback.miloapis.com + kind: Post diff --git a/config/milo/bindings/staff-reviewer-releases.yaml b/config/milo/bindings/staff-reviewer-releases.yaml new file mode 100644 index 0000000..6841688 --- /dev/null +++ b/config/milo/bindings/staff-reviewer-releases.yaml @@ -0,0 +1,17 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: feedback-staff-reviewer-releases + namespace: milo-system +spec: + roleRef: + name: feedback.miloapis.com-reviewer + namespace: milo-system + subjects: + - kind: Group + name: staff-users + namespace: milo-system + resourceSelector: + resourceKind: + apiGroup: feedback.miloapis.com + kind: Release diff --git a/config/milo/bindings/staff-reviewer-securityacknowledgements.yaml b/config/milo/bindings/staff-reviewer-securityacknowledgements.yaml new file mode 100644 index 0000000..d1c9c10 --- /dev/null +++ b/config/milo/bindings/staff-reviewer-securityacknowledgements.yaml @@ -0,0 +1,17 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: feedback-staff-reviewer-securityacknowledgements + namespace: milo-system +spec: + roleRef: + name: feedback.miloapis.com-reviewer + namespace: milo-system + subjects: + - kind: Group + name: staff-users + namespace: milo-system + resourceSelector: + resourceKind: + apiGroup: feedback.miloapis.com + kind: SecurityAcknowledgement diff --git a/config/milo/bindings/staff-reviewer-submissions.yaml b/config/milo/bindings/staff-reviewer-submissions.yaml new file mode 100644 index 0000000..8e42abb --- /dev/null +++ b/config/milo/bindings/staff-reviewer-submissions.yaml @@ -0,0 +1,17 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: feedback-staff-reviewer-submissions + namespace: milo-system +spec: + roleRef: + name: feedback.miloapis.com-reviewer + namespace: milo-system + subjects: + - kind: Group + name: staff-users + namespace: milo-system + resourceSelector: + resourceKind: + apiGroup: feedback.miloapis.com + kind: Submission diff --git a/config/milo/bindings/staff-reviewer-subscriptions.yaml b/config/milo/bindings/staff-reviewer-subscriptions.yaml new file mode 100644 index 0000000..66389df --- /dev/null +++ b/config/milo/bindings/staff-reviewer-subscriptions.yaml @@ -0,0 +1,17 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: feedback-staff-reviewer-subscriptions + namespace: milo-system +spec: + roleRef: + name: feedback.miloapis.com-reviewer + namespace: milo-system + subjects: + - kind: Group + name: staff-users + namespace: milo-system + resourceSelector: + resourceKind: + apiGroup: feedback.miloapis.com + kind: Subscription diff --git a/config/milo/bindings/staff-reviewer-votes.yaml b/config/milo/bindings/staff-reviewer-votes.yaml new file mode 100644 index 0000000..514e5f2 --- /dev/null +++ b/config/milo/bindings/staff-reviewer-votes.yaml @@ -0,0 +1,17 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: PolicyBinding +metadata: + name: feedback-staff-reviewer-votes + namespace: milo-system +spec: + roleRef: + name: feedback.miloapis.com-reviewer + namespace: milo-system + subjects: + - kind: Group + name: staff-users + namespace: milo-system + resourceSelector: + resourceKind: + apiGroup: feedback.miloapis.com + kind: Vote diff --git a/config/milo/discovery/discovery-context-policy.yaml b/config/milo/discovery/discovery-context-policy.yaml index 71781d5..4af392f 100644 --- a/config/milo/discovery/discovery-context-policy.yaml +++ b/config/milo/discovery/discovery-context-policy.yaml @@ -4,8 +4,26 @@ metadata: name: feedback-service spec: rules: - # FeedbackSource is staff configuration. User-context resources (Vote, - # Submission, Subscription, ...) get their own rules as they are added. + # What a signed-in user works with in their own context: browsing and + # voting, following, and sending feedback and security reports. - group: feedback.miloapis.com - resources: ["feedbacksources"] - contexts: ["Platform"] + resources: + - posts + - postqueries + - releases + - changelogentries + - votes + - subscriptions + - attachmentuploads + - submissions + - securityreports + - securityacknowledgements + contexts: + - Platform + - User + # Staff configuration. + - group: feedback.miloapis.com + resources: + - feedbacksources + contexts: + - Platform diff --git a/config/milo/iam/resources/attachmentuploads.yaml b/config/milo/iam/resources/attachmentuploads.yaml new file mode 100644 index 0000000..c4e9c31 --- /dev/null +++ b/config/milo/iam/resources/attachmentuploads.yaml @@ -0,0 +1,15 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: ProtectedResource +metadata: + name: feedback.miloapis.com-attachmentuploads +spec: + serviceRef: + name: "feedback.miloapis.com" + kind: AttachmentUpload + plural: attachmentuploads + singular: attachmentupload + permissions: + - create + parentResources: + - apiGroup: iam.miloapis.com + kind: User diff --git a/config/milo/iam/resources/changelogentries.yaml b/config/milo/iam/resources/changelogentries.yaml new file mode 100644 index 0000000..f65dac3 --- /dev/null +++ b/config/milo/iam/resources/changelogentries.yaml @@ -0,0 +1,26 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: ProtectedResource +metadata: + name: feedback.miloapis.com-changelogentries +spec: + serviceRef: + name: "feedback.miloapis.com" + kind: ChangelogEntry + plural: changelogentries + singular: changelogentry + subresources: + - name: status + permissions: + - get + - update + - patch + permissions: + - get + - list + - create + - update + - patch + - delete + parentResources: + - apiGroup: iam.miloapis.com + kind: User diff --git a/config/milo/iam/resources/feedbacksources.yaml b/config/milo/iam/resources/feedbacksources.yaml index 86c3903..0fe8782 100644 --- a/config/milo/iam/resources/feedbacksources.yaml +++ b/config/milo/iam/resources/feedbacksources.yaml @@ -15,10 +15,10 @@ spec: - update - patch permissions: - - list - get + - list - create - update - - delete - patch + - delete - watch diff --git a/config/milo/iam/resources/kustomization.yaml b/config/milo/iam/resources/kustomization.yaml index b450c78..4977620 100644 --- a/config/milo/iam/resources/kustomization.yaml +++ b/config/milo/iam/resources/kustomization.yaml @@ -3,3 +3,13 @@ kind: Component resources: - feedbacksources.yaml + - posts.yaml + - postqueries.yaml + - releases.yaml + - changelogentries.yaml + - votes.yaml + - subscriptions.yaml + - attachmentuploads.yaml + - submissions.yaml + - securityreports.yaml + - securityacknowledgements.yaml diff --git a/config/milo/iam/resources/postqueries.yaml b/config/milo/iam/resources/postqueries.yaml new file mode 100644 index 0000000..f1b632a --- /dev/null +++ b/config/milo/iam/resources/postqueries.yaml @@ -0,0 +1,15 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: ProtectedResource +metadata: + name: feedback.miloapis.com-postqueries +spec: + serviceRef: + name: "feedback.miloapis.com" + kind: PostQuery + plural: postqueries + singular: postquery + permissions: + - create + parentResources: + - apiGroup: iam.miloapis.com + kind: User diff --git a/config/milo/iam/resources/posts.yaml b/config/milo/iam/resources/posts.yaml new file mode 100644 index 0000000..a5ec36d --- /dev/null +++ b/config/milo/iam/resources/posts.yaml @@ -0,0 +1,32 @@ +# posts/hidden is a virtual subresource: the apiserver asks whether the +# caller may get it before showing hidden posts. Grant it to staff and the +# service's own components only. +apiVersion: iam.miloapis.com/v1alpha1 +kind: ProtectedResource +metadata: + name: feedback.miloapis.com-posts +spec: + serviceRef: + name: "feedback.miloapis.com" + kind: Post + plural: posts + singular: post + subresources: + - name: status + permissions: + - get + - update + - patch + - name: hidden + permissions: + - get + permissions: + - get + - list + - create + - update + - patch + - delete + parentResources: + - apiGroup: iam.miloapis.com + kind: User diff --git a/config/milo/iam/resources/releases.yaml b/config/milo/iam/resources/releases.yaml new file mode 100644 index 0000000..53a37a0 --- /dev/null +++ b/config/milo/iam/resources/releases.yaml @@ -0,0 +1,26 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: ProtectedResource +metadata: + name: feedback.miloapis.com-releases +spec: + serviceRef: + name: "feedback.miloapis.com" + kind: Release + plural: releases + singular: release + subresources: + - name: status + permissions: + - get + - update + - patch + permissions: + - get + - list + - create + - update + - patch + - delete + parentResources: + - apiGroup: iam.miloapis.com + kind: User diff --git a/config/milo/iam/resources/securityacknowledgements.yaml b/config/milo/iam/resources/securityacknowledgements.yaml new file mode 100644 index 0000000..47dc5dd --- /dev/null +++ b/config/milo/iam/resources/securityacknowledgements.yaml @@ -0,0 +1,16 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: ProtectedResource +metadata: + name: feedback.miloapis.com-securityacknowledgements +spec: + serviceRef: + name: "feedback.miloapis.com" + kind: SecurityAcknowledgement + plural: securityacknowledgements + singular: securityacknowledgement + permissions: + - get + - list + parentResources: + - apiGroup: iam.miloapis.com + kind: User diff --git a/config/milo/iam/resources/securityreports.yaml b/config/milo/iam/resources/securityreports.yaml new file mode 100644 index 0000000..3b6c388 --- /dev/null +++ b/config/milo/iam/resources/securityreports.yaml @@ -0,0 +1,29 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: ProtectedResource +metadata: + name: feedback.miloapis.com-securityreports +spec: + serviceRef: + name: "feedback.miloapis.com" + kind: SecurityReport + plural: securityreports + singular: securityreport + subresources: + - name: status + permissions: + - get + - update + - patch + - name: attachment + permissions: + - get + permissions: + - get + - list + - create + - update + - patch + - delete + parentResources: + - apiGroup: iam.miloapis.com + kind: User diff --git a/config/milo/iam/resources/submissions.yaml b/config/milo/iam/resources/submissions.yaml new file mode 100644 index 0000000..2598322 --- /dev/null +++ b/config/milo/iam/resources/submissions.yaml @@ -0,0 +1,37 @@ +# review and to-security-report are staff decisions; attachment serves a +# submission's files. +apiVersion: iam.miloapis.com/v1alpha1 +kind: ProtectedResource +metadata: + name: feedback.miloapis.com-submissions +spec: + serviceRef: + name: "feedback.miloapis.com" + kind: Submission + plural: submissions + singular: submission + subresources: + - name: status + permissions: + - get + - update + - patch + - name: review + permissions: + - create + - name: attachment + permissions: + - get + - name: to-security-report + permissions: + - create + permissions: + - get + - list + - create + - update + - patch + - delete + parentResources: + - apiGroup: iam.miloapis.com + kind: User diff --git a/config/milo/iam/resources/subscriptions.yaml b/config/milo/iam/resources/subscriptions.yaml new file mode 100644 index 0000000..efcd381 --- /dev/null +++ b/config/milo/iam/resources/subscriptions.yaml @@ -0,0 +1,20 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: ProtectedResource +metadata: + name: feedback.miloapis.com-subscriptions +spec: + serviceRef: + name: "feedback.miloapis.com" + kind: Subscription + plural: subscriptions + singular: subscription + permissions: + - get + - list + - create + - update + - patch + - delete + parentResources: + - apiGroup: iam.miloapis.com + kind: User diff --git a/config/milo/iam/resources/votes.yaml b/config/milo/iam/resources/votes.yaml new file mode 100644 index 0000000..4210f89 --- /dev/null +++ b/config/milo/iam/resources/votes.yaml @@ -0,0 +1,20 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: ProtectedResource +metadata: + name: feedback.miloapis.com-votes +spec: + serviceRef: + name: "feedback.miloapis.com" + kind: Vote + plural: votes + singular: vote + permissions: + - get + - list + - create + - update + - patch + - delete + parentResources: + - apiGroup: iam.miloapis.com + kind: User diff --git a/config/milo/iam/roles/admin.yaml b/config/milo/iam/roles/admin.yaml index d680418..9a72671 100644 --- a/config/milo/iam/roles/admin.yaml +++ b/config/milo/iam/roles/admin.yaml @@ -6,10 +6,15 @@ metadata: labels: app.kubernetes.io/name: admin app.kubernetes.io/part-of: feedback.miloapis.com + annotations: + kubernetes.io/display-name: Feedback Admin + kubernetes.io/description: "Everything an editor and a reviewer can do, plus deleting feedback sources. Security reports stay with the Security Team role." spec: launchStage: Alpha inheritedRoles: - name: feedback.miloapis.com-editor namespace: milo-system + - name: feedback.miloapis.com-reviewer + namespace: milo-system includedPermissions: - feedback.miloapis.com/feedbacksources.delete diff --git a/config/milo/iam/roles/editor.yaml b/config/milo/iam/roles/editor.yaml index 18362c1..faee26b 100644 --- a/config/milo/iam/roles/editor.yaml +++ b/config/milo/iam/roles/editor.yaml @@ -6,6 +6,9 @@ metadata: labels: app.kubernetes.io/name: editor app.kubernetes.io/part-of: feedback.miloapis.com + annotations: + kubernetes.io/display-name: Feedback Editor + kubernetes.io/description: "Configures which GitHub repositories feed the roadmap, on top of viewer access." spec: launchStage: Alpha inheritedRoles: diff --git a/config/milo/iam/roles/kustomization.yaml b/config/milo/iam/roles/kustomization.yaml index e539027..9dbc361 100644 --- a/config/milo/iam/roles/kustomization.yaml +++ b/config/milo/iam/roles/kustomization.yaml @@ -2,6 +2,10 @@ apiVersion: kustomize.config.k8s.io/v1alpha1 kind: Component resources: + - user.yaml + - public-reader.yaml - viewer.yaml + - reviewer.yaml - editor.yaml - admin.yaml + - security-team.yaml diff --git a/config/milo/iam/roles/public-reader.yaml b/config/milo/iam/roles/public-reader.yaml new file mode 100644 index 0000000..b6d6da3 --- /dev/null +++ b/config/milo/iam/roles/public-reader.yaml @@ -0,0 +1,23 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: Role +metadata: + name: feedback.miloapis.com-public-reader + namespace: milo-system + labels: + app.kubernetes.io/name: public-reader + app.kubernetes.io/part-of: feedback.miloapis.com + annotations: + kubernetes.io/display-name: Feedback Public Reader + kubernetes.io/description: "Reads what datum.net shows: visible posts, releases, changelog entries and security acknowledgements." +spec: + launchStage: Alpha + includedPermissions: + - feedback.miloapis.com/posts.get + - feedback.miloapis.com/posts.list + - feedback.miloapis.com/postqueries.create + - feedback.miloapis.com/releases.get + - feedback.miloapis.com/releases.list + - feedback.miloapis.com/changelogentries.get + - feedback.miloapis.com/changelogentries.list + - feedback.miloapis.com/securityacknowledgements.get + - feedback.miloapis.com/securityacknowledgements.list diff --git a/config/milo/iam/roles/reviewer.yaml b/config/milo/iam/roles/reviewer.yaml new file mode 100644 index 0000000..5e5045f --- /dev/null +++ b/config/milo/iam/roles/reviewer.yaml @@ -0,0 +1,28 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: Role +metadata: + name: feedback.miloapis.com-reviewer + namespace: milo-system + labels: + app.kubernetes.io/name: reviewer + app.kubernetes.io/part-of: feedback.miloapis.com + annotations: + kubernetes.io/display-name: Feedback Reviewer + kubernetes.io/description: "Works the feedback queue: reads, decides on and deletes submissions, moves misfiled ones to the security track, and hides posts. Cannot see security reports." +spec: + launchStage: Alpha + inheritedRoles: + - name: feedback.miloapis.com-viewer + namespace: milo-system + includedPermissions: + - feedback.miloapis.com/posts.update + - feedback.miloapis.com/posts.patch + - feedback.miloapis.com/submissions.get + - feedback.miloapis.com/submissions.list + - feedback.miloapis.com/submissions.update + - feedback.miloapis.com/submissions.patch + - feedback.miloapis.com/submissions.delete + - feedback.miloapis.com/submissions/status.get + - feedback.miloapis.com/submissions/review.create + - feedback.miloapis.com/submissions/attachment.get + - feedback.miloapis.com/submissions/to-security-report.create diff --git a/config/milo/iam/roles/security-team.yaml b/config/milo/iam/roles/security-team.yaml new file mode 100644 index 0000000..578b210 --- /dev/null +++ b/config/milo/iam/roles/security-team.yaml @@ -0,0 +1,26 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: Role +metadata: + name: feedback.miloapis.com-security-team + namespace: milo-system + labels: + app.kubernetes.io/name: security-team + app.kubernetes.io/part-of: feedback.miloapis.com + annotations: + kubernetes.io/display-name: Feedback Security Team + kubernetes.io/description: "Works vulnerability reports: reads every report, logs reports from email and GitHub, writes status, and reads attachments. Separate from feedback reviewers." +spec: + launchStage: Alpha + includedPermissions: + - feedback.miloapis.com/securityreports.get + - feedback.miloapis.com/securityreports.list + - feedback.miloapis.com/securityreports.create + - feedback.miloapis.com/securityreports.update + - feedback.miloapis.com/securityreports.patch + - feedback.miloapis.com/securityreports.delete + - feedback.miloapis.com/securityreports/status.get + - feedback.miloapis.com/securityreports/status.update + - feedback.miloapis.com/securityreports/status.patch + - feedback.miloapis.com/securityreports/attachment.get + - feedback.miloapis.com/securityacknowledgements.get + - feedback.miloapis.com/securityacknowledgements.list diff --git a/config/milo/iam/roles/user.yaml b/config/milo/iam/roles/user.yaml new file mode 100644 index 0000000..9654120 --- /dev/null +++ b/config/milo/iam/roles/user.yaml @@ -0,0 +1,44 @@ +apiVersion: iam.miloapis.com/v1alpha1 +kind: Role +metadata: + name: feedback.miloapis.com-user + namespace: milo-system + labels: + app.kubernetes.io/name: user + app.kubernetes.io/part-of: feedback.miloapis.com + annotations: + kubernetes.io/display-name: Feedback User + kubernetes.io/description: "What every signed-in user can do with feedback in their own user context: browse and search posts, vote, follow, send feedback and security reports, and read their own." +spec: + launchStage: Alpha + includedPermissions: + - feedback.miloapis.com/posts.get + - feedback.miloapis.com/posts.list + - feedback.miloapis.com/postqueries.create + - feedback.miloapis.com/releases.get + - feedback.miloapis.com/releases.list + - feedback.miloapis.com/changelogentries.get + - feedback.miloapis.com/changelogentries.list + - feedback.miloapis.com/votes.get + - feedback.miloapis.com/votes.list + - feedback.miloapis.com/votes.create + - feedback.miloapis.com/votes.update + - feedback.miloapis.com/votes.patch + - feedback.miloapis.com/votes.delete + - feedback.miloapis.com/subscriptions.get + - feedback.miloapis.com/subscriptions.list + - feedback.miloapis.com/subscriptions.create + - feedback.miloapis.com/subscriptions.update + - feedback.miloapis.com/subscriptions.patch + - feedback.miloapis.com/subscriptions.delete + - feedback.miloapis.com/attachmentuploads.create + - feedback.miloapis.com/submissions.get + - feedback.miloapis.com/submissions.list + - feedback.miloapis.com/submissions.create + - feedback.miloapis.com/submissions/attachment.get + - feedback.miloapis.com/securityreports.get + - feedback.miloapis.com/securityreports.list + - feedback.miloapis.com/securityreports.create + - feedback.miloapis.com/securityreports/attachment.get + - feedback.miloapis.com/securityacknowledgements.get + - feedback.miloapis.com/securityacknowledgements.list diff --git a/config/milo/iam/roles/viewer.yaml b/config/milo/iam/roles/viewer.yaml index a0e43a9..d3cc08f 100644 --- a/config/milo/iam/roles/viewer.yaml +++ b/config/milo/iam/roles/viewer.yaml @@ -6,6 +6,9 @@ metadata: labels: app.kubernetes.io/name: viewer app.kubernetes.io/part-of: feedback.miloapis.com + annotations: + kubernetes.io/display-name: Feedback Viewer + kubernetes.io/description: "Staff read access to feedback: sources, every post including hidden ones, releases, changelog entries, and votes and subscriptions across users. No submissions or security reports." spec: launchStage: Alpha includedPermissions: @@ -13,3 +16,20 @@ spec: - feedback.miloapis.com/feedbacksources.list - feedback.miloapis.com/feedbacksources.watch - feedback.miloapis.com/feedbacksources/status.get + - feedback.miloapis.com/posts.get + - feedback.miloapis.com/posts.list + - feedback.miloapis.com/posts/status.get + - feedback.miloapis.com/posts/hidden.get + - feedback.miloapis.com/postqueries.create + - feedback.miloapis.com/releases.get + - feedback.miloapis.com/releases.list + - feedback.miloapis.com/releases/status.get + - feedback.miloapis.com/changelogentries.get + - feedback.miloapis.com/changelogentries.list + - feedback.miloapis.com/changelogentries/status.get + - feedback.miloapis.com/votes.get + - feedback.miloapis.com/votes.list + - feedback.miloapis.com/subscriptions.get + - feedback.miloapis.com/subscriptions.list + - feedback.miloapis.com/securityacknowledgements.get + - feedback.miloapis.com/securityacknowledgements.list diff --git a/config/milo/kustomization.yaml b/config/milo/kustomization.yaml index a25611b..7d33e87 100644 --- a/config/milo/kustomization.yaml +++ b/config/milo/kustomization.yaml @@ -1,6 +1,11 @@ +# What the service registers in Milo: ProtectedResources and Roles (iam/), +# where resources appear (discovery/), and Kubernetes RBAC for the service's +# own components (rbac/). bindings/ is separate and optional: who holds the +# roles is the deployment's choice. apiVersion: kustomize.config.k8s.io/v1alpha1 kind: Component components: - iam/ - discovery/ + - rbac/ diff --git a/config/milo/rbac/feedback-apiserver.yaml b/config/milo/rbac/feedback-apiserver.yaml new file mode 100644 index 0000000..814d2ed --- /dev/null +++ b/config/milo/rbac/feedback-apiserver.yaml @@ -0,0 +1,62 @@ +# The apiserver calls Milo itself (--milo-kubeconfig): keyword search over +# posts, and the voter's organizations and linked GitHub username. Listing +# across users needs cluster-wide list; the field selectors it sends narrow +# the result, not the permission. +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: feedback.miloapis.com:apiserver +rules: + - apiGroups: [search.miloapis.com] + resources: [resourcesearchqueries] + verbs: [create] + - apiGroups: [resourcemanager.miloapis.com] + resources: [organizationmemberships] + verbs: [list] + - apiGroups: [identity.miloapis.com] + resources: [useridentities] + verbs: [list] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: feedback.miloapis.com:apiserver +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: feedback.miloapis.com:apiserver +subjects: + - apiGroup: rbac.authorization.k8s.io + kind: User + name: system:feedback-apiserver@feedback.miloapis.com +--- +# Delegated authentication and authorization against Milo, when the +# apiserver's --authentication-kubeconfig and --authorization-kubeconfig use +# this identity. Authorization is also how it decides who sees hidden posts +# (a SubjectAccessReview for get on posts/hidden). +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: feedback.miloapis.com:apiserver:auth-delegator +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: system:auth-delegator +subjects: + - apiGroup: rbac.authorization.k8s.io + kind: User + name: system:feedback-apiserver@feedback.miloapis.com +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: feedback.miloapis.com:apiserver:authentication-reader + namespace: kube-system +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: extension-apiserver-authentication-reader +subjects: + - apiGroup: rbac.authorization.k8s.io + kind: User + name: system:feedback-apiserver@feedback.miloapis.com diff --git a/config/milo/rbac/feedback-controller.yaml b/config/milo/rbac/feedback-controller.yaml new file mode 100644 index 0000000..9c2a717 --- /dev/null +++ b/config/milo/rbac/feedback-controller.yaml @@ -0,0 +1,33 @@ +# feedback-controller tracks delivery on posts, triages submissions, and +# sends notifications and digests, so it reads subscriptions, votes and +# security reports. It writes only post and submission status. +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: feedback.miloapis.com:controller +rules: + - apiGroups: [feedback.miloapis.com] + resources: [posts, releases, changelogentries, submissions, votes, subscriptions, securityreports, feedbacksources] + verbs: [get, list] + - apiGroups: [feedback.miloapis.com] + resources: [posts/status, submissions/status] + verbs: [get, update, patch] + - apiGroups: [feedback.miloapis.com] + resources: [posts/hidden] + verbs: [get] + - apiGroups: [feedback.miloapis.com] + resources: [postqueries] + verbs: [create] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: feedback.miloapis.com:controller +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: feedback.miloapis.com:controller +subjects: + - apiGroup: rbac.authorization.k8s.io + kind: User + name: system:feedback-controller@feedback.miloapis.com diff --git a/config/milo/rbac/feedback-sync.yaml b/config/milo/rbac/feedback-sync.yaml new file mode 100644 index 0000000..dfc695c --- /dev/null +++ b/config/milo/rbac/feedback-sync.yaml @@ -0,0 +1,43 @@ +# feedback-sync mirrors GitHub into posts, releases and changelog entries, +# reads its FeedbackSources, and links approved submissions to their posts. +# It authenticates to Milo with a client certificate whose common name is +# the subject below; the apiserver recognises the same name (--sync-users). +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: feedback.miloapis.com:sync +rules: + - apiGroups: [feedback.miloapis.com] + resources: [feedbacksources] + verbs: [get, list, watch] + - apiGroups: [feedback.miloapis.com] + resources: [feedbacksources/status] + verbs: [get, update, patch] + - apiGroups: [feedback.miloapis.com] + resources: [posts, releases, changelogentries] + verbs: [get, list, create, update, patch, delete] + - apiGroups: [feedback.miloapis.com] + resources: [posts/status, releases/status, changelogentries/status] + verbs: [get, update, patch] + - apiGroups: [feedback.miloapis.com] + resources: [posts/hidden] + verbs: [get] + - apiGroups: [feedback.miloapis.com] + resources: [submissions] + verbs: [get, list] + - apiGroups: [feedback.miloapis.com] + resources: [submissions/status] + verbs: [get, update, patch] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: feedback.miloapis.com:sync +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: feedback.miloapis.com:sync +subjects: + - apiGroup: rbac.authorization.k8s.io + kind: User + name: system:feedback-sync@feedback.miloapis.com diff --git a/config/milo/rbac/kustomization.yaml b/config/milo/rbac/kustomization.yaml new file mode 100644 index 0000000..1a6c575 --- /dev/null +++ b/config/milo/rbac/kustomization.yaml @@ -0,0 +1,9 @@ +# Kubernetes RBAC in Milo for the service's own components, which +# authenticate with client certificates rather than as Milo users. +apiVersion: kustomize.config.k8s.io/v1alpha1 +kind: Component + +resources: + - feedback-sync.yaml + - feedback-controller.yaml + - feedback-apiserver.yaml diff --git a/internal/apiserver/apiserver.go b/internal/apiserver/apiserver.go index 3c44257..596a74b 100644 --- a/internal/apiserver/apiserver.go +++ b/internal/apiserver/apiserver.go @@ -164,14 +164,14 @@ func (c completedConfig) New(ctx context.Context) (*FeedbackServer, error) { pool.Close() return nil, fmt.Errorf("create FeedbackSource storage: %w", err) } - v1alpha1Storage["feedbacksources"] = sourceStorage.Store + v1alpha1Storage["feedbacksources"] = sourceStorage v1alpha1Storage["feedbacksources/status"] = sourceStorage.Status // Everything else is in Postgres. db := &pg.DB{Pool: pool} ids := c.ExtraConfig.Identities - posts, err := post.NewREST(db, Scheme, ids) + posts, err := post.NewREST(db, Scheme, ids, post.WithHiddenAuthorizer(c.GenericConfig.Authorization.Authorizer)) if err != nil { pool.Close() return nil, fmt.Errorf("create Post storage: %w", err) diff --git a/internal/apiserver/miloconfig_test.go b/internal/apiserver/miloconfig_test.go new file mode 100644 index 0000000..a86a9fb --- /dev/null +++ b/internal/apiserver/miloconfig_test.go @@ -0,0 +1,210 @@ +package apiserver + +import ( + "bytes" + "os" + "path/filepath" + "reflect" + "slices" + "strings" + "testing" + + "sigs.k8s.io/yaml" + + "go.miloapis.com/feedback/internal/identity" + feedbackv1alpha1 "go.miloapis.com/feedback/pkg/apis/feedback/v1alpha1" +) + +const group = feedbackv1alpha1.GroupName + +type obj map[string]any + +func (o obj) str(path ...string) string { + var cur any = map[string]any(o) + for _, p := range path { + m, ok := cur.(map[string]any) + if !ok { + return "" + } + cur = m[p] + } + s, _ := cur.(string) + return s +} + +func (o obj) list(path ...string) []any { + var cur any = map[string]any(o) + for _, p := range path { + m, ok := cur.(map[string]any) + if !ok { + return nil + } + cur = m[p] + } + l, _ := cur.([]any) + return l +} + +// loadMilo reads every object under config/milo. +func loadMilo(t *testing.T) []obj { + t.Helper() + var out []obj + err := filepath.WalkDir("../../config/milo", func(path string, d os.DirEntry, err error) error { + if err != nil || d.IsDir() || !strings.HasSuffix(path, ".yaml") || d.Name() == "kustomization.yaml" { + return err + } + data, err := os.ReadFile(path) + if err != nil { + return err + } + for _, doc := range bytes.Split(data, []byte("\n---\n")) { + var o obj + if err := yaml.Unmarshal(doc, &o); err != nil { + t.Fatalf("%s: %v", path, err) + } + if len(o) > 0 { + out = append(out, o) + } + } + return nil + }) + if err != nil { + t.Fatal(err) + } + return out +} + +func ofKind(objs []obj, kind string) []obj { + var out []obj + for _, o := range objs { + if o.str("kind") == kind { + out = append(out, o) + } + } + return out +} + +// TestMiloConfig checks the IAM, RBAC and discovery config agree with the +// API and with each other. +func TestMiloConfig(t *testing.T) { + objs := loadMilo(t) + + // perms["posts"] = {"get", ...}; perms["posts/status"] = {...} + perms := map[string][]string{} + kinds := map[string]string{} // kind -> plural + for _, pr := range ofKind(objs, "ProtectedResource") { + plural := pr.str("spec", "plural") + kinds[pr.str("spec", "kind")] = plural + for _, v := range pr.list("spec", "permissions") { + perms[plural] = append(perms[plural], v.(string)) + } + for _, s := range pr.list("spec", "subresources") { + so := obj(s.(map[string]any)) + for _, v := range so.list("permissions") { + perms[plural+"/"+so.str("name")] = append(perms[plural+"/"+so.str("name")], v.(string)) + } + } + } + + notResources := []string{"SubmissionReview", "SubmissionToSecurityReport"} + for _, o := range feedbackv1alpha1.KnownTypes() { + kind := reflect.TypeOf(o).Elem().Name() + if strings.HasSuffix(kind, "List") || slices.Contains(notResources, kind) { + continue + } + if kinds[kind] == "" { + t.Errorf("no ProtectedResource for %s", kind) + } + } + + allows := func(resource, verb string) bool { return slices.Contains(perms[resource], verb) } + + roles := map[string]bool{} + for _, r := range ofKind(objs, "Role") { + roles[r.str("metadata", "name")] = true + } + for _, r := range ofKind(objs, "Role") { + name := r.str("metadata", "name") + for _, p := range r.list("spec", "includedPermissions") { + s := p.(string) + rest, ok := strings.CutPrefix(s, group+"/") + i := strings.LastIndex(rest, ".") + if !ok || i < 0 || !allows(rest[:i], rest[i+1:]) { + t.Errorf("role %s: %s isn't a declared permission", name, s) + } + } + for _, in := range r.list("spec", "inheritedRoles") { + if n := obj(in.(map[string]any)).str("name"); !roles[n] { + t.Errorf("role %s inherits unknown role %s", name, n) + } + } + } + + for _, cr := range ofKind(objs, "ClusterRole") { + for _, rule := range cr.list("rules") { + ro := obj(rule.(map[string]any)) + if !slices.ContainsFunc(ro.list("apiGroups"), func(g any) bool { return g == group }) { + continue + } + for _, res := range ro.list("resources") { + for _, v := range ro.list("verbs") { + if !allows(res.(string), v.(string)) { + t.Errorf("ClusterRole %s grants %s %s, which isn't declared", cr.str("metadata", "name"), v, res) + } + } + } + } + } + + subjects := map[string]bool{} + for _, b := range ofKind(objs, "ClusterRoleBinding") { + for _, s := range b.list("subjects") { + subjects[obj(s.(map[string]any)).str("name")] = true + } + } + for _, u := range []string{identity.SyncUser, identity.ControllerUser, identity.APIServerUser} { + if !subjects[u] { + t.Errorf("no ClusterRoleBinding for %s", u) + } + } + if !slices.Equal(identity.Default.Sync, []string{identity.SyncUser}) || !slices.Equal(identity.Default.Controller, []string{identity.ControllerUser}) { + t.Errorf("identity defaults don't match the RBAC subjects: %+v", identity.Default) + } + + discovered := map[string]bool{} + for _, d := range ofKind(objs, "DiscoveryContextPolicy") { + for _, rule := range d.list("spec", "rules") { + for _, r := range obj(rule.(map[string]any)).list("resources") { + discovered[r.(string)] = true + } + } + } + for _, plural := range kinds { + if !discovered[plural] { + t.Errorf("%s isn't in the DiscoveryContextPolicy", plural) + } + } + + for _, b := range ofKind(objs, "PolicyBinding") { + if n := b.str("spec", "roleRef", "name"); !roles[n] { + t.Errorf("PolicyBinding %s binds unknown role %s", b.str("metadata", "name"), n) + } + if k := b.str("spec", "resourceSelector", "resourceKind", "kind"); kinds[k] == "" { + t.Errorf("PolicyBinding %s selects unknown kind %s", b.str("metadata", "name"), k) + } + } + + // Security reports stay out of every role but the Security Team's and + // the user's own. + for _, r := range ofKind(objs, "Role") { + name := r.str("metadata", "name") + if name == group+"-security-team" || name == group+"-user" { + continue + } + for _, p := range r.list("spec", "includedPermissions") { + if strings.HasPrefix(p.(string), group+"/securityreports") { + t.Errorf("role %s grants %s", name, p) + } + } + } +} diff --git a/internal/identity/identity.go b/internal/identity/identity.go index 1d07c07..c4c45e9 100644 --- a/internal/identity/identity.go +++ b/internal/identity/identity.go @@ -24,13 +24,23 @@ type Identities struct { Controller []string } -// Default usernames, matching the Kubernetes service accounts in -// feedback-system. Override with flags where Milo names them differently. +// Default usernames: the common names of the client certificates the +// components present to Milo, which forwards them as the username. They +// match the subjects of the ClusterRoleBindings in config/milo/rbac. var Default = Identities{ - Sync: []string{"system:serviceaccount:feedback-system:feedback-sync"}, - Controller: []string{"system:serviceaccount:feedback-system:feedback-controller"}, + Sync: []string{SyncUser}, + Controller: []string{ControllerUser}, } +// Usernames of the service's components in Milo. +const ( + SyncUser = "system:feedback-sync@feedback.miloapis.com" + ControllerUser = "system:feedback-controller@feedback.miloapis.com" + // APIServerUser is the apiserver itself, calling Milo for search and + // user lookups. + APIServerUser = "system:feedback-apiserver@feedback.miloapis.com" +) + // Caller is who a request comes from, as far as field ownership goes. type Caller int diff --git a/internal/registry/feedbacksource/guard.go b/internal/registry/feedbacksource/guard.go new file mode 100644 index 0000000..e0f7a03 --- /dev/null +++ b/internal/registry/feedbacksource/guard.go @@ -0,0 +1,90 @@ +package feedbacksource + +import ( + "context" + "errors" + + 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/apimachinery/pkg/watch" + "k8s.io/apiserver/pkg/registry/rest" + + "go.miloapis.com/feedback/internal/storage/pg" + feedbackv1alpha1 "go.miloapis.com/feedback/pkg/apis/feedback/v1alpha1" +) + +// FeedbackSources are staff configuration, served only in the platform +// context. Milo IAM already keeps them out of other contexts; this is the +// apiserver's own check. +func platformOnly(ctx context.Context) error { + if kind, _ := pg.RequestContext(ctx); kind != pg.ScopePlatform { + return apierrors.NewForbidden(feedbackv1alpha1.Resource("feedbacksources"), "", + errors.New("feedback sources are only available in the platform context")) + } + return nil +} + +func (r *REST) Get(ctx context.Context, name string, opts *metav1.GetOptions) (runtime.Object, error) { + if err := platformOnly(ctx); err != nil { + return nil, err + } + return r.Store.Get(ctx, name, opts) +} + +func (r *REST) List(ctx context.Context, opts *metainternalversion.ListOptions) (runtime.Object, error) { + if err := platformOnly(ctx); err != nil { + return nil, err + } + return r.Store.List(ctx, opts) +} + +func (r *REST) Watch(ctx context.Context, opts *metainternalversion.ListOptions) (watch.Interface, error) { + if err := platformOnly(ctx); err != nil { + return nil, err + } + return r.Store.Watch(ctx, opts) +} + +func (r *REST) Create(ctx context.Context, obj runtime.Object, v rest.ValidateObjectFunc, opts *metav1.CreateOptions) (runtime.Object, error) { + if err := platformOnly(ctx); err != nil { + return nil, err + } + return r.Store.Create(ctx, obj, v, opts) +} + +func (r *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, cv rest.ValidateObjectFunc, uv rest.ValidateObjectUpdateFunc, force bool, opts *metav1.UpdateOptions) (runtime.Object, bool, error) { + if err := platformOnly(ctx); err != nil { + return nil, false, err + } + return r.Store.Update(ctx, name, objInfo, cv, uv, force, opts) +} + +func (r *REST) Delete(ctx context.Context, name string, v rest.ValidateObjectFunc, opts *metav1.DeleteOptions) (runtime.Object, bool, error) { + if err := platformOnly(ctx); err != nil { + return nil, false, err + } + return r.Store.Delete(ctx, name, v, opts) +} + +func (r *REST) DeleteCollection(ctx context.Context, v rest.ValidateObjectFunc, opts *metav1.DeleteOptions, listOpts *metainternalversion.ListOptions) (runtime.Object, error) { + if err := platformOnly(ctx); err != nil { + return nil, err + } + return r.Store.DeleteCollection(ctx, v, opts, listOpts) +} + +func (r *StatusREST) Get(ctx context.Context, name string, opts *metav1.GetOptions) (runtime.Object, error) { + if err := platformOnly(ctx); err != nil { + return nil, err + } + return r.Store.Get(ctx, name, opts) +} + +func (r *StatusREST) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, cv rest.ValidateObjectFunc, uv rest.ValidateObjectUpdateFunc, force bool, opts *metav1.UpdateOptions) (runtime.Object, bool, error) { + if err := platformOnly(ctx); err != nil { + return nil, false, err + } + return r.Store.Update(ctx, name, objInfo, cv, uv, force, opts) +} diff --git a/internal/registry/feedbacksource/guard_test.go b/internal/registry/feedbacksource/guard_test.go new file mode 100644 index 0000000..19fc0f7 --- /dev/null +++ b/internal/registry/feedbacksource/guard_test.go @@ -0,0 +1,36 @@ +package feedbacksource + +import ( + "context" + "testing" + + 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/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/endpoints/request" +) + +func TestPlatformOnly(t *testing.T) { + inUser := request.WithUser(context.Background(), &user.DefaultInfo{Name: "u", Extra: map[string][]string{ + "iam.miloapis.com/parent-type": {"User"}, "iam.miloapis.com/parent-name": {"u"}}}) + staff := request.WithUser(context.Background(), &user.DefaultInfo{Name: "staff"}) + if err := platformOnly(staff); err != nil { + t.Errorf("platform: %v", err) + } + // The guard runs before the store, so a nil store is never reached. + r := &REST{Status: &StatusREST{}} + for name, err := range map[string]error{ + "get": func() error { _, err := r.Get(inUser, "x", &metav1.GetOptions{}); return err }(), + "list": func() error { _, err := r.List(inUser, &metainternalversion.ListOptions{}); return err }(), + "watch": func() error { _, err := r.Watch(inUser, &metainternalversion.ListOptions{}); return err }(), + "create": func() error { _, err := r.Create(inUser, nil, nil, nil); return err }(), + "update": func() error { _, _, err := r.Update(inUser, "x", nil, nil, nil, false, nil); return err }(), + "delete": func() error { _, _, err := r.Delete(inUser, "x", nil, nil); return err }(), + "status": func() error { _, _, err := r.Status.Update(inUser, "x", nil, nil, nil, false, nil); return err }(), + } { + if !apierrors.IsForbidden(err) { + t.Errorf("%s from the user context: %v", name, err) + } + } +} diff --git a/internal/registry/post/storage.go b/internal/registry/post/storage.go index 6aeca6c..7facb27 100644 --- a/internal/registry/post/storage.go +++ b/internal/registry/post/storage.go @@ -10,7 +10,10 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/duration" + "k8s.io/apiserver/pkg/authorization/authorizer" + "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" + "k8s.io/klog/v2" "go.miloapis.com/feedback/internal/identity" "go.miloapis.com/feedback/internal/storage/pg" @@ -23,8 +26,27 @@ type REST struct { Status *pg.StatusStore } +// Option configures post storage. +type Option func(*options) + +type options struct { + authz authorizer.Authorizer +} + +// WithHiddenAuthorizer decides who sees hidden posts: callers allowed get on +// the posts/hidden virtual subresource (staff, feedback-sync, +// feedback-controller). Without it, every platform-context caller sees them, +// which is only right for tests and local development. +func WithHiddenAuthorizer(a authorizer.Authorizer) Option { + return func(o *options) { o.authz = a } +} + // NewREST returns Postgres-backed storage for Post. -func NewREST(db *pg.DB, typer runtime.ObjectTyper, ids identity.Identities) (*REST, error) { +func NewREST(db *pg.DB, typer runtime.ObjectTyper, ids identity.Identities, opts ...Option) (*REST, error) { + var o options + for _, opt := range opts { + opt(&o) + } s := newStrategy(typer, ids) h := hooks{db: db, ids: ids} store, err := pg.NewStore(db, pg.Kind{ @@ -46,7 +68,7 @@ func NewREST(db *pg.DB, typer runtime.ObjectTyper, ids identity.Identities) (*RE "status.release": "release", }, TableConvertor: tableConvertor{}, - ReadFilter: readFilter, + ReadFilter: o.readFilter, BeforeWrite: h.beforeWrite, AfterWrite: h.afterWrite, }) @@ -56,14 +78,36 @@ func NewREST(db *pg.DB, typer runtime.ObjectTyper, ids identity.Identities) (*RE return &REST{Store: store, Status: pg.NewStatusStore(store, statusStrategy{s})}, nil } -// readFilter hides hidden posts outside the platform context. Platform -// callers (staff, the website's service account, the service's own -// components) see them and filter with spec.hidden=false where needed. -func readFilter(ctx context.Context) string { - if kind, _ := pg.RequestContext(ctx); kind != pg.ScopePlatform { +// readFilter hides hidden posts from callers who may not see them. Being in +// the platform context isn't enough: the website's service account reads +// posts there too. +func (o options) readFilter(ctx context.Context) string { + if o.authz == nil { + if kind, _ := pg.RequestContext(ctx); kind == pg.ScopePlatform { + return "" + } return "NOT hidden" } - return "" + if CanSeeHidden(ctx, o.authz) { + return "" + } + return "NOT hidden" +} + +// CanSeeHidden reports whether the caller may get posts/hidden. +func CanSeeHidden(ctx context.Context, a authorizer.Authorizer) bool { + u, ok := request.UserFrom(ctx) + if !ok { + return false + } + d, _, err := a.Authorize(ctx, authorizer.AttributesRecord{ + User: u, Verb: "get", APIGroup: v1.GroupName, APIVersion: v1.SchemeGroupVersion.Version, + Resource: "posts", Subresource: "hidden", ResourceRequest: true, + }) + if err != nil { + klog.V(2).InfoS("Checking posts/hidden failed; hiding hidden posts", "user", u.GetName(), "err", err) + } + return d == authorizer.DecisionAllow } func columns(obj runtime.Object) []pg.Column { diff --git a/internal/registry/post/storage_test.go b/internal/registry/post/storage_test.go index 7f0e1d8..9be4697 100644 --- a/internal/registry/post/storage_test.go +++ b/internal/registry/post/storage_test.go @@ -3,6 +3,7 @@ package post import ( "context" "os" + "slices" "testing" "time" @@ -11,6 +12,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/registry/rest" "go.miloapis.com/feedback/internal/storage/pg" @@ -314,3 +316,48 @@ func TestVoteRetention(t *testing.T) { t.Fatalf("deleted_posts still has %d rows", tracked) } } + +// allowHidden lets only the named users get posts/hidden. +func allowHidden(users ...string) authorizer.Authorizer { + return authorizer.AuthorizerFunc(func(_ context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) { + if a.GetResource() == "posts" && a.GetSubresource() == "hidden" && a.GetVerb() == "get" && slices.Contains(users, a.GetUser().GetName()) { + return authorizer.DecisionAllow, "", nil + } + return authorizer.DecisionNoOpinion, "", nil + }) +} + +func TestHiddenNeedsPermission(t *testing.T) { + db := pgtest.Require(t, "posts", "votes", "post_embeddings", "deleted_posts") + scheme := runtime.NewScheme() + install.Install(scheme) + r, err := NewREST(db, scheme, ids, WithHiddenAuthorizer(allowHidden("staff@datum.net", "sync"))) + if err != nil { + t.Fatal(err) + } + website := pgtest.AsPlatform("datum-net") + obj, err := r.Create(syncCtx, newPost(1, v1.PostStateBacklog), nil, &metav1.CreateOptions{}) + if err != nil { + t.Fatal(err) + } + p := obj.(*v1.Post) + p.Spec.Hidden = true + if _, err := update(r, staffCtx, p); err != nil { + t.Fatal(err) + } + + if _, err := r.Get(website, p.Name, &metav1.GetOptions{}); !apierrors.IsNotFound(err) { + t.Errorf("website gets a hidden post: %v", err) + } + list, err := r.List(website, &metainternalversion.ListOptions{}) + if err != nil || len(list.(*v1.PostList).Items) != 0 { + t.Errorf("website lists a hidden post: %v %v", list, err) + } + get(t, r, staffCtx, p.Name) + // Sync keeps updating hidden posts. + q := get(t, r, syncCtx, p.Name) + q.Status.Title = "synced" + if _, err := update(r.Status, syncCtx, q); err != nil { + t.Errorf("sync updates a hidden post: %v", err) + } +} diff --git a/internal/registry/vote/subscription.go b/internal/registry/vote/subscription.go index 1221e8c..e15020a 100644 --- a/internal/registry/vote/subscription.go +++ b/internal/registry/vote/subscription.go @@ -93,7 +93,7 @@ func NewSubscriptionREST(db *pg.DB, typer runtime.ObjectTyper) (*SubscriptionRES FieldColumns: map[string]string{"spec.postRef.name": "post_name", "spec.reason": "reason"}, BeforeWrite: func(ctx context.Context, op pg.Op, _, obj runtime.Object) error { if op != pg.OpCreate { - return nil + return userOnly(ctx) } sub := obj.(*v1.Subscription) return CheckPost(ctx, db, v1.Kind("Subscription"), sub.Name, sub.Spec.PostRef.Name) diff --git a/internal/registry/vote/vote.go b/internal/registry/vote/vote.go index 30e1b08..86721db 100644 --- a/internal/registry/vote/vote.go +++ b/internal/registry/vote/vote.go @@ -134,7 +134,7 @@ func NewREST(db *pg.DB, typer runtime.ObjectTyper, users milo.Users, subs *Subsc FieldColumns: map[string]string{"spec.postRef.name": "post_name"}, BeforeWrite: func(ctx context.Context, op pg.Op, _, obj runtime.Object) error { if op != pg.OpCreate { - return nil + return userOnly(ctx) } v := obj.(*v1.Vote) if err := requireVerifiedEmail(ctx, v.Name); err != nil { @@ -168,6 +168,15 @@ func NewREST(db *pg.DB, typer runtime.ObjectTyper, users milo.Users, subs *Subsc return &REST{Store: store}, nil } +// userOnly keeps changes to votes and subscriptions with their owner: staff +// can list them in the platform context but not change or delete them. +func userOnly(ctx context.Context) error { + if kind, _ := pg.RequestContext(ctx); kind != pg.ScopeUser { + return apierrors.NewForbidden(v1.Resource("votes"), "", errors.New("votes and subscriptions are changed only by their owner")) + } + return nil +} + // requireVerifiedEmail rejects people whose email isn't verified. func requireVerifiedEmail(ctx context.Context, name string) error { if identity.EmailUnverified(ctx) { diff --git a/internal/registry/vote/vote_test.go b/internal/registry/vote/vote_test.go index e1f8232..0b1bb66 100644 --- a/internal/registry/vote/vote_test.go +++ b/internal/registry/vote/vote_test.go @@ -426,3 +426,22 @@ func TestSubscriptions(t *testing.T) { t.Errorf("after delete: %v", err) } } + +func TestStaffReadOnly(t *testing.T) { + e := setup(t, fakeUsers{}) + p := e.addPost(t, 1, 0) + v := e.vote(t, alice, p) + if _, _, err := e.votes.Delete(staff, p, nil, &metav1.DeleteOptions{}); !apierrors.IsForbidden(err) { + t.Errorf("staff delete vote: %v", err) + } + if _, _, err := e.subs.Delete(staff, p, nil, &metav1.DeleteOptions{}); !apierrors.IsForbidden(err) { + t.Errorf("staff delete subscription: %v", err) + } + v.Labels = map[string]string{"x": "y"} + if _, _, err := e.votes.Update(staff, p, rest.DefaultUpdatedObjectInfo(v), nil, nil, false, &metav1.UpdateOptions{}); !apierrors.IsForbidden(err) { + t.Errorf("staff update vote: %v", err) + } + if _, _, err := e.votes.Update(alice, p, rest.DefaultUpdatedObjectInfo(v), nil, nil, false, &metav1.UpdateOptions{}); err != nil { + t.Errorf("owner labels own vote: %v", err) + } +}