From 5a11cc85be8d4d2a9796bf9b2b83976e19c4f682 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 20 Aug 2026 14:30:32 +0100 Subject: [PATCH] docs: correct security claims, pipeline diagram, k8s example completeness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from review, verified against source before rewriting: - README.md Security notes: "auth events are logged" was false — grepped auth.go, zero logging calls exist anywhere in it. A lookup miss is a plain 401 envelope with no log line at all, not "logged without key material". Also corrected the adjacent providerHTTPError claim: the struct retains the upstream response body as a field (surfaced deliberately to the client under error.upstream); only its Error() string omits the body, and nothing in this codebase currently logs a providerHTTPError via a bare %v anyway. - README.md mermaid diagram: the previous version showed a single global Auth gate ahead of routing. ServeHTTP (llmgateway.go) actually dispatches on path+method first and calls identify() separately inside each recognized-route branch; an unrecognized route (passthroughUnknown/404 fallback) never calls identify() at all. Redrawn Route -> per-branch Auth, with the unauthenticated fallback branching directly off Route. Also fixed the 401 node's error type label from the nonexistent "invalid_api_key" to the actual envelope type, "authentication_error" (llmgateway.go, routes_unified.go). Re-rendered with mmdc before and after the fix. - README.md + examples/kubernetes.yaml: narrowed the bare-{} empty-group claim. Re-verified this session with an isolated, minimal repro (Traefik file provider only, no docker-compose configs: mechanism involved) that a bare `default: {}` still fails plugin construction with the file provider specifically: "expected a map, got 'string'". Scoped the claim to what is actually verified (file provider, twice) and explicitly flagged Docker/ECS label providers and the Kubernetes CRD provider as untested, rather than asserting the file provider is unaffected or that the CRD provider shares its decode path — the latter was an unverified inference in the prior text, now hedged. - examples/kubernetes.yaml: the IngressRoute referenced an undefined Service (llmgateway-dummy-backend), so the manifest did not actually apply as advertised. Added the missing Deployment + Service (traefik/ whoami, the same image already exercised end-to-end in examples/docker-compose.yml this session). Re-validated: 6-document YAML parses, Middleware's plugin config still round-trips through the real Config struct and New() (throwaway test, deleted after). - README.md Known limitations: dropped the standalone path-traversal bullet — it duplicated content already stated in the "request rejected before it reaches a provider" bullet immediately above it. NOTE for the reviewer: the ruling on the bare-{} finding stated "file provider observed working" and named Docker/ECS label providers as the affected ones. I re-tested this directly (twice, including a standalone repro outside the compose demo) and the file provider fails on a bare {} exactly as originally reported — I could not reproduce "file provider unaffected" and have not tested the label providers at all. Narrowed the doc wording to only what I verified rather than asserting either provider's behaviour without evidence; flagging the discrepancy in case the review had independent grounds I don't have visibility into. Gates: go test ./... -count=1, gofmt -l ., go vet ./..., make yaegi-check — all clean. Also re-ran go test -race, golangci-lint (incl. gosec), and fieldalignment, matching the standard this task already set; all clean. --- README.md | 66 ++++++++++++++++++++++++---------------- examples/kubernetes.yaml | 60 +++++++++++++++++++++++++++++------- 2 files changed, 89 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index 42bbaab..a20d53c 100644 --- a/README.md +++ b/README.md @@ -34,13 +34,16 @@ Catalog — see [Development](#development) for what that needs. ```mermaid flowchart LR - Req["incoming request"] --> Auth["auth
identify(): key digest lookup"] - Auth -->|"no match"| E401["401 invalid_api_key"] - Auth -->|"user, group"| Route["route
ServeHTTP dispatch"] - Route --> Unified["unified
/v1/chat/completions
/v1/embeddings
/v1/models"] - Route --> MCP["MCP/A2A proxy
/mcp/name/...
/a2a/name/..."] - Route --> Passthrough["native passthrough
/providerName/..."] - Route -->|"no route matches"| Fallback["passthroughUnknown ? next : 404"] + Req["incoming request"] --> Route{"route
ServeHTTP: path+method dispatch"} + Route -->|"a recognized route:
/v1/models, /v1/chat/completions,
/v1/embeddings, /v1/mcp/servers,
/v1/agents, /mcp/name/...,
/a2a/name/..., or /providerName/...
for a known provider"| Auth["auth (per branch)
identify(): key digest lookup"] + Route -->|"no route matches"| Fallback["passthroughUnknown ? next : 404
never calls identify()"] + Auth -->|"no match"| E401["401 authentication_error"] + Auth -->|"user, group"| Branch{"branch"} + Branch --> Models["GET /v1/models"] + Branch --> Unified["POST /v1/chat/completions
POST /v1/embeddings"] + Branch --> Registry["GET /v1/mcp/servers, /v1/agents"] + Branch --> MCP["MCP/A2A proxy"] + Branch --> Passthrough["native passthrough"] Unified --> Resolve["resolve model
registry.resolve"] Resolve -->|"unknown/denied"| E404["404 / 403"] Resolve --> Limit1["limiter.checkAndCount"] @@ -55,6 +58,8 @@ flowchart LR Exec --> Account["account: usage, cost"] Account --> Resp["respond to client"] ExecProxy --> Resp + Models --> Resp + Registry --> Resp ``` Every route the plugin recognises is handled entirely inside the @@ -120,11 +125,14 @@ http: > Write an "allow everything" group as `providers: []` (an explicit empty > list), not a bare `{}`. See the note at the top of -> [`examples/kubernetes.yaml`](examples/kubernetes.yaml) — an empty nested -> object can fail to round-trip through Traefik's own dynamic-config -> decoder with `expected a map, got 'string'`; an explicit empty list -> means the same thing (all values allowed) and decodes correctly. Verified -> against this exact plugin build in `examples/docker-compose.yml`. +> [`examples/kubernetes.yaml`](examples/kubernetes.yaml) — confirmed +> against Traefik's **file provider**, twice, with this exact plugin build: +> a bare `default: {}` fails plugin construction with +> `expected a map, got 'string'`; `default: {providers: []}` loads cleanly. +> Not tested against the Docker/ECS label providers or the Kubernetes CRD +> provider — no claim either way for those. An explicit empty list means +> the same thing as an empty object (all values allowed), so writing it +> this way costs nothing even where the bug does not apply. Attach the middleware to a router the same way as any other Traefik middleware. The router's backing service is never actually reached unless @@ -406,11 +414,17 @@ kept — a bad edit to the file never breaks already-authenticated traffic. stripped before any upstream or target request and never reaches a provider, MCP server, or A2A agent. - Neither kind of key is ever written to a log line — the logging helpers - (`logger.go`) carry an explicit "never log key material" contract, and - error types (`providerHTTPError`, etc.) deliberately omit request/response - bodies that could carry one from ending up in a log line by accident. -- Auth events are logged without key material: a lookup miss is a plain - 401 with no indication of which key was tried. + (`logger.go`) carry an explicit "never log key material" contract. + `providerHTTPError`'s `Error()` string (`providers.go`) separately omits + the upstream response body it still holds as a struct field, so a future + caller that ever formats the error bare (`%v`) cannot accidentally echo + provider response content into a log line; today's only caller + (`writeProviderUpstreamError`) reads that body directly and puts it in + the client-facing response, never in a log. +- **There is no auth-event logging at all** — `auth.go` has zero logging + calls of any kind. A key that fails lookup gets a plain 401 JSON + envelope and nothing else happens; there is no log line, successful or + failed, for a request to appear in. ## Known limitations @@ -452,19 +466,19 @@ kept — a bad edit to the file never breaks already-authenticated traffic. Go's `Transport` auto-negotiates; an upstream that used a different encoding on its own initiative (never observed against OpenAI, Anthropic, or Gemini) would pass through unmodified instead. -- Malformed-path rejects (a `..` traversal segment in a passthrough or - MCP/A2A rest-path) are checked, and rejected, before the rate limiter - runs — they never consume quota. ## Kubernetes See [`examples/kubernetes.yaml`](examples/kubernetes.yaml) for a complete, -YAML-validated example manifest: a `Secret` holding the users file, a -`Middleware` custom resource carrying the plugin's own configuration -(validated against the real `Config` struct — every field in that example -round-trips through `New()` cleanly), an `IngressRoute` wiring it to a -host, and a Traefik Helm values snippet for loading the plugin and wiring -provider keys in from Secrets via environment variables. +YAML-validated example manifest that `kubectl apply -f` runs as-is: a +`Secret` holding the users file, a `Middleware` custom resource carrying +the plugin's own configuration (validated against the real `Config` +struct — every field in that example round-trips through `New()` +cleanly), a dummy backing `Deployment`/`Service` (Traefik requires a real +router backend even though this middleware never actually reaches it — +see [Quickstart](#quickstart)), an `IngressRoute` wiring it all to a +host, and a Traefik Helm values snippet for loading the plugin and +wiring provider keys in from Secrets via environment variables. The short version: diff --git a/examples/kubernetes.yaml b/examples/kubernetes.yaml index 13b23e2..6acc4f7 100644 --- a/examples/kubernetes.yaml +++ b/examples/kubernetes.yaml @@ -10,12 +10,15 @@ # see the Helm values snippet at the bottom of this file. # # The empty-map gotcha: write an empty group as `providers: []`, never a -# bare `{}`. Traefik's own dynamic-config decode pipeline (which the -# Kubernetes CRD provider funnels plugin config through, same as the file -# provider) can fail to round-trip a truly empty nested object with -# "expected a map, got 'string'" — verified against this exact shape in -# examples/docker-compose.yml. An explicit empty list decodes correctly -# and means the same thing (all values allowed). +# bare `{}`. Confirmed against Traefik's file provider, twice, with this +# exact plugin build (examples/docker-compose.yml, plus an isolated +# minimal repro outside that demo's compose-specific config mechanism): a +# bare `default: {}` fails plugin construction with +# "expected a map, got 'string'"; an explicit empty list decodes +# correctly and means the same thing (all values allowed). Whether the +# Kubernetes CRD provider used below hits the same decode path as the +# file provider is NOT verified — this file uses the empty-list form +# defensively, not because the CRD path is confirmed affected. apiVersion: v1 kind: Secret @@ -97,6 +100,46 @@ spec: url: http://summarizer.agents.svc:8080 passthroughUnknown: false +--- +# Traefik requires a real backing Service for a router even though this +# middleware short-circuits every path it recognizes; whoami (the same +# image already exercised in ../examples/docker-compose.yml) is a small, +# always-up dummy, only ever reached via passthroughUnknown (default +# false, which 404s instead). Any always-up internal service works here. +apiVersion: apps/v1 +kind: Deployment +metadata: + name: llmgateway-dummy-backend + namespace: traefik +spec: + replicas: 1 + selector: + matchLabels: + app: llmgateway-dummy-backend + template: + metadata: + labels: + app: llmgateway-dummy-backend + spec: + containers: + - name: whoami + image: traefik/whoami:v1.10 + ports: + - containerPort: 80 + +--- +apiVersion: v1 +kind: Service +metadata: + name: llmgateway-dummy-backend + namespace: traefik +spec: + selector: + app: llmgateway-dummy-backend + ports: + - port: 80 + targetPort: 80 + --- apiVersion: traefik.io/v1alpha1 kind: IngressRoute @@ -111,11 +154,6 @@ spec: kind: Rule middlewares: - name: llmgateway - # Traefik requires a real backing Service for a router even though - # this middleware short-circuits every path it recognizes; a small - # dummy such as traefik/whoami (see ../examples/docker-compose.yml) - # or any always-up internal service works, since it is only ever - # reached via passthroughUnknown (default false, which 404s instead). services: - name: llmgateway-dummy-backend port: 80