api7 / api7/lua-resty-saml

Replay tracking is per instance, and a complete guarantee needs a shared record

Open
#51 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Perl
Stars
2
Forks
3
Avg merge
2d 20h
Merged PRs (30d)
3

Description

What

replay_dict remembers every assertion an instance accepts, so it refuses the same one twice. An lua_shared_dict is shared between the workers of one gateway and nowhere else, so the guarantee is really at most once per replica.

Behind a load balancer, a captured assertion replayed a second time lands on a replica that has never seen it and is accepted. With N replicas and no affinity, a blind replay has an (N-1)/N chance of picking a fresh one, so on a multi-replica deployment this is closer to a speed bump than a defence.

This is written down in the README today rather than fixed, under "Remembering assertions".

Why it is not urgent

Across replicas the work is done by the request binding from #43: SubjectConfirmationData/@InResponseTo has to name the AuthnRequest this SP issued, and that ID lives in the user's own session cookie, which travels with them to whichever replica they land on. A replayed assertion is refused on any replica, because the attacker's own session names a different request.

replay_dict is the defence for what that leaves uncovered: an IdP that sends no InResponseTo, which the README also states plainly. Those deployments are exactly the ones where the per-instance limit bites.

What it should do

A shared record with an atomic add, so the first presentation wins across the whole deployment:

  • an interface the caller supplies rather than a hardcoded backend, since APISIX, the EE gateway and a bare OpenResty deployment do not agree on what they have
  • Redis is the obvious first one, through SET key value NX EX ttl, which is the same one-shot claim safe_add gives locally
  • the local dict stays the default, so nothing new is required of a deployment that does not want it
  • failure of the shared store needs the same answer the full dict got: leave the login untracked and log it, rather than refusing every login when the store is unreachable
Notes

Raised by Copilot and by @jarvis9443 reviewing #50, on the README claim rather than on the code. Depends on nothing; #50 can merge as it stands.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the README's "Remembering assertions" section, then trace the replay_dict and safe_add entry points mentioned in the issue. Define the caller-supplied shared-store interface around an atomic add, keeping the local dict as default. Done means Redis can claim assertions across replicas and store failures leave logins untracked rather than rejecting them.

Written by the indexing model from the issue text.

Assessment

Tech stack
lua, redis
Domain
authentication, backend, security
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.