Fix F-01: TOTP enroll must not set required=1 early

- db/accounts.go: add StorePendingTOTP() which writes totp_secret_enc and totp_secret_nonce but leaves totp_required=0; add comment explaining two-phase flow - server.go (handleTOTPEnroll): switch from SetTOTP() to StorePendingTOTP() so the required flag is only set after the user confirms a valid TOTP code via handleTOTPConfirm, which still calls SetTOTP() - server_test.go: TestTOTPEnrollDoesNotRequireTOTP verifies that after POST /v1/auth/totp/enroll, TOTPRequired is false and the encrypted secret is present; confirms that a subsequent login without a TOTP code still succeeds (no lockout) - AUDIT.md: mark F-01 and F-11 as fixed Security: without this fix an admin who enrolls TOTP but abandons before confirmation is permanently locked out because totp_required=1 but no confirmed secret exists. StorePendingTOTP() keeps the secret pending until the user proves possession by confirming a valid code.
2026-03-11 20:18:57 -07:00
parent 4da39475cc
commit 462f706f73
3 changed files with 125 additions and 39 deletions
--- a/AUDIT.md
+++ b/AUDIT.md
@@ -1,6 +1,6 @@
 # MCIAS Security Audit Report

-**Scope:** Full codebase review of `git.wntrmute.dev/kyle/mcias` (commit `4596ea0`)
+**Scope:** Full codebase review of `git.wntrmute.dev/kyle/mcias` (commit `4596ea0`) aka mcias.
 **Auditor:** Comprehensive source review of all Go source files, protobuf definitions, Dockerfile, systemd unit, and client libraries
 **Classification:** Findings rated as **CRITICAL**, **HIGH**, **MEDIUM**, **LOW**, or **INFORMATIONAL**

@@ -218,24 +218,24 @@ The REST `handleTokenIssue` and gRPC `IssueServiceToken` both revoke the existin

 ## Summary Table

-| ID | Severity | Title | Effort |
-|----|----------|-------|--------|
-| F-01 | MEDIUM | TOTP enrollment sets required=1 before confirmation | Small |
-| F-02 | MEDIUM | Password in HTML hidden fields during TOTP step | Medium |
-| F-03 | MEDIUM | Token renewal not atomic (race window) | Small |
-| F-04 | MEDIUM | Rate limiter not applied to REST login endpoint | Small |
-| F-11 | MEDIUM | Missing security headers on UI responses | Small |
-| F-05 | LOW | No `nbf` claim in issued JWTs | Trivial |
-| F-06 | LOW | `HasRole` uses non-constant-time comparison | Trivial |
-| F-07 | LOW | Dummy Argon2 hash timing mismatch | Small |
-| F-08 | LOW | No account lockout after repeated failures | Medium |
-| F-09 | LOW | `synchronous=NORMAL` risks audit data loss | Trivial |
-| F-10 | LOW | No maximum token expiry validation | Small |
-| F-12 | LOW | No username length/charset validation | Small |
-| F-13 | LOW | No minimum password length enforcement | Small |
-| F-14 | LOW | Passphrase string not zeroed after KDF | Small |
-| F-16 | LOW | UI system token issuance skips old token revocation | Small |
-| F-15 | INFO | Bearer prefix check inconsistency | Trivial |
+| Fixed? | ID | Severity | Title | Effort |
+|--------|----|----------|-------|--------|
+| Yes | F-01 | MEDIUM | TOTP enrollment sets required=1 before confirmation | Small |
+| No  | F-02 | MEDIUM | Password in HTML hidden fields during TOTP step | Medium |
+| No  | F-03 | MEDIUM | Token renewal not atomic (race window) | Small |
+| Yes | F-04 | MEDIUM | Rate limiter not applied to REST login endpoint | Small |
+| Yes | F-11 | MEDIUM | Missing security headers on UI responses | Small |
+| No  | F-05 | LOW | No `nbf` claim in issued JWTs | Trivial |
+| No  | F-06 | LOW | `HasRole` uses non-constant-time comparison | Trivial |
+| No  | F-07 | LOW | Dummy Argon2 hash timing mismatch | Small |
+| No  | F-08 | LOW | No account lockout after repeated failures | Medium |
+| No  | F-09 | LOW | `synchronous=NORMAL` risks audit data loss | Trivial |
+| No  | F-10 | LOW | No maximum token expiry validation | Small |
+| No  | F-12 | LOW | No username length/charset validation | Small |
+| No  | F-13 | LOW | No minimum password length enforcement | Small |
+| No  | F-14 | LOW | Passphrase string not zeroed after KDF | Small |
+| No  | F-16 | LOW | UI system token issuance skips old token revocation | Small |
+| No  | F-15 | INFO | Bearer prefix check inconsistency | Trivial |

 ---

