AlinsRan commented on code in PR #13649:
URL: https://github.com/apache/apisix/pull/13649#discussion_r3733737119
##########
apisix/plugins/openid-connect.lua:
##########
@@ -488,8 +635,168 @@ 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)
+ -- lua-resty-openidc reads none of this while use_dpop is false, so a
+ -- configuration that stages the key material before turning DPoP on is
+ -- valid and must not be rejected
+ if not (dpop and dpop.enabled) then
+ return true
+ end
+
+ local jwk = 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
+ return false, "property \"dpop.public_jwk\" validation failed: kty
\""
+ .. jwk.kty .. "\" requires " .. concat(required, ",
")
+ end
+ -- the members go into the RFC 7638 thumbprint verbatim, so a
non-string
+ -- would be encoded as itself and produce a thumbprint no OP can match
+ if type(jwk[member]) ~= "string" or jwk[member] == "" then
+ return false, "property \"dpop.public_jwk\" validation failed: \""
+ .. member .. "\" must be a non-empty string"
+ end
+ end
+
+ local expected = dpop_alg_key_type[dpop.signing_alg]
+ if expected then
+ if expected.kty ~= jwk.kty then
+ return false, "property \"dpop.signing_alg\" \"" ..
dpop.signing_alg
+ .. "\" requires an " .. expected.kty .. "
\"dpop.public_jwk\""
+ end
+ if expected.crv and jwk.crv ~= expected.crv then
+ return false, "property \"dpop.signing_alg\" \"" ..
dpop.signing_alg
+ .. "\" requires \"dpop.public_jwk\" crv \"" ..
expected.crv
+ .. "\", got \"" .. tostring(jwk.crv) .. "\""
+ end
+
+ -- the proof is signed with the private key, not the JWK, so a matching
+ -- JWK says nothing about it; openidc_dpop_sign fails per request on a
+ -- key it cannot load or cannot use with the algorithm's padding
+ local private_key = dpop.private_key
+ if private_key and not secret.is_secret_ref(private_key) then
+ local key, err = pkey.new(private_key)
+ if not key then
+ return false, "property \"dpop.private_key\" is not a valid
key: "
+ .. tostring(err)
+ end
+ local key_type = key:get_key_type()
+ key_type = type(key_type) == "table" and key_type.sn or key_type
+ if key_type ~= dpop_openssl_key_type[expected.kty] then
+ return false, "property \"dpop.private_key\" is not an "
+ .. expected.kty .. " key, which
\"dpop.signing_alg\" \""
+ .. dpop.signing_alg .. "\" requires"
+ end
+ end
+ end
+
+ return true
+end
+
+
+-- The token endpoint only logs and falls back when its auth method cannot be
+-- used, but the PAR request fails outright (openidc.lua:547), which surfaces
+-- as a 500. PAR uses its own method when set and token_endpoint_auth_method
+-- otherwise, so validate whichever one it will actually use.
+local function check_par_auth_method(conf)
+ if not (conf.par and conf.par.enabled) then
+ return true
+ end
+
+ local method = conf.par.endpoint_auth_method
+ local source = "par.endpoint_auth_method"
+ if not method then
+ method = conf.token_endpoint_auth_method
+ source = "token_endpoint_auth_method"
+ end
+ if not method then
+ return true
+ end
+
+ local credential = token_auth_method_credential[method]
+ if credential == nil then
+ return false, "property \"" .. source .. "\" \"" .. method
+ .. "\" is not supported when \"par.enabled\" is true"
+ end
+ if credential and not conf[credential] then
+ return false, "property \"" .. source .. "\" \"" .. method
+ .. "\" requires \"" .. credential .. "\" when
\"par.enabled\" is true"
+ end
+
+ return true
+end
+
+
+-- The client assertion is signed with a single algorithm, but each endpoint
+-- picks its own auth method. lua-resty-openidc rejects a symmetric algorithm
+-- with private_key_jwt and an asymmetric one with client_secret_jwt when the
+-- endpoint is called, which surfaces as a 500. With no algorithm configured
+-- the library defaults per auth method, so the families cannot conflict.
+local function check_client_jwt_assertion_alg(conf)
+ local alg = conf.client_jwt_assertion_alg
+ if not alg then
+ return true
+ end
+
+ -- bearer_only never runs the authorization code flow, so only the
+ -- introspection endpoint is ever called
+ local selections = {
Review Comment:
Both reproduced and fixed in e2ed91e — my check was counting an endpoint the
configuration never calls.
The selection now follows the two conditions you describe: `introspect()` is
only entered when one of
`bearer_only`/`introspection_endpoint`/`public_key`/`use_jwks` is set, and
inside it `public_key` or `use_jwks` takes the local verification branch
instead, so the introspection method counts only when local verification is
absent and `(bearer_only or introspection_endpoint)` holds. TEST 81 pins both
branches.
##########
apisix/plugins/openid-connect.lua:
##########
@@ -488,8 +635,168 @@ 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)
+ -- lua-resty-openidc reads none of this while use_dpop is false, so a
+ -- configuration that stages the key material before turning DPoP on is
+ -- valid and must not be rejected
+ if not (dpop and dpop.enabled) then
+ return true
+ end
+
+ local jwk = 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
+ return false, "property \"dpop.public_jwk\" validation failed: kty
\""
+ .. jwk.kty .. "\" requires " .. concat(required, ",
")
+ end
+ -- the members go into the RFC 7638 thumbprint verbatim, so a
non-string
+ -- would be encoded as itself and produce a thumbprint no OP can match
+ if type(jwk[member]) ~= "string" or jwk[member] == "" then
+ return false, "property \"dpop.public_jwk\" validation failed: \""
+ .. member .. "\" must be a non-empty string"
+ end
+ end
+
+ local expected = dpop_alg_key_type[dpop.signing_alg]
+ if expected then
+ if expected.kty ~= jwk.kty then
+ return false, "property \"dpop.signing_alg\" \"" ..
dpop.signing_alg
+ .. "\" requires an " .. expected.kty .. "
\"dpop.public_jwk\""
+ end
+ if expected.crv and jwk.crv ~= expected.crv then
+ return false, "property \"dpop.signing_alg\" \"" ..
dpop.signing_alg
+ .. "\" requires \"dpop.public_jwk\" crv \"" ..
expected.crv
+ .. "\", got \"" .. tostring(jwk.crv) .. "\""
+ end
+
+ -- the proof is signed with the private key, not the JWK, so a matching
+ -- JWK says nothing about it; openidc_dpop_sign fails per request on a
+ -- key it cannot load or cannot use with the algorithm's padding
+ local private_key = dpop.private_key
+ if private_key and not secret.is_secret_ref(private_key) then
+ local key, err = pkey.new(private_key)
+ if not key then
+ return false, "property \"dpop.private_key\" is not a valid
key: "
+ .. tostring(err)
+ end
+ local key_type = key:get_key_type()
+ key_type = type(key_type) == "table" and key_type.sn or key_type
+ if key_type ~= dpop_openssl_key_type[expected.kty] then
+ return false, "property \"dpop.private_key\" is not an "
+ .. expected.kty .. " key, which
\"dpop.signing_alg\" \""
+ .. dpop.signing_alg .. "\" requires"
+ end
+ end
+ end
+
+ return true
+end
+
+
+-- The token endpoint only logs and falls back when its auth method cannot be
+-- used, but the PAR request fails outright (openidc.lua:547), which surfaces
+-- as a 500. PAR uses its own method when set and token_endpoint_auth_method
+-- otherwise, so validate whichever one it will actually use.
+local function check_par_auth_method(conf)
+ if not (conf.par and conf.par.enabled) then
Review Comment:
Both fixed in e2ed91e, and the second one is a good catch — I had the
fallback backwards.
`ensure_config()` assigns `opts.token_endpoint_auth_method =
openidc_get_token_auth_method(opts)` (openidc.lua:1209) before
`openidc_authorize()` runs, and that function drops an unusable method and
re-selects from the discovery document. So by the time PAR reads the field it
is usable by construction, exactly as you saw end to end. Only an explicit
`par.endpoint_auth_method` reaches `openidc_pushed_authorization_request()`
unchanged, so the credential check is now limited to that, and skipped entirely
under `bearer_only`.
The same resolution has a consequence for the algorithm-family check, which
I applied too: an unusable `token_endpoint_auth_method` is replaced before any
endpoint is called, so it can no longer conflict with the introspection method
and only counts while its credential is present.
TEST 74 keeps the explicit-method rejections, and the fallback and
bearer-only cases moved to TEST 75, where they are asserted as valid.
--
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]
