From 19383abaf896efd2b516b9b65e89a83c0ddc10da Mon Sep 17 00:00:00 2001 From: mrhid6 Date: Wed, 16 Sep 2026 14:26:42 +0000 Subject: [PATCH] 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 | 6 +---- server/internal/auth/mfa_enrol.go | 31 ++++++++++++++++++----- server/internal/auth/mfa_login.go | 32 +++++++++++++++++++----- server/internal/auth/mfaticket.go | 1 - server/internal/auth/oidc.go | 8 ++---- server/internal/auth/passkey_login.go | 5 ++-- server/internal/auth/stepup_test.go | 22 ++++++++++++++++ server/internal/auth/webauthn.go | 23 ++++++++++++++++- server/internal/auth/webauthn_test.go | 16 ++++++++++++ server/internal/services/mfa.go | 36 +++++++++++++++++++-------- server/internal/services/mfa_test.go | 29 +++++++++++++++++++++ 11 files changed, 171 insertions(+), 38 deletions(-) diff --git a/server/internal/auth/local.go b/server/internal/auth/local.go index e784f13..0a5940e 100644 --- a/server/internal/auth/local.go +++ b/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, diff --git a/server/internal/auth/mfa_enrol.go b/server/internal/auth/mfa_enrol.go index d009a28..d3c635b 100644 --- a/server/internal/auth/mfa_enrol.go +++ b/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 { diff --git a/server/internal/auth/mfa_login.go b/server/internal/auth/mfa_login.go index 680e172..3cdde9f 100644 --- a/server/internal/auth/mfa_login.go +++ b/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 diff --git a/server/internal/auth/mfaticket.go b/server/internal/auth/mfaticket.go index 5f29822..06bab8f 100644 --- a/server/internal/auth/mfaticket.go +++ b/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 diff --git a/server/internal/auth/oidc.go b/server/internal/auth/oidc.go index bfd1f34..8420850 100644 --- a/server/internal/auth/oidc.go +++ b/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, "/") } diff --git a/server/internal/auth/passkey_login.go b/server/internal/auth/passkey_login.go index fb8fc0b..1ecb944 100644 --- a/server/internal/auth/passkey_login.go +++ b/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{ diff --git a/server/internal/auth/stepup_test.go b/server/internal/auth/stepup_test.go index bb29ec3..a2f3b79 100644 --- a/server/internal/auth/stepup_test.go +++ b/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") + } +} diff --git a/server/internal/auth/webauthn.go b/server/internal/auth/webauthn.go index f65783b..7513773 100644 --- a/server/internal/auth/webauthn.go +++ b/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 +} diff --git a/server/internal/auth/webauthn_test.go b/server/internal/auth/webauthn_test.go index bf21573..2d6edfa 100644 --- a/server/internal/auth/webauthn_test.go +++ b/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") + } +} diff --git a/server/internal/services/mfa.go b/server/internal/services/mfa.go index b0a42d8..9fc6c12 100644 --- a/server/internal/services/mfa.go +++ b/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 { diff --git a/server/internal/services/mfa_test.go b/server/internal/services/mfa_test.go index fb4c928..b60f34f 100644 --- a/server/internal/services/mfa_test.go +++ b/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") + } +}