Skip to content

fix(wolf-rbac): scope identity headers to route configuration - #13905

Closed
shreemaan-abhishek wants to merge 8 commits into
apache:masterfrom
shreemaan-abhishek:fix/multi-auth-wolf-identity-headers
Closed

fix(wolf-rbac): scope identity headers to route configuration#13905
shreemaan-abhishek wants to merge 8 commits into
apache:masterfrom
shreemaan-abhishek:fix/multi-auth-wolf-identity-headers

Conversation

@shreemaan-abhishek

@shreemaan-abhishek shreemaan-abhishek commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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_prefix defines the identity-header namespace for a Route or Service when configured.
  • Consumer header_prefix remains the compatibility fallback and is now deprecated for new configurations.
  • Stored Route header_prefix values remain accepted but do not override the Consumer prefix.
  • multi-auth invokes an optional preparation hook for configured authenticators before evaluation.
  • Header preparation is constant-time and does not enumerate or cache Consumer configurations.
  • Identity output is added only after Wolf authorization succeeds.

Regression coverage includes both multi-auth orderings, default and custom output prefixes, successful Wolf authentication, the Route opt-in precedence, the Consumer fallback, and an upgrade case with a stored Route header_prefix: "X-" and a custom Consumer prefix.

The documentation recommends output_header_prefix on the Route or Service and marks Consumer header_prefix as deprecated.

Which issue(s) this PR fixes:

N/A

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have preserved existing Consumer-prefix behavior

@shreemaan-abhishek shreemaan-abhishek changed the title fix(multi-auth): clear reserved auth headers before fallback fix(wolf-rbac): scope identity headers to route configuration Sep 2, 2026

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread apisix/plugins/wolf-rbac.lua Outdated
return 401, fail_response("Invalid appid in rbac token")
end
core.log.info("consumer appid: ", appid)
local prefix = conf.header_prefix

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@shreemaan-abhishek shreemaan-abhishek Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ef63c5efmulti-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.

@shreemaan-abhishek

Copy link
Copy Markdown
Contributor Author

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 output_header_prefix. I fixed the second point in b34d34a by aligning the Chinese attribute table, guidance, and Route example with the implementation.

@shreemaan-abhishek

Copy link
Copy Markdown
Contributor Author

Superseded by #13929, which uses the clean Route/Service and Consumer schema split.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants