Repository navigation
feat(*): revoke cookie sessions by application supplied key - #212
janiussyafiq wants to merge 7 commits into
Conversation
Sessions can carry revocation keys in the encrypted payload via session:set_revocation_keys. On open, a key whose revocation mark is at or after the session creation time rejects the session with "session revoked". Marks are unix timestamps; the 4.2.0 mark "1" still means revoked regardless of time.
Writes a unix timestamp mark for an application supplied key through the configured revocation storage, so sessions carrying that key and created at or before the mark are rejected on open. Intended for identity provider initiated logout, where no cookie is available.
Check every audience's revocation keys when opening a remember cookie, since restoration mints a fresh session cookie carrying all of them. Reset the rejected cookie's meta on revocation so a re-login does not inherit the revoked creation time.
Run the key check before a remember cookie's data is kept for an absent audience, and leave a rejected remember cookie with empty metadata so save, logout and destroy do not reload its creation time.
|
@bungle need help reviewing this, to support backchannel logout this feature is needed, thanks |
| return nil, "session revocation is not enabled" | ||
| end | ||
|
|
||
| local storage_key, err = self.hash_storage_key(key) |
There was a problem hiding this comment.
Should the storage keys be prefixed with something, like: revoked: or r:, just to be sure that this mechanism cannot be used to manipulate existing sessions, or am I just thinking something that is a non-issue?
There was a problem hiding this comment.
Since we already added prefixes (subject:/ sid:) i think this should be a non-issue (?)
| -- @usage | ||
| -- local session = require("resty.session").new() | ||
| -- session:set_revocation_keys({ "sid:" .. sid, "sub:" .. sub }) | ||
| function metatable:set_revocation_keys(keys) |
There was a problem hiding this comment.
Should we also store revocation keys when store_metadata is enabled? Which could give later alternative way to do backchannel logout, aka directly deleting/timeouting the sessions from database?
There was a problem hiding this comment.
I was also thinking that do we ever need anything else than subject which we already have, and IDP sid?
There was a problem hiding this comment.
I mean that should we collect IDP sid on login too (optionally), e.g. with session:set_sid or session:associate(sid) (similar to session:set_subject). This would then make it possible to keep the association, and also use it with store_metadata.
There was a problem hiding this comment.
Other way to do it is to collect revocations and then check it when using a session (I think that is how you are doing it currently). Both have their uses, and probably their share of problems too.
There was a problem hiding this comment.
I mean that should we collect IDP sid on login too (optionally), e.g. with session:set_sid or session:associate(sid) (similar to session:set_subject). This would then make it possible to keep the association, and also use it with store_metadata.
I think the concern here is valid since subject also supports this, applied in the commit.
There was a problem hiding this comment.
https://openid.net/specs/openid-connect-backchannel-1_0.html#LogoutToken
A Logout Token MUST contain either a sub or a sid Claim, and MAY contain both. If a sid Claim is not present, the intent is that all sessions at the RP for the End-User identified by the iss and sub Claims be logged out.
Taken from the website verbatim, hence in this repo I think the sid and sub already covered. Let me know in case I'm missing something.
Replace set_revocation_keys/get_revocation_keys and session.revoke with structured fields: the existing subject plus a new sid (identity provider session id) set with session:set_sid, revoked with session.revoke_subject and session.revoke_sid. Revocation marks are stored under a "subject:" or "sid:" prefix, subject goes through hash_subject, so app supplied values cannot collide with session id marks.
|
@bungle need help review when u have time thanks |
This branch has not been deployed
2 participants
Closes #211.
Summary
session:set_revocation_keys(keys)/session:get_revocation_keys(): opaque keys carried per audience in the encrypted payload, e.g. an identity provider'ssidandsub.session.revoke(key, ttl, configuration): writes a unix-timestamp mark through the existingrevocationstorage, no open session needed.session:openrejects a session carrying a key whose mark is at or after the session's creation time; sessions created later are unaffected. The 4.2.0 mark"1"still means revoked regardless of time anddestroykeeps writing it.hash_storage_key; marks share one namespace percookie_name, remember cookies included.session.revokealways returns write errors;revocation_fail_modeis unchanged for open/destroy.key -> timestampstore format lets the library auto-derive them later.Consumer: zmartzone/lua-resty-openidc#572 (Back-Channel Logout receiver), then apache/apisix.
Test plan
make unitin the CI image (155 tests),make lintspec/06: key round-trip without subject, mark at/after creation revokes, mark before creation does not, legacy"1"mark, unrelated key, audience scoping, remember cookie,session.revokethen reopen, created after revokespec/07:session.revokereports a failed write in open fail mode, not-enabled error