n.siegfried/Project-SDE-WP-Suite
3
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

T10.2 D13 - login binds against the domain instead of comparing a hash

Browse Source 
server/app.py: login() now calls ldap_auth.verify(). No password is read from
the database on this path any more; the column is still there and unused until
T10.3 drops it, which keeps this commit revertible on its own.

THE THROTTLE IS THE POINT OF THIS COMMIT, not the bind.

AUTH_MAX_ATTEMPTS defaulted to 5, which is exactly this estate's AD lockout
threshold. Failures are now domain binds, and the API runs 2 gunicorn workers,
so that default would have let up to 10 binds reach a DC - locking the account
out of Windows twice over before the app's own lockout engaged. Anyone could
have done it to anyone, unauthenticated, from the login form. Now 2, so the
worst case is 4 against a threshold of 5, with the arithmetic written down at
the constant so the next person to raise it sees the constraint.

The persistent counter on `users` cannot cover this alone: under D13 accounts
are created on first login, so a real domain account can be attacked before it
has a local row. Added a username-keyed pre-account window for exactly that gap
- keyed by username rather than client IP, because it is the domain account
being protected, not this endpoint's capacity, and IP rotation would walk
straight past an IP-keyed limit.

Both throttles are checked BEFORE the directory is touched.

503 is now distinct from 401. LDAP unconfigured, unreachable, untrusted, or a
required group that does not resolve are OUR faults and return 503, because
D13 left no password fallback: reporting "invalid username or password" would
send people hunting for a password they no longer have while the real problem
is a broken deploy. A rejected credential and a missing group membership both
stay a single generic 401 - the useful detail goes to the log, never the
response, so account existence is not disclosed.

Also added a logger to app.py, which had none. An earlier draft of this commit
called log.warning() in a module where `log` was undefined; compileall does not
catch a NameError, so it would have surfaced as a 500 on the first failed
sign-in. Named wpsuite.api to match wpsuite.auth / .ldap / .notify.

Verified: app imports, the route is registered, LOGIN_MAX_ATTEMPTS is 2.
An authenticated user with no local row is refused with 403 until T10.4 adds
provisioning - deliberately explicit rather than silently admitted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cody Schaefer
2026-08-21 15:09:00 -05:00
parent 0577660c86
commit 2a3c4a83fa
1 changed files with 120 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

134
server/app.py
View File

