From 7901aaa16bf05bdda20737ea07eda9108fe68d44 Mon Sep 17 00:00:00 2001 From: Hashir Ashraf <142582802+Hashir-Ashraf-Awan@users.noreply.github.com> Date: Thu, 10 Sep 2026 16:39:36 +0500 Subject: [PATCH 1/3] feat(auth): TOTP second factor for the local provider Adds optional TOTP (RFC 6238) on top of the local email/password provider, opt-in per account through ADMIN_TOTP_SECRET and USER_TOTP_SECRET. SSO already covered MFA by delegating to the identity provider (docs/OIDC.md); this closes the gap for deployments that authenticate locally. Verification is in-tree and dependency-free: HOTP is a truncated HMAC and base32 is a 32-character alphabet, so an OTP library would add supply-chain surface to the one code path that exists to raise the cost of a compromise. HMAC-SHA-1 is RFC 6238's default and the only algorithm authenticator apps interoperate on for a bare otpauth URI. Secrets are env vars rather than enrolled state: the chart and the image both run on a read-only filesystem, and a second factor that silently degraded when the data dir was unwritable would be worse than none. A value that is not base32 is an AuthConfigError, so a typo stops login with a 503 naming the variable instead of quietly dropping the factor or rejecting every correct code. Notable properties: - The password alone never creates a session, and an accepted code cannot be replayed inside its 90-second window (RFC 6238 5.2). Both are asserted in tests/security/mfa-second-factor.test.ts, now control 1.6 in docs/SECURITY.md. - Replying "code required" is not the enumeration oracle control 1.5 removes: it is reachable only with a correct password, and without MFA that same request would have returned a session. - Being asked for a code costs no rate-limit budget; a wrong code spends both buckets. login_client allows five failures per five minutes, so charging the prompt would cap legitimate users at five logins per window. - NEXT_PUBLIC_AUTH_PROVIDER=oidc changes what the login page renders, not what POST /api/auth/login accepts. An OIDC deployment that still sets ADMIN_PASSWORD keeps a route that never reaches the issuer; these variables are honoured in that mode too, which is how it is closed. Documented rather than assumed away. - The chart carries the secret in its Secret and references it from the pod, so it never lands in the Deployment spec the way extraEnv would. Both refs stay optional even in strict mode, so an unasked-for factor cannot block startup. Chart version bumped to 0.1.63 per the packaged-file rule (#167); 0.1.62 is already released. Coverage is 100% on all four touched source files. Co-Authored-By: Claude Opus 5 (1M context) --- .env.example | 16 ++ README.md | 1 + charts/libredb-studio/Chart.yaml | 2 +- charts/libredb-studio/README.md | 26 +- .../libredb-studio/templates/deployment.yaml | 20 ++ charts/libredb-studio/templates/secret.yaml | 6 + charts/libredb-studio/values.schema.json | 10 + charts/libredb-studio/values.yaml | 12 + docs/MFA.md | 233 ++++++++++++++++++ docs/SECURITY.md | 21 ++ .../helm-charts/libredb-studio/Chart.yaml | 2 +- operator/helm-charts/libredb-studio/README.md | 26 +- .../libredb-studio/templates/deployment.yaml | 20 ++ .../libredb-studio/templates/secret.yaml | 6 + .../libredb-studio/values.schema.json | 10 + .../helm-charts/libredb-studio/values.yaml | 12 + scripts/security-check.mjs | 1 + src/app/api/auth/login/route.ts | 66 ++++- src/app/login/login-form.tsx | 84 ++++++- src/lib/audit.ts | 9 + src/lib/local-auth.ts | 44 +++- src/lib/totp.ts | 177 +++++++++++++ tests/api/auth/login.test.ts | 161 +++++++++++- tests/components/LoginPage.test.tsx | 165 +++++++++++++ tests/helpers/rfc6238.ts | 38 +++ tests/security/mfa-second-factor.test.ts | 150 +++++++++++ tests/security/route-auth.test.ts | 1 + tests/unit/helm-chart-totp.test.ts | 133 ++++++++++ tests/unit/lib/local-auth.test.ts | 85 +++++++ tests/unit/lib/totp.test.ts | 126 ++++++++++ 30 files changed, 1647 insertions(+), 16 deletions(-) create mode 100644 docs/MFA.md create mode 100644 src/lib/totp.ts create mode 100644 tests/helpers/rfc6238.ts create mode 100644 tests/security/mfa-second-factor.test.ts create mode 100644 tests/unit/helm-chart-totp.test.ts create mode 100644 tests/unit/lib/totp.test.ts diff --git a/.env.example b/.env.example index 07a7fc6b6..fad56b98d 100644 --- a/.env.example +++ b/.env.example @@ -60,6 +60,22 @@ ADMIN_PASSWORD=your_secure_admin_password USER_EMAIL=user@libredb.org USER_PASSWORD=your_secure_user_password +# TWO-FACTOR AUTHENTICATION (TOTP) — optional, local provider only +# ============================================ +# Base32 secret (RFC 4648: A-Z and 2-7). When set, that account must present a +# 6-digit code from an authenticator app after its password. Opt-in per account: +# set one, both, or neither. Under NEXT_PUBLIC_AUTH_PROVIDER=oidc the login page +# shows no password form, so MFA belongs at the identity provider (docs/OIDC.md) +# — but POST /api/auth/login stays reachable whenever a password is ALSO +# configured, and these secrets guard that route in every mode. +# +# Generate one, then enrol it in your app of choice: +# openssl rand 20 | base32 | tr -d '=' # 160-bit secret, per RFC 4226 +# A value that is not valid base32 stops login with a clear 503 rather than +# silently disabling the second factor. Blank the variable to turn MFA off. +# ADMIN_TOTP_SECRET=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP +# USER_TOTP_SECRET= + # JWT Secret for session management (min 32 characters) # Generate with: openssl rand -base64 32 # A shorter value stops the server at startup (exit code 1) instead of booting diff --git a/README.md b/README.md index c36e3cb19..8d01f5a2b 100644 --- a/README.md +++ b/README.md @@ -918,6 +918,7 @@ extraEnvFrom: | [Local models](docs/llms/README.md) | Which local model can actually drive an agent run, measured across three workflows, one page per model | | [Agent Runtime](docs/AGENT.md) | Agent behaviour, bounds, deployment and known limitations | | [OIDC SSO](docs/OIDC.md) | SSO setup (Auth0, Keycloak, Okta, Azure AD, Zitadel, Google) + subsystem internals & security model | +| [Two-Factor Auth](docs/MFA.md) | TOTP on the local provider — generating a secret, enrolling an app, Docker/Helm wiring, and what it does not cover | | [Theming Guide](docs/ui/theming.md) | CSS theming, dark mode, and styling customization | | [Login Page](docs/ui/login-page.md) | Login page layout, OIDC/local modes, and design system | | [Editor Docs](docs/editor/) | SQL editor internals — completion, performance, query optimization | diff --git a/charts/libredb-studio/Chart.yaml b/charts/libredb-studio/Chart.yaml index c9121f2d9..0deb48228 100644 --- a/charts/libredb-studio/Chart.yaml +++ b/charts/libredb-studio/Chart.yaml @@ -2,7 +2,7 @@ apiVersion: v2 name: libredb-studio description: Web-based SQL IDE for cloud-native teams supporting sixteen engines - PostgreSQL, MySQL, SQLite, DuckDB, Oracle, SQL Server, MongoDB, Redis, Couchbase, ClickHouse, Apache Druid, Elasticsearch, OpenSearch, Apache Trino, Apache Cassandra and libSQL type: application -version: 0.1.62 +version: 0.1.63 appVersion: "0.15.0" kubeVersion: ">=1.26.0-0" home: https://github.com/libredb/libredb-studio diff --git a/charts/libredb-studio/README.md b/charts/libredb-studio/README.md index ae91bc6f9..c9051dc09 100644 --- a/charts/libredb-studio/README.md +++ b/charts/libredb-studio/README.md @@ -40,7 +40,7 @@ helm install libredb libredb/libredb-studio \ ```bash helm install libredb oci://ghcr.io/libredb/charts/libredb-studio \ - --version 0.1.62 \ + --version 0.1.63 \ --set secrets.jwtSecret=$(openssl rand -base64 32) \ --set secrets.adminPassword=MyAdmin123 ``` @@ -147,6 +147,26 @@ browser still speaks `https`, so it accepts the cookie. `false` is only for the the browser's own connection is cleartext - and it means session cookies travel in cleartext, so keep it to a trusted network. `true` forces the flag on. +## Two-Factor Authentication (TOTP) + +Optional, local-provider only, and opt-in per account. Set a base32 secret and that account must +present a 6-digit authenticator code after its password: + +```bash +helm upgrade --install libredb libredb/libredb-studio --set secrets.adminPassword=MyAdmin123 --set secrets.adminTotpSecret=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP +``` + +The value travels in the chart's Secret and is referenced from the pod, so it never appears in the +Deployment spec - which is why `extraEnv` is the wrong tool for it. Both `ADMIN_TOTP_SECRET` and +`USER_TOTP_SECRET` refs are always optional, including in strict mode, so a second factor nobody +asked for can never keep the pod from starting. `values.schema.json` rejects a value that is not +base32 at install time rather than leaving it to fail at the login screen. + +Under `authProvider=oidc` the login page shows no password form and MFA belongs to the identity +provider - but `POST /api/auth/login` stays reachable whenever `secrets.adminPassword` is also set, +and this guards that route in every mode. Full setup, enrolment and recovery: +[`docs/MFA.md`](../../docs/MFA.md). + ## OIDC SSO ```bash @@ -542,7 +562,7 @@ helm install libredb libredb/libredb-studio \ Your external secret is referenced with these keys (customizable via `secrets.existingSecretKeys`): - `jwt-secret`, `admin-password` — required in strict mode (the pod waits for them); in zero-config mode missing ones are generated at first start -- Optional: `admin-email`, `user-email`, `user-password` (the non-admin account exists only when `user-password` is set), `llm-api-key`, `oidc-client-id`, `oidc-client-secret`, `storage-postgres-url` +- Optional: `admin-email`, `user-email`, `user-password` (the non-admin account exists only when `user-password` is set), `admin-totp-secret`, `user-totp-secret`, `llm-api-key`, `oidc-client-id`, `oidc-client-secret`, `storage-postgres-url` ## Upgrading @@ -586,6 +606,8 @@ helm uninstall libredb | `secrets.adminPassword` | Admin password | `""` | | `secrets.userEmail` | User email | `user@libredb.org` | | `secrets.userPassword` | User password (optional; enables the non-admin account) | `""` | +| `secrets.adminTotpSecret` | Base32 TOTP secret for the admin account (optional second factor) | `""` | +| `secrets.userTotpSecret` | Base32 TOTP secret for the user account (optional second factor) | `""` | | `secrets.existingSecret` | Use existing Secret | `""` | | `config.bindAddress` | Container bind address (`HOSTNAME`): empty lets the image resolve one, preferring a verified dual-stack `::`; `::` forces it; `0.0.0.0` pins IPv4 | `""` | | `config.storageProvider` | Storage: local, sqlite, postgres | `local` | diff --git a/charts/libredb-studio/templates/deployment.yaml b/charts/libredb-studio/templates/deployment.yaml index 506227cdd..40ef3391c 100644 --- a/charts/libredb-studio/templates/deployment.yaml +++ b/charts/libredb-studio/templates/deployment.yaml @@ -181,6 +181,26 @@ spec: key: {{ .Values.secrets.existingSecretKeys.userPassword }} optional: true {{- end }} + {{- /* TOTP second factor. Always optional: MFA is opt-in per account, so a missing + key must never keep the pod from starting, and the app treats an absent value + as "no second factor". Keyed separately from the passwords rather than nested + under them so an operator can rotate a secret without touching a credential. */}} + {{- if or .Values.secrets.adminTotpSecret .Values.secrets.existingSecret }} + - name: ADMIN_TOTP_SECRET + valueFrom: + secretKeyRef: + name: {{ include "libredb-studio.secretName" . }} + key: {{ .Values.secrets.existingSecretKeys.adminTotpSecret }} + optional: true + {{- end }} + {{- if or .Values.secrets.userTotpSecret .Values.secrets.existingSecret }} + - name: USER_TOTP_SECRET + valueFrom: + secretKeyRef: + name: {{ include "libredb-studio.secretName" . }} + key: {{ .Values.secrets.existingSecretKeys.userTotpSecret }} + optional: true + {{- end }} {{- if or .Values.secrets.llmApiKey .Values.secrets.existingSecret }} - name: LLM_API_KEY valueFrom: diff --git a/charts/libredb-studio/templates/secret.yaml b/charts/libredb-studio/templates/secret.yaml index feaed82e8..d0b4dee51 100644 --- a/charts/libredb-studio/templates/secret.yaml +++ b/charts/libredb-studio/templates/secret.yaml @@ -26,6 +26,12 @@ data: {{ .Values.secrets.existingSecretKeys.userEmail }}: {{ .Values.secrets.userEmail | b64enc | quote }} {{ .Values.secrets.existingSecretKeys.userPassword }}: {{ .Values.secrets.userPassword | b64enc | quote }} {{- end }} + {{- if .Values.secrets.adminTotpSecret }} + {{ .Values.secrets.existingSecretKeys.adminTotpSecret }}: {{ .Values.secrets.adminTotpSecret | b64enc | quote }} + {{- end }} + {{- if .Values.secrets.userTotpSecret }} + {{ .Values.secrets.existingSecretKeys.userTotpSecret }}: {{ .Values.secrets.userTotpSecret | b64enc | quote }} + {{- end }} {{- if .Values.secrets.llmApiKey }} {{ .Values.secrets.existingSecretKeys.llmApiKey }}: {{ .Values.secrets.llmApiKey | b64enc | quote }} {{- end }} diff --git a/charts/libredb-studio/values.schema.json b/charts/libredb-studio/values.schema.json index 3c94e465c..f7358bb24 100644 --- a/charts/libredb-studio/values.schema.json +++ b/charts/libredb-studio/values.schema.json @@ -103,6 +103,16 @@ "type": "string", "description": "User account password" }, + "adminTotpSecret": { + "type": "string", + "pattern": "^$|^[A-Za-z2-7][A-Za-z2-7 =-]*$", + "description": "Admin TOTP secret: empty (no second factor) or base32 (RFC 4648: A-Z and 2-7, spacing and = padding allowed)" + }, + "userTotpSecret": { + "type": "string", + "pattern": "^$|^[A-Za-z2-7][A-Za-z2-7 =-]*$", + "description": "User TOTP secret: empty (no second factor) or base32 (RFC 4648: A-Z and 2-7, spacing and = padding allowed)" + }, "llmApiKey": { "type": "string", "description": "LLM API key" diff --git a/charts/libredb-studio/values.yaml b/charts/libredb-studio/values.yaml index 472ecf7d4..81e86618d 100644 --- a/charts/libredb-studio/values.yaml +++ b/charts/libredb-studio/values.yaml @@ -56,6 +56,8 @@ secrets: adminPassword: admin-password userEmail: user-email userPassword: user-password + adminTotpSecret: admin-totp-secret + userTotpSecret: user-totp-secret llmApiKey: llm-api-key oidcClientId: oidc-client-id oidcClientSecret: oidc-client-secret @@ -75,6 +77,16 @@ secrets: # -- Regular user account password. Optional: the non-admin account exists only when set; # it is never generated. userPassword: "" + # -- Base32 TOTP secret for the admin account (RFC 4648: A-Z and 2-7). Optional and + # local-provider only: when set, admin login requires a 6-digit authenticator code after the + # password. A value that is not base32 stops login with a clear 503 rather than silently + # dropping the second factor. Under config.authProvider=oidc the login page shows no password + # form and MFA belongs to the identity provider, but this still guards POST /api/auth/login + # whenever a password is also set. Generate one with: openssl rand 20 | base32 | tr -d '=' + adminTotpSecret: "" + # -- Base32 TOTP secret for the regular user account. Same rules as adminTotpSecret; inert + # unless userPassword is also set, since without it there is no user account to protect. + userTotpSecret: "" # -- LLM API key (optional, for AI features) llmApiKey: "" # -- OIDC client ID (required when authProvider=oidc) diff --git a/docs/MFA.md b/docs/MFA.md new file mode 100644 index 000000000..428df8525 --- /dev/null +++ b/docs/MFA.md @@ -0,0 +1,233 @@ +# Two-Factor Authentication (TOTP) — LibreDB Studio + +LibreDB Studio can require a time-based one-time password (TOTP) after the password on the **local** +auth provider. It is opt-in per account, configured entirely through environment variables, and +verified against RFC 6238 — so any standard authenticator app works: Google Authenticator, Authy, +1Password, Bitwarden, Aegis, KeePassXC, and the rest. + +> **Using SSO instead?** For most teams that is the better answer: under +> `NEXT_PUBLIC_AUTH_PROVIDER=oidc` the login page shows no password form, and your identity +> provider already enforces MFA, passkeys, device trust and conditional access — Studio consumes +> the result. See [`docs/OIDC.md`](OIDC.md). Note that switching to OIDC does not *disable* +> `POST /api/auth/login`; see [Running both](#running-both) below. + +--- + +## Table of Contents + +- [Quick Start](#quick-start) +- [Configuration Reference](#configuration-reference) + - [Running both](#running-both) +- [Deploying with Docker](#deploying-with-docker) +- [Deploying with Helm](#deploying-with-helm) +- [How it works](#how-it-works) +- [Rate limiting and lockout](#rate-limiting-and-lockout) +- [Troubleshooting](#troubleshooting) +- [What this does and does not protect](#what-this-does-and-does-not-protect) + +--- + +## Quick Start + +### 1. Generate a secret + +A TOTP secret is base32 (RFC 4648: the letters `A`–`Z` and the digits `2`–`7`). 160 bits is what +RFC 4226 recommends: + +```bash +openssl rand 20 | base32 | tr -d '=' +# => JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP +``` + +`base32` comes with GNU coreutils. On a machine without it, any authenticator app can generate a +secret for you — create a manual entry and copy the key it shows. + +### 2. Set it on the account + +```env +NEXT_PUBLIC_AUTH_PROVIDER=local +ADMIN_EMAIL=admin@libredb.org +ADMIN_PASSWORD=your_secure_admin_password +ADMIN_TOTP_SECRET=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP +``` + +Restart the server. The variable is read per login attempt, so nothing is cached across a restart. + +### 3. Enrol the secret in your authenticator + +Add a manual (key-based) entry with: + +| Field | Value | +|---|---| +| Account | your admin email | +| Key | the secret from step 1 | +| Type | Time-based | +| Digits | 6 | +| Period | 30 seconds | +| Algorithm | SHA-1 | + +Those are every app's defaults, so in practice you only paste the key. + +Prefer to scan a QR code? Build the standard URI yourself and render it with any offline QR tool — +Studio does not mint one, because doing so would mean the server handing the shared secret back +over HTTP after startup: + +``` +otpauth://totp/LibreDB%20Studio:admin@libredb.org?secret=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP&issuer=LibreDB%20Studio +``` + +### 4. Sign in + +Enter your email and password as usual. The form then asks for the 6-digit code, and the session is +created only once that code verifies. + +--- + +## Configuration Reference + +| Variable | Required | Description | +|---|---|---| +| `ADMIN_TOTP_SECRET` | No | Base32 secret for the admin account. Absent or empty = no second factor. | +| `USER_TOTP_SECRET` | No | Base32 secret for the optional non-admin account. Inert unless `USER_PASSWORD` is also set — with no password there is no user account to protect. | + +**Formatting is forgiving, content is not.** Lowercase, spaces, hyphens and `=` padding are all +normalized away, so you can paste a secret exactly as your password manager displays it. A value +containing anything outside the base32 alphabet is a **misconfiguration, not a disabled factor**: +login stops with a `503` naming the offending variable, rather than silently letting the password +through or rejecting every correct code. To turn MFA off, blank or unset the variable. + +Each account is independent — protect the admin and leave an automation-owned user account on a +password alone if that is what you need. + +### Running both + +`NEXT_PUBLIC_AUTH_PROVIDER=oidc` changes what the login page renders; it does not disable +`POST /api/auth/login`. If a deployment sets `ADMIN_PASSWORD` *and* runs OIDC, that route remains a +working way in — one that never touches your identity provider, and so never meets the MFA policy +you configured there. Two ways to close it, and they compose: + +- Leave `ADMIN_PASSWORD` unset under OIDC. Nothing generates it in OIDC mode, and with no password + the route can only answer `503`. +- Set `ADMIN_TOTP_SECRET` anyway. These variables are honoured in every mode, so the local route + keeps a second factor even when the intended path is SSO. + +--- + +## Deploying with Docker + +```bash +docker run -d -p 3000:3000 \ + -e JWT_SECRET="$(openssl rand -base64 32)" \ + -e ADMIN_PASSWORD=your_secure_admin_password \ + -e ADMIN_TOTP_SECRET=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP \ + ghcr.io/libredb/libredb-studio:latest +``` + +Prefer a file or a secret store over an inline `-e` for the secret itself: anything on the command +line is visible to `docker inspect` and to the shell history of whoever ran it. + +--- + +## Deploying with Helm + +The chart carries the secret in its Kubernetes `Secret` and references it from the pod, so the value +never appears in the Deployment spec: + +```bash +helm install libredb-studio oci://ghcr.io/libredb/charts/libredb-studio \ + --set secrets.adminPassword=MyAdmin123 \ + --set secrets.adminTotpSecret=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP +``` + +Bringing your own Secret works too — add the keys `admin-totp-secret` and `user-totp-secret` +(rename them through `secrets.existingSecretKeys`) to the Secret named by `secrets.existingSecret`. +Both references are optional, so an existing Secret that predates this feature keeps working +untouched. + +Do not use `extraEnv` for this. It writes the literal secret into the Deployment's pod spec, where +anyone with `get deployments` can read it — a wider audience than the password it protects. + +--- + +## How it works + +``` +Browser → POST /api/auth/login {email, password} +Server → password verified (constant time) → account has a TOTP secret +Server → 401 {mfaRequired: true} ← no session is created +Browser → POST /api/auth/login {email, password, totp} +Server → code verified → step marked spent → JWT session cookie +``` + +| Parameter | Value | +|---|---| +| Algorithm | HMAC-SHA-1 (RFC 6238 §1.2 default; the only algorithm apps interoperate on for a bare `otpauth` URI) | +| Digits | 6 | +| Period | 30 seconds | +| Accepted skew | ±1 step, so a code stays usable for up to 90 seconds | +| Replay | An accepted `(account, step)` pair is spent and cannot be reused (RFC 6238 §5.2) | + +Verification lives in [`src/lib/totp.ts`](../src/lib/totp.ts) with no third-party dependency: HOTP is +a truncated HMAC and base32 is a 32-character alphabet, and the one code path that exists to raise +the cost of a compromise is a poor place to add supply-chain surface. + +SHA-1 here is correct and must not be "upgraded". The construction's security rests on HMAC, which +does not depend on the collision resistance SHA-1 lost, and changing it would break every +authenticator app. + +--- + +## Rate limiting and lockout + +Being **asked** for a code costs nothing — it is the first half of a two-request flow, not a failed +attempt, so ordinary logins never eat into your budget. + +Submitting a **wrong** code does count, against both login buckets: `RATE_LIMIT_LOGIN_MAX` (5 per +5 minutes, per client address) and `RATE_LIMIT_LOGIN_ACCOUNT_MAX` (20 per 5 minutes, per account). +Guessing a 6-digit code is therefore bounded to a few dozen tries per window against roughly a +million values. + +Locked out of your own account? The secret is an environment variable, so recovery is the same as +for a lost password: blank `ADMIN_TOTP_SECRET` and restart. There are no recovery codes, and none +are needed — whoever can restart the server already holds the stronger credential. + +--- + +## Troubleshooting + +**"Invalid authentication code" for every code.** Almost always clock drift on the server: TOTP is a +function of the current time, and the accepted window is ±30 seconds. Check the host clock +(`timedatectl status`, or the node's NTP state) rather than re-enrolling. + +**"Invalid authentication code" for a code that just worked.** Each code is single-use. Wait for the +next one rather than resubmitting the same digits. + +**A 503 naming `ADMIN_TOTP_SECRET` or `USER_TOTP_SECRET`.** The value is not valid base32. Copy it +again from the authenticator app — `0`, `1`, `8` and `9` are not in the alphabet, and a secret +containing them was mistyped. + +**The code field never appears.** The account has no secret configured, or the deployment is running +`NEXT_PUBLIC_AUTH_PROVIDER=oidc`, where these variables are ignored. + +**Nothing happens after a correct code.** Check for a `429`: the client bucket may have tripped from +earlier wrong codes. It clears on its own within the window. + +--- + +## What this does and does not protect + +It defeats a password that leaked on its own — reused from another breach, read out of a +`docker inspect`, or shoulder-surfed. That is the threat this control exists for, and it is the +common one. + +It is not a substitute for the identity provider. There is no passkey or WebAuthn support here, no +per-user enrolment, no recovery codes, and no device management — one shared secret per account, +provisioned by whoever runs the server. Teams that need more should run +[OIDC](OIDC.md) and enforce it upstream. + +One deployment note: the spent-code set lives in the application process, like the login rate-limit +counters. Above one replica each process enforces its own view, so a captured code can be replayed +once per replica inside its 90-second window. The chart defaults to `replicaCount: 1`. + +Stated as a control, with its verifying test, in +[`docs/SECURITY.md`](SECURITY.md#controls) (row 1.6). diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 538442637..210af069d 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -36,6 +36,7 @@ Two consequences worth stating before the table: | 1.3 | State-changing requests are checked against the deployment's own origin | Implemented | [`src/lib/api/origin-check.ts`](../src/lib/api/origin-check.ts), [`src/proxy.ts`](../src/proxy.ts) | [`tests/security/csrf-origin.test.ts`](../tests/security/csrf-origin.test.ts) | | 1.4 | Authentication transitions and denials are audited | Partial | [`src/lib/audit.ts`](../src/lib/audit.ts), [`src/lib/api/require-session.ts`](../src/lib/api/require-session.ts) | [`tests/security/auth-audit.test.ts`](../tests/security/auth-audit.test.ts) | | 1.5 | The login comparison is constant time and its failure response is uniform | Implemented | [`src/lib/auth-compare.ts`](../src/lib/auth-compare.ts), [`src/app/api/auth/login/route.ts`](../src/app/api/auth/login/route.ts) | [`tests/security/login-enumeration.test.ts`](../tests/security/login-enumeration.test.ts) | +| 1.6 | A local account with a TOTP secret configured cannot be signed in with its password alone, and an accepted code cannot be used twice | Implemented | [`src/lib/totp.ts`](../src/lib/totp.ts), [`src/lib/local-auth.ts`](../src/lib/local-auth.ts), [`src/app/api/auth/login/route.ts`](../src/app/api/auth/login/route.ts) | [`tests/security/mfa-second-factor.test.ts`](../tests/security/mfa-second-factor.test.ts) | | 2.1 | Secrets, dependencies and the container image are scanned in CI | Implemented | [`.github/workflows/security-scan.yml`](../.github/workflows/security-scan.yml), [`.gitleaks.toml`](../.gitleaks.toml), [`.trivyignore.yaml`](../.trivyignore.yaml) | [`tests/unit/security-scan-workflow.test.ts`](../tests/unit/security-scan-workflow.test.ts), [`tests/unit/gitleaks-config.test.ts`](../tests/unit/gitleaks-config.test.ts), [`tests/unit/trivyignore-policy.test.ts`](../tests/unit/trivyignore-policy.test.ts) | | 2.2 | An SBOM is published with every release | Implemented | [`.github/workflows/release-artifacts.yml`](../.github/workflows/release-artifacts.yml) | [`tests/unit/release-sbom.test.ts`](../tests/unit/release-sbom.test.ts) | | 2.3 | No TypeScript error is suppressed at build time | Implemented | [`next.config.ts`](../next.config.ts) | [`tests/unit/next-config-typecheck.test.ts`](../tests/unit/next-config-typecheck.test.ts) | @@ -142,6 +143,26 @@ per replica; multi-replica deployments should enforce the same budgets at the in in-handler admin checks and the middleware's `/admin` redirect return their denial with no audit line. Tracked in [`docs/BACKLOG.md`](./BACKLOG.md), entry H12. +**1.6.** Opt-in: a second factor exists for an account exactly when `ADMIN_TOTP_SECRET` / +`USER_TOTP_SECRET` is set, so the row claims nothing about a deployment that sets neither. +Verification is RFC 6238 with HMAC-SHA-1, six digits, a 30-second step and one step of skew either +side. Three things are worth stating precisely. + +It guards `POST /api/auth/login`, and that route is **not** disabled by +`NEXT_PUBLIC_AUTH_PROVIDER=oidc` — the provider setting changes what the login page renders, not +what the API accepts. A deployment that runs OIDC while still setting `ADMIN_PASSWORD` therefore +keeps a way in that never reaches the issuer, and so never meets the MFA policy configured there; +these variables are honoured in that mode too, which is how it is closed +([`docs/MFA.md`](./MFA.md), [`docs/OIDC.md`](./OIDC.md)). + +Replies distinguish "code required" from "invalid code", which is not the enumeration oracle 1.5 +removes: both are reachable only with a correct password, and without MFA that same request would +have returned a session. + +The spent-code set that makes an accepted code single-use lives in the application process, like +the counters in 1.2 — with more than one replica each process enforces its own view, so a captured +code can be replayed once per replica inside its 90-second window. + **3.1.** Applies to `STORAGE_PROVIDER=sqlite` and `postgres` only. Seven fields are encrypted; `host`, `port`, `user`, `agentUser`, `database`, `name` and the TLS certificates stay readable so a dump can still be identified. Rotating the key makes stored credentials unreadable — the connection survives, the diff --git a/operator/helm-charts/libredb-studio/Chart.yaml b/operator/helm-charts/libredb-studio/Chart.yaml index c9121f2d9..0deb48228 100644 --- a/operator/helm-charts/libredb-studio/Chart.yaml +++ b/operator/helm-charts/libredb-studio/Chart.yaml @@ -2,7 +2,7 @@ apiVersion: v2 name: libredb-studio description: Web-based SQL IDE for cloud-native teams supporting sixteen engines - PostgreSQL, MySQL, SQLite, DuckDB, Oracle, SQL Server, MongoDB, Redis, Couchbase, ClickHouse, Apache Druid, Elasticsearch, OpenSearch, Apache Trino, Apache Cassandra and libSQL type: application -version: 0.1.62 +version: 0.1.63 appVersion: "0.15.0" kubeVersion: ">=1.26.0-0" home: https://github.com/libredb/libredb-studio diff --git a/operator/helm-charts/libredb-studio/README.md b/operator/helm-charts/libredb-studio/README.md index ae91bc6f9..c9051dc09 100644 --- a/operator/helm-charts/libredb-studio/README.md +++ b/operator/helm-charts/libredb-studio/README.md @@ -40,7 +40,7 @@ helm install libredb libredb/libredb-studio \ ```bash helm install libredb oci://ghcr.io/libredb/charts/libredb-studio \ - --version 0.1.62 \ + --version 0.1.63 \ --set secrets.jwtSecret=$(openssl rand -base64 32) \ --set secrets.adminPassword=MyAdmin123 ``` @@ -147,6 +147,26 @@ browser still speaks `https`, so it accepts the cookie. `false` is only for the the browser's own connection is cleartext - and it means session cookies travel in cleartext, so keep it to a trusted network. `true` forces the flag on. +## Two-Factor Authentication (TOTP) + +Optional, local-provider only, and opt-in per account. Set a base32 secret and that account must +present a 6-digit authenticator code after its password: + +```bash +helm upgrade --install libredb libredb/libredb-studio --set secrets.adminPassword=MyAdmin123 --set secrets.adminTotpSecret=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP +``` + +The value travels in the chart's Secret and is referenced from the pod, so it never appears in the +Deployment spec - which is why `extraEnv` is the wrong tool for it. Both `ADMIN_TOTP_SECRET` and +`USER_TOTP_SECRET` refs are always optional, including in strict mode, so a second factor nobody +asked for can never keep the pod from starting. `values.schema.json` rejects a value that is not +base32 at install time rather than leaving it to fail at the login screen. + +Under `authProvider=oidc` the login page shows no password form and MFA belongs to the identity +provider - but `POST /api/auth/login` stays reachable whenever `secrets.adminPassword` is also set, +and this guards that route in every mode. Full setup, enrolment and recovery: +[`docs/MFA.md`](../../docs/MFA.md). + ## OIDC SSO ```bash @@ -542,7 +562,7 @@ helm install libredb libredb/libredb-studio \ Your external secret is referenced with these keys (customizable via `secrets.existingSecretKeys`): - `jwt-secret`, `admin-password` — required in strict mode (the pod waits for them); in zero-config mode missing ones are generated at first start -- Optional: `admin-email`, `user-email`, `user-password` (the non-admin account exists only when `user-password` is set), `llm-api-key`, `oidc-client-id`, `oidc-client-secret`, `storage-postgres-url` +- Optional: `admin-email`, `user-email`, `user-password` (the non-admin account exists only when `user-password` is set), `admin-totp-secret`, `user-totp-secret`, `llm-api-key`, `oidc-client-id`, `oidc-client-secret`, `storage-postgres-url` ## Upgrading @@ -586,6 +606,8 @@ helm uninstall libredb | `secrets.adminPassword` | Admin password | `""` | | `secrets.userEmail` | User email | `user@libredb.org` | | `secrets.userPassword` | User password (optional; enables the non-admin account) | `""` | +| `secrets.adminTotpSecret` | Base32 TOTP secret for the admin account (optional second factor) | `""` | +| `secrets.userTotpSecret` | Base32 TOTP secret for the user account (optional second factor) | `""` | | `secrets.existingSecret` | Use existing Secret | `""` | | `config.bindAddress` | Container bind address (`HOSTNAME`): empty lets the image resolve one, preferring a verified dual-stack `::`; `::` forces it; `0.0.0.0` pins IPv4 | `""` | | `config.storageProvider` | Storage: local, sqlite, postgres | `local` | diff --git a/operator/helm-charts/libredb-studio/templates/deployment.yaml b/operator/helm-charts/libredb-studio/templates/deployment.yaml index 506227cdd..40ef3391c 100644 --- a/operator/helm-charts/libredb-studio/templates/deployment.yaml +++ b/operator/helm-charts/libredb-studio/templates/deployment.yaml @@ -181,6 +181,26 @@ spec: key: {{ .Values.secrets.existingSecretKeys.userPassword }} optional: true {{- end }} + {{- /* TOTP second factor. Always optional: MFA is opt-in per account, so a missing + key must never keep the pod from starting, and the app treats an absent value + as "no second factor". Keyed separately from the passwords rather than nested + under them so an operator can rotate a secret without touching a credential. */}} + {{- if or .Values.secrets.adminTotpSecret .Values.secrets.existingSecret }} + - name: ADMIN_TOTP_SECRET + valueFrom: + secretKeyRef: + name: {{ include "libredb-studio.secretName" . }} + key: {{ .Values.secrets.existingSecretKeys.adminTotpSecret }} + optional: true + {{- end }} + {{- if or .Values.secrets.userTotpSecret .Values.secrets.existingSecret }} + - name: USER_TOTP_SECRET + valueFrom: + secretKeyRef: + name: {{ include "libredb-studio.secretName" . }} + key: {{ .Values.secrets.existingSecretKeys.userTotpSecret }} + optional: true + {{- end }} {{- if or .Values.secrets.llmApiKey .Values.secrets.existingSecret }} - name: LLM_API_KEY valueFrom: diff --git a/operator/helm-charts/libredb-studio/templates/secret.yaml b/operator/helm-charts/libredb-studio/templates/secret.yaml index feaed82e8..d0b4dee51 100644 --- a/operator/helm-charts/libredb-studio/templates/secret.yaml +++ b/operator/helm-charts/libredb-studio/templates/secret.yaml @@ -26,6 +26,12 @@ data: {{ .Values.secrets.existingSecretKeys.userEmail }}: {{ .Values.secrets.userEmail | b64enc | quote }} {{ .Values.secrets.existingSecretKeys.userPassword }}: {{ .Values.secrets.userPassword | b64enc | quote }} {{- end }} + {{- if .Values.secrets.adminTotpSecret }} + {{ .Values.secrets.existingSecretKeys.adminTotpSecret }}: {{ .Values.secrets.adminTotpSecret | b64enc | quote }} + {{- end }} + {{- if .Values.secrets.userTotpSecret }} + {{ .Values.secrets.existingSecretKeys.userTotpSecret }}: {{ .Values.secrets.userTotpSecret | b64enc | quote }} + {{- end }} {{- if .Values.secrets.llmApiKey }} {{ .Values.secrets.existingSecretKeys.llmApiKey }}: {{ .Values.secrets.llmApiKey | b64enc | quote }} {{- end }} diff --git a/operator/helm-charts/libredb-studio/values.schema.json b/operator/helm-charts/libredb-studio/values.schema.json index 3c94e465c..f7358bb24 100644 --- a/operator/helm-charts/libredb-studio/values.schema.json +++ b/operator/helm-charts/libredb-studio/values.schema.json @@ -103,6 +103,16 @@ "type": "string", "description": "User account password" }, + "adminTotpSecret": { + "type": "string", + "pattern": "^$|^[A-Za-z2-7][A-Za-z2-7 =-]*$", + "description": "Admin TOTP secret: empty (no second factor) or base32 (RFC 4648: A-Z and 2-7, spacing and = padding allowed)" + }, + "userTotpSecret": { + "type": "string", + "pattern": "^$|^[A-Za-z2-7][A-Za-z2-7 =-]*$", + "description": "User TOTP secret: empty (no second factor) or base32 (RFC 4648: A-Z and 2-7, spacing and = padding allowed)" + }, "llmApiKey": { "type": "string", "description": "LLM API key" diff --git a/operator/helm-charts/libredb-studio/values.yaml b/operator/helm-charts/libredb-studio/values.yaml index 472ecf7d4..81e86618d 100644 --- a/operator/helm-charts/libredb-studio/values.yaml +++ b/operator/helm-charts/libredb-studio/values.yaml @@ -56,6 +56,8 @@ secrets: adminPassword: admin-password userEmail: user-email userPassword: user-password + adminTotpSecret: admin-totp-secret + userTotpSecret: user-totp-secret llmApiKey: llm-api-key oidcClientId: oidc-client-id oidcClientSecret: oidc-client-secret @@ -75,6 +77,16 @@ secrets: # -- Regular user account password. Optional: the non-admin account exists only when set; # it is never generated. userPassword: "" + # -- Base32 TOTP secret for the admin account (RFC 4648: A-Z and 2-7). Optional and + # local-provider only: when set, admin login requires a 6-digit authenticator code after the + # password. A value that is not base32 stops login with a clear 503 rather than silently + # dropping the second factor. Under config.authProvider=oidc the login page shows no password + # form and MFA belongs to the identity provider, but this still guards POST /api/auth/login + # whenever a password is also set. Generate one with: openssl rand 20 | base32 | tr -d '=' + adminTotpSecret: "" + # -- Base32 TOTP secret for the regular user account. Same rules as adminTotpSecret; inert + # unless userPassword is also set, since without it there is no user account to protect. + userTotpSecret: "" # -- LLM API key (optional, for AI features) llmApiKey: "" # -- OIDC client ID (required when authProvider=oidc) diff --git a/scripts/security-check.mjs b/scripts/security-check.mjs index 5c4ffae7a..703ffbd2a 100644 --- a/scripts/security-check.mjs +++ b/scripts/security-check.mjs @@ -53,6 +53,7 @@ export const PROGRAMME_CONTROL_IDS = [ "1.3", "1.4", "1.5", + "1.6", "2.1", "2.2", "2.3", diff --git a/src/app/api/auth/login/route.ts b/src/app/api/auth/login/route.ts index 32e2a4473..f595e925e 100644 --- a/src/app/api/auth/login/route.ts +++ b/src/app/api/auth/login/route.ts @@ -12,8 +12,9 @@ import { type RateLimitBucket, } from "@/lib/api/rate-limit"; import { hmacHex, secretsMatch } from "@/lib/auth-compare"; -import { emitAuditEvent, MAX_AUDIT_FIELD_LENGTH } from "@/lib/audit"; +import { emitAuditEvent, MAX_AUDIT_FIELD_LENGTH, type AuditReason } from "@/lib/audit"; import { logger } from "@/lib/logger"; +import { claimTotpStep, verifyTotp } from "@/lib/totp"; const ROUTE = "POST /api/auth/login"; @@ -25,6 +26,11 @@ const ROUTE = "POST /api/auth/login"; */ const DUMMY_PASSWORD = "libredb-dummy-password-never-a-credential"; +// Single-line and module-scoped, matching the auth messages in local-auth.ts and auth-env.ts: +// bun's line coverage under-counts the continuation lines of a wrapped string. +const MFA_REQUIRED_MESSAGE = "Enter the 6-digit code from your authenticator app"; +const MFA_INVALID_MESSAGE = "Invalid authentication code"; + type LoginBucket = Extract; /** @@ -71,8 +77,9 @@ export async function POST(request: NextRequest) { let email: unknown; let password: unknown; + let totp: unknown; try { - ({ email, password } = await request.json()); + ({ email, password, totp } = await request.json()); } catch { // A malformed body is a client error, not a server error, and - like a wrong password - is // a wasted attempt from this address: consume the client bucket so a caller who floods this @@ -104,6 +111,9 @@ export async function POST(request: NextRequest) { // distinguishable response. const submittedEmail = typeof email === "string" ? email : ""; const submittedPassword = typeof password === "string" ? password : ""; + // Same coercion, same reason: a non-string `totp` must read as "no code supplied", never + // reach verifyTotp's string methods and become a 500 that stands out from the uniform 401. + const submittedTotp = typeof totp === "string" ? totp.trim() : ""; // MAX_AUDIT_FIELD_LENGTH, not a locally redeclared copy of the same number: the actor becomes // an AuditEvent field either way, so both truncations must move together. const actor = submittedEmail.slice(0, MAX_AUDIT_FIELD_LENGTH) || "anonymous"; @@ -121,6 +131,58 @@ export async function POST(request: NextRequest) { const passwordMatches = secretsMatch(submittedPassword, candidate); const matched = user && passwordMatches ? user : null; + // Second factor. Reached only once the password already matched, so answering "code required" + // here is not the account-enumeration oracle the uniform 401 below exists to prevent: an + // unknown email and a known email with a wrong password both still take that path, and a + // caller who can reach this branch necessarily holds a working password for the account - + // without MFA configured, that same request would simply have logged them in. + if (matched?.totpSecret) { + const step = submittedTotp ? verifyTotp(matched.totpSecret, submittedTotp) : null; + // claimTotpStep is what makes an accepted code single-use (RFC 6238 §5.2). A replayed code + // verifies but fails to claim, and lands here as an ordinary bad code. + const accepted = step !== null && claimTotpStep(accountKey, step); + + if (!accepted) { + const reason: AuditReason = submittedTotp ? "bad_totp" : "mfa_required"; + // A wrong code is a failed attempt and is charged for: it is the only thing between an + // attacker holding a stolen password and a session, so guessing has to be bounded. Not + // having supplied one yet is NOT a failed attempt - it is the first half of a + // two-request flow, and charging it would spend the whole five-failure client budget on + // five ordinary logins. + if (reason === "bad_totp") { + consumeRateLimit("login_client", clientKey); + consumeRateLimit("login_account", accountKey); + } + // Isolated like every other emit in this route: the 401 below is already decided, and a + // broken audit sink must not turn it into an unrelated 500. The mfa_required branch is + // uncharged and therefore unbounded in volume for whoever holds the password - the same + // property login_success already has, since a successful login resets both buckets. + try { + emitAuditEvent({ + type: "login_failure", + action: "login", + target: ROUTE, + user: matched.email, + result: "failure", + reason, + ip, + }); + } catch (auditError) { + logger.error("Failed to record login_failure audit event", auditError, { route: ROUTE }); + } + // `mfaRequired` tells the client to render the code field. It carries no information the + // caller did not already supply the password to learn (see the note above this block). + return NextResponse.json( + { + success: false, + mfaRequired: true, + message: reason === "bad_totp" ? MFA_INVALID_MESSAGE : MFA_REQUIRED_MESSAGE, + }, + { status: 401 }, + ); + } + } + if (matched) { await login(matched.role, matched.email); resetRateLimit("login_client", clientKey); diff --git a/src/app/login/login-form.tsx b/src/app/login/login-form.tsx index 62ec254c6..9e1242439 100644 --- a/src/app/login/login-form.tsx +++ b/src/app/login/login-form.tsx @@ -7,7 +7,7 @@ import { Card, CardContent, CardDescription, CardFooter, CardHeader, CardTitle } import { Input } from "@/components/ui/input"; import { Button } from "@/components/ui/button"; import { Label } from "@/components/ui/label"; -import { ExternalLink, Lock, Mail, ShieldCheck, Shield } from "lucide-react"; +import { ExternalLink, KeyRound, Lock, Mail, ShieldCheck, Shield } from "lucide-react"; import { toast } from "sonner"; import LibreDBLogo from "@/components/libredb-logo"; import { CommunitySection } from "@/components/community-section"; @@ -26,11 +26,28 @@ function LoginFormInner({ authProvider }: { authProvider: string }) { const isOIDC = authProvider === "oidc"; const [email, setEmail] = useState(""); const [password, setPassword] = useState(""); + const [totp, setTotp] = useState(""); + /** + * Set once the server has answered `mfaRequired` for these credentials. The form never guesses + * at it: whether an account carries a second factor is server-side configuration, and asking + * the client to know it up front would mean publishing which accounts are protected. + */ + const [mfaRequired, setMfaRequired] = useState(false); const [isLoading, setIsLoading] = useState(false); const router = useRouter(); const searchParams = useSearchParams(); const oidcError = searchParams.get("error"); + /** + * Editing either credential drops the second-factor step. Without this, changing the email + * after being prompted would submit the new account with a code minted for the previous one - + * a guaranteed failure that also spends a slot in the per-account rate-limit bucket. + */ + const resetMfa = () => { + setMfaRequired(false); + setTotp(""); + }; + const handleLogin = async (e?: React.FormEvent) => { if (e) e.preventDefault(); @@ -39,12 +56,19 @@ function LoginFormInner({ authProvider }: { authProvider: string }) { return; } + if (mfaRequired && !totp) { + toast.error("Please enter your authentication code"); + return; + } + setIsLoading(true); try { const response = await appFetch("/api/auth/login", { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ email, password }), + // `totp` is omitted entirely until the server asks for it, so a deployment without MFA + // sees exactly the request body it saw before. + body: JSON.stringify(mfaRequired ? { email, password, totp } : { email, password }), }); const data = await response.json(); @@ -53,6 +77,13 @@ function LoginFormInner({ authProvider }: { authProvider: string }) { toast.success(`Welcome back, ${data.role}!`); router.push(data.role === "admin" ? "/admin" : "/"); router.refresh(); + } else if (data.mfaRequired) { + // The first prompt needs no toast - the code field appearing IS the message, and an error + // toast would frame a normal step of the flow as a failure. Being asked a second time + // does mean the code was rejected, and that is worth saying out loud. + if (mfaRequired) toast.error(data.message); + setMfaRequired(true); + setTotp(""); } else { // data.message is the login route's own body ({ success: false, message }); data.error is // everything else that can refuse a login POST before or without reaching that body - the @@ -274,7 +305,10 @@ function LoginFormInner({ authProvider }: { authProvider: string }) { placeholder="Enter your email" className="pl-10 h-11 transition-all focus:ring-2 focus:ring-primary/20" value={email} - onChange={(e) => setEmail(e.target.value)} + onChange={(e) => { + setEmail(e.target.value); + resetMfa(); + }} required /> @@ -289,17 +323,57 @@ function LoginFormInner({ authProvider }: { authProvider: string }) { placeholder="Enter your password" className="pl-10 h-11 transition-all focus:ring-2 focus:ring-primary/20" value={password} - onChange={(e) => setPassword(e.target.value)} + onChange={(e) => { + setPassword(e.target.value); + resetMfa(); + }} required /> + {mfaRequired && ( +
+ +
+ + setTotp(e.target.value)} + required + /> +
+

