Address review feedback

api/types/events/events.proto

@@ -520,8 +520,8 @@ message UserLogin {
     google.protobuf.Struct IdentityAttributes = 5
         [ (gogoproto.jsontag) = "attributes,omitempty", (gogoproto.casttype) = "Struct" ];
 
-    // With2FA is the UUID of an MFA device used during the login.
-    string With2FA = 6 [ (gogoproto.jsontag) = "with_2fa,omitempty" ];
+    // MFA is the MFA device used during the login.
+    MFADeviceMetadata MFADevice = 6 [ (gogoproto.jsontag) = "mfa_device,omitempty" ];
 }
 
 // ResourceMetadata is a common resource metadata
@@ -1210,7 +1210,7 @@ message MFADeviceDelete {
     // User is a common user event metadata.
     UserMetadata User = 2
         [ (gogoproto.nullable) = false, (gogoproto.embed) = true, (gogoproto.jsontag) = "" ];
-    // Device is the new MFA device deleted by the user.
+    // Device is the MFA device deleted by the user.
     MFADeviceMetadata Device = 3 [ (gogoproto.nullable) = false, (gogoproto.jsontag) = "device" ];
 }
 

lib/auth/auth.go

@@ -57,13 +57,12 @@ import (
 
 	"github.com/coreos/go-oidc/oauth2"
 	"github.com/coreos/go-oidc/oidc"
+	"github.com/gravitational/trace"
 	"github.com/jonboulle/clockwork"
 	"github.com/pborman/uuid"
 	"github.com/prometheus/client_golang/prometheus"
 	saml2 "github.com/russellhaering/gosaml2"
 	"golang.org/x/crypto/ssh"
-
-	"github.com/gravitational/trace"
 )
 
 // ServerOption allows setting options as functional arguments to Server
