satellite/console: add rate limiter to login, register, password recovery

Added a per IP rate limiter to the console web.
Cleaned up password check to leak less bcyrpt info.

Change-Id: I3c882978bd8de3ee9428cb6434a41ab2fc405fb2

private/testplanet/satellite.go

@@ -27,6 +27,7 @@ import (
 	"storj.io/storj/pkg/revocation"
 	"storj.io/storj/pkg/server"
 	versionchecker "storj.io/storj/private/version/checker"
+	"storj.io/storj/private/web"
 	"storj.io/storj/satellite"
 	"storj.io/storj/satellite/accounting"
 	"storj.io/storj/satellite/accounting/live"
@@ -456,6 +457,11 @@ func (planet *Planet) newSatellites(count int, satelliteDatabases satellitedbtes
 				Config: console.Config{
 					PasswordCost: console.TestPasswordCost,
 				},
+				RateLimit: web.IPRateLimiterConfig{
+					Duration:  5 * time.Minute,
+					Burst:     3,
+					NumLimits: 10,
+				},
 			},
 			Marketing: marketingweb.Config{
 				Address:   "127.0.0.1:0",

private/web/ratelimiter.go

@@ -0,0 +1,135 @@
+// Copyright (C) 2019 Storj Labs, Inc.
+// See LICENSE for copying information.
+
+package web
+
+import (
+	"context"
+	"net"
+	"net/http"
+	"sync"
+	"time"
+
+	"golang.org/x/time/rate"
+)
+
+//IPRateLimiterConfig configures an IPRateLimiter.
+type IPRateLimiterConfig struct {
+	Duration  time.Duration `help:"the rate at which request are allowed" default:"5m"`
+	Burst     int           `help:"number of events before the limit kicks in" default:"3"`
+	NumLimits int           `help:"number of IPs whose rate limits we store" default:"1000"`
+}
+
+//IPRateLimiter imposes a rate limit per HTTP user IP.
+type IPRateLimiter struct {
+	config   IPRateLimiterConfig
+	mu       sync.Mutex
+	ipLimits map[string]*userLimit
+}
+
+//userLimit is the per-IP limiter.
+type userLimit struct {
+	//TODO:  consider using redis instead of in-memory state
+	// https://storjlabs.atlassian.net/browse/SM-749
+	//TODO:  consider using a single timer and fixed sized channels instead
+	// https://storjlabs.atlassian.net/browse/SM-750
+	limiter  *rate.Limiter
+	lastSeen time.Time
+}
+
+//NewIPRateLimiter constructs an IPRateLimiter.
+func NewIPRateLimiter(config IPRateLimiterConfig) *IPRateLimiter {
+	return &IPRateLimiter{
+		config:   config,
+		ipLimits: make(map[string]*userLimit),
+	}
+}
+
+// Run occasionally cleans old rate-limiting data, until context cancel.
+func (rl *IPRateLimiter) Run(ctx context.Context) {
+	cleanupTicker := time.NewTicker(rl.config.Duration)
+	defer cleanupTicker.Stop()
+	for {
+		select {
+		case <-ctx.Done():
+			return
+		case <-cleanupTicker.C:
+			rl.cleanupLimiters()
+		}
+	}
+}
+
+// cleanupLimiters removes old rate limits to free memory.
+func (rl *IPRateLimiter) cleanupLimiters() {
+	rl.mu.Lock()
+	defer rl.mu.Unlock()
+	for ip, v := range rl.ipLimits {
+		if time.Since(v.lastSeen) > rl.config.Duration {
+			delete(rl.ipLimits, ip)
+		}
+	}
+}
+
+//Limit applies a per IP rate limiting as an HTTP Handler.
+func (rl *IPRateLimiter) Limit(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		ip, _, err := net.SplitHostPort(r.RemoteAddr)
+		if err != nil {
+			http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
+			return
+		}
+		ipLimit := rl.getUserLimit(ip)
+		if !ipLimit.Allow() {
+			http.Error(w, http.StatusText(http.StatusTooManyRequests), http.StatusTooManyRequests)
+			return
+		}
+		next.ServeHTTP(w, r)
+	})
+}
+
+//getUserLimit returns a rate limiter for an IP.
+func (rl *IPRateLimiter) getUserLimit(ip string) *rate.Limiter {
+	rl.mu.Lock()
+	defer rl.mu.Unlock()
+
+	v, exists := rl.ipLimits[ip]
+	if !exists {
+		if len(rl.ipLimits) == rl.config.NumLimits {
+			// Tracking only N limits prevents an out-of-memory DOS attack
+			// Returning StatusTooManyRequests would be just as bad
+			// The least-bad option may be to remove the oldest key
+			oldestKey := ""
+			var oldestTime *time.Time
+			for ip, v := range rl.ipLimits {
+				// while we're looping, we'd prefer to just delete expired records
+				if time.Since(v.lastSeen) > rl.config.Duration {
+					delete(rl.ipLimits, ip)
+				}
+				// but we're prepared to delete the oldest non-expired
+				if oldestTime == nil || v.lastSeen.Before(*oldestTime) {
+					oldestTime = &v.lastSeen
+					oldestKey = ip
+				}
+			}
+			//only delete the oldest non-expired if there's still an issue
+			if oldestKey != "" && len(rl.ipLimits) == rl.config.NumLimits {
+				delete(rl.ipLimits, oldestKey)
+			}
+		}
+		limiter := rate.NewLimiter(rate.Limit(time.Second)/rate.Limit(rl.config.Duration), rl.config.Burst)
+		rl.ipLimits[ip] = &userLimit{limiter, time.Now()}
+		return limiter
+	}
+	v.lastSeen = time.Now()
+	return v.limiter
+}
+
+//Burst returns the number of events that happen before the rate limit.
+func (rl *IPRateLimiter) Burst() int {
+	return rl.config.Burst
+}
+
+//Duration returns the amount of time required between events.
+func (rl *IPRateLimiter) Duration() time.Duration {
+	return rl.config.Duration
+}

