diff --git a/app.py b/app.py index 54bf38f..f1e874e 100644 --- a/app.py +++ b/app.py @@ -111,6 +111,7 @@ def password_complexity(cls, v: str): from ai.utils import load_ai_config, is_critical_log, save_ai_results from security import session as session_store from security import ssrf +from security import totp as totp_store from core import agent_link from core import attack from core import config_validation @@ -886,6 +887,14 @@ async def register_agent(request): if not row: return sanic_json({"status": "error", "message": "Token not recognized"}, status=404) if row["used_at"] is not None: + # A one-time credential presented a second time. The + # ordinary cause is an installer re-run, and the one that + # matters is a token that leaked - they are the same + # request, so this is recorded rather than judged. + await _platform_event( + "ENROLMENT_TOKEN_REUSED", subject=token[:8] + "...", + source_ip=_client_ip(request), + detail=f"first used at {row['used_at']}") return sanic_json({"status": "error", "message": "Token already used"}, status=409) if row["expires_at"] and row["expires_at"] < datetime.now(): return sanic_json({"status": "error", "message": "Token expired"}, status=410) @@ -1236,6 +1245,8 @@ async def audit_log(request, action: str, resource: str, details: str = ""): # before the agent polled them, so they never ran and the UI showed them # green. They authenticate now — see _require_agent. "download_agent", "register_agent", "agent_bootstrap", + # See the path list above: the pending token is the credential. + "complete_second_factor", # The CA certificate, and only the certificate. It is the half of the pair # whose purpose is to be handed out, an installer needs it before it holds # any credential, and publishing it grants nothing - it lets a client check @@ -1277,6 +1288,11 @@ async def audit_log(request, action: str, resource: str, details: str = ""): # here only because the operator session middleware would otherwise reject # the handshake before the handler is ever reached. "/agent-link", + # The second half of a login. There is no session yet by definition - the + # credential is the pending token, minted moments earlier by a correct + # password, single-use, five-minute lifetime, and able to do exactly one + # thing. Gating it on a session would make the second factor unreachable. + "/login/2fa", # The CA *certificate*, so an agent can verify a self-signed server. # Public on purpose: a certificate is the half of the pair meant to be # handed out, and an installer needs it before it has any credential. @@ -1328,6 +1344,17 @@ async def wrapper(request, *args, **kwargs): # The middleware already resolved this; skip the duplicate query. if not getattr(request.ctx, "permission_checked", False): if not await user_has_permission(current_user_id(request), permission_name): + # One is a mis-click. A run of them from one session is + # somebody mapping what they can touch, and that only + # looks like anything once the attempts are counted + # together - which is what the fold in `self_defence` + # does. + await _platform_event( + "PERMISSION_DENIED", + subject=str(current_user_id(request) or "unknown"), + source_ip=_client_ip(request), + detail=f"reached for {permission_name!r} on " + f"{request.path}") return sanic_json({ "status": "error", "message": f"Permission denied: '{permission_name}' required" @@ -5211,6 +5238,32 @@ async def login(request): # database this never happened. With a pool that failed to # build at startup it happened every time, and told the person # with the right password that it was wrong. + # The second factor, if this account has a confirmed one. + # + # Before the session, not after it. Issuing one and + # "upgrading" it once a code arrives leaves a window in which + # a single factor is a logged-in operator, and every bug in + # that window is a bypass. + async with userdb_conn() as f_cnx: + f_cur = await f_cnx.cursor() + try: + needs_second = await _totp_required(f_cur, user_id) + finally: + await f_cur.close() + if needs_second: + pending = await _begin_second_factor( + request, user_id=user_id, username=db_username, + auth_type="local") + await log_login_attempt(username, "local", "pending", + "password accepted, second factor " + "required", ip) + return response.json({ + "status": "second_factor_required", + "token": pending, + "expires_in": totp_store.PENDING_TTL_SECONDS, + "message": "Enter the code from your authenticator.", + }) + try: user_perms = await get_user_permissions(user_id) raw_token = await _issue_session( @@ -5348,9 +5401,34 @@ async def login(request): print(f"[!] LDAP role lookup failed for {username!r}: {e}", flush=True) - await log_login_attempt(username, "ldap", "success", "", ip) user_perms = await get_role_permissions(app_role) ldap_uid = await _upsert_ldap_user(username, app_role) + + # The same gate as the local branch, and it has to be here + # too. A second factor that protects one login path and not + # the other protects nothing: an attacker with a password + # picks the path without it. + async with userdb_conn() as f_cnx: + f_cur = await f_cnx.cursor() + try: + needs_second = await _totp_required(f_cur, ldap_uid) + finally: + await f_cur.close() + if needs_second: + pending = await _begin_second_factor( + request, user_id=ldap_uid, username=username, + auth_type="ldap") + await log_login_attempt(username, "ldap", "pending", + "password accepted, second factor " + "required", ip) + return response.json({ + "status": "second_factor_required", + "token": pending, + "expires_in": totp_store.PENDING_TTL_SECONDS, + "message": "Enter the code from your authenticator.", + }) + + await log_login_attempt(username, "ldap", "success", "", ip) raw_token = await _issue_session( request, user_id=ldap_uid, username=username, role=app_role, auth_type="ldap", @@ -7407,6 +7485,436 @@ async def get_telemetry_health(request, agent): }) +# --------------------------------------------------------------------------- +# Second factor +# --------------------------------------------------------------------------- + +async def _totp_tables(cur) -> None: + """Create the three tables on first use. + + Alongside the rest of this file's guarded DDL rather than in + init_userdb.sql, so a deployment that predates two-factor gets them + without a migration step somebody has to remember to run. + """ + for statement in (totp_store.DDL_SECRETS, totp_store.DDL_RECOVERY, + totp_store.DDL_PENDING): + await cur.execute(statement) + + +async def _totp_secret_for(cur, user_id: int) -> tuple[str | None, bool]: + """(secret, confirmed) for a user, decrypted, or (None, False). + + An unreadable secret is reported as absent rather than raised. The one + situation that produces it is a Fernet key that has changed, and the + honest consequence is that the user has to enrol again - not that they + cannot log in at all. + """ + await _totp_tables(cur) + await cur.execute( + "SELECT secret, confirmed_at FROM user_totp WHERE user_id = %s", + (user_id,)) + row = await cur.fetchone() + if not row: + return None, False + try: + secret = fernet.decrypt(row[0].encode()).decode() + except Exception: + print(f"[2fa] the stored secret for user {user_id} cannot be " + f"decrypted; they will have to enrol again", flush=True) + return None, False + return secret, row[1] is not None + + +async def _totp_required(cur, user_id: int) -> bool: + """Whether this user has a *confirmed* second factor. + + Confirmed, not merely enrolled. A secret written when the QR code was + displayed and never proved would lock out everybody who scanned it into an + app they then deleted. + """ + _, confirmed = await _totp_secret_for(cur, user_id) + return confirmed + + +async def _begin_second_factor(request, *, user_id: int, username: str, + auth_type: str) -> str: + """Park a half-authenticated login and return its one-use token. + + Deliberately not a session. Issuing a real one and upgrading it later + leaves a window in which one factor is a logged-in operator, and every + bug in that window is a bypass. + """ + raw = totp_store.new_pending_token() + async with userdb_conn() as cnx: + cur = await cnx.cursor() + try: + await _totp_tables(cur) + await cur.execute( + "INSERT INTO totp_pending (token_hash, user_id, username, " + "auth_type, ip, expires_at) VALUES (%s, %s, %s, %s, %s, " + "DATE_ADD(NOW(), INTERVAL %s SECOND))", + (totp_store.hash_pending(raw), user_id, username, auth_type, + _client_ip(request), totp_store.PENDING_TTL_SECONDS)) + # Opportunistic, and cheap: without it this table only grows, and + # an abandoned login is exactly the row nobody comes back for. + await cur.execute("DELETE FROM totp_pending WHERE expires_at < NOW()") + await cnx.commit() + finally: + await cur.close() + return raw + + +@app.post("/login/2fa") +async def complete_second_factor(request): + """Finish a login that passed its password. + + Public in the session sense and not in any other: the pending token is a + credential minted moments ago by a correct password, and it can do exactly + one thing. + """ + data = request.json or {} + raw_token = (data.get("token") or "").strip() + code = (data.get("code") or "").strip() + ip = _client_ip(request) + + if not raw_token or not code: + return response.json({"status": "error", + "message": "Both the login token and a code are " + "required."}, status=400) + + async with userdb_conn() as cnx: + cur = await cnx.cursor() + try: + await _totp_tables(cur) + await cur.execute( + "SELECT user_id, username, auth_type, attempts FROM " + "totp_pending WHERE token_hash = %s AND expires_at > NOW()", + (totp_store.hash_pending(raw_token),)) + pending = await cur.fetchone() + if not pending: + # Expired, already spent, or never existed - one message for + # all three, because telling them apart tells an attacker + # which half of a captured pair is still good. + return response.json({ + "status": "error", + "message": "This login has expired. Sign in again.", + }, status=401) + + user_id, username, auth_type, attempts = pending + if attempts >= totp_store.MAX_PENDING_ATTEMPTS: + await cur.execute("DELETE FROM totp_pending WHERE token_hash = %s", + (totp_store.hash_pending(raw_token),)) + await cnx.commit() + await log_login_attempt(username, auth_type, "failure", + "too many second-factor attempts", ip) + return response.json({ + "status": "error", + "message": "Too many attempts. Sign in again.", + }, status=429) + + secret, confirmed = await _totp_secret_for(cur, user_id) + if not secret or not confirmed: + return response.json({ + "status": "error", + "message": "This account has no second factor configured.", + }, status=409) + + step = totp_store.verify(secret, code) + used_recovery = False + if step is None: + # A recovery code, maybe. Checked second so a normal login + # never touches the recovery table. + await cur.execute( + "UPDATE user_recovery_codes SET used_at = NOW() " + " WHERE user_id = %s AND code_hash = %s AND used_at IS NULL", + (user_id, totp_store.hash_recovery(code))) + used_recovery = cur.rowcount == 1 + if not used_recovery: + await cur.execute( + "UPDATE totp_pending SET attempts = attempts + 1 " + " WHERE token_hash = %s", + (totp_store.hash_pending(raw_token),)) + await cnx.commit() + await log_login_attempt(username, auth_type, "failure", + "second factor rejected", ip) + return response.json({ + "status": "error", + "message": "That code is not valid.", + }, status=401) + else: + # Single use, and this is the line that makes it so. + # + # TOTP is a shared secret and a clock: the same six digits are + # valid for the whole step, so without this anyone who watches + # a code being typed has thirty seconds to use it themselves. + # The comparison is `>=` rather than `=` so a code from + # *earlier* in the accepted window cannot be replayed either. + await cur.execute( + "UPDATE user_totp SET last_step = %s " + " WHERE user_id = %s AND (last_step IS NULL OR last_step < %s)", + (step, user_id, step)) + if cur.rowcount == 0: + await cur.execute( + "UPDATE totp_pending SET attempts = attempts + 1 " + " WHERE token_hash = %s", + (totp_store.hash_pending(raw_token),)) + await cnx.commit() + await log_login_attempt(username, auth_type, "failure", + "second-factor code reused", ip) + await _platform_event( + "TOTP_CODE_REPLAYED", subject=username, source_ip=ip, + detail="a code already used for this account was " + "presented again") + return response.json({ + "status": "error", + "message": "That code has already been used. Wait for " + "the next one.", + }, status=401) + + # Spent, whichever factor it was. + await cur.execute("DELETE FROM totp_pending WHERE token_hash = %s", + (totp_store.hash_pending(raw_token),)) + await cur.execute( + "SELECT role, must_change_password, created_at FROM users " + " WHERE id = %s", (user_id,)) + role_row = await cur.fetchone() + await cnx.commit() + finally: + await cur.close() + + role = role_row[0] if role_row else None + must_change = bool(role_row[1]) if role_row else False + + try: + user_perms = await get_user_permissions(user_id) + raw_session = await _issue_session( + request, user_id=user_id, username=username, role=role, + auth_type=auth_type) + except Exception as e: + print(f"[!] {username} passed both factors but the session could not " + f"be created: {e}", flush=True) + return response.json({ + "status": "error", + "message": "Both factors were accepted, but this server could not " + "start a session. This is a server fault, not a " + "credential one.", + }, status=503) + + await log_login_attempt( + username, auth_type, "success", + "recovery code" if used_recovery else "second factor", ip) + if used_recovery: + # Worth its own line and its own event: a recovery code means the + # normal factor was unavailable, which is either an honest lost phone + # or somebody who has the password and a stolen list. + await _platform_event("RECOVERY_CODE_USED", subject=username, + source_ip=ip, + detail="signed in with a recovery code rather " + "than an authenticator") + + resp = response.json({ + "status": "success", + "message": "Login successful.", + "user": { + "id": user_id, "username": username, "auth_type": auth_type, + "role": role, "permissions": user_perms, + "must_change_password": must_change, + "used_recovery_code": used_recovery, + }, + }) + _set_session_cookie(resp, raw_session) + return resp + + +@app.get("/api/2fa/status") +async def second_factor_status(request): + """Whether the signed-in operator has a second factor, and how many + recovery codes are left.""" + user_id = current_user_id(request) + if not user_id: + return sanic_json({"status": "error", "message": "Unauthorized"}, + status=401) + async with userdb_conn() as cnx: + cur = await cnx.cursor() + try: + _, confirmed = await _totp_secret_for(cur, user_id) + await cur.execute( + "SELECT COUNT(*) FROM user_recovery_codes " + " WHERE user_id = %s AND used_at IS NULL", (user_id,)) + remaining = (await cur.fetchone())[0] + finally: + await cur.close() + return sanic_json({"status": "success", "enabled": confirmed, + "recovery_codes_remaining": int(remaining or 0)}) + + +@app.post("/api/2fa/enrol") +async def second_factor_enrol(request): + """Start enrolment: a new secret and the URI to scan. + + Nothing is enforced by this. The secret is written unconfirmed, and only + `/api/2fa/confirm` - which requires a working code from it - turns it on. + Enrolling in one step would lock out anybody whose app did not keep it. + """ + user_id = current_user_id(request) + if not user_id: + return sanic_json({"status": "error", "message": "Unauthorized"}, + status=401) + + secret = totp_store.new_secret() + async with userdb_conn() as cnx: + cur = await cnx.cursor() + try: + await _totp_tables(cur) + await cur.execute("SELECT username FROM users WHERE id = %s", + (user_id,)) + row = await cur.fetchone() + username = row[0] if row else str(user_id) + # Replaces any unconfirmed attempt. Re-scanning a QR code that was + # abandoned halfway is the ordinary case, not an error. + await cur.execute( + "INSERT INTO user_totp (user_id, secret, confirmed_at) " + "VALUES (%s, %s, NULL) ON DUPLICATE KEY UPDATE " + "secret = IF(confirmed_at IS NULL, VALUES(secret), secret)", + (user_id, fernet.encrypt(secret.encode()).decode())) + await cur.execute( + "SELECT confirmed_at FROM user_totp WHERE user_id = %s", + (user_id,)) + already = (await cur.fetchone())[0] is not None + await cnx.commit() + finally: + await cur.close() + + if already: + # Re-enrolling with one confirmed already would silently invalidate + # the factor they are using. Disabling is a separate, deliberate act. + return sanic_json({ + "status": "error", + "message": "Two-factor is already enabled for this account. " + "Disable it first if you want to enrol a new device.", + }, status=409) + + return sanic_json({ + "status": "success", + "secret": secret, + "uri": totp_store.provisioning_uri(secret, account=username, + issuer="Sentora"), + "digits": totp_store.DIGITS, + "period": totp_store.STEP_SECONDS, + }) + + +@app.post("/api/2fa/confirm") +async def second_factor_confirm(request): + """Prove a code from the new secret, and receive the recovery codes. + + The recovery codes are returned here and nowhere else, ever. They are + stored as hashes, so the server genuinely cannot show them again - which + is the property that makes them safe to store at all. + """ + user_id = current_user_id(request) + if not user_id: + return sanic_json({"status": "error", "message": "Unauthorized"}, + status=401) + code = ((request.json or {}).get("code") or "").strip() + + async with userdb_conn() as cnx: + cur = await cnx.cursor() + try: + secret, confirmed = await _totp_secret_for(cur, user_id) + if not secret: + return sanic_json({"status": "error", + "message": "Start enrolment first."}, + status=409) + if confirmed: + return sanic_json({"status": "error", + "message": "Two-factor is already enabled."}, + status=409) + step = totp_store.verify(secret, code) + if step is None: + return sanic_json({ + "status": "error", + "message": "That code is not valid. Check the time on the " + "device running your authenticator.", + }, status=400) + + codes = totp_store.new_recovery_codes() + await cur.execute( + "UPDATE user_totp SET confirmed_at = NOW(), last_step = %s " + " WHERE user_id = %s", (step, user_id)) + await cur.execute("DELETE FROM user_recovery_codes WHERE user_id = %s", + (user_id,)) + for one in codes: + await cur.execute( + "INSERT INTO user_recovery_codes (user_id, code_hash) " + "VALUES (%s, %s)", (user_id, totp_store.hash_recovery(one))) + await cnx.commit() + finally: + await cur.close() + + await audit_log(request, "2FA_ENABLED", str(user_id), + "second factor confirmed and recovery codes issued") + return sanic_json({ + "status": "success", + "recovery_codes": codes, + "message": "Two-factor is on. Save these recovery codes now - they " + "are stored as hashes and cannot be shown again.", + }) + + +@app.post("/api/2fa/disable") +async def second_factor_disable(request): + """Turn it off, with the current password. + + The password is required because a hijacked session must not be able to + remove the control that would have stopped it. That is the whole reason + this endpoint is not simply "delete the row". + """ + user_id = current_user_id(request) + if not user_id: + return sanic_json({"status": "error", "message": "Unauthorized"}, + status=401) + password = ((request.json or {}).get("password") or "") + if not password: + return sanic_json({"status": "error", + "message": "Your password is required to turn off " + "two-factor."}, status=400) + + async with userdb_conn() as cnx: + cur = await cnx.cursor() + try: + await cur.execute("SELECT password FROM users WHERE id = %s", + (user_id,)) + row = await cur.fetchone() + ok = False + if row: + try: + ok = bcrypt.checkpw(password.encode("utf-8"), + row[0].encode("utf-8")) + except ValueError: + ok = False # an LDAP placeholder hash + if not ok: + await _platform_event( + "TOTP_DISABLE_REFUSED", subject=str(user_id), + source_ip=_client_ip(request), + detail="wrong password given when turning off two-factor") + return sanic_json({"status": "error", + "message": "That password is not correct."}, + status=403) + await cur.execute("DELETE FROM user_totp WHERE user_id = %s", + (user_id,)) + await cur.execute("DELETE FROM user_recovery_codes WHERE user_id = %s", + (user_id,)) + await cnx.commit() + finally: + await cur.close() + + await audit_log(request, "2FA_DISABLED", str(user_id), + "second factor removed by the account holder") + return sanic_json({"status": "success", + "message": "Two-factor is off for this account."}) + + @app.route("/api/platform/events") @require_permission("manage_system") async def get_platform_events(request): diff --git a/core/self_defence.py b/core/self_defence.py index 0dc9da2..6c5b7bc 100644 --- a/core/self_defence.py +++ b/core/self_defence.py @@ -69,12 +69,42 @@ "HIGH", "An enrolment token was presented after it had already been used. The " "token is a one-time credential, so a second use means it leaked."), - "TLS_DOWNGRADE_REFUSED": ( + "TOTP_CODE_REPLAYED": ( + "HIGH", + "A second-factor code that had already been used was presented again. " + "The code is valid for its whole thirty-second step, so this is what " + "somebody watching one being typed would try."), + "RECOVERY_CODE_USED": ( "MEDIUM", - "An agent tried the plaintext ingest port after this deployment " - "required TLS."), + "Somebody signed in with a recovery code instead of an authenticator. " + "Usually an honest lost phone; the other reading is a stolen password " + "and a stolen list."), + "TOTP_DISABLE_REFUSED": ( + "HIGH", + "A wrong password was given when turning off two-factor. A hijacked " + "session trying to remove the control that would have stopped it " + "looks exactly like this."), } +# `TLS_DOWNGRADE_REFUSED` was defined here and is deliberately gone. +# +# It described an agent trying the plaintext ingest port after the deployment +# required TLS - and `INGEST_TLS_REQUIRED=1` does not open that listener at +# all, so the server never sees the connection. The kind could not be raised +# by design. +# +# Making it raisable would mean binding the port and refusing politely, and +# that is worse for exactly the agents it would report. A build older than the +# receipt frame marks its rows sent once `sendall` returns; a clean close +# after we read its name would let it do that and lose the batch. ECONNREFUSED +# raises inside `sendall`, so the rows stay put. +# +# The visibility is not lost, it is on the other side: a refused send becomes +# `record_send_failure` on the agent, and the telemetry health view reports +# the table as `send failing` with the connection error and counts it as +# broken. That is the same fact, reported by the half of the system that can +# report it without risking anybody's data. + #: Severity order, so a caller can compare without hardcoding a list. SEVERITY_RANK = {"CRITICAL": 0, "HIGH": 1, "MEDIUM": 2, "LOW": 3, "INFO": 4} diff --git a/docs/production-deployment.md b/docs/production-deployment.md index 0d5647d..6d79293 100644 --- a/docs/production-deployment.md +++ b/docs/production-deployment.md @@ -804,10 +804,102 @@ Every route is deny-by-default. The exceptions are the login endpoint, the SPA shell, static assets and the agent-facing endpoints, which authenticate with `X-Agent-Key` or an enrolment token. -### 6.2 Hardening checklist +### 6.2 Two-factor authentication + +Off per account until an operator turns it on, at **My Security** in the +sidebar. There is no permission gate on that page and there deliberately is +not one: two-factor protects the account, not the tenancy, so an operator +whose role can do nothing else still has to be able to secure their own login. + +An account that has it on cannot be signed into with a password alone by +anybody, including an administrator. There is no override. + +#### What it protects against + +Nothing about the password changes. Guessing was already covered by +`core.login_guard`, which locks out per user and per address. What a second +factor adds is the case a lockout cannot touch: a password that is *correct* +and in the wrong hands - reused from a breached site, phished, or read out of +a browser. This console can isolate a host, run a command as SYSTEM on every +endpoint, and read every log the fleet has produced, and one string is enough +for all of it. + +#### Enrolling + +1. **My Security** -> *Turn on two-factor*. +2. Scan the QR with any authenticator, or type the key by hand - a QR is no + use on the machine you are already sitting at. +3. Enter a code to confirm. **Nothing changes about the login until this + step**: a secret written when the QR appeared, with no proof it was kept, + would lock out everybody who scanned it into an app they then deleted. +4. Save the recovery codes. They are shown exactly once. + +#### Recovery codes + +Ten, each good for one sign-in, stored as SHA-256 hashes. The server cannot +show them again - that is the property that makes storing them safe, and it is +also the thing to tell operators before they close the tab. + +**If they are lost and the authenticator is gone, the account cannot sign in.** +There is no support path that recovers it, by design: any mechanism that let +an administrator bypass somebody's second factor would be a bypass an attacker +could use too. The way back is a `users` row edit against the database, which +is deliberately awkward: + +```sql +-- Only from a shell on the database host, and audit-log the fact you did it. +DELETE FROM userdb.user_totp WHERE user_id = ; +DELETE FROM userdb.user_recovery_codes WHERE user_id = ; +``` + +Signing in with a recovery code raises `RECOVERY_CODE_USED` on the platform +events panel (see §6.4). It is usually an honest lost phone; the other reading +is a stolen password and a stolen list, and the two look identical from here. + +#### Settings + +There are none. The step is 30 seconds, codes are 6 digits, and the accepted +drift is one step either side - the interoperable values, and not worth being +unusual about. One step is 30 seconds of clock skew in each direction, which +covers a phone that has not synchronised recently; widening it would hand an +attacker more time to reuse a code they watched being typed. + +A code works once. TOTP is a shared secret and a clock, so the same six digits +stay valid for their whole step, and without single use anyone who sees one +entered has the rest of that step to use it themselves. A second attempt +raises `TOTP_CODE_REPLAYED`. + +#### What is stored + +| | | +| :--- | :--- | +| `user_totp.secret` | The seed, **Fernet-encrypted** with the server key. A dump of `userdb` alone does not hand over everybody's second factor. | +| `user_recovery_codes.code_hash` | SHA-256. Never the code. | +| `totp_pending.token_hash` | SHA-256 of the half-authenticated token, like a session. | + +The seed column is `VARCHAR(512)` for ciphertext, not for the ~32 characters a +base32 seed reads as - see §4.1 for what sizing a column to the human-readable +value cost this platform once already. + +**Losing the Fernet key takes every enrolled second factor with it.** The +secret becomes undecryptable, the server reports the account as having none, +and each operator has to enrol again. That is the survivable failure; the +alternative - refusing the login outright - would lock the whole console out +of itself. Back the key up (§4.1). + +#### LDAP accounts + +Gated identically. A second factor that protected the local path and not the +directory path would protect nothing: an attacker holding a password picks the +path without it. + +### 6.3 Hardening checklist 1. Change `admin / admin123` immediately under **Users & Roles**. 2. Create per-operator accounts; stop using the shared admin. + Have each one turn on two-factor at **My Security** (§6.2) - especially the + accounts that can reach SOAR, because those can run a command as SYSTEM on + every endpoint. 3. Map operators to roles: - `auditor` — `read_telemetry` only - `operator` — read + `manage_soar` (approve shadow, dispatch SOAR) @@ -823,7 +915,38 @@ shell, static assets and the agent-facing endpoints, which authenticate with call. It is a server-side request forgery primitive by nature; empty means the endpoint is inert. -### 6.3 Audit retention +### 6.4 Attacks on the platform itself + +`userdb.platform_events`, surfaced under **Activity Logs**. The server already +detected all of this and told nobody: a lockout went to a container log, a +rejected agent key went to a container log, a permission denial went to a +table nobody opens unless they are already investigating. The platform watched +every host in the fleet and was the one machine nobody watched. + +| Kind | Severity | What it means | +| :--- | :--- | :--- | +| `FLEET_KEY_ON_CHANNEL` | CRITICAL | The fleet-wide secret was offered on the agent channel. That channel carries `/self_destruct`, so a leaked master key must not open one against every endpoint at once. | +| `LOGIN_LOCKOUT` | HIGH | Failed logins crossed the threshold. Somebody guessing, or an integration on a credential that changed. | +| `AGENT_KEY_REJECTED` | HIGH | A channel was opened with a key this server does not know. Any revoked agent does this; so does anyone who found a key that no longer works. | +| `ENROLMENT_TOKEN_REUSED` | HIGH | A one-time token presented twice. Usually an installer re-run; the other reading is that it leaked. | +| `TOTP_CODE_REPLAYED` | HIGH | A second-factor code already used was presented again. | +| `TOTP_DISABLE_REFUSED` | HIGH | A wrong password when turning off two-factor. A hijacked session trying to remove the control that would have stopped it looks exactly like this. | +| `PERMISSION_DENIED` | MEDIUM | An operator reached past their role. One is a mis-click; a run of them is somebody mapping what they can touch. | +| `RECOVERY_CODE_USED` | MEDIUM | Signed in with a recovery code rather than an authenticator. | + +Repeats fold into one row whose count climbs, within a ten-minute window. A +brute-force that wrote a row per attempt would make the view meant to reveal +it into the thing that buries it. + +The panel is shown even when empty, and says so. An operator who never sees it +cannot tell "nothing has attacked us" from "this platform does not watch +itself" - and on a security view, a quiet twenty-four hours is a finding. + +Nothing here records a credential. These rows describe attempts *on* secrets, +and the obvious fields to include - the password tried, the key presented - +are exactly the ones that turn an audit trail into a second breach. + +### 6.5 Audit retention `login_logs` and `audit_logs` record every UI login attempt with source IP and result, every SOAR dispatch, every shadow approve/reject with operator and diff --git a/frontend/package-lock.json b/frontend/package-lock.json index 1937a29..6f64310 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -16,6 +16,7 @@ "axios": "^1.13.6", "fuse.js": "^7.1.0", "lucide-react": "^0.577.0", + "qrcode.react": "^4.2.0", "react": "^18.3.1", "react-dom": "^18.3.1", "react-router-dom": "^7.13.1", @@ -2697,6 +2698,15 @@ "node": ">=6" } }, + "node_modules/qrcode.react": { + "version": "4.2.0", + "resolved": "https://registry.npmjs.org/qrcode.react/-/qrcode.react-4.2.0.tgz", + "integrity": "sha512-QpgqWi8rD9DsS9EP3z7BT+5lY5SFhsqGjpgW5DY/i3mK4M9DTBNz3ErMi8BWYEfI3L0d8GIbGmcdFAS1uIRGjA==", + "license": "ISC", + "peerDependencies": { + "react": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0" + } + }, "node_modules/react": { "version": "18.3.1", "resolved": "https://registry.npmjs.org/react/-/react-18.3.1.tgz", diff --git a/frontend/package.json b/frontend/package.json index 3fa9d6e..3e2df90 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -19,6 +19,7 @@ "axios": "^1.13.6", "fuse.js": "^7.1.0", "lucide-react": "^0.577.0", + "qrcode.react": "^4.2.0", "react": "^18.3.1", "react-dom": "^18.3.1", "react-router-dom": "^7.13.1", diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 66eeaa7..96e620c 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -15,6 +15,7 @@ import Playbooks from './pages/Playbooks'; import Databases from './pages/Databases'; import LoginLogs from './pages/LoginLogs'; import AuditLogs from './pages/AuditLogs'; +import AccountSecurity from './pages/AccountSecurity'; import Automations from './pages/Automations'; import GlobalAlerts from './pages/GlobalAlerts'; import Agents from './pages/Agents'; @@ -148,6 +149,15 @@ const App: React.FC = () => { } /> + + {/* No permission gate. Two-factor protects the account rather than + the tenancy, so an operator whose role can do nothing else still + has to be able to secure their own login. */} + + + + } /> diff --git a/frontend/src/components/Sidebar.tsx b/frontend/src/components/Sidebar.tsx index 82be319..433e818 100644 --- a/frontend/src/components/Sidebar.tsx +++ b/frontend/src/components/Sidebar.tsx @@ -1,6 +1,6 @@ import React, { useState } from 'react'; import { NavLink, Link } from 'react-router-dom'; -import { Activity, BrainCircuit, ChevronRight, ClipboardList, Database, Download, Grid3x3, Key, LayoutDashboard, LogOut, Monitor, PlaySquare, Radar, Save, Search, Settings, ShieldAlert, Users, X, Zap } from 'lucide-react'; +import { Activity, BrainCircuit, ChevronRight, ClipboardList, Database, Download, Grid3x3, Key, LayoutDashboard, LogOut, Monitor, PlaySquare, Radar, Save, Search, Settings, ShieldAlert, ShieldCheck, Users, X, Zap } from 'lucide-react'; import { authService } from '../services/api'; interface SidebarProps { @@ -157,6 +157,9 @@ const Sidebar: React.FC = ({ isOpen }) => { {authService.hasPermission('manage_db') && } label="Databases" />} {authService.hasPermission('read_telemetry') && } label="Access Logs" />} {authService.hasPermission('read_telemetry') && } label="Activity Logs" />} + {/* Ungated on purpose - see the route. A link only some roles can + see would leave the rest unable to secure their own account. */} + } label="My Security" /> diff --git a/frontend/src/pages/AccountSecurity.tsx b/frontend/src/pages/AccountSecurity.tsx new file mode 100644 index 0000000..78c29f1 --- /dev/null +++ b/frontend/src/pages/AccountSecurity.tsx @@ -0,0 +1,289 @@ +import React, { useEffect, useState } from 'react'; +import { ShieldCheck, ShieldOff, KeyRound, Copy, Check, AlertTriangle } from 'lucide-react'; +import { QRCodeSVG } from 'qrcode.react'; +import { authService } from '../services/api'; +import { Card, PageHeader } from '../components/ui'; + +/** Every operator can reach this, whatever their role. + * + * Two-factor protects the account, not the tenancy - a role that can do + * nothing else must still be able to secure its own login. That is why the + * four endpoints behind this page are session-only rather than + * permission-gated. + */ +const note: React.CSSProperties = { + margin: '0 0 var(--space-3)', color: 'var(--text-secondary)', + fontSize: 'var(--text-sm)', maxWidth: '62ch', lineHeight: 1.5, +}; + +const mono: React.CSSProperties = { + fontFamily: 'monospace', fontSize: 'var(--text-sm)', + background: 'var(--surface-2)', border: '1px solid var(--border-color)', + borderRadius: 'var(--radius-md)', padding: 'var(--space-3)', + wordBreak: 'break-all', color: 'var(--text-primary)', +}; + +const AccountSecurity: React.FC = () => { + const [status, setStatus] = useState(null); + const [busy, setBusy] = useState(false); + const [error, setError] = useState(''); + + // Enrolment, which is three states rather than a boolean: nothing yet, a + // secret waiting to be proved, and the codes shown exactly once afterwards. + const [enrolling, setEnrolling] = useState(null); + const [code, setCode] = useState(''); + const [recoveryCodes, setRecoveryCodes] = useState(null); + const [copied, setCopied] = useState(false); + const [copiedSecret, setCopiedSecret] = useState(false); + + const [password, setPassword] = useState(''); + + const refresh = () => authService.twoFactorStatus().then(setStatus).catch(() => setStatus(null)); + useEffect(() => { refresh(); }, []); + + const begin = async () => { + setError(''); setBusy(true); + try { setEnrolling(await authService.twoFactorEnrol()); } + catch (e: any) { setError(e?.response?.data?.message || 'Could not start enrolment.'); } + finally { setBusy(false); } + }; + + const confirm = async (e: React.FormEvent) => { + e.preventDefault(); + setError(''); setBusy(true); + try { + const res = await authService.twoFactorConfirm(code); + setRecoveryCodes(res.recovery_codes); + setEnrolling(null); + setCode(''); + refresh(); + } catch (e: any) { + setError(e?.response?.data?.message || 'That code is not valid.'); + } finally { setBusy(false); } + }; + + const disable = async (e: React.FormEvent) => { + e.preventDefault(); + setError(''); setBusy(true); + try { + await authService.twoFactorDisable(password); + setPassword(''); + setRecoveryCodes(null); + refresh(); + } catch (e: any) { + setError(e?.response?.data?.message || 'Could not turn off two-factor.'); + } finally { setBusy(false); } + }; + + const enabled = !!status?.enabled; + + return ( +
+ + + {!!error && ( +
+ {error} +
+ )} + + {/* Shown once, and the page says so, because the server genuinely cannot + show them again - they are stored as hashes, which is the property + that makes storing them safe at all. */} + {!!recoveryCodes && ( + +

+ Each one signs you in once if you lose your authenticator. They are + stored as hashes, so this page cannot show them again — if you + leave without copying them, the only way back is to turn two-factor + off with your password and enrol again. +

+
+ {recoveryCodes.map((c) =>
{c}
)} +
+ + + )} + + +
+ {enabled + ? + : } +
+ {enabled + ? `Signing in needs a code from your authenticator. ${status?.recovery_codes_remaining ?? 0} recovery code(s) left.` + : 'Signing in needs only your password.'} +
+
+ + {!enabled && !enrolling && ( + <> +

+ A password alone protects this console against guessing and + nothing else. It can isolate a host, run a command as SYSTEM on + every endpoint, and read every log the fleet has produced — a + password that has been reused or phished is enough for all of it. +

+ + + )} + + {!!enrolling && ( + <> +

+ Scan this with your authenticator, then prove it with a code. + Nothing changes about your login until you do — a secret that is + scanned and never confirmed would lock you out. +

+ +
+ {/* On a white plate, always, in both themes. + A scanner needs dark modules on a light ground, and a code + drawn in theme colours on a dark card is a code that reads + perfectly to a human and not at all to a phone. */} +
+ +
+ +
+
+ CAN'T SCAN? TYPE THIS KEY INSTEAD +
+
+ {enrolling.secret} +
+ +
+
+ +
+ setCode(e.target.value)} + placeholder="123456" + inputMode="numeric" + autoComplete="one-time-code" + style={{ + padding: 'var(--space-3)', borderRadius: 'var(--radius-md)', + background: 'var(--bg-color)', border: '1px solid var(--border-color)', + color: 'var(--text-primary)', fontFamily: 'monospace', + letterSpacing: '0.25em', width: '160px', + }} + /> + + +
+ + )} + + {enabled && ( + <> +

+ Turning it off needs your password. A session that has been taken + over must not be able to remove the control that would have + stopped it. +

+
+ setPassword(e.target.value)} + placeholder="Your password" + style={{ + padding: 'var(--space-3)', borderRadius: 'var(--radius-md)', + background: 'var(--bg-color)', border: '1px solid var(--border-color)', + color: 'var(--text-primary)', width: '220px', + }} + /> + +
+ + )} + +
+ ); +}; + +export default AccountSecurity; diff --git a/frontend/src/pages/Login.tsx b/frontend/src/pages/Login.tsx index c714818..bd8bdf9 100644 --- a/frontend/src/pages/Login.tsx +++ b/frontend/src/pages/Login.tsx @@ -1,6 +1,6 @@ import React, { useState } from 'react'; import { useNavigate, useLocation } from 'react-router-dom'; -import { ShieldAlert, Lock, User, AlertCircle, Loader2 } from 'lucide-react'; +import { ShieldAlert, Lock, User, AlertCircle, Loader2, KeyRound } from 'lucide-react'; import { authService } from '../services/api'; const Login: React.FC = () => { @@ -8,6 +8,12 @@ const Login: React.FC = () => { const [password, setPassword] = useState(''); const [error, setError] = useState(''); const [loading, setLoading] = useState(false); + + /* The half-authenticated state. Held in a component, never in storage: it + is a credential with a five-minute life, and putting it anywhere + persistent would outlive the tab it was meant for. */ + const [pendingToken, setPendingToken] = useState(null); + const [code, setCode] = useState(''); const navigate = useNavigate(); const location = useLocation(); @@ -19,7 +25,11 @@ const Login: React.FC = () => { setLoading(true); try { - await authService.login({ username, password }); + const result = await authService.login({ username, password }); + if (result?.status === 'second_factor_required') { + setPendingToken(result.token); + return; + } navigate(from, { replace: true }); } catch (err: any) { setError(err.message || 'Login failed. Please check your credentials.'); @@ -28,6 +38,29 @@ const Login: React.FC = () => { } }; + const handleSecondFactor = async (e: React.FormEvent) => { + e.preventDefault(); + setError(''); + setLoading(true); + try { + await authService.completeSecondFactor(pendingToken as string, code); + navigate(from, { replace: true }); + } catch (err: any) { + setError(err.message || 'That code is not valid.'); + setCode(''); + /* An expired or spent token cannot be retried, and the server answers + every such case identically on purpose. Sending the operator back to + the password rather than leaving them typing codes at something that + will never accept one. */ + if (/expired/i.test(err.message || '')) { + setPendingToken(null); + setPassword(''); + } + } finally { + setLoading(false); + } + }; + return (
{
)} + {pendingToken ? ( + /* The second step replaces the first rather than appearing beside + it. Two forms on screen at once invites typing a code into a + password field, and the password has already been accepted - it + has nothing left to do here. */ +
+
+ +
+ + setCode(e.target.value)} + placeholder="123456" + autoFocus + autoComplete="one-time-code" + inputMode="numeric" + style={{ + width: '100%', padding: '14px 16px 14px 48px', + backgroundColor: 'var(--bg-color)', + border: '1px solid var(--border-color)', borderRadius: '4px', + color: 'var(--text-primary)', fontSize: '1.1rem', + letterSpacing: '0.25em', fontFamily: 'monospace', + outline: 'none', + }} + /> +
+

+ From your authenticator app, or one of your recovery codes. +

+
+ + + + +
+ ) : (
@@ -164,6 +259,7 @@ const Login: React.FC = () => { {loading ? : 'AUTHENTICATE'} + )}

api.post('/login', credentials).then(res => { + // A password that is correct but not sufficient. Returned to the caller + // rather than thrown: this used to fall through to the `throw` below and + // surface as "Login failed", so switching on two-factor would have locked + // every operator out of the console with a message saying their password + // was wrong. + if (res.data.status === 'second_factor_required') { + return res.data; + } if (res.data.status === 'success') { localStorage.setItem('userId', res.data.user.id.toString()); localStorage.setItem('user', JSON.stringify(res.data.user)); @@ -68,6 +76,33 @@ export const authService = { } throw new Error(res.data.message || 'Login failed'); }), + + /** Finish a login that passed its password. */ + completeSecondFactor: (token: string, code: string) => + api.post('/login/2fa', { token, code }).then(res => { + if (res.data.status !== 'success') { + throw new Error(res.data.message || 'That code is not valid.'); + } + // The same bookkeeping `login` does on success. Duplicated rather than + // shared because the two paths return different shapes and a helper + // that flattened them would hide which one issued the session. + localStorage.setItem('userId', res.data.user.id.toString()); + localStorage.setItem('user', JSON.stringify(res.data.user)); + if (res.data.user.must_change_password) { + localStorage.setItem('mustChangePassword', '1'); + } else { + localStorage.removeItem('mustChangePassword'); + } + return res.data; + }), + + twoFactorStatus: () => api.get('/api/2fa/status').then(r => r.data), + twoFactorEnrol: () => api.post('/api/2fa/enrol').then(r => r.data), + twoFactorConfirm: (code: string) => + api.post('/api/2fa/confirm', { code }).then(r => r.data), + twoFactorDisable: (password: string) => + api.post('/api/2fa/disable', { password }).then(r => r.data), + logout: async () => { // Revoke server-side first; clearing localStorage alone used to leave the // session valid for anyone who still had the cookie. diff --git a/security/totp.py b/security/totp.py new file mode 100644 index 0000000..dad1ed7 --- /dev/null +++ b/security/totp.py @@ -0,0 +1,268 @@ +"""A second factor for operator logins. + +The console has been protected by one password and a lockout. That is enough +against guessing and nothing at all against a password that has been reused, +phished, or read out of somebody's browser - and this console can isolate a +host, run a command as SYSTEM on every endpoint, and read every log the fleet +has produced. + +What this module is careful about, in the order the mistakes usually happen: + +**The pending state is not a session.** Passing the password gets a short-lived +token that can do exactly one thing: present a second factor. Issuing a real +session and "upgrading" it later means a window in which one factor is a +logged-in operator, and every bug in that window is a bypass. + +**A code may be used once.** TOTP is a shared secret and a clock, so the same +six digits are valid for the whole step. Without a replay guard, anyone who +watches one code being typed has thirty seconds to use it themselves - which +is exactly the position a shoulder-surfer or a proxy is in. + +**Recovery codes are credentials.** They are stored the way passwords are, as +hashes, and burnt on use. Storing them in the clear so the console can show +them again is the same mistake as storing a password, made for a friendlier +reason. + +**The secret is encrypted at rest.** A TOTP secret is enough to mint codes for +ever. The server already holds a Fernet key for telemetry; the seed goes +through it, so a dump of `userdb` alone does not hand over everybody's second +factor. + +**Enrolment is not complete until a code is proved.** A secret written at the +moment the QR code is shown, with no confirmation, locks out every operator +who scans it into an app they then delete. Nothing is enforced until one code +from the new secret has been verified. +""" +from __future__ import annotations + +import base64 +import hashlib +import hmac +import os +import secrets +import struct +import time + +#: Seconds per code. The interoperable value - every authenticator app assumes +#: it, and it is not worth being unusual about. +STEP_SECONDS = 30 + +#: How many steps either side of now are accepted. +#: +#: One. That is 30 seconds of clock skew in each direction, which covers a +#: phone that has not synchronised recently without widening the window an +#: attacker gets to reuse an observed code. Zero would reject honest users on +#: ordinary drift; three is 90 seconds of replay surface for no gain. +ALLOWED_DRIFT_STEPS = 1 + +#: Digits in a code. Six, for the same reason as the step: universal. +DIGITS = 6 + +#: Recovery codes issued when the second factor is enabled. +#: +#: Ten is enough to survive losing a phone more than once without becoming a +#: list somebody prints and leaves on a desk. +RECOVERY_CODE_COUNT = 10 + +#: Bytes of entropy per recovery code. 20 hex characters at 80 bits - long +#: enough that guessing is not a strategy, short enough to type from paper. +RECOVERY_CODE_BYTES = 10 + + +def new_secret() -> str: + """A fresh base32 seed, in the shape every authenticator app expects. + + 160 bits, per RFC 4226's recommendation for HMAC-SHA1. `secrets` rather + than `random`: this is key material. + """ + return base64.b32encode(secrets.token_bytes(20)).decode("ascii").rstrip("=") + + +def _hotp(secret_b32: str, counter: int) -> str: + """One HOTP value, which is all TOTP is with the counter set from a clock. + + Written out rather than pulled in. It is eleven lines of RFC 4226, it has + no dependency worth adding for, and the padding rule below is the kind of + detail a comment can carry and a library cannot. + """ + # Authenticator apps hand out secrets with the `=` padding stripped; + # `b32decode` insists on it. Restoring it here means a user pasting a + # secret from anywhere works. + padded = secret_b32.strip().replace(" ", "").upper() + padded += "=" * (-len(padded) % 8) + key = base64.b32decode(padded, casefold=True) + + digest = hmac.new(key, struct.pack(">Q", counter), hashlib.sha1).digest() + offset = digest[-1] & 0x0F + truncated = struct.unpack(">I", digest[offset:offset + 4])[0] & 0x7FFFFFFF + return str(truncated % (10 ** DIGITS)).zfill(DIGITS) + + +def current_step(at: float | None = None) -> int: + """Which time step we are in. Exposed so the replay guard can name one.""" + return int((time.time() if at is None else at) // STEP_SECONDS) + + +def codes_in_window(secret_b32: str, at: float | None = None) -> dict[int, str]: + """{step: code} for every step currently acceptable. + + Returns the steps as well as the codes because the caller has to record + *which* step it accepted - see the replay guard. A function that returned + only a boolean would make single-use impossible to implement. + """ + now = current_step(at) + return {step: _hotp(secret_b32, step) + for step in range(now - ALLOWED_DRIFT_STEPS, + now + ALLOWED_DRIFT_STEPS + 1)} + + +def verify(secret_b32: str, code: str, at: float | None = None) -> int | None: + """The step a code is valid for, or None. + + Compared with `compare_digest`. A plain `==` on a six-digit code leaks how + much of it was right through timing, and six digits is a small enough + space that it is worth not helping. + """ + cleaned = (code or "").strip().replace(" ", "").replace("-", "") + + # ASCII digits specifically, not `isdigit()`. + # + # `"٠١٢٣٤٥".isdigit()` is True - those are + # Arabic-Indic digits, and Python counts every decimal digit in Unicode. + # Six of them pass a length check and reach `compare_digest`, which raises + # `TypeError: comparing strings with non-ASCII characters is not + # supported`. So a login form that accepted any non-Latin numeral turned a + # typo into a 500 on the authentication path. + if len(cleaned) != DIGITS or not all(c in "0123456789" for c in cleaned): + return None + for step, expected in codes_in_window(secret_b32, at).items(): + if hmac.compare_digest(cleaned, expected): + return step + return None + + +def provisioning_uri(secret_b32: str, *, account: str, issuer: str) -> str: + """The `otpauth://` URI an authenticator app scans. + + Assembled rather than formatted with the secret interpolated raw: an + account name is a username and usernames contain characters that would + otherwise end the query string early. + """ + from urllib.parse import quote, urlencode + + label = quote(f"{issuer}:{account}", safe="") + params = urlencode({ + "secret": secret_b32, + "issuer": issuer, + "algorithm": "SHA1", + "digits": DIGITS, + "period": STEP_SECONDS, + }) + return f"otpauth://totp/{label}?{params}" + + +# --------------------------------------------------------------------------- +# Recovery codes +# --------------------------------------------------------------------------- + +def new_recovery_codes(count: int = RECOVERY_CODE_COUNT) -> list[str]: + """Codes to show once and never again. + + Grouped with a dash purely so they can be read aloud and typed from paper + without losing your place; `verify_recovery` strips it back out. + """ + codes = [] + for _ in range(count): + raw = secrets.token_hex(RECOVERY_CODE_BYTES) + codes.append(f"{raw[:10]}-{raw[10:]}") + return codes + + +def hash_recovery(code: str) -> str: + """How a recovery code is stored. Never the code itself. + + SHA-256 and not bcrypt, deliberately, and the reasoning matters because + the opposite is right for passwords: these are 80 bits of machine-chosen + entropy, so there is no dictionary to slow down and nothing to stretch. A + cost factor here would only make the login path slower. + """ + cleaned = (code or "").strip().replace("-", "").replace(" ", "").lower() + return hashlib.sha256(cleaned.encode("utf-8")).hexdigest() + + +# --------------------------------------------------------------------------- +# Schema +# --------------------------------------------------------------------------- + +DDL_SECRETS = """ +CREATE TABLE IF NOT EXISTS user_totp ( + user_id INT NOT NULL PRIMARY KEY, + secret VARCHAR(512) NOT NULL, + confirmed_at TIMESTAMP NULL DEFAULT NULL, + created_at TIMESTAMP NULL DEFAULT CURRENT_TIMESTAMP, + last_step BIGINT UNSIGNED NULL, + CONSTRAINT fk_totp_user FOREIGN KEY (user_id) REFERENCES users(id) + ON DELETE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 +""" + +# `secret` is VARCHAR(512) rather than the ~32 characters a base32 seed needs, +# because it is stored encrypted and Fernet ciphertext is a hundred-odd +# characters regardless of the plaintext. Sizing a column for the value a +# human would read is how `network_connections` lost a day of telemetry to +# "Data too long" on this codebase already. + +DDL_RECOVERY = """ +CREATE TABLE IF NOT EXISTS user_recovery_codes ( + id BIGINT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + user_id INT NOT NULL, + code_hash CHAR(64) NOT NULL, + used_at TIMESTAMP NULL DEFAULT NULL, + created_at TIMESTAMP NULL DEFAULT CURRENT_TIMESTAMP, + UNIQUE KEY uq_recovery (user_id, code_hash), + CONSTRAINT fk_recovery_user FOREIGN KEY (user_id) REFERENCES users(id) + ON DELETE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 +""" + +#: How long a half-authenticated login may sit before the password has to be +#: presented again. +#: +#: Five minutes: long enough to find a phone, short enough that a token left +#: in a URL bar or a proxy log is not a standing invitation. +PENDING_TTL_SECONDS = 300 + +DDL_PENDING = """ +CREATE TABLE IF NOT EXISTS totp_pending ( + token_hash CHAR(64) NOT NULL PRIMARY KEY, + user_id INT NOT NULL, + username VARCHAR(50) NOT NULL, + auth_type VARCHAR(16) NOT NULL, + ip VARCHAR(64) NULL, + attempts INT UNSIGNED NOT NULL DEFAULT 0, + expires_at TIMESTAMP NOT NULL, + created_at TIMESTAMP NULL DEFAULT CURRENT_TIMESTAMP, + KEY idx_pending_expiry (expires_at), + CONSTRAINT fk_pending_user FOREIGN KEY (user_id) REFERENCES users(id) + ON DELETE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 +""" + +# Stored as a hash, like a session token, and for the same reason: a dump of +# this table must not let anybody finish a login that somebody else started. + +#: Wrong codes allowed against one pending login before it is thrown away. +#: +#: Five. A six-digit code is a million possibilities, so this is not what +#: stops guessing on its own - `core.login_guard` already counts failures per +#: user and per address. What it stops is one captured pending token being +#: used as an unlimited oracle. +MAX_PENDING_ATTEMPTS = 5 + + +def new_pending_token() -> str: + return secrets.token_urlsafe(32) + + +def hash_pending(raw: str) -> str: + return hashlib.sha256((raw or "").encode("utf-8")).hexdigest() diff --git a/tests/test_auth_wiring.py b/tests/test_auth_wiring.py index 14dbdc1..9670344 100644 --- a/tests/test_auth_wiring.py +++ b/tests/test_auth_wiring.py @@ -165,6 +165,20 @@ def test_permission_registry_is_keyed_before_the_wrapper_is_built(tree): # the fleet-wide secret, which the ordinary agent routes accept, because # this channel carries /self_destruct. "agent_link_socket", + # Two-factor, for the calling account only. + # + # Each of these reads `current_user_id(request)` and refuses without it, + # then acts on that id and no other - there is no user parameter to + # confuse. A permission would be the wrong check: enrolling a second + # factor is something every operator must be able to do for themselves, + # including one whose role can do nothing else. + # + # `second_factor_disable` additionally demands the account password, so a + # hijacked session cannot remove the control that would have stopped it. + "second_factor_status", + "second_factor_enrol", + "second_factor_confirm", + "second_factor_disable", } # Named individually so a bulk edit to the list above cannot quietly reopen diff --git a/tests/test_second_factor.py b/tests/test_second_factor.py new file mode 100644 index 0000000..62fa4c6 --- /dev/null +++ b/tests/test_second_factor.py @@ -0,0 +1,451 @@ +"""A second factor, and the five ways one gets built wrong. + +The console has been protected by a password and a lockout. That is enough +against guessing and nothing at all against a password that has been reused, +phished, or read out of a browser - and this console can isolate a host, run a +command as SYSTEM on every endpoint, and read every log the fleet produced. + +The mechanism is the easy part; RFC 4226 is eleven lines. What this file pins +is the surrounding design, because each of these is a way to ship something +that looks like two-factor and is not: + + a session issued before the second factor, then "upgraded" + a code that can be used twice inside its thirty-second step + recovery codes stored so they can be shown again + a secret readable from a database dump + one login path gated and another not +""" +from __future__ import annotations + +import ast +import base64 +import pathlib +import time + +import pytest + +ROOT = pathlib.Path(__file__).resolve().parent.parent +APP = ROOT / "app.py" + +from security import totp # noqa: E402 + + +def _function(name: str): + return next(n for n in ast.walk(ast.parse(APP.read_text(encoding="utf-8"))) + if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef)) + and n.name == name) + + +# -------------------------------------------------------------------------- +# The mechanism +# -------------------------------------------------------------------------- + +def test_rfc4226_vectors(): + """The published test vectors. Written out rather than depending on a + library, so this is the only thing standing between an arithmetic slip and + codes that nobody's authenticator agrees with.""" + secret = base64.b32encode(b"12345678901234567890").decode() + expected = ["755224", "287082", "359152", "969429", "338314", + "254676", "287922", "162583", "399871", "520489"] + assert [totp._hotp(secret, c) for c in range(10)] == expected + + +def test_a_secret_is_key_material(): + a, b = totp.new_secret(), totp.new_secret() + assert a != b + assert len(a) >= 32 + assert set(a) <= set("ABCDEFGHIJKLMNOPQRSTUVWXYZ234567") + + +def test_padding_stripped_secrets_still_work(): + """Authenticator apps hand out secrets without `=` padding and + `b32decode` insists on it. A user pasting one from anywhere has to work.""" + secret = totp.new_secret().rstrip("=") + assert totp.verify(secret, totp._hotp(secret, totp.current_step())) is not None + + +def test_the_drift_window_is_narrow(): + """One step either side. Zero rejects honest users on ordinary clock + drift; three hands an attacker ninety seconds to reuse a code they saw.""" + assert totp.ALLOWED_DRIFT_STEPS == 1 + now = time.time() + assert len(totp.codes_in_window("A" * 32, now)) == 3 + + secret = totp.new_secret() + stale = totp._hotp(secret, totp.current_step(now) - 2) + assert totp.verify(secret, stale, now) is None + + +@pytest.mark.parametrize("bad", ["", "12345", "1234567", "abcdef", "12 34 5", + None, "٠١٢٣٤٥"]) +def test_a_malformed_code_is_refused_without_raising(bad): + """The login path must not turn a typo into a 500.""" + assert totp.verify(totp.new_secret(), bad) is None + + +def test_the_comparison_is_constant_time(): + """Six digits is a small space, and `==` on a string leaks how much of it + was right through timing. Not worth helping.""" + source = (ROOT / "security" / "totp.py").read_text(encoding="utf-8") + fn = next(n for n in ast.parse(source).body + if isinstance(n, ast.FunctionDef) and n.name == "verify") + assert "compare_digest" in ast.unparse(fn) + + +# -------------------------------------------------------------------------- +# The design +# -------------------------------------------------------------------------- + +def test_no_session_exists_until_both_factors_pass(): + """The mistake that makes two-factor decorative: issue a session on the + password, then upgrade it when a code arrives. Every bug in that window is + a bypass.""" + login = ast.unparse(_function("login")) + gate = login.index("needs_second") + issue = login.index("_issue_session") + assert gate < issue, "a session is created before the second factor is asked for" + + +def test_both_login_paths_are_gated(): + """A factor that protects one path and not the other protects nothing - + an attacker with a password picks the path without it.""" + login = ast.unparse(_function("login")) + assert login.count("_totp_required") >= 2, \ + "only one of the local and LDAP branches checks for a second factor" + assert login.count("second_factor_required") >= 2 + + +def test_a_code_cannot_be_used_twice(): + """TOTP is a shared secret and a clock: the same six digits stay valid for + the whole step. Without this, watching one code being typed buys thirty + seconds to use it.""" + body = ast.unparse(_function("complete_second_factor")) + assert "last_step" in body + assert "last_step < %s" in body or "last_step <" in body, \ + "nothing stops a code from earlier in the accepted window being replayed" + assert "TOTP_CODE_REPLAYED" in body, "a replay is not reported" + + +def test_a_pending_login_is_not_an_unlimited_oracle(): + body = ast.unparse(_function("complete_second_factor")) + assert "MAX_PENDING_ATTEMPTS" in body + assert totp.MAX_PENDING_ATTEMPTS <= 10 + + +def test_the_pending_token_is_stored_hashed(): + """Like a session token, and for the same reason: a dump of this table + must not let anybody finish a login somebody else started.""" + body = ast.unparse(_function("complete_second_factor")) + assert "hash_pending" in body + assert "token_hash" in body + + +def test_the_pending_token_expires(): + assert 0 < totp.PENDING_TTL_SECONDS <= 900 + assert "expires_at > NOW()" in ast.unparse(_function("complete_second_factor")) + + +def test_expired_spent_and_invalid_are_one_message(): + """Telling them apart tells an attacker which half of a captured pair is + still good.""" + body = ast.unparse(_function("complete_second_factor")) + assert body.count("This login has expired. Sign in again.") == 1 + + +# -------------------------------------------------------------------------- +# Recovery codes are credentials +# -------------------------------------------------------------------------- + +def test_recovery_codes_are_stored_as_hashes(): + body = ast.unparse(_function("second_factor_confirm")) + assert "hash_recovery" in body + assert "code_hash" in body + + +def test_they_are_shown_once_and_only_once(): + """Storing them so the console can show them again is the same mistake as + storing a password, made for a friendlier reason.""" + # The values, not the words. The first version matched any handler + # mentioning "recovery_codes", which `second_factor_disable` does because + # it deletes them - so the test failed on a route that leaks nothing. + source = APP.read_text(encoding="utf-8") + leaking = [] + for node in ast.walk(ast.parse(source)): + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + body = ast.unparse(node) + if "'recovery_codes': codes" in body or '"recovery_codes": codes' in body: + leaking.append(node.name) + assert leaking == ["second_factor_confirm"], ( + f"the generated codes are returned from {leaking}; they may leave the " + f"server exactly once" + ) + + status = ast.unparse(_function("second_factor_status")) + assert "recovery_codes_remaining" in status + assert "code_hash" not in status, "the status route can leak the stored hashes" + + +def test_a_recovery_code_is_burnt_on_use(): + body = ast.unparse(_function("complete_second_factor")) + assert "used_at = NOW()" in body + assert "used_at IS NULL" in body, "a spent recovery code can be replayed" + assert "RECOVERY_CODE_USED" in body, \ + "signing in with a recovery code is not reported" + + +def test_recovery_hashing_is_not_stretched(): + """Deliberate, and the opposite of the rule for passwords: these are 80 + bits of machine-chosen entropy, so there is no dictionary to slow down. A + cost factor would only make the login path slower.""" + source = (ROOT / "security" / "totp.py").read_text(encoding="utf-8") + fn = next(n for n in ast.parse(source).body + if isinstance(n, ast.FunctionDef) and n.name == "hash_recovery") + # Statements only. The docstring explains why *not* bcrypt, so matching + # the whole function finds the explanation and reads it as the code - + # the eighth time that has happened in this suite. + statements = [n for n in fn.body + if not (isinstance(n, ast.Expr) + and isinstance(n.value, ast.Constant))] + body = "\n".join(ast.unparse(n) for n in statements) + assert "sha256" in body + assert "bcrypt" not in body + + +# -------------------------------------------------------------------------- +# The secret at rest +# -------------------------------------------------------------------------- + +def test_the_secret_is_encrypted_in_the_database(): + """A TOTP seed mints codes for ever. A dump of `userdb` alone must not + hand over everybody's second factor.""" + enrol = ast.unparse(_function("second_factor_enrol")) + assert "fernet.encrypt" in enrol + + read = ast.unparse(_function("_totp_secret_for")) + assert "fernet.decrypt" in read + + +def test_the_column_is_sized_for_ciphertext(): + """Fernet output is a hundred-odd characters whatever the plaintext. + Sizing a column for the value a human would read is how this codebase + already lost a day of telemetry to "Data too long".""" + source = (ROOT / "security" / "totp.py").read_text(encoding="utf-8") + secrets_ddl = source[source.index("DDL_SECRETS"):source.index("DDL_RECOVERY")] + assert "VARCHAR(512)" in secrets_ddl + + +def test_an_unreadable_secret_does_not_lock_the_account(): + """A changed Fernet key should mean "enrol again", not "you can never log + in". Reported loudly, treated as absent.""" + body = ast.unparse(_function("_totp_secret_for")) + assert "except Exception" in body + assert "return (None, False)" in body or "return None, False" in body + + +# -------------------------------------------------------------------------- +# Enrolment and removal +# -------------------------------------------------------------------------- + +def test_enrolment_is_not_complete_until_a_code_is_proved(): + """A secret written when the QR code is shown, with nothing confirming it, + locks out everybody who scans it into an app they then delete.""" + enrol = ast.unparse(_function("second_factor_enrol")) + assert "confirmed_at" in enrol and "NULL" in enrol + + confirm = ast.unparse(_function("second_factor_confirm")) + assert "verify" in confirm + assert "confirmed_at = NOW()" in confirm + + required = ast.unparse(_function("_totp_required")) + assert "confirmed" in required, \ + "an unconfirmed enrolment would be enforced at the next login" + + +def test_re_enrolling_cannot_silently_replace_a_working_factor(): + enrol = ast.unparse(_function("second_factor_enrol")) + assert "status=409" in enrol + + +def test_turning_it_off_needs_the_password(): + """A hijacked session must not be able to remove the control that would + have stopped it.""" + body = ast.unparse(_function("second_factor_disable")) + assert "bcrypt.checkpw" in body + assert "TOTP_DISABLE_REFUSED" in body, "a failed attempt to disable is not reported" + + +def test_disabling_clears_the_recovery_codes_too(): + """Leaving them behind means the next enrolment inherits a list the user + thinks is gone.""" + body = ast.unparse(_function("second_factor_disable")) + assert "DELETE FROM user_recovery_codes" in body + + +# -------------------------------------------------------------------------- +# It has to actually be importable +# -------------------------------------------------------------------------- + +def test_the_module_is_imported(): + """Every other test in this file reads the AST, which does not care + whether a name resolves. The routes were spliced in without their import + and every one of them passed while the live server answered + + [!] Unhandled Exception: name 'totp_store' is not defined + + with a 500 on the second half of every login. A test suite that parses + but never runs the thing it is checking needs one assertion that closes + that gap. + """ + source = APP.read_text(encoding="utf-8") + assert "import totp as totp_store" in source, \ + "app.py uses totp_store and never imports it" + + +def test_every_name_the_routes_use_is_defined(): + """The general form of the above, since the same splice could drop any + helper. Compares what the 2FA handlers reference against what app.py + defines and imports at module level.""" + import builtins + + tree = ast.parse(APP.read_text(encoding="utf-8")) + defined = set(dir(builtins)) + for node in tree.body: + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): + defined.add(node.name) + elif isinstance(node, ast.Assign): + defined.update(t.id for t in node.targets if isinstance(t, ast.Name)) + elif isinstance(node, (ast.Import, ast.ImportFrom)): + defined.update(a.asname or a.name.split(".")[0] for a in node.names) + + handlers = ("complete_second_factor", "second_factor_status", + "second_factor_enrol", "second_factor_confirm", + "second_factor_disable", "_begin_second_factor", + "_totp_secret_for", "_totp_required", "_totp_tables") + missing = set() + for name in handlers: + fn = _function(name) + local = {a.arg for a in fn.args.args} | {a.arg for a in fn.args.kwonlyargs} + for inner in ast.walk(fn): + if isinstance(inner, ast.Name) and isinstance(inner.ctx, ast.Store): + local.add(inner.id) + elif isinstance(inner, (ast.AsyncWith, ast.With)): + for item in inner.items: + if isinstance(item.optional_vars, ast.Name): + local.add(item.optional_vars.id) + elif isinstance(inner, ast.comprehension) and isinstance(inner.target, ast.Name): + local.add(inner.target.id) + elif isinstance(inner, ast.ExceptHandler) and inner.name: + # `except Exception as e` binds a name too. Missing it made + # the scan report `e` as undefined - a gap in the detector + # rather than in the code it was checking. + local.add(inner.name) + for inner in ast.walk(fn): + if (isinstance(inner, ast.Name) and isinstance(inner.ctx, ast.Load) + and inner.id not in local and inner.id not in defined): + missing.add(f"{name}: {inner.id}") + + assert not missing, f"names used and never defined: {sorted(missing)}" + + +# -------------------------------------------------------------------------- +# Enrolment has to be usable +# -------------------------------------------------------------------------- + +FRONTEND = ROOT / "frontend" / "src" + + +def test_the_secret_is_offered_as_a_qr_code(): + """Typing a 32-character base32 key by hand is the kind of friction that + decides whether a security control gets turned on at all. The first + version skipped the dependency and made every operator do it.""" + page = (FRONTEND / "pages" / "AccountSecurity.tsx").read_text(encoding="utf-8") + assert "QRCodeSVG" in page + assert "enrolling.uri" in page, "the QR encodes something other than the otpauth URI" + + package = (ROOT / "frontend" / "package.json").read_text(encoding="utf-8") + assert "qrcode.react" in package, "the component is imported and not declared" + + +def test_the_qr_is_drawn_on_a_light_ground(): + """A scanner needs dark modules on a light background. A code drawn in + theme colours on a dark card reads perfectly to a human and not at all to + a phone - and it fails silently, which is the worst way for this to be + wrong.""" + page = (FRONTEND / "pages" / "AccountSecurity.tsx").read_text(encoding="utf-8") + # Anchored on the element, not the name: the first occurrence of + # "QRCodeSVG" is the import at the top of the file, so slicing back from + # it found nothing and the test failed against correct markup. + at = page.index(" str: + text = GUIDE.read_text(encoding="utf-8") + return text[text.index("## 6. Identity"):text.index("\n## 7.")] + + +def test_two_factor_is_in_the_operations_guide(): + """This whole body of work started from a guide describing three settings + the code never read. A shipped feature the guide never mentions is the + same drift with the sign flipped - and it is the one an operator hits when + they need it at three in the morning.""" + section = _identity_section() + assert "Two-factor" in section + assert "My Security" in section, "the guide never says where to turn it on" + + +def test_the_guide_says_what_happens_when_recovery_codes_are_lost(): + """The question every operator eventually asks, and the one with no happy + answer - any mechanism letting an administrator bypass somebody's second + factor is a mechanism an attacker can use. Better said than discovered.""" + section = _identity_section() + assert "cannot sign in" in section or "no support path" in section + assert "user_totp" in section, "no documented way back at all" + + +def test_the_guide_warns_about_the_fernet_key(): + """Losing it takes every enrolled second factor with it. That is + survivable and has to be planned for, not found out.""" + section = _identity_section() + assert "Fernet" in section + + +def test_the_platform_event_kinds_are_documented(): + """A severity in a table nobody has a key for is a severity nobody acts + on.""" + from core import self_defence + + section = _identity_section() + for kind in self_defence.KINDS: + assert kind in section, f"{kind} is raised and never explained in the guide" + + +def test_every_section_reference_resolves(): + """The renumbering that adding these sections required silently broke a + forward reference the first time. A guide that points at a section that + does not exist is worse than one that points at nothing.""" + import re + + text = GUIDE.read_text(encoding="utf-8") + headings = set(re.findall(r"^#{2,3} (\d+(?:\.\d+)*)", text, re.M)) + referenced = set(re.findall(r"§(\d+(?:\.\d+)*)", text)) + dangling = sorted(r for r in referenced if r not in headings) + assert not dangling, f"references to sections that do not exist: {dangling}" diff --git a/tests/test_self_defence.py b/tests/test_self_defence.py index a58ec4c..33418fa 100644 --- a/tests/test_self_defence.py +++ b/tests/test_self_defence.py @@ -166,13 +166,58 @@ def test_it_does_not_block_the_event_loop(): # Detected and recorded is only half of it # -------------------------------------------------------------------------- -@pytest.mark.parametrize("kind", ["LOGIN_LOCKOUT", "AGENT_KEY_REJECTED", - "FLEET_KEY_ON_CHANNEL"]) -def test_the_detections_are_actually_wired(kind): - """A vocabulary nothing raises is a vocabulary. These three are already - detected in app.py and used to stop at a print.""" - assert f'"{kind}"' in APP.read_text(encoding="utf-8"), \ - f"{kind} is defined and never raised" +def _raised_kinds() -> set: + """Kinds that actually reach `_platform_event`, following variables. + + Reading the call sites rather than searching the file. The first version + of this test asserted the name appeared *somewhere* in app.py, which a + mention in a comment satisfies - and three of six kinds sat unwired while + it passed. Two of the three that were wired pass their kind through a + variable, so looking only for a literal argument misses those too; the + assignments are followed. + """ + tree = ast.parse(APP.read_text(encoding="utf-8")) + raised = set() + for node in ast.walk(tree): + if not (isinstance(node, ast.Call) + and getattr(node.func, "id", "") == "_platform_event" + and node.args): + continue + first = node.args[0] + if isinstance(first, ast.Constant): + raised.add(first.value) + continue + name = getattr(first, "id", "") + for other in ast.walk(tree): + if isinstance(other, ast.Assign) and any( + getattr(t, "id", "") == name for t in other.targets): + raised.update(c.value for c in ast.walk(other.value) + if isinstance(c, ast.Constant) + and isinstance(c.value, str)) + return raised + + +@pytest.mark.parametrize("kind", sorted(self_defence.KINDS)) +def test_every_kind_is_actually_raised(kind): + """A vocabulary nothing raises is a vocabulary. + + Parametrised over the whole map rather than a hand-written list, so a kind + added and never wired fails here instead of sitting quietly. That is how + three of them sat: defined, explained, given a severity, and unreachable. + """ + assert kind in _raised_kinds(), ( + f"{kind} is defined in self_defence.KINDS and nothing calls " + f"_platform_event with it" + ) + + +def test_nothing_raises_a_kind_that_does_not_exist(): + """The other direction. `describe()` files an unknown kind under MEDIUM + rather than dropping it, which is the right runtime behaviour and would + hide a typo for ever.""" + invented = sorted(k for k in _raised_kinds() + if k.isupper() and "_" in k and k not in self_defence.KINDS) + assert not invented, f"raised but never defined: {invented}" def test_there_is_somewhere_to_read_them():