fix: accept OneTimeUse, and let the replay record honour it - #53
fix: accept OneTimeUse, and let the replay record honour it#53shreemaan-abhishek wants to merge 13 commits into
Conversation
SAML Core 2.5.1.5 makes OneTimeUse always valid: a condition on use, asking the SP to keep a record of the assertions it has spent. It was refused as a condition this SP cannot satisfy, so an IdP stamping its assertions single-use could not log in at all. The reader now carries it as a flag. With replay_dict set the record exists and the assertion is single-use as asked; without it the login goes through and a warning names the option. Closes #46
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe change recognizes SAML ChangesOneTimeUse assertion handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change accepts SAML OneTimeUse assertions, enforces replay protection when configured, and warns when it is unavailable; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant IdP as SAML response
participant Parser as src/xml.c
participant Lua as lua/resty/saml.lua
participant Replay as replay_dict
IdP->>Parser: Parse OneTimeUse condition
Parser->>Lua: Return one_time_use assertion flag
alt replay_dict configured
Lua->>Replay: Check and record assertion ID
Replay-->>Lua: Accept first use or reject replay
else replay_dict unavailable
Lua-->>Lua: Log replay_dict warning
Lua-->>IdP: Accept authentication
end
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: E2e Test Quality ReviewExplanation PASS. The PR adds end-to-end coverage through Test::Nginx::Socket::Lua, real signed SAML responses, the HTTP login callback, sessions, and lua_shared_dict replay storage. Tests cover acceptance without replay_dict, replay rejection with replay_dict, full-dictionary failures, TTL limits, condition ordering, unknown conditions, and the exported one_time_use field. The test file disables shuffling and flushes replay dictionaries where needed. The implementation changes stay within the OneTimeUse and replay-tracking flow, use atomic safe_add calls with request-local warning state, and introduce no unrelated behavior or unchecked new error path that matches the review criteria. Full details: Security CheckExplanation No explicit security-check failure was introduced. Category 1: no API keys, tokens, credentials, authentication headers, or secret-bearing configuration are logged or returned; new logs contain only a replay dictionary name, a SAML assertion ID passed through ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Accepts SAML OneTimeUse conditions and integrates them with existing replay protection.
Changes:
- Parses and exposes
OneTimeUseon assertions. - Warns when replay tracking is unavailable.
- Adds documentation and end-to-end replay tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/xml.c |
Recognizes and parses OneTimeUse. |
src/saml.h |
Adds the assertion flag. |
src/lua_saml.c |
Exposes the flag to Lua. |
lua/resty/saml.lua |
Warns when enforcement is unavailable. |
README.md |
Documents behavior and configuration. |
t/assertion-conditions.t |
Tests acceptance and replay rejection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| -- record of the assertions it has spent. replay_dict is that record; | ||
| -- without it the IdP's request goes unmet, and the operator is told | ||
| -- what to configure rather than the user refused | ||
| if assertion.one_time_use and not opts.replay_dict then |
There was a problem hiding this comment.
Two problems with the condition itself.
It reads "was replay_dict configured", not "was the record actually taken". spend_assertions (620-626) fails open on any safe_add error other than "exists" — it logs a generic ERR and lets the login through untracked, which TEST 40 codifies. So when the zone is full the assertion is accepted, nothing is recorded, and this warning does not fire, because opts.replay_dict is set. Verified on this branch: with a 32k saml_replay_full filled until safe_add returns no memory, the identical signed response carrying <saml:OneTimeUse/> returns 302 / then 302 /, and no OneTimeUse line reaches the log. The one deployment that did everything the README asks and still gets no enforcement is the one that produces no diagnostic. Zone exhaustion is attacker-drivable and does not self-heal, since safe_add never evicts.
It also reads a different field than the gate that enforces. This checks opts.replay_dict — the raw name on the caller's table, held by reference at 934. Line 759 enforces on self.replay_dict, the handle resolved once at 952. They agree today only because the constructor errors on a name that does not resolve. The APISIX saml-auth plugin hands resty_saml.new the live plugin conf table and holds the object in a 300s lrucache, so any post-construction mutation splits them in one direction or the other: enforce-while-warning, or suppress-the-warning-while-recording-nothing. assertions_acceptable only receives opts, so it cannot consult the value that governs — passing self (or the resolved handle) would make the invariant structural. TEST 48 cannot catch either direction, since both fields are truthy there.
There was a problem hiding this comment.
Second point taken: assertions_acceptable now receives self.replay_dict, the handle the last gate enforces on, so the two read one value (8bf6364).
On the first, the full-dict path is not silent. spend_assertions logs at ERR, could not remember assertion <id> in <dict>: no memory, this login is not covered by replay tracking, and TEST 40 asserts that line. What it lacked was the word; it now ends though it carries OneTimeUse when the assertion does, and TEST 49 pins it. Fail-open on a full dict is #50's documented choice and stays.
There was a problem hiding this comment.
Rechecked both halves on the built branch. The handle change is real; the behavioural gap the wording was meant to cover is still open.
The full-dict state is not just under-diagnosed, it is unenforced, and now untested in the behavioural direction. The same signed OneTimeUse response presented three times to the replay_full SP returns 302 / 302 / 302. At base 238ff7a the first presentation was already 401. So a deployment that did exactly what the README asks — set replay_dict — and merely under-sized the zone (the README itself calls 1m too small) gets zero single-use enforcement, indefinitely. I accept fail-open as #50's documented choice for a plain assertion; the question this PR opens is whether it is still the right choice for an assertion whose IdP explicitly demanded single use, and the PR answers it implicitly by changing only the log string. TEST 49 drives one login and greps a line, so neither direction of that behaviour is pinned.
The handle fix is half-applied and reverts green. assertions_acceptable now takes the resolved handle — good — but line 634 still prints opts.replay_dict in the one diagnostic that state has, so the ERR can name a dict the record was never written to. The comment at :425-426 ("the two cannot disagree") is true of the gate and not of that message. _M.new still stores the caller's table by reference ({opts = opts} at :946), which is exactly the shape the APISIX plugin uses. Also worth a test: reverting the parameter back to opts.replay_dict leaves the suite 246/246 green, so the fix has no regression guard in either direction.
| -- Core 2.5.1.5: OneTimeUse is always valid, and asks the SP to keep a | ||
| -- record of the assertions it has spent. replay_dict is that record; | ||
| -- without it the IdP's request goes unmet, and the operator is told | ||
| -- what to configure rather than the user refused |
There was a problem hiding this comment.
"replay_dict is that record" is the claim the rest of the PR rests on, and there are three paths where the record does not cover the window in which the assertion is still accepted. All three verified on this branch.
An assertion that bounds nothing gets a 600s record and unbounded acceptance (line 600). conditions({ body = "<saml:OneTimeUse/>" }) emits Conditions with no NotBefore/NotOnOrAfter and assertion() emits no SubjectConfirmation, so last_moment_usable returns nil and ttl falls to DEFAULT_REPLAY_TTL. After the first login ngx.shared.saml_replay:ttl(replay_key("otu-noexp")) reads 599.999; drop the record and the identical response logs in again. time_bounds_ok(nil, nil, ...) is true forever, and the confirmation loop is skipped when #confirmations == 0. This is exactly TEST 48's fixture — it passes only because it replays immediately.
last_moment_usable decides confirms_here without looking at not_before (line 543). A confirmation that confirmation_ok will refuse still counts, still sets unbounded, and still wipes a satisfiable sibling's close at 558. An assertion with C1 (this ACS, NotBefore an hour out, no NotOnOrAfter) and C2 (this ACS, NotOnOrAfter 4h out) is accepted on C2, but the record TTL reads 599.999 instead of ~14460 — replayable from T+600 to T+4h. The comment at 528-538 says this class is excluded, but the exclusion only covers an unparseable not_on_or_after; confirms_here has no clock in it at all.
MAX_REPLAY_TTL caps the record with no matching cap on acceptance (line 607). IdP sets NotOnOrAfter a week out: the record dies at 86400 and time_bounds_ok keeps admitting the assertion for another six days. TEST 39 already asserts that capping for a plain assertion; this PR routes a class where the IdP has explicitly demanded single use through the same path, without deciding whether "forget it and accept it again" is the right answer, and without refusing or warning when the assertion outlives its record.
There was a problem hiding this comment.
All three are bounds of the record #50 introduced, untouched here, and the README's Remembering assertions states the first and third in the words you quote; TESTs 37 and 39 pin them. Every real IdP bounds a bearer confirmation to minutes, so none of the three occurs on shipped defaults.
The second is a misread. A confirmation with a NotBefore an hour out and no NotOnOrAfter becomes satisfiable at T+1h and never stops, so the assertion is acceptable for good from then on. last_moment_usable answering unbounded is correct, and the fallback record is the designed answer; a ~14460s record would leave the assertion replayable from T+4h with no record at all.
What survives is the diagnostic. spend_assertions now WARNs when an OneTimeUse assertion's record fell back to replay_ttl or was capped at a day (bd5226e, TEST 50), and TEST 48's fixture carries a NotOnOrAfter so it no longer passes by replaying inside 600s (a2e67be).
There was a problem hiding this comment.
I do not think the misread is on my side — the "later gap" argument cuts the other way, and there is a second instance of the same class.
Measured on the replay SP with C1 = {Recipient=ACS, NotBefore=T+3600, no close} and C2 = {Recipient=ACS, NotOnOrAfter=T+14400}: C2 alone gives 302 with record ttl=14460. C1 alone gives 401 — it cannot admit the assertion now. C1+C2 gives 302 with record ttl=599.996.
Acceptance is [T, inf) under both policies, because C1 opens at T+3540 and never closes. So the bounded record and the fallback leave the identical tail gap; the fallback additionally leaves [T+600, T+14460) — 13,860s inside the IdP's own stated window — unprotected. "A ~14460s record would leave the assertion replayable from T+4h with no record at all" is true, but the 600s record leaves it replayable from T+10m with no record at all. One is a strict superset of the other. The operational shape is what bothers me: adding a confirmation that cannot admit the assertion removes protection from the one that can.
The function also disagrees with itself on the more permissive input. An assertion valid until 9999-12-31 — acceptable for ~8000 years — gets ttl=86399.995. One with no bound at all, acceptable forever and strictly more dangerous, gets ttl=599.999: a 144× shorter record for the worse input. Whatever bound is right, those two paths should agree. usable_until == nil -> ttl = MAX_REPLAY_TTL dominates both and is one line.
Separately, and inside shipped defaults this time: confirms_here ignores SubjectConfirmation@Method. A 30-minute bearer confirmation alone records ttl=1859.999; add a urn:oasis:names:tc:SAML:2.0:cm:sender-vouches sibling with the same Recipient and no close and it drops to ttl=599.999 — 1260s lost. A 30-minute window is squarely inside README:140's "the assertion window at most an hour", which is the sentence "single-use within the bounds above" leans on. From the same probe: a sender-vouches confirmation alone returns 302, so confirmation_ok does not check Method either, though the Web Browser SSO profile requires bearer. method is parsed in C and read by nobody.
| -- without it the IdP's request goes unmet, and the operator is told | ||
| -- what to configure rather than the user refused | ||
| if assertion.one_time_use and not opts.replay_dict then | ||
| ngx.log(ngx.WARN, "assertion ", loggable(assertion.id), |
There was a problem hiding this comment.
This line is the only thing standing between the old refusal and silent acceptance, and it fails in both directions.
Invisible by default. nginx's documented default is error_log logs/error.log error;, and OpenResty does not raise it, so an embedder that never sets a level discards this entirely — the login changes from refused to accepted-and-replayable with no notice at all. The suite only sees it because t/assertion-conditions.t:3 calls log_level('info'). README:148 presents the warning as the delivery mechanism without saying what level is required to see it.
Unbounded when it is visible. It sits behind only a session lookup and a RelayState comparison against the caller's own session, with no once-per-worker latch. Verified: a OneTimeUse assertion restricted to another audience returns 401 nil and still writes the warning, so one captured signed assertion replayed in a GET / -> POST /acs loop writes WARN lines indefinitely for logins that never succeed. And the population this PR unblocks — IdPs that stamp OneTimeUse on every assertion — gets one line per login forever, with no way to silence it.
There was a problem hiding this comment.
The warning is a courtesy, not the safety line; silent acceptance is what Core 2.5.1.5 prescribes and what Spring, Shibboleth SP and Keycloak do.
Level: both consumers default error_log_level to warn (apisix/cli/config.lua, EE config-default.yaml), so it is visible where it matters. The README now says it is logged at warn (b162b7b).
Volume: a refused attempt on that loop already writes response from IdP rejected: ... at ERR, so the vector exists today one level up; this adds a line to it. One line per accepted login is the correct signal for a deployment whose IdP asks for single use and which has not configured it, and a once-per-worker latch would hide a persistent state after the first hit. Left as is.
There was a problem hiding this comment.
Pushback 2 verified and withdrawn for the two named consumers — warn at apisix/apisix/cli/config.lua:94, apisix/conf/config.yaml.example:203, api7-ee-3-gateway/conf/config-default.yaml:221, CP docker-compose/gateway_conf/config.yaml:5, helm charts/gateway/values.yaml:702, and both cli/ngx_tpl.lua:37 emit warn even when the key is absent. Inside those two products the line is visible. The residual is only that this is a generic rock whose README addresses OpenResty operators directly, where a level-less error_log logs/error.log; is error and drops it — confirmed empirically.
What I did not raise last time, and is the sharper point: this warning fires before the SP decides whether to refuse the login at all. It sits at :427, ahead of time_bounds_ok, the audience loop and the confirmation loop. Verified: three presentations of a OneTimeUse assertion audience-restricted to elsewhere on the plain SP produce three [warn] ... cannot enforce without replay_dict lines, each followed by [error] response from IdP rejected: assertion probe-refused is restricted to elsewhere. The advice is wrong for those requests — replay_dict would not have changed the outcome — and it means anyone holding any signed assertion from the trusted IdP, including one minted for a different SP or long expired, has an unrate-limited log-write lever at one line per POST.
Your volume argument was that a refused attempt already writes an ERR one level up, so the vector exists. It does, but that one is the SP reporting its own refusal; this one advertises a configuration change that would not have helped. spend_assertions is deliberately placed at "the last gate" (:586) for exactly this reason. Moving the OneTimeUse check to the same place makes the line mean "a login the SP accepted went untracked", which is the statement you actually want, and it drops the refused-login volume to zero.
| **This is what `<saml:OneTimeUse/>` asks for.** An IdP stamps that condition on an | ||
| assertion to ask the SP to keep exactly this record. SAML Core 2.5.1.5 makes the | ||
| condition always valid, a condition on use rather than on validity, so the login goes | ||
| through with or without the option. With it, the assertion is single-use as the IdP |
There was a problem hiding this comment.
This sentence is an unqualified guarantee that the three paragraphs directly above it already deny, and that the code denies further.
118-124 says the record is per nginx instance and a replay through a load balancer "is accepted"; 129-131 says a full zone "leaves that assertion untracked"; 136-142 says an unbounded assertion is "accepted for good" past replay_ttl, and one valid beyond a day is "accepted again past it". None of that is carried forward here. The deleted text stated its limitation plainly ("is still refused outright... the two do not meet yet"); the replacement states a property the code does not have, so an operator can set replay_dict and report OneTimeUse compliance that does not survive contact with the deployment.
The without-it/with-it framing is also binary where the code has three outcomes: unset -> warn and accept; set and recorded -> enforced; set and not recorded -> accepted with no OneTimeUse warning at all.
There was a problem hiding this comment.
The sentence is better. Three things the paragraph still does not carry.
"single-use within the bounds above" most naturally reads as the immediately preceding paragraph — the replay_ttl fallback and the day cap. But the state where single-use actually breaks outright is a full zone leaving the assertion replayable indefinitely, and that is two paragraphs earlier, not what "above" points at. README:130-131 still describes that case as only "logs an error naming the assertion and the zone", with no mention of the new suffix.
The second new WARN (saml.lua:617-620) is documented nowhere.
And the severity mapping is inverted relative to how much trouble each state is: the permanent, total non-enforcement state — replay_dict unset — is WARN and hideable, while the transient, single-login state is ERR and always visible.
Also, README:139-140's "Both need an IdP far outside shipped defaults" is now falsified by the sender-vouches case on the other thread: a 30-minute bearer window, well inside "at most an hour", loses 1260s of record to a sibling confirmation.
| return is_assertion_el(node, "AudienceRestriction") || | ||
| is_assertion_el(node, "ProxyRestriction"); | ||
| is_assertion_el(node, "ProxyRestriction") || | ||
| is_assertion_el(node, "OneTimeUse"); |
There was a problem hiding this comment.
Matching on element name only leaves the other schema-valid encoding of the same condition refused: <saml:Condition xsi:type="saml:OneTimeUseType"/>. OneTimeUseType extends ConditionAbstractType, so the XSD accepts that form, but is_assertion_el compares node->name — which is Condition for that shape — so is_known_condition returns 0 and unknown_condition is set.
Verified: an assertion whose only condition is <saml:Condition xmlns:xsi=... xsi:type="saml:OneTimeUseType"/> returns 401 nil with "carries a condition this SP cannot satisfy: Condition" on both the plain SP and the replay_dict-configured one — the refusal fires at saml.lua:417, before one_time_use is ever consulted. The same holds for xsi:type="saml:ProxyRestrictionType", which #42 explicitly meant to accept.
So the PR body's premise — "there is no configuration that gets past the refusal" — remains true for this encoding after the fix.
There was a problem hiding this comment.
Pre-existing and deliberate: #42 refuses the xsi:type spelling for every condition, AudienceRestriction included, and TEST 13 pins that with xsi:type="saml:AudienceRestrictionType" as its unrecognised case. Refusing is fail-closed and in spec (Core 2.5.1.1 rule 3). Accepting it means resolving xsi:type in C for every reader at once; an xsi:type AudienceRestriction accepted as known with its audiences unread would be a bypass. No browser-SSO IdP writes that spelling. Tracked in #54, out of scope here.
There was a problem hiding this comment.
The #54 deferral is defensible on fail-closed grounds and I am not asking for it here. What the note does not say is that the asymmetry is created by this PR rather than inherited.
Before this PR both spellings of the same condition — <saml:OneTimeUse/> and the schema-legal <saml:Condition xsi:type="saml:OneTimeUseType"/> — produced the same 401. After it, the first is 302 and the second is still 401 ... carries a condition this SP cannot satisfy: Condition. An IdP that switches SAML serializers flips from working logins to a hard 401 for every user, where before the behaviour was at least uniform.
The message degradation is new too. Schema validation runs first and ConditionsType (xsd/saml-schema-assertion-2.0.xsd:128-136) is a closed choice over exactly four elements; this PR promotes the third to known, so Condition becomes the only name that can reach unknown_condition, and xmlStrdup copies the element name and never the xsi:type. Verified: an operator whose IdP sends xsi:type="saml:ProxyRestrictionType" — a condition this SP does support in its element spelling — gets a 401 naming neither the condition nor replay_dict. Two semantically different unknown conditions in one Conditions both report Condition.
Worth one sentence in the deferral note saying the split is new, so #54 is not read as purely pre-existing.
| // ProxyRestriction binds an IdP issuing on behalf of another IdP and asks | ||
| // nothing of the SP consuming the assertion. OneTimeUse is always valid by | ||
| // Core 2.5.1.5, a condition on use rather than on validity: it asks the SP to | ||
| // keep a record of the assertions it has spent, which the caller has or has | ||
| // not, so it is reported as a flag. |
There was a problem hiding this comment.
"which the caller has or has not" is the load-bearing assumption, and in the shipping product the caller cannot have it.
Both apisix/apisix/plugins/saml-auth.lua and api7-ee-3-gateway/apisix/plugins/saml-auth.lua are pinned to lua-resty-saml = 0.2.5 and declare a schema with no replay_dict, replay_ttl, idp_issuers, sp_audiences or clock_skew, and there is no saml_replay shared dict to name. Every gateway deployment therefore lands permanently on the accept-and-warn path, with a warning naming an option the operator has no way to set. There is also no strict / refuse-unenforceable knob anywhere in this diff to restore the old behaviour.
An operator who smuggles replay_dict past the schema (there is no additionalProperties: false) without a matching shared dict hits _M.new's error("no lua_shared_dict named ..."), which core/lrucache.lua calls without pcall — a hard 500 on every request through the route.
This is not a regression against any shipped version, since #42's refusal postdates 0.2.5. But a companion PR exposing replay_dict in both plugin schemas should land with this one; otherwise the net effect of the release is OneTimeUse going from refused to accepted and unenforced.
There was a problem hiding this comment.
Agreed on the facts, and it is the follow-up: api7/api7-ee-3-gateway#2177 covers exposing replay_dict/replay_ttl in saml-auth, declaring the dict in ngx_tpl.lua, and the same in apache/apisix plus the control-plane sync. It cannot land with this PR: the plugins pin 0.2.5, so it follows #39 and the pin bump, in two other repos.
As you note, 0.2.5 already accepts OneTimeUse unenforced; this PR keeps that and adds the warning, where 0.2.6 without it would refuse. A knob to restore that refusal would restore a spec-nonconformant behaviour nobody asked for, so none is added.
| } | ||
|
|
||
| for (xmlNode* child = conditions->children; child != NULL; child = child->next) { | ||
| if (is_assertion_el(child, "OneTimeUse")) { |
There was a problem hiding this comment.
This sits before the break below, so one_time_use is document-order dependent: a <saml:OneTimeUse/> appearing after an unknown condition is never seen, and the newly exported public field silently reads false.
Verified through saml.doc_assertions(): Conditions with <saml:OneTimeUse/> then an unknown <saml:Condition> reports one_time_use=true unknown_condition=Condition; the reverse order reports one_time_use=false unknown_condition=Condition. Same document content, different derived state.
So the field does not mean "this assertion carries OneTimeUse", it means "it carries OneTimeUse and no unrecognised condition preceded it". Harmless only because assertions_acceptable refuses on unknown_condition (417) before reading one_time_use (426) — two facts in different files coupled by statement order, with nothing recording the dependency. Soften unknown-condition handling the way this PR just softened OneTimeUse, and the flag starts under-reporting.
There was a problem hiding this comment.
Fixed (c769806). The scan no longer stops at the first unknown condition; the first unknown name is still what is reported. TEST 51 reads the flag in both orders.
There was a problem hiding this comment.
Fix confirmed: restoring the break fails TEST 51, verified with a rebuild in between. Two things about the shape it left behind.
The commit changes no login outcome. The removed break matters in exactly one shape, Conditions = [saml:Condition, saml:OneTimeUse]. But an assertion with unknown_condition set is refused at lua/resty/saml.lua:416-419, which returns before the OneTimeUse warn at :427 and before spend_assertions runs. Verified: login_with('plain', ...) on <saml:Condition xsi:type="saml:OneTimeUseType"/><saml:OneTimeUse/> returns 401 with exactly one log line — "carries a condition this SP cannot satisfy: Condition" — and zero "carries OneTimeUse" warns. So "the scan goes on so what else the assertion carries is read whatever the order" does not hold for this library's own consumer; the recovered flag is observable only through the exported saml.doc_assertions reader, which is precisely and only what TEST 51 asserts on. Worth saying so in the comment, or exiting the loop once both flags are settled.
Neither guard on the rewritten if is observable by the suite. child->type == XML_ELEMENT_NODE: binding.c:187/243/305 call xmlReadMemory with options 0, so XML_PARSE_NOBLANKS is off and real indented IdP XML keeps whitespace text nodes under <Conditions>. For a text node is_assertion_el returns 0 at its first conjunct, so !is_known_condition(child) is TRUE and only this guard prevents xmlStrdup("text") — a 401 ... carries a condition this SP cannot satisfy: text on every login. A guard-less build passes 246/246, because conditions() at :169 concatenates with no separator and the file contains no whitespace after a <saml:Conditions> open tag. a->unknown_condition == NULL, added here: deleting it also leaves 246/246, and it is not merely untested but unfalsifiable — schema validation runs first and ConditionsType is a closed choice over four elements, three now known, so no test can construct two distinct unknown names.
Both disappear if the flag is read separately from the refusal scan: a->one_time_use = assertion_child(conditions, "OneTimeUse") != NULL; answers the same question in one line, lets this loop revert verbatim to its pre-PR form, and is net 2 lines shorter. Verified: builds clean, 246/246 including TEST 51's order cases.
|
|
||
| for (xmlNode* child = conditions->children; child != NULL; child = child->next) { | ||
| if (is_assertion_el(child, "OneTimeUse")) { | ||
| a->one_time_use = 1; |
There was a problem hiding this comment.
Setting the flag on every occurrence means a <Conditions> carrying two or more <saml:OneTimeUse/> elements is now accepted. Core 2.5.1.5 makes that a MUST NOT, and the bundled XSD cannot catch it — xsd/saml-schema-assertion-2.0.xsd:128-135 declares the ConditionsType body as <choice minOccurs="0" maxOccurs="unbounded">, so duplicates validate. Before this PR any occurrence was refused, so they could not get through.
Verified: <saml:OneTimeUse/><saml:OneTimeUse/> returns 302 / and parses as one_time_use=true, unknown_condition=nil.
A PR whose subject is spec conformance for this element leaving the element's own cardinality rule unenforced seems worth a second look, especially since the body cites Keycloak's broker ("only checks there is at most one") as the peer behaviour being matched. count_assertion_el(conditions, "OneTimeUse") — already used by read_audience_restrictions and read_subject_confirmations — gives presence and cardinality in one call.
| if (is_assertion_el(child, "OneTimeUse")) { | ||
| a->one_time_use = 1; | ||
| } | ||
| if (child->type == XML_ELEMENT_NODE && !is_known_condition(child)) { |
There was a problem hiding this comment.
Promoting OneTimeUse to a known condition leaves unknown_condition (line 520) able to hold exactly one value: the literal string Condition.
XSD validation runs on every parse before anything reads the document (src/binding.c:196 for POST, :314 for redirect), and ConditionsType at xsd/saml-schema-assertion-2.0.xsd:128 is a closed choice over exactly four elements — Condition, AudienceRestriction, OneTimeUse, ProxyRestriction — with no <any> wildcard. Three of the four are now in is_known_condition, so the only name that can reach xmlStrdup(child->name) is Condition. And xmlStrdup copies the element name, never the xsi:type attribute, which is the only part that says what the condition actually is.
Verified: OneTimeUseType, ProxyRestrictionType and AudienceRestrictionType all produce the identical line assertion <id> carries a condition this SP cannot satisfy: Condition. An operator debugging a broken IdP gets a message naming neither the condition nor replay_dict — so the exact case this PR wants them to recognise is the one the log can no longer identify — and the name-reporting machinery becomes dead weight for a single hardcoded outcome.
There was a problem hiding this comment.
The facts hold: after this PR Condition is the only element that can reach unknown_condition. Keeping the name generic costs a strdup and stays right if the XSD or the reader ever admits a fifth element, so it stays. Appending the xsi:type value to that message, so the one case it fires in names the type, is in #54 with the rest of the xsi:type question. No IdP emits the long spelling, so the case this PR wants operators to recognise is the plain element, which the warning names.
| -- asks for a record this SP does not keep, which is said, not refused | ||
| ngx.say(login_with("plain", saml_response({ | ||
| conditions = conditions({ body = "<saml:OneTimeUse/>" }), | ||
| id = "single", conditions = conditions({ body = "<saml:OneTimeUse/>" }), |
There was a problem hiding this comment.
This block lost its only behavioural assertion about OneTimeUse and gained no replacement, so the weakening the PR ships has no coverage.
The deleted 401 nil plus qr/carries a condition this SP cannot satisfy: OneTimeUse/ proved the assertion was refused. The new 302 / plus the warn regex proves only that one presentation is accepted. Nothing in the suite presents the same OneTimeUse assertion twice to an SP without replay_dict, which is the property that actually changed. Verified that it is genuinely reusable: two login_with("plain", xml) calls with the identical response both return 302 /.
As it stands, a change that started refusing OneTimeUse again on the no-dict path breaks no test, and neither does a regression in the other direction.
There was a problem hiding this comment.
TEST 13's first 302 is the coverage: refusing OneTimeUse again on the dict-less SP returns 401 there and fails the block, which is what the run against main shows. The narrower gap is real, and TEST 52 now presents the same OneTimeUse assertion twice to the dict-less SP and expects 302 both times (b8728ad).
|
|
||
|
|
||
|
|
||
| === TEST 48: OneTimeUse is met by the replay record where there is one |
There was a problem hiding this comment.
This is TEST 32 with the conditions body swapped, and it does not exercise anything OneTimeUse-specific: spend_assertions never reads assertion.one_time_use, so deleting <saml:OneTimeUse/> from the payload leaves the block passing. TEST 32 (line 1011) already has the same flush_all, the same two login_with("replay", xml) calls, the same 302 / / 401 nil, and the same "has been presented already" assertion.
Three hygiene points while it is being reworked:
- It never inspects the record, though
replay_key(line 240) exists for exactly that and TESTs 34/37/38/41/42 use it. As written it passes for any reason the second login is refused. Asserting the TTL would also have caught the 600s fallback noted onsaml.lua. - It is the only block in the file supplying its own
--- no_error_log, which stops theadd_block_preprocessorelsif(lines 17-22) from firing and silently drops the[alert]and[emerg]guards every other error_log-carrying block gets. Appending to the injected list instead keeps them. - It reuses
id = "single", the same string TEST 13 gains at line 580, against the convention the file's ownhttp_configstates. Latent today only because TEST 13 runs on the dict-lessplainSP.
Separately: TEST 16 (line 641) is the only block that inspects the raw doc_assertions table, and it was not extended with one_time_use, so the C reader contract for the new field has no direct assertion anywhere.
There was a problem hiding this comment.
The first 302 is the OneTimeUse-specific part; on main it is 401. The second half passing without the element is the claim under test: once the record exists the element changes nothing, and the title now says so.
Hygiene items all taken (a2e67be): the fixture bounds the assertion with a NotOnOrAfter, the record's TTL is read back through replay_key, the ID is its own, the no_error_log list carries the guards the preprocessor would have injected, and TEST 16 reports one_time_use.
There was a problem hiding this comment.
Hygiene items confirmed applied to TEST 48. The same trap caught the two new blocks.
TEST 50 (:1456) and TEST 52 (:1506) each declare --- error_log, which makes the preprocessor at :15-22 swap the default no_error_log: [error] for [crit]/[alert]/[emerg] — and neither hand-restores it the way TEST 48 now does. Unlike 48 and 49, neither legitimately logs an error, so nothing is gained by the swap.
Verified twice by injecting ngx.log(ngx.ERR, ...) into every successful login: pre-existing TEST 39 and TEST 45 catch it; TEST 50 and TEST 52 both pass. So a regression that makes a OneTimeUse login emit an error while still returning 302 ships green through the two blocks that exist specifically to cover OneTimeUse logins — and TEST 50 is the block standing in for TEST 13's deleted 401 coverage.
Same one-line fix as 48: append the guards to the no_error_log list each block declares.
| xmlChar* id; | ||
| xmlChar* issuer; | ||
| int has_conditions; | ||
| int one_time_use; |
There was a problem hiding this comment.
Adding a field here is worth pairing with a one-line Makefile fix: neither object rule lists src/saml.h as a prerequisite. saml.o: src/*.c globs only .c files, and lua_saml.o: src/lua_saml.c names one.
Verified with make -n: touch src/saml.h alone reports "saml.so is up to date"; touch src/saml.h src/xml.c rebuilds only saml.o and relinks it against the old lua_saml.o. Had this field changed the layout, saml_doc_assertions would stride the array with one sizeof while doc_assertions indexed fields with another — shifted pointers into Lua and xmlFree on garbage, on the login path, with no compiler or linker diagnostic.
This PR escapes by luck: int one_time_use lands in the tail padding after int has_conditions, and both layouts measure sizeof=80 with id=0 issuer=8 has_conditions=16 not_before=24 not_on_or_after=32 unknown_condition=40 — byte-identical. CI always builds fresh, so it would never surface there.
There was a problem hiding this comment.
Fixed (3884513): both object rules list src/*.h.
There was a problem hiding this comment.
Confirmed fixed and it is load-bearing: touch src/saml.h now rebuilds both objects where the old rules printed "up to date". Worth noting src/saml.c is a unity build #include-ing xml.c, so a mid-struct field would have shifted every field after has_conditions in a stale lua_saml.o — the failure mode was real, not theoretical.
The identical class survives one level up, if you want to close it properly while you are here. touch Makefile && make -n saml.so still reports "up to date", and so does switching OPENSSL_DIR from /usr/local/openresty/openssl3 to .../openssl111: objects compiled against one OpenSSL's headers get linked against the other's libs with no diagnostic. That is the two-runtime shape this project actually builds in, so it is more likely to bite than the header case. touch xmlsec1-1.2.28/src/.libs/libxmlsec1.a likewise does not relink, since saml.so does not list $(XMLSEC1_STATIC_LIBS).
Minor, opposite direction: saml.o: src/*.c over-approximates — touch src/lua_saml.c needlessly recompiles saml.o. -MMD -MP plus a Makefile prerequisite covers all of it and is smaller than the glob.
…t drops OneTimeUse assertions_acceptable read opts.replay_dict while the last gate reads the resolved self.replay_dict. They agree today, but only by the constructor's say-so; passing the handle makes it structural. A full dict already logs that the login went untracked. When the assertion carries OneTimeUse the line now says so.
The record falls back to replay_ttl when nothing bounds acceptance and is capped at a day when the IdP's window runs longer, so past it the assertion is accepted again. Both need an IdP outside shipped defaults; where that IdP also asked for single use, the login now logs that the record fell short.
The scan stopped at the first unknown condition, so a OneTimeUse after one was never seen. The name of the first unknown is still what is reported.
…d its own ID The block passed by replaying inside replay_ttl; it now bounds the assertion and reads the record back. Its no_error_log carries the guards the preprocessor would have injected, and TEST 16 reports the new field.
Neither object rule listed the headers, so a struct change rebuilt only the object whose .c was touched and linked it against the other, stale one.
There was a problem hiding this comment.
🟡 Changes recommended
The replay-cap warning is incorrectly emitted when the record lifetime equals the cap without being truncated.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
| -- the record is bounded where acceptance is not, so past it the | ||
| -- assertion is accepted again. An IdP that asked for single use is | ||
| -- told, since it is the IdP's window that made the record fall short | ||
| if assertion.one_time_use and (usable_until == nil or ttl == MAX_REPLAY_TTL) then |
There was a problem hiding this comment.
Right: a window of exactly 86,400 seconds read as capped and drew the warning for a record that covers it to the second. Fixed in fe7774e by weighing the uncapped lifetime against the cap before the clamp, with > so the exact boundary stays quiet.
A window of exactly the cap read as capped and warned that the assertion outlives a record that covers it to the second.
| -- the record is bounded where acceptance is not, so past it the | ||
| -- assertion is accepted again. An IdP that asked for single use is | ||
| -- told, since it is the IdP's window that made the record fall short. | ||
| -- Weighed before the clamp: a window of exactly the cap is covered | ||
| local outlives = usable_until == nil or usable_until + skew - now > MAX_REPLAY_TTL | ||
| if assertion.one_time_use and outlives then | ||
| ngx.log(ngx.WARN, "assertion ", loggable(assertion.id), | ||
| " carries OneTimeUse but stays acceptable past its record, which lapses in ", | ||
| ttl, " seconds") | ||
| end |
There was a problem hiding this comment.
This block runs before dict:safe_add two lines down, so it asserts a record lifetime in the states where no such record exists. Verified three ways at 3884513; fe7774e changes the predicate but not the placement.
(a) Full dict plus an unbounded OneTimeUse assertion — TEST 49's exact fixture — emits [warn] ... stays acceptable past its record, which lapses in 600 seconds immediately followed by [error] ... no memory, this login is not covered by replay tracking though it carries OneTimeUse. Two adjacent, directly contradictory lines: no record exists and nothing lapses in 600 seconds. TEST 49 greps only the ERR, so the contradiction ships untested.
(b) Replaying one captured unbounded OneTimeUse assertion at the dict SP produces, per attempt, [warn] ... lapses in 600 seconds then [error] ... has been presented already — the warn claims the record falls short at the exact moment it is holding, and it doubles the log volume of an attacker-paced loop on the enforced path.
(c) The multi-assertion rollback at :628-630 deletes the key microseconds after the warn quoted its TTL.
Moving the block into the if added then arm fixes all three and makes the message true by construction: it then describes a record that exists, with the TTL it was actually stored with.
There was a problem hiding this comment.
All three confirmed, and the fix is a step past the if added then arm (9fcf54f): the messages are gathered per record written and emitted only after the loop, once every record stands. Inside the added arm the warn for a first assertion would still precede a later sibling's collision and quote a TTL the rollback then deletes, your (c). With the deferred emit: full dict says only the ERR (TEST 49 now asserts the warn's absence), a replay says only has been presented already, and a rolled-back response says nothing.
| -- told, since it is the IdP's window that made the record fall short. | ||
| -- Weighed before the clamp: a window of exactly the cap is covered | ||
| local outlives = usable_until == nil or usable_until + skew - now > MAX_REPLAY_TTL | ||
| if assertion.one_time_use and outlives then |
There was a problem hiding this comment.
Two things about this gate. First, credit where due: fe7774e fixes the clamp problem I was about to raise — at 3884513 the predicate was ttl == MAX_REPLAY_TTL, which tested the post-clamp value and fired falsely on any window landing exactly on the cap (NotOnOrAfter = now+86340 with the default clock_skew=60 logged the warn while its record covered acceptance precisely). Weighing usable_until + skew - now before the clamp is the right fix and I verified the spurious warn is gone.
What remains is the assertion.one_time_use conjunct.
It is untested. Deleting assertion.one_time_use and leaves the suite 246/246 green (verified twice, independently, at 3884513; the test file is unchanged since). The warn then fires for every unbounded or day-capped assertion, telling operators their IdP asked for single use when it did not, and CI stays green.
It is also arguably the wrong gate. "The record lapses before acceptance does" is true of every assertion, not just OneTimeUse ones — TEST 37 pins the replay_ttl fallback and TEST 39 the day cap for plain assertions, and the code says nothing about either. Verified by running the byte-identical fixture without <saml:OneTimeUse/>: zero log output about the same shrinkage. The IdP that never stamps OneTimeUse is the majority of traffic and gets no signal; the one that already told you it cares gets the line. one_time_use reads more naturally in the message text than in the gate.
Either way the coverage half is one line on an existing fixture: --- no_error_log: stays acceptable past its record on TEST 45 (:1312, unbounded, no OneTimeUse) or TEST 39 (:1141, day-capped) makes the mutation fail and still passes at head.
There was a problem hiding this comment.
Coverage taken (c3d5bc5): the day-capped and fallback fixtures, TESTs 39 and 45, assert the warn's absence, so dropping the one_time_use conjunct fails the suite.
The gate stays narrow. For an unstamped assertion the shrunken record is #50's stated trade-off: the README's the record is bounded even where acceptance is not paragraph documents both bounds, TESTs 37-39 pin them silently, and both need an IdP outside shipped defaults. A per-login warn for that would relitigate #50's contract here; if the general shrinkage deserves a line, that is a #50-scoped issue on its own. The stamped case warns because the IdP asked for something the record stops delivering, and that request is what separates the rows.
| opts.replay_dict, ": ", add_err, | ||
| ", this login is not covered by replay tracking") | ||
| ", this login is not covered by replay tracking", | ||
| assertion.one_time_use and " though it carries OneTimeUse" or "") |
There was a problem hiding this comment.
The negative direction of this suffix is untested: a mutant that appends it unconditionally passes all 246 tests.
Verified by replacing assertion.one_time_use and " though it carries OneTimeUse" or "" with the unconditional string — suite stays 246/246 green. TEST 40's --- error_log is a plain substring (in saml_replay_full: no memory, this login is not covered by replay tracking) for id='untracked', an assertion carrying no OneTimeUse, and that string is a strict prefix of the longer message, so it still matches.
So a regression that tells every zone-exhausted login its IdP asked for single use — when it did not — ships with green CI. One line closes it: --- no_error_log: though it carries OneTimeUse on TEST 40.
There was a problem hiding this comment.
Taken (672ccd7): TEST 40 asserts though it carries OneTimeUse stays out of its line, with the [crit]/[alert]/[emerg] guards carried explicitly since a block-level no_error_log replaces the injected list.
The warn ran before safe_add, so it described a record the add could still refuse for room, one already holding against a replay, or one the multi-assertion rollback was about to take back. The messages are gathered per record written and said once every one of them stands; a refused response says nothing.
The day-capped and fallback fixtures now assert the warn's absence, so widening its gate past one_time_use fails the suite.
TEST 40's substring is a prefix of the stamped variant, so an unconditional suffix passed; the block now asserts the suffix's absence.
There was a problem hiding this comment.
🟡 Changes recommended
Warnings can describe rejected assertions as unenforced, and the stated enforcement guarantee conflicts with cache expiry behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
lua/resty/saml.lua:430
- This warning is emitted before the assertion's time, audience, and confirmation checks—and before the later issuer and identity checks. Consequently, an expired or otherwise rejected
OneTimeUseassertion without a replay dictionary still logs that the SP could not enforce single use, even though the assertion was never used. Emit this only at the callback's final acceptance gate (analogous to replay spending), after every rejecting check has passed.
if assertion.one_time_use and not replay_dict then
ngx.log(ngx.WARN, "assertion ", loggable(assertion.id),
" carries OneTimeUse, which this SP cannot enforce without replay_dict")
end
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
| if assertion.one_time_use and outlives then | ||
| warned[#warned + 1] = "assertion " .. loggable(assertion.id) .. | ||
| " carries OneTimeUse but stays acceptable past its record" .. | ||
| ", which lapses in " .. ttl .. " seconds" |
There was a problem hiding this comment.
Right, the description overclaimed where the README already did not; it now says the single use is enforced within the record's documented bounds, and that past the record the assertion is accepted again with a warn saying so. Refusing or retaining for the full usable lifetime is the alternative #50 weighed and declined: an assertion with no expiry would need a record nothing reclaims, which is what the fallback and the day cap exist to avoid.
Closes #46. Lands before #39, so v0.2.6 does not ship the refusal.
What was wrong
#42 refuses an assertion carrying
<saml:OneTimeUse/>as a condition this SP cannot satisfy, on a reading of SAML Core 2.5.1.5 that the text does not support. Verbatim:The record of spent assertions is a SHOULD, and the one MUST binds implementations that retain assertions for future use, which this SP does not: it reads the assertion once, mints its own session, and drops the document. Every peer reads it the same way (Spring Security's default validator returns
VALIDfor it, Shibboleth SP's default policy ignores it, Keycloak's broker only checks there is at most one).Keycloak emits the condition behind a per-client toggle. Those IdPs logged in before #42 and cannot log in after it, and neither
saml-authplugin exposesreplay_dict, so there is no configuration that gets past the refusal.What it does now
OneTimeUseis back onis_known_condition, and the reader carries it asone_time_useonsaml_assertion_tand the Lua table.replay_dictset, nothing more happens: fix: let an assertion be presented only once #50 already remembers every accepted assertion, so the single use the IdP asked for is enforced within the record's documented bounds (per instance, and for as long as the record lives: until acceptance ends, capped at a day, orreplay_ttlwhen nothing bounds acceptance). Past the record the assertion is accepted again, and the login says so atwarnreplay_dictunset, the login goes through and a warning names the option:assertion <id> carries OneTimeUse, which this SP cannot enforce without replay_dictA condition the reader has never heard of is still refused.
Tests
TEST 13 now expects
302and the warning on the SP with no dict, and TEST 48 presents anOneTimeUseassertion twice on the SP with one:302then401 has been presented already, with no warning. Both fail onmainas it stands and pass here; the rest oft/assertion-conditions.tandt/login-callback.tare unchanged and green.Summary by CodeRabbit
OneTimeUsecondition.