fix(access-review): verify the 2FA filter, and purge stale TOTP rows on rotation #431

Sammanfogat

supernaut sammanfogade 2 incheckningar från fix/access-review-verify-2fa-filter in i main

2026-08-17 10:44:20 +00:00

supernaut kommenterad

2026-08-16 18:01:52 +00:00

Ägare

Two of the four prevention items on #292. Two commits, unrelated to each other except by that
issue.

1. The access review verified its own filter

access-review-2fa.py derives the whole 2FA column from a query parameter, and Forgejo ignores
query parameters it does not recognise rather than rejecting them.

So if is_2fa_enabled is ever not honoured, the filtered call returns every user and the report
marks the entire estate as 2FA-enabled. The failure is invisible in the output: a clean, confident,
entirely false access review, submitted as ISO 27001 evidence for bitborg-docs#30.

Both halves are now fetched and checked against the unfiltered list. They must partition it exactly,
no account in both and none in neither. Otherwise the script exits with the counts rather than
rendering a fabricated column.

What prompted it: the swagger checked into the workspace is 15.0.3+gitea-1.22.0 while
production runs v16, and it documents no is_2fa_enabled parameter and no 2FA endpoints at all.
The filter is probably fine on v16. But "probably fine" and "verified" are different claims for
something that goes into an audit, and my read-only token could not settle it either way.

Covered by a test inducing three cases:

PASS caught ignored filter -> error: the `is_2fa_enabled` filter is not being applied ...
PASS working filter accepted; with 2FA: 2 of 5
PASS caught overlapping lists

2. Purge stale TOTP rows as part of rotating

The rotation section enumerated stale enrolments but left purging them as a later exercise, so a
rotation could complete leaving a live lockout for whoever signs in next. Adds the DELETE
alongside the SELECT.

Adds a pre-rotation guard that was missing entirely: there is no API to reset 2FA, only
forgejo admin user reset-mfa on the host or the admin web UI, and that UI sits behind the very
2FA the rotation breaks. Without a non-TOTP factor on at least one admin, a rotation can lock out
the only account able to repair it.

Records what survives, verified against the v16.0.2 source rather than assumed. keying appears
on three lines of models/auth/twofactor.go, VerifyScratchToken is plain PBKDF2, and
models/auth/webauthn.go has no keying call. So scratch codes, security keys, PATs, SSH keys and
re-enrolment all still work; only the TOTP secret is lost. The existing triage treats the account as
fully locked out, which overstates it and hides the ways back in.

Also notes that over HTTP Basic the breakage surfaces as a 401, not a 500, so a git-over-HTTPS
report reads as bad credentials; and that rotation is unsupported upstream and unscheduled, with the
issue numbers, so nobody waits for a fix that is not coming.

What is NOT in this PR, and why

#292's remaining item asks the access review to flag native 2FA enrolments predating the current
SECRET_KEY. That is not implementable through the admin API:

the User model exposes no 2FA field at all;
there are no 2FA or WebAuthn API paths;
nothing exposes an enrolment timestamp.

The timestamp exists only in the database (two_factor.created_unix), which is why the runbook's
procedure is a psql query rather than a script. Building a weaker API-based approximation and
calling the box ticked would be worse than saying so. The runbook change above covers the same
ground operationally.

The other two items are decisions rather than code: whether to prevent native 2FA enrolment
outright, and whether to report the 500 upstream. Left for #292.

Verification

./scripts/ruff.sh clean (pinned container).
pnpm mdlint and pnpm format:check clean.
The guard test above runs the real collect() against injected API responses.

