diff --git a/core/authenticate/service.go b/core/authenticate/service.go index f045bac87..b76681152 100644 --- a/core/authenticate/service.go +++ b/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 { diff --git a/core/authenticate/service_test.go b/core/authenticate/service_test.go index 665cd443f..32b2b9ff3 100644 --- a/core/authenticate/service_test.go +++ b/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"