@@ -9,6 +9,7 @@ Run (prod): gunicorn -k uvicorn.workers.UvicornWorker -b 127.0.0.1:8000 serve
Interactive docs: http://<host>/api/docs
"""
import base64
import logging
import os
import re
import uuid
@@ -26,7 +27,11 @@ from sqlalchemy import select, delete, func
from sqlalchemy.orm import Session
from .db import Base, engine, get_db
from . import models, auth, notify, assets_db
from . import models, auth, notify, ldap_auth, assets_db
# Same naming as the other modules' loggers (wpsuite.auth / .ldap / .notify), so a
# deployment can raise the level on one subsystem without raising it on all of them.
log = logging.getLogger("wpsuite.api")
# Schema management:
# • Local dev (SQLite) auto-creates tables for a zero-config run.
@@ -674,38 +679,139 @@ class AutoAddIn(BaseModel):
role: str = ""
LOGIN_MAX_ATTEMPTS = int(os.getenv("AUTH_MAX_ATTEMPTS", "5"))
# D13 / T10.2 — THIS NUMBER IS NOT A UX PREFERENCE, IT IS A SAFETY LIMIT.
#
# Failures are now LDAP binds, so every one counts against the DOMAIN account
# lockout policy. This estate's AD threshold is 5. The throttle below is
# per-process and the API runs 2 gunicorn workers, so a local limit of N lets up
# to 2N binds reach a domain controller: 2 x 2 = 4, one under the threshold.
#
# It defaulted to 5 before this task, which would have allowed up to 10 binds and
# locked the account out of WINDOWS — twice over — before the local lockout ever
# engaged. If you raise this, or add a worker, redo the arithmetic first.
LOGIN_MAX_ATTEMPTS = int(os.getenv("AUTH_MAX_ATTEMPTS", "2"))
LOGIN_LOCKOUT_MINUTES = int(os.getenv("AUTH_LOCKOUT_MINUTES", "15"))
# Pre-account throttle, keyed by USERNAME (not by client IP — an attacker rotating
# IPs would sail past an IP-keyed limit, and it is the domain account we are
# protecting, not this endpoint's capacity).
#
# The DB counter on `users` is shared across workers and persists, but it can only
# work for an account that already has a local row. Under D13 accounts are created
# on first successful login, so a REAL domain account can be hammered before any
# row exists — this dict covers exactly that window. Per-worker and lost on
# restart, which is why LOGIN_MAX_ATTEMPTS is halved rather than trusted.
_bind_attempts: dict[str, list[float]] = {}
_BIND_WINDOW_SECONDS = LOGIN_LOCKOUT_MINUTES * 60
def _bind_throttled(sam: str) -> bool:
"""True if this username has already used its bind budget in the window.
Checked BEFORE any call to the directory."""
now = monotonic()
key = (sam or "").strip().lower()
hits = [t for t in _bind_attempts.get(key, []) if now - t < _BIND_WINDOW_SECONDS]
_bind_attempts[key] = hits
if len(_bind_attempts) > 5000: # bound the dict on a long-lived worker
for k in [k for k, v in _bind_attempts.items()
if not v or now - max(v) > _BIND_WINDOW_SECONDS]:
_bind_attempts.pop(k, None)
return len(hits) >= LOGIN_MAX_ATTEMPTS
def _record_bind_failure(sam: str) -> None:
_bind_attempts.setdefault((sam or "").strip().lower(), []).append(monotonic())
def _clear_bind_failures(sam: str) -> None:
_bind_attempts.pop((sam or "").strip().lower(), None)
@app.post("/api/auth/login")
def login(body: LoginIn, request: Request, response: Response, db: Session = Depends(get_db)):
"""Verify credentials and, on success, set the HttpOnly session cookie.
Throttles online password guessing: after LOGIN_MAX_ATTEMPTS consecutive
failures an account is locked for LOGIN_LOCKOUT_MINUTES."""
user = auth.find_user(db, body.username)
"""Authenticate against the domain (D13 / T10.2) and set the session cookie.
The suite stores no passwords: this is an LDAPS simple bind as
`<sAMAccountName>@prime.local`, and a successful bind IS the authentication.
See server/ldap_auth.py for the transport and its two hard guards.
ORDER MATTERS HERE. Every throttle check runs BEFORE the directory is touched,
because a failed bind counts against the DOMAIN lockout policy — so this
endpoint must not be usable to lock a colleague out of Windows. Nothing below
reaches `ldap_auth.verify` until the local budget has been checked twice: once
against the persistent per-account counter, once against the pre-account
window that covers usernames with no local row yet.
Three outcomes, deliberately distinguished:
401 the credential was rejected, or the account is not in the required
group. One generic message for every case — the response must never
reveal whether an account exists (see `_ERR49` in ldap_auth: the useful
detail goes to the log).
403 the local account exists and is disabled. Independent of the directory.
503 OUR fault — LDAP unconfigured, unreachable, untrusted, or the required
group does not resolve. D13 left no password fallback, so this must not
masquerade as 401: "your password is wrong" sends people hunting for a
password they no longer have, while the real problem is a broken deploy.
"""
now = models.utcnow()
# Always run the hash comparison first — even for missing or locked accounts —
# so response timing doesn't leak which usernames exist. verify_password
# tolerates an empty hash.
valid = auth.verify_password(body.password, user.password_hash if user else "")
sam = ldap_auth.normalize_username(body.username)
if not sam or not (body.password or "").strip():
# No directory call for empty input. ldap_auth.verify guards this too; the
# duplication is intentional, since an empty password would otherwise be an
# anonymous bind and anonymous binds SUCCEED.
raise HTTPException(status_code=401, detail="Invalid username or password")
user = auth.find_user(db, sam)
# ── throttle 1: the persistent per-account lockout ────────────────────────
locked = user.locked_until if user else None
if locked is not None and locked.tzinfo is None:
locked = locked.replace(tzinfo=timezone.utc) # SQLite returns naive datetimes; normalize to UTC
locked = locked.replace(tzinfo=timezone.utc) # SQLite returns naive datetimes
if locked is not None and locked > now:
raise HTTPException(status_code=429, detail="Too many failed attempts. Try again later.")
if not user or not valid:
# ── throttle 2: the pre-account window ────────────────────────────────────
if _bind_throttled(sam):
log.warning("refusing to bind for %r — local attempt budget (%d) spent; "
"protecting the domain account from lockout", sam, LOGIN_MAX_ATTEMPTS)
raise HTTPException(status_code=429, detail="Too many failed attempts. Try again later.")
# ── the directory ─────────────────────────────────────────────────────────
settings = notify.get_settings(db)
result = ldap_auth.verify(sam, body.password,
required_group=settings.get("ldap_required_group"))
if result.is_config_problem:
# Ours, not theirs. Never counted against the user, never a 401.
log.error("sign-in unavailable — %s: %s", result.reason, result.detail)
raise HTTPException(status_code=503,
detail="Sign-in is temporarily unavailable. Contact IT.")
if not result.ok:
log.info("sign-in refused for %r (%s: %s)", sam, result.reason, result.detail)
_record_bind_failure(sam)
if user:
user.failed_attempts = (user.failed_attempts or 0) + 1
if user.failed_attempts >= LOGIN_MAX_ATTEMPTS:
user.locked_until = now + timedelta(minutes=LOGIN_LOCKOUT_MINUTES)
user.failed_attempts = 0
log_event(db, user.username, "login_locked", "user", user.id, summary=user.username,
detail={"minutes": LOGIN_LOCKOUT_MINUTES})
log_event(db, user.username, "login_locked", "user", user.id,
summary=user.username,
detail={"minutes": LOGIN_LOCKOUT_MINUTES, "reason": result.reason})
db.commit()
raise HTTPException(status_code=401, detail="Invalid username or password")
# ── authenticated ─────────────────────────────────────────────────────────
_clear_bind_failures(sam)
if not user:
# T10.4 provisions the account here. Until that lands, an authenticated
# person with no local row is refused rather than silently admitted.
log.warning("%r authenticated against the domain but has no local account "
"(JIT provisioning arrives in T10.4)", result.sam)
raise HTTPException(status_code=403, detail="No account on this system yet.")
if not user.is_active:
raise HTTPException(status_code=403, detail="Account is disabled")
user.failed_attempts = 0
user.locked_until = None
user.last_login_at = now