fix: let an assertion be presented only once #44
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -320,6 +320,9 @@ end | |
| -- what stops an assertion minted for another SP in the same federation. | ||
| local DEFAULT_CLOCK_SKEW = 60 | ||
|
|
||
| -- how long an assertion that sets no expiry of its own is remembered | ||
| local DEFAULT_REPLAY_TTL = 600 | ||
|
|
||
| local function time_bounds_ok(not_before, not_on_or_after, now, skew) | ||
| local opens, closes, err | ||
|
|
||
|
|
@@ -501,6 +504,52 @@ local function issuers_allowed(allowed, issuers) | |
| return true | ||
| end | ||
|
|
||
| -- A bearer assertion is good for one login. Nothing above stops the same one | ||
| -- being presented again inside its validity window, so its ID is kept until it | ||
| -- expires and a second presentation is refused. | ||
| -- | ||
| -- The window from the assertion's own Conditions decides how long the entry | ||
| -- lives, so the cache holds exactly what is still usable. An assertion that | ||
| -- names no expiry is replayable for as long as it is remembered, which is what | ||
| -- replay_ttl bounds. | ||
| local function assertions_unused(dict, opts, assertions, now) | ||
| local skew = opts.clock_skew or DEFAULT_CLOCK_SKEW | ||
|
|
||
| for _, assertion in ipairs(assertions) do | ||
| if not assertion.id then | ||
| 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 So for the ordinary shape of 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 Two smaller things on the same lines:
|
||
| if assertion.not_on_or_after then | ||
| local expires = parse_iso8601_utc_time(assertion.not_on_or_after) | ||
| if expires then | ||
| ttl = expires + skew - now | ||
| end | ||
| end | ||
| if ttl < 1 then | ||
|
Comment on lines
+523
to
+530
There was a problem hiding this comment. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/sh
set -eu
printf '%s\n' '--- replay_ttl references ---'
rg -n -C 5 'replay_ttl|DEFAULT_REPLAY_TTL|function _M\.new|_M\.new' lua/resty/saml.lua
printf '%s\n' '--- cited implementation ---'
sed -n '500,540p' lua/resty/saml.lua
printf '%s\n' '--- initialization implementation ---'
sed -n '820,885p' lua/resty/saml.lua
printf '%s\n' '--- available Lua runtimes ---'
command -v lua || true
command -v luajit || trueRepository: api7/lua-resty-saml Length of output: 6008 🏁 Script executed: #!/bin/sh
set -eu
printf '%s\n' '--- assertions_unused remainder and callers ---'
sed -n '515,570p' lua/resty/saml.lua
rg -n -C 12 'assertions_unused|replay_dict|replay_ttl' lua t test spec README.md 2>/dev/null || true
printf '%s\n' '--- Lua comparison and clamp probe ---'
lua <<'LUA'
local values = {
negative = -5,
zero = 0,
positive = 30,
string = "30",
nan = 0/0,
positive_infinity = math.huge,
negative_infinity = -math.huge,
}
for name, value in pairs(values) do
local ok, result = pcall(function()
local ttl = value
if ttl < 1 then
ttl = 1
end
return ttl
end)
if ok then
print(name, tostring(value), "ok", tostring(result))
else
print(name, tostring(value), "error", result)
end
end
LUARepository: api7/lua-resty-saml Length of output: 13242 🌐 Web query:
💡 Result: In OpenResty's Citations:
Validate When replay tracking is enabled, require a finite number greater than zero. A truthy string causes 🤖 Prompt for AI Agents |
||
| ttl = 1 | ||
| end | ||
|
|
||
| -- an SP name in the key so instances sharing one dict stay apart | ||
| local key = tostring(opts.sp_issuer) .. "|" .. assertion.id | ||
| 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) | ||
|
Comment on lines
+536
to
+541
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 Whatever transactional shape fixes the ordering should also move the commit past the last thing that can reject.
Comment on lines
+536
to
+541
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- relevant source ---'
sed -n '300,350p' lua/resty/saml.lua
sed -n '490,565p' lua/resty/saml.lua
sed -n '625,665p' lua/resty/saml.lua
sed -n '840,875p' lua/resty/saml.lua
printf '%s\n' '--- replay/assertion references ---'
rg -n -C 3 'assertions_unused|replay_dict|assertion\.id|dict:add|dict:delete' lua t spec .github 2>/dev/null || true
printf '%s\n' '--- candidate tests ---'
git ls-files | rg -i 'test|spec' | head -80Repository: api7/lua-resty-saml Length of output: 12652 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- assertion extraction and acceptance ---'
sed -n '360,475p' lua/resty/saml.lua
rg -n -C 4 'multiple|assertions|replay|presented already|could not track|saml_replay' t lua/resty/saml.lua
printf '%s\n' '--- read-only partial-write model ---'
python3 - <<'PY'
class SharedDict:
def __init__(self, keys=()):
self.keys = set(keys)
self.deleted = []
def add(self, key, value, ttl):
if key in self.keys:
return False, "exists", False
if key == "sp|unrecordable":
return False, "no memory", False
self.keys.add(key)
return True, None, False
def delete(self, key):
self.deleted.append(key)
self.keys.discard(key)
def assertions_unused(dict_, sp_issuer, assertions):
for assertion in assertions:
key = f"{sp_issuer}|{assertion['id']}"
added, err, forcible = dict_.add(key, True, 600)
if not added:
if err == "exists":
return False, f"assertion {assertion['id']} has been presented already"
return False, f"could not track assertion {assertion['id']}: {err}"
return True, None
dict_ = SharedDict({"sp|later-used"})
result = assertions_unused(dict_, "sp", [
{"id": "earlier-new"},
{"id": "later-used"},
])
print("result:", result)
print("keys after failure:", sorted(dict_.keys))
assert result[0] is False
assert "sp|earlier-new" in dict_.keys
dict_ = SharedDict()
result = assertions_unused(dict_, "sp", [
{"id": "earlier-new"},
{"id": "unrecordable"},
])
print("result:", result)
print("keys after storage failure:", sorted(dict_.keys))
assert result[0] is False
assert "sp|earlier-new" in dict_.keys
PYRepository: api7/lua-resty-saml Length of output: 18986 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- assertion extraction and acceptance ---'
sed -n '360,475p' lua/resty/saml.lua
rg -n -C 4 'multiple|assertions|replay|presented already|could not track|saml_replay' t lua/resty/saml.lua
printf '%s\n' '--- read-only partial-write model ---'
python3 - <<'PY'
class SharedDict:
def __init__(self, keys=()):
self.keys = set(keys)
def add(self, key, value, ttl):
if key in self.keys:
return False, "exists", False
if key == "sp|unrecordable":
return False, "no memory", False
self.keys.add(key)
return True, None, False
def assertions_unused(dict_, sp_issuer, assertions):
for assertion in assertions:
key = f"{sp_issuer}|{assertion['id']}"
added, err, _ = dict_.add(key, True, 600)
if not added:
if err == "exists":
return False, f"assertion {assertion['id']} has been presented already"
return False, f"could not track assertion {assertion['id']}: {err}"
return True, None
dict_ = SharedDict({"sp|later-used"})
result = assertions_unused(dict_, "sp", [
{"id": "earlier-new"},
{"id": "later-used"},
])
print("result:", result)
print("keys after failure:", sorted(dict_.keys))
assert result[0] is False
assert "sp|earlier-new" in dict_.keys
dict_ = SharedDict()
result = assertions_unused(dict_, "sp", [
{"id": "earlier-new"},
{"id": "unrecordable"},
])
print("result:", result)
print("keys after storage failure:", sorted(dict_.keys))
assert result[0] is False
assert "sp|earlier-new" in dict_.keys
PYRepository: api7/lua-resty-saml Length of output: 18986 Roll back replay records when a later assertion cannot be recorded.
🤖 Prompt for AI Agents |
||
| end | ||
| if forcible then | ||
|
There was a problem hiding this comment. Beyond the message wording, the semantics here are fail-open.
|
||
| ngx.log(ngx.WARN, "the assertion replay dict is full, older assertions are ", | ||
| "no longer tracked") | ||
|
Comment on lines
+544
to
+545
|
||
| end | ||
| end | ||
|
|
||
| return true | ||
| end | ||
|
|
||
|
|
||
| local function login_callback(self, opts) | ||
| local sess = session.start(self.session_config) | ||
|
|
||
|
|
@@ -584,12 +633,21 @@ local function login_callback(self, opts) | |
| ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) | ||
| end | ||
|
|
||
| local now = ngx.time() | ||
| local acceptable, reason = assertions_acceptable(opts, assertions, expected, now) | ||
| if not acceptable then | ||
| ngx.log(ngx.ERR, "response from IdP rejected: ", loggable(reason)) | ||
| ngx.exit(ngx.HTTP_UNAUTHORIZED) | ||
| end | ||
|
|
||
| if self.replay_dict then | ||
| local unused, used_reason = assertions_unused(self.replay_dict, opts, assertions, now) | ||
| if not unused then | ||
| ngx.log(ngx.ERR, "response from IdP rejected: ", loggable(used_reason)) | ||
| ngx.exit(ngx.HTTP_UNAUTHORIZED) | ||
| end | ||
| end | ||
|
Comment on lines
+643
to
+649
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- target file outline ---'
ast-grep outline lua/resty/saml.lua 2>/dev/null | head -200 || true
printf '%s\n' '--- relevant symbols and references ---'
rg -n -C 8 'assertions_unused|login_callback|replay_dict|replay_ttl|idp_issuers|name_id|SessionNotOnOrAfter' lua spec test tests 2>/dev/null | head -1000Repository: api7/lua-resty-saml Length of output: 17168 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- replay helper ---'
sed -n '500,555p' lua/resty/saml.lua
printf '%s\n' '--- callback validation flow ---'
sed -n '553,705p' lua/resty/saml.lua
printf '%s\n' '--- test files ---'
git ls-files | rg '(^|/)(spec|test|tests)(/|$)|saml.*spec|saml.*test' | head -200
printf '%s\n' '--- replay-related tests and fixtures ---'
rg -n -C 5 'replay|assertion|unexpected issuer|SessionNotOnOrAfter|name.?id' spec test tests 2>/dev/null | head -1200Repository: api7/lua-resty-saml Length of output: 8533 🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
source = Path("lua/resty/saml.lua").read_text()
replay = source.index("if self.replay_dict then")
issuer = source.index("local allowed, unexpected = issuers_allowed", replay)
name_id = source.index("if not name_id then", issuer)
session_expiry = source.index("local session_expires = saml.doc_session_expires", name_id)
save = source.index("sess:save()", session_expiry)
assert replay < issuer < name_id < session_expiry < save
print("callback order: replay < issuer < name_id < session expiry < session save")
class Dict:
def __init__(self, fail_on=None):
self.values = []
self.fail_on = fail_on
def add(self, key):
if key in self.values:
return False, "exists"
if key == self.fail_on:
return False, "no memory"
self.values.append(key)
return True, None
def assertions_unused(assertions, fail_on=None):
d = Dict(fail_on)
for assertion in assertions:
added, err = d.add(assertion)
if not added:
return False, err, d.values
return True, None, d.values
accepted, reason, recorded = assertions_unused(["a1"])
print("later validation rejection leaves:", recorded)
assert recorded == ["a1"]
accepted, reason, recorded = assertions_unused(["a1", "a2"], fail_on="a2")
print("later assertion add failure leaves:", recorded)
assert recorded == ["a1"]
PY
printf '%s\n' '--- tracked repository files near the root ---'
git ls-files | head -200
printf '%s\n' '--- test/configuration indicators ---'
git ls-files | rg -i '(^|/)(spec|test|tests|t)/|busted|luacheck|resty|fixture|assertion|replay' | head -300Repository: api7/lua-resty-saml Length of output: 2026 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- callback tests ---'
rg -n -C 12 'callback|issuer|name.?id|SessionNotOnOrAfter|shared_dict|replay|assertion' t/login-callback.t t/assertion-conditions.t t/saml-post.t
printf '%s\n' '--- issuer extraction implementation ---'
rg -n -C 12 'doc_issuers|doc_issuer|Issuer|issuer' src lua/resty/saml.lua | head -1000
printf '%s\n' '--- test harness setup ---'
sed -n '1,220p' t/login-callback.tRepository: api7/lua-resty-saml Length of output: 50376 Defer replay recording until callback validation is complete. Replay tracking currently runs before issuer, identity, and
🤖 Prompt for AI Agents |
||
|
|
||
| local issuer = saml.doc_issuer(doc) | ||
| local attrs = saml.doc_attrs(doc) | ||
| local name_id = saml.doc_name_id(doc) | ||
|
|
@@ -800,6 +858,10 @@ function _M.new(opts) | |
| obj.idp_cert_func = function(doc) return idp_cert end | ||
| obj.auth_protocol_binding_method = opts.auth_protocol_binding_method | ||
| obj.idp_issuers = issuer_set(opts.idp_issuers) | ||
| if opts.replay_dict then | ||
| obj.replay_dict = assert(ngx.shared[opts.replay_dict], | ||
|
There was a problem hiding this comment.
Returning Separately, for this option to be reachable at all the gateway needs |
||
| "no lua_shared_dict named " .. opts.replay_dict) | ||
| end | ||
| local cookie_secure, cookie_same_site | ||
| if opts.auth_protocol_binding_method == "HTTP-POST" then | ||
| cookie_secure = true | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,11 @@ _EOC_ | |
| lua_package_path '$pwd/lua/?.lua;$pwd/deps/share/lua/5.1/?.lua;$pwd/t/?.lua;;'; | ||
| lua_package_cpath '$pwd/?.so;$pwd/deps/lib/lua/5.1/?.so;;'; | ||
|
|
||
| # blocks driving it flush it first: a zone of the same name and size is | ||
| # reused across a reload, so entries otherwise outlive the block that made | ||
| # them under TEST_NGINX_USE_HUP=1 | ||
| lua_shared_dict saml_replay 1m; | ||
|
|
||
| init_by_lua_block { | ||
| saml = require "saml" | ||
| local err = saml.init({ debug = true, data_dir = os.getenv("SAML_DATA_DIR") }) | ||
|
|
@@ -100,6 +105,7 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== | |
| skew = { clock_skew = 300 }, | ||
| audiences = { sp_audiences = { "https://sp.example.com/metadata" } }, | ||
| acs = { sp_acs_url = "http://127.0.0.1:1984/acs" }, | ||
| replay = { replay_dict = "saml_replay" }, | ||
|
There was a problem hiding this comment. A few coverage gaps I found by mutating
Also, on #42's side but same file: removing skew tolerance from |
||
| } | ||
| SPS = {} | ||
|
|
||
|
|
@@ -986,3 +992,59 @@ offers no subject confirmation this SP can satisfy | |
| 302 http://127.0.0.1:1984/idp | ||
| --- error_log | ||
| session carries no request id, starting the login again | ||
|
|
||
|
|
||
| === TEST 32: an assertion is good for one login | ||
| --- config | ||
| location /t { | ||
| content_by_lua_block { | ||
| ngx.shared.saml_replay:flush_all() | ||
| local xml = saml_response({ conditions = conditions({ not_on_or_after = at(600) }) }) | ||
| ngx.say(login_with("replay", xml)) | ||
| ngx.say(login_with("replay", xml)) | ||
| } | ||
| } | ||
| --- response_body | ||
| 302 / | ||
| 401 nil | ||
| --- error_log | ||
| assertion a1 has been presented already | ||
|
|
||
|
|
||
| === TEST 33: a second assertion of its own is accepted | ||
| --- config | ||
| location /t { | ||
| content_by_lua_block { | ||
| ngx.shared.saml_replay:flush_all() | ||
| ngx.say(login_with("replay", saml_response({ id = "a1" }))) | ||
| ngx.say(login_with("replay", saml_response({ id = "a2" }))) | ||
| } | ||
| } | ||
| --- response_body | ||
| 302 / | ||
| 302 / | ||
|
|
||
|
|
||
| === TEST 34: an assertion is remembered for as long as it is usable | ||
| --- config | ||
| location /t { | ||
| content_by_lua_block { | ||
| ngx.shared.saml_replay:flush_all() | ||
| ngx.say(login_with("replay", saml_response({ | ||
| conditions = conditions({ not_on_or_after = at(600) }), | ||
| }))) | ||
| -- the window plus the skew allowance, which is when it stops being | ||
| -- accepted and so stops being worth remembering | ||
| local ttl = ngx.shared.saml_replay:ttl("sp|a1") | ||
| ngx.say("tracked: ", ttl > 600 and ttl <= 660) | ||
|
|
||
| ngx.say(login_with("replay", saml_response({ id = "a2" }))) | ||
| local default = ngx.shared.saml_replay:ttl("sp|a2") | ||
| ngx.say("default: ", default > 590 and default <= 600) | ||
| } | ||
| } | ||
| --- response_body | ||
| 302 / | ||
| tracked: true | ||
| 302 / | ||
| default: true | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things worth stating in these rows.
lua_shared_dictis 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 theSubjectConfirmationData/@NotOnOrAfterwindow that also keeps the assertion acceptable. Details in the thread onassertions_unused.