AlinsRan commented on code in PR #13649:
URL: https://github.com/apache/apisix/pull/13649#discussion_r3733735587
##########
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:
All three reproduced and fixed in e2ed91e. You are right that
`get_key_type()` is the wrong granularity — the third case is the one that
convinced me, since a key of the right type that simply is not the pair of the
configured JWK produces a proof the OP cannot verify, and nothing downstream
would say why.
The check now requires `key:is_private()` and compares the JWK openssl
derives from the key (`dump_jwk(key, false)`) against the configured `kty` plus
the members that key type needs. That subsumes the type and curve checks, so
the separate ones are gone:
```
property "dpop.private_key" has no private key in it
property "dpop.public_jwk" is not the public key of "dpop.private_key": x is
"w_cMR6PybrVWcfO1..."
property "dpop.public_jwk" is not the public key of "dpop.private_key": crv
is "P-384"
```
`dump_jwk` is called through `pcall` since it lives under `auxiliary`, so an
API change there degrades to a clear error rather than breaking config loading.
TEST 79 covers public-only, mismatched-pair and wrong-curve.
##########
apisix/plugins/openid-connect.lua:
##########
@@ -383,6 +514,22 @@ local schema = {
type = "integer",
default = 60
},
+ -- resty.jwt signs the client assertion and raises an uncaught Lua
+ -- error for an algorithm it cannot handle, so an unconstrained value
+ -- would surface as a 500 per request instead of a rejected config.
+ -- Two rocks provide resty/jwt.lua here: the api7-lua-resty-jwt this
+ -- rockspec pins, and the lua-resty-jwt lua-resty-openidc depends on.
+ -- This enum is what the api7 fork signs, a subset of what the other
+ -- one signs, so it holds whichever of the two ends up installed.
+ client_jwt_assertion_alg = {
+ description = "Signing algorithm for the client assertion JWT.",
+ type = "string",
+ enum = {"HS256", "HS512", "RS256", "RS512", "ES256", "ES512"}
Review Comment:
Reproduced, and it is worse than a 500 — I confirmed the SIGSEGV. Signing
with `alg = ES256` and an RSA key under the pinned `api7-lua-resty-jwt` kills
the process outright:
```
$ resty sign-es256-with-rsa.lua
STEP1 loaded key
STEP2 required resty.jwt
exit code=139 (killed by signal 11)
```
Fixed in e2ed91e by validating the key against the algorithm rather than
restricting the field to RSA. `client_rsa_private_key` is the only key input
for `private_key_jwt`, and an EC key in it is legitimate for the `ES*`
algorithms the enum already allows, so narrowing the enum would have removed
working configurations; requiring the key to match keeps them and closes the
crash:
```
property "client_jwt_assertion_alg" "ES256" requires an EC
"client_rsa_private_key"
property "client_jwt_assertion_alg" "ES512" requires a
"client_rsa_private_key" on curve "P-521", got "P-256"
```
The curve half covers your P-256/ES512 case. This only runs when
`private_key_jwt` is actually reachable, and it skips secret references. TEST
80 covers RSA+ES256, P-256+ES512 and EC+RS256.
--
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]
