nic-6443 commented on code in PR #13649:
URL: https://github.com/apache/apisix/pull/13649#discussion_r3727774941
##########
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
Review Comment:
`get_key_type()` is not enough to make this a usable DPoP private key. I
reproduced three configurations that still pass `check_schema`: a public-only
RSA PEM (`pkey.new` accepts it and reports `rsaEncryption`, but `key:sign`
fails), a P-384 EC private key under ES256, and a different P-256 private key
from the configured JWK. In the latter two cases the proof advertises the
supplied JWK while the signature comes from another key, so the OP cannot
verify it. Please require `key:is_private()` and compare the public JWK derived
from the key (`resty.openssl.auxiliary.jwk.dump_jwk(key, false)`) with the
configured `kty`/`crv`/`x`/`y` or `e`/`n`; this also checks the private EC
curve. Please add public-only and mismatched-key regressions.
##########
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:
`bearer_only` does not always call introspection. With `public_key` or
`use_jwks`, `introspect()` takes the local `bearer_jwt_verify` branch and never
reads `introspection_endpoint_auth_method`. I reproduced `{bearer_only = true,
public_key = "<valid public key>", introspection_endpoint_auth_method =
"client_secret_jwt", client_jwt_assertion_alg = "RS256"}`: it worked before
this commit but is now rejected. Please add the introspection selection only
when local verification is not configured, with cases for both `public_key` and
`use_jwks`.
##########
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:
`check_par_auth_method` also needs the authorization-flow reachability guard
used below. `bearer_only` never enters the authorization code flow, so PAR
cannot be called. I reproduced `{bearer_only = true, use_jwks = true, par =
{enabled = true, endpoint_auth_method = "private_key_jwt"}}`: it is rejected
for a missing `client_rsa_private_key` even though runtime never reaches PAR.
Please skip this check under `bearer_only`, and add a regression.
--
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]
