moonming commented on PR #13651:
URL: https://github.com/apache/apisix/pull/13651#issuecomment-6886205955
Thanks for the update, @Hockenba. Keeping revocation opt-in and implementing
it in the session library makes sense. I reviewed `dfccf896` and found two
cases that need addressing before merging. Could you try reproducing and fixing
these, upstream where appropriate?
1. **Logout can rotate the session before revoking it, leaving the incoming
cookie usable.**
In openidc 1.9.3, [`authenticate()` starts the session before checking
the logout
path](https://github.com/zmartzone/lua-resty-openidc/blob/v1.9.3/lib/resty/openidc.lua#L2086).
`session.start()` can refresh the session, and [rolling refresh calls
`save()`](https://github.com/bungle/lua-resty-session/blob/v4.3.0/lib/resty/session.lua#L2236),
generating a new SID. `destroy()` then revokes that new SID rather than the
one in the incoming cookie.
Reproduction with real Keycloak and Redis, complete session 4.3.0 and
openidc 1.9.3:
- Set `rolling_timeout=20`, `revocation="redis"`, and
`revocation_fail_mode="closed"`; leave cache TTLs at their defaults.
- Log in and retain the authenticated cookie.
- Wait 16 seconds, then call `/logout` with that cookie: **302**.
- Replay the same cookie against the protected endpoint: **200**,
including the protected response.
- Control: logging out immediately causes replay to return **302**, as
expected.
This was a real HTTP integration harness calling `openidc.authenticate()`
directly, not the full APISIX plugin chain. The shorter rolling timeout was
deliberate; no fake clock or mocked services were used. Please avoid automatic
refresh when opening a session for logout, preserve fail-closed error
propagation, and add a real Keycloak/Redis regression in APISIX. Using
`session.open()` instead of `session.start()` on the logout path looks like a
narrowly scoped fix. Revoking all historical cookies from earlier saves is a
separate limitation and need not be folded into this fix.
2. **A fail-open cache entry can be reused by a fail-closed configuration.**
The [4.3.0 cache
key](https://github.com/bungle/lua-resty-session/blob/v4.3.0/lib/resty/session.lua#L381)
contains only the cookie name and hashed SID. A store error under `open` is
cached as `false`, indistinguishable from a successful not-revoked lookup.
With real Redis stopped and default 5-second cache TTLs, I observed:
- A fresh cookie opened under `closed`: rejected with a revocation-store
error.
- Another cookie opened under `open`: accepted.
- That same cookie immediately opened under `closed` on the same worker:
**accepted with no error**.
- After 5.2 seconds, `closed` rejects it again.
This was reproduced at the session-library level. It matters when
configurations share a cookie name and secret but use different failure
policies. Please isolate cache entries by backend/configuration and ensure
cached failures are interpreted according to the current policy, with a
regression covering this case.
Please also qualify the documentation's maximum-staleness claim: the cache
TTL bounds propagation of a successfully written marker for the same SID; it
does not cover older rotated SIDs or failed fail-open writes. The current TEST
4 error stub and TEST 5 backend stub do not exercise the real dependency paths
above, so please add real-service coverage and rerun the full
`openid-connect*.t` suite after the fixes.
--
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]
