diff --git a/AUDIT.md b/AUDIT.md index 4521b15..53e6a7e 100644 --- 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 | --- diff --git a/internal/server/server.go b/internal/server/server.go index fa0be37..e3e8bf3 100644 --- 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 } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 92235e6..3200d0b 100644 --- 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")