AlinsRan commented on code in PR #13649:
URL: https://github.com/apache/apisix/pull/13649#discussion_r3671970343
##########
apisix-master-0.rockspec:
##########
@@ -51,7 +51,7 @@ dependencies = {
"opentracing-openresty = 0.1-0",
"lua-resty-radixtree = 2.9.2-0",
"lua-protobuf = 0.5.3-1",
- "lua-resty-openidc = 1.8.0-1",
+ "lua-resty-openidc = 1.9.0-1",
Review Comment:
The 1.8.0 -> 1.9.0 bump also changes behavior for existing configurations,
beyond the opt-in PAR/DPoP options.
1.8.0 `openidc.introspect()` unconditionally put `client_id`/`client_secret`
in the introspection POST body. 1.9.0 only does that when
`introspection_endpoint_auth_method` is nil (`openidc.lua:2337`). APISIX
defaults that field to `client_secret_basic`
(`apisix/plugins/openid-connect.lua:78-81`), and the default reaches the conf
that is handed to the library on both paths: Admin API
(`apisix/admin/resource.lua:258` validates and `:314` stores the same table)
and config load (`apisix/http/route.lua:122` -> `plugin_checker` ->
`check_schema`), since the schema validator injects defaults in place. So after
this bump every route that validates tokens through introspection stops sending
credentials in the POST body; only the Basic header remains.
For an OP that authenticates the introspection call from the body, this
breaks with a completely unchanged APISIX configuration. The workaround is
`introspection_endpoint_auth_method: "client_secret_post"`, but users have no
way to know that from this PR - worth an upgrade note in the docs or at least
in the PR description.
I checked the other 1.9.0 changes as well: negative introspection caching is
opt-in via `introspection_enable_negative_cache` (default false), so that one
is not a behavior change.
##########
t/plugin/openid-connect.t:
##########
@@ -1845,7 +1849,278 @@ done
-=== TEST 51: Configure plugin with a custom session.cookie_name.
+=== TEST 51a: Accept PAR, DPoP, and client assertion algorithm options.
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local ok, err = plugin.check_schema({
+ client_id = "a",
+ discovery =
"https://example.com/.well-known/openid-configuration";,
+ bearer_only = false,
+ use_pkce = true,
+ par = {
+ enabled = true,
+ endpoint = "https://example.com/par";,
+ endpoint_auth_method = "private_key_jwt",
+ },
+ dpop = {
+ enabled = true,
+ signing_alg = "PS256",
+ private_key = "-----BEGIN PRIVATE
KEY-----\nMIIEowIBAAK\n-----END PRIVATE KEY-----",
+ public_jwk = {
+ kty = "RSA",
+ e = "AQAB",
+ n = "abc",
+ },
+ },
+ token_endpoint_auth_method = "private_key_jwt",
+ client_rsa_private_key = "-----BEGIN RSA PRIVATE
KEY-----\nMIIEowIBAAK\n-----END RSA PRIVATE KEY-----",
+ client_jwt_assertion_alg = "PS256",
+ client_jwt_assertion_audience =
"https://issuer.example.com/token";,
+ session = { secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK" },
+ })
+ if not ok then
+ ngx.say(err)
+ end
+ ngx.say("done")
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 52b: Reject unsupported DPoP signing algorithm in schema.
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local ok, err = plugin.check_schema({
+ client_id = "a",
+ client_secret = "b",
+ discovery =
"https://example.com/.well-known/openid-configuration";,
+ dpop = {
+ signing_alg = "HS256",
+ },
+ session = { secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK" },
+ })
+ if not ok then
+ ngx.say(err)
+ end
+ ngx.say("done")
+ }
+ }
+--- response_body
+property "dpop" validation failed: property "signing_alg" validation failed:
matches none of the enum values
+done
+
+
+
+=== TEST 53c: Accept PAR enabled without endpoint in schema.
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local ok, err = plugin.check_schema({
+ client_id = "a",
+ client_secret = "b",
+ discovery =
"https://example.com/.well-known/openid-configuration";,
+ par = {
+ enabled = true,
+ },
+ session = { secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK" },
+ })
+ if not ok then
+ ngx.say(err)
+ end
+ ngx.say("done")
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 54d: Reject DPoP enabled without key material in schema.
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local ok, err = plugin.check_schema({
+ client_id = "a",
+ client_secret = "b",
+ discovery =
"https://example.com/.well-known/openid-configuration";,
+ dpop = {
+ enabled = true,
+ },
+ session = { secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK" },
+ })
+ if not ok then
+ ngx.say(err)
+ end
+ ngx.say("done")
+ }
+ }
+--- response_body
+property "dpop" validation failed: then clause did not match
+done
+
+
+
+=== TEST 55e: Reject private key material in DPoP public JWK.
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.openid-connect")
+ local ok, err = plugin.check_schema({
+ client_id = "a",
+ client_secret = "b",
+ discovery =
"https://example.com/.well-known/openid-configuration";,
+ dpop = {
+ enabled = true,
+ private_key = "-----BEGIN PRIVATE
KEY-----\nMIIEowIBAAK\n-----END PRIVATE KEY-----",
+ public_jwk = {
+ kty = "RSA",
+ e = "AQAB",
+ n = "abc",
+ d = "private-exponent",
+ },
+ },
+ session = { secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK" },
+ })
+ if not ok then
+ ngx.say(err)
+ end
+ ngx.say("done")
+ }
+ }
+--- response_body_like
+property "dpop" validation failed: property "public_jwk" validation failed:.*
+done
+
+
+
+=== TEST 56: PAR runtime mapping sends authorization parameters through PAR.
+--- http_config
+ server {
+ listen 16969;
+ server_name localhost;
+
+ location /.well-known/openid-configuration {
+ content_by_lua_block {
+ ngx.header.content_type = "application/json"
+ ngx.say([[{
+ "issuer": "http://127.0.0.1:16969";,
+ "authorization_endpoint":
"http://127.0.0.1:16969/authorize";,
+ "token_endpoint": "http://127.0.0.1:16969/token";,
+ "userinfo_endpoint": "http://127.0.0.1:16969/userinfo";,
+ "jwks_uri": "http://127.0.0.1:16969/jwks";
+ }]])
+ }
+ }
+
+ location /par {
Review Comment:
This runtime block covers the PAR mapping, but the four `dpop_*` assignments
in `flatten_openidc_options` are still only covered by schema tests - a typo
there, or an option rename upstream, would leave DPoP silently disabled with CI
green.
This mock can cover most of it at no extra cost: when `use_dpop` is set,
lua-resty-openidc adds `params.dpop_jkt` before pushing to the PAR endpoint
(`openidc.lua:651-657`), so enabling `dpop` in the route config and asserting
`args.dpop_jkt` here exercises the `use_dpop` and `dpop_public_jwk` mappings.
No token endpoint mock is needed: `dpop_private_key` is only read in
`openidc_dpop_proof()` (`openidc.lua:232`), so a placeholder value is enough to
satisfy the schema, and the thumbprint only requires `e`/`n` to be present on
the RSA JWK - it does not decode them.
##########
apisix/plugins/openid-connect.lua:
##########
@@ -371,6 +454,14 @@ local schema = {
type = "integer",
default = 60
},
+ client_jwt_assertion_alg = {
Review Comment:
No enum here, while `dpop.signing_alg` added in this PR has one. An invalid
value is only caught per request by the library (`openidc.lua:836-844`:
symmetric alg rejected for `private_key_jwt`, asymmetric for
`client_secret_jwt`, plus the
`token_endpoint_auth_signing_alg_values_supported` check), so a typo becomes a
runtime authentication failure instead of a config-time rejection. Constraining
it in the schema would match the new `dpop.signing_alg`.
For balance: `token_endpoint_auth_method` and
`introspection_endpoint_auth_method` in this schema are plain strings too, so
this is not an established convention in this file - take it as a suggestion.
--
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]
