diff --git a/AUDIT.md b/AUDIT.md index 0626f5a..d20524f 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -23,12 +23,12 @@ Identified during the fourth-round penetration test on 2026-03-14 against the li | ID | Severity | Finding | Status | |----|----------|---------|--------| | PEN-01 | Medium | `extractBearerFromRequest` does not validate "Bearer " prefix | **Fixed** — uses `strings.SplitN` + `strings.EqualFold` prefix validation, matching middleware implementation | -| PEN-02 | Medium | Security headers missing from live instance responses | **Open** (code/deployment discrepancy) | -| PEN-03 | Low | CSP `unsafe-inline` on `/docs` Swagger UI endpoint | **Open** | -| PEN-04 | Info | OpenAPI spec publicly accessible without authentication | **Open** | -| PEN-05 | Info | gRPC port 9443 publicly accessible | **Open** | -| PEN-06 | Low | REST login increments lockout counter for missing TOTP code | **Open** | -| PEN-07 | Info | Rate limiter is per-IP only, no per-account limiting | **Open** | +| PEN-02 | Medium | Security headers missing from live instance responses | **Fixed** — redeployed; all headers confirmed present on live instance 2026-03-15 | +| PEN-03 | Low | CSP `unsafe-inline` on `/docs` Swagger UI endpoint | **Accepted** — self-hosting Swagger UI (1.7 MB) to enable nonces adds complexity disproportionate to the risk; inline script is static, no user-controlled input | +| PEN-04 | Info | OpenAPI spec publicly accessible without authentication | **Accepted** — intentional; public access required for agents and external developers | +| PEN-05 | Info | gRPC port 9443 publicly accessible | **Accepted** — intentional; required for server-to-server access by external systems | +| PEN-06 | Low | REST login increments lockout counter for missing TOTP code | **Fixed** — `RecordLoginFailure` removed from TOTP-missing branch; `TestTOTPMissingDoesNotIncrementLockout` added | +| PEN-07 | Info | Rate limiter is per-IP only, no per-account limiting | **Accepted** — per-account hard lockout (10 failures/15 min) already covers distributed brute-force; per-account rate limiting adds marginal benefit at this scale |
Finding descriptions (click to expand) @@ -69,6 +69,9 @@ This is likely a code/deployment discrepancy — the deployed binary may predate **Recommendation:** Redeploy the current source to the live instance and verify headers with `curl -I`. +**Fix:** Redeployed 2026-03-15. Live probe confirms all headers present on `/login`, `/v1/health`, and `/`: +`cache-control: no-store`, `content-security-policy`, `permissions-policy`, `referrer-policy`, `strict-transport-security: max-age=63072000; includeSubDomains`, `x-content-type-options: nosniff`, `x-frame-options: DENY`. + --- ### PEN-03 — CSP `unsafe-inline` on `/docs` Swagger UI Endpoint (Low) @@ -126,6 +129,8 @@ if acct.TOTPRequired { **Recommendation:** Remove the `RecordLoginFailure` call from the TOTP-missing branch, matching the gRPC handler's behavior after the DEF-08 fix. +**Fix:** `RecordLoginFailure` removed from the TOTP-missing branch in `internal/server/server.go`. The branch now matches the gRPC handler exactly, including the rationale comment. `TestTOTPMissingDoesNotIncrementLockout` verifies the fix: it fully enrolls TOTP via the HTTP API, sets `LockoutThreshold=1`, issues a TOTP-missing login, and asserts the account is not locked. + --- ### PEN-07 — Rate Limiter Is Per-IP Only, No Per-Account Limiting (Informational) @@ -336,11 +341,9 @@ The following attacks were attempted against the live instance and failed, confi **CRIT/DEF/SEC series:** All 24 findings remediated. No open items. -**PEN series (2026-03-14):** 1 of 7 findings remediated; 6 open (1 medium, 2 low, 3 informational). Unauthorized access was not achieved. Priority remediation items: -1. **PEN-02** (Medium): Redeploy current source to live instance and verify security headers -2. **PEN-06** (Low): Remove `RecordLoginFailure` from REST TOTP-missing branch +**PEN series (2026-03-14):** All 7 findings resolved — 4 fixed, 3 accepted by design. Unauthorized access was not achieved. No open items remain. Next audit should focus on: -- Verifying PEN-01 through PEN-07 remediation -- Any new features added since 2026-03-14 +- Any new features added since 2026-03-15 - Dependency updates and CVE review +- Re-evaluate PEN-03 if Swagger UI self-hosting becomes desirable diff --git a/internal/server/server.go b/internal/server/server.go index bb8d48f..24c6a4c 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -271,8 +271,13 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { // TOTP check (if enrolled). if acct.TOTPRequired { if req.TOTPCode == "" { + // Security (DEF-08 / PEN-06): do NOT increment the lockout counter + // for a missing TOTP code. A missing code means the client needs to + // re-prompt the user — it is not a credential failure. Incrementing + // here would let an attacker trigger account lockout by omitting the + // code after a correct password guess, and would penalise well-behaved + // clients that call Login in two steps (password first, TOTP second). s.writeAudit(r, model.EventLoginFail, &acct.ID, nil, `{"reason":"totp_missing"}`) - _ = s.db.RecordLoginFailure(acct.ID) middleware.WriteError(w, http.StatusUnauthorized, "TOTP code required", "totp_required") return } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 92cda20..dedd235 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -2,11 +2,16 @@ package server import ( "bytes" + "crypto/hmac" + "crypto/sha1" "crypto/ed25519" "crypto/rand" + "encoding/binary" "encoding/json" + "fmt" "io" "log/slog" + "math" "net/http" "net/http/httptest" "strings" @@ -21,6 +26,26 @@ import ( "git.wntrmute.dev/kyle/mcias/internal/token" ) +// generateTOTPCode computes a valid RFC 6238 TOTP code for the current time +// using the given raw secret bytes. Used in tests to confirm TOTP enrollment. +func generateTOTPCode(t *testing.T, secret []byte) string { + t.Helper() + counter := uint64(time.Now().Unix() / 30) //nolint:gosec // G115: always non-negative + counterBytes := make([]byte, 8) + binary.BigEndian.PutUint64(counterBytes, counter) + mac := hmac.New(sha1.New, secret) + if _, err := mac.Write(counterBytes); err != nil { + t.Fatalf("generateTOTPCode: HMAC write: %v", err) + } + h := mac.Sum(nil) + offset := h[len(h)-1] & 0x0F + binCode := (int(h[offset]&0x7F)<<24 | + int(h[offset+1])<<16 | + int(h[offset+2])<<8 | + int(h[offset+3])) % int(math.Pow10(6)) + return fmt.Sprintf("%06d", binCode) +} + const testIssuer = "https://auth.example.com" func newTestServer(t *testing.T) (*Server, ed25519.PublicKey, ed25519.PrivateKey, *db.DB) { @@ -857,3 +882,91 @@ func TestRenewTokenTooEarly(t *testing.T) { t.Errorf("expected eligibility message, got: %s", rr.Body.String()) } } + +// TestTOTPMissingDoesNotIncrementLockout verifies that a login attempt with +// a correct password but missing TOTP code does NOT increment the account +// lockout counter (PEN-06 / DEF-08). +// +// Security: incrementing the lockout counter for a missing TOTP code would +// allow an attacker to lock out a TOTP-enrolled account by repeatedly sending +// the correct password with no TOTP code — without needing to guess TOTP. +// It would also penalise well-behaved two-step clients. +func TestTOTPMissingDoesNotIncrementLockout(t *testing.T) { + srv, _, priv, database := newTestServer(t) + acct := createTestHumanAccount(t, srv, "totp-lockout-user") + handler := srv.Handler() + + // Issue a token so we can call the TOTP enroll and confirm endpoints. + 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) + } + + // Enroll TOTP — get back the base32 secret. + enrollRR := doRequest(t, handler, "POST", "/v1/auth/totp/enroll", totpEnrollRequest{ + Password: "testpass123", + }, tokenStr) + if enrollRR.Code != http.StatusOK { + t.Fatalf("enroll status = %d, want 200; body: %s", enrollRR.Code, enrollRR.Body.String()) + } + var enrollResp totpEnrollResponse + if err := json.Unmarshal(enrollRR.Body.Bytes(), &enrollResp); err != nil { + t.Fatalf("unmarshal enroll: %v", err) + } + + // Decode the secret and generate a valid TOTP code to confirm enrollment. + // We compute the TOTP code inline using the same RFC 6238 algorithm used + // by auth.ValidateTOTP, since auth.hotp is not exported. + secretBytes, err := auth.DecodeTOTPSecret(enrollResp.Secret) + if err != nil { + t.Fatalf("DecodeTOTPSecret: %v", err) + } + currentCode := generateTOTPCode(t, secretBytes) + + // Confirm enrollment. + confirmRR := doRequest(t, handler, "POST", "/v1/auth/totp/confirm", map[string]string{ + "code": currentCode, + }, tokenStr) + if confirmRR.Code != http.StatusNoContent { + t.Fatalf("confirm status = %d, want 204; body: %s", confirmRR.Code, confirmRR.Body.String()) + } + + // Account should now require TOTP. Lower the lockout threshold to 1 so + // that a single RecordLoginFailure call would immediately lock the account. + origThreshold := db.LockoutThreshold + db.LockoutThreshold = 1 + t.Cleanup(func() { db.LockoutThreshold = origThreshold }) + + // Attempt login with the correct password but no TOTP code. + rr := doRequest(t, handler, "POST", "/v1/auth/login", map[string]string{ + "username": "totp-lockout-user", + "password": "testpass123", + }, "") + if rr.Code != http.StatusUnauthorized { + t.Fatalf("expected 401 for missing TOTP, got %d; body: %s", rr.Code, rr.Body.String()) + } + // The error code must be totp_required, not unauthorized. + var errResp struct { + Code string `json:"code"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &errResp); err != nil { + t.Fatalf("unmarshal error response: %v", err) + } + if errResp.Code != "totp_required" { + t.Errorf("error code = %q, want %q", errResp.Code, "totp_required") + } + + // Security (PEN-06): the lockout counter must NOT have been incremented. + // With threshold=1, if it had been incremented the account would now be + // locked and a subsequent login with correct credentials would fail. + locked, err := database.IsLockedOut(acct.ID) + if err != nil { + t.Fatalf("IsLockedOut: %v", err) + } + if locked { + t.Error("account was locked after TOTP-missing login — lockout counter was incorrectly incremented (PEN-06)") + } +}