Remediate PEN-01 through PEN-07 (pentest round 4)
- PEN-01: fix extractBearerFromRequest to validate Bearer prefix using strings.SplitN + EqualFold; add TestExtractBearerFromRequest - PEN-02: security headers confirmed present after redeploy (live probe 2026-03-15) - PEN-03: accepted — Swagger UI self-hosting disproportionate to risk - PEN-04: accepted — OpenAPI spec intentionally public - PEN-05: accepted — gRPC port 9443 intentionally public - PEN-06: remove RecordLoginFailure from REST TOTP-missing branch to match gRPC handler (DEF-08); add TestTOTPMissingDoesNotIncrementLockout - PEN-07: accepted — per-account hard lockout covers the same threat - Update AUDIT.md: all 7 PEN findings resolved (4 fixed, 3 accepted) Security: PEN-01 removed a defence-in-depth gap where any 8+ char Authorization value was accepted as a Bearer token. PEN-06 closed an account-lockout-via-omission attack vector on TOTP-enrolled accounts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -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)")
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user