private/web/ratelimiter_test.go

@@ -0,0 +1,65 @@
+// Copyright (C) 2019 Storj Labs, Inc.
+// See LICENSE for copying information.
+
+package web_test
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/spf13/pflag"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"storj.io/common/testcontext"
+	"storj.io/private/cfgstruct"
+	"storj.io/storj/private/web"
+)
+
+func TestNewIPRateLimiter(t *testing.T) {
+	//create a rate limiter with defaults except NumLimits = 2
+	config := web.IPRateLimiterConfig{}
+	cfgstruct.Bind(&pflag.FlagSet{}, &config, cfgstruct.UseDevDefaults())
+	config.NumLimits = 2
+	rateLimiter := web.NewIPRateLimiter(config)
+	//run ratelimiter cleanup until end of test
+	ctx := testcontext.New(t)
+	defer ctx.Cleanup()
+	ctx2, cancel := context.WithCancel(ctx)
+	defer cancel()
+	ctx.Go(func() error {
+		rateLimiter.Run(ctx2)
+		return nil
+	})
+	//make the default HTTP handler return StatusOK
+	handler := rateLimiter.Limit(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	//expect burst number of successes
+	testWithAddress(t, "192.168.1.1:5000", rateLimiter.Burst(), handler)
+	//expect similar results for a different IP
+	testWithAddress(t, "127.0.0.1:5000", rateLimiter.Burst(), handler)
+	//expect similar results for a different IP
+	testWithAddress(t, "127.0.0.100:5000", rateLimiter.Burst(), handler)
+	//expect original IP to work again because numLimits == 2
+	testWithAddress(t, "192.168.1.1:5000", rateLimiter.Burst(), handler)
+}
+
+func testWithAddress(t *testing.T, remoteAddress string, burst int, handler http.Handler) {
+	//create HTTP request
+	req, err := http.NewRequest("GET", "", nil)
+	require.NoError(t, err)
+	req.RemoteAddr = remoteAddress
+	//expect burst number of successes
+	for x := 0; x < burst; x++ {
+		rr := httptest.NewRecorder()
+		handler.ServeHTTP(rr, req)
+		assert.Equal(t, rr.Code, http.StatusOK, remoteAddress)
+	}
+	//then expect failure
+	rr := httptest.NewRecorder()
+	handler.ServeHTTP(rr, req)
+	assert.Equal(t, rr.Code, http.StatusTooManyRequests, remoteAddress)
+}

satellite/console/consoleweb/consoleapi/auth.go

@@ -67,6 +67,7 @@ func (a *Auth) Token(w http.ResponseWriter, r *http.Request) {
 
 	token, err := a.service.Token(ctx, tokenRequest.Email, tokenRequest.Password)
 	if err != nil {
+		a.log.Info("Error authenticating token request", zap.String("email", tokenRequest.Email), zap.Error(ErrAuthAPI.Wrap(err)))
 		a.serveJSONError(w, err)
 		return
 	}

satellite/console/consoleweb/server.go

@@ -32,6 +32,7 @@ import (
 	"storj.io/common/errs2"
 	"storj.io/common/uuid"
 	"storj.io/storj/pkg/auth"
+	"storj.io/storj/private/web"
 	"storj.io/storj/satellite/console"
 	"storj.io/storj/satellite/console/consoleweb/consoleapi"
 	"storj.io/storj/satellite/console/consoleweb/consoleql"
@@ -75,6 +76,8 @@ type Config struct {
 	AccountActivationRedirectURL string `help:"url link for account activation redirect" default:""`
 	VerificationPageURL          string `help:"url link to sign up verification page" default:"https://tardigrade.io/satellites/verify"`
 
+	RateLimit web.IPRateLimiterConfig
+
 	console.Config
 }
 
@@ -89,9 +92,10 @@ type Server struct {
 	mailService      *mailservice.Service
 	referralsService *referrals.Service
 
-	listener   net.Listener
-	server     http.Server
-	cookieAuth *consolewebauth.CookieAuth
+	listener    net.Listener
+	server      http.Server
+	cookieAuth  *consolewebauth.CookieAuth
+	rateLimiter *web.IPRateLimiter
 
 	stripePublicKey string
 
@@ -117,6 +121,7 @@ func NewServer(logger *zap.Logger, config Config, service *console.Service, mail
 		mailService:      mailService,
 		referralsService: referralsService,
 		stripePublicKey:  stripePublicKey,
+		rateLimiter:      web.NewIPRateLimiter(config.RateLimit),
 	}
 
 	logger.Debug("Starting Satellite UI.", zap.Stringer("Address", server.listener.Addr()))
@@ -164,10 +169,10 @@ func NewServer(logger *zap.Logger, config Config, service *console.Service, mail
 	authRouter.Handle("/account/change-password", server.withAuth(http.HandlerFunc(authController.ChangePassword))).Methods(http.MethodPost)
 	authRouter.Handle("/account/delete", server.withAuth(http.HandlerFunc(authController.DeleteAccount))).Methods(http.MethodPost)
 	authRouter.HandleFunc("/logout", authController.Logout).Methods(http.MethodPost)
-	authRouter.HandleFunc("/token", authController.Token).Methods(http.MethodPost)
-	authRouter.HandleFunc("/register", authController.Register).Methods(http.MethodPost)
-	authRouter.HandleFunc("/forgot-password/{email}", authController.ForgotPassword).Methods(http.MethodPost)
-	authRouter.HandleFunc("/resend-email/{id}", authController.ResendEmail).Methods(http.MethodPost)
+	authRouter.Handle("/token", server.rateLimiter.Limit(http.HandlerFunc(authController.Token))).Methods(http.MethodPost)
+	authRouter.Handle("/register", server.rateLimiter.Limit(http.HandlerFunc(authController.Register))).Methods(http.MethodPost)
+	authRouter.Handle("/forgot-password/{email}", server.rateLimiter.Limit(http.HandlerFunc(authController.ForgotPassword))).Methods(http.MethodPost)
+	authRouter.Handle("/resend-email/{id}", server.rateLimiter.Limit(http.HandlerFunc(authController.ResendEmail))).Methods(http.MethodPost)
 
 	paymentController := consoleapi.NewPayments(logger, service)
 	paymentsRouter := router.PathPrefix("/api/v0/payments").Subrouter()
@@ -220,6 +225,10 @@ func (server *Server) Run(ctx context.Context) (err error) {
 		<-ctx.Done()
 		return server.server.Shutdown(context.Background())
 	})
+	group.Go(func() error {
+		server.rateLimiter.Run(ctx)
+		return nil
+	})
 	group.Go(func() error {
 		defer cancel()
 		err := server.server.Serve(server.listener)

satellite/console/service.go

@@ -654,11 +654,7 @@ func (s *Service) Token(ctx context.Context, email, password string) (token stri
 
 	err = bcrypt.CompareHashAndPassword(user.PasswordHash, []byte(password))
 	if err != nil {
-		if err == bcrypt.ErrMismatchedHashAndPassword {
-			return "", ErrUnauthorized.New(credentialsErrMsg)
-		}
-
-		return "", Error.Wrap(err)
+		return "", ErrUnauthorized.New(credentialsErrMsg)
 	}
 
 	claims := consoleauth.Claims{
@@ -737,11 +733,7 @@ func (s *Service) ChangePassword(ctx context.Context, pass, newPass string) (err
 
 	err = bcrypt.CompareHashAndPassword(auth.User.PasswordHash, []byte(pass))
 	if err != nil {
-		if err == bcrypt.ErrMismatchedHashAndPassword {
-			return ErrUnauthorized.Wrap(err)
-		}
-
-		return Error.Wrap(err)
+		return ErrUnauthorized.New(credentialsErrMsg)
 	}
 
 	if err := ValidatePassword(newPass); err != nil {
@@ -772,11 +764,7 @@ func (s *Service) DeleteAccount(ctx context.Context, password string) (err error
 
 	err = bcrypt.CompareHashAndPassword(auth.User.PasswordHash, []byte(password))
 	if err != nil {
-		if err == bcrypt.ErrMismatchedHashAndPassword {
-			return ErrUnauthorized.Wrap(err)
-		}
-
-		return Error.Wrap(err)
+		return ErrUnauthorized.New(credentialsErrMsg)
 	}
 
 	err = s.store.Users().Delete(ctx, auth.User.ID)

scripts/testdata/satellite-config.yaml.lock

@@ -88,6 +88,15 @@ compensation.withheld-percents: 75,75,75,50,50,50,25,25,25,0,0,0,0,0,0
 # enable open registration
 # console.open-registration-enabled: false
 
+# number of events before the limit kicks in
+# console.rate-limit.burst: 3
+
+# the rate at which request are allowed
+# console.rate-limit.duration: 5m0s
+
+# number of IPs whose rate limits we store
+# console.rate-limit.num-limits: 1000
+
 # used to display at web satellite console
 # console.satellite-name: Storj