fix(wolf-rbac): scope identity headers to route configuration - #13905
fix(wolf-rbac): scope identity headers to route configuration#13905shreemaan-abhishek wants to merge 8 commits into
Conversation
membphis
left a comment
There was a problem hiding this comment.
BREAKING CHANGE: Existing Routes can already contain a schema-generated header_prefix: "X-" even when users never explicitly configured it. Giving that stored default precedence changes the upstream identity-header contract for Consumers with custom prefixes.
Please fix the compatibility regression described in the inline comment before merging, and explicitly document the breaking change, affected existing configurations, migration steps, rollout order, and recovery guidance in the PR description.
| return 401, fail_response("Invalid appid in rbac token") | ||
| end | ||
| core.log.info("consumer appid: ", appid) | ||
| local prefix = conf.header_prefix |
There was a problem hiding this comment.
[P1] Preserve the identity-header contract of existing Route configurations
This is a breaking change for existing deployments, not only a precedence change for newly configured Routes. The old schema inserts header_prefix: "X-" when the Route field is omitted, and the Admin API persists the validated configuration. After upgrade, this branch treats that stored default as an explicit Route choice and overrides a Consumer-level custom prefix.
For an existing Route containing the historical "X-" default and a Consumer using "X-Wolf-", a focused test of the actual old and new rewrite functions shows that the old code replaces a client-supplied X-Wolf-UserId with the authenticated user ID. The new code leaves the client-supplied X-Wolf-UserId intact and writes the authenticated ID to X-UserId instead. An upstream that still relies on X-Wolf-* can therefore lose the trusted identity or consume a spoofed value after upgrade. Default-prefix Consumers and newly created Routes that truly omit the field do not trigger this case.
Please fix the compatibility regression by preserving the historical Consumer-prefix contract until an explicit migration or a distinguishable opt-in selects Route precedence. Removing the default from the new schema does not remove defaults already stored in old configurations.
Please also explicitly declare this as a BREAKING CHANGE in the PR description, including affected historical configurations, required migration steps, rollout order, and recovery guidance. Add an upgrade regression starting from an old-schema-defaulted Route and assert both authenticated identity output and removal or replacement of spoofed headers in the trusted namespace.
There was a problem hiding this comment.
Good catch. Updated to preserve Consumer header_prefix behavior and use a new Route/Service output_header_prefix as the explicit opt-in. I also added the persisted header_prefix: "X-" upgrade regression.
membphis
left a comment
There was a problem hiding this comment.
The stored Route / Consumer prefix compatibility regression is fixed in this head. The remaining findings are below.
[P2] Clear legacy Consumer-prefixed identity headers before authentication can short-circuit
The pre-chain hook clears the three identity headers under conf.output_header_prefix or "X-". When a supported legacy Consumer uses header_prefix: "X-Wolf-" and the Route has no output_header_prefix, that Consumer namespace is only cleared after Wolf parses the token and identifies the Consumer.
A request with a valid key-auth credential and a client-supplied X-Wolf-UserId therefore retains the forged header if key-auth succeeds first, or if Wolf runs first but returns early for a missing or malformed token. An upstream that trusts these headers can receive an unverified Wolf identity even though only the other authenticator succeeded. This is incomplete coverage of the existing cleanup issue for a still-supported configuration.
I reproduced both authenticator orderings using the reviewed Lua source under LuaJIT with mocked dependencies: key-auth succeeds, Wolf makes zero authorization calls, and X-Wolf-UserId=forged remains on the request. The default prefix, an explicit matching Route output_header_prefix, and successful Wolf authentication with the Consumer fallback pass the corresponding checks.
Please provide upgrade validation and migration to an explicit output_header_prefix for these legacy multi-auth Routes, or prepare the complete reserved-prefix set when configuration changes and clear it before the authentication chain runs. Add regressions for both orderings and missing/malformed Wolf tokens, while preserving the now-correct successful Consumer-prefix fallback.
Evidence: pre-chain cleanup, deferred Consumer-prefix cleanup, and authentication short-circuit.
[P2] Use output_header_prefix in the Chinese Route configuration documentation
The Chinese attribute table and Route example still recommend header_prefix and say the Route value takes precedence over the Consumer. The implementation intentionally ignores the stored Route header_prefix for output-prefix selection and only treats output_header_prefix as an explicit Route override.
For example, a Consumer with header_prefix: "X-Wolf-" and a Route configured as the documented header_prefix: "X-" still emits the Consumer's X-Wolf-* identity headers. The configuration is accepted, so users receive no indication that the documented Route setting does not control the output or pre-chain cleanup namespace.
Please update the Chinese attribute table, explanatory text, and example to use output_header_prefix, and document the deprecated Consumer fallback and precedence consistently with the English documentation.
Evidence: Chinese attribute table, Route example, and actual prefix selection.
|
|
||
|
|
||
| function _M.clear_auth_headers(conf, ctx) | ||
| clear_identity_headers(ctx, conf.output_header_prefix or default_header_prefix) |
There was a problem hiding this comment.
clear_auth_headers only knows the Route-level output_header_prefix, so the pre-clear pass in multi-auth can only scrub the X- namespace. In the configuration this PR documents as the supported compatibility path — no output_header_prefix on the Route, a custom header_prefix on the Consumer — the spoofing hole stays open whenever another authenticator wins first.
The Consumer-fallback clear added further down (if not conf.output_header_prefix and prefix ~= default_header_prefix) does not cover it: it sits inside rewrite() after cur_consumer has been resolved, and rewrite() never runs at all once an earlier authenticator returns success.
Repro on ef63c5ef — multi-auth: [key-auth, wolf-rbac{}], Consumer header_prefix: "X-Wolf-", request carries a valid apikey plus spoofed X-Wolf-* headers:
not ok - response_body
got: 'nil,nil,nil,spoofed-consumer,spoofed-consumer,spoofed-consumer'
expected: 'nil,nil,nil,nil,nil,nil'
not ok - pattern "hit wolf-rbac rewrite" should match a line in error.log
The second assertion is the core.log.info("hit wolf-rbac rewrite") already present in rewrite(); it never fires, which confirms the fallback clear is unreachable on this path.
The added coverage does not exercise this: TEST 25 sets output_header_prefix on the Route, and TEST 24 injects only X-* headers even though TEST 23 creates the wolf-prefixed Consumer.
This needs a decision rather than one more test case. Either the pre-clear pass resolves the Consumer prefixes (which the PR description rules out — "does not enumerate or cache Consumer configurations"), or Consumer-level header_prefix is treated as unsafe and removed outright instead of kept as a working deprecated fallback. As written, neither holds.
|
Thanks. The first point is the deliberate backward-compatibility trade-off: the Consumer prefix remains a post-match fallback, while deterministic route-level behavior requires |
|
Superseded by #13929, which uses the clean Route/Service and Consumer schema split. |
Description
Wolf identity headers form part of the upstream contract of a Route or Service, while existing deployments may derive their prefix from Consumer configuration.
This change introduces an explicit Route or Service opt-in without changing the historical Consumer-prefix behavior:
output_header_prefixdefines the identity-header namespace for a Route or Service when configured.header_prefixremains the compatibility fallback and is now deprecated for new configurations.header_prefixvalues remain accepted but do not override the Consumer prefix.multi-authinvokes an optional preparation hook for configured authenticators before evaluation.Regression coverage includes both
multi-authorderings, default and custom output prefixes, successful Wolf authentication, the Route opt-in precedence, the Consumer fallback, and an upgrade case with a stored Routeheader_prefix: "X-"and a custom Consumer prefix.The documentation recommends
output_header_prefixon the Route or Service and marks Consumerheader_prefixas deprecated.Which issue(s) this PR fixes:
N/A
Checklist