Skip to content

Commit d285bed

Browse files
authored
Merge pull request #94 from MicroPyramid/dev
feat: Implement per-IP rate limiting for MFA verification
2 parents c202850 + dc3ea93 commit d285bed

44 files changed

Lines changed: 2165 additions & 178 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

CHANGELOG.md

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,74 @@ Versions follow [PEP 440](https://peps.python.org/pep-0440/). The version in
1212
`pyproject.toml` is the only place it is written; the git tag and the GitHub
1313
Release are derived from it (see [docs/contributing.md](docs/contributing.md)).
1414

15+
## 4.5.0
16+
17+
### Added
18+
19+
- **MFA session tokens, for clients that cannot hold a cookie.** `POST` to
20+
the new `session/` endpoint returns a token; present it in the
21+
`X-MFA-Session` header and a verification completed on one request is
22+
still in force on the next. This is what 4.4.0's JSON API was missing —
23+
its own docs said a cookie-less client "can call every endpoint and will
24+
still never get anywhere".
25+
26+
The token **is** a Django session key, deliberately: it is revocable
27+
(`DELETE session/`), expires with `SESSION_COOKIE_AGE`, is cleaned up by
28+
`clearsessions`, and leaves `django_mfa/session.py` as the only module
29+
that touches MFA state. It is bound to the account it was issued to, so a
30+
token presented alongside a different identity is refused rather than
31+
inherited. A request carrying no cookie at all is CSRF-exempt — nothing
32+
ambient for a cross-site page to forge — while any request with a cookie
33+
keeps full enforcement. `SESSION_ENGINE = "...signed_cookies"` cannot
34+
issue one and says so with a 501. See
35+
[docs/rest_api.md](docs/rest_api.md).
36+
- **A per-IP verification budget**, `MFA_VERIFY_IP_RATE_LIMIT`, defaulting
37+
to `"50/5m"`. `MFA_VERIFY_RATE_LIMIT` budgets guesses per *account*, which
38+
an attacker holding a list of stolen passwords routes around entirely: one
39+
guess against each of ten thousand accounts leaves every counter at 1 and
40+
none of them ever binding. A successful verification clears the user's
41+
counter and deliberately not the IP's.
42+
- **`MFA_CLIENT_IP_RESOLVER`** (default `None``REMOTE_ADDR`). `X-Forwarded-For`
43+
is **not** read unless you point this at a resolver that does, because a
44+
client-set header breaks the budget in both directions — an attacker who
45+
varies it is never throttled, one who forges your office's address locks
46+
your staff out. **Set this if you run behind a proxy.**
47+
- **`MFA_RATE_LIMIT_BACKEND`** (default `"database"`) and
48+
**`MFA_RATE_LIMIT_FAIL_OPEN`** (default `True`). See below.
49+
- **`manage.py mfa_prune`**, deleting expired rate-limit counters. Schedule
50+
it beside `django-admin clearsessions`.
51+
- **System checks `django_mfa.E007``E009`**, rejecting a malformed rate-limit
52+
spec, an unknown rate-limit backend, and an unimportable or non-callable
53+
`MFA_CLIENT_IP_RESOLVER`. Without them each of those surfaces from inside
54+
the first verification attempt after a deploy — a 500 on the challenge page
55+
for whichever user logs in first.
56+
57+
### Changed
58+
59+
- **Rate-limit counters now live in a database table by default**
60+
(`RateLimitCounter`, migration `0010`), not the cache. A cache-only counter
61+
is erased by a Redis restart, an eviction under memory pressure, or a
62+
`cache.clear()` in an unrelated deploy step, and every erasure silently
63+
hands an attacker mid-run a fresh budget with no trace. The cost is one row
64+
read per verification attempt and one write per failed one. Set
65+
`MFA_RATE_LIMIT_BACKEND = "cache"` for the previous behaviour; it needs no
66+
migration and no pruning.
67+
- **A session that has never been challenged no longer counts as verified.**
68+
`decorators.enforcement_state()` treats "holds a primary factor, and this
69+
session carries no MFA stamp at all" as `PENDING`. Previously
70+
`session.is_pending()` — which means *stamped, not yet passed* — was False
71+
for such a session and the request fell through as though it had passed.
72+
See [docs/upgrading.md](docs/upgrading.md) for who this affects; the
73+
ordinary login path is unchanged, because `user_logged_in` stamps every
74+
session it creates.
75+
76+
### Fixed
77+
78+
- Two rate-limit tests were passing spuriously: they patched
79+
`ratelimit._db_get`/`_cache_get`, which the backend table binds at import,
80+
so the "broken store" they simulated was in fact a healthy one. They now
81+
break the store at the ORM and cache boundary.
82+
1583
## 4.4.0
1684

1785
### Added

README.md

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ Django's own `user_logged_in` signal.
4646
| 🖥️ **Remember this browser** | Optional, off by default. Trust a browser for N days after one successful challenge. |
4747
|**Several keys at once** | A user can register a work laptop's Touch ID *and* a backup YubiKey, each with its own name. |
4848
| 🌍 **Six languages** | German, Spanish, French, Brazilian Portuguese, Japanese and Simplified Chinese ship translated. Switch on `USE_I18N` and they work. |
49-
| 🔌 **A JSON API** | Opt-in. Every flow above as JSON, for an SPA or mobile client that renders its own screens. No DRF dependency. |
49+
| 🔌 **A JSON API** | Opt-in. Every flow above as JSON, for an SPA or mobile client that renders its own screens. No DRF dependency, and a revocable session token for clients that hold no cookie. |
5050

5151
## Install
5252

@@ -150,10 +150,13 @@ The parts that are easy to get subtly wrong, done deliberately:
150150
handle, unknown credential, bad signature, expired ceremony, tampered payload —
151151
returns one identical generic response.
152152
- **Rate limiting that isn't an oracle.** Failed attempts are capped per user per factor
153-
(`MFA_VERIFY_RATE_LIMIT`, default 5 per 5 minutes). A locked-out attempt returns the
154-
*same* response as a wrong code, so the lockout itself leaks nothing. The counter
155-
lives in the cache with no database fallback, so it fails open rather than locking
156-
everyone out — a secondary control shouldn't be able to take your site down.
153+
(`MFA_VERIFY_RATE_LIMIT`, default 5 per 5 minutes) *and* per client address across
154+
every account (`MFA_VERIFY_IP_RATE_LIMIT`, default 50 per 5 minutes) — the second
155+
catches one guess sprayed at ten thousand accounts, which the first cannot see. A
156+
locked-out attempt returns the *same* response as a wrong code, so the lockout itself
157+
leaks nothing. Counters are rows by default, so a cache restart can't quietly hand an
158+
attacker a fresh budget, and the limiter fails open if its store is unreachable — a
159+
secondary control shouldn't be able to take your site down.
157160
- **Cloned-authenticator detection.** WebAuthn signature counters are checked on every
158161
assertion, with an explicit carve-out for authenticators that legitimately never
159162
implement one (iCloud passkeys always report 0).

django_mfa/api/tokens.py

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
"""MFA state for a client that cannot hold a cookie.
2+
3+
``MFA_API_AUTHENTICATION`` answers *who you are* from a bearer token, but
4+
whether this caller has passed a challenge -- and how recently -- is read from
5+
``request.session`` by the same code the browser flow uses. A client that
6+
discards cookies therefore arrives with a brand-new empty session on every
7+
request, and a verification completed in one can never be seen by the next.
8+
9+
This module closes that without inventing a second home for MFA state. The
10+
token a client holds **is** a session key: ``issue()`` mints a server-side
11+
session, hands back its key, and ``load()`` puts it on the request as
12+
``request.session`` when the client presents it in the ``X-MFA-Session``
13+
header. Everything downstream -- django_mfa.session, the enforcement rungs,
14+
step-up freshness -- runs unchanged and unaware.
15+
16+
What that buys, and why it beat a signed stateless token holding
17+
``{"verified": true}``:
18+
19+
* **Revocation.** ``DELETE`` on the session endpoint flushes the row. A signed
20+
token is valid until it expires, and cannot be withdrawn from a device its
21+
holder has lost.
22+
* **Expiry, already solved.** ``SESSION_COOKIE_AGE`` and ``clearsessions``
23+
apply as-is.
24+
* **One reader.** ``django_mfa/session.py`` stays the only module that touches
25+
the ``mfa`` session key, which is the rule the rest of this package is
26+
built on.
27+
28+
Three properties below are load-bearing rather than tidy; each says so at its
29+
own definition: the token is bound to the user it was issued to, no
30+
``Set-Cookie`` is ever sent for one, and CSRF is enforced for cookie requests
31+
only.
32+
33+
Not covered: the passkey endpoints. Passwordless login establishes identity,
34+
and a token cannot be issued before there is an identity to bind it to --
35+
minting an unbound one and rotating it after the ceremony would be textbook
36+
session fixation for the window in between. Those two endpoints stay
37+
cookie-borne; see docs/rest_api.md.
38+
"""
39+
40+
from importlib import import_module
41+
42+
from django.conf import settings as django_settings
43+
44+
#: The request header a client presents its token in. Deliberately not
45+
#: ``Authorization``: that is where the client's *own* credential already
46+
#: lives (a DRF token, a JWT), and this is a second, orthogonal one -- an
47+
#: answer to "has this caller passed MFA", not "who is this caller".
48+
HEADER = "X-MFA-Session"
49+
META_KEY = "HTTP_X_MFA_SESSION"
50+
51+
#: Where issue() records the user a token belongs to, inside the session it
52+
#: mints. See load() for why a token that lacks this is refused rather than
53+
#: trusted.
54+
USER_KEY = "_mfa_api_user"
55+
56+
57+
class TokenSessionsUnsupported(Exception):
58+
"""``SESSION_ENGINE`` cannot mint a server-side session key.
59+
60+
True of ``django.contrib.sessions.backends.signed_cookies``, where the
61+
"key" is the signed payload itself: it changes every time the session data
62+
changes, so a token issued before a challenge would no longer name the
63+
session that passed it. There is nothing to hand out, and pretending
64+
otherwise would give a client a token that silently stopped working at the
65+
exact moment it started to matter.
66+
"""
67+
68+
69+
def _store(session_key=None):
70+
return import_module(django_settings.SESSION_ENGINE).SessionStore(
71+
session_key)
72+
73+
74+
def token_from(request):
75+
"""The token this request presented, or None.
76+
77+
Cheap and non-validating on purpose -- it answers "is this a token-borne
78+
request", which is the question the CSRF branch in api/views.py asks
79+
before it has a user to validate against.
80+
"""
81+
return request.META.get(META_KEY) or None
82+
83+
84+
def issue(user):
85+
"""Mint a session for ``user`` and return ``(token, expires_in)``."""
86+
store = _store()
87+
# str(): the default session serializer is JSON, and a UUID primary key
88+
# (or any other non-JSON-native pk a swapped AUTH_USER_MODEL might use)
89+
# would otherwise raise on save. load() compares the same way.
90+
store[USER_KEY] = str(user.pk)
91+
store.create()
92+
if store.session_key is None:
93+
raise TokenSessionsUnsupported(django_settings.SESSION_ENGINE)
94+
return store.session_key, store.get_expiry_age()
95+
96+
97+
def load(request, user):
98+
"""The session this request's token names, or None to refuse it.
99+
100+
Returns None both for "no token presented" and for a token that exists but
101+
does not belong to ``user``. The caller distinguishes them by whether
102+
``token_from()`` was truthy.
103+
104+
**The binding check is the security of this module.** Without it, a client
105+
could present a token issued to somebody else alongside its own identity
106+
credential and inherit that session's verified state -- a complete
107+
second-factor bypass needing only a token overheard once. It is also what
108+
keeps an ordinary browser ``sessionid`` from being replayed through this
109+
header: no browser session carries USER_KEY, so none is accepted here, and
110+
the CSRF exemption that rides on the header therefore cannot be reached
111+
with a stolen cookie.
112+
"""
113+
key = token_from(request)
114+
if key is None:
115+
return None
116+
store = _store(key)
117+
# Reading a nonexistent key yields an empty session rather than raising,
118+
# so an expired, revoked or invented token simply carries no USER_KEY and
119+
# fails the comparison below alongside a genuinely mismatched one.
120+
if store.get(USER_KEY) != str(user.pk):
121+
return None
122+
return store
123+
124+
125+
def persist(request):
126+
"""Save a token-borne session without ever setting a cookie.
127+
128+
SessionMiddleware writes ``Set-Cookie`` on the way out for any session it
129+
finds modified. For a token client that cookie is at best ignored and at
130+
worst confusing -- a browser holding both would send a session cookie the
131+
server never meant it to have. Saving here and clearing ``modified``
132+
leaves the middleware nothing to do, which is the whole trick.
133+
"""
134+
if request.session.modified:
135+
request.session.save()
136+
request.session.modified = False
137+
138+
139+
def revoke(request):
140+
"""Destroy the token-borne session this request presented, if any.
141+
142+
Guarded on a token actually being in play: an unguarded flush() would log
143+
a *cookie* client out of Django entirely, which is not what "revoke my API
144+
token" can be allowed to mean.
145+
"""
146+
if token_from(request) is None:
147+
return False
148+
request.session.flush()
149+
return True

django_mfa/api/urls.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
api_patterns = ([
1818
path("state/", views.state, name="state"),
19+
path("session/", views.mfa_session, name="session"),
1920

2021
path("enroll/<str:factor_type>/begin/",
2122
views.enroll_begin, name="enroll_begin"),

0 commit comments

Comments
 (0)