nic-6443 commented on code in PR #13649:
URL: https://github.com/apache/apisix/pull/13649#discussion_r3727775262
##########
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:
This selection still does not match the runtime reachability of the
introspection endpoint. `introspection_endpoint_auth_method` is read only when
`introspect()` takes its remote branch: `public_key` or `use_jwks` always takes
local JWT verification, and in non-bearer mode with no explicit
`introspection_endpoint`, the outer condition in `rewrite()` does not call
`introspect()` at all. I reproduced both the local-verification case and a
non-bearer configuration with token `private_key_jwt`, introspection
`client_secret_jwt`, no explicit introspection endpoint, and `RS256`; both are
rejected even though the conflicting introspection method is unreachable.
Please add this selection only when local verification is absent and
`(conf.bearer_only or conf.introspection_endpoint)` is true, with regressions
for 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:
`check_par_auth_method` has two reachability problems. First, `bearer_only`
never enters the authorization code flow, so PAR cannot be called and this
check must be skipped. Second, when `par.endpoint_auth_method` is unset,
`ensure_config()` resolves `token_endpoint_auth_method` before
`openidc_authorize()` calls PAR. I verified this end to end against 1.9.0:
configured `private_key_jwt` without a key plus discovery advertising
`client_secret_post` successfully sends the PAR request with
`client_secret_post`; the current check rejects it first. Only an explicit PAR
auth override reaches `openidc_pushed_authorization_request()` unchanged and
fails its `can_use_token_auth_method` check. Please restrict the credential
validation to non-bearer PAR with an explicit `par.endpoint_auth_method`, and
add regressions for the bearer-only and discovery-fallback cases.
##########
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:
The new enum admits asymmetric algorithms without checking that the
configured key can use them. `private_key_jwt` with the documented RSA PEM and
`ES256` passes `check_schema`; with the pinned `api7-lua-resty-jwt`, signing
that combination terminates the OpenResty process with SIGSEGV in the EC
signature conversion. A P-256 key with `ES512` also passes and emits a
P-256-sized signature even though ES512 requires P-521. Please either keep this
RSA-named field to RSA algorithms, or validate that ES uses a private EC key on
the required curve before accepting the config. Add regressions for RSA plus
ES256 and P-256 plus ES512.
##########
docs/en/latest/plugins/openid-connect.md:
##########
@@ -60,6 +60,15 @@ The `openid-connect` Plugin supports the integration with
[OpenID Connect (OIDC)
| public_key | string | False | | | Public key used to verify JWT signature if
asymmetric algorithm is used. Providing this value to perform token
verification will skip token introspection in client credentials flow. You can
pass the public key in `-----BEGIN PUBLIC KEY-----\n……\n-----END PUBLIC
KEY-----` format. |
| use_jwks | boolean | False | false | | If true and if `public_key` is not
set, use the JWKS to verify JWT signature and skip token introspection in
client credentials flow. The JWKS endpoint is parsed from the discovery
document. |
| use_pkce | boolean | False | false | | If true, use the Proof Key for Code
Exchange (PKCE) for Authorization Code Flow as defined in [RFC
7636](https://datatracker.ietf.org/doc/html/rfc7636). |
+| par | object | False | | | Pushed Authorization Requests (PAR)
configuration. |
+| par.enabled | boolean | False | false | | If true, use OAuth 2.0 Pushed
Authorization Requests (PAR) as defined in [RFC
9126](https://datatracker.ietf.org/doc/html/rfc9126). Authorization request
parameters are sent to the PAR endpoint and the browser is redirected with the
returned `request_uri`. |
+| par.endpoint | string | False | | | URL of the PAR endpoint. If unset, the
endpoint from the well-known discovery document is used. |
+| par.endpoint_auth_method | string | False | | | Authentication method for
the PAR endpoint. If unset, `token_endpoint_auth_method` is used. |
Review Comment:
This new PAR method has a four-value enum and credential requirements, but
the documentation leaves the valid-values column empty. The shared credential
rows below are now inaccurate too: `client_rsa_private_key`, its key ID, and
the assertion lifetime are described as applying only to
`token_endpoint_auth_method`, while lua-resty-openidc also uses them for PAR
and introspection JWT authentication. Please document the enum and credential
mapping, update the shared assertion-field descriptions, and mirror the changes
in the Chinese document; otherwise a PAR-only `private_key_jwt` configuration
is documented as not needing the key that `check_schema` requires.
--
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]
