From d28e55d3b69013c7d42aae33c35332d02fc24837 Mon Sep 17 00:00:00 2001 From: Stuart Carnie Date: Fri, 9 Oct 2020 17:54:05 -0700 Subject: [PATCH] fix(kv): Reorganize bcrypt password hashing outside transactions This ensures that transaction lifetimes are shorter to reduce lock contention. Additionally, this PR removes the ability to set an empty password when running the initial on-boarding process. According to the original developer, this behavior was required to permit Cloud processes to complete. As this code is no longer shared, this security issue is corrected. --- kv/onboarding.go | 12 ++-- kv/passwords.go | 128 +++++++++++++++++++++++++----------------- kv/passwords_test.go | 9 --- testing/onboarding.go | 46 ++------------- 4 files changed, 90 insertions(+), 105 deletions(-) diff --git a/kv/onboarding.go b/kv/onboarding.go index 7b1030218e..e2ddebe7c4 100644 --- a/kv/onboarding.go +++ b/kv/onboarding.go @@ -124,15 +124,19 @@ func (s *Service) OnboardInitialUser(ctx context.Context, req *influxdb.Onboardi Token: req.Token, } + var passwordHash []byte + passwordHash, err = s.generatePasswordHash(req.Password) + if err != nil { + return nil, err + } + err = s.kv.Update(ctx, func(tx Tx) error { if err := s.createUser(ctx, tx, u); err != nil { return err } - if req.Password != "" { - if err := s.setPassword(ctx, tx, u.ID, req.Password); err != nil { - return err - } + if err := s.setPasswordHashTx(ctx, tx, u.ID, passwordHash); err != nil { + return err } if err := s.createOrganization(ctx, tx, o); err != nil { diff --git a/kv/passwords.go b/kv/passwords.go index 06f6ebf9c7..8e4e340071 100644 --- a/kv/passwords.go +++ b/kv/passwords.go @@ -75,34 +75,88 @@ var _ influxdb.PasswordsService = (*Service)(nil) // CompareAndSetPassword checks the password and if they match // updates to the new password. func (s *Service) CompareAndSetPassword(ctx context.Context, userID influxdb.ID, old string, new string) error { - return s.kv.Update(ctx, func(tx Tx) error { - if err := s.comparePassword(ctx, tx, userID, old); err != nil { - return err - } - return s.setPassword(ctx, tx, userID, new) - }) + newHash, err := s.generatePasswordHash(new) + if err != nil { + return err + } + + if err := s.compareUserPassword(ctx, userID, old); err != nil { + return err + } + + return s.setPasswordHash(ctx, userID, newHash) } // SetPassword overrides the password of a known user. func (s *Service) SetPassword(ctx context.Context, userID influxdb.ID, password string) error { - return s.kv.Update(ctx, func(tx Tx) error { - return s.setPassword(ctx, tx, userID, password) - }) + hash, err := s.generatePasswordHash(password) + if err != nil { + return err + } + + return s.setPasswordHash(ctx, userID, hash) } // ComparePassword checks if the password matches the password recorded. // Passwords that do not match return errors. func (s *Service) ComparePassword(ctx context.Context, userID influxdb.ID, password string) error { - return s.kv.View(ctx, func(tx Tx) error { - return s.comparePassword(ctx, tx, userID, password) + return s.compareUserPassword(ctx, userID, password) +} + +func (s *Service) getPasswordHash(ctx context.Context, userID influxdb.ID) ([]byte, error) { + var passwordHash []byte + err := s.kv.View(ctx, func(tx Tx) error { + var err error + encodedID, err := userID.Encode() + if err != nil { + return CorruptUserIDError(userID.String(), err) + } + + if _, err := s.findUserByID(ctx, tx, userID); err != nil { + return EIncorrectUser + } + + b, err := tx.Bucket(userpasswordBucket) + if err != nil { + return UnavailablePasswordServiceError(err) + } + + passwordHash, err = b.Get(encodedID) + if err != nil { + return EIncorrectPassword + } + + return nil + }) + + return passwordHash, err +} + +func (s *Service) compareUserPassword(ctx context.Context, userID influxdb.ID, password string) error { + passwordHash, err := s.getPasswordHash(ctx, userID) + if err != nil { + return err + } + + hasher := s.Hash + if hasher == nil { + hasher = &Bcrypt{} + } + + if err := hasher.CompareHashAndPassword(passwordHash, []byte(password)); err != nil { + return EIncorrectPassword + } + + return nil +} + +func (s *Service) setPasswordHash(ctx context.Context, userID influxdb.ID, hash []byte) error { + return s.kv.Update(ctx, func(tx Tx) error { + return s.setPasswordHashTx(ctx, tx, userID, hash) }) } -func (s *Service) setPassword(ctx context.Context, tx Tx, userID influxdb.ID, password string) error { - if len(password) < MinPasswordLength { - return EShortPassword - } - +func (s *Service) setPasswordHashTx(ctx context.Context, tx Tx, userID influxdb.ID, hash []byte) error { encodedID, err := userID.Encode() if err != nil { return CorruptUserIDError(userID.String(), err) @@ -117,53 +171,27 @@ func (s *Service) setPassword(ctx context.Context, tx Tx, userID influxdb.ID, pa return UnavailablePasswordServiceError(err) } - hasher := s.Hash - if hasher == nil { - hasher = &Bcrypt{} - } - - hash, err := hasher.GenerateFromPassword([]byte(password), DefaultCost) - if err != nil { - return InternalPasswordHashError(err) - } - if err := b.Put(encodedID, hash); err != nil { return UnavailablePasswordServiceError(err) } + return nil } -func (s *Service) comparePassword(ctx context.Context, tx Tx, userID influxdb.ID, password string) error { - encodedID, err := userID.Encode() - if err != nil { - return CorruptUserIDError(userID.String(), err) - } - - if _, err := s.findUserByID(ctx, tx, userID); err != nil { - return EIncorrectUser - } - - b, err := tx.Bucket(userpasswordBucket) - if err != nil { - return UnavailablePasswordServiceError(err) - } - - hash, err := b.Get(encodedID) - if err != nil { - // User exists but has no password has been set. - return EIncorrectPassword +func (s *Service) generatePasswordHash(password string) ([]byte, error) { + if len(password) < MinPasswordLength { + return nil, EShortPassword } hasher := s.Hash if hasher == nil { hasher = &Bcrypt{} } - - if err := hasher.CompareHashAndPassword(hash, []byte(password)); err != nil { - // User exists but the password was incorrect - return EIncorrectPassword + hash, err := hasher.GenerateFromPassword([]byte(password), DefaultCost) + if err != nil { + return nil, InternalPasswordHashError(err) } - return nil + return hash, nil } // DefaultCost is the cost that will actually be set if a cost below MinCost diff --git a/kv/passwords_test.go b/kv/passwords_test.go index a384cd2700..20b1836b05 100644 --- a/kv/passwords_test.go +++ b/kv/passwords_test.go @@ -158,9 +158,6 @@ func TestService_SetPassword(t *testing.T) { } return nil, kv.ErrKeyNotFound }, - PutFn: func(key, val []byte) error { - return nil - }, }, nil }, } @@ -193,9 +190,6 @@ func TestService_SetPassword(t *testing.T) { } return nil, kv.ErrKeyNotFound }, - PutFn: func(key, val []byte) error { - return nil - }, }, nil }, } @@ -231,9 +225,6 @@ func TestService_SetPassword(t *testing.T) { } return nil, kv.ErrKeyNotFound }, - PutFn: func(key, val []byte) error { - return nil - }, }, nil }, } diff --git a/testing/onboarding.go b/testing/onboarding.go index 203e132fc0..d60800dcc0 100644 --- a/testing/onboarding.go +++ b/testing/onboarding.go @@ -124,7 +124,7 @@ func OnboardInitialUser( }, }, { - name: "missing password should still work", + name: "missing password should fail", fields: OnboardingFields{ IDGenerator: &loopIDGenerator{ s: []string{oneID, twoID, threeID, fourID}, @@ -134,50 +134,12 @@ func OnboardInitialUser( }, args: args{ request: &platform.OnboardingRequest{ - User: "admin", - Org: "org1", - Bucket: "bucket1", + User: "admin", + Org: "org1", }, }, wants: wants{ - results: &platform.OnboardingResults{ - User: &platform.User{ - ID: MustIDBase16(oneID), - Name: "admin", - Status: platform.Active, - }, - Org: &platform.Organization{ - ID: MustIDBase16(twoID), - Name: "org1", - CRUDLog: platform.CRUDLog{ - CreatedAt: time.Date(2006, 5, 4, 1, 2, 3, 0, time.UTC), - UpdatedAt: time.Date(2006, 5, 4, 1, 2, 3, 0, time.UTC), - }, - }, - Bucket: &platform.Bucket{ - ID: MustIDBase16(threeID), - Name: "bucket1", - OrgID: MustIDBase16(twoID), - RetentionPeriod: 0, - CRUDLog: platform.CRUDLog{ - CreatedAt: time.Date(2006, 5, 4, 1, 2, 3, 0, time.UTC), - UpdatedAt: time.Date(2006, 5, 4, 1, 2, 3, 0, time.UTC), - }, - }, - Auth: &platform.Authorization{ - ID: MustIDBase16(fourID), - Token: oneToken, - Status: platform.Active, - UserID: MustIDBase16(oneID), - Description: "admin's Token", - OrgID: MustIDBase16(twoID), - Permissions: platform.OperPermissions(), - CRUDLog: platform.CRUDLog{ - CreatedAt: time.Date(2006, 5, 4, 1, 2, 3, 0, time.UTC), - UpdatedAt: time.Date(2006, 5, 4, 1, 2, 3, 0, time.UTC), - }, - }, - }, + errCode: platform.EEmptyValue, }, }, {