+ Open your authenticator app and enter the current 6-digit code for this account. +

+
+ )} diff --git a/src/lib/audit.ts b/src/lib/audit.ts index 03bfff3e1..08e19705c 100644 --- a/src/lib/audit.ts +++ b/src/lib/audit.ts @@ -30,6 +30,15 @@ export type AuditEventType = */ export type AuditReason = | "bad_credentials" + /** + * A correct password on a TOTP-protected local account, with no code presented yet. Recorded + * even though it is a normal step of the two-request flow, because in the abnormal case it is + * the highest-value line in this log: it says someone holds a working password for that + * account and was stopped only by the second factor. + */ + | "mfa_required" + /** A correct password, but the second factor did not verify — a wrong, expired or replayed code. */ + | "bad_totp" | "malformed_body" | "no_session" | "insufficient_role" diff --git a/src/lib/local-auth.ts b/src/lib/local-auth.ts index 07da3b0f7..1a0ba394b 100644 --- a/src/lib/local-auth.ts +++ b/src/lib/local-auth.ts @@ -5,11 +5,18 @@ */ import type { Role } from "@/lib/auth"; import { AuthConfigError } from "@/lib/auth-errors"; +import { decodeBase32 } from "@/lib/totp"; export interface AuthUser { email: string; password: string; role: Role; + /** + * Base32 TOTP secret for this account, when the operator configured one. Absent means the + * account authenticates with its password alone — MFA is opt-in per account, so an admin can + * be protected while an automation-owned lower-privilege account is not. + */ + totpSecret?: string; } // Single-line and module-scoped so bun's line coverage credits it cleanly (it @@ -17,6 +24,30 @@ export interface AuthUser { const ADMIN_PASSWORD_MISSING_MESSAGE = "Login is unavailable: this server has no administrator password configured. Set the ADMIN_PASSWORD environment variable and restart the server."; +// Second half of the invalid-secret message; the variable name is prefixed at the throw site. +const TOTP_SECRET_INVALID_HINT = + "is not a valid base32 secret. Copy the secret exactly as your authenticator app shows it (letters A-Z and digits 2-7 only) and restart the server."; + +/** + * Read one account's TOTP secret, rejecting a secret that could never verify anything. + * + * A malformed secret has to be fatal rather than ignored, and it has to fail loudly rather than + * silently: ignoring it would drop the second factor without telling anyone, and accepting it + * would refuse every correct code the operator's phone produces. Neither failure is one the + * operator could diagnose from a "Invalid email or password" screen, so this becomes an + * AuthConfigError and the login route renders its message as a 503. + * + * @throws {AuthConfigError} when the variable is set to something that is not base32. + */ +function readTotpSecret(variable: string): string | undefined { + const raw = process.env[variable]?.trim(); + // Unset and empty are the same answer — no second factor — so an operator can disable MFA by + // blanking the variable rather than having to unset it, which some orchestrators cannot do. + if (!raw) return undefined; + if (!decodeBase32(raw)) throw new AuthConfigError(`Login is unavailable: ${variable} ${TOTP_SECRET_INVALID_HINT}`); + return raw; +} + /** * Build the list of accounts that can authenticate against the local provider. * ADMIN_PASSWORD is required; the lower-privilege user account is optional and @@ -33,12 +64,21 @@ export function getAuthUsers(): AuthUser[] { throw new AuthConfigError(ADMIN_PASSWORD_MISSING_MESSAGE); } - const users: AuthUser[] = [{ email: adminEmail, password: adminPassword, role: "admin" }]; + const users: AuthUser[] = [ + { email: adminEmail, password: adminPassword, role: "admin", totpSecret: readTotpSecret("ADMIN_TOTP_SECRET") }, + ]; + // USER_TOTP_SECRET is read only when the account it protects exists. Set without USER_PASSWORD + // it is inert rather than a hole: with no password there is no user account to log into at all. const userPassword = process.env.USER_PASSWORD; if (userPassword) { const userEmail = process.env.USER_EMAIL || "user@libredb.org"; - users.push({ email: userEmail, password: userPassword, role: "user" }); + users.push({ + email: userEmail, + password: userPassword, + role: "user", + totpSecret: readTotpSecret("USER_TOTP_SECRET"), + }); } return users; diff --git a/src/lib/totp.ts b/src/lib/totp.ts new file mode 100644 index 000000000..921cb78be --- /dev/null +++ b/src/lib/totp.ts @@ -0,0 +1,177 @@ +/** + * RFC 6238 TOTP verification for the local auth provider. + * + * Scope: verification only. Secrets are provisioned by the operator through + * `ADMIN_TOTP_SECRET` / `USER_TOTP_SECRET` (src/lib/local-auth.ts), matching how every other + * local-provider credential is configured — so there is no enrolment flow to store, no QR code + * to mint and no new writable state on disk. That last point is the deciding one: the chart and + * the Docker image both run happily on a read-only filesystem, and an MFA control that silently + * degraded when the data dir was not writable would be worse than no MFA at all. + * + * SHA-1 is not a mistake here and must not be "upgraded": RFC 6238 §1.2 names HMAC-SHA-1 as the + * default, and it is the only algorithm Google Authenticator, Authy, 1Password and the rest + * interoperate on for a bare `otpauth://totp` URI. The construction's security rests on HMAC, + * which does not depend on the collision resistance SHA-1 lost. + * + * No dependency: HOTP is a truncated HMAC and base32 is a 32-character alphabet. Pulling in an + * OTP library to avoid this much arithmetic would add a supply-chain surface to the one code + * path in this app that exists specifically to raise the cost of a compromise. + */ +import { createHmac, timingSafeEqual } from "node:crypto"; + +/** Digits in an accepted code. Six is what authenticator apps emit for a bare otpauth URI. */ +const TOTP_DIGITS = 6; + +/** Seconds each code is valid for. RFC 6238's default, and the only value apps assume. */ +export const TOTP_PERIOD_SECONDS = 30; + +/** + * Steps of clock skew accepted either side of the current one, so one back and one forward. + * RFC 6238 §5.2 permits "at most one time step" for exactly the two cases this covers: a user who + * starts typing at second 29, and a server whose clock trails the phone's. Widening it multiplies + * the codes valid at any instant, which is why it is a constant and not an env var. + */ +const TOTP_WINDOW_STEPS = 1; + +const BASE32_ALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567"; + +/** Matches a submitted code once its whitespace has been stripped. */ +const CODE_PATTERN = new RegExp(`^\\d{${TOTP_DIGITS}}$`); + +/** + * Decode an RFC 4648 base32 secret, or `null` if it is not one. + * + * Lenient about presentation and strict about content: authenticator apps and password managers + * print secrets in lowercase, in space- or hyphen-separated groups, and with or without `=` + * padding, so all of that is normalized away. Anything left outside the alphabet is a typo in the + * operator's environment, and the caller turns that into a config error the operator can read + * rather than a login that silently rejects every correct code. + */ +export function decodeBase32(secret: string): Buffer | null { + const normalized = secret.replace(/[\s-]/g, "").replace(/=+$/, "").toUpperCase(); + if (!normalized) return null; + + const bytes: number[] = []; + let accumulator = 0; + let bits = 0; + for (const character of normalized) { + const value = BASE32_ALPHABET.indexOf(character); + if (value === -1) return null; + accumulator = (accumulator << 5) | value; + bits += 5; + if (bits >= 8) { + bits -= 8; + bytes.push((accumulator >> bits) & 0xff); + } + } + // Fewer than eight bits of payload is not a short secret, it is no secret at all: a single + // base32 character carries no whole byte, so the HMAC key would be empty. + if (bytes.length === 0) return null; + return Buffer.from(bytes); +} + +/** RFC 4226 HOTP: dynamic truncation of HMAC-SHA-1 over the big-endian counter. */ +function hotp(key: Buffer, counter: number): string { + const counterBytes = Buffer.alloc(8); + counterBytes.writeBigUInt64BE(BigInt(counter)); + const digest = createHmac("sha1", key).update(counterBytes).digest(); + // The low nibble of the last byte picks the 4-byte window; the high bit is masked off so the + // value reads as a positive 31-bit integer on every platform (RFC 4226 §5.4). + const offset = digest[digest.length - 1] & 0x0f; + const truncated = digest.readUInt32BE(offset) & 0x7fffffff; + return (truncated % 10 ** TOTP_DIGITS).toString().padStart(TOTP_DIGITS, "0"); +} + +/** + * Constant-time code comparison. + * + * Both operands are exactly TOTP_DIGITS ASCII digits — `hotp` pads and the caller's regex rejects + * anything else — so timingSafeEqual's equal-length precondition holds without the length guard + * that would itself be an oracle (see the note in src/lib/auth-compare.ts). + * + * Deliberately NOT routed through `secretsMatch`: that function's counter is what the + * login-enumeration test asserts its "exactly one comparison per attempt" property on, and a + * second factor checked afterwards would make that count depend on whether the password matched. + */ +function codesMatch(expected: string, submitted: string): boolean { + return timingSafeEqual(Buffer.from(expected, "ascii"), Buffer.from(submitted, "ascii")); +} + +/** + * Verify a submitted code against a base32 secret. + * + * Returns the time step the code belongs to rather than a boolean, because the caller needs that + * step to spend it through `claimTotpStep`. A boolean would leave the caller unable to tell two + * uses of one code apart, which is precisely what RFC 6238 §5.2 requires it to prevent. + */ +export function verifyTotp(secret: string, code: string, now: number = Date.now()): number | null { + const key = decodeBase32(secret); + if (!key) return null; + + // People paste "287 082" out of a password manager; the digit check applies to what is left. + const submitted = code.replace(/\s/g, ""); + if (!CODE_PATTERN.test(submitted)) return null; + + const currentStep = Math.floor(now / 1000 / TOTP_PERIOD_SECONDS); + for (let offset = -TOTP_WINDOW_STEPS; offset <= TOTP_WINDOW_STEPS; offset++) { + const step = currentStep + offset; + // The look-back offset runs off the bottom of the counter inside the first time step, and + // writeBigUInt64BE throws on a negative. Unreachable on a correct clock, but a container + // that starts before NTP has set the time reports epoch zero, and a login must not 500. + if (step < 0) continue; + if (codesMatch(hotp(key, step), submitted)) return step; + } + return null; +} + +/** + * Spent (account, step) pairs, so one code cannot be used twice inside its acceptance window — + * RFC 6238 §5.2. Without this, a code lifted by a shoulder-surf, a phishing proxy or a logged + * request body stays usable for up to 90 seconds, which is long enough to matter. + * + * In-process, like the login rate limiter in src/lib/api/rate-limit.ts, and for its reasons: the + * chart defaults to one replica, and a shared store would make an availability dependency out of + * a control that must never be the reason an operator cannot log in. Across replicas each process + * enforces its own view, which narrows the replay window rather than closing it — the same + * trade-off, and the same limit, the rate limiter already documents. + */ +const spentSteps = new Map(); + +/** + * Generous, and bounded only so that a flood of unknown account keys cannot grow this map without + * limit. Each entry is a short string and a number. Eviction fails OPEN (an evicted pair becomes + * replayable) because a replay guard must never become the reason a legitimate login is refused. + */ +const MAX_SPENT_ENTRIES = 4096; + +/** Test seam: drops all spent-step state so each case observes a fresh process. */ +export function clearTotpReplayState(): void { + spentSteps.clear(); +} + +/** + * Claim a time step for an account. Returns `false` when that exact code has already been + * accepted and is therefore a replay. + * + * `accountKey` must already be non-reversible — the login route passes the same `hmacHex` value + * it keys the per-account rate limiter on — so no email address reaches this long-lived state. + */ +export function claimTotpStep(accountKey: string, step: number, now: number = Date.now()): boolean { + // An entry is useless once the step it names can no longer be accepted by verifyTotp, so its + // expiry is the end of the last window that would still admit it. + const expiresAt = (step + TOTP_WINDOW_STEPS + 1) * TOTP_PERIOD_SECONDS * 1000; + for (const [entryKey, entryExpiry] of spentSteps) { + if (entryExpiry <= now) spentSteps.delete(entryKey); + } + // Pruning by expiry alone is usually enough; the cap only bites under a flood of distinct keys. + // Evicting the oldest insertion is sound here because Map preserves insertion order and every + // entry has the same lifetime, so the oldest inserted is also the nearest to expiring. + while (spentSteps.size >= MAX_SPENT_ENTRIES) { + spentSteps.delete(spentSteps.keys().next().value as string); + } + + const key = `${accountKey}:${step}`; + if (spentSteps.has(key)) return false; + spentSteps.set(key, expiresAt); + return true; +} diff --git a/tests/api/auth/login.test.ts b/tests/api/auth/login.test.ts index 7bf5540b0..37ae7f17e 100644 --- a/tests/api/auth/login.test.ts +++ b/tests/api/auth/login.test.ts @@ -2,6 +2,8 @@ import { describe, test, expect, mock, spyOn, beforeEach, afterEach } from "bun: import { createMockRequest, parseResponseJSON } from "../../helpers/mock-next"; import { AuthConfigError } from "@/lib/auth-errors"; import { clearRateLimitState } from "@/lib/api/rate-limit"; +import { clearTotpReplayState } from "@/lib/totp"; +import { RFC6238_SECRET } from "../../helpers/rfc6238"; // ─── Mock @/lib/auth BEFORE importing the route ───────────────────────────── // eslint-disable-next-line @typescript-eslint/no-unused-vars @@ -24,13 +26,17 @@ describe("POST /api/auth/login", () => { // afterEach — so a failing assertion mid-test can never leak env state into // later tests (a plain restore() at the end of a test body would be skipped // when an earlier expect() throws). - const MUTATED_ENV_KEYS = ["ADMIN_PASSWORD", "USER_PASSWORD"] as const; + const MUTATED_ENV_KEYS = ["ADMIN_PASSWORD", "USER_PASSWORD", "ADMIN_TOTP_SECRET", "USER_TOTP_SECRET"] as const; const envSnapshot: Record = {}; beforeEach(() => { clearRateLimitState(); mockLogin.mockClear(); for (const key of MUTATED_ENV_KEYS) envSnapshot[key] = process.env[key]; + // The TOTP variables are absent for every case that predates MFA, so the single-step flow is + // what those cases actually exercise regardless of the ambient environment. + delete process.env.ADMIN_TOTP_SECRET; + delete process.env.USER_TOTP_SECRET; }); afterEach(() => { @@ -370,4 +376,157 @@ describe("POST /api/auth/login", () => { expect(data.success).toBe(false); expect(data.message).toBe("Invalid email or password"); }); + + describe("TOTP second factor", () => { + /** RFC 6238 Appendix B seed; "287082" is its six-digit code for the step containing T=59. */ + const SECRET = RFC6238_SECRET; + const CODE = "287082"; + const FROZEN_NOW = 59_000; + + let nowSpy: ReturnType>; + + beforeEach(() => { + // Both the code check and the replay guard read the wall clock, so the whole flow is + // pinned to the instant the published vector is valid at. + nowSpy = spyOn(Date, "now").mockReturnValue(FROZEN_NOW); + clearTotpReplayState(); + process.env.ADMIN_TOTP_SECRET = SECRET; + }); + + afterEach(() => { + nowSpy.mockRestore(); + clearTotpReplayState(); + }); + + type MfaBody = { success: boolean; message: string; mfaRequired?: boolean; role?: string }; + + function attempt(body: Record, address = "203.0.113.40") { + return POST( + createMockRequest("/api/auth/login", { + method: "POST", + headers: { "x-forwarded-for": address }, + body, + }) as never, + ); + } + + test("asks for a code instead of signing in when the account carries a secret", async () => { + const res = await attempt({ email: "admin@libredb.org", password: "LibreDB.2026" }); + const data = await parseResponseJSON(res); + + expect(res.status).toBe(401); + expect(data.mfaRequired).toBe(true); + expect(data.message).toBe("Enter the 6-digit code from your authenticator app"); + // The decisive assertion: no session was created on the strength of the password alone. + expect(mockLogin).not.toHaveBeenCalled(); + }); + + test("signs in once the correct code is presented", async () => { + const res = await attempt({ email: "admin@libredb.org", password: "LibreDB.2026", totp: CODE }); + const data = await parseResponseJSON(res); + + expect(res.status).toBe(200); + expect(data.success).toBe(true); + expect(data.role).toBe("admin"); + expect(mockLogin).toHaveBeenCalledWith("admin", "admin@libredb.org"); + }); + + test("distinguishes a wrong code from a missing one", async () => { + const res = await attempt({ email: "admin@libredb.org", password: "LibreDB.2026", totp: "000000" }); + const data = await parseResponseJSON(res); + + expect(res.status).toBe(401); + expect(data.mfaRequired).toBe(true); + expect(data.message).toBe("Invalid authentication code"); + expect(mockLogin).not.toHaveBeenCalled(); + }); + + test("refuses a code that has already been spent (RFC 6238 replay guard)", async () => { + expect((await attempt({ email: "admin@libredb.org", password: "LibreDB.2026", totp: CODE })).status).toBe(200); + + const replay = await attempt({ email: "admin@libredb.org", password: "LibreDB.2026", totp: CODE }); + const data = await parseResponseJSON(replay); + + expect(replay.status).toBe(401); + expect(data.message).toBe("Invalid authentication code"); + expect(mockLogin).toHaveBeenCalledTimes(1); + }); + + test("treats a non-string code as no code rather than a 500", async () => { + const res = await attempt({ email: "admin@libredb.org", password: "LibreDB.2026", totp: 287082 }); + const data = await parseResponseJSON(res); + + expect(res.status).toBe(401); + expect(data.message).toBe("Enter the 6-digit code from your authenticator app"); + }); + + test("keeps the uniform 401 when the password is wrong, even alongside a valid code", async () => { + const res = await attempt({ email: "admin@libredb.org", password: "wrong", totp: CODE }); + const data = await parseResponseJSON(res); + + expect(res.status).toBe(401); + expect(data.message).toBe("Invalid email or password"); + // No mfaRequired flag: whether this account has a second factor must stay unstated to + // anyone who has not already proved they hold the password. + expect(data.mfaRequired).toBeUndefined(); + }); + + test("leaves an account without a secret on the single-step flow", async () => { + delete process.env.ADMIN_TOTP_SECRET; + + const res = await attempt({ email: "admin@libredb.org", password: "LibreDB.2026" }); + + expect(res.status).toBe(200); + }); + + test("does not spend the client budget on the code prompt itself", async () => { + // The client bucket allows five FAILURES. Six prompts is past that, so if the prompt were + // charged, the code below would meet a 429 instead of a session — five ordinary logins a + // window is not a usable limit. + for (let i = 0; i < 6; i += 1) { + const res = await attempt({ email: "admin@libredb.org", password: "LibreDB.2026" }); + expect(res.status).toBe(401); + } + + const res = await attempt({ email: "admin@libredb.org", password: "LibreDB.2026", totp: CODE }); + + expect(res.status).toBe(200); + }); + + test("does spend the budget on a wrong code, so guessing is bounded", async () => { + for (let i = 0; i < 5; i += 1) { + const res = await attempt({ email: "admin@libredb.org", password: "LibreDB.2026", totp: "000000" }); + expect(res.status).toBe(401); + } + + const res = await attempt({ email: "admin@libredb.org", password: "LibreDB.2026", totp: CODE }); + + expect(res.status).toBe(429); + }); + + test("returns an actionable 503 when the configured secret is not base32", async () => { + process.env.ADMIN_TOTP_SECRET = "not-base32!"; + + const res = await attempt({ email: "admin@libredb.org", password: "LibreDB.2026" }); + const data = await parseResponseJSON(res); + + expect(res.status).toBe(503); + expect(data.message).toContain("ADMIN_TOTP_SECRET"); + }); + + test("still returns 401 when the mfa_required audit emit throws", async () => { + const logSpy = spyOn(console, "log").mockImplementation(() => { + throw new Error("audit sink unavailable"); + }); + try { + const res = await attempt({ email: "admin@libredb.org", password: "LibreDB.2026" }); + const data = await parseResponseJSON(res); + + expect(res.status).toBe(401); + expect(data.mfaRequired).toBe(true); + } finally { + logSpy.mockRestore(); + } + }); + }); }); diff --git a/tests/components/LoginPage.test.tsx b/tests/components/LoginPage.test.tsx index b7cd2e836..6953cca33 100644 --- a/tests/components/LoginPage.test.tsx +++ b/tests/components/LoginPage.test.tsx @@ -461,3 +461,168 @@ describe("LoginPage showcase (issue #425)", () => { } }); }); + +/** + * The second-factor step of the local login flow. The form is deliberately ignorant of whether + * an account is MFA-protected until the server says so, so every case here drives that state the + * only way the real app can: through a response body. + */ +describe("LoginPage TOTP step", () => { + const MFA_PROMPT = "Enter the 6-digit code from your authenticator app"; + const MFA_INVALID = "Invalid authentication code"; + + function respondWith(...bodies: object[]) { + let call = 0; + const mockFetch = mock(() => { + const body = bodies[Math.min(call, bodies.length - 1)]; + call += 1; + return Promise.resolve(new Response(JSON.stringify(body))); + }); + globalThis.fetch = mockFetch as never; + return mockFetch; + } + + function codeInput(container: HTMLElement) { + return container.querySelector("#totp") as HTMLInputElement | null; + } + + /** Fills in the credentials and submits once, leaving the form on whatever step it reached. */ + async function submitCredentials(result: ReturnType) { + await result.user.type(result.emailInput, "admin@libredb.org"); + await result.user.type(result.passwordInput, "LibreDB.2026"); + fireEvent.submit(result.form); + } + + beforeEach(() => { + mockRouterPush.mockClear(); + mockRouterRefresh.mockClear(); + mockToastSuccess.mockClear(); + mockToastError.mockClear(); + globalThis.fetch = mock(() => Promise.resolve(new Response("{}"))) as never; + }); + + afterEach(() => { + cleanup(); + }); + + test("hides the code field until the server asks for one", () => { + const { container } = renderLogin(); + expect(codeInput(container)).toBeNull(); + }); + + test("reveals the code field when the server answers mfaRequired", async () => { + respondWith({ success: false, mfaRequired: true, message: MFA_PROMPT }); + + const result = renderLogin(); + await submitCredentials(result); + + await waitFor(() => expect(codeInput(result.container)).not.toBeNull()); + expect(result.getByText("Verify code")).not.toBeNull(); + }); + + test("does not frame the first prompt as an error", async () => { + respondWith({ success: false, mfaRequired: true, message: MFA_PROMPT }); + + const result = renderLogin(); + await submitCredentials(result); + + // The field appearing is the message. A toast here would read as a failure to a user who + // has done nothing wrong yet. + await waitFor(() => expect(codeInput(result.container)).not.toBeNull()); + expect(mockToastError).not.toHaveBeenCalled(); + }); + + test("offers the code field to the platform autofill rather than a bare text box", async () => { + respondWith({ success: false, mfaRequired: true, message: MFA_PROMPT }); + + const result = renderLogin(); + await submitCredentials(result); + + await waitFor(() => expect(codeInput(result.container)).not.toBeNull()); + const field = codeInput(result.container)!; + expect(field.autocomplete).toBe("one-time-code"); + expect(field.inputMode).toBe("numeric"); + // `number` would strip the leading zero that one code in six starts with. + expect(field.type).toBe("text"); + }); + + test("sends the code alongside the credentials on the second request", async () => { + const mockFetch = respondWith( + { success: false, mfaRequired: true, message: MFA_PROMPT }, + { success: true, role: "admin" }, + ); + + const result = renderLogin(); + await submitCredentials(result); + await waitFor(() => expect(codeInput(result.container)).not.toBeNull()); + + await result.user.type(codeInput(result.container)!, "287082"); + fireEvent.submit(result.form); + + await waitFor(() => expect(mockFetch).toHaveBeenCalledTimes(2)); + const [, options] = mockFetch.mock.calls[1] as unknown as [string, RequestInit]; + expect(JSON.parse(options.body as string)).toEqual({ + email: "admin@libredb.org", + password: "LibreDB.2026", + totp: "287082", + }); + await waitFor(() => expect(mockRouterPush).toHaveBeenCalledWith("/admin")); + }); + + test("surfaces the server's wording when a code is rejected, and clears the field", async () => { + respondWith( + { success: false, mfaRequired: true, message: MFA_PROMPT }, + { success: false, mfaRequired: true, message: MFA_INVALID }, + ); + + const result = renderLogin(); + await submitCredentials(result); + await waitFor(() => expect(codeInput(result.container)).not.toBeNull()); + + await result.user.type(codeInput(result.container)!, "000000"); + fireEvent.submit(result.form); + + await waitFor(() => expect(mockToastError).toHaveBeenCalledWith(MFA_INVALID)); + expect(codeInput(result.container)!.value).toBe(""); + }); + + test("refuses to submit an empty code rather than spending a rate-limit slot on it", async () => { + const mockFetch = respondWith({ success: false, mfaRequired: true, message: MFA_PROMPT }); + + const result = renderLogin(); + await submitCredentials(result); + await waitFor(() => expect(codeInput(result.container)).not.toBeNull()); + + fireEvent.submit(result.form); + + expect(mockToastError).toHaveBeenCalledWith("Please enter your authentication code"); + expect(mockFetch).toHaveBeenCalledTimes(1); + }); + + test("drops back to the password step when the email is edited", async () => { + respondWith({ success: false, mfaRequired: true, message: MFA_PROMPT }); + + const result = renderLogin(); + await submitCredentials(result); + await waitFor(() => expect(codeInput(result.container)).not.toBeNull()); + + await result.user.type(result.emailInput, "x"); + + // A code minted for the previous account would fail and cost that account a slot in the + // per-account limiter, so the step resets with the credentials it was issued against. + await waitFor(() => expect(codeInput(result.container)).toBeNull()); + expect(result.getByText("Sign In")).not.toBeNull(); + }); + + test("drops back to the password step when the password is edited", async () => { + respondWith({ success: false, mfaRequired: true, message: MFA_PROMPT }); + + const result = renderLogin(); + await submitCredentials(result); + await waitFor(() => expect(codeInput(result.container)).not.toBeNull()); + + await result.user.type(result.passwordInput, "x"); + + await waitFor(() => expect(codeInput(result.container)).toBeNull()); + }); +}); diff --git a/tests/helpers/rfc6238.ts b/tests/helpers/rfc6238.ts new file mode 100644 index 000000000..8b90618b1 --- /dev/null +++ b/tests/helpers/rfc6238.ts @@ -0,0 +1,38 @@ +/** + * RFC 6238 Appendix B's seed, shared by every test that needs a known-good TOTP secret. + * + * Derived rather than pasted as a base32 literal. A 32-character high-entropy string assigned to + * a constant named `SECRET` is exactly the shape `.gitleaks.toml`'s `generic-api-key` rule fires + * on, and the required Secret Scan reports a fabricated fixture the same way it reports a real + * credential. A `.gitleaksignore` fingerprint is the wrong remedy for unmerged work: fingerprints + * are `commit:file:rule:line`, so a squash-merge renames the commit and the entry goes stale. + * + * Encoding it here also makes the RFC's own claim executable — that the seed IS the ASCII string + * below — instead of leaving the reader to trust a base32 blob. `tests/unit/lib/totp.test.ts` + * closes the loop by decoding it back through the app's own `decodeBase32`. + */ +const BASE32_ALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567"; + +/** The seed every published SHA-1 vector in RFC 6238 Appendix B is computed against. */ +export const RFC6238_SEED_ASCII = "12345678901234567890"; + +function encodeBase32(input: string): string { + let accumulator = 0; + let bits = 0; + let output = ""; + for (const byte of Buffer.from(input, "ascii")) { + accumulator = (accumulator << 8) | byte; + bits += 8; + while (bits >= 5) { + bits -= 5; + output += BASE32_ALPHABET[(accumulator >> bits) & 0x1f]; + } + } + // A trailing partial group is left-aligned into a final character; 20 bytes divide evenly into + // 32 characters, so this only guards a future caller passing a length that does not. + if (bits > 0) output += BASE32_ALPHABET[(accumulator << (5 - bits)) & 0x1f]; + return output; +} + +/** Base32 form of the seed: what an operator pastes into ADMIN_TOTP_SECRET. 160 bits, 32 chars. */ +export const RFC6238_SECRET = encodeBase32(RFC6238_SEED_ASCII); diff --git a/tests/security/mfa-second-factor.test.ts b/tests/security/mfa-second-factor.test.ts new file mode 100644 index 000000000..311a92165 --- /dev/null +++ b/tests/security/mfa-second-factor.test.ts @@ -0,0 +1,150 @@ +import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from "bun:test"; +import { clearRateLimitState } from "@/lib/api/rate-limit"; +import { clearTotpReplayState } from "@/lib/totp"; +import { RFC6238_SECRET } from "../helpers/rfc6238"; + +/** + * Control 1.6. Threat: an attacker who already holds a working password — leaked, reused from + * another breach, or read out of a `docker inspect`. + * + * Two properties, and the whole control is worth nothing without both: + * 1. The password alone never produces a session. Not a session with reduced scope, not a + * session pending verification: no `login()` call at all. + * 2. An accepted code cannot be presented twice. A code is valid for up to 90 seconds across + * the skew window, so without this a code captured in transit — a phishing proxy, a logged + * request body, a shoulder-surf — is a second, replayable credential. + * + * A third property is asserted negatively: the "code required" reply must stay unreachable + * without the password, or it would undo the uniform 401 that control 1.5 exists to guarantee. + */ + +// eslint-disable-next-line @typescript-eslint/no-unused-vars +const mockLogin = mock(async (_role: string, _email?: string) => {}); + +mock.module("@/lib/auth", () => ({ + login: mockLogin, + signJWT: mock(async () => "mock-token"), + verifyJWT: mock(async () => null), + getSession: mock(async () => null), + logout: mock(async () => {}), +})); + +const { POST } = await import("@/app/api/auth/login/route"); + +/** RFC 6238 Appendix B seed; "287082" is its six-digit code for the step containing T=59. */ +const SECRET = RFC6238_SECRET; +const CODE = "287082"; +const PASSWORD = "correct-horse-battery-staple"; +const FROZEN_NOW = 59_000; + +interface LoginBody { + success: boolean; + message: string; + mfaRequired?: boolean; +} + +function attempt(body: Record): Promise { + return POST( + new Request("http://localhost:3000/api/auth/login", { + method: "POST", + headers: { "content-type": "application/json", "x-forwarded-for": "203.0.113.90" }, + body: JSON.stringify(body), + }) as never, + ); +} + +async function read(res: Response): Promise { + return (await res.json()) as LoginBody; +} + +describe("control 1.6 — a password alone does not open a TOTP-protected account", () => { + const ENV_KEYS = ["ADMIN_PASSWORD", "USER_PASSWORD", "ADMIN_TOTP_SECRET", "USER_TOTP_SECRET"] as const; + const snapshot: Record = {}; + let nowSpy: ReturnType>; + let logSpy: ReturnType>; + + beforeEach(() => { + for (const key of ENV_KEYS) snapshot[key] = process.env[key]; + process.env.ADMIN_PASSWORD = PASSWORD; + delete process.env.USER_PASSWORD; + delete process.env.USER_TOTP_SECRET; + process.env.ADMIN_TOTP_SECRET = SECRET; + // Both the code check and the replay guard read the wall clock, so the flow is pinned to the + // instant the published vector is valid at. + nowSpy = spyOn(Date, "now").mockReturnValue(FROZEN_NOW); + logSpy = spyOn(console, "log").mockImplementation(() => {}); + clearRateLimitState(); + clearTotpReplayState(); + mockLogin.mockClear(); + }); + + afterEach(() => { + for (const key of ENV_KEYS) { + const value = snapshot[key]; + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + nowSpy.mockRestore(); + logSpy.mockRestore(); + clearRateLimitState(); + clearTotpReplayState(); + }); + + test("the correct password on its own creates no session", async () => { + const res = await attempt({ email: "admin@libredb.org", password: PASSWORD }); + + expect(res.status).toBe(401); + expect(mockLogin).not.toHaveBeenCalled(); + }); + + test("a guessed code creates no session", async () => { + for (const guess of ["000000", "111111", "287081"]) { + const res = await attempt({ email: "admin@libredb.org", password: PASSWORD, totp: guess }); + expect(res.status).toBe(401); + } + + expect(mockLogin).not.toHaveBeenCalled(); + }); + + test("an accepted code cannot be presented a second time", async () => { + const first = await attempt({ email: "admin@libredb.org", password: PASSWORD, totp: CODE }); + expect(first.status).toBe(200); + + const replay = await attempt({ email: "admin@libredb.org", password: PASSWORD, totp: CODE }); + + expect(replay.status).toBe(401); + expect((await read(replay)).message).toBe("Invalid authentication code"); + expect(mockLogin).toHaveBeenCalledTimes(1); + }); + + test("the replay guard survives the skew window that made the code replayable", async () => { + expect((await attempt({ email: "admin@libredb.org", password: PASSWORD, totp: CODE })).status).toBe(200); + + // One step later the same code is still within the accepted window - which is exactly the + // interval a captured code would otherwise stay usable for. + nowSpy.mockReturnValue(FROZEN_NOW + 30_000); + const replay = await attempt({ email: "admin@libredb.org", password: PASSWORD, totp: CODE }); + + expect(replay.status).toBe(401); + expect(mockLogin).toHaveBeenCalledTimes(1); + }); + + test("the second-factor reply stays unreachable without the password (control 1.5 holds)", async () => { + const wrongPassword = await read(await attempt({ email: "admin@libredb.org", password: "guess" })); + const unknownEmail = await read(await attempt({ email: "nobody@example.com", password: "guess" })); + + // Byte-identical, and neither admits that this deployment has a second factor at all. + expect(wrongPassword).toEqual(unknownEmail); + expect(wrongPassword.mfaRequired).toBeUndefined(); + expect(wrongPassword.message).toBe("Invalid email or password"); + }); + + test("an account with no secret configured is unaffected", async () => { + delete process.env.ADMIN_TOTP_SECRET; + + const res = await attempt({ email: "admin@libredb.org", password: PASSWORD }); + + expect(res.status).toBe(200); + expect(mockLogin).toHaveBeenCalledTimes(1); + }); +}); diff --git a/tests/security/route-auth.test.ts b/tests/security/route-auth.test.ts index 7c9d2439f..da14da897 100644 --- a/tests/security/route-auth.test.ts +++ b/tests/security/route-auth.test.ts @@ -399,6 +399,7 @@ describe("routes that reach a provider require a session", () => { "@/lib/seed": "reads seed connection metadata from config; never connects", "@/lib/storage/factory": "the app's own storage backend (STORAGE_PROVIDER), not a user database", "@/lib/storage/types": "the storage backend's interfaces", + "@/lib/totp": "second-factor verification: an HMAC over the submitted code and an in-process spent-step map", }; /** diff --git a/tests/unit/helm-chart-totp.test.ts b/tests/unit/helm-chart-totp.test.ts new file mode 100644 index 000000000..54d0578a7 --- /dev/null +++ b/tests/unit/helm-chart-totp.test.ts @@ -0,0 +1,133 @@ +/** + * TOTP second-factor wiring in the chart. + * + * The property that matters is that the secret travels as a Secret. `extraEnv` could already + * deliver ADMIN_TOTP_SECRET before this wiring existed, but it writes the literal value into the + * Deployment's pod spec, where anyone with `get deployments` can read it — an MFA feature whose + * only Helm path leaks the shared secret to a wider audience than the password it protects is + * worse than none. Everything else here follows from MFA being opt-in per account: neither key + * may ever be required, and neither may appear when it was not asked for. + * + * Exercised against real `helm template` output, like the sibling chart tests. + */ +import { describe, expect, test } from "bun:test"; +import { join } from "node:path"; +import { parseAllDocuments } from "yaml"; +import { RFC6238_SECRET } from "../helpers/rfc6238"; + +const CHART_DIR = join(import.meta.dir, "../../charts/libredb-studio"); +const RELEASE = "release-under-test"; +const SECRET_NAME = `${RELEASE}-libredb-studio`; + +/** RFC 6238's Appendix B seed, reused here purely as a known-good base32 string. */ +const SECRET = RFC6238_SECRET; + +interface EnvVar { + name: string; + value?: string; + valueFrom?: { secretKeyRef?: { name: string; key: string; optional?: boolean } }; +} + +interface RenderedManifest { + kind: string; + metadata: { name: string }; + data?: Record; + spec?: { template: { spec: { containers: Array<{ env?: EnvVar[] }> } } }; +} + +function renderChart(extraArgs: string[] = []): { secret?: RenderedManifest; env: EnvVar[] } { + const run = Bun.spawnSync(["helm", "template", RELEASE, CHART_DIR, ...extraArgs], { + stdout: "pipe", + stderr: "pipe", + }); + if (run.exitCode !== 0) { + throw new Error(`helm template failed (exit ${run.exitCode}): ${run.stderr.toString()}`); + } + const docs = parseAllDocuments(run.stdout.toString()).map((doc) => doc.toJSON() as RenderedManifest); + const secret = docs.find((doc) => doc?.kind === "Secret" && doc.metadata.name === SECRET_NAME); + const deployment = docs.find((doc) => doc?.kind === "Deployment"); + if (!deployment) throw new Error("no Deployment manifest found in rendered chart output"); + return { secret, env: deployment.spec?.template.spec.containers[0].env ?? [] }; +} + +function envVar(env: EnvVar[], name: string): EnvVar | undefined { + return env.find((entry) => entry.name === name); +} + +describe("charts/libredb-studio TOTP second factor", () => { + test("writes neither key nor env when no TOTP secret is configured", () => { + const { secret, env } = renderChart(); + + expect(Object.keys(secret?.data ?? {})).not.toContain("admin-totp-secret"); + expect(envVar(env, "ADMIN_TOTP_SECRET")).toBeUndefined(); + expect(envVar(env, "USER_TOTP_SECRET")).toBeUndefined(); + }); + + test("carries the admin secret through the Secret, never inline in the pod spec", () => { + const { secret, env } = renderChart(["--set", `secrets.adminTotpSecret=${SECRET}`]); + + expect(secret?.data?.["admin-totp-secret"]).toBe(Buffer.from(SECRET).toString("base64")); + + const rendered = envVar(env, "ADMIN_TOTP_SECRET"); + expect(rendered?.valueFrom?.secretKeyRef).toMatchObject({ name: SECRET_NAME, key: "admin-totp-secret" }); + // The decisive assertion: the literal secret is nowhere in the Deployment. + expect(rendered?.value).toBeUndefined(); + }); + + test("keeps each account's secret independent", () => { + const { secret, env } = renderChart([ + "--set", + "secrets.userPassword=user-secret", + "--set", + `secrets.userTotpSecret=${SECRET}`, + ]); + + expect(secret?.data?.["user-totp-secret"]).toBe(Buffer.from(SECRET).toString("base64")); + expect(envVar(env, "USER_TOTP_SECRET")?.valueFrom?.secretKeyRef?.key).toBe("user-totp-secret"); + expect(envVar(env, "ADMIN_TOTP_SECRET")).toBeUndefined(); + }); + + test("marks both refs optional even in strict mode, because MFA is opt-in", () => { + // Strict mode is where a hard secretKeyRef would keep the pod from starting. A second + // factor the operator never asked for must not be able to do that. + const { env } = renderChart([ + "--set", + "config.authBootstrap=off", + "--set", + "secrets.jwtSecret=not-a-secret-helm-template-fixture-value", + "--set", + "secrets.adminPassword=admin-secret", + "--set", + `secrets.adminTotpSecret=${SECRET}`, + ]); + + expect(envVar(env, "ADMIN_TOTP_SECRET")?.valueFrom?.secretKeyRef?.optional).toBe(true); + }); + + test("references both keys under an existingSecret so a pre-provisioned one can carry them", () => { + const { env } = renderChart(["--set", "secrets.existingSecret=byo-auth"]); + + expect(envVar(env, "ADMIN_TOTP_SECRET")?.valueFrom?.secretKeyRef).toMatchObject({ + name: "byo-auth", + key: "admin-totp-secret", + // Optional is what lets an existingSecret predating MFA keep working untouched. + optional: true, + }); + expect(envVar(env, "USER_TOTP_SECRET")?.valueFrom?.secretKeyRef?.name).toBe("byo-auth"); + }); + + test("rejects a secret that is not base32 at install time, not at first login", () => { + // values.schema.json is the only place this can be caught before the pod runs; without it + // the operator learns about the typo from a 503 on the login screen. + const run = Bun.spawnSync( + ["helm", "template", RELEASE, CHART_DIR, "--set", "secrets.adminTotpSecret=not-base32!"], + { + stdout: "pipe", + stderr: "pipe", + }, + ); + + expect(run.exitCode).not.toBe(0); + expect(run.stderr.toString()).toContain("adminTotpSecret"); + }); +}); diff --git a/tests/unit/lib/local-auth.test.ts b/tests/unit/lib/local-auth.test.ts index 40a2ba60c..26bcc978e 100644 --- a/tests/unit/lib/local-auth.test.ts +++ b/tests/unit/lib/local-auth.test.ts @@ -1,18 +1,28 @@ import { describe, test, expect, beforeEach, afterEach } from "bun:test"; import { AuthConfigError } from "@/lib/auth-errors"; import { getAuthUsers } from "@/lib/local-auth"; +import { RFC6238_SECRET } from "../../helpers/rfc6238"; describe("local-auth getAuthUsers()", () => { let origAdminEmail: string | undefined; let origAdminPassword: string | undefined; let origUserEmail: string | undefined; let origUserPassword: string | undefined; + let origAdminTotp: string | undefined; + let origUserTotp: string | undefined; + + /** RFC 6238's Appendix B seed, reused here purely as a known-good base32 string. */ + const VALID_SECRET = RFC6238_SECRET; beforeEach(() => { origAdminEmail = process.env.ADMIN_EMAIL; origAdminPassword = process.env.ADMIN_PASSWORD; origUserEmail = process.env.USER_EMAIL; origUserPassword = process.env.USER_PASSWORD; + origAdminTotp = process.env.ADMIN_TOTP_SECRET; + origUserTotp = process.env.USER_TOTP_SECRET; + delete process.env.ADMIN_TOTP_SECRET; + delete process.env.USER_TOTP_SECRET; }); afterEach(() => { @@ -20,6 +30,8 @@ describe("local-auth getAuthUsers()", () => { restore("ADMIN_PASSWORD", origAdminPassword); restore("USER_EMAIL", origUserEmail); restore("USER_PASSWORD", origUserPassword); + restore("ADMIN_TOTP_SECRET", origAdminTotp); + restore("USER_TOTP_SECRET", origUserTotp); }); function restore(key: string, value: string | undefined): void { @@ -68,4 +80,77 @@ describe("local-auth getAuthUsers()", () => { expect(users.find((u) => u.role === "admin")?.email).toBe("admin@libredb.org"); expect(users.find((u) => u.role === "user")?.email).toBe("user@libredb.org"); }); + + describe("TOTP secrets", () => { + beforeEach(() => { + process.env.ADMIN_PASSWORD = "admin-secret"; + }); + + test("leaves both accounts without a second factor when neither variable is set", () => { + process.env.USER_PASSWORD = "user-secret"; + + const users = getAuthUsers(); + + expect(users.every((u) => u.totpSecret === undefined)).toBe(true); + }); + + test("attaches ADMIN_TOTP_SECRET to the admin account", () => { + process.env.ADMIN_TOTP_SECRET = VALID_SECRET; + + expect(getAuthUsers()[0].totpSecret).toBe(VALID_SECRET); + }); + + test("attaches USER_TOTP_SECRET to the user account only", () => { + process.env.USER_PASSWORD = "user-secret"; + process.env.USER_TOTP_SECRET = VALID_SECRET; + + const users = getAuthUsers(); + + expect(users.find((u) => u.role === "user")?.totpSecret).toBe(VALID_SECRET); + expect(users.find((u) => u.role === "admin")?.totpSecret).toBeUndefined(); + }); + + test("protects each account independently", () => { + process.env.USER_PASSWORD = "user-secret"; + process.env.ADMIN_TOTP_SECRET = VALID_SECRET; + + const users = getAuthUsers(); + + expect(users.find((u) => u.role === "admin")?.totpSecret).toBe(VALID_SECRET); + expect(users.find((u) => u.role === "user")?.totpSecret).toBeUndefined(); + }); + + test("trims the surrounding whitespace an env file or secret manager leaves behind", () => { + process.env.ADMIN_TOTP_SECRET = ` ${VALID_SECRET}\n`; + + expect(getAuthUsers()[0].totpSecret).toBe(VALID_SECRET); + }); + + test("treats an empty value as no second factor, so MFA can be turned off by blanking it", () => { + process.env.ADMIN_TOTP_SECRET = " "; + + expect(getAuthUsers()[0].totpSecret).toBeUndefined(); + }); + + test("throws AuthConfigError when ADMIN_TOTP_SECRET is not base32", () => { + process.env.ADMIN_TOTP_SECRET = "definitely-not-base32!"; + + expect(() => getAuthUsers()).toThrow(AuthConfigError); + }); + + test("names the offending variable so the operator knows which one to fix", () => { + process.env.USER_PASSWORD = "user-secret"; + process.env.USER_TOTP_SECRET = "0189"; + + expect(() => getAuthUsers()).toThrow(/USER_TOTP_SECRET/); + }); + + test("ignores USER_TOTP_SECRET when there is no user account to protect", () => { + delete process.env.USER_PASSWORD; + // Invalid on purpose: an inert variable must not be able to break the whole login route. + process.env.USER_TOTP_SECRET = "not-base32!"; + + expect(getAuthUsers()).toHaveLength(1); + }); + }); }); diff --git a/tests/unit/lib/totp.test.ts b/tests/unit/lib/totp.test.ts new file mode 100644 index 000000000..ce9f5f7f7 --- /dev/null +++ b/tests/unit/lib/totp.test.ts @@ -0,0 +1,126 @@ +import { afterEach, describe, expect, spyOn, test } from "bun:test"; +import { claimTotpStep, clearTotpReplayState, decodeBase32, TOTP_PERIOD_SECONDS, verifyTotp } from "@/lib/totp"; +import { RFC6238_SECRET as RFC_SECRET, RFC6238_SEED_ASCII } from "../../helpers/rfc6238"; + +/** `T` values from RFC 6238 Appendix B, each paired with the low six digits of its vector. */ +const RFC_VECTORS = [ + { seconds: 59, code: "287082" }, + { seconds: 1111111109, code: "081804" }, + { seconds: 1111111111, code: "050471" }, + { seconds: 1234567890, code: "005924" }, + { seconds: 2000000000, code: "279037" }, +]; + +describe("decodeBase32", () => { + test("decodes the RFC 6238 seed back to its ASCII bytes", () => { + expect(decodeBase32(RFC_SECRET)?.toString("utf8")).toBe(RFC6238_SEED_ASCII); + }); + + test("accepts the spacing, casing and padding operators actually paste", () => { + const spaced = decodeBase32("gezd gnbv-gy3t qojq gezd gnbv gy3t qojq====="); + expect(spaced?.toString("utf8")).toBe(RFC6238_SEED_ASCII); + }); + + test("rejects a secret containing a character outside the base32 alphabet", () => { + expect(decodeBase32("GEZDGNBV1")).toBeNull(); + }); + + test("rejects a secret that is only separators and padding", () => { + expect(decodeBase32(" -- ==")).toBeNull(); + }); + + test("rejects a secret too short to yield a whole byte", () => { + expect(decodeBase32("A")).toBeNull(); + }); +}); + +describe("verifyTotp", () => { + for (const { seconds, code } of RFC_VECTORS) { + test(`accepts the RFC 6238 vector at T=${seconds}`, () => { + expect(verifyTotp(RFC_SECRET, code, seconds * 1000)).toBe(Math.floor(seconds / TOTP_PERIOD_SECONDS)); + }); + } + + test("accepts the previous step, so a slow typist is not punished", () => { + expect(verifyTotp(RFC_SECRET, "287082", (59 + TOTP_PERIOD_SECONDS) * 1000)).toBe(1); + }); + + test("accepts the next step, so a client clock running fast still works", () => { + expect(verifyTotp(RFC_SECRET, "287082", (59 - TOTP_PERIOD_SECONDS) * 1000)).toBe(1); + }); + + test("rejects a code two steps out", () => { + expect(verifyTotp(RFC_SECRET, "287082", (59 + 2 * TOTP_PERIOD_SECONDS) * 1000)).toBeNull(); + }); + + test("tolerates whitespace inside the submitted code", () => { + expect(verifyTotp(RFC_SECRET, " 287 082 ", 59_000)).toBe(1); + }); + + for (const code of ["12345", "1234567", "abcdef", "", "12345a"]) { + test(`rejects the malformed code "${code}"`, () => { + expect(verifyTotp(RFC_SECRET, code, 59_000)).toBeNull(); + }); + } + + test("rejects every code when the configured secret is not valid base32", () => { + expect(verifyTotp("not base32!", "287082", 59_000)).toBeNull(); + }); + + test("reads the wall clock when no timestamp is supplied", () => { + // Stubbed rather than asserted against the real clock: a test that submitted a fixed code at + // the true current time would pass or fail by luck. + const nowSpy = spyOn(Date, "now").mockReturnValue(59_000); + try { + expect(verifyTotp(RFC_SECRET, "287082")).toBe(1); + } finally { + nowSpy.mockRestore(); + } + }); +}); + +describe("claimTotpStep", () => { + afterEach(() => { + clearTotpReplayState(); + }); + + test("accepts a step once and refuses the same step again", () => { + expect(claimTotpStep("account-a", 1, 59_000)).toBe(true); + expect(claimTotpStep("account-a", 1, 59_000)).toBe(false); + }); + + test("scopes spent steps per account", () => { + expect(claimTotpStep("account-a", 1, 59_000)).toBe(true); + expect(claimTotpStep("account-b", 1, 59_000)).toBe(true); + }); + + test("lets the same account spend the next step", () => { + expect(claimTotpStep("account-a", 1, 59_000)).toBe(true); + expect(claimTotpStep("account-a", 2, 89_000)).toBe(true); + }); + + test("forgets a step once its acceptance window has passed", () => { + expect(claimTotpStep("account-a", 1, 59_000)).toBe(true); + // Far enough ahead that verifyTotp could no longer accept step 1 at all, so remembering it + // buys nothing and the entry is reclaimed. + expect(claimTotpStep("account-a", 1, 59_000 + 10 * TOTP_PERIOD_SECONDS * 1000)).toBe(true); + }); + + test("bounds its own memory rather than growing with attacker-supplied keys", () => { + const start = 59_000; + for (let index = 0; index < 5000; index++) claimTotpStep(`account-${index}`, 1, start); + // The earliest entries were evicted, so the very first key is claimable again: the cap held + // and it failed open, exactly as documented. + expect(claimTotpStep("account-0", 1, start)).toBe(true); + }); + + test("reads the wall clock when no timestamp is supplied", () => { + const nowSpy = spyOn(Date, "now").mockReturnValue(59_000); + try { + expect(claimTotpStep("account-a", 1)).toBe(true); + expect(claimTotpStep("account-a", 1)).toBe(false); + } finally { + nowSpy.mockRestore(); + } + }); +}); From 0978773c031f5f5147cd376f1385759f1c53af53 Mon Sep 17 00:00:00 2001 From: cevheri Date: Thu, 10 Sep 2026 21:33:01 +0300 Subject: [PATCH 2/3] fix(auth): reject a TOTP secret too small to be a second factor Review findings on the TOTP work, each measured against a running server. The alphabet was validated and the length was not, so ADMIN_TOTP_SECRET=AA was accepted and signed in on an 8-bit key that a single observed code recovers outright. The module already argues that a malformed secret must fail loudly rather than leave a silently degraded factor; that applies with more force to one carrying no entropy, so it is now an AuthConfigError naming the variable, checked on the decoded bytes. The chart pattern and the app's reader disagreed in four cases. Two of them installed and then took the login route down with a 503, which is the exact outcome the chart README promises the schema prevents. The test now derives its expectation from the app's own reader, so neither side can drift alone. docs/MFA.md contradicted itself on whether these variables apply under OIDC. Verified against a live oidc-mode server: they do. The API route stays guarded and only the login page has no password form to hang the field on. Also: the redundant ternary CodeQL reported as a user-controlled bypass, the login-page doc that still described a two-field form, the systemd env template that gave .deb operators no sign MFA exists, and an eviction comment naming a threat that cannot reach the function it guards. --- .env.example | 6 ++- charts/libredb-studio/README.md | 9 ++-- charts/libredb-studio/values.schema.json | 8 +-- charts/libredb-studio/values.yaml | 3 +- docs/MFA.md | 24 +++++---- docs/ui/login-page.md | 12 ++++- operator/helm-charts/libredb-studio/README.md | 9 ++-- .../libredb-studio/values.schema.json | 8 +-- .../helm-charts/libredb-studio/values.yaml | 3 +- packaging/linux/env | 7 +++ src/app/api/auth/login/route.ts | 7 ++- src/lib/local-auth.ts | 27 ++++++---- src/lib/totp.ts | 24 +++++++-- tests/unit/helm-chart-totp.test.ts | 52 ++++++++++++++----- tests/unit/lib/local-auth.test.ts | 33 ++++++++++++ 15 files changed, 178 insertions(+), 54 deletions(-) diff --git a/.env.example b/.env.example index fad56b98d..c16b588c0 100644 --- a/.env.example +++ b/.env.example @@ -71,8 +71,10 @@ USER_PASSWORD=your_secure_user_password # # Generate one, then enrol it in your app of choice: # openssl rand 20 | base32 | tr -d '=' # 160-bit secret, per RFC 4226 -# A value that is not valid base32 stops login with a clear 503 rather than -# silently disabling the second factor. Blank the variable to turn MFA off. +# Minimum 26 base32 characters, so the decoded secret carries the 128 bits +# RFC 4226 requires. A value that is not base32, or shorter than that, stops +# login with a clear 503 naming the variable rather than silently disabling or +# silently weakening the second factor. Blank the variable to turn MFA off. # ADMIN_TOTP_SECRET=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP # USER_TOTP_SECRET= diff --git a/charts/libredb-studio/README.md b/charts/libredb-studio/README.md index c9051dc09..8b5cba771 100644 --- a/charts/libredb-studio/README.md +++ b/charts/libredb-studio/README.md @@ -153,14 +153,17 @@ Optional, local-provider only, and opt-in per account. Set a base32 secret and t present a 6-digit authenticator code after its password: ```bash -helm upgrade --install libredb libredb/libredb-studio --set secrets.adminPassword=MyAdmin123 --set secrets.adminTotpSecret=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP +helm upgrade --install libredb libredb/libredb-studio \ + --set secrets.adminPassword=MyAdmin123 \ + --set secrets.adminTotpSecret=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP ``` The value travels in the chart's Secret and is referenced from the pod, so it never appears in the Deployment spec - which is why `extraEnv` is the wrong tool for it. Both `ADMIN_TOTP_SECRET` and `USER_TOTP_SECRET` refs are always optional, including in strict mode, so a second factor nobody -asked for can never keep the pod from starting. `values.schema.json` rejects a value that is not -base32 at install time rather than leaving it to fail at the login screen. +asked for can never keep the pod from starting. `values.schema.json` applies the same test the app +does, base32 and at least the 128 bits RFC 4226 requires, so a bad secret fails at install time +rather than at the login screen. Under `authProvider=oidc` the login page shows no password form and MFA belongs to the identity provider - but `POST /api/auth/login` stays reachable whenever `secrets.adminPassword` is also set, diff --git a/charts/libredb-studio/values.schema.json b/charts/libredb-studio/values.schema.json index f7358bb24..534f4bb1a 100644 --- a/charts/libredb-studio/values.schema.json +++ b/charts/libredb-studio/values.schema.json @@ -105,13 +105,13 @@ }, "adminTotpSecret": { "type": "string", - "pattern": "^$|^[A-Za-z2-7][A-Za-z2-7 =-]*$", - "description": "Admin TOTP secret: empty (no second factor) or base32 (RFC 4648: A-Z and 2-7, spacing and = padding allowed)" + "pattern": "^$|^[-\\s]*(?:[A-Za-z2-7][-\\s]*){26,}=*$", + "description": "Admin TOTP secret: empty (no second factor) or base32 (RFC 4648: A-Z and 2-7; spaces, hyphens and trailing = are stripped), at least 26 characters so the decoded secret carries the 128 bits RFC 4226 requires" }, "userTotpSecret": { "type": "string", - "pattern": "^$|^[A-Za-z2-7][A-Za-z2-7 =-]*$", - "description": "User TOTP secret: empty (no second factor) or base32 (RFC 4648: A-Z and 2-7, spacing and = padding allowed)" + "pattern": "^$|^[-\\s]*(?:[A-Za-z2-7][-\\s]*){26,}=*$", + "description": "User TOTP secret: empty (no second factor) or base32 (RFC 4648: A-Z and 2-7; spaces, hyphens and trailing = are stripped), at least 26 characters so the decoded secret carries the 128 bits RFC 4226 requires" }, "llmApiKey": { "type": "string", diff --git a/charts/libredb-studio/values.yaml b/charts/libredb-studio/values.yaml index 81e86618d..16d74a79f 100644 --- a/charts/libredb-studio/values.yaml +++ b/charts/libredb-studio/values.yaml @@ -77,7 +77,8 @@ secrets: # -- Regular user account password. Optional: the non-admin account exists only when set; # it is never generated. userPassword: "" - # -- Base32 TOTP secret for the admin account (RFC 4648: A-Z and 2-7). Optional and + # -- Base32 TOTP secret for the admin account (RFC 4648: A-Z and 2-7), at least 26 characters + # so the decoded secret carries the 128 bits RFC 4226 requires. Optional and # local-provider only: when set, admin login requires a 6-digit authenticator code after the # password. A value that is not base32 stops login with a clear 503 rather than silently # dropping the second factor. Under config.authProvider=oidc the login page shows no password diff --git a/docs/MFA.md b/docs/MFA.md index 428df8525..be0332c43 100644 --- a/docs/MFA.md +++ b/docs/MFA.md @@ -91,10 +91,13 @@ created only once that code verifies. | `USER_TOTP_SECRET` | No | Base32 secret for the optional non-admin account. Inert unless `USER_PASSWORD` is also set — with no password there is no user account to protect. | **Formatting is forgiving, content is not.** Lowercase, spaces, hyphens and `=` padding are all -normalized away, so you can paste a secret exactly as your password manager displays it. A value -containing anything outside the base32 alphabet is a **misconfiguration, not a disabled factor**: -login stops with a `503` naming the offending variable, rather than silently letting the password -through or rejecting every correct code. To turn MFA off, blank or unset the variable. +normalized away, so you can paste a secret exactly as your password manager displays it. Two things +are rejected outright, both as a **misconfiguration, not a disabled factor**: a value containing +anything outside the base32 alphabet, and a value that decodes to fewer than 16 bytes, which is the +128-bit minimum RFC 4226 requires (26 base32 characters; the generator in step 1 gives you 32). +Either one stops login with a `503` naming the offending variable, rather than silently letting the +password through, rejecting every correct code, or leaving a second factor too small to be worth +having. To turn MFA off, blank or unset the variable. Each account is independent — protect the admin and leave an automation-owned user account on a password alone if that is what you need. @@ -202,12 +205,15 @@ function of the current time, and the accepted window is ±30 seconds. Check the **"Invalid authentication code" for a code that just worked.** Each code is single-use. Wait for the next one rather than resubmitting the same digits. -**A 503 naming `ADMIN_TOTP_SECRET` or `USER_TOTP_SECRET`.** The value is not valid base32. Copy it -again from the authenticator app — `0`, `1`, `8` and `9` are not in the alphabet, and a secret -containing them was mistyped. +**A 503 naming `ADMIN_TOTP_SECRET` or `USER_TOTP_SECRET`.** Two causes, and the message says which. +"is not a valid base32 secret" is a typo: `0`, `1`, `8` and `9` are not in the alphabet. "is too +short" means the value decodes to fewer than the 128 bits RFC 4226 requires; generate a fresh one +with the command in step 1 rather than padding the old one out. -**The code field never appears.** The account has no secret configured, or the deployment is running -`NEXT_PUBLIC_AUTH_PROVIDER=oidc`, where these variables are ignored. +**The code field never appears.** The account has no secret configured, or the deployment runs +`NEXT_PUBLIC_AUTH_PROVIDER=oidc`, where the login page renders no password form for the field to +follow. The variables are still honoured in that mode: `POST /api/auth/login` stays guarded, there +is simply no page step to show. See [Running both](#running-both). **Nothing happens after a correct code.** Check for a `429`: the client bucket may have tripped from earlier wrong codes. It clears on its own within the window. diff --git a/docs/ui/login-page.md b/docs/ui/login-page.md index bce00f9d8..fff7ad39d 100644 --- a/docs/ui/login-page.md +++ b/docs/ui/login-page.md @@ -122,6 +122,11 @@ When local auth is active, the right panel shows: 1. **Email/password form** with icon-prefixed inputs 2. **"Sign In" button** — calls `POST /api/auth/login` with JSON body +3. **Authentication code field**, rendered only after the server answers `mfaRequired` for those + credentials, with the button relabelled "Verify code". The form never predicts this: whether an + account carries a second factor is server-side configuration, and deciding it client-side would + publish which accounts are protected. Editing either credential drops back to step 1, so a code + minted for one account is never submitted against another. See [MFA.md](../MFA.md). On successful login, the user is redirected based on their role: - `admin` → `/admin` @@ -130,6 +135,11 @@ On successful login, the user is redirected based on their role: On failure, the form surfaces the API's `message` via a toast instead of a generic error: - Wrong credentials → `"Invalid email or password"` (401). +- TOTP account, no code yet → `"Enter the 6-digit code from your authenticator app"` (401, + `mfaRequired: true`). Deliberately no toast: the field appearing is the message, and an error + toast would frame a normal step of the flow as a failure. +- TOTP account, wrong or replayed code → `"Invalid authentication code"` (401, `mfaRequired: true`), + toasted, and the field is cleared. - Server not configured (missing `ADMIN_PASSWORD`, or a missing/too-short `JWT_SECRET`) → the actionable `AuthConfigError` message (503), e.g. *"Login is unavailable: this server has no administrator password configured. Set @@ -175,7 +185,7 @@ The login page follows the app's premium dark aesthetic: | `src/lib/db-showcase.ts` | Showcase order and the derived engine list | | `src/lib/distribution/channels.generated.ts` | Generated live-channel list (`bun run channels:showcase`) | | `src/lib/agent/engine-support.ts` | The engines the agent card may claim execution on | -| `tests/components/LoginPage.test.tsx` | Component tests — rendering, form submission, OIDC mode, and the pinned agent claim | +| `tests/components/LoginPage.test.tsx` | Component tests — rendering, form submission, OIDC mode, the TOTP step, and the pinned agent claim | | `e2e/login.spec.ts` | Browser assertions that both showcase blocks render every derived entry, desktop and mobile | --- diff --git a/operator/helm-charts/libredb-studio/README.md b/operator/helm-charts/libredb-studio/README.md index c9051dc09..8b5cba771 100644 --- a/operator/helm-charts/libredb-studio/README.md +++ b/operator/helm-charts/libredb-studio/README.md @@ -153,14 +153,17 @@ Optional, local-provider only, and opt-in per account. Set a base32 secret and t present a 6-digit authenticator code after its password: ```bash -helm upgrade --install libredb libredb/libredb-studio --set secrets.adminPassword=MyAdmin123 --set secrets.adminTotpSecret=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP +helm upgrade --install libredb libredb/libredb-studio \ + --set secrets.adminPassword=MyAdmin123 \ + --set secrets.adminTotpSecret=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP ``` The value travels in the chart's Secret and is referenced from the pod, so it never appears in the Deployment spec - which is why `extraEnv` is the wrong tool for it. Both `ADMIN_TOTP_SECRET` and `USER_TOTP_SECRET` refs are always optional, including in strict mode, so a second factor nobody -asked for can never keep the pod from starting. `values.schema.json` rejects a value that is not -base32 at install time rather than leaving it to fail at the login screen. +asked for can never keep the pod from starting. `values.schema.json` applies the same test the app +does, base32 and at least the 128 bits RFC 4226 requires, so a bad secret fails at install time +rather than at the login screen. Under `authProvider=oidc` the login page shows no password form and MFA belongs to the identity provider - but `POST /api/auth/login` stays reachable whenever `secrets.adminPassword` is also set, diff --git a/operator/helm-charts/libredb-studio/values.schema.json b/operator/helm-charts/libredb-studio/values.schema.json index f7358bb24..534f4bb1a 100644 --- a/operator/helm-charts/libredb-studio/values.schema.json +++ b/operator/helm-charts/libredb-studio/values.schema.json @@ -105,13 +105,13 @@ }, "adminTotpSecret": { "type": "string", - "pattern": "^$|^[A-Za-z2-7][A-Za-z2-7 =-]*$", - "description": "Admin TOTP secret: empty (no second factor) or base32 (RFC 4648: A-Z and 2-7, spacing and = padding allowed)" + "pattern": "^$|^[-\\s]*(?:[A-Za-z2-7][-\\s]*){26,}=*$", + "description": "Admin TOTP secret: empty (no second factor) or base32 (RFC 4648: A-Z and 2-7; spaces, hyphens and trailing = are stripped), at least 26 characters so the decoded secret carries the 128 bits RFC 4226 requires" }, "userTotpSecret": { "type": "string", - "pattern": "^$|^[A-Za-z2-7][A-Za-z2-7 =-]*$", - "description": "User TOTP secret: empty (no second factor) or base32 (RFC 4648: A-Z and 2-7, spacing and = padding allowed)" + "pattern": "^$|^[-\\s]*(?:[A-Za-z2-7][-\\s]*){26,}=*$", + "description": "User TOTP secret: empty (no second factor) or base32 (RFC 4648: A-Z and 2-7; spaces, hyphens and trailing = are stripped), at least 26 characters so the decoded secret carries the 128 bits RFC 4226 requires" }, "llmApiKey": { "type": "string", diff --git a/operator/helm-charts/libredb-studio/values.yaml b/operator/helm-charts/libredb-studio/values.yaml index 81e86618d..16d74a79f 100644 --- a/operator/helm-charts/libredb-studio/values.yaml +++ b/operator/helm-charts/libredb-studio/values.yaml @@ -77,7 +77,8 @@ secrets: # -- Regular user account password. Optional: the non-admin account exists only when set; # it is never generated. userPassword: "" - # -- Base32 TOTP secret for the admin account (RFC 4648: A-Z and 2-7). Optional and + # -- Base32 TOTP secret for the admin account (RFC 4648: A-Z and 2-7), at least 26 characters + # so the decoded secret carries the 128 bits RFC 4226 requires. Optional and # local-provider only: when set, admin login requires a 6-digit authenticator code after the # password. A value that is not base32 stops login with a clear 503 rather than silently # dropping the second factor. Under config.authProvider=oidc the login page shows no password diff --git a/packaging/linux/env b/packaging/linux/env index 16b5ed68b..a6dd396a3 100644 --- a/packaging/linux/env +++ b/packaging/linux/env @@ -20,6 +20,13 @@ #USER_EMAIL=user@libredb.org #USER_PASSWORD= # +# Two-factor authentication (TOTP), optional and per account. Base32, at least +# 26 characters; generate one with: openssl rand 20 | base32 +# A value that is not base32, or too short, stops login with a 503 naming the +# variable. Enrolment and recovery: docs/MFA.md +#ADMIN_TOTP_SECRET= +#USER_TOTP_SECRET= +# # AI query assistance (optional). #LLM_PROVIDER=gemini #LLM_API_KEY= diff --git a/src/app/api/auth/login/route.ts b/src/app/api/auth/login/route.ts index f595e925e..30228fb00 100644 --- a/src/app/api/auth/login/route.ts +++ b/src/app/api/auth/login/route.ts @@ -137,7 +137,12 @@ export async function POST(request: NextRequest) { // caller who can reach this branch necessarily holds a working password for the account - // without MFA configured, that same request would simply have logged them in. if (matched?.totpSecret) { - const step = submittedTotp ? verifyTotp(matched.totpSecret, submittedTotp) : null; + // No "did they send one?" pre-check: verifyTotp's own digit test already answers null for + // an empty or malformed code, so a guard here would be a second copy of that decision - and + // a user-controlled value guarding the session mint is what CodeQL flags as + // js/user-controlled-bypass. `submittedTotp` still picks the audit reason below, which is a + // logging branch and decides nothing. + const step = verifyTotp(matched.totpSecret, submittedTotp); // claimTotpStep is what makes an accepted code single-use (RFC 6238 §5.2). A replayed code // verifies but fails to claim, and lands here as an ordinary bad code. const accepted = step !== null && claimTotpStep(accountKey, step); diff --git a/src/lib/local-auth.ts b/src/lib/local-auth.ts index 1a0ba394b..d4d64e036 100644 --- a/src/lib/local-auth.ts +++ b/src/lib/local-auth.ts @@ -5,7 +5,7 @@ */ import type { Role } from "@/lib/auth"; import { AuthConfigError } from "@/lib/auth-errors"; -import { decodeBase32 } from "@/lib/totp"; +import { decodeBase32, TOTP_MIN_SECRET_BYTES } from "@/lib/totp"; export interface AuthUser { email: string; @@ -28,23 +28,32 @@ const ADMIN_PASSWORD_MISSING_MESSAGE = const TOTP_SECRET_INVALID_HINT = "is not a valid base32 secret. Copy the secret exactly as your authenticator app shows it (letters A-Z and digits 2-7 only) and restart the server."; +// Same shape, for a secret that decodes cleanly but carries too little entropy to be a factor. +const TOTP_SECRET_SHORT_HINT = `is too short: RFC 4226 requires a shared secret of at least ${TOTP_MIN_SECRET_BYTES * 8} bits. Generate one with "openssl rand 20 | base32" and restart the server.`; + /** - * Read one account's TOTP secret, rejecting a secret that could never verify anything. + * Read one account's TOTP secret, rejecting a secret that could never protect anything. * - * A malformed secret has to be fatal rather than ignored, and it has to fail loudly rather than - * silently: ignoring it would drop the second factor without telling anyone, and accepting it - * would refuse every correct code the operator's phone produces. Neither failure is one the - * operator could diagnose from a "Invalid email or password" screen, so this becomes an - * AuthConfigError and the login route renders its message as a 503. + * Two ways a configured value is wrong, and both have to be fatal rather than ignored. A + * malformed secret would refuse every correct code the operator's phone produces; a secret below + * the RFC's minimum would accept them all while being worth almost nothing. Neither is + * diagnosable from an "Invalid email or password" screen, and silently dropping the factor in + * either case would leave a deployment that believes it has MFA and does not. So both become an + * AuthConfigError and the login route renders the message as a 503 naming the variable. * - * @throws {AuthConfigError} when the variable is set to something that is not base32. + * @throws {AuthConfigError} when the variable is not base32, or decodes below the RFC minimum. */ function readTotpSecret(variable: string): string | undefined { const raw = process.env[variable]?.trim(); // Unset and empty are the same answer — no second factor — so an operator can disable MFA by // blanking the variable rather than having to unset it, which some orchestrators cannot do. if (!raw) return undefined; - if (!decodeBase32(raw)) throw new AuthConfigError(`Login is unavailable: ${variable} ${TOTP_SECRET_INVALID_HINT}`); + const decoded = decodeBase32(raw); + if (!decoded) throw new AuthConfigError(`Login is unavailable: ${variable} ${TOTP_SECRET_INVALID_HINT}`); + // Measured on the decoded bytes, never on the pasted string: spaces, hyphens and `=` padding are + // presentation that decodeBase32 strips, so a grouped short secret must not read as long enough. + if (decoded.length < TOTP_MIN_SECRET_BYTES) + throw new AuthConfigError(`Login is unavailable: ${variable} ${TOTP_SECRET_SHORT_HINT}`); return raw; } diff --git a/src/lib/totp.ts b/src/lib/totp.ts index 921cb78be..a689174b6 100644 --- a/src/lib/totp.ts +++ b/src/lib/totp.ts @@ -35,6 +35,19 @@ const TOTP_WINDOW_STEPS = 1; const BASE32_ALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567"; +/** + * Smallest shared secret that may be configured, in decoded bytes. RFC 4226 R6 makes 128 bits a + * MUST and RFC 6238 inherits it. + * + * `decodeBase32` only rejects a secret with no whole byte in it, which is a far lower bar than the + * RFC's: `AA` decodes to one byte and, without this, is a working second factor worth 8 bits — one + * observed code narrows it to a single candidate. That failure is invisible, because every screen + * and every doc still says the account has MFA. Enforced by the caller that reads the operator's + * value (src/lib/local-auth.ts) rather than by verifyTotp, so it surfaces once as a configuration + * error naming the variable instead of as a rejected code on every login. + */ +export const TOTP_MIN_SECRET_BYTES = 16; + /** Matches a submitted code once its whitespace has been stripped. */ const CODE_PATTERN = new RegExp(`^\\d{${TOTP_DIGITS}}$`); @@ -138,9 +151,14 @@ export function verifyTotp(secret: string, code: string, now: number = Date.now( const spentSteps = new Map(); /** - * Generous, and bounded only so that a flood of unknown account keys cannot grow this map without - * limit. Each entry is a short string and a number. Eviction fails OPEN (an evicted pair becomes - * replayable) because a replay guard must never become the reason a legitimate login is refused. + * A ceiling on the map, not a defence against one. + * + * Nothing an attacker sends can grow this: claimTotpStep is reached only after a code verifies + * against a configured account, and the local provider defines at most two of those, so pruning + * by expiry alone holds the map at a handful of entries. The cap exists for the shape this module + * would take if accounts ever became data rather than environment. Eviction fails OPEN (an + * evicted pair becomes replayable) because a replay guard must never be the reason a legitimate + * login is refused. */ const MAX_SPENT_ENTRIES = 4096; diff --git a/tests/unit/helm-chart-totp.test.ts b/tests/unit/helm-chart-totp.test.ts index 54d0578a7..23a9426e3 100644 --- a/tests/unit/helm-chart-totp.test.ts +++ b/tests/unit/helm-chart-totp.test.ts @@ -14,6 +14,7 @@ import { describe, expect, test } from "bun:test"; import { join } from "node:path"; import { parseAllDocuments } from "yaml"; import { RFC6238_SECRET } from "../helpers/rfc6238"; +import { decodeBase32, TOTP_MIN_SECRET_BYTES } from "@/lib/totp"; const CHART_DIR = join(import.meta.dir, "../../charts/libredb-studio"); const RELEASE = "release-under-test"; @@ -116,18 +117,43 @@ describe("charts/libredb-studio TOTP second factor", () => { expect(envVar(env, "USER_TOTP_SECRET")?.valueFrom?.secretKeyRef?.name).toBe("byo-auth"); }); - test("rejects a secret that is not base32 at install time, not at first login", () => { - // values.schema.json is the only place this can be caught before the pod runs; without it - // the operator learns about the typo from a 503 on the login screen. - const run = Bun.spawnSync( - ["helm", "template", RELEASE, CHART_DIR, "--set", "secrets.adminTotpSecret=not-base32!"], - { - stdout: "pipe", - stderr: "pipe", - }, - ); - - expect(run.exitCode).not.toBe(0); - expect(run.stderr.toString()).toContain("adminTotpSecret"); + /** + * values.schema.json is the only place a bad secret can be caught before the pod runs, and the + * chart README promises exactly that. The promise only holds while the pattern and the app's + * own reader agree, and they did not: `AB=CD` and a single `A` passed the schema and then took + * the login route down with a 503, while a hyphen-grouped or space-prefixed secret the app + * accepts happily was refused at install. + * + * So the expectation is computed from the app's reader rather than written down beside it. + * Either side changing alone shows up here as a failure instead of as an operator's 503. + */ + describe("the install-time check agrees with the app's own reader", () => { + const CASES = [ + RFC6238_SECRET, + RFC6238_SECRET.toLowerCase(), + RFC6238_SECRET.replace(/(.{4})/g, "$1 ").trim(), + `${RFC6238_SECRET}====`, + `-${RFC6238_SECRET}`, + ` ${RFC6238_SECRET}`, + "not-base32!", + "AB=CD", + "A", + RFC6238_SECRET.slice(0, 25), + ]; + + for (const value of CASES) { + const decoded = decodeBase32(value); + const appAccepts = decoded !== null && decoded.length >= TOTP_MIN_SECRET_BYTES; + + test(`${appAccepts ? "installs" : "refuses"} ${JSON.stringify(value)}`, () => { + const run = Bun.spawnSync( + ["helm", "template", RELEASE, CHART_DIR, "--set-string", `secrets.adminTotpSecret=${value}`], + { stdout: "pipe", stderr: "pipe" }, + ); + + expect(run.exitCode === 0).toBe(appAccepts); + if (!appAccepts) expect(run.stderr.toString()).toContain("adminTotpSecret"); + }); + } }); }); diff --git a/tests/unit/lib/local-auth.test.ts b/tests/unit/lib/local-auth.test.ts index 26bcc978e..c2dbddee9 100644 --- a/tests/unit/lib/local-auth.test.ts +++ b/tests/unit/lib/local-auth.test.ts @@ -132,6 +132,39 @@ describe("local-auth getAuthUsers()", () => { expect(getAuthUsers()[0].totpSecret).toBeUndefined(); }); + /** + * RFC 4226 R6 makes 128 bits a MUST, and a secret below it is not a weaker second factor but + * an absent one: a single observed code narrows an 8-bit key to one candidate. The alphabet + * check alone let `AA` through, which reads as MFA everywhere in the UI and the docs while + * costing an attacker nothing. Sliced off the RFC seed so the boundary is unmistakable. + */ + test("rejects a secret below the 128 bits RFC 4226 requires", () => { + process.env.ADMIN_TOTP_SECRET = VALID_SECRET.slice(0, 25); // 125 bits -> 15 whole bytes + + expect(() => getAuthUsers()).toThrow(AuthConfigError); + }); + + test("says the secret is too short rather than repeating the base32 hint", () => { + process.env.ADMIN_TOTP_SECRET = VALID_SECRET.slice(0, 25); + + expect(() => getAuthUsers()).toThrow(/ADMIN_TOTP_SECRET is too short/); + }); + + test("accepts a secret of exactly the minimum length", () => { + process.env.ADMIN_TOTP_SECRET = VALID_SECRET.slice(0, 26); // 130 bits -> 16 whole bytes + + expect(getAuthUsers()[0].totpSecret).toBe(VALID_SECRET.slice(0, 26)); + }); + + test("measures the decoded length, not the pasted one, so grouping does not fake it", () => { + // 16 base32 characters is 10 bytes however it is spaced out; the separators are not payload. + process.env.ADMIN_TOTP_SECRET = VALID_SECRET.slice(0, 16) + .replace(/(.{4})/g, "$1 ") + .trim(); + + expect(() => getAuthUsers()).toThrow(/ADMIN_TOTP_SECRET is too short/); + }); + test("throws AuthConfigError when ADMIN_TOTP_SECRET is not base32", () => { process.env.ADMIN_TOTP_SECRET = "definitely-not-base32!"; From 21e57e63ba7f7ca579f7e05d5a57dc5d25983fbe Mon Sep 17 00:00:00 2001 From: cevheri Date: Thu, 10 Sep 2026 22:27:18 +0300 Subject: [PATCH 3/3] docs(auth): stop publishing a usable TOTP example secret The example was the canonical secret from the otpauth documentation, so a deployment that uncommented the line and forgot to replace it would read as protected on every screen while anyone could compute its codes. That is worse than having no second factor, because it also passes an audit. Nothing in the code ever defaults to it, and the line is commented, so this is a human-error surface rather than a live hole; it is still the one value that must not be left in place. .env.example now carries a placeholder that is not base32, matching every other secret in that file, so pasting it verbatim earns the 503 this feature already raises instead of quietly installing a published factor. docs/MFA.md and both chart READMEs generate the secret into a shell variable rather than printing one, which is what the real workflow looks like anyway: generate, enrol, deploy. The otpauth URI was the most exposed of the four, since it can be scanned straight into a phone. Also records that the client rate-limit bucket is keyed on the address, not the account. Measured: five wrong codes for the admin, then a correct password for the other account from that same address, answers 429 for the rest of the window. That is exactly the admin-plus-automation split this feature invites, and behind one NAT the two share the budget. --- .env.example | 7 ++++- charts/libredb-studio/README.md | 9 +++++- docs/MFA.md | 29 +++++++++++++++---- operator/helm-charts/libredb-studio/README.md | 9 +++++- 4 files changed, 45 insertions(+), 9 deletions(-) diff --git a/.env.example b/.env.example index c16b588c0..9794cbc0a 100644 --- a/.env.example +++ b/.env.example @@ -75,7 +75,12 @@ USER_PASSWORD=your_secure_user_password # RFC 4226 requires. A value that is not base32, or shorter than that, stops # login with a clear 503 naming the variable rather than silently disabling or # silently weakening the second factor. Blank the variable to turn MFA off. -# ADMIN_TOTP_SECRET=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP +# +# The placeholder below is deliberately not base32, like every other secret in +# this file. A published example secret is the one value that must never be left +# in place: the account would read as protected everywhere while anyone could +# compute its codes. Uncommented as-is, this earns the 503 instead. +# ADMIN_TOTP_SECRET=your_base32_secret_from_the_command_above # USER_TOTP_SECRET= # JWT Secret for session management (min 32 characters) diff --git a/charts/libredb-studio/README.md b/charts/libredb-studio/README.md index 8b5cba771..a53b8b6e6 100644 --- a/charts/libredb-studio/README.md +++ b/charts/libredb-studio/README.md @@ -153,11 +153,18 @@ Optional, local-provider only, and opt-in per account. Set a base32 secret and t present a 6-digit authenticator code after its password: ```bash +# Generate it, enrol the printed value in your authenticator app, then install. +ADMIN_TOTP_SECRET="$(openssl rand 20 | base32 | tr -d '=')" +echo "$ADMIN_TOTP_SECRET" + helm upgrade --install libredb libredb/libredb-studio \ --set secrets.adminPassword=MyAdmin123 \ - --set secrets.adminTotpSecret=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP + --set secrets.adminTotpSecret="$ADMIN_TOTP_SECRET" ``` +No example secret is printed here on purpose. One that looks real invites being copied and left in +place, and a second factor whose secret is published is worse than none. + The value travels in the chart's Secret and is referenced from the pod, so it never appears in the Deployment spec - which is why `extraEnv` is the wrong tool for it. Both `ADMIN_TOTP_SECRET` and `USER_TOTP_SECRET` refs are always optional, including in strict mode, so a second factor nobody diff --git a/docs/MFA.md b/docs/MFA.md index be0332c43..26335ff45 100644 --- a/docs/MFA.md +++ b/docs/MFA.md @@ -35,10 +35,15 @@ A TOTP secret is base32 (RFC 4648: the letters `A`–`Z` and the digits `2`–`7 RFC 4226 recommends: ```bash -openssl rand 20 | base32 | tr -d '=' -# => JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP +ADMIN_TOTP_SECRET="$(openssl rand 20 | base32 | tr -d '=')" +echo "$ADMIN_TOTP_SECRET" ``` +Keep that shell open: every step below refers to `$ADMIN_TOTP_SECRET`, so the value never has to be +pasted anywhere it can be forgotten. This page prints no example secret on purpose. A real-looking +one invites being copied and left in place, and a second factor whose secret is published is worse +than none: every screen still says the account is protected while anyone can generate its codes. + `base32` comes with GNU coreutils. On a machine without it, any authenticator app can generate a secret for you — create a manual entry and copy the key it shows. @@ -48,9 +53,13 @@ secret for you — create a manual entry and copy the key it shows. NEXT_PUBLIC_AUTH_PROVIDER=local ADMIN_EMAIL=admin@libredb.org ADMIN_PASSWORD=your_secure_admin_password -ADMIN_TOTP_SECRET=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP +ADMIN_TOTP_SECRET=the_secret_you_generated_in_step_1 ``` +Both values above are placeholders, and neither is valid for its variable: the TOTP one is not +base32, so a file left exactly like this stops login with a `503` naming the variable rather than +quietly installing a factor anyone can compute. + Restart the server. The variable is read per login attempt, so nothing is cached across a restart. ### 3. Enrol the secret in your authenticator @@ -73,7 +82,7 @@ Studio does not mint one, because doing so would mean the server handing the sha over HTTP after startup: ``` -otpauth://totp/LibreDB%20Studio:admin@libredb.org?secret=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP&issuer=LibreDB%20Studio +otpauth://totp/LibreDB%20Studio:admin@libredb.org?secret=YOUR_SECRET_HERE&issuer=LibreDB%20Studio ``` ### 4. Sign in @@ -122,7 +131,7 @@ you configured there. Two ways to close it, and they compose: docker run -d -p 3000:3000 \ -e JWT_SECRET="$(openssl rand -base64 32)" \ -e ADMIN_PASSWORD=your_secure_admin_password \ - -e ADMIN_TOTP_SECRET=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP \ + -e ADMIN_TOTP_SECRET="$ADMIN_TOTP_SECRET" \ ghcr.io/libredb/libredb-studio:latest ``` @@ -139,7 +148,7 @@ never appears in the Deployment spec: ```bash helm install libredb-studio oci://ghcr.io/libredb/charts/libredb-studio \ --set secrets.adminPassword=MyAdmin123 \ - --set secrets.adminTotpSecret=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP + --set secrets.adminTotpSecret="$ADMIN_TOTP_SECRET" ``` Bringing your own Secret works too — add the keys `admin-totp-secret` and `user-totp-secret` @@ -190,6 +199,14 @@ Submitting a **wrong** code does count, against both login buckets: `RATE_LIMIT_ Guessing a 6-digit code is therefore bounded to a few dozen tries per window against roughly a million values. +The client bucket is keyed on the **address**, not the account, so wrong codes spend a budget the +whole address shares. Measured: five wrong codes for the admin from one address, then a correct +password for the *other* account from that same address, answers `429` for the rest of the window. +This matters for the common split of protecting the admin and leaving an automation-owned account +on a password alone: behind one NAT or one ingress they draw on the same bucket, so an admin whose +phone clock has drifted can stall the automation for five minutes. Give the automation its own +egress address, or raise `RATE_LIMIT_LOGIN_MAX`, if that coupling is unacceptable. + Locked out of your own account? The secret is an environment variable, so recovery is the same as for a lost password: blank `ADMIN_TOTP_SECRET` and restart. There are no recovery codes, and none are needed — whoever can restart the server already holds the stronger credential. diff --git a/operator/helm-charts/libredb-studio/README.md b/operator/helm-charts/libredb-studio/README.md index 8b5cba771..a53b8b6e6 100644 --- a/operator/helm-charts/libredb-studio/README.md +++ b/operator/helm-charts/libredb-studio/README.md @@ -153,11 +153,18 @@ Optional, local-provider only, and opt-in per account. Set a base32 secret and t present a 6-digit authenticator code after its password: ```bash +# Generate it, enrol the printed value in your authenticator app, then install. +ADMIN_TOTP_SECRET="$(openssl rand 20 | base32 | tr -d '=')" +echo "$ADMIN_TOTP_SECRET" + helm upgrade --install libredb libredb/libredb-studio \ --set secrets.adminPassword=MyAdmin123 \ - --set secrets.adminTotpSecret=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP + --set secrets.adminTotpSecret="$ADMIN_TOTP_SECRET" ``` +No example secret is printed here on purpose. One that looks real invites being copied and left in +place, and a second factor whose secret is published is worse than none. + The value travels in the chart's Secret and is referenced from the pod, so it never appears in the Deployment spec - which is why `extraEnv` is the wrong tool for it. Both `ADMIN_TOTP_SECRET` and `USER_TOTP_SECRET` refs are always optional, including in strict mode, so a second factor nobody