Topic/review findings - #3
Merged
Merged
Conversation
…y (C-3) The package-wide credential cache held both the participant access token and the provider self-description derived from it, but was keyed only on `baseURL + "|" + tokenAudience` — and TokenAudience defaults to the constant "consent-manager" for every route. Two routes fronting different provider tenants against the same consent-manager therefore collapsed onto one cache entry. Whichever route warmed it first installed its token and its selfDescriptionURL; the other then ran its identifier search scoped to the wrong provider and its consents lookup as the wrong participant. Decisions were silently wrong in both directions: denials for subjects who had consented, and allows against another provider's consent records. cacheKey() now covers every input that can change the cached token or SD: base URL, Host override, API prefix, token audience, token-service URL, provider SD, and the static token and consent key (hashed with SHA-256, so the key never retains a secret verbatim). Components are joined with a NUL separator, which cannot occur in a URL or audience name, so no two distinct identities can collide on the joined string. Tests: a table-driven case asserting a distinct key for each differing field, and an end-to-end regression test with two token services against one consent-manager asserting each participant presents its own token. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…olver (C-1) When `owner_resolver_url` was unset — the documented default — the plugin took the consent subject from the access token's `sub` claim. That answers "has the CALLER granted some consent?", which establishes no link whatsoever between the caller and the data the upstream is about to return. Concretely: Alice holds one granted consent and a valid token. She requests /ngsi-ld/v1/entities/urn:ngsi-ld:PersonalProfile:bob. The plugin resolved *Alice's* user identifier, found *Alice's* granted consent, and returned *Bob's* personal data. Any subject with a single granted consent was a universal reader, so the gate was a no-op against the threat it exists for. The JWT signature is also not verified by this plugin, so on a route without an auth plugin the `sub` was attacker-supplied and the check collapsed entirely. The owner-resolver path already answers the right question — it derives ownership from the response DATA and checks consent per resolved owner — so the fix is to finish that migration rather than patch the unsound path: - `owner_resolver_url` is now required; `Validate()` rejects a config without one, so a route that cannot determine ownership fails to load instead of silently gating nothing. - `buildConsentRequest`, the legacy `checkConsent` helper and the `jwtSubjectClaim` constant are deleted; `evaluate()` always goes through the resolver. - `jwt_claims_to_forward` is documented for what it now does (naming the consumer for the contract lookup), not as the source of the data subject. - `RequestFilter` documents that the JWT is decoded, never verified, and that an authenticating plugin in front of this one is a hard route prerequisite. BREAKING CHANGE: routes without `owner_resolver_url` no longer load. Tests: the plugin and integration suites are ported to resolver mode, and TestResponseFilter_OwnerNotRequestor / TestIntegration_OwnerNotRequestor pin the property directly — the resolver names Bob as the owner while the token's `sub` is Alice, and the identifier search must ask about Bob. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d purpose (C-2) hasGrantedConsent filtered the consent list on `status` alone (plus, optionally, `data[].resource`). It never checked WHO the consent was granted to, even though the plugin had already resolved the consumer's self-description URL for the resolver call and was holding it. So: Bob grants consent to participant X for the purpose "insurance quote". Participant Y — a different consumer, with no consent from Bob — requests Bob's data through the gateway. The plugin saw Bob's granted consent to X and let Y through. Under GDPR terms it authorised a processing purpose the subject never agreed to, on the evidence of an entirely different agreement. A consent is now matched only when all of these hold: - status is "granted"; - it was granted to the consuming participant named in the request; - it covers the purpose, when the resolver could name one; - it covers the data resource, when the check is resource-scoped. Supporting changes: - `ConsentRequest` gains `Consumer` (required) and `Purpose` (optional). `CheckConsent` denies outright when no consumer is identified, rather than falling back to "does this subject have any consent at all?". - The receipt is decoded into a `consentRecord` that reads the consumer from either `consumer` or `dataConsumer`, in either the bare-string or the embedded object shape, matching on selfDescriptionURL / _id / did. A record that names no consumer matches nothing — the plugin cannot tell whose agreement it is, and guessing is what the finding is about. - `ownerresolver.Claim` gains `purpose`, so the resolver can name the contract purpose governing a claim; absent, only the consumer match applies. - Deny reasons now say which scope failed, so the audit log distinguishes "consented to someone else" from "did not consent to this resource". Tests: consumer-scoping cases in the client decision matrix (granted to another consumer, consent naming no consumer, request without a consumer) and a plugin-level regression test proving the wiring end to end. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… (H-1)
Both party lookups in evaluateWithResolver logged their error and carried on:
if consumerSD, sdErr := ...; sdErr != nil { log.Printf(...) } else { ... }
if providerSD, sdErr := ...; sdErr != nil { log.Printf(...) } else { ... }
Both failing left `resolveParties` empty, `Parties.IsZero()` then omitted the
field from the /resolve request entirely, and the plugin went on to trust
whatever came back — including `consentRequired: false`, which is an
unconditional allow. A fail-closed design with a fail-open seam in the middle
of it, reachable by nothing more dramatic than a briefly unreachable
consent-manager or a revoked participant token.
Both lookups now run before the resolver is asked anything, and either failing
is terminal — the fail policy applies instead of an unidentified-party
/resolve result reaching the allow branch. A token that names no consuming
participant at all is treated the same way. No degraded mode is offered: with
the consumer now scoping the consent match itself (C-2), a check without one
could not be sound anyway.
`ParticipantSelfDescriptionByDID` also gains what only `CheckConsent` had:
- a 401 refresh-and-retry, so a cached token that has since been revoked does
not make the party mapping — and with it the whole exchange — fail
terminally;
- negative caching of "no such participant" (30s), so one misconfigured DID no
longer re-fetches the entire participant list on every request;
- a cache key that includes the full credential identity rather than just the
base URL, matching the C-3 fix.
Tests: the 401 retry and the negative caching in the consent package, and two
plugin cases asserting an unresolvable consumer and a token with no consumer
claim both deny without the resolver being contacted at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uration (H-2) `fail_open` defaulted to true, so every unresolved situation — consent-manager down, resolver erroring, missing request context, missing credentials, unreadable body — became ALLOW unless the operator explicitly opted out. For an availability filter that default is defensible; for a consent gate on personal data it inverts the safe default, and it is reached by omission. Two changes: 1. `IsFailOpen()` now returns false when unset. `fail_open: true` remains available as a deliberate opt-out, and `ParseConfig` logs a warning when it is enabled so the choice is visible in the runner's output. 2. Not every unresolved situation is an outage, so `failOutcome` now takes a `failMode`. `failByPolicy` (resolver/consent-manager/body-read failures) is what `fail_open` governs. `failAlwaysClosed` denies regardless: no correlation id, no request context, no participant credentials at all, and "consent required but no owner resolved". None of those are transient conditions to ride out — failing them open turns a lost request or a typo in the route config into a silent, total bypass of the gate, which is exactly how this finding composes with the documentation drift. Missing credentials are recognised through a new `consent.ErrNoCredentials` sentinel rather than by string matching. BREAKING CHANGE: routes that relied on the implicit fail-open default now deny on a dependency failure. Set `"fail_open": true` explicitly to keep the old behaviour. Tests: each fail-open case is split into a default (deny) and an explicit opt-in (pass through) case, plus two cases proving that a missing request context and missing participant credentials deny even with fail_open enabled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r tokens (H-3) The store bridging the request and response phases was a package-level sync.Map written in RequestFilter and deleted only by the response phase. It had no TTL, no size cap, no eviction sweep and no gauge. Any request whose response phase never runs leaked one entry permanently: the client disconnects before the upstream answers, the upstream connect times out, an earlier APISIX plugin short-circuits after pre-req, ext-plugin-post-resp is missing from one of several routes, the runner restarts between phases. The runner is a long-lived process, so the map grew monotonically until OOM — and it was remotely drivable: open connections, send the request, abort before the response, and you have an unauthenticated memory-exhaustion primitive. Each leaked entry also held `RequestContext.Headers`, a copy of every request header including `Authorization: Bearer <jwt>`. Nothing ever read it — the only consumer was `len(rc.Headers)` in String() — so the leak was a leak of credentials retained indefinitely, for no benefit at all. - `RequestContext.Headers` and its capture loop are gone. Only method, path and the decoded claims are kept, which is all the response phase uses. - Entries carry the time they were stored. A background janitor sweeps entries older than RequestContextTTL (60s) every 10s, and a load that finds an expired entry reports it as absent, so a stale context can never decide a fresh request. - MaxRequestContexts (100k) caps the store. On overflow it sweeps expired entries first and, if everything is live, evicts the oldest — a leak from an older request never refuses service to a new one. - `RequestContextStoreSize()` and `RequestContextsEvicted()` expose the leak: the size tracks in-flight gated requests and returns to zero when idle, and a climbing eviction count means requests are being lost or the store driven deliberately. - The unused exported `LoadRequestContext` / `DeleteRequestContext` are removed (L-1); tests assert cleanup through the size gauge instead. Tests cover expiry-on-load, the janitor sweep leaving live entries alone, cap enforcement evicting the oldest, and a guard that RequestContext holds no headers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ft in CI (C-4)
The README documented a configuration surface that had been removed from the
code months earlier:
documented reality
client_id / client_secret removed from Config
CONSENT_CLIENT_ID/_SECRET env vars are CONSENT_KEY,
CONSENT_TOKEN_SERVICE_URL,
CONSENT_AUDIT_OTLP_ENDPOINT
POST /participants/login fetchToken posts to token_service_url
— token_service_url, token_audience,
owner_resolver_url, owner_resolver_timeout,
service, consumer_claim, consent_api_host
all undocumented
Because `json.Unmarshal` silently discards unknown fields, an operator copying
the README's curl example got a config that parsed and validated cleanly, then
could not authenticate as the participant at all. Under the previously
documented `fail_open: true` default that meant every request was allowed, the
gate entirely bypassed, with nothing to signal it but one log line per request.
A documentation defect here is a security defect.
- The README's configuration table is rewritten field by field against
config.go, the route example is a working `jq`/`curl` invocation carrying the
same conf to both phases, and the two-call section describes the actual
endpoints, the consumer/purpose scoping and the token-service flow.
- New sections state what was implicit and load-bearing: ownership comes from
the data and never from the requestor, the JWT is decoded but not verified so
an auth plugin in front is a hard prerequisite, and ext-plugin-post-resp
buffers the whole upstream response.
- CLAUDE.md is refreshed: it described an `internal/filter` package that does
not exist, omitted `internal/audit` and `internal/ownerresolver`, and still
described the removed client-credentials login.
- `TestREADMEDocumentsEveryConfigField` parses the json tags off the Config AST
and the README's configuration table and fails if either side has a field the
other lacks. Drift on a security control needs a machine, not discipline —
and it runs in `make test`, so CI already enforces it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nt registry (H-1/H-2) Follow-up to the fail-mode classification. A consumer DID that the participant registry does not know was mapped to failByPolicy, so with `fail_open: true` it released the data. But "not registered" is not an outage: it is a permanent condition, and retrying will not change it. Failing it open does not ride out a blip — it grants that consumer standing, indefinite access to personal data through a gate that is nominally enabled. `ParticipantSelfDescriptionByDID` now wraps a new `consent.ErrParticipantNotRegistered` sentinel, and `failModeForError` treats it like `ErrNoCredentials`: always closed, regardless of `fail_open`. An unreachable or erroring registry stays under the operator's policy, which is the genuine availability case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The resolver path — the sound one, the one every other critical finding lives in — had 0% coverage. `evaluateWithResolver`, `consumerFromClaims`, `resourceOrPath`, `ParticipantSelfDescriptionByDID`, `decodeParticipants` and `ProviderSelfDescription` were all measured at 0.0%, while the legacy mode that has now been deleted was the one with eleven end-to-end cases behind it. Coverage was also mis-measured. `make test-cover` and tests.yml both omitted `-coverpkg=./...`, so the integration package's coverage of internal/plugin was discarded outright — which understated the real number and, worse, made the genuine 0% functions look like measurement noise. And the profile was uploaded as an artifact and never asserted, so nothing would have noticed the gap. - `internal/integration` gains a resolver-mode harness: a mock `/resolve` and a per-owner consent-manager, driving multi-owner deny_all, an owner unknown to the consent-manager, `consentRequired: false`, empty claims, a claim with an empty ownerId, resolver 5xx under both fail policies, and owner dedup. - Two further integration tests pin what the plugin actually tells the resolver (resource descriptor, mapped contract parties, JSON payload) and the H-1 fail-closed seam (an unidentified consumer never reaches the resolver and denies even with fail_open). - `consent.ResetCaches()` gives tests a reset hook for the package-wide token and participant-SD caches. They passed only because httptest happens to allocate a fresh base URL per server; a reused URL or `t.Parallel()` would have produced order-dependent flakes. - Unit tests for `claimKeysToDecode` and `decodeParticipants` (both registry shapes), and `TestConfig_IsFailOpen` moves to config_test.go where the rest of the config tests live. - `-coverpkg=./...` in `make test-cover` and tests.yml, plus `hack/coverage-floor.sh` and a `coverage-floor` target enforcing an 80% floor in both CI pipelines. Total coverage: 69.1% -> 85.7%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The loop over the resolver's claims ran one full two-call consent check per
distinct (owner, dataResource), sequentially, each on context.Background() with
its own per-call timeout.
A collection endpoint returning 200 entities with 200 distinct owners therefore
issued ~400 sequential HTTP calls; at the default 5s per-call timeout the worst
case was ~2000s of held-open response while APISIX buffered the body. The client
and APISIX give up long before that, but the runner's goroutine keeps working
and its connections stay open, so a handful of such requests saturate both the
runner and the consent-manager. Any caller who can reach a list endpoint could
trigger it.
Four changes, one per limb of the problem:
(a) One `context.WithTimeout` for the whole response phase, derived once and
passed to the party lookups, the resolve call and every consent check. The
total time APISIX holds the response is now bounded regardless of owner
count, and a cancelled phase propagates instead of leaving work running.
New `response_phase_timeout` (default 10000ms, range 1-120000).
(b) `max_owners_per_response` (default 50, range 1-1000) caps the distinct
owners checked. Above it the response is denied — a deliberate limit, so it
denies even under `fail_open`.
(c) The per-owner checks run concurrently, at most 8 in flight, and cancel the
rest on the first denial or error, since nothing the others could return
would change a deny_all verdict. The reported outcome is always the
lowest-indexed problem, so the decision and its audit record do not depend
on which goroutine won the race.
(d) The subject -> userIdentifier mapping is memoised for 60s, so one owner
appearing under several data resources is resolved once rather than once per
resource. Only positive results are cached: a subject can register at any
moment, and remembering "unknown" would keep denying them after they had
consented.
Claim dedup moves into `distinctClaims`, which also turns a claim with no owner
into an error rather than an inline early return.
Tests: the owner cap denying even with fail_open, the phase deadline bounding a
consent-manager that never answers, `distinctClaims` as a table, and the
identifier memo (reused for a second resource, never reused for an unknown
subject).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
denyResponse set Content-Type and the status, wrote the deny body, and touched nothing else — so every other upstream response header survived into the 403. That is a side channel straight around the gate. A caller who was just refused the data still learned that the entity exists and which version it is (ETag / Last-Modified), how many records matched (X-Total-Count, NGSILD-Results-Count), that more pages follow (Link), and was handed whatever Set-Cookie the upstream issued. Content-Encoding was worse than a leak: an upstream that answered `gzip` left the client trying to inflate a plain-JSON deny body, and Content-Length still advertised the upstream body's size. The denial now removes every upstream header except the `Access-Control-*` family — CORS headers describe the exchange rather than the resource, and dropping them would show a browser client a CORS error instead of the 403 it was actually given — then sets Content-Type and a Content-Length computed from the deny body itself. The test mocks also stopped diverging from the runner in ways that hid this: `mockHeader.View()` now returns the live map as the runner's does (so a caller iterating and deleting behaves as in production), and `mockResponse.Write` appends into a buffer rather than replacing, so a double-write regression would now be caught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ss the API (M-2)
`Validate()` checked URLs, one timeout and the status-code range, but never that
the route had any way to authenticate as the participant. The README even
documented this as intentional ("None are enforced at parse time").
So a route with neither `token_service_url` nor `participant_token` loaded
cleanly and only failed per request, on the data path, as one log line saying
"no participant_token and no token_service_url configured". Combined with the
old fail-open default that was a route which loaded successfully and allowed
everything. A config that cannot complete a single check is not a config worth
loading — it is now rejected at parse time.
Two more unchecked fields in the same function:
- `owner_resolver_timeout` was defaulted but never range-checked, unlike
`consent_api_timeout`. Now bounded to 1–60000ms.
- `consent_api_prefix` was concatenated into every endpoint URL unchecked, so a
prefix without a leading `/` silently produced a malformed URL and every call
failed as a confusing 404. It must now start with `/`, and a trailing `/` is
trimmed in applyDefaults so it cannot produce a double slash either.
The required-field checks also move ahead of the numeric range checks, so a
config missing `owner_resolver_url` reports that rather than complaining about a
timeout it never got the chance to default.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rt (M-3) The audit trail could not answer the question an access-decision log exists to answer, and lost records on every redeploy. - **It named the wrong owners.** In resolver mode only the *first denying* owner was recorded, and an allow recorded no owner at all — so the log said whether a response was released but not whose consent was consulted or what each said. `responseOutcome` now carries a `checked` entry per consulted owner and `recordAudit` emits one record for each. A request that failed before any owner was reached is still recorded once, as itself. - **The emitter cache ignored half its configuration.** `Get` keyed on `endpoint|serviceName`, so the first route to create an emitter silently imposed its timeout on every other route sharing that Collector. The key is now the full `Config` (endpoint, service name, timeout, headers — header order normalised). - **Nothing was flushed at shutdown.** `Shutdown` existed and was tested but `main()` never called it, so each restart discarded up to one flush interval (2s) of decisions. `audit.ShutdownAll()` is now wired to SIGTERM/SIGINT. - **Upstream error bodies reached the sink.** Reasons are built by wrapping dependency errors, and those embed the consent-manager's response body, which can carry identifiers. `SanitizeReason` (applied inside `Emit`, so no call site can bypass it) collapses control characters and bounds the length. - **Dropped records were invisible.** The bounded queue drops rather than blocking the request path — correct — but that means an attacker who can generate load can suppress the record of their own access. `Emitter.Dropped()` and package-level `Dropped()` expose the count instead of only logging every hundredth drop. - **No way to authenticate to the Collector.** New optional `audit_otlp_headers` config carries extra headers on every export. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`consumerFromClaims` walked a dotted path through `map[string]interface{}` only.
The default path is `verifiableCredential.issuer`, but a Verifiable Presentation
routinely carries `verifiableCredential` as a JSON **array** — so on an ordinary
VP token the type assertion failed, "" came back, and the function logged
nothing. The consumer was simply absent, which now (post H-1) denies every such
request with no indication of why, and previously fed the unidentified-party
allow seam.
- A bare segment landing on an array traverses its first element, so the default
path works on the common shape without any configuration.
- Explicit indexing is supported — `verifiableCredential[0].issuer`, including
repeated indices for nested arrays (`a[0][0].b`).
- The function returns an error rather than "", naming the segment that failed
and distinguishing "no path configured" (`errClaimPathUnset`) from "the
configured path did not resolve". A mistyped path is now diagnosable from the
log line instead of looking like a consumer that simply is not there.
- `claimKeysToDecode` uses a shared `claimPathRoot` so an indexed first segment
still resolves to the top-level claim the request phase must decode.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Roughly twenty `log.Printf` calls with a hand-written "[consent-filter]" prefix were spread across plugin/ and audit/. Three consequences, all on the request path: - The runner ships its own zap logger and configures its level from the runner's environment, so these lines bypassed it entirely — an operator could not raise or lower the plugin's verbosity, or suppress it, at all. - Messages carried subject and participant DIDs and upstream error bodies: personal data on stdout, with no retention policy, which is exactly what the OTLP audit path was built to avoid. - Nothing was rate-limited, so a broken consent-manager produced one line per request. A new `internal/logging` package addresses all three and every call site is converted: - `Debugf`/`Infof`/`Warnf`/`Errorf` delegate to the runner's logger, so the runner's level configuration applies and each line has an appropriate level instead of everything being an undifferentiated print. - `Redact` turns an identifier into a stable 8-hex-character fingerprint — enough to correlate lines about the same subject while debugging, not enough to be a handle on the subject. The consent client's "no participant registered" error now carries a fingerprint rather than the DID, since that error is both logged and used as an audit reason. - `Sanitize` collapses control characters and bounds length, so an HTML or JSON error page from a dependency cannot be splatted across the log. The audit package's `SanitizeReason` now delegates to it rather than duplicating it. - `WarnfEvery`/`ErrorfEvery` collapse a repeated failure to one line per 10s per key and report how many occurrences were suppressed, so the rate limiting is never silent about itself. The key set is bounded so it cannot grow into a leak. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The documented dev workflow could not start: - `docker-compose.yaml` mounted `./apisix-config.yaml`, which was not in the repository. Docker creates a *directory* at a missing bind-mount path, so APISIX started with a directory where its config should be and failed to parse it. - Both services bind-mounted `/tmp/runner.sock` — a socket file that does not exist at compose time, so Docker created a directory there too and the runner could not bind. The socket has to live inside a shared *directory*; a named `runner-socket` volume mounted at `/opt/runner` now provides one. - `version: "3.8"` is obsolete under Compose v2. Since README advertised `docker compose up --build` as the dev workflow, this was the first thing a new contributor hit. The stack is now self-contained and exercisable rather than merely startable: - `dev/apisix-config.yaml` wires `ext-plugin.path_for_test` at the shared socket and enables both external-plugin phases. - `mock` (WireMock, stubs in `dev/mocks/`) stands in for the consent-manager, the OwnerResolver and the participant token service — including the consumer scoping, so editing `consents.json` flips the gate's answer. - `upstream` is an echo service so the resolver has a real payload to be asked about, and `otel-collector` receives the access-decision audit log with the routing that `service.name=consent-access-audit` exists for. - Credentials reach the runner through env vars, mirroring how the Kubernetes deployment sources them from a Secret. The README section now gives the route-creation and request commands end to end. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`security-analysis.yml` ran both `govulncheck` and `gosec` with `continue-on-error: true`, so their findings were informational only and a known-vulnerable dependency merged cleanly. A security gate that cannot fail is a dashboard, not a gate — both now block the pipeline, and the SARIF upload still populates the Security tab either way. Versions were also unpinned. `gosec` was installed from `@latest` and `golangci-lint-action` used `version: latest`, which made CI non-reproducible (a scanner release could redden an untouched PR) and put an unpinned binary inside the build's trust boundary. Worse, the Gitea pipeline pinned golangci-lint to v2.1.6 while GitHub used `latest`, so the two CIs could disagree about whether the same commit lints. - `gosec` is pinned via a `GOSEC_VERSION` env var, and runs without `-no-fail`; a reviewed finding is suppressed at the call site with a `#nosec` comment carrying its reason, so the exception shows up in the diff. - golangci-lint is pinned to v2.13.1 in both `.github/workflows/style-guide.yml` and `.gitea/workflows/ci.yaml`, with a comment on each pointing at the other. - `go mod verify` runs before the vulnerability scan, checking the module cache against go.sum rather than trusting whatever was downloaded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`internal/consent/models.go` still described a `POST /check` API returning allow/deny/**filter** with a list of denied fields. That design was replaced by the coarse two-call gate; the client never calls such an endpoint and the plugin does no field-level filtering. The types were referenced only by their own tests, which flattered the coverage number, and their `json:` tags actively misled anyone reading the file for the wire format. Removed: `DecisionFilter`, `validDecisions`, `Decision.IsValid`, `ConsentResponse.Validate`, `ConsentResponse.DeniedFields`, `ConsentRequest.ResponseFields` and `ConsentRequest.Claims` (the last of which also meant the whole decoded token was being carried into a request body that is never sent anywhere). `ownerresolver.Claim.Selector` / `.Participant` and `Result.Scheme` were decoded and never read; they are gone too, with a comment saying the reply carries more than this and only what the plugin acts on is decoded — an unread field should not suggest the plugin considers something it does not. `ConsentRequest.Subject` is now documented for what it is: the data owner from the resolver, never the requestor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`internal/plugin`'s package doc (duplicated across config.go and consent.go) still said the plugin "applies consent-based filtering for personal data", and RequestFilter's doc still claimed it "captures all request headers" — which it stopped doing when the header capture was removed. `internal/consent`'s package doc described deciding whether data is "allowed, denied, or filtered". None of that is true: the gate is coarse allow/deny, there is no field-level filtering, and no headers are retained. The plugin package doc now says so and says why the coarse choice is deliberate — a filter that removes fields silently misses the one it does not know about, while a coarse gate still covers an empty or non-JSON personal-data response. The consent package doc was corrected when the dead filtering model was deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The runtime image ran as **root** with no `USER`, on `alpine:3.19` (past end-of-support, so no security patches), with no `HEALTHCHECK` and no `.dockerignore` — meaning `COPY . .` pulled in `.git`, changing the build context digest on every commit and invalidating the cache even when no source file had changed. - A fixed non-root uid (10001) runs the binary. The runner binds a unix socket in a directory it is given and makes outbound HTTP calls; root only widens what a compromise of it reaches. The uid is fixed so a shared socket volume can be given predictable ownership — the compose stack hands the volume over with a one-shot `socket-init` service, since a named volume is created root-owned. - Base bumped to `alpine:3.22`. - `HEALTHCHECK` probes the listener: the runner speaks the ext-plugin protocol rather than HTTP, so a bound socket is the only meaningful signal that it is accepting RPCs. APISIX now waits for it via `service_healthy`. - `.dockerignore` excludes `.git`, CI config, docs and build artifacts. - `CGO_ENABLED=0` for a static binary, and `go mod verify` in the build stage so the dependencies are checked against go.sum before anything is compiled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The direct dependencies had drifted years behind with nothing configured to notice: - `github.com/stretchr/testify` 1.8.4 -> 1.12.1 - `github.com/api7/ext-plugin-proto` v0.6.0 -> v0.6.1 `go mod tidy` also drops `github.com/davecgh/go-spew` and `github.com/pmezard/go-difflib` from the graph and moves yaml to `go.yaml.in/yaml/v3`. Tests pass under `-race` and the linter is clean on the new versions. `.github/dependabot.yml` turns future drift into a pull request rather than a review finding, covering Go modules, GitHub Actions and Docker base images. The go-plugin-runner is deliberately excluded from the batch group: it is pinned at v0.5.0 and drags an old transitive tree (zap 1.17, flatbuffers 2.0.0), so updating it is a decision to take deliberately — and a plugin whose runner is unmaintained is a strategic risk worth being reminded of. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`evaluateWithResolver` called `w.Header()` to read the upstream's Content-Type for the resolve request. The runner materialises its header map on that first call and `HasChange()` then returns true — so merely *looking* at a header sent every gated response back to APISIX down the "this response was modified" path, carrying an empty header diff. Probably benign, entirely untested, and needless. The Content-Type now comes from the Nginx `$upstream_http_content_type` variable, which has no such side effect. The header is consulted only when the variable is unavailable, as a degraded fallback rather than the norm. The response mock grows a `headerReads` counter so this is observable, and `TestResponseFilter_AllowDoesNotTouchHeaders` asserts an allowed response never materialises the header map. Both content-type sources are covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A component that can deny production traffic had no counters at all. Nothing
said how many requests were allowed, denied or failed open, how long the
consent-manager was taking, how large the request-context store had grown, or
how many audit records had been dropped. Logs were the only signal, and they are
unstructured and rate-limited — a misconfigured route denying everything looked
exactly like a quiet one.
`internal/metrics` exposes, in the Prometheus text format:
- `consent_decisions_total{decision,fail_mode}` — crucially labelled by fail
mode, so "denied because there is no consent" and "denied because the
consent-manager was down" are separable. They mean opposite things, and one of
them is a page.
- `consent_dependency_calls_total{dependency,outcome}` and
`consent_dependency_duration_seconds{dependency}` — a histogram whose buckets
straddle the default 5s per-call timeout, so a dependency drifting toward it
is visible before it starts failing.
- `consent_request_context_store_size` and
`consent_request_contexts_evicted_total` — the H-3 leak, made observable: the
size returns to zero when idle, and a rising floor is the leak.
- `consent_audit_events_dropped_total` — an attacker who can generate load can
suppress the record of their own access, so the loss must be alertable.
The exporter is hand-rolled rather than pulling in a Prometheus client, for the
same reason the OTLP encoder is: this is a sidecar-adjacent plugin whose
dependency tree is part of its risk surface. Output is ordered deterministically
so a scrape diff reflects real change.
`main.go` serves `/metrics` only when `CONSENT_METRICS_ADDRESS` is set — the
runner is otherwise reached only over its unix socket, so opening a TCP port is
the deployment's decision — and a failure there logs rather than taking the gate
down.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`time.Duration(cfg.ParticipantTokenTTL) * time.Second` was computed from a field nothing validated. A large enough value overflows into a NEGATIVE duration, and the cached token's expiry then lands in the past — so every request refetches a token, hammering the token service, from a config that looked merely eccentric. `participant_token_ttl` now defaults to `DefaultParticipantTokenTTL` (3000s, matching the consent client's own default, and matching what the README already claimed) and is bounded to 1–86400. A day is far longer than any token this plugin is issued, and comfortably below the overflow range, so the conversion at the call site is safe by construction — with a comment there saying why. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A repository that gates access to personal data had no private path for reporting a vulnerability and no required reviewers. Someone finding a way to make the gate release data had nowhere to send it but a public issue. - `SECURITY.md` points at GitHub private vulnerability reporting, says what to include (for a gate, "the response was released" is the key fact), and draws the scope line explicitly — a response released without consent, a consent for one consumer authorising another, ownership taken from the requestor, side channels around a denial, and audit suppression are all in scope; the absence of JWT signature verification is not, since it is a documented deployment requirement rather than a defect. - `.github/CODEOWNERS` requires review everywhere, calling out the decision path, the audit trail and the CI/release configuration by name. - CONTRIBUTING.md points at the security policy before it talks about PRs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`go.mod` requires Go 1.26, and on a machine with an older system Go where `GOTOOLCHAIN` cannot download, the build fails with `toolchain not available` — which reads like "Go 1.26 does not exist" rather than "the download was blocked". The bare major version is what fails to resolve; a full patch version in the module cache works. CONTRIBUTING.md now says so, with the commands to find a cached toolchain and pin it, and points at where CI pins golangci-lint so a contributor lints with the same version the pipeline uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The response phase read the whole upstream body, then `ownerresolver.Resolve` ran `json.Valid` over it and `json.Marshal` copied it again into the request envelope as a `json.RawMessage`. Peak footprint is therefore roughly **3x** the body size per in-flight request, on top of APISIX's own buffering of the same response — so a handful of concurrent large-collection responses can drive the runner's memory well past what the response size suggests. Nothing bounded it, and transport security does not help: cluster mTLS does not make the copy smaller. New `max_resolve_body_bytes` (default 1 MiB, range 1–100 MiB). A larger body is **denied** rather than forwarded — a body too large to examine is not a body the gate can vouch for — and, like the owner cap, it denies regardless of `fail_open`, since it is a deliberate limit rather than an outage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… (L-11) When `json.Valid(payload)` failed, the resolve envelope was sent with `encoding: "none"` — exactly as it was when the response carried no body at all. The resolver therefore could not tell "this response carried a payload I could not parse" from "this response had no payload", and fell back to resolving ownership from the resource descriptor alone. So a malformed-but-personal payload was judged without ever being inspected: a truncated write, a content-type mismatch, an upstream answering XML or NDJSON on a route declared JSON. The gate looked at nothing and let it through. The envelope now has a third encoding, `opaque`, carrying the declared content type and the payload's size but not the payload. The resolver can act on the distinction — fail closed on a body it was told exists but cannot be read — rather than being told a falsehood about it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The review document was swept into the C-1 commit by `git add -A`. It is a local working note, not a repository artifact — untracked here and ignored so it cannot be committed by accident again. The file itself stays in the working tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The review's test-suite assessment noted there was no test for `$request_id` being unavailable in only one phase. That case matters because it is the one where the plugin is structurally unable to gate: with no correlation key the two phases cannot be matched, so there is no captured context to decide on. Three cases added: - the response phase cannot read `$request_id` — denies, even with `fail_open`, since this is not an outage to ride out; - the request phase left no context at all — likewise denies; - the request phase with no correlation key stores nothing, rather than leaking an entry per request into the context store, and with one stores what the response phase later needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The shutdown flush was present and essentially never completed. `runner.Run` installs its own `signal.Notify` for SIGINT/SIGTERM and returns as soon as one arrives — its shutdown path is `close(done)` and return, i.e. instant. The flush waited for the same signal in a goroutine of its own. Go delivers a signal to *every* registered channel, so on SIGTERM both woke at once: the runner returned, `main` returned, and the process exited while `audit.ShutdownAll()` was still draining its queue and waiting on an HTTP export (up to the 5s export timeout). So a rolling redeploy still discarded up to one flush interval of access decisions — every allow and deny in that window — with the added problem that the loss was now invisible: `consent_audit_events_dropped_total` counts queue-overflow drops, and these were not that. The fix looked done and fired almost never. Since `Run` already blocks until the signal, the flush belongs after it, synchronously, where nothing can exit out from under it — and the plugin needs no signal handler of its own. Registering one was actively harmful anyway: `signal.Notify` disables the default SIGTERM disposition, so had the runner's handler ever stopped returning promptly, nothing would have terminated the process. `runAndFlush` makes the ordering explicit and testable; `main_test.go` pins it, because a flush that never completes still passes any test that only checks it was called. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Introduced by the H-5 concurrency work. After `wg.Wait()`, `checkOwners` walked the per-owner results in index order and returned the first entry marked as a problem — whether that problem was an error or a deny. Index ordering made the verdict deterministic, which was the goal and was correctly achieved, but it ranked error and deny purely by position, and they are not equivalent: a deny is a definite answer, an error is the absence of one, and only the absence is subject to the operator's fail policy. Failure scenario: a response resolves to owners A (index 0) and B (index 1) on a route configured `fail_open: true`. Both checks are in flight; B's returns deny (no granted consent) while A's returns HTTP 500, both landing before either cancellation takes effect. The loop reached index 0 first, saw a non-cancellation error, and applied the fail policy — releasing B's data even though B has explicitly not consented. The audit record then showed the contradiction: a per-owner deny for B alongside an enforced allow. The reduction is now by decisiveness first and index second: one pass looks for a deny across all results, and only if there is none does the error pass run. Within each pass the lowest index still wins, so the verdict remains independent of goroutine scheduling. The regression test forces the exact race with a two-party barrier holding both checks open until both have genuinely completed, so neither result is a cancellation artifact. It fails against the previous reduction and passes with this one. A second table test confirms the error path still applies `fail_open` when no deny exists anywhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
When the store was at `MaxRequestContexts`, `evictForSpaceLocked` swept expired entries (a full map scan) and, if none had expired, scanned the whole map again to find the single oldest — all under `requestContextMu`, all to free exactly one slot. The next request then repeated both scans. At the 100k cap that is two ~100k-entry scans per incoming request, serialised behind one mutex. And the store only *reaches* the cap under the leak or the abort-flood the cap exists to contain, so the O(n) path was guaranteed to engage precisely when load was already pathological: the cap converted an unbounded memory leak into an unbounded latency cliff, which undercuts the mitigation it was added to provide. An overflow now evicts a batch of the oldest entries (`contextEvictionBatch`, 1% of the cap) in one pass, so the scan is paid once per thousand requests rather than on every one — the following requests find room without scanning at all. The eviction still takes the *oldest* entries, so which contexts are dropped is unchanged; only how many, and how often the scan runs. The cap test now asserts the batch semantics, and a second test pins the amortisation directly: after an overflow, the next `contextEvictionBatch-1` stores must not evict again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The regression test I added with N-2 drove the ordering through two concurrent HTTP checks held open by a barrier. It was inherently racy and failed about half the time: whichever owner completes first cancels the other, so the sibling's deny can arrive as a `context.Canceled` error rather than a deny, and the test then passed or failed on timing instead of on the property it was meant to pin. A flaky test that guards a security property is worse than none — it teaches people to re-run. The result reduction is now a pure function, `reduceOwnerResults`, with `ownerCheckResult` lifted to package scope, so the ranking can be exercised with scripted results at the level where it actually lives. The table covers a deny at a higher index than an error (the finding, which still fails against the index-only reduction), a deny at a lower index, two denies, error-only under both fail policies, a cancellation caused by a sibling deny versus one caused by the phase deadline, and an owner whose check never started. No behaviour change: `checkOwners` calls the extracted function and the ranking rule is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`coversPurpose` returns true for an empty purpose, which is a defensible and documented rule — but it made half of the C-2 fix contingent on a *different service* populating an optional field. A resolver deployment whose rules never set `purpose` runs with purpose scoping entirely disabled, and nothing anywhere said so: no log line, no metric, no way to demand it. The failure is quiet and compliance-shaped: the resolver is upgraded, a rule loses its purpose mapping, and consent granted for "insurance quote" now also authorises release for research. The consumer match still holds, so it narrows rather than opens the gate — but a property that was supposed to be enforced silently stopped being, and no signal would have revealed it. - `consent_purpose_unconstrained_total` counts every check run without a purpose to match against, so the state is alertable rather than merely documented, and a rate-limited warning says the same thing in the log. - `require_purpose` (default false) turns a claim without a purpose into a denial. It defaults off because requiring it would break every deployment whose resolver does not emit one yet; turn it on once yours does and the property is enforced rather than hoped for. Being a policy the operator asked for rather than an outage, it is `failAlwaysClosed` — `fail_open` does not lift it. Tests: `checkPurposeScoping` as a table (counted when not required, denied when required, denied even with fail_open), plus the `coversPurpose` cases the re-review flagged as missing at the consent-client level — matching purpose allows, a different purpose denies, a consent covering no purpose denies a purpose-scoped check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pe (N-5)
`Sanitize`'s doc comment stated the problem exactly — "messages built by
wrapping dependency errors embed the dependency's response body, which can carry
identifiers or other personal data" — and then collapsed control characters and
truncated to 200 characters. Neither removes the body. Truncation is not
redaction, and the surviving prefix of a JSON error page is usually precisely
the part with the identifiers in it.
Five call sites in the consent client (and one in the resolver client) embedded
`truncateBody(body)` in their errors, and those errors become the plugin's
decision reason — exported to the audit sink and written to stdout. A
consent-manager 500 echoing the user identifier landed in both, now
single-line and ≤200 chars, which is tidier but no less identifying.
`unexpectedStatus` now builds a stable, low-cardinality classification
("consent client: identifier search returned status 500") and sends the body to
a rate-limited DEBUG log instead, so it is available when debugging, off by
default, and never on a path that escapes. The resolver client does the same for
its own error page, which can echo the payload it was handed — the very personal
data the gate exists to protect. An audit `reason` wants a classification
anyway: it is queried, not read.
`truncateBody` and `truncate` are gone with their test; `logging.DebugfEvery` is
the new home for this kind of detail.
The regression test drives a consent-manager whose 500 body contains an email
address and asserts it does not reach the OTLP payload, while the classification
does. It fails if the body is put back into the error.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…LP (N-6) `consent_request_contexts_evicted_total` and `consent_audit_events_dropped_total` were registered through `RegisterGauge` and emitted as `# TYPE … gauge`. Both are monotonic, and their `_total` suffix says so: `promtool check metrics` flags the mismatch, and anyone reaching for `rate(…_total[5m])` over a declared gauge is relying on an accident rather than a stated contract. Gauge families also emitted no `# HELP` line at all, unlike the counters and the histogram. - `RegisterCounter` joins `RegisterGauge`; both now take a help string, and the two `_total` callbacks moved across. The constants are renamed to say which they are (`ContextEvictedCounter`, `AuditDroppedCounter`), leaving `ContextStoreSizeGauge` a gauge, which it correctly is — it goes down. - Every callback family emits `# HELP` and its true `# TYPE`. - The redundant `cumulative` local in the histogram rendering is gone, with a comment noting that `counts` is already cumulative because `observe` increments every bucket at or above the value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…h (§4)
The re-review listed three small gaps, all in code that is live rather than
incidental.
- `audit.Dropped()` was at 0% despite being read by the registered metric
callback — the only thing that makes suppression of an access record visible.
Now covered, including that it aggregates across emitters.
- `fetchProviderSD` was at 65%. It runs before any consent check, so a failure
there takes the whole exchange down; its 401, 5xx, unparseable-body and
empty-selfDescriptionURL branches are now pinned, along with an assertion that
the dependency's response body does not travel in the error (N-5). The static
provider-SD override is covered too, asserting it makes no HTTP call at all.
- No benchmark existed for the store under load, so N-3's fix was unmeasured.
`BenchmarkStoreRequestContextAtCap` fills the store to `MaxRequestContexts` and
measures the overflow path — the one that only engages under the leak or
abort-flood the cap exists to contain, i.e. exactly when the gate is already
under pressure. On this machine:
eviction batch 1 (the old behaviour) ~3,000,000 ns/op
eviction batch 1% of cap (current) ~17,000 ns/op
about a 175x difference, and the batched figure sits within ~2x of the
uncontended `BenchmarkStoreRequestContext`. That is the latency cliff the
finding described, now measurable rather than argued.
Total coverage 89.0% -> 91.1%.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…M-6 residual) `docker compose config -q` passing is not the same as the stack running, and actually running it — to settle the M-1 Content-Length question end to end — turned up two things that made it fail: - **`bitnami/etcd:3.5` no longer resolves.** Bitnami retired their public catalog, so the very first `docker compose up` died on `manifest unknown` before anything started. Switched to the upstream `quay.io/coreos/etcd`, which needs no auth-disabling env var because it is open by default — fine for a local stack, which this is, and it is not a deployment example. - **WireMock rejected every stub file.** The `"//"` keys I used as comments are unknown top-level fields on a StubMapping, so WireMock exited at startup with `Unrecognized field "//"`. The explanations now live in the mapping's own `name` and `metadata` fields, which is what they are for. The second failure was quiet in a way worth fixing rather than just correcting: `docker compose ps` hides an exited container, so a dead mock surfaced only as an unexplained deny from the gate several steps later, with a DNS error buried in the runner's log. The mock now has a healthcheck and the runner waits on it, so a broken stub fails where it happens. Also wires `CONSENT_METRICS_ADDRESS` into the runner and publishes port 9091, so the metrics endpoint is exercisable in the dev stack rather than only in tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.