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 ```release-2.3
parent
2840260b3b
commit
ee522025b3
|
@ -88,6 +88,12 @@ type Store interface {
|
||||||
AuthEnabled() bool
|
AuthEnabled() bool
|
||||||
EnableAuth() error
|
EnableAuth() error
|
||||||
DisableAuth() error
|
DisableAuth() error
|
||||||
|
PasswordStore
|
||||||
|
}
|
||||||
|
|
||||||
|
type PasswordStore interface {
|
||||||
|
CheckPassword(user User, password string) bool
|
||||||
|
HashPassword(password string) (string, error)
|
||||||
}
|
}
|
||||||
|
|
||||||
type store struct {
|
type store struct {
|
||||||
|
@ -97,6 +103,8 @@ type store struct {
|
||||||
|
|
||||||
mu sync.Mutex // protect enabled
|
mu sync.Mutex // protect enabled
|
||||||
enabled *bool
|
enabled *bool
|
||||||
|
|
||||||
|
PasswordStore
|
||||||
}
|
}
|
||||||
|
|
||||||
type User struct {
|
type User struct {
|
||||||
|
@ -141,12 +149,26 @@ func authErr(hs int, s string, v ...interface{}) Error {
|
||||||
|
|
||||||
func NewStore(server doer, timeout time.Duration) Store {
|
func NewStore(server doer, timeout time.Duration) Store {
|
||||||
s := &store{
|
s := &store{
|
||||||
server: server,
|
server: server,
|
||||||
timeout: timeout,
|
timeout: timeout,
|
||||||
|
PasswordStore: passwordStore{},
|
||||||
}
|
}
|
||||||
return s
|
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) {
|
func (s *store) AllUsers() ([]string, error) {
|
||||||
resp, err := s.requestResource("/users/", false)
|
resp, err := s.requestResource("/users/", false)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
@ -217,11 +239,11 @@ func (s *store) createUserInternal(user User) (User, error) {
|
||||||
if user.Password == "" {
|
if user.Password == "" {
|
||||||
return user, authErr(http.StatusBadRequest, "Cannot create user %s with an empty password", user.User)
|
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 {
|
if err != nil {
|
||||||
return user, err
|
return user, err
|
||||||
}
|
}
|
||||||
user.Password = string(hash)
|
user.Password = hash
|
||||||
|
|
||||||
_, err = s.createResource("/users/"+user.User, user)
|
_, err = s.createResource("/users/"+user.User, user)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
@ -261,6 +283,13 @@ func (s *store) UpdateUser(user User) (User, error) {
|
||||||
}
|
}
|
||||||
return old, err
|
return old, err
|
||||||
}
|
}
|
||||||
|
|
||||||
|
hash, err := s.HashPassword(user.Password)
|
||||||
|
if err != nil {
|
||||||
|
return old, err
|
||||||
|
}
|
||||||
|
user.Password = hash
|
||||||
|
|
||||||
newUser, err := old.merge(user)
|
newUser, err := old.merge(user)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return old, err
|
return old, err
|
||||||
|
@ -448,11 +477,7 @@ func (u User) merge(n User) (User, error) {
|
||||||
}
|
}
|
||||||
out.User = u.User
|
out.User = u.User
|
||||||
if n.Password != "" {
|
if n.Password != "" {
|
||||||
hash, err := bcrypt.GenerateFromPassword([]byte(n.Password), bcrypt.DefaultCost)
|
out.Password = n.Password
|
||||||
if err != nil {
|
|
||||||
return User{}, err
|
|
||||||
}
|
|
||||||
out.Password = string(hash)
|
|
||||||
} else {
|
} else {
|
||||||
out.Password = u.Password
|
out.Password = u.Password
|
||||||
}
|
}
|
||||||
|
@ -476,11 +501,6 @@ func (u User) merge(n User) (User, error) {
|
||||||
return out, nil
|
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
|
// merge for a role works the same as User above -- atomic Role application to
|
||||||
// each of the substructures.
|
// each of the substructures.
|
||||||
func (r Role) merge(n Role) (Role, error) {
|
func (r Role) merge(n Role) (Role, error) {
|
||||||
|
|
|
@ -19,7 +19,6 @@ import (
|
||||||
"testing"
|
"testing"
|
||||||
"time"
|
"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"
|
"github.com/coreos/etcd/Godeps/_workspace/src/golang.org/x/net/context"
|
||||||
etcderr "github.com/coreos/etcd/error"
|
etcderr "github.com/coreos/etcd/error"
|
||||||
"github.com/coreos/etcd/etcdserver"
|
"github.com/coreos/etcd/etcdserver"
|
||||||
|
@ -74,7 +73,7 @@ func TestMergeUser(t *testing.T) {
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
User{User: "foo"},
|
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"},
|
User{User: "foo", Roles: []string{}, Password: "$2a$10$aUPOdbOGNawaVSusg3g2wuC3AH6XxIr9/Ms4VgDvzrAVOJPYzZILa"},
|
||||||
false,
|
false,
|
||||||
},
|
},
|
||||||
|
@ -86,10 +85,6 @@ func TestMergeUser(t *testing.T) {
|
||||||
t.Fatalf("Got unexpected error on item %d", i)
|
t.Fatalf("Got unexpected error on item %d", i)
|
||||||
}
|
}
|
||||||
if !tt.iserr {
|
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) {
|
if !reflect.DeepEqual(out, tt.expect) {
|
||||||
t.Errorf("Unequal merge expectation on item %d: got: %#v, expect: %#v", i, 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) {
|
func TestCreateAndUpdateUser(t *testing.T) {
|
||||||
olduser := `{"user": "cat", "roles" : ["animal"]}`
|
olduser := `{"user": "cat", "roles" : ["animal"]}`
|
||||||
newuser := `{"user": "cat", "roles" : ["animal", "pet"]}`
|
newuser := `{"user": "cat", "roles" : ["animal", "pet"]}`
|
||||||
|
@ -410,7 +414,7 @@ func TestCreateAndUpdateUser(t *testing.T) {
|
||||||
update := User{User: "cat", Grant: []string{"pet"}}
|
update := User{User: "cat", Grant: []string{"pet"}}
|
||||||
expected := User{User: "cat", Roles: []string{"animal", "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)
|
out, created, err := s.CreateOrUpdateUser(user)
|
||||||
if created == false {
|
if created == false {
|
||||||
t.Error("Should have created user, instead updated?")
|
t.Error("Should have created user, instead updated?")
|
||||||
|
|
|
@ -53,7 +53,8 @@ func hasRootAccess(sec auth.Store, r *http.Request) bool {
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
ok = rootUser.CheckPassword(password)
|
|
||||||
|
ok = sec.CheckPassword(rootUser, password)
|
||||||
if !ok {
|
if !ok {
|
||||||
plog.Warningf("auth: wrong password for user %s", username)
|
plog.Warningf("auth: wrong password for user %s", username)
|
||||||
return false
|
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)
|
plog.Warningf("auth: no such user: %s.", username)
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
authAsUser := user.CheckPassword(password)
|
authAsUser := sec.CheckPassword(user, password)
|
||||||
if !authAsUser {
|
if !authAsUser {
|
||||||
plog.Warningf("auth: incorrect password for user: %s.", username)
|
plog.Warningf("auth: incorrect password for user: %s.", username)
|
||||||
return false
|
return false
|
||||||
|
|
|
@ -25,7 +25,7 @@ import (
|
||||||
"github.com/coreos/etcd/etcdserver/auth"
|
"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 {
|
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))
|
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) EnableAuth() error { return s.err }
|
||||||
func (s *mockAuthStore) DisableAuth() 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) {
|
func TestAuthFlow(t *testing.T) {
|
||||||
enableMapMu.Lock()
|
enableMapMu.Lock()
|
||||||
enabledMap = make(map[capability]bool)
|
enabledMap = make(map[capability]bool)
|
||||||
|
|
Loading…
Reference in New Issue