fix: bind the assertion to the request this SP issued - #43
fix: bind the assertion to the request this SP issued#43shreemaan-abhishek wants to merge 3 commits into
Conversation
login generated an AuthnRequest ID and threw it away, so nothing tied the response back to a login this SP started. An assertion captured from one login stayed usable in any later one. The ID is kept on the session now. A SubjectConfirmationData naming a different request makes that confirmation unsatisfiable, and a Response answering a different request is refused outright. The confirmation is the binding that holds: it sits inside the signature, while the Response around it is usually unsigned.
|
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
Binds SAML responses and signed subject confirmations to the originating authentication request.
Changes:
- Persists each generated AuthnRequest ID in the session.
- Validates
InResponseTovalues and clears request state after success. - Adds end-to-end mismatch and success coverage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
lua/resty/saml.lua |
Stores and validates the request ID. |
src/lua_saml.c |
Exposes root InResponseTo to Lua. |
t/assertion-conditions.t |
Tests mismatched and matching request IDs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| -- the Response is often left unsigned, so this only catches a stray answer; | ||
| -- the binding that holds is the one inside the signed assertion below | ||
| local in_response_to = saml.doc_in_response_to(doc) | ||
| if in_response_to and in_response_to ~= expected.request_id then |
There was a problem hiding this comment.
Neither InResponseTo check ever requires the binding to be present, and the side an attacker controls is the removable one — so against a real replay this adds nothing for a large class of IdPs.
The outer check here reads an attribute on the <samlp:Response> wrapper, which is unsigned in the default shape your own tests use (insert_after = { XMLNS_ASSERTION, "Issuer" }). Deleting the attribute is not a parse or schema error, so in_response_to comes back nil and the check is skipped rather than failed.
The inner one at line 334 is the one meant to hold, but it is the same if x and x ~= expected shape, and InResponseTo on SubjectConfirmationData is optional in the schema — plenty of IdPs omit it, and IdP-initiated SSO omits it by definition.
I ran the combination through your harness: an IdP-signed assertion whose SubjectConfirmationData has no InResponseTo, delivered in a Response with the attribute deleted, returns 302 /. saml.doc_in_response_to(doc) is nil so :433 does not fire, subject_confirmations[1].in_response_to is nil so :334 does not fire, and the login is bound to no request at all. Put the attribute back and the same document is correctly rejected, which shows the check works and is simply skippable.
TEST 18 and 19 both feed a wrong ID, which is the easy half; there is no test for a removed one. If this is meant to be a binding, it needs to be "at least one of the two was present and matched" — ideally opt-in so IdPs that genuinely do not send it keep working.
Worth noting this is also reachable via the empty-<SubjectConfirmation/> shape I flagged on #42: one extra element next to a correctly bound confirmation makes :334 unreachable even when the IdP does send InResponseTo.
| local acs_url = saml_get_redirect_uri(opts.login_callback_uri) | ||
| local expected = { | ||
| acs_url = saml_get_redirect_uri(opts.login_callback_uri), | ||
| request_id = sess:get("saml_request_id"), |
There was a problem hiding this comment.
saml_request_id only exists on sessions minted by the new login, and sessions are cookie-backed, so during a rolling upgrade every login that started before the deploy comes back with saml_state but no saml_request_id. expected.request_id is nil, and any IdP that does send InResponseTo on the Response — Keycloak, Okta and ADFS all do — hits line 433 and dead-ends at 401 rather than bouncing back to the IdP.
Failing closed is the right instinct in general, but a nil request_id means "this session predates the binding", not "this response answers someone else". Degrading to #42's behaviour when it is nil would ride out the upgrade window without weakening anything for new sessions.
Either way this is user-visible, and #43 is the only one of the three that does not touch the README — the new saml_request_id session key and the upgrade behaviour are both worth a line.
| 302 / | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Heads up that none of these tests run in CI. .github/workflows/test.yml triggers on pull_request: branches: [ main ], which filters on the base branch, and this PR's base is fix/assertion-conditions. gh pr checks 42 shows a build check; 43 and 44 show only CodeRabbit and the CLA.
So TESTs 18-20 here and 21-23 in #44 have never executed anywhere but your machine. Since these are the evidence for both the request binding and the replay cache, worth either adding the stacked branches to the workflow filter or retargeting to main before merge rather than after.
Two things I hit running the suite locally that are worth fixing while you are in here:
TEST_NGINX_USE_HUP=1 prove t/assertion-conditions.t fails 5 subtests — TESTs 22 and 23 get assertion a1 has been presented already, because nginx reuses an shm zone of the same name and size across a reload, so saml_replay carries sp|a1 over from TEST 21. It passes today only because Test::Nginx fully restarts per block by default. A ngx.shared.saml_replay:flush_all() at the top of each replay block, or unique IDs per block, makes them self-contained.
The block preprocessor sets no_error_log => "[error]" only when neither error_log nor no_error_log is defined, so the mixed blocks that assert both a rejection and a success (TESTs 4, 7, 13, 14, 21) assert nothing about unexpected errors on their success path. I confirmed by injecting an ngx.log(ngx.ERR, ...) just before the final redirect: nine blocks catch it, those five do not.
| local request_uri = ngx.var.request_uri | ||
| -- kept so the callback can tell the answer to this request from the answer | ||
| -- to some other one | ||
| local request_id = generate_saml_id() |
There was a problem hiding this comment.
Minor and pre-existing, but this PR is what makes it load-bearing. uuid.seed() runs at module scope (line 3), and jit-uuid seeds with ngx.time() + ngx.worker.pid(). If resty.saml is first required from init_by_lua, that is the master's pid and the forked workers all inherit the same PRNG state, so every worker emits the same UUID sequence.
Until now that only affected saml_state; now it is also the request ID the InResponseTo checks pin against. Worth a README note that the module has to be required — or uuid.seed() called — from init_worker_by_lua.
Part of #37, item 4. Stacked on #42, which brought items 1 to 3; the base branch retargets to
mainwhen that merges. Item 5, the assertion replay cache, follows.What was wrong
generate_saml_idminted an ID for everyAuthnRequestandloginthrew it away. Nothing afterwards tied the response back to a login this SP had started, so an assertion captured from one login stayed usable in any later one.RelayStatedoes not cover this. It is opaque state this SP chose, it is not signed, and the party replaying an assertion controls their own browser session: start a fresh login to get a matchingsaml_state, then post the old assertion back with theRelayStatethat login handed out.What it does now
loginkeeps the ID it issued on the session assaml_request_idand clears it on success alongsidesaml_state.login_callbackthen refuses:ResponsewhoseInResponseTonames a different requestSubjectConfirmationDatawhoseInResponseTonames a different request, by making that confirmation unsatisfiable, which is where the confirmation rules from fix: weigh the conditions an assertion attaches to itself #42 already liveThe second is the one that binds.
SubjectConfirmationDatasits inside the assertion the signature covers, so an attacker replaying a captured assertion cannot rewrite it. TheResponsearound it is usually left unsigned, so itsInResponseTocatches a stray or misdirected answer rather than a deliberate one, which is why both are checked rather than only the outer.An absent
InResponseTois not enforced, keeping the line #42 drew: an IdP that omits it keeps working.Tests
TESTs 18 to 20 in
t/assertion-conditions.t. TEST 20 builds the response after the SP has issued its request, reading the ID back out of the redirect the SP sent the browser, so it exercises a genuine matching ID rather than a fixture.Full run on this branch, 60 subtests, all pass. Rebuilt against #42's
src/andlua/with the new tests kept, the two that should fail do and only those:TEST 20 passes on both, which is the point of it.