Prevent user enumeration (#533)

- Return same error in all cases for login endpoint.
- Log error details in server logs.
- Make most login errors take ~1s to prevent timing attacks.
- Don't return forgot password errors.
- Log password errors in server logs.
- Make most forgot password requests take ~1s to prevent timing attacks.

Fixes #531

cmd/fleetctl/login.go

@@ -71,12 +71,10 @@ Interactively prompts for email and password if not specified in the flags or en
 			token, err := fleet.Login(flEmail, flPassword)
 			if err != nil {
 				switch err.(type) {
-				case service.InvalidLoginErr:
-					return err
 				case service.NotSetupErr:
 					return err
 				}
-				return errors.Wrap(err, "error logging in")
+				return errors.Wrap(err, "Login failed")
 			}
 
 			configPath, context := c.String("config"), c.String("context")

cypress/integration/sessions/sessions.spec.ts

@@ -40,7 +40,7 @@ describe('Sessions', () => {
       .click();
 
     cy.url().should('match', /\/login$/);
-    cy.contains('username or email and password do not match');
+    cy.contains('Authentication failed');
   });
 
   it('Fails to access authenticated resource', () => {

server/service/client_errors.go

@@ -20,21 +20,6 @@ func (e setupAlreadyErr) SetupAlready() bool {
 	return true
 }
 
-type InvalidLoginErr interface {
-	InvalidLogin() bool
-	Error() string
-}
-
-type invalidLoginErr struct{}
-
-func (e invalidLoginErr) Error() string {
-	return "The credentials supplied were invalid"
-}
-
-func (e invalidLoginErr) InvalidLogin() bool {
-	return true
-}
-
 type NotSetupErr interface {
 	NotSetup() bool
 	Error() string

server/service/client_sessions.go

@@ -24,8 +24,6 @@ func (c *Client) Login(email, password string) (string, error) {
 	switch response.StatusCode {
 	case http.StatusNotFound:
 		return "", notSetupErr{}
-	case http.StatusUnauthorized:
-		return "", invalidLoginErr{}
 	}
 	if response.StatusCode != http.StatusOK {
 		return "", errors.Errorf(

server/service/endpoint_middleware.go

@@ -70,7 +70,7 @@ func authenticatedUser(jwtKey string, svc kolide.Service, next endpoint.Endpoint
 		// if not succesful, try again this time with errors
 		bearer, ok := token.FromContext(ctx)
 		if !ok {
-			return nil, authError{reason: "no auth token"}
+			return nil, authRequiredError{internal: "no auth token"}
 		}
 
 		v, err := authViewer(ctx, jwtKey, bearer, svc)
@@ -92,30 +92,30 @@ func authViewer(ctx context.Context, jwtKey string, bearerToken token.Token, svc
 		return []byte(jwtKey), nil
 	})
 	if err != nil {
-		return nil, authError{reason: err.Error()}
+		return nil, authRequiredError{internal: err.Error()}
 	}
-	if jwtToken.Valid != true {
-		return nil, authError{reason: "invalid jwt token"}
+	if !jwtToken.Valid {
+		return nil, authRequiredError{internal: "invalid jwt token"}
 	}
 	claims, ok := jwtToken.Claims.(jwt.MapClaims)
 	if !ok {
-		return nil, authError{reason: "no jwt claims"}
+		return nil, authRequiredError{internal: "no jwt claims"}
 	}
 	sessionKeyClaim, ok := claims["session_key"]
 	if !ok {
-		return nil, authError{reason: "no session_key in JWT claims"}
+		return nil, authRequiredError{internal: "no session_key in JWT claims"}
 	}
 	sessionKey, ok := sessionKeyClaim.(string)
 	if !ok {
-		return nil, authError{reason: "non-string key in sessionClaim"}
+		return nil, authRequiredError{internal: "non-string key in sessionClaim"}
 	}
 	session, err := svc.GetSessionByKey(ctx, sessionKey)
 	if err != nil {
-		return nil, authError{reason: err.Error()}
+		return nil, authRequiredError{internal: err.Error()}
 	}
 	user, err := svc.User(ctx, session.UserID)
 	if err != nil {
-		return nil, authError{reason: err.Error()}
+		return nil, authRequiredError{internal: err.Error()}
 	}
 	return &viewer.Viewer{User: user, Session: session}, nil
 }

server/service/endpoint_sessions.go

@@ -6,8 +6,8 @@ import (
 	"html/template"
 	"time"
 
-	"github.com/go-kit/kit/endpoint"
 	"github.com/fleetdm/fleet/server/kolide"
+	"github.com/go-kit/kit/endpoint"
 )
 
 ////////////////////////////////////////////////////////////////////////////////

server/service/endpoint_users.go

@@ -4,8 +4,8 @@ import (
 	"context"
 	"net/http"
 
-	"github.com/go-kit/kit/endpoint"
 	"github.com/fleetdm/fleet/server/kolide"
+	"github.com/go-kit/kit/endpoint"
 )
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -308,10 +308,10 @@ func (r forgotPasswordResponse) status() int  { return http.StatusAccepted }
 func makeForgotPasswordEndpoint(svc kolide.Service) endpoint.Endpoint {
 	return func(ctx context.Context, request interface{}) (interface{}, error) {
 		req := request.(forgotPasswordRequest)
-		err := svc.RequestPasswordReset(ctx, req.Email)
-		if err != nil {
-			return forgotPasswordResponse{Err: err}, nil
-		}
+		// Any error returned by the service should not be returned to the
+		// client to prevent information disclosure (it will be logged in the
+		// server logs).
+		_ = svc.RequestPasswordReset(ctx, req.Email)
 		return forgotPasswordResponse{}, nil
 	}
 }

server/service/logging.go

@@ -19,13 +19,21 @@ func NewLoggingService(svc kolide.Service, logger kitlog.Logger) kolide.Service
 
 // loggerDebug returns the the info level if there error is non-nil, otherwise defaulting to the debug level.
 func (mw loggingMiddleware) loggerDebug(err error) kitlog.Logger {
-	if err != nil {
-		return level.Info(mw.logger)
+	logger := mw.logger
+	if e, ok := err.(ErrWithInternal); ok {
+		logger = kitlog.With(logger, "err_internal", e.Internal())
 	}
-	return level.Debug(mw.logger)
+	if err != nil {
+		return level.Info(logger)
+	}
+	return level.Debug(logger)
 }
 
 // loggerInfo returns the info level
 func (mw loggingMiddleware) loggerInfo(err error) kitlog.Logger {
-	return level.Info(mw.logger)
+	logger := mw.logger
+	if e, ok := err.(ErrWithInternal); ok {
+		logger = kitlog.With(logger, "err_internal", e.Internal())
+	}
+	return level.Info(logger)
 }

server/service/service_errors.go

@@ -1,6 +1,23 @@
 package service
 
-import "fmt"
+import (
+	"fmt"
+	"net/http"
+)
+
+// ErrWithInternal is an interface for errors that include extra "internal"
+// information that should be logged in server logs but not sent to clients.
+type ErrWithInternal interface {
+	error
+	Internal() string
+}
+
+// ErrWithStatusCode is an interface for errors that should set a specific HTTP
+// status when encoding.
+type ErrWithStatusCode interface {
+	error
+	StatusCode() int
+}
 
 type invalidArgumentError []invalidArgument
 type invalidArgument struct {
@@ -57,24 +74,38 @@ func (e invalidArgumentError) Invalid() []map[string]string {
 	return invalid
 }
 
-// authentication error
-type authError struct {
-	reason string
-	// client reason is used to provide
-	// a different error message to the client
-	// when security is a concern
-	clientReason string
+type authFailedError struct {
+	// internal is the reason that should only be logged internally
+	internal string
 }
 
-func (e authError) Error() string {
-	return e.reason
+func (e authFailedError) Error() string {
+	return "Authentication failed"
 }
 
-func (e authError) AuthError() string {
-	if e.clientReason != "" {
-		return e.clientReason
-	}
-	return "username or email and password do not match"
+func (e authFailedError) Internal() string {
+	return e.internal
+}
+
+func (e authFailedError) StatusCode() int {
+	return http.StatusUnauthorized
+}
+
+type authRequiredError struct {
+	// internal is the reason that should only be logged internally
+	internal string
+}
+
+func (e authRequiredError) Error() string {
+	return "Authentication required"
+}
+
+func (e authRequiredError) Internal() string {
+	return e.internal
+}
+
+func (e authRequiredError) StatusCode() int {
+	return http.StatusUnauthorized
 }
 
 // permissionError, set when user is authenticated, but not allowed to perform action

server/service/service_sessions.go

@@ -123,26 +123,38 @@ func (svc service) CallbackSSO(ctx context.Context, auth kolide.Auth) (*kolide.S
 }
 
 func (svc service) Login(ctx context.Context, username, password string) (*kolide.User, string, error) {
+	// If there is an error, sleep until the request has taken at least 1
+	// second. This means that generally a login failure for any reason will
+	// take ~1s and frustrate a timing attack.
+	var err error
+	defer func(start time.Time) {
+		if err != nil {
+			time.Sleep(time.Until(start.Add(1 * time.Second)))
+		}
+	}(time.Now())
+
 	user, err := svc.userByEmailOrUsername(username)
 	if _, ok := err.(kolide.NotFoundError); ok {
-		return nil, "", authError{reason: "no such user"}
+		return nil, "", authFailedError{internal: "user not found"}
 	}
 	if err != nil {
-		return nil, "", err
+		return nil, "", authFailedError{internal: err.Error()}
 	}
+
+	if err = user.ValidatePassword(password); err != nil {
+		return nil, "", authFailedError{internal: "invalid password"}
+	}
+
 	if !user.Enabled {
-		return nil, "", authError{reason: "account disabled", clientReason: "account disabled"}
+		return nil, "", authFailedError{internal: "account disabled"}
 	}
 	if user.SSOEnabled {
-		const errMessage = "password login not allowed for single sign on users"
-		return nil, "", authError{reason: errMessage, clientReason: errMessage}
-	}
-	if err = user.ValidatePassword(password); err != nil {
-		return nil, "", authError{reason: "bad password"}
+		return nil, "", authFailedError{internal: "password login disabled for sso users"}
 	}
+
 	token, err := svc.makeSession(user.ID)
 	if err != nil {
-		return nil, "", err
+		return nil, "", authFailedError{internal: err.Error()}
 	}
 
 	return user, token, nil
@@ -261,10 +273,7 @@ func (svc service) DeleteSession(ctx context.Context, id uint) error {
 
 func (svc service) validateSession(session *kolide.Session) error {
 	if session == nil {
-		return authError{
-			reason:       "active session not present",
-			clientReason: "session error",
-		}
+		return authRequiredError{internal: "active session not present"}
 	}
 
 	sessionDuration := svc.config.Session.Duration
@@ -274,10 +283,7 @@ func (svc service) validateSession(session *kolide.Session) error {
 		if err != nil {
 			return errors.Wrap(err, "destroying session")
 		}
-		return authError{
-			reason:       "expired session",
-			clientReason: "session error",
-		}
+		return authRequiredError{internal: "expired session"}
 	}
 
 	return svc.ds.MarkSessionAccessed(session)

server/service/service_users.go

@@ -322,6 +322,12 @@ func (svc service) RequirePasswordReset(ctx context.Context, uid uint, require b
 }
 
 func (svc service) RequestPasswordReset(ctx context.Context, email string) error {
+	// Regardless of error, sleep until the request has taken at least 1 second.
+	// This means that any request to this method will take ~1s and frustrate a timing attack.
+	defer func(start time.Time) {
+		time.Sleep(time.Until(start.Add(1 * time.Second)))
+	}(time.Now())
+
 	user, err := svc.ds.UserByEmail(email)
 	if err != nil {
 		return err

server/service/transport_error.go

@@ -149,9 +149,16 @@ func encodeError(ctx context.Context, err error, w http.ResponseWriter) {
 		return
 	}
 
-	w.WriteHeader(http.StatusInternalServerError)
+	// Get specific status code if it is available from this error type,
+	// defaulting to HTTP 500
+	status := http.StatusInternalServerError
+	if e, ok := err.(ErrWithStatusCode); ok {
+		status = e.StatusCode()
+	}
+
+	w.WriteHeader(status)
 	je := jsonError{
-		Message: "Unknown Error",
+		Message: err.Error(),
 		Errors:  baseError(err.Error()),
 	}
 	enc.Encode(je)