From 39e9b1f75ac6598c4d0ceebb98a8981fa1c2ce98 Mon Sep 17 00:00:00 2001
From: Hitoshi Mitake <mitake.hitoshi@gmail.com>
Date: Thu, 13 Oct 2016 19:30:35 +0900
Subject: [PATCH] auth, etcdserver: check password at API layer

The cost of bcrypt password checking is quite high (almost 100ms on a
modern machine) so executing it in apply loop will be
problematic. This commit exclude the checking mechanism to the API
layer. The password checking is validated with the OCC like way
similar to the auth of serializable get.

This commit also removes a unit test of Authenticate RPC from
auth/store_test.go. It is because the RPC now accepts an auth request
unconditionally and delegates the checking functionality to
authStore.CheckPassword() (so a unit test for CheckPassword() is
added). The combination of the two functionalities can be tested by
e2e (e.g. TestCtlV3AuthWriteKey).

Fixes https://github.com/coreos/etcd/issues/6530
---
 auth/store.go           | 26 ++++++++++++++++-----
 auth/store_test.go      | 11 ++++-----
 etcdserver/v3_server.go | 50 ++++++++++++++++++++++++++++-------------
 3 files changed, 59 insertions(+), 28 deletions(-)

diff --git a/auth/store.go b/auth/store.go
index 1db89c03f..325af5b84 100644
--- a/auth/store.go
+++ b/auth/store.go
@@ -146,6 +146,9 @@ type AuthStore interface {
 
 	// Revision gets current revision of authStore
 	Revision() uint64
+
+	// CheckPassword checks a given pair of username and password is correct
+	CheckPassword(username, password string) (uint64, error)
 }
 
 type authStore struct {
@@ -232,11 +235,6 @@ func (as *authStore) Authenticate(ctx context.Context, username, password string
 		return nil, ErrAuthFailed
 	}
 
-	if bcrypt.CompareHashAndPassword(user.Password, []byte(password)) != nil {
-		plog.Noticef("authentication failed, invalid password for user %s", username)
-		return &pb.AuthenticateResponse{}, ErrAuthFailed
-	}
-
 	token := fmt.Sprintf("%s.%d", simpleToken, index)
 	as.assignSimpleTokenToUser(username, token)
 
@@ -244,6 +242,24 @@ func (as *authStore) Authenticate(ctx context.Context, username, password string
 	return &pb.AuthenticateResponse{Token: token}, nil
 }
 
+func (as *authStore) CheckPassword(username, password string) (uint64, error) {
+	tx := as.be.BatchTx()
+	tx.Lock()
+	defer tx.Unlock()
+
+	user := getUser(tx, username)
+	if user == nil {
+		return 0, ErrAuthFailed
+	}
+
+	if bcrypt.CompareHashAndPassword(user.Password, []byte(password)) != nil {
+		plog.Noticef("authentication failed, invalid password for user %s", username)
+		return 0, ErrAuthFailed
+	}
+
+	return getRevision(tx), nil
+}
+
 func (as *authStore) Recover(be backend.Backend) {
 	enabled := false
 	as.be = be
diff --git a/auth/store_test.go b/auth/store_test.go
index 68174ea39..89feeac95 100644
--- a/auth/store_test.go
+++ b/auth/store_test.go
@@ -67,7 +67,7 @@ func enableAuthAndCreateRoot(as *authStore) error {
 	return as.AuthEnable()
 }
 
-func TestAuthenticate(t *testing.T) {
+func TestCheckPassword(t *testing.T) {
 	b, tPath := backend.NewDefaultTmpBackend()
 	defer func() {
 		b.Close()
@@ -87,8 +87,7 @@ func TestAuthenticate(t *testing.T) {
 	}
 
 	// auth a non-existing user
-	ctx1 := context.WithValue(context.WithValue(context.TODO(), "index", uint64(1)), "simpleToken", "dummy")
-	_, err = as.Authenticate(ctx1, "foo-test", "bar")
+	_, err = as.CheckPassword("foo-test", "bar")
 	if err == nil {
 		t.Fatalf("expected %v, got %v", ErrAuthFailed, err)
 	}
@@ -97,15 +96,13 @@ func TestAuthenticate(t *testing.T) {
 	}
 
 	// auth an existing user with correct password
-	ctx2 := context.WithValue(context.WithValue(context.TODO(), "index", uint64(2)), "simpleToken", "dummy")
-	_, err = as.Authenticate(ctx2, "foo", "bar")
+	_, err = as.CheckPassword("foo", "bar")
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	// auth an existing user but with wrong password
-	ctx3 := context.WithValue(context.WithValue(context.TODO(), "index", uint64(3)), "simpleToken", "dummy")
-	_, err = as.Authenticate(ctx3, "foo", "")
+	_, err = as.CheckPassword("foo", "")
 	if err == nil {
 		t.Fatalf("expected %v, got %v", ErrAuthFailed, err)
 	}
diff --git a/etcdserver/v3_server.go b/etcdserver/v3_server.go
index 6abe7dfaa..0e7f74938 100644
--- a/etcdserver/v3_server.go
+++ b/etcdserver/v3_server.go
@@ -419,24 +419,42 @@ func (s *EtcdServer) AuthDisable(ctx context.Context, r *pb.AuthDisableRequest)
 }
 
 func (s *EtcdServer) Authenticate(ctx context.Context, r *pb.AuthenticateRequest) (*pb.AuthenticateResponse, error) {
-	st, err := s.AuthStore().GenSimpleToken()
-	if err != nil {
-		return nil, err
+	var result *applyResult
+
+	for {
+		checkedRevision, err := s.AuthStore().CheckPassword(r.Name, r.Password)
+		if err != nil {
+			plog.Errorf("invalid authentication request to user %s was issued", r.Name)
+			return nil, err
+		}
+
+		st, err := s.AuthStore().GenSimpleToken()
+		if err != nil {
+			return nil, err
+		}
+
+		internalReq := &pb.InternalAuthenticateRequest{
+			Name:        r.Name,
+			Password:    r.Password,
+			SimpleToken: st,
+		}
+
+		result, err = s.processInternalRaftRequestOnce(ctx, pb.InternalRaftRequest{Authenticate: internalReq})
+		if err != nil {
+			return nil, err
+		}
+		if result.err != nil {
+			return nil, result.err
+		}
+
+		if checkedRevision != s.AuthStore().Revision() {
+			plog.Infof("revision when password checked is obsolete, retrying")
+			continue
+		}
+
+		break
 	}
 
-	internalReq := &pb.InternalAuthenticateRequest{
-		Name:        r.Name,
-		Password:    r.Password,
-		SimpleToken: st,
-	}
-
-	result, err := s.processInternalRaftRequestOnce(ctx, pb.InternalRaftRequest{Authenticate: internalReq})
-	if err != nil {
-		return nil, err
-	}
-	if result.err != nil {
-		return nil, result.err
-	}
 	return result.resp.(*pb.AuthenticateResponse), nil
 }