Skip to content

Commit

Permalink
auth, etcdserver: hash password in the API layer
Browse files Browse the repository at this point in the history
  • Loading branch information
@mitake
mitake committed Jun 10, 2020
1 parent e007d4f commit b085cf6
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 37 deletions.
56 changes: 26 additions & 30 deletions auth/store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package auth
import (
"bytes"
"context"
"encoding/base64"
"encoding/binary"
"errors"
"sort"
Expand Down Expand Up @@ -185,6 +186,9 @@ type AuthStore interface {

// HasRole checks that user has role
HasRole(user, role string) bool

// BcryptCost gets strength of hashing bcrypted auth password
BcryptCost() int
}

type TokenProvider interface {
Expand Down Expand Up @@ -376,22 +380,6 @@ func (as *authStore) UserAdd(r *pb.AuthUserAddRequest) (*pb.AuthUserAddResponse,
return nil, ErrUserEmpty
}

var hashed []byte
var err error

noPassword := r.Options != nil && r.Options.NoPassword
if !noPassword {
hashed, err = bcrypt.GenerateFromPassword([]byte(r.Password), as.bcryptCost)
if err != nil {
as.lg.Error(
"failed to bcrypt hash password",
zap.String("user-name", r.Name),
zap.Error(err),
)
return nil, err
}
}

tx := as.be.BatchTx()
tx.Lock()
defer tx.Unlock()
Expand All @@ -408,9 +396,19 @@ func (as *authStore) UserAdd(r *pb.AuthUserAddRequest) (*pb.AuthUserAddResponse,
}
}

var decodedPassword []byte
var err error

if !options.NoPassword {
decodedPassword, err = base64.StdEncoding.DecodeString(r.Password)
if err != nil {
return nil, ErrNoPasswordUser
}
}

newUser := &authpb.User{
Name: []byte(r.Name),
Password: hashed,
Password: decodedPassword,
Options: options,
}

Expand Down Expand Up @@ -455,18 +453,6 @@ func (as *authStore) UserDelete(r *pb.AuthUserDeleteRequest) (*pb.AuthUserDelete
}

func (as *authStore) UserChangePassword(r *pb.AuthUserChangePasswordRequest) (*pb.AuthUserChangePasswordResponse, error) {
// TODO(mitake): measure the cost of bcrypt.GenerateFromPassword()
// If the cost is too high, we should move the encryption to outside of the raft
hashed, err := bcrypt.GenerateFromPassword([]byte(r.Password), as.bcryptCost)
if err != nil {
as.lg.Error(
"failed to bcrypt hash password",
zap.String("user-name", r.Name),
zap.Error(err),
)
return nil, err
}

tx := as.be.BatchTx()
tx.Lock()
defer tx.Unlock()
Expand All @@ -476,10 +462,20 @@ func (as *authStore) UserChangePassword(r *pb.AuthUserChangePasswordRequest) (*p
return nil, ErrUserNotFound
}

var decodedPassword []byte
var err error

if !user.Options.NoPassword {
decodedPassword, err = base64.StdEncoding.DecodeString(r.Password)
if err != nil {
return nil, ErrNoPasswordUser
}
}

updatedUser := &authpb.User{
Name: []byte(r.Name),
Roles: user.Roles,
Password: hashed,
Password: decodedPassword,
Options: user.Options,
}

Expand Down
20 changes: 13 additions & 7 deletions auth/store_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package auth

import (
"context"
"encoding/base64"
"fmt"
"os"
"reflect"
Expand Down Expand Up @@ -95,6 +96,11 @@ func TestNewAuthStoreBcryptCost(t *testing.T) {
b.Close()
}

func encodePassword(s string) string {
hashedPassword, _ := bcrypt.GenerateFromPassword([]byte(s), bcrypt.MinCost)
return base64.StdEncoding.EncodeToString([]byte(hashedPassword))
}

func setupAuthStore(t *testing.T) (store *authStore, teardownfunc func(t *testing.T)) {
b, tPath := backend.NewDefaultTmpBackend()

Expand All @@ -114,7 +120,7 @@ func setupAuthStore(t *testing.T) (store *authStore, teardownfunc func(t *testin
t.Fatal(err)
}

ua := &pb.AuthUserAddRequest{Name: "foo", Password: "bar", Options: &authpb.UserAddOptions{NoPassword: false}}
ua := &pb.AuthUserAddRequest{Name: "foo", Password: encodePassword("bar"), Options: &authpb.UserAddOptions{NoPassword: false}}
_, err = as.UserAdd(ua) // add a non-existing user
if err != nil {
t.Fatal(err)
Expand All @@ -129,7 +135,7 @@ func setupAuthStore(t *testing.T) (store *authStore, teardownfunc func(t *testin
}

func enableAuthAndCreateRoot(as *authStore) error {
_, err := as.UserAdd(&pb.AuthUserAddRequest{Name: "root", Password: "root", Options: &authpb.UserAddOptions{NoPassword: false}})
_, err := as.UserAdd(&pb.AuthUserAddRequest{Name: "root", Password: encodePassword("root"), Options: &authpb.UserAddOptions{NoPassword: false}})
if err != nil {
return err
}
Expand Down Expand Up @@ -239,7 +245,7 @@ func TestUserChangePassword(t *testing.T) {
t.Fatal(err)
}

_, err = as.UserChangePassword(&pb.AuthUserChangePasswordRequest{Name: "foo", Password: "baz"})
_, err = as.UserChangePassword(&pb.AuthUserChangePasswordRequest{Name: "foo", Password: encodePassword("baz")})
if err != nil {
t.Fatal(err)
}
Expand All @@ -251,7 +257,7 @@ func TestUserChangePassword(t *testing.T) {
}

// change a non-existing user
_, err = as.UserChangePassword(&pb.AuthUserChangePasswordRequest{Name: "foo-test", Password: "bar"})
_, err = as.UserChangePassword(&pb.AuthUserChangePasswordRequest{Name: "foo-test", Password: encodePassword("bar")})
if err == nil {
t.Fatalf("expected %v, got %v", ErrUserNotFound, err)
}
Expand Down Expand Up @@ -396,7 +402,7 @@ func TestListUsers(t *testing.T) {
as, tearDown := setupAuthStore(t)
defer tearDown(t)

ua := &pb.AuthUserAddRequest{Name: "user1", Password: "pwd1", Options: &authpb.UserAddOptions{NoPassword: false}}
ua := &pb.AuthUserAddRequest{Name: "user1", Password: encodePassword("pwd1"), Options: &authpb.UserAddOptions{NoPassword: false}}
_, err := as.UserAdd(ua) // add a non-existing user
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -764,7 +770,7 @@ func TestHammerSimpleAuthenticate(t *testing.T) {
// create lots of users
for i := 0; i < 50; i++ {
u := fmt.Sprintf("user-%d", i)
ua := &pb.AuthUserAddRequest{Name: u, Password: "123", Options: &authpb.UserAddOptions{NoPassword: false}}
ua := &pb.AuthUserAddRequest{Name: u, Password: encodePassword("123"), Options: &authpb.UserAddOptions{NoPassword: false}}
if _, err := as.UserAdd(ua); err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -809,7 +815,7 @@ func TestRolesOrder(t *testing.T) {
}

username := "user"
_, err = as.UserAdd(&pb.AuthUserAddRequest{Name: username, Password: "pass", Options: &authpb.UserAddOptions{NoPassword: false}})
_, err = as.UserAdd(&pb.AuthUserAddRequest{Name: username, Password: encodePassword("pass"), Options: &authpb.UserAddOptions{NoPassword: false}})
if err != nil {
t.Fatal(err)
}
Expand Down
14 changes: 14 additions & 0 deletions etcdserver/v3_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package etcdserver
import (
"bytes"
"context"
"encoding/base64"
"encoding/binary"
"time"

Expand All @@ -32,6 +33,7 @@ import (

"github.com/gogo/protobuf/proto"
"go.uber.org/zap"
"golang.org/x/crypto/bcrypt"
)

const (
Expand Down Expand Up @@ -464,6 +466,12 @@ func (s *EtcdServer) Authenticate(ctx context.Context, r *pb.AuthenticateRequest
}

func (s *EtcdServer) UserAdd(ctx context.Context, r *pb.AuthUserAddRequest) (*pb.AuthUserAddResponse, error) {
hashedPassword, err := bcrypt.GenerateFromPassword([]byte(r.Password), s.authStore.BcryptCost())
if err != nil {
return nil, err
}
r.Password = base64.StdEncoding.EncodeToString(hashedPassword)

resp, err := s.raftRequest(ctx, pb.InternalRaftRequest{AuthUserAdd: r})
if err != nil {
return nil, err
Expand All @@ -480,6 +488,12 @@ func (s *EtcdServer) UserDelete(ctx context.Context, r *pb.AuthUserDeleteRequest
}

func (s *EtcdServer) UserChangePassword(ctx context.Context, r *pb.AuthUserChangePasswordRequest) (*pb.AuthUserChangePasswordResponse, error) {
hashedPassword, err := bcrypt.GenerateFromPassword([]byte(r.Password), s.authStore.BcryptCost())
if err != nil {
return nil, err
}
r.Password = string(hashedPassword[:]) // TODO(mitake): check this conversion can work well

resp, err := s.raftRequest(ctx, pb.InternalRaftRequest{AuthUserChangePassword: r})
if err != nil {
return nil, err
Expand Down

0 comments on commit b085cf6

Please sign in to comment.