feat(openid-connect): support PAR and DPoP client options - #13649
feat(openid-connect): support PAR and DPoP client options#13649kevinlzw wants to merge 10 commits into
Conversation
dosubot
Bot
added
size:L
|
This PR should also fix #13085. |
| }, | ||
| public_jwk = { | ||
| description = "Public JWK matching dpop.private_key.", | ||
| type = "object", |
There was a problem hiding this comment.
I verified a JWK carrying private key material passes check_schema here — {kty = "RSA", e = "AQAB", n = "...", d = "..."} is accepted. Since public_jwk is (rightly) not in encrypt_fields, a user who pastes their complete JWK ends up with the private exponent stored in plaintext in etcd, and the mistake only surfaces per request when lua-resty-openidc rejects the proof. Rejecting private JWK fields at config time keeps the key out of etcd and fails fast:
public_jwk = {
description = "Public JWK matching dpop.private_key.",
type = "object",
["not"] = {anyOf = {
{required = {"d"}}, {required = {"p"}}, {required = {"q"}},
{required = {"dp"}}, {required = {"dq"}}, {required = {"qi"}},
{required = {"oth"}}, {required = {"k"}},
}},
},Same field list as upstream's openidc_dpop_public_jwk_private_field; I checked the not/anyOf combo works with the jsonschema library APISIX uses.
There was a problem hiding this comment.
Thanks for the detailed catch. I added the schema-side rejection for private JWK members (d, p, q, dp, dq, qi, oth, and k) under dpop.public_jwk, so complete/private JWKs now fail check_schema before anything can be persisted in etcd. I also added a regression case that verifies a JWK containing d is rejected.
| end | ||
|
|
||
|
|
||
| local function flatten_openidc_options(conf) |
There was a problem hiding this comment.
The new tests are all schema-level, so nothing exercises this mapping at runtime — a typo in any of these assignments (or an option rename on the lua-resty-openidc side) would still pass CI. A single e2e block would cover it: serve a fake discovery doc plus a PAR mock from the test nginx (t/plugin/openid-connect-redis.t already does the fake-discovery part with a local authorization_endpoint), enable par with endpoint pointing at the mock, hit the route, and assert the 302 Location carries only client_id and request_uri and that scope/state stay out of the query. That one test proves the PAR wiring and the flattening end to end.
There was a problem hiding this comment.
Thanks, that makes sense. I added a Test::Nginx runtime block with a fake discovery document and a fake PAR endpoint, then configured the plugin through the Admin API and hit the route to exercise the actual flattening path. The test now parses the final authorize redirect and asserts the query shape exactly: only client_id and request_uri are present, while scope, state, response_type, and redirect_uri are absent.
| ngx.say(conf.client_rsa_private_key ~= "89ae4c8edadf1cd1c9f034335f136f87ad84b625c8f1") | ||
| ngx.say(conf.dpop.private_key ~= "dpop-private-key") |
There was a problem hiding this comment.
These ~= checks turn vacuously true when the field comes back nil, so a value dropped somewhere between admin API and etcd would read as "encrypted". The encryption in tests is deterministic (fixed key), so pinning the ciphertext like the old test did still works; if you'd rather avoid that brittleness, something like type(conf.dpop.private_key) == "string" and conf.dpop.private_key ~= "dpop-private-key" keeps the nil case failing.
There was a problem hiding this comment.
Good point. I updated the assertions to require the stored values to be strings before comparing them with the plaintext values, so a missing/nil value no longer passes as encrypted. I kept it value-agnostic rather than pinning the deterministic ciphertext, to avoid making the test depend on the exact encrypted blob.
dosubot
Bot
added
size:XL
… option mapping
Three follow-ups from reviewing this PR.
`client_jwt_assertion_alg` accepted any string, but lua-resty-openidc signs
the client assertion with `r_jwt:sign()` and no pcall, and resty.jwt ends in
`error({reason="unsupported alg: "..alg})` for anything it does not handle.
An unsupported value is therefore a 500 with a stack trace on every request,
not a failed authentication. `PS256`, which the docs example and TEST 51a
used, is one of those: the rockspec pins api7-lua-resty-jwt, whose sign()
handles only HS256/HS512/RS256/RS512/ES256/ES512. lua-resty-openidc pulls in
lua-resty-jwt, which does support PS256, but both rocks install the same
resty/jwt.lua and installing them in either order leaves the pinned one in
place, so the effective set is the smaller one. Constrained by an enum, and
the docs example and test now use RS512. dpop.signing_alg is unaffected --
DPoP proofs are signed through resty.openssl.pkey, and that enum already
matches the library's supported_dpop_signing_algs.
The dpop_* mappings had no coverage. `dpop_jkt` on the PAR mock only reaches
`use_dpop` and `dpop_public_jwk`; a DPoP proof is built only when
`ep_name == "token"`, which the PAR call never is, so `dpop_private_key` and
`dpop_signing_alg` were never exercised. flatten_openidc_options is exported
the way _build_session_opts already is, and TEST 62/63 assert all six flat
names plus the clearing of conf.par/conf.dpop.
TEST 56 asserted par.endpoint_auth_method vacuously: dropping the mapping
makes lua-resty-openidc fall back to token_endpoint_auth_method, which
defaults to client_secret_basic, and the mock accepted that too. The mock
now requires the credentials in the body, which only client_secret_post
sends. Breaking any of the six mappings now fails a test.
TEST 64 pins the introspection change the 1.9.0 bump brings for unmodified
configurations, and both docs get an upgrade note for it: the credentials
now travel in the Authorization header under the default and under
client_secret_basic, and reach the body only under client_secret_post.
# Conflicts: # t/plugin/openid-connect.t
…flows lua-resty-openidc 1.9.0 keeps one authorization state per in-flight flow instead of a single state per session, so a second browser tab no longer invalidates the first tab's callback. t/plugin/openid-connect11.t drove the state mismatch by opening a second tab, which stopped producing one: TEST 2 got a 500 from the token endpoint instead of the redirect, and TEST 3 kept passing while no longer reaching the branch it guards -- it passes with the GET-only condition removed. Both now use a state the session never issued, which is what a replayed or pruned callback looks like under 1.9.0. Verified discriminating: dropping the state-mismatch branch fails TEST 2, dropping the GET-only condition fails TEST 3. TEST 5 covers the behavior 1.9.0 adds -- the first tab's callback is still accepted after a second tab starts its own flow. It keeps the first tab's cookie when the second flow reuses the session, since losing the session would silently turn it into the no-session case of TEST 4, and asserts the token-endpoint failure that proves the state was accepted rather than rejected. The comment on the redirect branch is updated: concurrent logins in several tabs are now handled by the library, and the branch is left for replayed or pruned callbacks.
membphis
left a comment
membphis
left a comment
There was a problem hiding this comment.
[P1] Preserve shared verification caching for bearer JWTs
With lua-resty-openidc 1.9.0, the existing bearer_jwt_verify(conf, opts) call passes claim-verification options that bypass the shared JWT verification cache. Repeated requests carrying the same token therefore perform cryptographic verification on every request instead of hitting the cache, affecting both public_key and use_jwks paths.
This is a merge blocker because it turns cache hits into per-request cryptographic verification on existing configurations. Please adapt the integration so claim validation remains correct without disabling shared verification caching, and add a repeated-token regression proving that the second request uses the cache.
4 participants
Description
I maintain
lua-resty-openidc, and the latestlua-resty-openidc1.9.0 release added client-side PAR and DPoP support. This PR bumps APISIX to that release and exposes the new client-side OAuth/OIDC options through theopenid-connectPlugin.This PR adds nested APISIX Plugin configuration for:
The APISIX Plugin keeps user-facing PAR and DPoP options grouped under
paranddpop, then maps them to the flat option names expected bylua-resty-openidcbefore invoking the library. It also encryptsdpop.private_keyin etcd and documents the new options in English and Chinese.Related to #11219. This PR covers the
openid-connectPlugin acting as an OAuth/OIDC client/Relying Party. It does not implement APISIX resource-server-side DPoP proof validation.Backward compatibility
This change is backward compatible for existing
openid-connectPlugin configurations:par,dpop, or client assertion algorithm attributes.dpop.private_keyencryption only affects the new DPoP private-key field.Which issue(s) this PR fixes:
Fixes #13085
Related to #11219
Checklist
Tests
git -C apisix diff --checkapisix/plugins/openid-connect.luaprove -I../test-nginx/lib -I./ -r -s t/plugin/openid-connect.tFiles=1, Tests=175, Result: PASSmake lintwas run in a Docker temporary copy of the Windows checkout after normalizing CRLF line endings and installingluacheckin the temporary container:luacheck -q apisix t/lib:0 warnings / 0 errors in 376 fileslj-relengstill fails on existing repository-wide line-length/style findings, including pre-existingopenid-connect.lualong lines not introduced by this PR.