AlinsRan commented on PR #13649:
URL: https://github.com/apache/apisix/pull/13649#issuecomment-5173491103
@kevinlzw I pushed a follow-up commit to this branch (maintainer edits are
enabled) rather than leaving three more review threads — happy to revert it if
you'd rather apply the changes yourself. It is a plain fast-forward, nothing
existing was rewritten.
What it changes, all of it following from the threads above:
**`client_jwt_assertion_alg` now has an enum, and `PS256` is out of the docs
example and TEST 51a.** `lua-resty-openidc` signs the client assertion with
`r_jwt:sign()` and no `pcall`, and `resty.jwt` ends in
`error({reason="unsupported alg: " .. alg})` for anything it does not handle —
so an unsupported value is a 500 with a stack trace on every request, not a
failed authentication. The rockspec pins `api7-lua-resty-jwt`, whose `sign()`
handles only `HS256`/`HS512`/`RS256`/`RS512`/`ES256`/`ES512`.
`lua-resty-openidc` depends on `lua-resty-jwt`, which does support `PS256`, but
both rocks install the same `resty/jwt.lua`; installing them into a clean tree
in either order leaves the pinned one in place, so the effective set is the
smaller one. `dpop.signing_alg` is unaffected — DPoP proofs go through
`resty.openssl.pkey`, and that enum already matches the library's
`supported_dpop_signing_algs` exactly.
**The `dpop_*` mappings are now covered.** My earlier suggestion about
`dpop_jkt` was incomplete: it reaches `use_dpop` and `dpop_public_jwk`, but a
DPoP proof is only built when `ep_name == "token"` (`openidc.lua:913`), which
the PAR call never is, so `dpop_private_key` and `dpop_signing_alg` were never
exercised — that is also why the placeholder key in TEST 56 works.
`flatten_openidc_options` is now exported the way `_build_session_opts` already
was, and TEST 62/63 assert all six flat names plus that `conf.par`/`conf.dpop`
are cleared.
**TEST 56's `par.endpoint_auth_method` assertion was vacuous.** Dropping
that mapping makes the library fall back to `token_endpoint_auth_method`, which
the schema defaults to `client_secret_basic`, and the mock accepted that too —
so the test passed either way. The mock now requires the credentials in the
POST body, which only `client_secret_post` sends. I checked each of the six
mappings by breaking it in turn; every one now fails a test.
**TEST 64 and an upgrade note in both docs** for the introspection change,
since it hits configurations that are not modified at all. The test asserts the
credentials land in the `Authorization` header under the default and under
`client_secret_basic`, and in the body only under `client_secret_post`.
Removing the schema default flips the first case, which is what makes it a real
regression guard.
Local runs against `lua-resty-openidc` 1.9.0: TEST 51a-56 and TEST 61-64
pass, `luacheck` clean. The Keycloak-backed cases I did not run locally, CI
covers those.
--
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]