@@ -876,7 +875,7 @@ func (a *Server) GetMFAAuthenticateChallenge(user string, password []byte) (*MFA
 	ctx := context.TODO()
 
 	err := a.WithUserLock(user, func() error {
-		return a.CheckPasswordWOToken(user, password)
+		return a.checkPasswordWOToken(user, password)
 	})
 	if err != nil {
 		return nil, trace.Wrap(err)

lib/auth/auth_with_roles.go

@@ -772,7 +772,7 @@ func (a *ServerWithRoles) CheckPassword(user string, password []byte, otpToken s
 	if err := a.currentUserAction(user); err != nil {
 		return trace.Wrap(err)
 	}
-	_, err := a.authServer.CheckPassword(user, password, otpToken)
+	_, err := a.authServer.checkPassword(user, password, otpToken)
 	return trace.Wrap(err)
 }
 

lib/auth/grpcserver.go

@@ -23,15 +23,6 @@ import (
 	"net"
 	"time"
 
-	"github.com/golang/protobuf/ptypes/empty"
-	"github.com/jonboulle/clockwork"
-	"github.com/sirupsen/logrus"
-	"google.golang.org/grpc"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/keepalive"
-	"google.golang.org/grpc/peer"
-	"google.golang.org/grpc/status"
-
 	"github.com/gravitational/teleport"
 	"github.com/gravitational/teleport/api/client"
 	"github.com/gravitational/teleport/api/client/proto"
@@ -44,8 +35,17 @@ import (
 	"github.com/gravitational/teleport/lib/services"
 	"github.com/gravitational/teleport/lib/session"
 	"github.com/gravitational/teleport/lib/utils"
+
+	"github.com/golang/protobuf/ptypes/empty"
 	"github.com/gravitational/trace"
 	"github.com/gravitational/trace/trail"
+	"github.com/jonboulle/clockwork"
+	"github.com/sirupsen/logrus"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/keepalive"
+	"google.golang.org/grpc/peer"
+	"google.golang.org/grpc/status"
 
 	// Register gzip compressor for gRPC.
 	_ "google.golang.org/grpc/encoding/gzip"

lib/auth/helpers.go

@@ -40,9 +40,8 @@ import (
 	"github.com/gravitational/teleport/lib/session"
 	"github.com/gravitational/teleport/lib/utils"
 
-	"github.com/jonboulle/clockwork"
-
 	"github.com/gravitational/trace"
+	"github.com/jonboulle/clockwork"
 )
 
 // TestAuthServerConfig is auth server test config

lib/auth/methods.go

@@ -97,7 +97,8 @@ func (s *Server) AuthenticateUser(req AuthenticateUserRequest) error {
 		Method: events.LoginMethodLocal,
 	}
 	if mfaDev != nil {
-		event.With2FA = mfaDev.Id
+		m := mfaDeviceEventMetadata(mfaDev)
+		event.MFADevice = &m
 	}
 	if err != nil {
 		event.Code = events.UserLocalLoginFailureCode
@@ -143,9 +144,12 @@ func (s *Server) authenticateUser(ctx context.Context, req AuthenticateUserReque
 	case req.OTP != nil:
 		var mfaDev *types.MFADevice
 		err := s.WithUserLock(req.Username, func() error {
-			var err error
-			mfaDev, err = s.CheckPassword(req.Username, req.OTP.Password, req.OTP.Token)
-			return err
+			res, err := s.checkPassword(req.Username, req.OTP.Password, req.OTP.Token)
+			if err != nil {
+				return trace.Wrap(err)
+			}
+			mfaDev = res.mfaDev
+			return nil
 		})
 		if err != nil {
 			// provide obscure message on purpose, while logging the real
@@ -180,7 +184,7 @@ func (s *Server) authenticateUser(ctx context.Context, req AuthenticateUserReque
 			return nil, trace.AccessDenied("missing second factor")
 		}
 		err := s.WithUserLock(req.Username, func() error {
-			return s.CheckPasswordWOToken(req.Username, req.Pass.Password)
+			return s.checkPasswordWOToken(req.Username, req.Pass.Password)
 		})
 		if err != nil {
 			// provide obscure message on purpose, while logging the real

lib/auth/password.go

@@ -92,9 +92,9 @@ func (s *Server) ChangePassword(req services.ChangePasswordReq) error {
 		secondFactor := authPreference.GetSecondFactor()
 		switch secondFactor {
 		case constants.SecondFactorOff:
-			return s.CheckPasswordWOToken(userID, req.OldPassword)
+			return s.checkPasswordWOToken(userID, req.OldPassword)
 		case constants.SecondFactorOTP:
-			_, err := s.CheckPassword(userID, req.OldPassword, req.SecondFactorToken)
+			_, err := s.checkPassword(userID, req.OldPassword, req.SecondFactorToken)
 			return trace.Wrap(err)
 		case constants.SecondFactorU2F:
 			if req.U2FSignResponse == nil {
@@ -105,7 +105,7 @@ func (s *Server) ChangePassword(req services.ChangePasswordReq) error {
 			return trace.Wrap(err)
 		case constants.SecondFactorOn:
 			if req.SecondFactorToken != "" {
-				_, err := s.CheckPassword(userID, req.OldPassword, req.SecondFactorToken)
+				_, err := s.checkPassword(userID, req.OldPassword, req.SecondFactorToken)
 				return trace.Wrap(err)
 			}
 			if req.U2FSignResponse != nil {
@@ -115,7 +115,7 @@ func (s *Server) ChangePassword(req services.ChangePasswordReq) error {
 			return trace.AccessDenied("missing second factor authentication")
 		case constants.SecondFactorOptional:
 			if req.SecondFactorToken != "" {
-				_, err := s.CheckPassword(userID, req.OldPassword, req.SecondFactorToken)
+				_, err := s.checkPassword(userID, req.OldPassword, req.SecondFactorToken)
 				return trace.Wrap(err)
 			}
 			if req.U2FSignResponse != nil {
@@ -160,9 +160,9 @@ func (s *Server) ChangePassword(req services.ChangePasswordReq) error {
 	return nil
 }
 
-// CheckPasswordWOToken checks just password without checking OTP tokens
+// checkPasswordWOToken checks just password without checking OTP tokens
 // used in case of SSH authentication, when token has been validated.
-func (s *Server) CheckPasswordWOToken(user string, password []byte) error {
+func (s *Server) checkPasswordWOToken(user string, password []byte) error {
 	const errMsg = "invalid username or password"
 
 	err := services.VerifyPassword(password)
@@ -195,14 +195,22 @@ func (s *Server) CheckPasswordWOToken(user string, password []byte) error {
 	return nil
 }
 
-// CheckPassword checks the password and OTP token. Called by tsh or lib/web/*.
-func (s *Server) CheckPassword(user string, password []byte, otpToken string) (*types.MFADevice, error) {
-	err := s.CheckPasswordWOToken(user, password)
+type checkPasswordResult struct {
+	mfaDev *types.MFADevice
+}
+
+// checkPassword checks the password and OTP token. Called by tsh or lib/web/*.
+func (s *Server) checkPassword(user string, password []byte, otpToken string) (*checkPasswordResult, error) {
+	err := s.checkPasswordWOToken(user, password)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
 
-	return s.checkOTP(user, otpToken)
+	mfaDev, err := s.checkOTP(user, otpToken)
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
+	return &checkPasswordResult{mfaDev: mfaDev}, nil
 }
 
 // checkOTP determines the type of OTP token used (for legacy HOTP support), fetches the

lib/auth/password_test.go

@@ -121,7 +121,7 @@ func (s *PasswordSuite) TestTiming(c *C) {
 		go func() {
 			defer wg.Done()
 			start := time.Now()
-			err := s.a.CheckPasswordWOToken(username, []byte(password))
+			err := s.a.checkPasswordWOToken(username, []byte(password))
 			resCh <- res{
 				exists:  true,
 				elapsed: time.Since(start),
@@ -132,7 +132,7 @@ func (s *PasswordSuite) TestTiming(c *C) {
 		go func() {
 			defer wg.Done()
 			start := time.Now()
-			err := s.a.CheckPasswordWOToken("blah", []byte(password))
+			err := s.a.checkPasswordWOToken("blah", []byte(password))
 			resCh <- res{
 				exists:  false,
 				elapsed: time.Since(start),
@@ -167,7 +167,7 @@ func (s *PasswordSuite) TestUserNotFound(c *C) {
 	username := "unknown-user"
 	password := "barbaz"
 
-	err := s.a.CheckPasswordWOToken(username, []byte(password))
+	err := s.a.checkPasswordWOToken(username, []byte(password))
 	c.Assert(err, NotNil)
 	// Make sure the error is not a NotFound. That would be a username oracle.
 	c.Assert(trace.IsBadParameter(err), Equals, true)
@@ -259,7 +259,7 @@ func (s *PasswordSuite) TestChangePasswordWithToken(c *C) {
 	c.Assert(err, IsNil)
 
 	// password should be updated
-	err = s.a.CheckPasswordWOToken(username, password)
+	err = s.a.checkPasswordWOToken(username, password)
 	c.Assert(err, IsNil)
 }
 
@@ -296,7 +296,7 @@ func (s *PasswordSuite) TestChangePasswordWithTokenOTP(c *C) {
 	})
 	c.Assert(err, IsNil)
 
-	err = s.a.CheckPasswordWOToken(username, password)
+	err = s.a.checkPasswordWOToken(username, password)
 	c.Assert(err, IsNil)
 }
 

lib/auth/resetpasswordtoken_test.go

@@ -73,7 +73,7 @@ func TestCreateResetPasswordToken(t *testing.T) {
 	require.Empty(t, devs)
 
 	// verify that password was reset
-	err = srv.Auth().CheckPasswordWOToken(username, []byte(pass))
+	err = srv.Auth().checkPasswordWOToken(username, []byte(pass))
 	require.Error(t, err)
 
 	// create another reset token for the same user