Two of the four prevention items on #292. Two commits, unrelated to each other except by that issue. ## 1. The access review verified its own filter `access-review-2fa.py` derives the whole 2FA column from a **query parameter**, and Forgejo ignores query parameters it does not recognise rather than rejecting them. So if `is_2fa_enabled` is ever not honoured, the filtered call returns **every** user and the report marks the entire estate as 2FA-enabled. The failure is invisible in the output: a clean, confident, entirely false access review, submitted as ISO 27001 evidence for bitborg-docs#30. Both halves are now fetched and checked against the unfiltered list. They must partition it exactly, no account in both and none in neither. Otherwise the script exits with the counts rather than rendering a fabricated column. **What prompted it:** the swagger checked into the workspace is `15.0.3+gitea-1.22.0` while production runs v16, and it documents **no** `is_2fa_enabled` parameter and no 2FA endpoints at all. The filter is probably fine on v16. But "probably fine" and "verified" are different claims for something that goes into an audit, and my read-only token could not settle it either way. Covered by a test inducing three cases: ``` PASS caught ignored filter -> error: the `is_2fa_enabled` filter is not being applied ... PASS working filter accepted; with 2FA: 2 of 5 PASS caught overlapping lists ``` ## 2. Purge stale TOTP rows as part of rotating The rotation section enumerated stale enrolments but left purging them as a later exercise, so a rotation could complete leaving a live lockout for whoever signs in next. Adds the `DELETE` alongside the `SELECT`. Adds a **pre-rotation guard that was missing entirely**: there is no API to reset 2FA, only `forgejo admin user reset-mfa` on the host or the admin web UI, and that UI sits behind the very 2FA the rotation breaks. Without a non-TOTP factor on at least one admin, a rotation can lock out the only account able to repair it. Records what **survives**, verified against the v16.0.2 source rather than assumed. `keying` appears on three lines of `models/auth/twofactor.go`, `VerifyScratchToken` is plain PBKDF2, and `models/auth/webauthn.go` has no keying call. So scratch codes, security keys, PATs, SSH keys and re-enrolment all still work; only the TOTP secret is lost. The existing triage treats the account as fully locked out, which overstates it and hides the ways back in. Also notes that over HTTP Basic the breakage surfaces as a **401, not a 500**, so a git-over-HTTPS report reads as bad credentials; and that rotation is unsupported upstream and unscheduled, with the issue numbers, so nobody waits for a fix that is not coming. ## What is NOT in this PR, and why #292's remaining item asks the access review to **flag native 2FA enrolments predating the current `SECRET_KEY`**. That is not implementable through the admin API: - the `User` model exposes no 2FA field at all; - there are no 2FA or WebAuthn API paths; - nothing exposes an enrolment timestamp. The timestamp exists only in the database (`two_factor.created_unix`), which is why the runbook's procedure is a `psql` query rather than a script. Building a weaker API-based approximation and calling the box ticked would be worse than saying so. The runbook change above covers the same ground operationally. The other two items are decisions rather than code: whether to prevent native 2FA enrolment outright, and whether to report the 500 upstream. Left for #292. ## Verification - `./scripts/ruff.sh` clean (pinned container). - `pnpm mdlint` and `pnpm format:check` clean. - The guard test above runs the real `collect()` against injected API responses.

supernaut lade till 2 incheckningar

2026-08-16 18:01:52 +00:00

fix(access-review): verify the 2FA filter instead of trusting it 8f0b5ad158

The 2FA column is derived from a query parameter, and Forgejo ignores query
parameters it does not recognise rather than rejecting them.

So if `is_2fa_enabled` is ever not honoured — an older instance, an upstream
rename, a typo here — the filtered call returns EVERY user and the report marks
the whole estate as 2FA-enabled. The failure is invisible in the output: a
clean, confident, entirely false access review, submitted as ISO 27001 evidence
for bitborg-docs#30.

Both halves of the filter are now fetched and checked against the unfiltered
list. They must partition it exactly, with no account in both lists and none in
neither. If they do not, the script exits with the three counts rather than
rendering a fabricated column.

Prompted by finding that the swagger checked into the workspace is v15.0.3 while
production runs v16, and that it documents no `is_2fa_enabled` parameter and no
2FA endpoints at all. The filter is probably fine on v16, but "probably fine"
and "verified" are different claims for evidence that goes into an audit.

Covered by a test that induces the ignored-filter case, the working case and a
partial-overlap case.

docs(runbook): purge stale TOTP rows as part of rotating, and record what survives

ci / ci (pull_request) Successful in 1m11s

Detaljer

872a83d2e5

The rotation section enumerated stale enrolments but left purging them as a
later exercise, so a rotation could complete leaving a live lockout for whoever
signed in next. The ciphertext is unrecoverable and native TOTP is redundant
under SSO-only, so the row has no value. Adds the DELETE alongside the SELECT.

Adds a pre-rotation guard that was missing entirely: there is NO API to reset
2FA, only the host CLI or the admin web UI, and that UI sits behind the very 2FA
the rotation breaks. Without a non-TOTP factor on at least one admin, a rotation
can lock out the only account able to repair it.

Records what SURVIVES, verified against the v16.0.2 source rather than assumed.
keying appears on three lines of models/auth/twofactor.go, VerifyScratchToken is
plain PBKDF2, and models/auth/webauthn.go has no keying call. So scratch codes,
security keys, PATs, SSH keys and re-enrolment all still work. Only the TOTP
secret is lost. Triage currently treats the account as fully locked out, which
overstates it and hides the ways back in.

Also notes that over HTTP Basic the same breakage surfaces as a 401 rather than
a 500, so a git-over-HTTPS report reads as bad credentials.

Finally records that rotation is unsupported upstream and unscheduled, with the
issue numbers, so nobody waits for a fix that is not coming.

supernaut refererade till denna ändringsförfrågan

2026-08-16 18:02:23 +00:00

forgejo: stale native TOTP enrolments become a 500 lockout after SECRET_KEY rotation #292

supernaut tvångsskickade fix/access-review-verify-2fa-filter från 872a83d2e5

ci / ci (pull_request) Successful in 1m11s

Detaljer

till 28353e9f63

ci / ci (pull_request) Successful in 1m8s

Detaljer

2026-08-17 10:04:05 +00:00

Jämför

supernaut sammanfogade incheckning 53136e5ae2 till main