From 2e5e0b2ecd6a6cff5e730b03dc20554fcb2da8d8 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 09:35:48 +0000 Subject: [PATCH] fix(gateway)!: return error, not *StreamError, from the exported scope API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Go typed-nil trap, in a public API, in a security control. It cost the first host to adopt this ten minutes: var err error if err = policy.Validate(scope); err != nil { ... } // FIRES ON SUCCESS A nil *StreamError stored in an error interface is not nil, so a scope the gateway accepted came back as a refusal — reported as the memorable "must be one the gateway serves: ". That failure is safe only by luck. An Authorizer written the same way refuses everyone and is noticed in a minute. Invert the check and the identical bug admits everyone, silently. ScopeFromQuery and ScopePolicy.Validate now return `error`. The concrete value is still a *StreamError and the gateway still switches on it via the asStreamError/errors.As machinery that already existed; a host that wants the wire code asks for it explicitly. TestSuccessIsNilThroughTheErrorInterface pins it shut: it declares `var err error` first, so it still compiles if the signature regresses, and fails with the reporter's own message. asStreamError is also hardened against the same trap arriving from a host: errors.As matching a nil *StreamError used to hand every downstream path a nil to dereference, which is a panic in the gateway caused by a bug in a host. It is now refused as an internal error. Second fix: kube upstream errors named neither the server they dialed nor the resource path they built, so a mis-built path-prefixed URL (a kcp workspace with two /clusters/ segments) reported "the server could not find the requested resource" — indistinguishable from "that CRD is not installed". Backend could not do better: it only ever held a dynamic.Interface. NewBackendForConfig builds the client from a rest.Config and remembers the host, and the three upstream wraps now name host + path. NewBackend still works and omits the host it cannot know. adopting.md documents errors.As, and states that a path-prefixed rest.Config Host works — which was true, but only provable by trying it. BREAKING CHANGE: ScopeFromQuery and ScopePolicy.Validate return error rather than *StreamError. Callers reading .Code directly use errors.As instead. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/adopting.md | 32 ++++++++++++++++++ gateway/handler.go | 10 +++--- gateway/kube/backend.go | 55 +++++++++++++++++++++++++++++-- gateway/scope.go | 32 +++++++++++++++--- gateway/scope_conformance_test.go | 15 ++++++--- gateway/scope_policy_test.go | 37 ++++++++++++++++++++- gateway/stream.go | 9 ++++- 7 files changed, 172 insertions(+), 18 deletions(-) diff --git a/docs/adopting.md b/docs/adopting.md index 407ca05..f2fb1ac 100644 --- a/docs/adopting.md +++ b/docs/adopting.md @@ -19,6 +19,38 @@ namespaced resource. `ScopePolicy` makes the host choose explicitly. Keep all-namespaces access rare. It changes the size, disclosure risk, and operating cost of a stream. Use an `Authorizer` to pin a user to a namespace or target before any watch opens. +### Errors from the exported surface are `error` + +`ScopeFromQuery` and `ScopePolicy.Validate` return `error`, and the concrete value is a +`*StreamError`. Reach the wire code with `errors.As`: + +```go +var serr *gateway.StreamError +if errors.As(err, &serr) { + log.Printf("refused with %s: %s", serr.Code, serr.Message) +} +``` + +They return the interface rather than `*StreamError` on purpose. A fallible function returning a +concrete pointer type is the Go typed-nil trap: assign its result into a variable already declared as +`error` and a *successful* call comes back non-nil, because an interface holding a typed nil is not +nil. In a scope check that is a refusal you cannot explain. In an authorization check written the +same way, with the condition inverted, it is an admission you never see. + +## 1b. Targets, and hosts that carry a path + +A `rest.Config` whose `Host` includes a path prefix works. A kcp workspace URL +(`https://kcp.example/clusters/root:org:ws`) is an ordinary host as far as client-go is concerned, and +the dynamic client appends `/apis/...` to it correctly. Nothing in the gateway parses, rewrites or +second-guesses that URL: the host resolves a target to a backend, and the backend is whatever +`rest.Config` you built. + +Build that URL with `kube.NewBackendForConfig(cfg)` rather than `kube.NewBackend(dynamicClient)` when +you can. Both work, but only the former knows the address it dialed, so an upstream failure can name +it. That matters most exactly here: get a path-prefixed URL subtly wrong (two `/clusters/` segments, +say) and the API server answers `the server could not find the requested resource`, which is +indistinguishable from "that CRD is not installed" until the error says which URL it asked. + ## 2. Mount the same-origin cookie endpoint ```go diff --git a/gateway/handler.go b/gateway/handler.go index 535c5b4..eb2f44d 100644 --- a/gateway/handler.go +++ b/gateway/handler.go @@ -108,12 +108,12 @@ func Handler(o Options) http.Handler { return } - scope, serr := ScopeFromQuery(r.URL.Query()) - if serr == nil { - serr = o.Scopes.Validate(scope) + scope, err := ScopeFromQuery(r.URL.Query()) + if err == nil { + err = o.Scopes.Validate(scope) } - if serr != nil { - refuse(w, serr) + if err != nil { + refuse(w, asStreamError(err)) return } diff --git a/gateway/kube/backend.go b/gateway/kube/backend.go index 1d20be6..7b74d0f 100644 --- a/gateway/kube/backend.go +++ b/gateway/kube/backend.go @@ -54,6 +54,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/dynamic" + "k8s.io/client-go/rest" "k8s.io/utils/ptr" "github.com/ConfigButler/krm-stream/gateway" @@ -73,6 +74,13 @@ const initialEventsEndAnnotation = metav1.InitialEventsAnnotationKey type Backend struct { client dynamic.Interface + // host is what this backend dialed, for error messages ONLY — it is never sent to a browser and + // never parsed. An upstream error that names neither the server nor the resource path reads like + // "that resource does not exist", when what actually happened may be "you built the wrong URL". + // A kcp workspace, whose kubeconfig carries a path prefix, is exactly where the two are + // indistinguishable without this. + host string + // mu guards listThenWatch, which is a per-GroupVersion memory of "this API refused the // streaming list". It is a cache of a fact about the SERVER, not about a request, so it is // shared across every stream this Backend serves. @@ -81,6 +89,9 @@ type Backend struct { } // NewBackend wraps a dynamic client as a gateway upstream. +// +// The client alone cannot say what server it points at, so errors from this constructor name the +// resource path but not the host. Prefer NewBackendForConfig, which knows both. func NewBackend(client dynamic.Interface) *Backend { return &Backend{ client: client, @@ -88,6 +99,44 @@ func NewBackend(client dynamic.Interface) *Backend { } } +// NewBackendForConfig builds the dynamic client AND remembers the address it will dial, so that an +// upstream failure can say where it happened. +// +// Path-prefixed hosts work, and this is the constructor that makes them debuggable: a kcp workspace +// URL (https://kcp.example/clusters/root:org:ws) is an ordinary rest.Config.Host as far as client-go +// is concerned, and the dynamic client appends /apis/... to it correctly. If you build that URL +// wrongly — two /clusters/ segments, say — the API server answers "the server could not find the +// requested resource", which is indistinguishable from "that CRD is not installed" until the error +// tells you which URL it dialed. Now it does. +func NewBackendForConfig(cfg *rest.Config) (*Backend, error) { + client, err := dynamic.NewForConfig(cfg) + if err != nil { + return nil, fmt.Errorf("krm-stream/kube: dynamic client for %s: %w", cfg.Host, err) + } + b := NewBackend(client) + b.host = cfg.Host + return b, nil +} + +// upstream describes what a failing call was actually reaching for: the server, and the resource path +// built from the scope. Both halves are what a "could not find the requested resource" needs before +// it can be diagnosed rather than guessed at. +func (b *Backend) upstream(scope gateway.Scope) string { + path := "/api/" + scope.Version + if scope.Group != "" { + path = "/apis/" + scope.Group + "/" + scope.Version + } + if scope.Namespace != "" { + path += "/namespaces/" + scope.Namespace + } + path += "/" + scope.Resource + + if b.host == "" { + return path + } + return strings.TrimSuffix(b.host, "/") + path +} + var _ gateway.Backend = (*Backend)(nil) // Watch opens a snapshot-then-live stream for one scope. @@ -114,7 +163,7 @@ func (b *Backend) Watch(ctx context.Context, scope gateway.Scope) (gateway.Watch // is NOT ours to paper over. Falling back on every failure would turn "you may not watch // Secrets" into a second, differently-worded permission denial, which is how a gateway ends // up lying about why it could not open a stream. - return nil, fmt.Errorf("krm-stream/kube: streaming list for %s: %w", scope.Resource, err) + return nil, fmt.Errorf("krm-stream/kube: streaming list for %s: %w", b.upstream(scope), err) } // F6, in production. This API is aggregated (or otherwise has WatchList off); §3b is not a @@ -181,7 +230,7 @@ func (b *Backend) rememberListThenWatch(gv schema.GroupVersion) { func (b *Backend) listThenWatchStream(ctx context.Context, ri dynamic.ResourceInterface, scope gateway.Scope) (gateway.Watcher, error) { list, err := ri.List(ctx, selectors(scope)) if err != nil { - return nil, fmt.Errorf("krm-stream/kube: list %s: %w", scope.Resource, err) + return nil, fmt.Errorf("krm-stream/kube: list %s: %w", b.upstream(scope), err) } o := selectors(scope) @@ -190,7 +239,7 @@ func (b *Backend) listThenWatchStream(ctx context.Context, ri dynamic.ResourceIn w, err := ri.Watch(ctx, o) if err != nil { return nil, fmt.Errorf("krm-stream/kube: watch %s from resourceVersion %q: %w", - scope.Resource, list.GetResourceVersion(), err) + b.upstream(scope), list.GetResourceVersion(), err) } // The snapshot, in the shape §3a would have delivered it — including the boundary bookmark, diff --git a/gateway/scope.go b/gateway/scope.go index 4b60186..d9921fb 100644 --- a/gateway/scope.go +++ b/gateway/scope.go @@ -32,13 +32,15 @@ var forbiddenParams = []string{"server", "apiserver", "api-server", "url", "endp // ScopeFromQuery parses and validates a scope from a URL query, or explains why it will not. // -// The error is a *StreamError with CodeScopeInvalid and Terminal set: a browser's EventSource -// reconnects on its own, so a non-terminal rejection would have it hammer a malformed scope forever. +// It returns `error`, not `*StreamError`, and that is deliberate — see the note on ScopePolicy.Validate. +// The concrete value IS a *StreamError with CodeScopeInvalid and Terminal set (a browser's EventSource +// reconnects on its own, so a non-terminal rejection would have it hammer a malformed scope forever); +// reach it with errors.As when you need the code. // // Unknown parameters are TOLERATED (a host may carry its own — the replay server has `fixture` and // `pace`) except for the ones in forbiddenParams, which are the ones that would mean something // dangerous if we were the kind of gateway that honoured them. -func ScopeFromQuery(q url.Values) (Scope, *StreamError) { +func ScopeFromQuery(q url.Values) (Scope, error) { for _, p := range forbiddenParams { for key := range q { if strings.EqualFold(key, p) { @@ -181,7 +183,29 @@ type ScopePolicy struct { } // Validate reports whether a scope is one this gateway will stream at all. -func (p ScopePolicy) Validate(s Scope) *StreamError { +// +// # Why this returns `error` and not `*StreamError` +// +// Because returning a concrete pointer type from a fallible exported function is the Go typed-nil +// trap, and this one is a security control. A caller writing the ordinary thing: +// +// var err error // …or an err already in scope from something else +// if err = policy.Validate(scope); err != nil { +// return err // FIRES ON SUCCESS. Always. +// } +// +// gets a non-nil `error` holding a nil *StreamError, because an interface holding a typed nil is not +// nil. Validate said yes and the caller read no. That version fails safe, and it is the lucky one: an +// authorizer written the same way refuses everyone and someone notices in a minute. Invert the check — +// `if err == nil { serve() }` — and the same bug admits everyone, silently, and nobody notices at all. +// +// So the exported surface speaks `error`, which callers cannot get wrong. The concrete value is still +// a *StreamError, and the gateway still switches on it internally; a host that wants the wire code +// asks for it explicitly: +// +// var se *gateway.StreamError +// if errors.As(err, &se) { … se.Code … } +func (p ScopePolicy) Validate(s Scope) error { if s.LabelSelector != "" && !p.AllowLabelSelector { return ScopeInvalid("label selectors are not enabled for this stream endpoint") } diff --git a/gateway/scope_conformance_test.go b/gateway/scope_conformance_test.go index 7abd4aa..9d1bd21 100644 --- a/gateway/scope_conformance_test.go +++ b/gateway/scope_conformance_test.go @@ -2,6 +2,7 @@ package gateway_test import ( "encoding/json" + "errors" "net/url" "os" "testing" @@ -48,12 +49,18 @@ func TestScopeFromQueryConformance(t *testing.T) { t.Fatalf("the fixture's own query does not parse: %v", err) } - got, serr := gateway.ScopeFromQuery(q) + got, err := gateway.ScopeFromQuery(q) if c.Code != "" { - if serr == nil { + if err == nil { t.Fatalf("expected %s (%s), but the scope was ACCEPTED as %+v", c.Code, c.Because, got) } + // errors.As, because the exported surface speaks `error` — see ScopePolicy.Validate on + // why. This is the pattern a host copies to get at the wire code. + var serr *gateway.StreamError + if !errors.As(err, &serr) { + t.Fatalf("rejection was not a *StreamError: %v", err) + } if string(serr.Code) != c.Code { t.Errorf("code = %s, want %s", serr.Code, c.Code) } @@ -65,8 +72,8 @@ func TestScopeFromQueryConformance(t *testing.T) { return } - if serr != nil { - t.Fatalf("valid scope was rejected: %v (%s)", serr, c.Because) + if err != nil { + t.Fatalf("valid scope was rejected: %v (%s)", err, c.Because) } if got != c.Scope { t.Errorf("scope = %+v, want %+v", got, c.Scope) diff --git a/gateway/scope_policy_test.go b/gateway/scope_policy_test.go index 57be8dc..593ee4a 100644 --- a/gateway/scope_policy_test.go +++ b/gateway/scope_policy_test.go @@ -1,6 +1,41 @@ package gateway -import "testing" +import ( + "net/url" + "testing" +) + +// The typed-nil trap, pinned shut — in the exact shape that bit the first host to adopt this. +// +// Validate and ScopeFromQuery must return `error`, not `*StreamError`. If either ever goes back to +// the concrete pointer type, THIS TEST STILL COMPILES and starts failing, because assigning a nil +// *StreamError into an `error` produces a non-nil interface. That is the whole bug: the gateway says +// yes and the caller reads no. +// +// It fails safe here (a refusal nobody expected) and it fails safe in an authorizer written the same +// way (everyone refused, someone notices in a minute). Invert the check — `if err == nil { serve() }` +// — and the identical bug admits everyone, silently. That asymmetry is why this is a test and not a +// doc comment. +func TestSuccessIsNilThroughTheErrorInterface(t *testing.T) { + policy := ScopePolicy{ + Targets: []string{"demo"}, + Resources: []GroupResource{{Resource: "configmaps", Scope: ResourceScopeNamespaced}}, + } + scope := Scope{Target: "demo", Version: "v1", Resource: "configmaps", Namespace: "app"} + + // Declared as `error` FIRST, then assigned — which is what a host does, and what makes a concrete + // return type dangerous. `err := policy.Validate(...)` would hide this by inferring *StreamError. + var err error + + if err = policy.Validate(scope); err != nil { + t.Fatalf("a valid scope came back non-nil through an error interface: %v — the typed-nil trap is back", err) + } + + q := url.Values{"target": {"demo"}, "version": {"v1"}, "resource": {"configmaps"}, "namespace": {"app"}} + if _, err = ScopeFromQuery(q); err != nil { + t.Fatalf("a valid query came back non-nil through an error interface: %v — the typed-nil trap is back", err) + } +} func TestScopePolicyRequiresExplicitNamespaceSemantics(t *testing.T) { policy := ScopePolicy{ diff --git a/gateway/stream.go b/gateway/stream.go index 08fed48..a6b09cb 100644 --- a/gateway/stream.go +++ b/gateway/stream.go @@ -497,9 +497,16 @@ func isDecimalResourceVersion(rv string) bool { return true } +// asStreamError finds the *StreamError a host meant to send, or makes one. +// +// The `se == nil` arm is the typed-nil trap seen from the other side. A host whose Authorizer is +// declared to return `error` and returns a nil *StreamError hands us a non-nil error interface +// wrapping nothing: errors.As matches, sets se to nil, and every path downstream would dereference it +// to build the event. That is a panic in the gateway caused by a bug in a host — so it is refused as +// an internal error instead, which is the honest description and does not take the process down. func asStreamError(err error) *StreamError { var se *StreamError - if !errors.As(err, &se) { + if !errors.As(err, &se) || se == nil { return &StreamError{Code: CodeInternal, Message: err.Error(), Terminal: true} } return se