Skip to content

fix: bind the assertion to the request this SP issued #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shreemaan-abhishek wants to merge 3 commits into
base: fix/assertion-conditions
Choose a base branch
from
+131 −35
Open

fix: bind the assertion to the request this SP issued #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a9fa958
fix: bind the assertion to the request this SP issued
shreemaan-abhishek Aug 18, 2026
d59bd4a
Merge branch 'fix/assertion-conditions' into fix/assertion-in-respons…
shreemaan-abhishek Aug 19, 2026
c2edc13
Merge branch 'fix/assertion-conditions' into fix/assertion-in-respons…
shreemaan-abhishek Aug 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 29 additions & 10 deletions lua/resty/saml.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,13 +158,13 @@ local AUTHN_REQUEST = [[
</samlp:AuthnRequest>
]]

local function authn_request(opts)
local function authn_request(opts, request_id)
return interp(AUTHN_REQUEST, {
acs_url = sp_acs_url(opts),
destination = opts.idp_uri,
issue_instant = os.date("!%Y-%m-%dT%TZ"),
issuer = opts.sp_issuer,
uuid = generate_saml_id(),
uuid = request_id,
auth_protocol_binding_method = opts.auth_protocol_binding_method,
})
end
Expand Down Expand Up @@ -214,13 +214,17 @@ local function login(self, opts)

local state = uuid.generate_v4()
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()

@jarvis9443 jarvis9443 Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


sess:set("saml_state", state)
sess:set("saml_request_id", request_id)
sess:set("request_uri", request_uri)
sess:save()

local query_str, err = create_redirect(self.sign_key, {
SAMLRequest = authn_request(opts),
SAMLRequest = authn_request(opts, request_id),
SigAlg = RSA_SHA_512_HREF,
RelayState = state,
})
Expand Down Expand Up @@ -332,8 +336,11 @@ end
-- The assertion may be presented to whoever the Recipient names, for as long as
-- the confirmation data allows. Several confirmations can be offered and any one
-- of them being satisfiable is enough.
local function confirmation_ok(confirmation, acs_url, now, skew)
if confirmation.recipient and confirmation.recipient ~= acs_url then
local function confirmation_ok(confirmation, expected, now, skew)
if confirmation.recipient and confirmation.recipient ~= expected.acs_url then
return false
end
if confirmation.in_response_to and confirmation.in_response_to ~= expected.request_id then
return false
end
return (time_bounds_ok(confirmation.not_before, confirmation.not_on_or_after, now, skew))
Expand All @@ -342,7 +349,7 @@ end

-- Every top-level assertion the verified signature left in the document is one
-- the readers draw identity from, so every one of them has to hold up.
local function assertions_acceptable(opts, assertions, acs_url, now)
local function assertions_acceptable(opts, assertions, expected, now)
local skew = opts.clock_skew or DEFAULT_CLOCK_SKEW
local accepted = opts.sp_audiences or { opts.sp_issuer }

Expand Down Expand Up @@ -373,7 +380,7 @@ local function assertions_acceptable(opts, assertions, acs_url, now)
if #confirmations > 0 then
local satisfiable = false
for _, confirmation in ipairs(confirmations) do
if confirmation_ok(confirmation, acs_url, now, skew) then
if confirmation_ok(confirmation, expected, now, skew) then
satisfiable = true
break
end
Expand Down Expand Up @@ -425,10 +432,21 @@ local function login_callback(self, opts)
ngx.exit(ngx.HTTP_UNAUTHORIZED)
end

local acs_url = sp_acs_url(opts)
local expected = {
acs_url = sp_acs_url(opts),
request_id = sess:get("saml_request_id"),

@jarvis9443 jarvis9443 Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

-- 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

@jarvis9443 jarvis9443 Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ngx.log(ngx.ERR, "response from IdP answers request ", in_response_to)
ngx.exit(ngx.HTTP_UNAUTHORIZED)
end

local destination = saml.doc_destination(doc)
if destination and destination ~= acs_url then
if destination and destination ~= expected.acs_url then
ngx.log(ngx.ERR, "response from IdP is addressed to ", destination)
ngx.exit(ngx.HTTP_UNAUTHORIZED)
end
Expand All @@ -439,7 +457,7 @@ local function login_callback(self, opts)
ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR)
end

local acceptable, reason = assertions_acceptable(opts, assertions, acs_url, ngx.time())
local acceptable, reason = assertions_acceptable(opts, assertions, expected, ngx.time())
if not acceptable then
ngx.log(ngx.ERR, "response from IdP rejected: ", reason)
ngx.exit(ngx.HTTP_UNAUTHORIZED)
Expand Down Expand Up @@ -478,6 +496,7 @@ local function login_callback(self, opts)

-- clear temporary authentication state no longer needed after successful login
sess:set("saml_state", nil)
sess:set("saml_request_id", nil)
sess:set("request_uri", nil)
sess:save()

Expand Down
29 changes: 29 additions & 0 deletions src/lua_saml.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,34 @@ static int doc_attrs(lua_State* L) {
}


/***
Get the InResponseTo attribute of the root message
@function doc_in_response_to
@tparam xmlDoc* doc
@treturn ?string in_response_to
*/
static int doc_in_response_to(lua_State* L) {
lua_settop(L, 1);
xmlDoc* doc = doc_check(L, 1);
lua_pop(L, 1);

xmlNode* root = xmlDocGetRootElement(doc);
if (root == NULL) {
lua_pushnil(L);
return 1;
}

xmlChar* in_response_to = xmlGetNoNsProp(root, (const xmlChar*)"InResponseTo");
if (in_response_to == NULL) {
lua_pushnil(L);
} else {
lua_pushstring(L, (char*)in_response_to);
xmlFree(in_response_to);
}
return 1;
}


/***
Get the Destination attribute of the root message
@function doc_destination
Expand Down Expand Up @@ -1294,6 +1322,7 @@ static const struct luaL_Reg saml_funcs[] = {
{"doc_attrs", doc_attrs},
{"doc_assertions", doc_assertions},
{"doc_destination", doc_destination},
{"doc_in_response_to", doc_in_response_to},

{"key_read_memory", key_read_memory},
{"key_read_file", key_read_file},
Expand Down
Loading