fix(auth): store mail OTP codes as bcrypt hashes

core/authenticate/service.go

@@ -3,10 +3,11 @@ package authenticate
 import (
 	"bytes"
 	"context"
-	"crypto/subtle"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
+	"log/slog"
+	"slices"
 	"strings"
 	"time"
 
@@ -16,8 +17,6 @@ import (
 
 	"github.com/raystack/frontier/core/audit"
 
-	"slices"
-
 	frontiersession "github.com/raystack/frontier/core/authenticate/session"
 	"github.com/raystack/frontier/core/serviceuser"
 	patModels "github.com/raystack/frontier/core/userpat/models"
@@ -33,13 +32,12 @@ import (
 
 	"github.com/raystack/frontier/pkg/mailer"
 
-	"log/slog"
-
 	"github.com/google/uuid"
 	"github.com/raystack/frontier/core/authenticate/strategy"
 	"github.com/raystack/frontier/core/user"
 	"github.com/raystack/frontier/pkg/str"
 	"github.com/robfig/cron/v3"
+	"golang.org/x/crypto/bcrypt"
 	"golang.org/x/oauth2"
 )
 
@@ -49,6 +47,9 @@ const (
 	otpAttemptKey  = "attempt"
 )
 
+// OTPHashCost is a var (not const) so tests can drop it to bcrypt.MinCost
+var OTPHashCost = 12
+
 var (
 	refreshTime              = "0 0 * * *" // Once a day at midnight
 	ErrStrategyNotApplicable = errors.New("strategy not applicable")
@@ -180,6 +181,15 @@ func (s Service) SanitizeCallbackURL(url string) string {
 	return ""
 }
 
+// hashOTP hashes the emailed code using bcrypt
+func hashOTP(otp string) (string, error) {
+	hash, err := bcrypt.GenerateFromPassword([]byte(otp), OTPHashCost)
+	if err != nil {
+		return "", err
+	}
+	return string(hash), nil
+}
+
 func (s Service) StartFlow(ctx context.Context, request RegistrationStartRequest) (*RegistrationStartResponse, error) {
 	if !utils.Contains(s.SupportedStrategies(), request.Method) {
 		return nil, ErrUnsupportedMethod
@@ -233,7 +243,11 @@ func (s Service) StartFlow(ctx context.Context, request RegistrationStartRequest
 			return nil, err
 		}
 
-		flow.Nonce = nonce
+		nonceHash, err := hashOTP(nonce)
+		if err != nil {
+			return nil, err
+		}
+		flow.Nonce = nonceHash
 		if s.config.MailOTP.Validity != 0 {
 			flow.ExpiresAt = flow.CreatedAt.Add(s.config.MailOTP.Validity)
 		}
@@ -258,7 +272,11 @@ func (s Service) StartFlow(ctx context.Context, request RegistrationStartRequest
 			return nil, err
 		}
 
-		flow.Nonce = nonce
+		nonceHash, err := hashOTP(nonce)
+		if err != nil {
+			return nil, err
+		}
+		flow.Nonce = nonceHash
 		if s.config.MailLink.Validity != 0 {
 			flow.ExpiresAt = flow.CreatedAt.Add(s.config.MailLink.Validity)
 		}
@@ -355,7 +373,7 @@ func (s Service) applyMailOTP(ctx context.Context, request RegistrationFinishReq
 		return nil, ErrFlowInvalid
 	}
 
-	if subtle.ConstantTimeCompare([]byte(flow.Nonce), []byte(request.Code)) == 0 {
+	if bcrypt.CompareHashAndPassword([]byte(flow.Nonce), []byte(request.Code)) != nil {
 		// avoid brute forcing otp
 		attemptInt := 0
 		if attempts, ok := flow.Metadata[otpAttemptKey]; ok {

core/authenticate/service_test.go

@@ -4,18 +4,20 @@ import (
 	"context"
 	"encoding/base64"
 	"errors"
+	"io"
+	"log/slog"
 	"math/rand"
+	"reflect"
 	"testing"
 	"time"
 
+	"golang.org/x/crypto/bcrypt"
+
 	"github.com/raystack/frontier/core/authenticate/strategy"
 	testusers "github.com/raystack/frontier/core/authenticate/test_users"
 	"github.com/raystack/frontier/pkg/mailer"
 	"github.com/stretchr/testify/assert"
 
-	"io"
-	"log/slog"
-
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/uuid"
 	"github.com/raystack/frontier/core/authenticate"
@@ -280,6 +282,10 @@ func TestService_GetPrincipal(t *testing.T) {
 }
 
 func TestService_StartFlow(t *testing.T) {
+	defaultHashCost := authenticate.OTPHashCost
+	authenticate.OTPHashCost = bcrypt.MinCost
+	t.Cleanup(func() { authenticate.OTPHashCost = defaultHashCost })
+
 	// Since, 'Flow' contains a call to UUID.New(), it will return a new UUID on each call.
 	// We manipulate the seed so that fixed UUID is returned. This is done in setup.
 	id := uuid.MustParse("52fdfc07-2182-454f-963f-5f0f9a621d72") // fixed UUID returned for first call of UUID.New()
@@ -298,6 +304,17 @@ func TestService_StartFlow(t *testing.T) {
 		},
 	}
 
+	// Set receives the flow with a bcrypt hash in Nonce; verify the hash
+	// against the fixed OTP and compare the remaining fields to the fixture
+	flowWithHashedNonce := mock.MatchedBy(func(got *authenticate.Flow) bool {
+		if bcrypt.CompareHashAndPassword([]byte(got.Nonce), []byte(flow.Nonce)) != nil {
+			return false
+		}
+		cp := *got
+		cp.Nonce = flow.Nonce
+		return reflect.DeepEqual(&cp, flow)
+	})
+
 	type args struct {
 		ctx     context.Context
 		request authenticate.RegistrationStartRequest
@@ -342,7 +359,7 @@ func TestService_StartFlow(t *testing.T) {
 				mockFlowRepo, _, _, _, _ := createMocks(t)
 				ctx := context.Background()
 				_ = strategy.NewMailOTP(mockDialer, "test-subject", "test-body")
-				mockFlowRepo.EXPECT().Set(ctx, flow).Return(nil)
+				mockFlowRepo.EXPECT().Set(ctx, flowWithHashedNonce).Return(nil)
 				srv := authenticate.NewService(
 					nil,
 					authenticate.Config{
@@ -374,7 +391,7 @@ func TestService_StartFlow(t *testing.T) {
 				mockFlowRepo, _, _, _, _ := createMocks(t)
 				ctx := context.Background()
 				_ = strategy.NewMailOTP(mockDialer, "test-subject", "test-body")
-				mockFlowRepo.EXPECT().Set(ctx, flow).Return(sampleErr)
+				mockFlowRepo.EXPECT().Set(ctx, flowWithHashedNonce).Return(sampleErr)
 				srv := authenticate.NewService(
 					nil,
 					authenticate.Config{
@@ -431,11 +448,214 @@ func TestService_StartFlow(t *testing.T) {
 			} else {
 				require.NoError(t, err)
 			}
+			if tt.want != nil && got != nil {
+				// nonce is a bcrypt hash; the Set matcher already verified it
+				got.Flow.Nonce = tt.want.Flow.Nonce
+			}
 			assert.Equal(t, tt.want, got)
 		})
 	}
 }
 
+func mailOTPFlow(id uuid.UUID, now time.Time, nonce string, md pkgMetadata.Metadata) *authenticate.Flow {
+	return &authenticate.Flow{
+		ID:        id,
+		Method:    authenticate.MailOTPAuthMethod.String(),
+		CreatedAt: now,
+		ExpiresAt: now.Add(10 * time.Minute),
+		Email:     "test@example.com",
+		Nonce:     nonce,
+		Metadata:  md,
+	}
+}
+
+func TestService_FinishFlow(t *testing.T) {
+	flowID := uuid.New()
+	timeNow := time.Now()
+	otpHash, err := bcrypt.GenerateFromPassword([]byte("111111"), bcrypt.MinCost)
+	require.NoError(t, err)
+	sampleUser := user.User{ID: "user-id", Email: "test@example.com"}
+
+	type args struct {
+		ctx     context.Context
+		request authenticate.RegistrationFinishRequest
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    *authenticate.RegistrationFinishResponse
+		wantErr error
+		setup   func() *authenticate.Service
+	}{
+		{
+			name: "return the user and consume the flow if the code is valid",
+			args: args{
+				ctx: context.Background(),
+				request: authenticate.RegistrationFinishRequest{
+					Method: authenticate.MailOTPAuthMethod.String(),
+					State:  flowID.String(),
+					Code:   "111111",
+				},
+			},
+			want: &authenticate.RegistrationFinishResponse{
+				User: sampleUser,
+				Flow: mailOTPFlow(flowID, timeNow, string(otpHash), pkgMetadata.Metadata{"callback_url": ""}),
+			},
+			wantErr: nil,
+			setup: func() *authenticate.Service {
+				mockFlowRepo, mockUserService, _, _, _ := createMocks(t)
+				ctx := context.Background()
+				mockFlowRepo.EXPECT().Get(ctx, flowID).
+					Return(mailOTPFlow(flowID, timeNow, string(otpHash), pkgMetadata.Metadata{"callback_url": ""}), nil)
+				mockFlowRepo.EXPECT().Delete(ctx, flowID).Return(nil)
+				mockUserService.EXPECT().GetByID(ctx, "test@example.com").Return(sampleUser, nil)
+				srv := authenticate.NewService(nil, authenticate.Config{}, mockFlowRepo, nil,
+					nil, nil, mockUserService, nil, nil, nil)
+				srv.Now = func() time.Time {
+					return timeNow
+				}
+				return srv
+			},
+		},
+		{
+			name: "return ErrInvalidMailOTP and record the attempt if the code is wrong",
+			args: args{
+				ctx: context.Background(),
+				request: authenticate.RegistrationFinishRequest{
+					Method: authenticate.MailOTPAuthMethod.String(),
+					State:  flowID.String(),
+					Code:   "222222",
+				},
+			},
+			want:    nil,
+			wantErr: authenticate.ErrInvalidMailOTP,
+			setup: func() *authenticate.Service {
+				mockFlowRepo, _, _, _, _ := createMocks(t)
+				ctx := context.Background()
+				mockFlowRepo.EXPECT().Get(ctx, flowID).
+					Return(mailOTPFlow(flowID, timeNow, string(otpHash), pkgMetadata.Metadata{"callback_url": ""}), nil)
+				mockFlowRepo.EXPECT().Set(ctx, mock.MatchedBy(func(f *authenticate.Flow) bool {
+					return f.Metadata["attempt"] == 1 && f.Nonce == string(otpHash)
+				})).Return(nil)
+				srv := authenticate.NewService(nil, authenticate.Config{}, mockFlowRepo, nil,
+					nil, nil, nil, nil, nil, nil)
+				srv.Now = func() time.Time {
+					return timeNow
+				}
+				return srv
+			},
+		},
+		{
+			name: "reject the correct code if the stored nonce is not hashed",
+			args: args{
+				ctx: context.Background(),
+				request: authenticate.RegistrationFinishRequest{
+					Method: authenticate.MailOTPAuthMethod.String(),
+					State:  flowID.String(),
+					Code:   "111111",
+				},
+			},
+			want:    nil,
+			wantErr: authenticate.ErrInvalidMailOTP,
+			setup: func() *authenticate.Service {
+				mockFlowRepo, _, _, _, _ := createMocks(t)
+				ctx := context.Background()
+				mockFlowRepo.EXPECT().Get(ctx, flowID).
+					Return(mailOTPFlow(flowID, timeNow, "111111", pkgMetadata.Metadata{"callback_url": ""}), nil)
+				mockFlowRepo.EXPECT().Set(ctx, mock.MatchedBy(func(f *authenticate.Flow) bool {
+					return f.Metadata["attempt"] == 1 && f.Nonce == "111111"
+				})).Return(nil)
+				srv := authenticate.NewService(nil, authenticate.Config{}, mockFlowRepo, nil,
+					nil, nil, nil, nil, nil, nil)
+				srv.Now = func() time.Time {
+					return timeNow
+				}
+				return srv
+			},
+		},
+		{
+			name: "destroy the flow if the code is wrong past the attempt cap",
+			args: args{
+				ctx: context.Background(),
+				request: authenticate.RegistrationFinishRequest{
+					Method: authenticate.MailOTPAuthMethod.String(),
+					State:  flowID.String(),
+					Code:   "222222",
+				},
+			},
+			want:    nil,
+			wantErr: authenticate.ErrInvalidMailOTP,
+			setup: func() *authenticate.Service {
+				mockFlowRepo, _, _, _, _ := createMocks(t)
+				ctx := context.Background()
+				mockFlowRepo.EXPECT().Get(ctx, flowID).
+					Return(mailOTPFlow(flowID, timeNow, string(otpHash), pkgMetadata.Metadata{"callback_url": "", "attempt": 3}), nil)
+				mockFlowRepo.EXPECT().Delete(ctx, flowID).Return(nil)
+				srv := authenticate.NewService(nil, authenticate.Config{}, mockFlowRepo, nil,
+					nil, nil, nil, nil, nil, nil)
+				srv.Now = func() time.Time {
+					return timeNow
+				}
+				return srv
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			s := tt.setup()
+			got, err := s.FinishFlow(tt.args.ctx, tt.args.request)
+			if tt.wantErr != nil {
+				require.Error(t, err)
+				assert.ErrorIs(t, err, tt.wantErr)
+			} else {
+				require.NoError(t, err)
+			}
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
+func TestService_FinishFlow_WrongThenRightOTP(t *testing.T) {
+	flowID := uuid.New()
+	timeNow := time.Now()
+	otpHash, err := bcrypt.GenerateFromPassword([]byte("111111"), bcrypt.MinCost)
+	require.NoError(t, err)
+	sampleUser := user.User{ID: "user-id", Email: "test@example.com"}
+
+	ctx := context.Background()
+	mockFlowRepo, mockUserService, _, _, _ := createMocks(t)
+	flow := mailOTPFlow(flowID, timeNow, string(otpHash), pkgMetadata.Metadata{"callback_url": ""})
+
+	mockFlowRepo.EXPECT().Get(ctx, flowID).Return(flow, nil).Twice()
+	mockFlowRepo.EXPECT().Set(ctx, flow).Return(nil).Once()
+	mockFlowRepo.EXPECT().Delete(ctx, flowID).Return(nil).Once()
+	mockUserService.EXPECT().GetByID(ctx, "test@example.com").Return(sampleUser, nil).Once()
+
+	srv := authenticate.NewService(nil, authenticate.Config{}, mockFlowRepo, nil,
+		nil, nil, mockUserService, nil, nil, nil)
+	srv.Now = func() time.Time {
+		return timeNow
+	}
+
+	request := func(code string) authenticate.RegistrationFinishRequest {
+		return authenticate.RegistrationFinishRequest{
+			Method: authenticate.MailOTPAuthMethod.String(),
+			State:  flowID.String(),
+			Code:   code,
+		}
+	}
+
+	got, err := srv.FinishFlow(ctx, request("222222"))
+	assert.ErrorIs(t, err, authenticate.ErrInvalidMailOTP)
+	assert.Nil(t, got)
+	assert.Equal(t, 1, flow.Metadata["attempt"])
+
+	got, err = srv.FinishFlow(ctx, request("111111"))
+	require.NoError(t, err)
+	require.NotNil(t, got)
+	assert.Equal(t, sampleUser, got.User)
+}
+
 func TestService_GetPrincipal_JWTGrantSkipsNonGrantToken(t *testing.T) {
 	userID := uuid.New()
 	patValue := "fpt_opaque-not-a-jwt"