AlinsRan commented on code in PR #13649:
URL: https://github.com/apache/apisix/pull/13649#discussion_r3718312086
##########
apisix/plugins/openid-connect.lua:
##########
@@ -58,6 +58,25 @@ local function build_session_opts(session_conf)
end
+local function flatten_openidc_options(conf)
Review Comment:
Reproduced and fixed in fe0e0c6. `check_schema` accepted `{use_dpop = true,
dpop_private_key = "plaintext-key", dpop_public_jwk = {...}}`, and since
`conf.dpop` is absent `flatten_openidc_options` left all three intact, so they
reached the library with the key unencrypted — exactly as you describe.
All seven flat names the two objects own are now rejected, each pointing at
the nested option that replaces it:
```
property "use_dpop" is not allowed, use "dpop.enabled" instead
property "pushed_authorization_request_endpoint" is not allowed, use
"par.endpoint" instead
...
```
I did not use `additionalProperties: false` on the root: that would also
reject every undeclared `lua-resty-openidc` option that currently passes
through, which is a separate and much wider behavior change.
TEST 67 covers all seven names and TEST 68 is the Admin API regression.
Verified discriminating — removing the check fails both.
##########
apisix/plugins/openid-connect.lua:
##########
@@ -304,6 +323,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",
+ },
+ },
+ additionalProperties = false,
+ },
+ dpop = {
+ description = "Demonstrating Proof-of-Possession (DPoP)
configuration.",
+ type = "object",
+ properties = {
+ enabled = {
+ description = "When true, use DPoP proof JWTs.",
+ type = "boolean",
+ default = false,
+ },
+ signing_alg = {
+ description = "DPoP proof JWT signing algorithm.",
+ type = "string",
+ enum = {"ES256", "RS256", "PS256"},
+ default = "ES256",
+ },
+ private_key = {
+ description = "Private key used to sign DPoP proof JWTs.",
+ type = "string",
+ },
+ public_jwk = {
+ description = "Public JWK matching dpop.private_key.",
+ type = "object",
Review Comment:
Confirmed and fixed in fe0e0c6. `{kty = "RSA"}` passed, and so did an
unknown `kty` and `ES256` paired with an RSA JWK.
`check_schema` now requires the members `openidc_dpop_jwk_thumbprint` needs
per key type (`e`/`n` for RSA, `crv`/`x`/`y` for EC), rejects any other `kty` —
those two are all the library supports — and requires the signing algorithm to
match the key type, since `openidc_dpop_signing_params` selects the padding by
algorithm: `ES256` needs EC, `RS256`/`PS256` need RSA.
```
property "dpop.public_jwk" validation failed: kty "RSA" requires e, n
property "dpop.public_jwk" validation failed: kty "OKP" is not supported
property "dpop.signing_alg" "ES256" requires an EC "dpop.public_jwk"
```
TEST 69 and TEST 70 cover them. This did surface one thing in the existing
tests: the runtime PAR block configures an RSA JWK and was relying on the
`ES256` default, so it now sets `signing_alg` explicitly.
--
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]
