Merge pull request #3762 from jonboulle/auth

etcdserver: restructure auth.Store and auth.User

etcdserver/auth/auth.go

@@ -88,6 +88,12 @@ type Store interface {
 	AuthEnabled() bool
 	EnableAuth() error
 	DisableAuth() error
+	PasswordStore
+}
+
+type PasswordStore interface {
+	CheckPassword(user User, password string) bool
+	HashPassword(password string) (string, error)
 }
 
 type store struct {
@@ -97,6 +103,8 @@ type store struct {
 
 	mu      sync.Mutex // protect enabled
 	enabled *bool
+
+	PasswordStore
 }
 
 type User struct {
@@ -141,12 +149,26 @@ func authErr(hs int, s string, v ...interface{}) Error {
 
 func NewStore(server doer, timeout time.Duration) Store {
 	s := &store{
-		server:  server,
-		timeout: timeout,
+		server:        server,
+		timeout:       timeout,
+		PasswordStore: passwordStore{},
 	}
 	return s
 }
 
+// passwordStore implements PasswordStore using bcrypt to hash user passwords
+type passwordStore struct{}
+
+func (_ passwordStore) CheckPassword(user User, password string) bool {
+	err := bcrypt.CompareHashAndPassword([]byte(user.Password), []byte(password))
+	return err == nil
+}
+
+func (_ passwordStore) HashPassword(password string) (string, error) {
+	hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost)
+	return string(hash), err
+}
+
 func (s *store) AllUsers() ([]string, error) {
 	resp, err := s.requestResource("/users/", false)
 	if err != nil {
@@ -217,11 +239,11 @@ func (s *store) createUserInternal(user User) (User, error) {
 	if user.Password == "" {
 		return user, authErr(http.StatusBadRequest, "Cannot create user %s with an empty password", user.User)
 	}
-	hash, err := bcrypt.GenerateFromPassword([]byte(user.Password), bcrypt.DefaultCost)
+	hash, err := s.HashPassword(user.Password)
 	if err != nil {
 		return user, err
 	}
-	user.Password = string(hash)
+	user.Password = hash
 
 	_, err = s.createResource("/users/"+user.User, user)
 	if err != nil {
@@ -261,6 +283,13 @@ func (s *store) UpdateUser(user User) (User, error) {
 		}
 		return old, err
 	}
+
+	hash, err := s.HashPassword(user.Password)
+	if err != nil {
+		return old, err
+	}
+	user.Password = hash
+
 	newUser, err := old.merge(user)
 	if err != nil {
 		return old, err
@@ -448,11 +477,7 @@ func (u User) merge(n User) (User, error) {
 	}
 	out.User = u.User
 	if n.Password != "" {
-		hash, err := bcrypt.GenerateFromPassword([]byte(n.Password), bcrypt.DefaultCost)
-		if err != nil {
-			return User{}, err
-		}
-		out.Password = string(hash)
+		out.Password = n.Password
 	} else {
 		out.Password = u.Password
 	}
@@ -476,11 +501,6 @@ func (u User) merge(n User) (User, error) {
 	return out, nil
 }
 
-func (u User) CheckPassword(password string) bool {
-	err := bcrypt.CompareHashAndPassword([]byte(u.Password), []byte(password))
-	return err == nil
-}
-
 // merge for a role works the same as User above -- atomic Role application to
 // each of the substructures.
 func (r Role) merge(n Role) (Role, error) {

etcdserver/auth/auth_test.go

@@ -19,7 +19,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/coreos/etcd/Godeps/_workspace/src/golang.org/x/crypto/bcrypt"
 	"github.com/coreos/etcd/Godeps/_workspace/src/golang.org/x/net/context"
 	etcderr "github.com/coreos/etcd/error"
 	"github.com/coreos/etcd/etcdserver"
@@ -74,7 +73,7 @@ func TestMergeUser(t *testing.T) {
 		},
 		{
 			User{User: "foo"},
-			User{User: "foo", Password: "bar"},
+			User{User: "foo", Password: "$2a$10$aUPOdbOGNawaVSusg3g2wuC3AH6XxIr9/Ms4VgDvzrAVOJPYzZILa"},
 			User{User: "foo", Roles: []string{}, Password: "$2a$10$aUPOdbOGNawaVSusg3g2wuC3AH6XxIr9/Ms4VgDvzrAVOJPYzZILa"},
 			false,
 		},
@@ -86,10 +85,6 @@ func TestMergeUser(t *testing.T) {
 			t.Fatalf("Got unexpected error on item %d", i)
 		}
 		if !tt.iserr {
-			err := bcrypt.CompareHashAndPassword([]byte(out.Password), []byte(tt.merge.Password))
-			if err == nil {
-				tt.expect.Password = out.Password
-			}
 			if !reflect.DeepEqual(out, tt.expect) {
 				t.Errorf("Unequal merge expectation on item %d: got: %#v, expect: %#v", i, out, tt.expect)
 			}
@@ -357,6 +352,15 @@ func TestEnsure(t *testing.T) {
 	}
 }
 
+type fastPasswordStore struct {
+}
+
+func (_ fastPasswordStore) CheckPassword(user User, password string) bool {
+	return user.Password == password
+}
+
+func (_ fastPasswordStore) HashPassword(password string) (string, error) { return password, nil }
+
 func TestCreateAndUpdateUser(t *testing.T) {
 	olduser := `{"user": "cat", "roles" : ["animal"]}`
 	newuser := `{"user": "cat", "roles" : ["animal", "pet"]}`
@@ -410,7 +414,7 @@ func TestCreateAndUpdateUser(t *testing.T) {
 	update := User{User: "cat", Grant: []string{"pet"}}
 	expected := User{User: "cat", Roles: []string{"animal", "pet"}}
 
-	s := store{server: d, timeout: testTimeout, ensuredOnce: true}
+	s := store{server: d, timeout: testTimeout, ensuredOnce: true, PasswordStore: fastPasswordStore{}}
 	out, created, err := s.CreateOrUpdateUser(user)
 	if created == false {
 		t.Error("Should have created user, instead updated?")

etcdserver/etcdhttp/client_auth.go

@@ -53,7 +53,8 @@ func hasRootAccess(sec auth.Store, r *http.Request) bool {
 	if err != nil {
 		return false
 	}
-	ok = rootUser.CheckPassword(password)
+
+	ok = sec.CheckPassword(rootUser, password)
 	if !ok {
 		plog.Warningf("auth: wrong password for user %s", username)
 		return false
@@ -89,7 +90,7 @@ func hasKeyPrefixAccess(sec auth.Store, r *http.Request, key string, recursive b
 		plog.Warningf("auth: no such user: %s.", username)
 		return false
 	}
-	authAsUser := user.CheckPassword(password)
+	authAsUser := sec.CheckPassword(user, password)
 	if !authAsUser {
 		plog.Warningf("auth: incorrect password for user: %s.", username)
 		return false

etcdserver/etcdhttp/client_auth_test.go

@@ -25,7 +25,7 @@ import (
 	"github.com/coreos/etcd/etcdserver/auth"
 )
 
-const goodPassword = "$2a$10$VYdJecHfm6WNodzv8XhmYeIG4n2SsQefdo5V2t6xIq/aWDHNqSUQW"
+const goodPassword = "good"
 
 func mustJSONRequest(t *testing.T, method string, p string, body string) *http.Request {
 	req, err := http.NewRequest(method, path.Join(authPrefix, p), strings.NewReader(body))
@@ -77,6 +77,14 @@ func (s *mockAuthStore) AuthEnabled() bool  { return s.enabled }
 func (s *mockAuthStore) EnableAuth() error  { return s.err }
 func (s *mockAuthStore) DisableAuth() error { return s.err }
 
+func (s *mockAuthStore) CheckPassword(user auth.User, password string) bool {
+	return user.Password == password
+}
+
+func (s *mockAuthStore) HashPassword(password string) (string, error) {
+	return password, nil
+}
+
 func TestAuthFlow(t *testing.T) {
 	enableMapMu.Lock()
 	enabledMap = make(map[capability]bool)