From debc3cc11c3c058c710c73e74f45626977503d9b Mon Sep 17 00:00:00 2001 From: David Norton Date: Fri, 26 Jun 2015 15:30:00 -0400 Subject: [PATCH 1/5] fix #3102: add authentication cache --- influxql/ast.go | 6 +++--- influxql/parser.go | 17 ++++++----------- meta/store.go | 14 ++++++++++++++ 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/influxql/ast.go b/influxql/ast.go index 259764d746..46905d1aa4 100644 --- a/influxql/ast.go +++ b/influxql/ast.go @@ -92,6 +92,9 @@ func (*DropRetentionPolicyStatement) node() {} func (*DropSeriesStatement) node() {} func (*DropUserStatement) node() {} func (*GrantStatement) node() {} +func (*RevokeStatement) node() {} +func (*SelectStatement) node() {} +func (*SetPasswordUserStatement) node() {} func (*ShowContinuousQueriesStatement) node() {} func (*ShowGrantsForUserStatement) node() {} func (*ShowServersStatement) node() {} @@ -105,9 +108,6 @@ func (*ShowDiagnosticsStatement) node() {} func (*ShowTagKeysStatement) node() {} func (*ShowTagValuesStatement) node() {} func (*ShowUsersStatement) node() {} -func (*RevokeStatement) node() {} -func (*SelectStatement) node() {} -func (*SetPasswordUserStatement) node() {} func (*BinaryExpr) node() {} func (*BooleanLiteral) node() {} diff --git a/influxql/parser.go b/influxql/parser.go index 5210b0e951..e60bcef303 100644 --- a/influxql/parser.go +++ b/influxql/parser.go @@ -94,7 +94,7 @@ func (p *Parser) ParseStatement() (Statement, error) { case ALTER: return p.parseAlterStatement() case SET: - return p.parseSetStatement() + return p.parseSetPasswordUserStatement() default: return nil, newParseError(tokstr(tok, lit), []string{"SELECT", "DELETE", "SHOW", "CREATE", "DROP", "GRANT", "REVOKE", "ALTER", "SET"}, pos) } @@ -207,19 +207,14 @@ func (p *Parser) parseAlterStatement() (Statement, error) { return nil, newParseError(tokstr(tok, lit), []string{"RETENTION"}, pos) } -// parseSetStatement parses a string and returns a set statement. +// parseSetPasswordUserStatement parses a string and returns a set statement. // This function assumes the SET token has already been consumed. -func (p *Parser) parseSetStatement() (*SetPasswordUserStatement, error) { +func (p *Parser) parseSetPasswordUserStatement() (*SetPasswordUserStatement, error) { stmt := &SetPasswordUserStatement{} - // Consume the required PASSWORD token. - if tok, pos, lit := p.scanIgnoreWhitespace(); tok != PASSWORD { - return nil, newParseError(tokstr(tok, lit), []string{"PASSWORD"}, pos) - } - - // Consume the required FOR token. - if tok, pos, lit := p.scanIgnoreWhitespace(); tok != FOR { - return nil, newParseError(tokstr(tok, lit), []string{"FOR"}, pos) + // Consume the required PASSWORD FOR tokens. + if err := p.parseTokens([]Token{PASSWORD, FOR}); err != nil { + return nil, err } // Parse username diff --git a/meta/store.go b/meta/store.go index 31e862b9dd..2d0a7ff066 100644 --- a/meta/store.go +++ b/meta/store.go @@ -96,6 +96,9 @@ type Store struct { // The amount of time without an apply before sending a heartbeat. CommitTimeout time.Duration + // Authentication cache. + authCache map[string]string + Logger *log.Logger } @@ -116,6 +119,7 @@ func NewStore(c Config) *Store { ElectionTimeout: time.Duration(c.ElectionTimeout), LeaderLeaseTimeout: time.Duration(c.LeaderLeaseTimeout), CommitTimeout: time.Duration(c.CommitTimeout), + authCache: make(map[string]string, 0), Logger: log.New(os.Stderr, "", log.LstdFlags), } } @@ -994,11 +998,19 @@ func (s *Store) Authenticate(username, password string) (ui *UserInfo, err error return ErrUserNotFound } + // Check the local auth cache first. + if p, ok := s.authCache[username]; ok && p == password { + ui = u + return nil + } + // Compare password with user hash. if err := bcrypt.CompareHashAndPassword([]byte(u.Hash), []byte(password)); err != nil { return err } + s.authCache[username] = password + ui = u return nil }) @@ -1578,6 +1590,7 @@ func (fsm *storeFSM) applyDropUserCommand(cmd *internal.Command) interface{} { return err } fsm.data = other + delete(fsm.authCache, v.GetName()) return nil } @@ -1591,6 +1604,7 @@ func (fsm *storeFSM) applyUpdateUserCommand(cmd *internal.Command) interface{} { return err } fsm.data = other + delete(fsm.authCache, v.GetName()) return nil } From 8af9e62a5de94618099e2cd2cc447dcfbbbb747d Mon Sep 17 00:00:00 2001 From: David Norton Date: Fri, 26 Jun 2015 15:35:01 -0400 Subject: [PATCH 2/5] fix #3102: update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 04f0f36ad1..659cfbe864 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,7 @@ - [#2608](https://github.com/influxdb/influxdb/issues/2608): drop measurement while writing points to that measurement has race condition that can panic - [#3183](https://github.com/influxdb/influxdb/issues/3183): using line protocol measurement names cannot contain commas - [#3193](https://github.com/influxdb/influxdb/pull/3193): Fix panic for SHOW STATS and in collectd +- [#3102](https://github.com/influxdb/influxdb/issues/3102): Add authentication cache ## v0.9.0 [2015-06-11] From 3463d906e9a8b4a158c3424c68d7b7350b6c1694 Mon Sep 17 00:00:00 2001 From: David Norton Date: Fri, 26 Jun 2015 16:44:04 -0400 Subject: [PATCH 3/5] fix #3102: add unit test for authentication --- meta/store.go | 1 - meta/store_test.go | 55 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/meta/store.go b/meta/store.go index 2d0a7ff066..df8cc10328 100644 --- a/meta/store.go +++ b/meta/store.go @@ -1003,7 +1003,6 @@ func (s *Store) Authenticate(username, password string) (ui *UserInfo, err error ui = u return nil } - // Compare password with user hash. if err := bcrypt.CompareHashAndPassword([]byte(u.Hash), []byte(password)); err != nil { return err diff --git a/meta/store_test.go b/meta/store_test.go index c249ca11e7..b26bef6dda 100644 --- a/meta/store_test.go +++ b/meta/store_test.go @@ -17,6 +17,7 @@ import ( "github.com/influxdb/influxdb/meta" "github.com/influxdb/influxdb/tcp" "github.com/influxdb/influxdb/toml" + "golang.org/x/crypto/bcrypt" ) func init() { @@ -656,6 +657,60 @@ func TestStore_UpdateUser(t *testing.T) { } } +// Ensure Authentication works. +func TestStore_Authentication(t *testing.T) { + t.Parallel() + + if testing.Short() { + t.SkipNow() + } + + s := MustOpenStore() + defer s.Close() + + // Set the hash function back to the real thing for this test. + oldHashFn := meta.HashPassword + meta.HashPassword = func(password string) ([]byte, error) { + return bcrypt.GenerateFromPassword([]byte(password), 4) + } + defer func() { meta.HashPassword = oldHashFn }() + + // Create user. + s.CreateUser("susy", "pass", true) + + // Authenticate user. + if ui, err := s.Authenticate("susy", "pass"); err != nil { + t.Fatal(err) + } else if ui.Name != "susy" { + t.Fatalf(`expected "susy", got "%s"`, ui.Name) + } + + // Update user's password. + s.UpdateUser("susy", "pass2") + + // Make sure authentication with old password does NOT work. + if _, err := s.Authenticate("susy", "pass"); err == nil { + t.Fatal("expected authentication error") + } + + // Authenticate user with new password + if ui, err := s.Authenticate("susy", "pass2"); err != nil { + t.Fatal(err) + } else if ui.Name != "susy" { + t.Fatalf(`expected "susy", got "%s"`, ui.Name) + } + + // Drop user. + s.DropUser("susy") + + // Make sure authentication with both old passwords does NOT work. + if _, err := s.Authenticate("susy", "pass"); err == nil { + t.Fatal("expected authentication error") + } else if _, err := s.Authenticate("susy", "pass2"); err == nil { + t.Fatal("expected authentication error") + } +} + // Ensure the store can return the count of users in it. func TestStore_UserCount(t *testing.T) { t.Parallel() From ba6bddc5f8c4657e90c11e2d2b1b9f1ebbc74cd1 Mon Sep 17 00:00:00 2001 From: David Norton Date: Mon, 29 Jun 2015 13:37:36 -0400 Subject: [PATCH 4/5] fix #3102: fix race in test --- meta/store.go | 61 +++++++++++++++++++++++++++++++--------------- meta/store_test.go | 21 +++++++--------- 2 files changed, 51 insertions(+), 31 deletions(-) diff --git a/meta/store.go b/meta/store.go index df8cc10328..24e87c2ec1 100644 --- a/meta/store.go +++ b/meta/store.go @@ -99,6 +99,10 @@ type Store struct { // Authentication cache. authCache map[string]string + // hashPassword generates a cryptographically secure hash for password. + // Returns an error if the password is invalid or a hash cannot be generated. + hashPassword HashPasswordFn + Logger *log.Logger } @@ -120,7 +124,10 @@ func NewStore(c Config) *Store { LeaderLeaseTimeout: time.Duration(c.LeaderLeaseTimeout), CommitTimeout: time.Duration(c.CommitTimeout), authCache: make(map[string]string, 0), - Logger: log.New(os.Stderr, "", log.LstdFlags), + hashPassword: func(password string) ([]byte, error) { + return bcrypt.GenerateFromPassword([]byte(password), BcryptCost) + }, + Logger: log.New(os.Stderr, "", log.LstdFlags), } } @@ -989,6 +996,9 @@ func (s *Store) AdminUserExists() (exists bool, err error) { return } +// ErrAuthenticate is returned when authentication fails. +var ErrAuthenticate = errors.New("authentication failed") + // Authenticate retrieves a user with a matching username and password. func (s *Store) Authenticate(username, password string) (ui *UserInfo, err error) { err = s.read(func(data *Data) error { @@ -999,13 +1009,17 @@ func (s *Store) Authenticate(username, password string) (ui *UserInfo, err error } // Check the local auth cache first. - if p, ok := s.authCache[username]; ok && p == password { - ui = u - return nil + if p, ok := s.authCache[username]; ok { + if p == password { + ui = u + return nil + } else { + return ErrAuthenticate + } } // Compare password with user hash. if err := bcrypt.CompareHashAndPassword([]byte(u.Hash), []byte(password)); err != nil { - return err + return ErrAuthenticate } s.authCache[username] = password @@ -1019,7 +1033,7 @@ func (s *Store) Authenticate(username, password string) (ui *UserInfo, err error // CreateUser creates a new user in the store. func (s *Store) CreateUser(name, password string, admin bool) (*UserInfo, error) { // Hash the password before serializing it. - hash, err := HashPassword(password) + hash, err := s.hashPassword(password) if err != nil { return nil, err } @@ -1049,7 +1063,7 @@ func (s *Store) DropUser(name string) error { // UpdateUser updates an existing user in the store. func (s *Store) UpdateUser(name, password string) error { // Hash the password before serializing it. - hash, err := HashPassword(password) + hash, err := s.hashPassword(password) if err != nil { return err } @@ -1312,6 +1326,27 @@ func (s *Store) sync(index uint64, timeout time.Duration) error { } } +// BcryptCost is the cost associated with generating password with Bcrypt. +// This setting is lowered during testing to improve test suite performance. +var BcryptCost = 10 + +// HashPasswordFn represnets a password hashing function. +type HashPasswordFn func(password string) ([]byte, error) + +// GetHashPasswordFn returns the current password hashing function. +func (s *Store) GetHashPasswordFn() HashPasswordFn { + s.mu.Lock() + defer s.mu.Unlock() + return s.hashPassword +} + +// SetHashPasswordFn sets the password hashing function. +func (s *Store) SetHashPasswordFn(fn HashPasswordFn) { + s.mu.Lock() + defer s.mu.Unlock() + s.hashPassword = fn +} + // storeFSM represents the finite state machine used by Store to interact with Raft. type storeFSM Store @@ -1750,18 +1785,6 @@ func (rpu *RetentionPolicyUpdate) SetName(v string) { rpu.Name = &v } func (rpu *RetentionPolicyUpdate) SetDuration(v time.Duration) { rpu.Duration = &v } func (rpu *RetentionPolicyUpdate) SetReplicaN(v int) { rpu.ReplicaN = &v } -// BcryptCost is the cost associated with generating password with Bcrypt. -// This setting is lowered during testing to improve test suite performance. -var BcryptCost = 10 - -// HashPassword generates a cryptographically secure hash for password. -// Returns an error if the password is invalid or a hash cannot be generated. -var HashPassword = func(password string) ([]byte, error) { - // The second arg is the cost of the hashing, higher is slower but makes - // it harder to brute force, since it will be really slow and impractical - return bcrypt.GenerateFromPassword([]byte(password), BcryptCost) -} - // assert will panic with a given formatted message if the given condition is false. func assert(condition bool, msg string, v ...interface{}) { if !condition { diff --git a/meta/store_test.go b/meta/store_test.go index b26bef6dda..f498767ec5 100644 --- a/meta/store_test.go +++ b/meta/store_test.go @@ -20,13 +20,6 @@ import ( "golang.org/x/crypto/bcrypt" ) -func init() { - // Disable password hashing to speed up testing. - meta.HashPassword = func(password string) ([]byte, error) { - return []byte(password), nil - } -} - // Ensure the store returns an error func TestStore_Open_ErrStoreOpen(t *testing.T) { t.Parallel() @@ -668,12 +661,10 @@ func TestStore_Authentication(t *testing.T) { s := MustOpenStore() defer s.Close() - // Set the hash function back to the real thing for this test. - oldHashFn := meta.HashPassword - meta.HashPassword = func(password string) ([]byte, error) { + // Set the password hash function to the real thing for this test. + s.SetHashPasswordFn(func(password string) ([]byte, error) { return bcrypt.GenerateFromPassword([]byte(password), 4) - } - defer func() { meta.HashPassword = oldHashFn }() + }) // Create user. s.CreateUser("susy", "pass", true) @@ -826,6 +817,7 @@ func NewStore(c meta.Config) *Store { Store: meta.NewStore(c), } s.Logger = log.New(&s.Stderr, "", log.LstdFlags) + s.SetHashPasswordFn(mockHashPassword) return s } @@ -990,3 +982,8 @@ func MustTempFile() string { os.Remove(f.Name()) return f.Name() } + +// mockHashPassword is used for most tests to avoid slow calls to bcrypt. +func mockHashPassword(password string) ([]byte, error) { + return []byte(password), nil +} From 153d18f38ced9ecc877b760d91aa45d6da325c70 Mon Sep 17 00:00:00 2001 From: Jason Wilder Date: Tue, 30 Jun 2015 13:51:32 -0600 Subject: [PATCH 5/5] Add synchronization around authcache accesses --- meta/store.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/meta/store.go b/meta/store.go index 24e87c2ec1..28e6a7546e 100644 --- a/meta/store.go +++ b/meta/store.go @@ -1002,6 +1002,9 @@ var ErrAuthenticate = errors.New("authentication failed") // Authenticate retrieves a user with a matching username and password. func (s *Store) Authenticate(username, password string) (ui *UserInfo, err error) { err = s.read(func(data *Data) error { + s.mu.Lock() + defer s.mu.Unlock() + // Find user. u := data.User(username) if u == nil { @@ -1017,6 +1020,7 @@ func (s *Store) Authenticate(username, password string) (ui *UserInfo, err error return ErrAuthenticate } } + // Compare password with user hash. if err := bcrypt.CompareHashAndPassword([]byte(u.Hash), []byte(password)); err != nil { return ErrAuthenticate @@ -1335,8 +1339,8 @@ type HashPasswordFn func(password string) ([]byte, error) // GetHashPasswordFn returns the current password hashing function. func (s *Store) GetHashPasswordFn() HashPasswordFn { - s.mu.Lock() - defer s.mu.Unlock() + s.mu.RLock() + defer s.mu.RUnlock() return s.hashPassword }