From ee522025b300871dee1a5d5ac6007614afae1cd9 Mon Sep 17 00:00:00 2001 From: Jonathan Boulle Date: Tue, 27 Oct 2015 15:22:17 -0700 Subject: [PATCH] etcdserver: restructure auth.Store and auth.User This attempts to decouple password-related functions, which previously existed both in the Store and User structs, by splitting them out into a separate interface, PasswordStore. This means that they can be more easily swapped out during testing. This also changes the relevant tests to use mock password functions instead of the bcrypt-backed implementations; as a result, the tests are much faster. Before: ``` github.com/coreos/etcd/etcdserver/auth 31.495s github.com/coreos/etcd/etcdserver/etcdhttp 91.205s ``` After: ``` github.com/coreos/etcd/etcdserver/auth 1.207s github.com/coreos/etcd/etcdserver/etcdhttp 1.207s ``` --- etcdserver/auth/auth.go | 48 +++++++++++++++++-------- etcdserver/auth/auth_test.go | 18 ++++++---- etcdserver/etcdhttp/client_auth.go | 5 +-- etcdserver/etcdhttp/client_auth_test.go | 10 +++++- 4 files changed, 57 insertions(+), 24 deletions(-) diff --git a/etcdserver/auth/auth.go b/etcdserver/auth/auth.go index 93f8909dd..3f42dfa2e 100644 --- a/etcdserver/auth/auth.go +++ b/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) { diff --git a/etcdserver/auth/auth_test.go b/etcdserver/auth/auth_test.go index 1f4615af4..80831e16b 100644 --- a/etcdserver/auth/auth_test.go +++ b/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?") diff --git a/etcdserver/etcdhttp/client_auth.go b/etcdserver/etcdhttp/client_auth.go index 0637afa13..c5678ad00 100644 --- a/etcdserver/etcdhttp/client_auth.go +++ b/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 diff --git a/etcdserver/etcdhttp/client_auth_test.go b/etcdserver/etcdhttp/client_auth_test.go index 97bea77e6..4bf190e08 100644 --- a/etcdserver/etcdhttp/client_auth_test.go +++ b/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)