Commit 757a6b5
Add MCPAuthzConfig CRD for backend-agnostic authorization (#4777)
* Add MCPAuthzConfig CRD for backend-agnostic authz
Introduce a namespace-scoped MCPAuthzConfig CRD so authorization
policy can be defined once and shared across MCPServer,
MCPRemoteProxy, and VirtualMCPServer workloads, mirroring the
existing MCPOIDCConfig sharing pattern.
The spec carries a backend Type plus an opaque Config RawExtension.
A ConfigKey() method on AuthorizerFactory (cedar->"cedar",
http->"pdp") lets the controller reconstruct the full authorizer
config and validate it via the factory registry, keeping the
controller backend-agnostic; the backends are registered through
blank imports in the operator entrypoint.
The controller validates the spec, computes a config hash, manages a
finalizer, tracks referencing workloads, and blocks deletion while
referenced. AuthzConfigRef fields are added to the three workload
specs with CEL XValidation enforcing mutual exclusivity against the
existing inline authzConfig, matching the oidcConfig/oidcConfigRef
pattern.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Add mcpauthzconfigs to RBAC E2E golden fixtures
The chainsaw operator RBAC assertions compare the live
toolhive-operator-manager-role ClusterRole against a hardcoded
expected manifest. Adding the MCPAuthzConfig CRD introduced new
mcpauthzconfigs rules in the generated ClusterRole, making the
fixtures stale and failing E2E on all kind versions.
Add the mcpauthzconfigs, mcpauthzconfigs/finalizers, and
mcpauthzconfigs/status entries in the same alphabetical position the
generated role uses (after embeddingservers*, before
mcpexternalauthconfigs*) to both the multi-tenancy and single-tenancy
setup fixtures.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Guard reserved ConfigKey values and tighten MCPAuthzConfig.Validate
Register() now panics if a factory's ConfigKey() returns one of the
reserved envelope keys ("", "version", "type"). BuildFullAuthzConfigJSON
assembles its config with a Go map literal where those two keys are
fixed metadata; a factory returning a reserved key would silently
overwrite the envelope. Catching this at startup makes the
misconfiguration impossible to ship.
MCPAuthzConfig.Validate() now consults authorizers.IsRegistered so any
caller (CLI tooling, future webhook, defense-in-depth fallback) fails
closed on an unknown spec.type rather than deferring entirely to the
controller. The unit tests blank-import the cedar backend so the
existing "cedarv1" cases continue to pass.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Canonicalize MCPAuthzConfig spec before hashing
The config hash is computed via ctrlutil.CalculateConfigHash, which walks
the spec struct including Spec.Config.Raw. RawExtension preserves the
user's raw bytes verbatim, so two semantically-equal configs that differ
only in whitespace or JSON key order produced different hashes and flipped
status on every noop kubectl-apply round-trip.
canonicalizeSpecForHash returns a copy of the spec whose Config.Raw has
been re-marshalled (Go's encoding/json sorts map keys), giving a stable
hash for the same logical config regardless of source formatting.
Malformed JSON is returned unchanged so Validate / validateAuthzConfig
remain the source of the user-facing error.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Use MutateAndPatchStatus/Spec in MCPAuthzConfig controller
Status writes now flow through controllerutil.MutateAndPatchStatus and
finalizer writes through controllerutil.MutateAndPatchSpec, per the
operator rule (.claude/rules/operator.md). The previous r.Status().Update
calls sent full PUT bodies that would clobber conditions written by any
disjoint owner of Status.Conditions on this CRD; the previous r.Update
calls had no optimistic-lock guard around finalizer arrays.
Condition and field mutations have moved inside the helper closures so
the pre-mutate snapshot reflects the live state rather than already
containing the change — a MutateAndPatchStatus prerequisite. A small
helper, setValidTrueCondition, factors out the Valid=True transition so
the success path doesn't duplicate the metav1.Condition literal.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Tighten MCPAuthzConfig reconcile flow
Collapses the previously-separate hash-change branch and steady-state
status update into a single MutateAndPatchStatus call on the success
path. Side effects of doing so:
- F4: ReferencingWorkloads / ReferenceCount now refresh in the same
reconcile that writes a new ConfigHash, so the print column doesn't
lag a workload event behind.
- F8: The success path explicitly removes the DeletionBlocked
condition. A user who cancels a deletion (e.g., by removing the
finalizer manually after observing the block) no longer carries a
stale DeletionBlocked=True forward across reconciles.
- F9: findReferencingWorkloads errors now return up to controller-
runtime instead of being logged and swallowed, so a transient
apiserver hiccup is retried with backoff rather than silently
writing an empty references list.
Two new tests exercise the F4 (one-reconcile property) and F8
(condition-clear) paths.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Unexport buildFullAuthzConfigJSON
No out-of-package caller exists for this helper today. PR #4778 will
need it once workload controllers resolve AuthzConfigRef, and the
cleaner home at that point will be cmd/thv-operator/pkg/controllerutil/
alongside BuildInlineCedarAuthzConfig. Until then keep the signature
package-private so the contract can evolve without breaking external
consumers.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Declare mcpservers RBAC marker on MCPAuthzConfig controller
findReferencingWorkloads lists MCPServer alongside VirtualMCPServer and
MCPRemoteProxy, but the kubebuilder marker only declared the latter
two. The clusterrole currently works because mcpserver_controller's
markers cover get;list;watch on mcpservers, so the rendered output is
unchanged here — but a future regeneration that reshuffles those
declarations could silently strip the permission the MCPAuthzConfig
controller actually depends on.
Verified the rendered clusterrole and chainsaw asserts are unchanged
after task operator-manifests.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Test condition preservation and last-ref finalizer transition
Two new tests for MCPAuthzConfig reconciler coverage:
- FinalizerRemovedAfterLastRefDropped: exercises the N→0 transition
in handleDeletion. The existing static-state cases cover "blocked"
and "no refs" endpoints but not the flip between them, which is the
user-observed behaviour for policy rotation.
- PreservesForeignConditions: the canary for the merge-patch
conditions-array semantic. JSON merge-patch on CRDs replaces the
conditions array wholesale (the +listType=map marker only matters to
strategic-merge-patch), so a regression that re-introduces
r.Status().Update on this resource — or that pre-mutates Status
outside a MutateAndPatchStatus closure — would erase any concurrently-
set foreign condition. This test fails in that scenario.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Add envtest for authzConfig/authzConfigRef mutual exclusion
CEL XValidation rules run at API admission, which unit tests with the
fake client cannot exercise. The three workload specs (MCPServer,
MCPRemoteProxy, VirtualMCPServer.IncomingAuth) each declare the same
mutex rule between the legacy inline authzConfig and the new
authzConfigRef — without envtest coverage the rules could silently
regress on a CRD regeneration.
For each spec, the new tests apply only-inline, only-ref, and both-set
CRs and assert (c) is rejected with the expected message. They follow
the existing pattern in cmd/thv-operator/test-integration/.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Reframe AuthzConfigRef as part 1 of 2 in godocs
The new AuthzConfigRef field on MCPServer, MCPRemoteProxy, and
VirtualMCPServer.IncomingAuth is wired into the MCPAuthzConfig
controller's reference tracking (status.referenceCount, deletion
protection) but no workload controller actually resolves the ref into a
runtime authz config in this PR. That wiring lands in a follow-up.
The previous godocs marked the inline AuthzConfig field "Deprecated:
Use AuthzConfigRef" — which pointed users at a non-functional field
and risked workloads running with no authorization enforced. Drop the
premature Deprecated annotation and add an explicit NOTE on
AuthzConfigRef so adopters know to stick with the inline form until
the consumer-side wiring lands.
The CEL mutex rule and the controller's reference tracking are
unchanged; only the descriptive godocs move.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Regenerate CRD manifests and reference docs
Picks up the F1 godoc edits: the CRD YAML descriptions and the
generated CRD API reference (docs/operator/crd-api.md) now match the
authoritative godoc on AuthzConfigRef / AuthzConfig (no premature
Deprecated annotation; explicit "consumed in a follow-up PR" note on
AuthzConfigRef).
Generated by task operator-generate + task operator-manifests +
task crdref-gen.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Use compound (kind, name) listMapKey on ReferencingWorkloads
The original review of this PR flagged that +listMapKey=name alone is
insufficient: two workloads of different kinds that share a name (an
MCPServer "foo" and a VirtualMCPServer "foo") would collide under
merge-patch semantics, with the second-applied entry silently
overwriting the first. Adding +listMapKey=kind makes the map key the
(kind, name) pair so cross-kind name reuse stays distinct.
Limited to MCPAuthzConfig here. The two sibling controllers
(MCPOIDCConfig, MCPExternalAuthConfig) share the same WorkloadReference
type and have the same listMapKey=name asymmetry — filed as a parity
follow-up rather than expanding this PR's scope.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Refresh validateAuthzConfig doc comment
The prior comment said backend validation lived in the controller "because
the API types package must not import the authorizer backends" — but the
types package now imports pkg/authz/authorizers to call IsRegistered in
Validate(). Restate the real reason: type-level Validate handles structural
+ registration checks; backend ValidateConfig requires the full
reconstructed JSON envelope that only the controller builds.
Drive-by: gitignore .pr-review/ so PR-review skill scratch dirs don't
pollute git status in worktrees.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Thread factory through buildFullAuthzConfigJSON, drop redundant lookups
buildFullAuthzConfigJSON now returns the resolved AuthorizerFactory
alongside the envelope JSON, so validateAuthzConfig can dispatch
ValidateConfig directly without a second GetFactory call and without
re-Unmarshalling the JSON it just built.
Before: Validate() ran IsRegistered, buildFullAuthzConfigJSON ran
GetFactory, validateAuthzConfig re-Unmarshalled the envelope and ran
GetFactory again — three near-identical lookups for the same key on
every reconcile, with three slightly different error messages.
The authzConfigEnvelope struct is removed; nothing else consumed it.
Tests updated to assert the returned factory's ConfigKey matches the
expected nested envelope key, locking in the bidirectional contract.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Drop marshalJSONString in favor of strconv.Quote
json.Marshal of a Go string cannot fail at runtime — encoding/json's
stringEncoder has no error path for valid Go strings. The helper existed
to placate errcheck rather than to catch a real failure mode, adding two
unreachable error branches per call site. strconv.Quote produces the
same JSON-encoded bytes with no error-checking overhead, and the call
sites become readable single expressions.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Demote hash-change log to DEBUG
Silent-success per .claude/rules/go-style.md: routine state transitions
log at DEBUG, INFO is reserved for long-running operations. A noisy
INFO on every kubectl-apply that bumps a hash is exactly the case the
rule was written to prevent.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Mark AuthzConfigRef staging NOTE with TODO(#4778)
The staging note on AuthzConfigRef becomes inaccurate the moment the
workload-controller wiring lands. A grep-able TODO(#4778) marker
in the Go source ensures whoever lands #4778 finds the stale text
without manually scanning three type files. controller-gen strips
TODO lines from the rendered CRD description, so the user-facing
schema stays clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Lock ConfigKey()/struct tag contract per backend
Factory.ConfigKey() returns a string ("cedar" / "pdp") that must match
the json tag on the backend's Config.Options field — otherwise the
controller will emit envelope JSON the backend's own Unmarshal cannot
parse. A new TestFactory_ConfigKeyMatchesStructTag per backend builds
an envelope around ConfigKey() and asserts it deserialises with
Options populated. A future rename of either string in isolation will
fail the test.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Tighten test docstrings, fixtures, and assertions
PreservesForeignConditions: docstring reframed to describe the property
it actually guards (snapshot-and-diff doesn't erase foreign conditions
during patch construction) and to be explicit about what it does NOT
catch (r.Status().Update under the fake client — requires a
WithInterceptorFuncs-backed concurrent-writer scenario).
HashAndRefsLandInOneReconcile: the MCPServer fixture now sets the
required Image field so the test remains portable to envtest. Added an
ObservedGeneration assertion so all four fields written in the single
MutateAndPatchStatus closure are pinned (previously hash, references,
referencingWorkloads.name were locked but ObservedGeneration was not).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Update cmd/thv-operator/controllers/mcpauthzconfig_controller.go
Co-authored-by: Jakub Hrozek <jakub.hrozek@posteo.se>
* Remove unused strconv import
Cascade fix from the web-UI edit that replaced strconv.Quote with
json.Marshal: the import was left in place, breaking compilation and
every downstream CI job (lint, tests, govulncheck x2, helm lint, e2e
lifecycle x3).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Co-authored-by: Jakub Hrozek <jakub.hrozek@posteo.se>1 parent 2ccfae9 commit 757a6b5
34 files changed
Lines changed: 3552 additions & 32 deletions
File tree
- cmd/thv-operator
- api
- v1alpha1
- v1beta1
- app
- controllers
- test-integration
- mcp-remote-proxy
- mcp-server
- virtualmcp
- deploy/charts
- operator-crds
- files/crds
- templates
- operator/templates/clusterrole
- docs/operator
- pkg/authz/authorizers
- cedar
- http
- test/e2e/chainsaw/operator
- multi-tenancy/setup
- single-tenancy/setup
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
50 | 50 | | |
51 | 51 | | |
52 | 52 | | |
| 53 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
126 | 126 | | |
127 | 127 | | |
128 | 128 | | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
129 | 158 | | |
130 | 159 | | |
131 | 160 | | |
| |||
402 | 431 | | |
403 | 432 | | |
404 | 433 | | |
| 434 | + | |
405 | 435 | | |
406 | 436 | | |
407 | 437 | | |
| |||
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
Lines changed: 89 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
36 | 36 | | |
37 | 37 | | |
38 | 38 | | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
39 | 43 | | |
40 | 44 | | |
41 | 45 | | |
| |||
83 | 87 | | |
84 | 88 | | |
85 | 89 | | |
86 | | - | |
| 90 | + | |
| 91 | + | |
87 | 92 | | |
88 | 93 | | |
89 | 94 | | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
90 | 110 | | |
91 | 111 | | |
92 | 112 | | |
| |||
0 commit comments