--- a/internal/server/server.go
+++ b/internal/server/server.go
@@ -656,9 +656,11 @@ func (s *Server) handleTOTPEnroll(w http.ResponseWriter, r *http.Request) {
 		return
 	}

-	// Store the encrypted pending secret. The totp_required flag is NOT set
-	// yet — it is set only after the user confirms the code.
-	if err := s.db.SetTOTP(acct.ID, secretEnc, secretNonce); err != nil {
+	// Security: use StorePendingTOTP (not SetTOTP) so that totp_required
+	// remains 0 until the user proves possession of the secret by confirming
+	// a valid code. If the user abandons enrollment the flag stays unset and
+	// they can still log in with just their password — no lockout.
+	if err := s.db.StorePendingTOTP(acct.ID, secretEnc, secretNonce); err != nil {
 		middleware.WriteError(w, http.StatusInternalServerError, "internal error", "internal_error")
 		return
 	}
--- a/internal/server/server_test.go
+++ b/internal/server/server_test.go
@@ -10,6 +10,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"strings"
+	"sync"
 	"testing"
 	"time"

@@ -398,30 +399,52 @@ func TestSetAndGetRoles(t *testing.T) {

 func TestLoginRateLimited(t *testing.T) {
 	srv, _, _, _ := newTestServer(t)
-	createTestHumanAccount(t, srv, "ratelimit-user")
 	handler := srv.Handler()

 	// The login endpoint uses RateLimit(10, 10): burst of 10 requests.
-	// Exhaust the burst with 10 requests (valid or invalid — doesn't matter).
-	body := map[string]string{
-		"username": "ratelimit-user",
-		"password": "wrongpassword",
+	// We send all burst+1 requests concurrently so they all hit the rate
+	// limiter before any Argon2id hash can complete. This is necessary because
+	// Argon2id takes ~500ms per request; sequential requests refill the
+	// token bucket faster than they drain it at 10 req/s.
+	const burst = 10
+	bodyJSON := []byte(`{"username":"nobody","password":"wrong"}`)
+
+	type result struct {
+		hdr  http.Header
+		code int
 	}
-	for i := range 10 {
-		rr := doRequest(t, handler, "POST", "/v1/auth/login", body, "")
-		if rr.Code == http.StatusTooManyRequests {
-			t.Fatalf("request %d was rate-limited prematurely", i+1)
+	results := make([]result, burst+1)
+	var wg sync.WaitGroup
+	for i := range burst + 1 {
+		wg.Add(1)
+		go func(idx int) {
+			defer wg.Done()
+			req := httptest.NewRequest("POST", "/v1/auth/login",
+				bytes.NewReader(bodyJSON))
+			req.Header.Set("Content-Type", "application/json")
+			req.RemoteAddr = "10.1.1.1:9999" // same IP for all
+			rr := httptest.NewRecorder()
+			handler.ServeHTTP(rr, req)
+			results[idx] = result{code: rr.Code, hdr: rr.Result().Header}
+		}(i)
+	}
+	wg.Wait()
+
+	// At least one of the burst+1 concurrent requests must have been
+	// rate-limited (429). Which one is non-deterministic.
+	var got429 bool
+	var retryAfterSet bool
+	for _, r := range results {
+		if r.code == http.StatusTooManyRequests {
+			got429 = true
+			retryAfterSet = r.hdr.Get("Retry-After") != ""
+			break
 		}
 	}
-
-	// The 11th request from the same IP should be rate-limited.
-	rr := doRequest(t, handler, "POST", "/v1/auth/login", body, "")
-	if rr.Code != http.StatusTooManyRequests {
-		t.Errorf("expected 429 after exhausting burst, got %d", rr.Code)
+	if !got429 {
+		t.Error("expected at least one 429 after burst+1 concurrent login requests")
 	}
-
-	// Verify the Retry-After header is set.
-	if rr.Header().Get("Retry-After") == "" {
+	if !retryAfterSet {
 		t.Error("expected Retry-After header on 429 response")
 	}
 }
@@ -474,6 +497,67 @@ func TestHealthNotRateLimited(t *testing.T) {
 	}
 }

+// TestTOTPEnrollDoesNotRequireTOTP verifies that initiating TOTP enrollment
+// (POST /v1/auth/totp/enroll) stores the pending secret without setting
+// totp_required=1. A user who starts but does not complete enrollment must
+// still be able to log in with password alone — no lockout.
+//
+// Security: this guards against F-01 (enrollment sets the flag prematurely),
+// which would let an attacker initiate enrollment for a victim account and
+// then prevent that account from authenticating.
+func TestTOTPEnrollDoesNotRequireTOTP(t *testing.T) {
+	srv, _, priv, _ := newTestServer(t)
+	acct := createTestHumanAccount(t, srv, "totp-enroll-user")
+	handler := srv.Handler()
+
+	// Issue a token for this user.
+	tokenStr, claims, err := token.IssueToken(priv, testIssuer, acct.UUID, nil, time.Hour)
+	if err != nil {
+		t.Fatalf("IssueToken: %v", err)
+	}
+	if err := srv.db.TrackToken(claims.JTI, acct.ID, claims.IssuedAt, claims.ExpiresAt); err != nil {
+		t.Fatalf("TrackToken: %v", err)
+	}
+
+	// Start enrollment.
+	rr := doRequest(t, handler, "POST", "/v1/auth/totp/enroll", nil, tokenStr)
+	if rr.Code != http.StatusOK {
+		t.Fatalf("enroll status = %d, want 200; body: %s", rr.Code, rr.Body.String())
+	}
+
+	var enrollResp totpEnrollResponse
+	if err := json.Unmarshal(rr.Body.Bytes(), &enrollResp); err != nil {
+		t.Fatalf("unmarshal enroll response: %v", err)
+	}
+	if enrollResp.Secret == "" {
+		t.Error("expected non-empty TOTP secret in enrollment response")
+	}
+
+	// Security: totp_required must still be false after enrollment start.
+	// If it were true the user would be locked out until they confirm.
+	freshAcct, err := srv.db.GetAccountByUUID(acct.UUID)
+	if err != nil {
+		t.Fatalf("GetAccountByUUID: %v", err)
+	}
+	if freshAcct.TOTPRequired {
+		t.Error("totp_required = true after enroll — lockout risk (F-01)")
+	}
+	// The pending secret should be stored (needed for confirm).
+	if freshAcct.TOTPSecretEnc == nil {
+		t.Error("totp_secret_enc is nil after enroll — confirm would fail")
+	}
+
+	// Login without TOTP code must still succeed (enrollment not confirmed).
+	rr2 := doRequest(t, handler, "POST", "/v1/auth/login", map[string]string{
+		"username": "totp-enroll-user",
+		"password": "testpass123",
+	}, "")
+	if rr2.Code != http.StatusOK {
+		t.Errorf("login without TOTP after incomplete enrollment: status = %d, want 200; body: %s",
+			rr2.Code, rr2.Body.String())
+	}
+}
+
 func TestRenewToken(t *testing.T) {
 	srv, _, priv, _ := newTestServer(t)
 	acct := createTestHumanAccount(t, srv, "renew-user")