Repository navigation
Honour OneTimeUse when replay tracking is configured #46
Description
Activity
🔗 Related PRs
#42 - fix: weigh the conditions an assertion attaches to itself [open]
📝 Issue Planner
Check the box below or use the
@coderabbitai plancommand to generate an implementation plan and prompts that you can use with your favorite coding assistant.- Create Plan
🧪 Issue enrichment is currently in open beta.
You can configure auto-planning by selecting labels in the issue_enrichment configuration.
To disable automatic issue enrichment, add the following to your
.coderabbit.yaml:issue_enrichment: auto_enrich: enabled: false
💬 Have feedback or questions? Drop into our discord!
Two updates from #50, which replaced #44 after it was closed.
The record this depends on now exists there:
replay_dictremembers every accepted assertion until it expires, which is the stateOneTimeUsedemands. The refusal is untouched, so a deployment with the dict configured still cannot log in against an IdP that stamps its assertions single-use. Measured on #50 by @jarvis9443: 401,carries a condition this SP cannot satisfy: OneTimeUse.The second is about when this has to land. Before #42 the condition was silently ignored, so those IdPs logged in and simply got no single-use enforcement. #42 turned that into an outright refusal, which means an IdP that worked before stops working. That regression ships the moment v0.2.6 goes out, so this needs to be merged before #39 rather than whenever it is convenient.
Scope is smaller than this issue assumes, if the C reader is left alone: the refusal already carries the element name, so
assertions_acceptablecan acceptOneTimeUsewhilereplay_dictis set and keep refusing it while it is not. Carrying it throughsaml_assertion_tas its own flag is the tidier shape and can follow.
What
OneTimeUseis refused as of #42, on the grounds that honouring it means remembering which assertions have been spent and this SP keeps no such record. The refusal rests on a reading of SAML Core 2.5.1.5 that the text does not support. Verbatim:The record is a SHOULD ("a relying party should maintain a cache of the assertions it has processed containing such a condition"), 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. So an assertion carrying the condition is understood and Valid whatever the SP has configured, and refusing it as Indeterminate under 2.5.1.1 rule 3 (an element "not understood") is the SP getting the spec wrong.
Every peer reads it that way. Spring Security's default validator returns
VALIDforOneTimeUse("applications should validate their own OneTimeUse conditions"), Shibboleth SP's default policy ignores it, Keycloak's broker checks only that there is at most one, python3-saml and passport-saml do nothing with it. ADFS 2.0 is the one refuser, and Microsoft's own remedy is to switch the condition off at the IdP.What it should do
Accept
OneTimeUsein every case.saml_assertion_tand a field on the Lua table, rather than letting it fall intounknown_conditionis_known_conditioninsrc/xml.creplay_dictset, nothing more is needed: fix: let an assertion be presented only once #50 already remembers every accepted assertion, so the single use the IdP asked for is enforced for realreplay_dictunset, let the login through and log a warning namingreplay_dict, so an operator reads it as something to configureWhy it is worth doing
Keycloak emits the condition behind a per-client toggle, "Include OneTimeUse Condition" (
saml.onetimeuse.condition). Before #42 those IdPs logged in and simply got no single-use enforcement. #42 turned that into an outright refusal, so an IdP that worked before stops working, and neither the APISIX nor the EEsaml-authplugin exposesreplay_dict, so there is no configuration that gets past it. That regression ships the moment v0.2.6 goes out, so this lands before #39.Refusing buys no security: the flag is the IdP's advisory to the SP, and a party replaying a captured assertion does not care whether it is present.
Notes
Raised by @jarvis9443 reviewing #42. Depends on #50, which has merged.