AlinsRan commented on code in PR #13649:
URL: https://github.com/apache/apisix/pull/13649#discussion_r3725725687
##########
apisix/plugins/openid-connect.lua:
##########
@@ -488,8 +618,92 @@ local _M = {
name = plugin_name,
schema = schema,
_build_session_opts = build_session_opts,
+ _flatten_openidc_options = flatten_openidc_options,
}
+-- lua-resty-openidc rejects a JWK that lacks the members its key type needs,
+-- but only once the first authorization request builds the thumbprint, which
+-- surfaces as a 500 per request instead of a rejected configuration.
+local function check_dpop_key(dpop)
+ local jwk = dpop and dpop.public_jwk
+ if not jwk then
+ return true
+ end
+
+ local required = dpop_jwk_required_members[jwk.kty]
+ if not required then
+ return false, "property \"dpop.public_jwk\" validation failed: kty \""
+ .. tostring(jwk.kty) .. "\" is not supported"
+ end
+
+ for _, member in ipairs(required) do
+ if jwk[member] == nil then
Review Comment:
Confirmed and fixed in 309a29d — `{kty = "EC", crv = "P-384", x = "x", y =
"y"}` passed, and so did `x = 1` and an empty string.
The members are now required to be non-empty strings, and ES256 requires
`crv` P-256 per RFC 7518. Both matter for the same reason: the members go into
the RFC 7638 thumbprint verbatim (`openidc_dpop_jwk_thumbprint` concatenates
them through `cjson.encode`), so a number or another curve yields a `dpop_jkt`
no OP can match.
While adding it I extended the check to the private key as well, since that
is what actually signs the proof and a matching JWK says nothing about it:
`dpop.private_key` now has to load and be of the key type the algorithm needs,
which closes `unable to load DPoP private key` and the padding mismatch — the
two remaining per-request 500s on this path. Secret references are skipped, and
it is safe against `encrypt_fields` because `plugin_checker` decrypts before
validating.
TEST 73 covers P-384 and the member types, TEST 75 the private key. TEST 51a
now carries a real EC key pair rather than a placeholder PEM, since a
placeholder no longer validates.
##########
apisix/plugins/openid-connect.lua:
##########
@@ -304,6 +354,70 @@ local schema = {
type = "boolean",
default = false
},
+ par = {
+ description = "Pushed Authorization Requests (PAR) configuration.",
+ type = "object",
+ properties = {
+ enabled = {
+ description = "When true, use Pushed Authorization
Requests (PAR).",
+ type = "boolean",
+ default = false,
+ },
+ endpoint = {
+ description = "URL of the Pushed Authorization Requests
endpoint.",
+ type = "string",
+ },
+ endpoint_auth_method = {
+ description = "Authentication method for the PAR
endpoint.",
+ type = "string",
Review Comment:
Confirmed and fixed in 309a29d. All three cases you list passed
`check_schema` and would have failed on the first PAR request.
Worth recording why PAR is different from the token endpoint:
`openidc_get_token_auth_method` logs `... is not supported, ignoring it` and
falls back (`openidc.lua:1155`), so an unusable `token_endpoint_auth_method`
degrades. `openidc_pushed_authorization_request` returns the error instead
(`openidc.lua:547`), which reaches the plugin as a 500. That is why this needs
config-time validation while the token endpoint has survived without it.
`par.endpoint_auth_method` now has an enum of the four methods in
`supported_token_auth_methods`, and when `par.enabled` is true the method PAR
will actually use has to have its credential — checking the effective one, so
the fallback case is covered too:
```
property "par.endpoint_auth_method" "private_key_jwt" requires
"client_rsa_private_key" when "par.enabled" is true
property "token_endpoint_auth_method" "private_key_jwt" requires
"client_rsa_private_key" when "par.enabled" is true
```
The second line is the fallback: `par.enabled = true` with no
`endpoint_auth_method` of its own. TEST 74 covers all four rejections, TEST 75
pins that the valid combinations still pass — including an unusable
`token_endpoint_auth_method` while PAR is off, which stays valid because the
token endpoint falls back on its own.
I did not add the same enum to `token_endpoint_auth_method` /
`introspection_endpoint_auth_method`. It would be worth doing — an unknown
value there makes `openidc_apply_client_auth` match no branch and send the
request with no client authentication at all, silently — but it is pre-existing
behavior and a wider change than this PR.
--
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]
