fix(cas-auth): harden session and callback handling#13427
Open
shreemaan-abhishek wants to merge 9 commits into
Open
fix(cas-auth): harden session and callback handling#13427shreemaan-abhishek wants to merge 9 commits into
shreemaan-abhishek wants to merge 9 commits into
Conversation
Move the HMAC verification of the CAS_REQUEST_URI cookie above the session-write so the callback only creates a session when the request carries a valid initiation cookie. Previously a missing or invalid cookie silently fell back to redirecting to /, and the session was still written.
Each route's cas-auth session is now keyed under a cookie name derived from its idp_uri and cas_callback_uri via SHA-256, and the session payload stored in the shared dict carries the same fingerprint. Sessions issued by one route are no longer honoured by a route configured with a different idp_uri or cas_callback_uri. This is a behaviour change: existing CAS_SESSION cookies and any unfingerprinted entries in the cas_sessions shared dict are not recognised after upgrade. Affected users will go through their route's CAS flow once to re-establish a session.
shreemaan-abhishek
changed the title
The mock upstream in t/lib/server.lua dispatches by mapping the URI to a function name in `_M`; an undefined action returns 404. Route the scope test's Route A at /hello and Route B at /uri (both defined), and split the setup into its own test block so the route create runs in a separate request from the flow assertions.
Capture ok/err/forcible from store:set during session refresh and log the failure or forcible-eviction cases instead of swallowing them silently. Mirrors the handling already present in set_store_and_cookie.
Prefix every store:add/get/set/delete key with "<fingerprint>:" so that tickets issued independently by different CAS IdPs cannot collide in cas_sessions. Without the namespace, two routes pointing at different IdPs that happen to receive the same ticket string would have the second validation fall into the "exists" branch and return 401 even though the configurations are unrelated. Cookie value remains the raw ticket; only the server-side store key changes, so this is transparent to clients. The fingerprint check on the stored value stays in place as defence in depth. Also extends the callback-gate tests (TEST 15, TEST 16 in apisix) to assert no shared-dict entry is written for ST-test under any fingerprint namespace, not just that no Set-Cookie was emitted.
Replace the synthetic unit tests for session_opts / pack_entry /
unpack_entry, and the planted-session integration test that depended
on test-only exports, with a single end-to-end test driven through the
real Keycloak fixture:
1. Log in via sp1 (cas_callback_uri = /cas_callback).
2. Confirm the session works on sp1 (sanity check).
3. Replay the same cookie against a route configured with
cas_callback_uri = /cas_callback_alt (different fingerprint) and
assert it is redirected to its own IdP rather than honoured.
Step 1 exercises set_store_and_cookie -> pack_entry; step 2 exercises
with_session_id -> unpack_entry -> fingerprint check; step 3 exercises
session_opts producing distinct cookie names per CAS configuration.
Any regression in those helpers fails this test.
With the integration test covering the same surface, the test-only
exports of session_opts, pack_entry and unpack_entry can be dropped
from _test_helpers.
The Keycloak-driven flow worked locally and on the apache/apisix CI
but failed on the EE pipeline at the kc.login_keycloak step, where
the shared Keycloak fixture's state after the preceding cas-auth tests
caused the login URL to return a non-200 response that the helper
treats as failure.
Replace the Keycloak login with a synthetic session-plant that mirrors
what the plugin would write (key "<fp>:<ticket>", value "<fp>|<user>"),
recomputing the per-config SHA-256 fingerprint inline rather than
exposing the plugin's session_opts helper. Three assertions:
1. Route sp1 honours its own session (exercises with_session_id ->
store:get -> unpack_entry -> fingerprint check).
2. Sending sp1's cookie name to the diff-cb route hits a different
cookie_name on read and falls through to first_access (302).
3. A cookie forged under diff-cb's own name pointing at sp1's ticket
misses the namespaced store key under fp_b and also redirects.
Plugin code and _test_helpers exports are unchanged.
dosubot
Bot
added
the
size:L
The earlier version of this test sent requests to sp1 (host=127.0.0.1)
expecting the cas1 route from TEST 1 to match. After TEST 12 registers
the cas-abs route with no host filter, two routes match those requests
and the radixtree's tiebreak is non-deterministic across environments
(passed locally and on apache CI but matched cas-abs on EE CI).
Register two dedicated routes (cas-scope-a at 127.0.0.10 and
cas-scope-b at 127.0.0.11) with priority=10 so the new test's routing
is unambiguous regardless of other routes registered earlier in the
file. Plant a session for scope-a's fingerprint and assert that:
1. scope-a honours its own session (upstream returns 200).
2. Sending scope-a's cookie name to scope-b hits scope-b's own
cookie_name lookup and falls through to first_access (302).
3. A cookie forged under scope-b's cookie name pointing at scope-a's
ticket misses the fingerprint-namespaced store key and also
redirects.
Also wrap session-cookie reads in the plugin with a get_cookie helper
that falls back to manual Cookie-header parsing when nginx's
$cookie_<name> doesn't return a value, for portability across the
OpenResty versions in CI.
The prior step-3 assertion exercises the store-key namespacing (scope-b's namespaced lookup misses), but the fingerprint check inside with_session_id never fires because the entry is not found. Add a fourth step that plants under scope-b's namespaced key with scope-a's fingerprint embedded in the stored value, so the plugin retrieves the entry, calls unpack_entry, and rejects on the fp mismatch -> 302.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
cas-authplugin has two related callback/session weaknesses that this PR addresses together. Both are behaviour changes; normal end-to-end CAS flows are unaffected.1. Require a valid initiation cookie at the callback (commit
05841df4a)validate_with_caspreviously wrote the session and then attempted to read the HMAC-signedCAS_REQUEST_URIcookie to decide where to redirect. If that cookie was missing or its signature did not verify, the handler fell back to redirecting to/, but the session was still created.validate_with_casnow verifies the HMAC ofCAS_REQUEST_URIfirst and refuses to create a session if the cookie is missing or invalid. A request that reaches/cas_callbackwithout going throughfirst_accessnow returns401 invalid callback stateand writes no session cookie.Builds on the HMAC cookie machinery (
sign_value/verify_value,is_safe_redirect,cookie.secretschema field,SameSite=Lax) introduced in #13331.2. Scope the session cookie per CAS configuration (commit
ff2e242ca)Every route using
cas-authpreviously shared a single fixed cookie nameCAS_SESSIONand a single globalcas_sessionsshared dict keyed by ticket, regardless of the route'sidp_uri/cas_callback_uri. A browser session minted under one route's CAS configuration was therefore reused by any othercas-authroute on the same host, even if that route pointed at a different IdP.Each route now uses a cookie name derived from
sha256(idp_uri || cas_callback_uri)(CAS_SESSION_<hex>), and the shared-dict entry carries that fingerprint as a prefix.with_session_idrejects entries whose stored fingerprint does not match the current route's fingerprint. Mirrors the per-client_idpattern already used byauthz-casdoor.Breaking changes
/cas_callback?ticket=...directly without going throughfirst_accessnow receive401 invalid callback state. Normal browser CAS flows always go throughfirst_access, which sets the signed initiation cookie before the IdP redirect, so they are unaffected.CAS_SESSIONcookies and any unfingerprinted entries in thecas_sessionsshared dict are not recognised after upgrade. End users with a live session at upgrade time are redirected once through the IdP to re-establish a session under the new cookie name.Which issue(s) this PR fixes:
Fixes #
Checklist
Notes:
t/plugin/cas-auth.t:session_optsderives distinct cookie names + fingerprints per CAS configuration), TEST 18 (pack_entry/unpack_entryroundtrip and rejection of legacy entries), TEST 19 (two-route integration: a session minted under one CAS configuration is not honoured under a different configuration). It also updates the^CAS_SESSION=assertion in TEST 15 / TEST 16 to^CAS_SESSION_to match the new cookie shape.