AlinsRan commented on code in PR #13649:
URL: https://github.com/apache/apisix/pull/13649#discussion_r3671971018
##########
apisix/plugins/openid-connect.lua:
##########
@@ -371,6 +454,14 @@ local schema = {
type = "integer",
default = 60
},
+ client_jwt_assertion_alg = {
Review Comment:
No enum here, and unlike an invalid `dpop.signing_alg` this is not caught
per request either: `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 cannot sign. An
unsupported value is therefore a 500 with a stack trace on every request, not a
failed authentication.
`PS256` — used by the docs example and TEST 51a — is one of those values.
The rockspec pins `api7-lua-resty-jwt`, whose `sign()` handles only
`HS256`/`HS512`/`RS256`/`RS512`/`ES256`/`ES512`. `lua-resty-openidc` pulls in
`lua-resty-jwt`, which does support `PS256`, but both rocks install the same
`resty/jwt.lua`, and installing them into a clean tree in either order leaves
the pinned one in place — so the effective set is the smaller one.
Addressed in c74b8fb: enum added, docs example and TEST 51a switched to
`RS512`, TEST 63 covers the rejection. `dpop.signing_alg` is unaffected — DPoP
proofs go through `resty.openssl.pkey`, and that enum already matches the
library's `supported_dpop_signing_algs`.
##########
apisix/plugins/openid-connect.lua:
##########
@@ -371,6 +454,14 @@ local schema = {
type = "integer",
default = 60
},
+ client_jwt_assertion_alg = {
Review Comment:
Following up on this with something firmer than the suggestion I originally
made: `PS256`, the value this PR's own docs example and TEST 51a use, cannot be
signed by the `resty.jwt` this repository ships.
`call_token_endpoint` reaches `r_jwt:sign(key, assertion)`
(`openidc.lua:863`) with no `pcall` around it. `resty.jwt`'s `sign()` ends in
`error({reason="unsupported alg: " .. alg})` for anything it does not handle,
so an unsupported value is not a failed authentication — it is an uncaught Lua
error, i.e. a 500 with a stack trace on every request through the route.
The rockspec pins `api7-lua-resty-jwt = 0.2.6-0` (line 46), whose `sign()`
handles only `HS256`, `HS512`, `RS256`, `RS512`, `ES256`, `ES512`.
`lua-resty-openidc` depends on `lua-resty-jwt >= 0.2.0`, which resolves to
0.3.2 and does support `PS256`, but both rocks install the same
`resty/jwt.lua`. I checked what actually ends up on disk by installing them
into a clean tree in both orders:
```
api7-lua-resty-jwt 0.2.6 then lua-resty-jwt 0.3.2 -> resty/jwt.lua == api7
fork
lua-resty-jwt 0.3.2 then api7-lua-resty-jwt 0.2.6 -> resty/jwt.lua == api7
fork
```
Byte-identical to the fork either way, so the effective set is the six
algorithms above regardless of resolution order. Nothing here reaches the
checks earlier in that branch first: `PS256` is not `HS*`, and
`openidc_supported_discovery_value` passes when the OP advertises `PS256` in
`token_endpoint_auth_signing_alg_values_supported` (or omits the field).
Two things worth changing: the docs example and TEST 51a should not
recommend `PS256`, and an `enum` of the six supported values would turn this
class of typo into a config-time rejection instead of a per-request 500. Note
this does not apply to `dpop.signing_alg` — DPoP proofs are signed through
`resty.openssl.pkey` in `openidc_dpop_sign`, and that enum already matches the
library's `supported_dpop_signing_algs` exactly.
--
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]
