fix(auth): final-review MFA fixes F1 F2 F3 F5 F7 and drop Ticket.Attempts

- OIDC sessions minted through saveSignIn with AMR oidc and StepUpAt
- passwordless passkey uses ValidateDiscoverableLogin with owner-handle check
- TOTP replay guard burns the matched step, not the current one
- enrol-only tickets refused once the user already has a factor
- bootstrap owner session minted through mintSession

server/internal/auth/local.go

@@ -259,14 +259,10 @@ func HandleBootstrap(c *gin.Context) {
 		c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
 		return
 	}
-	sessionID, err := SaveSession(c.Request.Context(), &Session{
-		UserID: u.UserID, InstanceID: u.InstanceID, Role: u.Role, Email: u.Email,
-	})
-	if err != nil {
+	if err := mintSession(c, u, []string{"pwd"}); err != nil {
 		c.JSON(http.StatusInternalServerError, gin.H{"error": "session save failed"})
 		return
 	}
-	SetSessionCookie(c, sessionID)
 	c.JSON(http.StatusCreated, gin.H{
 		"instance":    inst,
 		"slug":        inst.Slug,

server/internal/auth/mfa_enrol.go

@@ -11,6 +11,25 @@ import (
 	"github.com/go-webauthn/webauthn/webauthn"
 )
 
+// enrolTicketStillValid refuses an enrol-only ticket once the user has any
+// factor. Otherwise someone holding only the password who started sign-in
+// before the real user enrolled could, within the ticket's lifetime, add
+// their own passkey and replace the user's recovery codes. Answered as an
+// expired ticket, the single indistinguishable error, and the ticket is
+// destroyed. A lookup failure is treated the same way: fail closed.
+//
+// It runs before the factor is written, not in finishEnrolment: by then the
+// factor being enrolled already counts, so HasMFA would always be true.
+func enrolTicketStillValid(c *gin.Context, t *Ticket, ticketID string) bool {
+	has, err := services.HasMFA(t.InstanceID, t.UserID)
+	if err != nil || has {
+		_ = DeleteTicket(c.Request.Context(), ticketID)
+		abortTicketExpired(c)
+		return false
+	}
+	return true
+}
+
 // HandleEnrolTOTPSetup starts enrolment for a user the instance requires MFA
 // from, before they hold a session. Only an enrol-only ticket reaches it.
 //
@@ -21,8 +40,8 @@ import (
 //	@Failure	401	{object}	object{error=string,code=string}
 //	@Router		/auth/mfa/enrol/totp/setup [post]
 func HandleEnrolTOTPSetup(c *gin.Context) {
-	t, _, ok := ticketFromRequest(c, scopeEnrol)
-	if !ok {
+	t, ticketID, ok := ticketFromRequest(c, scopeEnrol)
+	if !ok || !enrolTicketStillValid(c, t, ticketID) {
 		return
 	}
 	inst, err := services.GetInstance(t.InstanceID)
@@ -50,7 +69,7 @@ func HandleEnrolTOTPSetup(c *gin.Context) {
 //	@Router		/auth/mfa/enrol/totp/confirm [post]
 func HandleEnrolTOTPConfirm(c *gin.Context) {
 	t, ticketID, ok := ticketFromRequest(c, scopeEnrol)
-	if !ok {
+	if !ok || !enrolTicketStillValid(c, t, ticketID) {
 		return
 	}
 	var body struct {
@@ -107,8 +126,8 @@ func finishEnrolment(c *gin.Context, t *Ticket, ticketID, factor string) {
 //	@Failure	401	{object}	object{error=string,code=string}
 //	@Router		/auth/mfa/enrol/passkey/begin [post]
 func HandleEnrolPasskeyBegin(c *gin.Context) {
-	t, _, ok := ticketFromRequest(c, scopeEnrol)
-	if !ok {
+	t, ticketID, ok := ticketFromRequest(c, scopeEnrol)
+	if !ok || !enrolTicketStillValid(c, t, ticketID) {
 		return
 	}
 	handle, err := services.WebAuthnHandle(t.InstanceID, t.UserID)
@@ -159,7 +178,7 @@ func HandleEnrolPasskeyBegin(c *gin.Context) {
 //	@Router		/auth/mfa/enrol/passkey/finish [post]
 func HandleEnrolPasskeyFinish(c *gin.Context) {
 	t, ticketID, ok := ticketFromRequest(c, scopeEnrol)
-	if !ok {
+	if !ok || !enrolTicketStillValid(c, t, ticketID) {
 		return
 	}
 	var body struct {

server/internal/auth/mfa_login.go

@@ -21,18 +21,38 @@ func loginDecision(hasMFA, requireMFA bool) string {
 	}
 }
 
+// newSession builds the session a sign-in produces. Sign-in counts as a
+// step-up, so StepUpAt is always the moment of sign-in.
+func newSession(u *models.User, amr []string, now time.Time) *Session {
+	return &Session{
+		UserID: u.UserID, InstanceID: u.InstanceID, Role: u.Role, Email: u.Email,
+		AMR: amr, StepUpAt: &now,
+	}
+}
+
+// oidcSession is the session an SSO callback mints. AMR "oidc" is what exempts
+// it from step-up: the IdP owns authentication policy, and these users have
+// no password or local factor to step up with.
+func oidcSession(u *models.User, name string, now time.Time) *Session {
+	s := newSession(u, []string{"oidc"}, now)
+	s.Name = name
+	return s
+}
+
 // mintSession is the single place a session is created from a user, so every
 // path records amr and step-up freshness the same way.
 func mintSession(c *gin.Context, u *models.User, amr []string) error {
-	now := time.Now()
-	sessionID, err := SaveSession(c.Request.Context(), &Session{
-		UserID: u.UserID, InstanceID: u.InstanceID, Role: u.Role, Email: u.Email,
-		AMR: amr, StepUpAt: &now,
-	})
+	return saveSignIn(c, newSession(u, amr, time.Now()))
+}
+
+// saveSignIn persists a sign-in session and sets its cookie. The pending-MFA
+// cookie is cleared because a completed sign-in supersedes any ticket.
+func saveSignIn(c *gin.Context, sess *Session) error {
+	sessionID, err := SaveSession(c.Request.Context(), sess)
 	if err != nil {
 		return err
 	}
-	_ = services.TouchLastLogin(u.UserID)
+	_ = services.TouchLastLogin(sess.UserID)
 	ClearPendingCookie(c)
 	SetSessionCookie(c, sessionID)
 	return nil

server/internal/auth/mfaticket.go

@@ -33,7 +33,6 @@ type Ticket struct {
 	Email      string   `json:"email"`
 	Methods    []string `json:"methods"`
 	EnrolOnly  bool     `json:"enrol_only"`
-	Attempts   int      `json:"attempts"`
 }
 
 type ticketScope int

server/internal/auth/oidc.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"strings"
 	"sync"
+	"time"
 
 	"gitea.hostxtra.co.uk/mrhid6/vantage/server/internal/models"
 	"gitea.hostxtra.co.uk/mrhid6/vantage/server/internal/services"
@@ -225,15 +226,10 @@ func completeSSOLogin(c *gin.Context, instanceID, email, name string) {
 		}
 	}
 
-	sessionID, err := SaveSession(c.Request.Context(), &Session{
-		UserID: u.UserID, InstanceID: u.InstanceID, Role: u.Role, Email: u.Email, Name: name,
-	})
-	if err != nil {
+	if err := saveSignIn(c, oidcSession(u, name, time.Now())); err != nil {
 		c.Redirect(http.StatusFound, "/login?error=session_failed")
 		return
 	}
-	_ = services.TouchLastLogin(u.UserID)
-	SetSessionCookie(c, sessionID)
 	c.Redirect(http.StatusFound, "/")
 }
 

server/internal/auth/passkey_login.go

@@ -80,8 +80,9 @@ func HandlePasskeyLoginFinish(c *gin.Context) {
 		return
 	}
 	// finishAssertion is given no userID or email, so it resolves the owning
-	// user itself from the credential ID scoped to this instance - the same
-	// requirement a discoverable login has to meet.
+	// user from the credential ID scoped to this instance and validates with
+	// the library's discoverable path, refusing a user handle that is not
+	// that owner's.
 	cred, err := finishAssertion(c, instanceID, "", "")
 	if err != nil {
 		c.JSON(http.StatusUnauthorized, gin.H{

server/internal/auth/stepup_test.go

@@ -6,6 +6,8 @@ import (
 	"testing"
 	"time"
 
+	"gitea.hostxtra.co.uk/mrhid6/vantage/server/internal/models"
+
 	"github.com/gin-gonic/gin"
 )
 
@@ -52,3 +54,23 @@ func TestRequireStepUpNilSessionRefusesWithoutPanic(t *testing.T) {
 		t.Fatalf("status = %d, want %d", w.Code, http.StatusUnauthorized)
 	}
 }
+
+func TestOIDCSessionIsExemptFromStepUp(t *testing.T) {
+	now := time.Now()
+	u := &models.User{UserID: "u1", InstanceID: "i1", Role: "member", Email: "a@example.com"}
+	s := oidcSession(u, "Ann", now.Add(-24*time.Hour))
+	if !stepUpFresh(s, now) {
+		t.Fatal("an OIDC session must be exempt from step-up however old its sign-in")
+	}
+	if s.StepUpAt == nil || s.Name != "Ann" || s.UserID != "u1" || s.InstanceID != "i1" {
+		t.Fatalf("oidc session missing fields: %+v", s)
+	}
+}
+
+func TestSignInSessionIsFreshAtSignIn(t *testing.T) {
+	now := time.Now()
+	s := newSession(&models.User{UserID: "u1"}, []string{"pwd"}, now)
+	if !stepUpFresh(s, now) {
+		t.Fatal("a session is fresh at the moment of sign-in")
+	}
+}

server/internal/auth/webauthn.go

@@ -3,6 +3,7 @@ package auth
 import (
 	"bytes"
 	"context"
+	"crypto/subtle"
 	"encoding/json"
 	"errors"
 	"net"
@@ -429,7 +430,21 @@ func finishAssertion(c *gin.Context, instanceID, userID, email string) (*webauth
 		return nil, err
 	}
 	user := waUser{handle: handle, name: email, credentials: []webauthn.Credential{toLibCredential(*stored)}}
-	cred, err := w.ValidateLogin(user, *sessionData, parsed)
+	var cred *webauthn.Credential
+	if userID == "" {
+		// Discoverable (passwordless) ceremony: the session carries no user,
+		// so the library asks us to resolve the authenticator's user handle.
+		// The only acceptable answer is the credential's own stored owner in
+		// this instance; any other handle is refused.
+		cred, err = w.ValidateDiscoverableLogin(func(_, userHandle []byte) (webauthn.User, error) {
+			if !discoverableHandleMatches(handle, userHandle) {
+				return nil, errors.New("user handle does not match the credential owner")
+			}
+			return user, nil
+		}, *sessionData, parsed)
+	} else {
+		cred, err = w.ValidateLogin(user, *sessionData, parsed)
+	}
 	if err != nil {
 		return nil, err
 	}
@@ -443,3 +458,9 @@ func finishAssertion(c *gin.Context, instanceID, userID, email string) (*webauth
 	}
 	return cred, nil
 }
+
+// discoverableHandleMatches reports whether the user handle an authenticator
+// returned belongs to the credential's stored owner. Empty never matches.
+func discoverableHandleMatches(ownerHandle, userHandle []byte) bool {
+	return len(ownerHandle) > 0 && subtle.ConstantTimeCompare(ownerHandle, userHandle) == 1
+}

server/internal/auth/webauthn_test.go

@@ -1,6 +1,7 @@
 package auth
 
 import (
+	"bytes"
 	"net/http/httptest"
 	"testing"
 
@@ -34,3 +35,18 @@ func TestRPConfig(t *testing.T) {
 		})
 	}
 }
+
+func TestDiscoverableHandleMatches(t *testing.T) {
+	owner := bytes.Repeat([]byte{7}, 64)
+	if !discoverableHandleMatches(owner, bytes.Repeat([]byte{7}, 64)) {
+		t.Fatal("the owner's own handle must match")
+	}
+	other := bytes.Repeat([]byte{7}, 64)
+	other[63] = 8
+	if discoverableHandleMatches(owner, other) {
+		t.Fatal("another user's handle must not match")
+	}
+	if discoverableHandleMatches(nil, nil) || discoverableHandleMatches(owner, nil) {
+		t.Fatal("an empty handle must never match")
+	}
+}

server/internal/services/mfa.go

@@ -300,13 +300,28 @@ func VerifyTOTPCode(instanceID, userID, code string) error {
 	if err != nil {
 		return err
 	}
-	valid, err := totp.ValidateCustom(code, secret, time.Now(), totp.ValidateOpts{
-		Period: 30, Skew: 1, Digits: otp.DigitsSix, Algorithm: otp.AlgorithmSHA1,
-	})
-	if err != nil || !valid {
+	step, ok := matchTOTPStep(code, secret, time.Now())
+	if !ok {
 		return ErrBadCode
 	}
-	return burnTOTPCode(userID, code)
+	return burnTOTPStep(userID, step)
+}
+
+// matchTOTPStep returns the 30-second time step the code was generated for,
+// trying the current step and one either side (the accepted skew). The replay
+// guard must burn this step, not the current one: burning the current step
+// would let a code accepted as s-1 or s+1 be accepted again one step later.
+func matchTOTPStep(code, secret string, now time.Time) (int64, bool) {
+	cur := now.Unix() / 30
+	for _, step := range []int64{cur - 1, cur, cur + 1} {
+		want, err := totp.GenerateCodeCustom(secret, time.Unix(step*30, 0), totp.ValidateOpts{
+			Period: 30, Digits: otp.DigitsSix, Algorithm: otp.AlgorithmSHA1,
+		})
+		if err == nil && subtle.ConstantTimeCompare([]byte(want), []byte(code)) == 1 {
+			return step, true
+		}
+	}
+	return 0, false
 }
 
 // ClearMFA removes every factor. Used by an owner or admin reset.
@@ -367,18 +382,17 @@ func RequireMFAForInstance(instanceID string) bool {
 	return models.RequireMFA(s)
 }
 
-// burnTOTPCode makes a step single-use for 90 seconds - longer than the +-1
+// burnTOTPStep makes a step single-use for 90 seconds - longer than the +-1
 // step window it could still validate in. Keyed on the time step the code
-// was accepted against, never on the code itself: a raw code sitting in a
-// Redis key name would be a currently-valid credential readable by anything
-// that can list keys. Redis is already required for sessions.
-func burnTOTPCode(userID, code string) error {
+// matched, never on the code itself: a raw code sitting in a Redis key name
+// would be a currently-valid credential readable by anything that can list
+// keys. Redis is already required for sessions.
+func burnTOTPStep(userID string, step int64) error {
 	if RedisClient == nil {
 		return nil
 	}
 	ctx, cancel := mfaCtx()
 	defer cancel()
-	step := time.Now().Unix() / 30
 	key := "km:totp:" + userID + ":" + strconv.FormatInt(step, 10)
 	ok, err := RedisClient.SetNX(ctx, key, 1, 90*time.Second).Result()
 	if err != nil {

server/internal/services/mfa_test.go

@@ -6,6 +6,8 @@ import (
 	"time"
 
 	"gitea.hostxtra.co.uk/mrhid6/vantage/server/internal/models"
+	"github.com/pquerna/otp"
+	"github.com/pquerna/otp/totp"
 )
 
 func TestGenerateRecoveryCodesReturnsTenUniqueHashedCodes(t *testing.T) {
@@ -84,3 +86,30 @@ func TestHasFactorRequiresConfirmedTOTPOrAPasskey(t *testing.T) {
 		})
 	}
 }
+
+func TestMatchTOTPStepReturnsTheStepTheCodeWasGeneratedFor(t *testing.T) {
+	const secret = "JBSWY3DPEHPK3PXP"
+	now := time.Unix(1_800_000_015, 0) // mid-step, well clear of a boundary
+	cur := now.Unix() / 30
+	for _, off := range []int64{-1, 0, 1} {
+		code, err := totp.GenerateCodeCustom(secret, time.Unix((cur+off)*30, 0), totp.ValidateOpts{
+			Period: 30, Digits: otp.DigitsSix, Algorithm: otp.AlgorithmSHA1,
+		})
+		if err != nil {
+			t.Fatal(err)
+		}
+		step, ok := matchTOTPStep(code, secret, now)
+		if !ok || step != cur+off {
+			t.Fatalf("offset %d: got step %d ok=%v, want %d", off, step, ok, cur+off)
+		}
+	}
+	far, _ := totp.GenerateCodeCustom(secret, time.Unix((cur+5)*30, 0), totp.ValidateOpts{
+		Period: 30, Digits: otp.DigitsSix, Algorithm: otp.AlgorithmSHA1,
+	})
+	if _, ok := matchTOTPStep(far, secret, now); ok {
+		t.Fatal("a code from five steps ahead must not match")
+	}
+	if _, ok := matchTOTPStep("000000x", secret, now); ok {
+		t.Fatal("a malformed code must not match")
+	}
+}