fix: let an assertion be presented only once - #44
Conversation
Nothing stopped the same assertion being posted back a second time inside its validity window. Its ID is remembered now, in an lua_shared_dict the deployment names, and a second presentation is refused. The entry lives as long as the assertion's own Conditions leave it usable, so the cache holds exactly what could still be replayed. An assertion that names no expiry is remembered for replay_ttl, since nothing in the assertion says when to stop. Unset replay_dict leaves assertions untracked, which is what deployments with no shared dict to spare get today.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Pull request overview
Adds optional assertion replay protection to address item 5 of issue #37.
Changes:
- Tracks assertion IDs in a configured shared dictionary.
- Derives retention from assertion expiry or
replay_ttl. - Adds configuration documentation and replay tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lua/resty/saml.lua |
Implements replay detection and storage. |
README.md |
Documents replay configuration. |
t/assertion-conditions.t |
Tests replay rejection and TTL behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| local added, err, forcible = dict:add(key, true, ttl) | ||
| if not added then | ||
| if err == "exists" then | ||
| return false, "assertion " .. assertion.id .. " has been presented already" | ||
| end | ||
| return false, "could not track assertion " .. assertion.id .. ": " .. tostring(err) |
There was a problem hiding this comment.
Same problem one level up, worth folding into whatever fix you land here: the adds are also committed before the rest of login_callback can still reject. assertions_unused runs at line 506, but the name_id check 401s at 521 and the session_expires parse can ngx.exit(500) at 531 — both after the IDs are in the dict. So a browser re-POST of the same response, or a retry after a dropped reply, gets "has been presented already" instead of the original error and the user has to restart SSO.
Whatever transactional shape fixes the ordering should also move the commit past the last thing that can reject.
| ngx.log(ngx.WARN, "the assertion replay dict is full, older assertions are ", | ||
| "no longer tracked") |
| return false, "an assertion without an ID cannot be tracked" | ||
| end | ||
|
|
||
| local ttl = opts.replay_ttl or DEFAULT_REPLAY_TTL |
There was a problem hiding this comment.
The TTL only ever comes from Conditions/@NotOnOrAfter, but that is not the only window assertions_acceptable honours — confirmation_ok also accepts on SubjectConfirmationData/@NotOnOrAfter, and that is the one the Web Browser SSO profile actually mandates on a bearer confirmation, while Conditions/@NotOnOrAfter is optional.
So for the ordinary shape of <Conditions> carrying only an AudienceRestriction plus <SubjectConfirmationData NotOnOrAfter="+1h"/>, the entry lives 600s while the assertion stays acceptable for an hour. From t+601 a captured response replays cleanly with replay_dict fully configured and nothing in the log to say so. Same for an assertion with no <Conditions> at all, which #42 accepts indefinitely (TEST 15) but this remembers for 600s — TEST 23's a2 case is exactly that, under the heading "remembered for as long as it is usable".
The comment above says "the cache holds exactly what is still usable"; to make that true the TTL wants to be the max over the Conditions expiry and every confirmation expiry the assertion offers. An assertion with no bound at all arguably should not be accepted rather than remembered for a default 600s.
Two smaller things on the same lines:
ttl is taken verbatim with no upper clamp. An assertion with NotOnOrAfter="9999-12-31T23:59:59Z" is stored with ttl = 251617708859 — I measured it. replay_ttl reads like it should cap this, not only be the fallback, and an unbounded entry accelerates the forcible-eviction path below.
tostring(opts.sp_issuer) yields the literal "nil" when sp_issuer is unset, so the key becomes "nil|<id>" and the per-SP namespace the comment promises collapses. Reachable because assertions_acceptable explicitly supports sp_audiences as an alternative to sp_issuer. Low severity since most deployments set sp_issuer anyway, but the key deserves a non-nil guarantee.
| obj.idp_cert_func = function(doc) return idp_cert end | ||
| obj.auth_protocol_binding_method = opts.auth_protocol_binding_method | ||
| if opts.replay_dict then | ||
| obj.replay_dict = assert(ngx.shared[opts.replay_dict], |
There was a problem hiding this comment.
assert here is not "fails loudly at new()" in the deployment that matters. The consumer is the gateway's saml-auth plugin, which builds the object per request in the rewrite phase via core.lrucache.plugin_ctx(lrucache, ctx, nil, resty_saml.new, conf). There is no pcall on that path — core/lrucache.lua calls create_obj_fun(...) directly and plugin.lua calls the phase function directly — so a replay_dict naming a zone that does not exist is an uncaught Lua error and a hard 500 on every request through the route, not the plugin's return 500, {message = ...}. The lrucache TTL is 300s, so it re-raises indefinitely rather than once.
Returning nil, err instead would land in the branch the plugin already has.
Separately, for this option to be reachable at all the gateway needs replay_dict/replay_ttl in the saml-auth schema and the zone declared in nginx_config.http.custom_lua_shared_dict (and the helm chart's customLuaSharedDicts). None of that exists today, and the plugin schema does not set additionalProperties: false, so the option validates and then takes the route down. Worth landing those alongside, or the feature cannot reach a user.
| end | ||
| return false, "could not track assertion " .. assertion.id .. ": " .. tostring(err) | ||
| end | ||
| if forcible then |
There was a problem hiding this comment.
Beyond the message wording, the semantics here are fail-open. forcible means nginx made room by evicting other entries, and those are assertions still inside their validity window that just became replayable again. The login proceeds and the only trace is a WARN.
safe_add would return false, "no memory" and drop into the branch you already have at line 424, which fails closed. Worth making that choice deliberately, since nothing sizes the dict and the TTL is unbounded (see the thread above), so the eviction path is easy to reach rather than exotic.
| | `sp_audiences` | array of strings | `{ sp_issuer }` | Audiences this SP answers to. An assertion carrying an `AudienceRestriction` has to name one of them; an assertion carrying none is unrestricted. | | ||
| | `clock_skew` | number | `60` | Seconds of clock difference tolerated against the IdP when weighing `NotBefore` and `NotOnOrAfter`. | | ||
| | `replay_dict` | string | None | Name of an `lua_shared_dict` in which to remember the assertions already presented, so none is accepted twice. Unset leaves them untracked. | | ||
| | `replay_ttl` | number | `600` | Seconds to remember an assertion that names no `NotOnOrAfter` of its own. One that names it is remembered until it expires. | |
There was a problem hiding this comment.
Two things worth stating in these rows.
lua_shared_dict is scoped to one nginx instance's worker group, so a horizontally scaled SP — the normal shape behind a load balancer, and the shape this library ships into on Kubernetes — gets no cross-node protection. An assertion burned on one replica is still fresh on the next and the attacker just retries. Nothing currently says that.
And "One that names it is remembered until it expires" is not quite what the code does: the TTL follows only Conditions/@NotOnOrAfter, not the SubjectConfirmationData/@NotOnOrAfter window that also keeps the assertion acceptable. Details in the thread on assertions_unused.
| plain = {}, | ||
| skew = { clock_skew = 300 }, | ||
| audiences = { sp_audiences = { "https://sp.example.com/metadata" } }, | ||
| replay = { replay_dict = "saml_replay" }, |
There was a problem hiding this comment.
A few coverage gaps I found by mutating lua/resty/saml.lua and re-running the suite — each of these mutations leaves it fully green:
for i, assertion in ipairs(assertions) do if i > 1 then break endin bothassertions_acceptableandassertions_unused. No test drives a multi-assertion response throughlogin_callbackat all — TEST 16 is the only two-assertion block and it callssaml.doc_assertionsdirectly, bypassing the SP. So "every top-level assertion has to hold up" and "every assertion ID is tracked" are unpinned end to end, which is the shape closest to the wrapping attacks this is defending against.- dropping the SP scoping from the key (
local key = "sp|" .. assertion.id). All four SPs inOPTSusesp_issuer = "sp", and TEST 23 hardcodes the literal"sp|a1", so the scoping the comment promises cannot be tested. local ttl = DEFAULT_REPLAY_TTL, i.e. ignoringopts.replay_ttl.OPTS.replaysets onlyreplay_dict, so the one public knob this PR adds is never exercised.- removing the
if not assertion.idguard, and removing theforciblewarning. Thesaml_replay 1mdict is never filled, so the eviction path is never hit. - replacing
assert(ngx.shared[opts.replay_dict], ...)in_M.newwith a plain lookup — a typo'd dict name would silently degrade to no replay protection and no test would notice.
Also, on #42's side but same file: removing skew tolerance from NotBefore (if now < at then) is green, because TEST 3's NotBefore is at(3600), far outside any skew — a real IdP running a few seconds fast would break every login with no test catching it.
Closes #37, item 5 of its suggested scope. Stacked on #43, which is stacked on #42; the base branch retargets as each merges.
What was wrong
An assertion could be posted back as many times as its window allowed. #42 bounds that window and #43 ties the assertion to one
AuthnRequest, which together shrink the opening a great deal, but neither makes an assertion single-use, and single-use is what "bearer" means: whoever holds it is the subject.What it does now
login_callbackremembers the ID of every assertion it accepts and refuses a response carrying one it has seen. The store is anlua_shared_dictthe deployment names through the newreplay_dictoption, because a library cannot declare one and the entry has to be shared across workers. Unset leaves assertions untracked, which is today's behaviour; a name that nolua_shared_dictmatches fails loudly atnew()rather than quietly not tracking anything.How long an entry lives is taken from the assertion rather than from configuration:
Conditions/@NotOnOrAfterplus theclock_skewallowance is the last moment the checks in #42 would still accept it, so the cache holds exactly what is still replayable and no more. An assertion that names no expiry has nothing to derive from and is remembered forreplay_ttl, 600 seconds by default.Two smaller points:
sp_issuer, so several SP instances sharing one dict do not collide.lua_shared_dictevicts under pressure. An eviction weakens replay protection silently, so a forcible insert logs a warning naming the dict as full.Tests
TESTs 21 to 23 in
t/assertion-conditions.t. TEST 23 reads the entry's TTL back out of the dict, covering both the derived window and thereplay_ttlfallback.Full run on this branch,
t/assertion-conditions.tandt/signed-response.t, 120 subtests, all pass. Rebuilt against #43'slua/with the new tests kept, the two that should fail do and only those:TEST 22 passes on both, which is the point of it.