AlinsRan commented on PR #13649:
URL: https://github.com/apache/apisix/pull/13649#issuecomment-5188771427
@membphis Thanks — the library change you describe is real, but I do not
think the impact lands on this repository. Details, since the conclusion turns
on one fact.
**The 1.9.0 change is real.** `openidc.jwt_verify` gained a gate on the
number of varargs:
```lua
function openidc.jwt_verify(access_token, opts, ...)
+ local has_claim_specs = select("#", ...) > 0
+ local v
+ if not has_claim_specs then
+ v = get_cached_jwt_verification(opts, access_token)
+ end
```
and the same guard around `set_cached_jwt_verification`. The plugin calls
`openidc.bearer_jwt_verify(conf, opts)`, so `select("#", ...)` is always 1 and
both branches are skipped. That part is exactly as you describe.
**But the cache it skips has never been active here.**
`get_cached_jwt_verification` reads `ngx.shared["jwt_verification"]` — same
dict name in 1.8.0 and 1.9.0 — and that dict is not declared anywhere in this
repository:
```
$ grep -rn "jwt_verification" apisix/ conf/ | grep -v
jwt_verification_cache_ignore
(no matches)
```
`apisix/cli/ngx_tpl.lua` declares `discovery`, `jwks` and `introspection`;
`introspection` backs `openidc.introspect` (`openidc.lua:2293`), not JWT
verification (`:2423`) — its `# cache for JWT verification results` comment is
misleading. Probed at runtime, `ngx.shared["jwt_verification"]` is nil. With a
nil dict `openidc_cache_get` returns nil and `openidc_cache_set` is a no-op, so
bearer JWTs were being verified cryptographically on every request under 1.8.0
as well. The bump changes nothing here, and a "second request uses the cache"
regression test cannot pass on either version today.
Two things fall out of that, neither of which I have changed yet:
1. **The JWT verification cache has never worked in this plugin.** That is a
pre-existing gap, not something this PR introduces, and
`jwt_verification_cache_ignore` in the schema implies it was meant to. Fixing
it means declaring the dict — worth its own change, and I would rather not fold
a new shared dict into this PR.
2. **The 1.9.0 gate does matter for a deployment that declares
`jwt_verification` itself** through `nginx_config.http.lua_shared_dict`. There,
caching would work on 1.8.0 and stop on 1.9.0.
If you want (2) closed inside this PR, the fix is to stop passing a claim
spec and validate the issuer in the plugin instead. The claim spec we pass
carries exactly one thing — `lua-resty-jwt` expands `{valid_issuers = ...}` to
`{iss = equals_any_of(valid_issuers)}` and nothing else, since we never set
`lifetime_grace_period` or `require_*_claim` — so it is replaceable. Two
consequences to weigh: the issuer-mismatch error text changes, which
`t/plugin/openid-connect8.t` TEST 4 asserts verbatim (`Claim 'iss' (...)
returned failure`), and passing no claim spec re-enables lua-resty-jwt's
default `exp`/`nbf` validators, which the current call suppresses.
Happy to do that, but it is a behavior change on a path that is inert by
default, so I would rather have your call before making it. Or I can open a
separate issue for the missing dict and leave this PR to the bump.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
