feat(cas-auth): sign request URI cookie and tighten cookie attributes#13331
Open
shreemaan-abhishek wants to merge 6 commits into
Open
feat(cas-auth): sign request URI cookie and tighten cookie attributes#13331shreemaan-abhishek wants to merge 6 commits into
shreemaan-abhishek wants to merge 6 commits into
Conversation
The CAS_REQUEST_URI cookie used by the plugin to remember the pre-login URL is now signed with HMAC-SHA256 and verified on the IdP callback using a constant-time comparison. The recovered redirect target is also validated as a same-origin path before being applied; on mismatch the plugin falls back to /. Cookie attributes are tightened to include Secure; SameSite=Lax. Adds a required cookie_secret field (minLength 32) to the plugin schema. The same value must be configured on every APISIX node.
- Move cookie options under a nested cookie object: cookie.secret, cookie.secure (default true), cookie.samesite (default Lax, enum Lax|None). Strict is intentionally omitted because it breaks the IdP->SP cross-site redirect. - Mark cookie.secret as encrypt_fields so it is encrypted at rest in etcd, matching how other plugins protect plugin secrets. - Guard first_access against sign_value returning nil so a transient HMAC failure cannot crash the request with a Lua concatenation error; fall through and let the safe-redirect fallback take over. - Capture and log the error from compute_hmac in verify_value rather than dropping it. - Build cookie attributes per-config so deployments that legitimately run over HTTP can disable Secure without giving up SameSite. - Strip session identifiers, ticket values, and full request URIs from info-level logs; demote raw SLO LogoutRequest body to debug. - Update t/lib/keycloak_cas.lua and t/plugin/security-warning.t for the new schema; add unit-style tests for the schema, the safe redirect predicate, and the HMAC sign/verify roundtrip with tampering, wrong-secret, and malformed-input cases.
- Move encrypt_fields = {"cookie.secret"} from the plugin module
table into the schema table. The framework's encryption pipeline
reads schema.encrypt_fields (apisix/plugin.lua); declaring it on
_M was silently a no-op and meant cookie.secret was being stored
in plaintext at rest in etcd despite the previous claim. Other
plugins (basic-auth, openid-connect, etc.) place it on the schema
table for the same reason.
- Drop the debug-level log of the raw SLO LogoutRequest body. Even
at debug level, the body contains the SAML SessionIndex (the
ticket value the plugin reuses as a session id), which is
authentication material that should not be written to logs at any
level. The "SLO request received from IdP" info log already
captures the operational signal; raw-body inspection can be done
via packet capture if ever needed.
membphis
requested changes
May 11, 2026
Member
membphis
left a comment
There was a problem hiding this comment.
This is a breakchange
This point must be clearly stated in the PR description.
apisix/plugins/jwt-auth/parser.lua already uses the resty.openssl.mac.new(key, "HMAC", nil, "sha256"):final(data) shape for HMAC computation against the same lua-resty-openssl dependency. Switch cas-auth's compute_hmac to the same module and shape so the codebase has a single style for HMAC via openssl rather than two flavours of the same dependency.
- Reject cookie.samesite="None" combined with cookie.secure=false in check_schema. Browsers refuse to store SameSite=None cookies unless Secure is set, so allowing that combination would silently break the login flow (the CAS_REQUEST_URI and CAS_SESSION cookies would never reach the browser). Fail loud at config time instead. - Default cookie.secure and cookie.samesite in cookie_attrs() rather than relying on JSONSchema defaults. core.schema.check() validates but does not mutate the config to apply declared defaults, so a conf that omits these fields would (a) panic at the SameSite concatenation and (b) silently drop the Secure attribute we declared as default true. - Drop user identifier from the session-refresh info log. This is a per-request event and turns routine auth traffic into per-request PII logging. The validation-success and SLO-delete info logs still include user= because those are one-shot, audit-worthy events.
The cas-auth.t test added in 59507fb used a TEST 7b suffix, which the t/plugin/check-test-code-style.sh CI step rewrites via the reindex util. CI then fails because the file is dirty after the auto-fix. Renumber the new test as TEST 8 and shift the subsequent two tests so reindex reports skipped.
Contributor
Author
|
@membphis it is mentioned at the end of the PR description that it is not backward compatible. |
Member
only this is not enough, pls write some detail doc about the
|
Contributor
Author
|
@membphis updated the PR description with a Breaking change section that:
Happy to add anything else if there's a specific deployment concern you want covered. |
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
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
This change updates the
cas-authplugin so that theCAS_REQUEST_URIcookie — which the plugin uses to remember where the user was headed before being bounced to the CAS IdP — is signed with HMAC-SHA256 and verified on the IdP callback using a constant-time comparison.After successful CAS ticket validation, the recovered redirect target is also validated as a same-origin path before being applied; if the cookie is missing, malformed, fails signature verification, or carries something that is not a relative path, the plugin falls back to
/instead of using the cookie value verbatim.A new required
cookieobject is added to the plugin schema:cookie.secret(string, required, minLength 32) — HMAC-signing key. Must match across all APISIX nodes so cookies issued on one node remain verifiable on another. Encrypted at rest viaschema.encrypt_fields.cookie.secure(boolean, defaulttrue) — whether to set theSecureattribute on issued cookies. Set tofalseonly for deployments where the protected route is not served over HTTPS.cookie.samesite(string, default"Lax", enum"Lax"/"None") — value for theSameSitecookie attribute."Strict"is intentionally not exposed because it suppresses the cookie on the IdP→SP top-level redirect when the IdP is on a different site.samesite="None"requiressecure=true(rejected atcheck_schematime) because browsers refuse to storeSameSite=Nonecookies withoutSecure.HttpOnlycontinues to be set unconditionally on the issued cookies.t/lib/keycloak_cas.luais updated so existing integration tests continue to pass; new tests int/plugin/cas-auth.texercise the schema constraints, the safe-redirect predicate, and the HMAC sign/verify roundtrip including tampering, wrong-secret, and malformed-cookie cases.Breaking change
This PR is not backward compatible for existing
cas-authplugin configurations. A new fieldcookie.secretis added as a required field of the schema, and a route that hascas-authenabled without it will failcheck_schemaand be rejected at admin-API time.Why it has to be required: the cookie integrity guarantee this PR introduces depends on every issued cookie being signed and every received cookie being verified. Making the secret optional would mean some deployments sign and some do not — the verification path could not assume a signature exists, the safe-redirect fallback could not distinguish "unsigned cookie" from "tampered cookie", and the protection would be defeated in exactly the deployments that did not opt in. Requiring the secret keeps the invariant "every cookie issued by this plugin is signed" simple and enforceable.
Migration for existing configurations: add a
cookie.secretof at least 32 characters to every route whosepluginscontainscas-auth. The same value must be configured on every APISIX node in the deployment so that cookies issued on one node remain verifiable on another. Generate one with e.g.openssl rand -base64 48.Before:
{ "plugins": { "cas-auth": { "idp_uri": "https://cas.example.com", "cas_callback_uri": "/cas/callback", "logout_uri": "/cas/logout" } } }After:
{ "plugins": { "cas-auth": { "idp_uri": "https://cas.example.com", "cas_callback_uri": "/cas/callback", "logout_uri": "/cas/logout", "cookie": { "secret": "<32+ char random string>" } } } }cookie.secureandcookie.samesiteare optional and default totrueand"Lax"respectively, which match the previous unconditional behavior introduced earlier in this PR.Operational impact: during a rolling upgrade, any node already running the new code will reject old-format configs from etcd until they are updated. Recommended order: update all
cas-authroute configurations to includecookie.secretfirst, then roll out the new APISIX binary.Which issue(s) this PR fixes:
N/A
Checklist