From 6c41e979b555df8869f2dfa6eaaa28997a892fe2 Mon Sep 17 00:00:00 2001 From: davidby-influx <72418212+davidby-influx@users.noreply.github.com> Date: Fri, 5 Apr 2024 15:42:37 -0700 Subject: [PATCH] feat: add optional stricter password requirements (#24857) (#24890) Allow password length and character class checking. closes https://github.com/influxdata/influxdb/issues/24856 (cherry picked from commit 7d8884beca1586a38b60a1aae9a0eb4f0461b123) closes https://github.com/influxdata/influxdb/issues/24884 --- cmd/influxd/launcher/cmd.go | 8 ++ cmd/influxd/launcher/launcher.go | 4 +- cmd/influxd/recovery/user/user_test.go | 3 +- cmd/influxd/upgrade/upgrade.go | 2 +- {tenant => kit/platform/errors}/error_user.go | 88 ++++++++------- session/http_server.go | 6 +- tenant/service.go | 18 ++- tenant/service_onboarding_test.go | 4 +- tenant/service_user.go | 104 +++++++++++++++--- tenant/service_user_test.go | 80 +++++++++++++- tenant/storage_user.go | 36 +++--- tenant/storage_user_test.go | 9 +- testing/passwords.go | 39 +++++-- v1/authorization/authorizer_test.go | 3 +- .../caching_password_service_test.go | 10 +- v1/authorization/service.go | 20 +++- v1/authorization/service_password.go | 24 ++-- 17 files changed, 341 insertions(+), 117 deletions(-) rename {tenant => kit/platform/errors}/error_user.go (57%) diff --git a/cmd/influxd/launcher/cmd.go b/cmd/influxd/launcher/cmd.go index 35a97601349..12b92b8a36e 100644 --- a/cmd/influxd/launcher/cmd.go +++ b/cmd/influxd/launcher/cmd.go @@ -191,6 +191,7 @@ type InfluxdOpts struct { Viper *viper.Viper HardeningEnabled bool + StrongPasswords bool } // NewOpts constructs options with default values. @@ -243,6 +244,7 @@ func NewOpts(viper *viper.Viper) *InfluxdOpts { TestingAlwaysAllowSetup: false, HardeningEnabled: false, + StrongPasswords: false, } } @@ -657,6 +659,12 @@ func (o *InfluxdOpts) BindCliOpts() []cli.Opt { Default: o.HardeningEnabled, Desc: "enable hardening options (disallow private IPs within flux and templates HTTP requests)", }, + { + DestP: &o.StrongPasswords, + Flag: "strong-passwords", + Default: o.StrongPasswords, + Desc: "enable password strength enforcement", + }, } } diff --git a/cmd/influxd/launcher/launcher.go b/cmd/influxd/launcher/launcher.go index 5fd9a2987be..78e467ee7b9 100644 --- a/cmd/influxd/launcher/launcher.go +++ b/cmd/influxd/launcher/launcher.go @@ -259,7 +259,7 @@ func (m *Launcher) run(ctx context.Context, opts *InfluxdOpts) (err error) { m.reg.MustRegister(infprom.NewInfluxCollector(procID, info)) tenantStore := tenant.NewStore(m.kvStore) - ts := tenant.NewSystem(tenantStore, m.log.With(zap.String("store", "new")), m.reg, metric.WithSuffix("new")) + ts := tenant.NewSystem(tenantStore, m.log.With(zap.String("store", "new")), m.reg, opts.StrongPasswords, metric.WithSuffix("new")) serviceConfig := kv.ServiceConfig{ FluxLanguageService: fluxlang.DefaultService, @@ -644,7 +644,7 @@ func (m *Launcher) run(ctx context.Context, opts *InfluxdOpts) (err error) { return err } - authSvcV1 = authv1.NewService(authStore, ts) + authSvcV1 = authv1.NewService(authStore, ts, authv1.WithPasswordChecking(opts.StrongPasswords)) passwordV1 = authv1.NewCachingPasswordsService(authSvcV1) } diff --git a/cmd/influxd/recovery/user/user_test.go b/cmd/influxd/recovery/user/user_test.go index a74ccf06523..87d012f71dd 100644 --- a/cmd/influxd/recovery/user/user_test.go +++ b/cmd/influxd/recovery/user/user_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/influxdata/influxdb/v2/cmd/influxd/recovery/testhelper" + "github.com/influxdata/influxdb/v2/kit/platform/errors" "github.com/stretchr/testify/assert" ) @@ -20,7 +21,7 @@ func Test_User_Basic(t *testing.T) { "user with name testuser already exists") // user needs a long-ish password - assert.EqualError(t, testhelper.RunCommand(t, NewUserCommand(), "create", "--bolt-path", db.Name(), "--username", "testuser2", "--password", "foo"), "passwords must be at least 8 characters long") + assert.EqualError(t, testhelper.RunCommand(t, NewUserCommand(), "create", "--bolt-path", db.Name(), "--username", "testuser2", "--password", "foo"), errors.EPasswordLength.Error()) assert.NoError(t, testhelper.RunCommand(t, NewUserCommand(), "create", "--bolt-path", db.Name(), "--username", "testuser2", "--password", "my_password"), "") // at least run the update code diff --git a/cmd/influxd/upgrade/upgrade.go b/cmd/influxd/upgrade/upgrade.go index aa4526feee1..6e53f053949 100644 --- a/cmd/influxd/upgrade/upgrade.go +++ b/cmd/influxd/upgrade/upgrade.go @@ -634,7 +634,7 @@ func newInfluxDBv2(ctx context.Context, opts *optionsV2, log *zap.Logger) (svc * // Create Tenant service (orgs, buckets, ) svc.tenantStore = tenant.NewStore(svc.kvStore) - svc.ts = tenant.NewSystem(svc.tenantStore, log.With(zap.String("store", "new")), reg, metric.WithSuffix("new")) + svc.ts = tenant.NewSystem(svc.tenantStore, log.With(zap.String("store", "new")), reg, false, metric.WithSuffix("new")) svc.meta = meta.NewClient(meta.NewConfig(), svc.kvStore) if err := svc.meta.Open(); err != nil { diff --git a/tenant/error_user.go b/kit/platform/errors/error_user.go similarity index 57% rename from tenant/error_user.go rename to kit/platform/errors/error_user.go index c564e37b040..00f8e0f7301 100644 --- a/tenant/error_user.go +++ b/kit/platform/errors/error_user.go @@ -1,73 +1,85 @@ -package tenant +package errors import ( "fmt" - - "github.com/influxdata/influxdb/v2/kit/platform/errors" ) const MinPasswordLen int = 8 +const MaxPasswordLen = 72 +const SpecialChars = `!@#$%^&*()_+` var ( // ErrUserNotFound is used when the user is not found. - ErrUserNotFound = &errors.Error{ + ErrUserNotFound = &Error{ Msg: "user not found", - Code: errors.ENotFound, + Code: ENotFound, } // EIncorrectPassword is returned when any password operation fails in which // we do not want to leak information. - EIncorrectPassword = &errors.Error{ - Code: errors.EForbidden, + EIncorrectPassword = &Error{ + Code: EForbidden, Msg: "your username or password is incorrect", } // EIncorrectUser is returned when any user is failed to be found which indicates // the userID provided is for a user that does not exist. - EIncorrectUser = &errors.Error{ - Code: errors.EForbidden, + EIncorrectUser = &Error{ + Code: EForbidden, Msg: "your userID is incorrect", } - // EShortPassword is used when a password is less than the minimum - // acceptable password length. - EShortPassword = &errors.Error{ - Code: errors.EInvalid, - Msg: fmt.Sprintf("passwords must be at least %d characters long", MinPasswordLen), + // EPasswordLength is used when a password is less than the minimum + // acceptable password length or longer than the maximum acceptable password length + EPasswordLength = &Error{ + Code: EInvalid, + Msg: fmt.Sprintf("passwords must be between %d and %d characters long", MinPasswordLen, MaxPasswordLen), + } + + EPasswordChars = &Error{ + Code: EInvalid, + Msg: fmt.Sprintf( + "passwords must contain at least three of the following character types: uppercase, lowercase, numbers, and special characters: %s", + SpecialChars), + } + + EPasswordChangeRequired = &Error{ + Code: EForbidden, + Msg: "password change required", } ) // UserAlreadyExistsError is used when attempting to create a user with a name // that already exists. -func UserAlreadyExistsError(n string) *errors.Error { - return &errors.Error{ - Code: errors.EConflict, +func UserAlreadyExistsError(n string) *Error { + return &Error{ + Code: EConflict, Msg: fmt.Sprintf("user with name %s already exists", n), } } // UserIDAlreadyExistsError is used when attempting to create a user with an ID // that already exists. -func UserIDAlreadyExistsError(id string) *errors.Error { - return &errors.Error{ - Code: errors.EConflict, +func UserIDAlreadyExistsError(id string) *Error { + return &Error{ + Code: EConflict, Msg: fmt.Sprintf("user with ID %s already exists", id), } } // UnexpectedUserBucketError is used when the error comes from an internal system. -func UnexpectedUserBucketError(err error) *errors.Error { - return &errors.Error{ - Code: errors.EInternal, +func UnexpectedUserBucketError(err error) *Error { + return &Error{ + Code: EInternal, Msg: fmt.Sprintf("unexpected error retrieving user bucket; Err: %v", err), Op: "kv/userBucket", } } // UnexpectedUserIndexError is used when the error comes from an internal system. -func UnexpectedUserIndexError(err error) *errors.Error { - return &errors.Error{ - Code: errors.EInternal, +func UnexpectedUserIndexError(err error) *Error { + return &Error{ + Code: EInternal, Msg: fmt.Sprintf("unexpected error retrieving user index; Err: %v", err), Op: "kv/userIndex", } @@ -75,9 +87,9 @@ func UnexpectedUserIndexError(err error) *errors.Error { // InvalidUserIDError is used when a service was provided an invalid ID. // This is some sort of internal server error. -func InvalidUserIDError(err error) *errors.Error { - return &errors.Error{ - Code: errors.EInvalid, +func InvalidUserIDError(err error) *Error { + return &Error{ + Code: EInvalid, Msg: "user id provided is invalid", Err: err, } @@ -85,9 +97,9 @@ func InvalidUserIDError(err error) *errors.Error { // ErrCorruptUser is used when the user cannot be unmarshalled from the bytes // stored in the kv. -func ErrCorruptUser(err error) *errors.Error { - return &errors.Error{ - Code: errors.EInternal, +func ErrCorruptUser(err error) *Error { + return &Error{ + Code: EInternal, Msg: "user could not be unmarshalled", Err: err, Op: "kv/UnmarshalUser", @@ -95,9 +107,9 @@ func ErrCorruptUser(err error) *errors.Error { } // ErrUnprocessableUser is used when a user is not able to be processed. -func ErrUnprocessableUser(err error) *errors.Error { - return &errors.Error{ - Code: errors.EUnprocessableEntity, +func ErrUnprocessableUser(err error) *Error { + return &Error{ + Code: EUnprocessableEntity, Msg: "user could not be marshalled", Err: err, Op: "kv/MarshalUser", @@ -107,9 +119,9 @@ func ErrUnprocessableUser(err error) *errors.Error { // UnavailablePasswordServiceError is used if we aren't able to add the // password to the store, it means the store is not available at the moment // (e.g. network). -func UnavailablePasswordServiceError(err error) *errors.Error { - return &errors.Error{ - Code: errors.EUnavailable, +func UnavailablePasswordServiceError(err error) *Error { + return &Error{ + Code: EUnavailable, Msg: fmt.Sprintf("Unable to connect to password service. Please try again; Err: %v", err), Op: "kv/setPassword", } diff --git a/session/http_server.go b/session/http_server.go index b78d5fea8f7..f6de1569f28 100644 --- a/session/http_server.go +++ b/session/http_server.go @@ -2,6 +2,7 @@ package session import ( "context" + eBase "errors" "net/http" "github.com/go-chi/chi" @@ -95,7 +96,10 @@ func (h *SessionHandler) handleSignin(w http.ResponseWriter, r *http.Request) { } if err := h.passSvc.ComparePassword(ctx, u.ID, req.Password); err != nil { - h.api.Err(w, r, ErrUnauthorized) + if !eBase.Is(err, errors.EPasswordChangeRequired) { + err = ErrUnauthorized + } + h.api.Err(w, r, err) return } diff --git a/tenant/service.go b/tenant/service.go index 3bb0d2f99fb..dc657a517e1 100644 --- a/tenant/service.go +++ b/tenant/service.go @@ -28,6 +28,8 @@ func isInternal(ctx context.Context) bool { type Service struct { store *Store + // Store raw version (not interface) for test purposes. + userSvc *UserSvc influxdb.UserService influxdb.PasswordsService influxdb.UserResourceMappingService @@ -44,11 +46,11 @@ func (s *Service) RUnlock() { } // NewService creates a new base tenant service. -func NewService(st *Store) *Service { +func NewService(st *Store, UserSvcOptFns ...func(svc *UserSvc)) *Service { svc := &Service{store: st} - userSvc := NewUserSvc(st, svc) - svc.UserService = userSvc - svc.PasswordsService = userSvc + svc.userSvc = NewUserSvc(st, svc, UserSvcOptFns...) + svc.UserService = svc.userSvc + svc.PasswordsService = svc.userSvc svc.UserResourceMappingService = NewUserResourceMappingSvc(st, svc) svc.OrganizationService = NewOrganizationSvc(st, svc) svc.BucketService = NewBucketSvc(st, svc) @@ -56,9 +58,13 @@ func NewService(st *Store) *Service { return svc } +func (s *Service) SetUserOptions(opts ...func(*UserSvc)) { + s.userSvc.SetOptions(opts...) +} + // creates a new Service with logging and metrics middleware wrappers. -func NewSystem(store *Store, log *zap.Logger, reg prometheus.Registerer, metricOpts ...metric.ClientOptFn) *Service { - ts := NewService(store) +func NewSystem(store *Store, log *zap.Logger, reg prometheus.Registerer, strongPasswords bool, metricOpts ...metric.ClientOptFn) *Service { + ts := NewService(store, WithPasswordChecking(strongPasswords)) ts.UserService = NewUserLogger(log, NewUserMetrics(reg, ts.UserService, metricOpts...)) ts.PasswordsService = NewPasswordLogger(log, NewPasswordMetrics(reg, ts.PasswordsService, metricOpts...)) ts.UserResourceMappingService = NewURMLogger(log, NewUrmMetrics(reg, ts.UserResourceMappingService, metricOpts...)) diff --git a/tenant/service_onboarding_test.go b/tenant/service_onboarding_test.go index e10a20288fe..0c85f8649f8 100644 --- a/tenant/service_onboarding_test.go +++ b/tenant/service_onboarding_test.go @@ -9,10 +9,12 @@ import ( "github.com/influxdata/influxdb/v2" "github.com/influxdata/influxdb/v2/authorization" icontext "github.com/influxdata/influxdb/v2/context" + influx_errors "github.com/influxdata/influxdb/v2/kit/platform/errors" "github.com/influxdata/influxdb/v2/kv" "github.com/influxdata/influxdb/v2/pkg/testing/assert" "github.com/influxdata/influxdb/v2/tenant" influxdbtesting "github.com/influxdata/influxdb/v2/testing" + assert2 "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -252,5 +254,5 @@ func TestOnboardService_WeakPassword(t *testing.T) { Org: "name", Bucket: "name", }) - assert.Equal(t, err, tenant.EShortPassword) + assert2.ErrorIs(t, err, influx_errors.EPasswordLength) } diff --git a/tenant/service_user.go b/tenant/service_user.go index 3236d8ca6e7..531f878c0ec 100644 --- a/tenant/service_user.go +++ b/tenant/service_user.go @@ -2,6 +2,9 @@ package tenant import ( "context" + eBase "errors" + "strings" + "unicode" "github.com/influxdata/influxdb/v2" "github.com/influxdata/influxdb/v2/kit/platform" @@ -11,14 +14,30 @@ import ( ) type UserSvc struct { - store *Store - svc *Service + store *Store + svc *Service + strongPasswords bool } -func NewUserSvc(st *Store, svc *Service) *UserSvc { - return &UserSvc{ - store: st, - svc: svc, +func NewUserSvc(st *Store, svc *Service, OptionFns ...func(*UserSvc)) *UserSvc { + userSvc := &UserSvc{ + store: st, + svc: svc, + strongPasswords: false, + } + userSvc.SetOptions(OptionFns...) + return userSvc +} + +func (s *UserSvc) SetOptions(opts ...func(*UserSvc)) { + for _, opt := range opts { + opt(s) + } +} + +func WithPasswordChecking(strong bool) func(*UserSvc) { + return func(u *UserSvc) { + u.strongPasswords = strong } } @@ -45,7 +64,7 @@ func (s *UserSvc) FindUserByID(ctx context.Context, id platform.ID) (*influxdb.U func (s *UserSvc) FindUser(ctx context.Context, filter influxdb.UserFilter) (*influxdb.User, error) { // if im given no filters its not a valid find user request. (leaving it unchecked seems dangerous) if filter.ID == nil && filter.Name == nil { - return nil, ErrUserNotFound + return nil, errors.ErrUserNotFound } if filter.ID != nil { @@ -179,8 +198,8 @@ func (s *UserSvc) FindPermissionForUser(ctx context.Context, uid platform.ID) (i // SetPassword overrides the password of a known user. func (s *UserSvc) SetPassword(ctx context.Context, userID platform.ID, password string) error { - if len(password) < MinPasswordLen { - return EShortPassword + if err := IsPasswordStrong(password, s.strongPasswords); err != nil { + return err } passHash, err := encryptPassword(password) if err != nil { @@ -190,27 +209,37 @@ func (s *UserSvc) SetPassword(ctx context.Context, userID platform.ID, password return s.store.Update(ctx, func(tx kv.Tx) error { _, err := s.store.GetUser(ctx, tx, userID) if err != nil { - return EIncorrectUser + return errors.EIncorrectUser } return s.store.SetPassword(ctx, tx, userID, passHash) }) } -// ComparePassword checks if the password matches the password recorded. -// Passwords that do not match return errors. func (s *UserSvc) ComparePassword(ctx context.Context, userID platform.ID, password string) error { + err := s.comparePasswordNoStrengthCheck(ctx, userID, password) + // If a password matches, but is too weak, force user to change + if errStrength := IsPasswordStrong(password, s.strongPasswords); err == nil && errStrength != nil { + return eBase.Join(errors.EPasswordChangeRequired, errStrength) + } else { + return err + } +} + +// comparePasswordNoStrengthCheck checks if the password matches the password recorded. +// Passwords that do not match return errors. +func (s *UserSvc) comparePasswordNoStrengthCheck(ctx context.Context, userID platform.ID, password string) error { // get password var hash []byte err := s.store.View(ctx, func(tx kv.Tx) error { _, err := s.store.GetUser(ctx, tx, userID) if err != nil { - return EIncorrectUser + return errors.EIncorrectUser } h, err := s.store.GetPassword(ctx, tx, userID) if err != nil { if err == kv.ErrKeyNotFound { - return EIncorrectPassword + return errors.EIncorrectPassword } return err } @@ -222,7 +251,7 @@ func (s *UserSvc) ComparePassword(ctx context.Context, userID platform.ID, passw } // compare password if err := bcrypt.CompareHashAndPassword(hash, []byte(password)); err != nil { - return EIncorrectPassword + return errors.EIncorrectPassword } return nil @@ -231,7 +260,7 @@ func (s *UserSvc) ComparePassword(ctx context.Context, userID platform.ID, passw // CompareAndSetPassword checks the password and if they match // updates to the new password. func (s *UserSvc) CompareAndSetPassword(ctx context.Context, userID platform.ID, old, new string) error { - err := s.ComparePassword(ctx, userID, old) + err := s.comparePasswordNoStrengthCheck(ctx, userID, old) if err != nil { return err } @@ -247,6 +276,49 @@ func encryptPassword(password string) (string, error) { return string(passHash), nil } +var classes []func(rune) bool = []func(rune) bool{ + unicode.IsNumber, + unicode.IsUpper, + unicode.IsLower, + func(r rune) bool { + found := false + for _, s := range errors.SpecialChars { + found = (s == r) || found + } + return found + }, +} + +// IsPasswordStrong checks if a password is strong enough. +func IsPasswordStrong(password string, doCheck bool) error { + const numClassesRequired = 3 + var eSlice []error = nil + l := len(password) + if l < errors.MinPasswordLen || l > errors.MaxPasswordLen { + eSlice = append(eSlice, errors.EPasswordLength) + } + if doCheck { + // make a password copy that is the length of the max password length + constLenPassword := strings.Repeat(password, 1+(errors.MaxPasswordLen/len(password)))[:errors.MaxPasswordLen] + n := 0 + + // Walk the whole string for each class, for constant time operation + for _, f := range classes { + found := false + for _, r := range constLenPassword { + found = f(r) || found + } + if found { + n++ + } + } + if n < numClassesRequired { + eSlice = append(eSlice, errors.EPasswordChars) + } + } + return eBase.Join(eSlice...) +} + func permissionFromMapping(mappings []*influxdb.UserResourceMapping) ([]influxdb.Permission, error) { ps := make([]influxdb.Permission, 0, len(mappings)) for _, m := range mappings { diff --git a/tenant/service_user_test.go b/tenant/service_user_test.go index a6b2f8bbc92..d41d754662d 100644 --- a/tenant/service_user_test.go +++ b/tenant/service_user_test.go @@ -2,11 +2,14 @@ package tenant_test import ( "context" + "errors" + "strings" "testing" "github.com/google/go-cmp/cmp" "github.com/influxdata/influxdb/v2" "github.com/influxdata/influxdb/v2/kit/platform" + influx_errors "github.com/influxdata/influxdb/v2/kit/platform/errors" "github.com/influxdata/influxdb/v2/kv" "github.com/influxdata/influxdb/v2/tenant" influxdbtesting "github.com/influxdata/influxdb/v2/testing" @@ -44,6 +47,80 @@ func initUserService(s kv.Store, f influxdbtesting.UserFields, t *testing.T) (in } } +func TestPasswordStrengthChecker(t *testing.T) { + tests := []struct { + testName string + password string + checkPassword bool + want []error + }{ + { + testName: "valid password", + password: "Password123", + checkPassword: true, + want: nil, + }, + { + testName: "valid password with specials hardened", + password: "[]*()&&Tord123", + checkPassword: true, + want: nil, + }, + { + testName: "too short", + password: "1aS*", + checkPassword: false, + want: []error{influx_errors.EPasswordLength}, + }, + { + testName: "too short hardened", + password: "1aS*", + checkPassword: true, + want: []error{influx_errors.EPasswordLength}, + }, + { + testName: "too short and too few classes hardened", + password: "admin", + checkPassword: true, + want: []error{influx_errors.EPasswordLength, influx_errors.EPasswordChars}, + }, + { + testName: "too long", + password: strings.Repeat("Aa$3456789", 8), + checkPassword: false, + want: []error{influx_errors.EPasswordLength}, + }, + { + testName: "too long hardened", + password: strings.Repeat("Aa$3456789", 8), + checkPassword: true, + want: []error{influx_errors.EPasswordLength}, + }, + { + testName: "too long and too few classes", + password: strings.Repeat("A123456789", 8), + checkPassword: false, + want: []error{influx_errors.EPasswordLength}, + }, + { + testName: "too long and too few classes hardened", + password: strings.Repeat("A123456789", 8), + checkPassword: true, + want: []error{influx_errors.EPasswordLength, influx_errors.EPasswordChars}, + }, + } + for _, tt := range tests { + t.Run(tt.testName, func(t *testing.T) { + err := tenant.IsPasswordStrong(tt.password, tt.checkPassword) + for _, want := range tt.want { + if !errors.Is(err, want) { + t.Errorf("expected %v, got %v", tt.want, err) + } + } + }) + } +} + func TestBoltPasswordService(t *testing.T) { influxdbtesting.PasswordsService(initBoltPasswordsService, t) } @@ -59,7 +136,7 @@ func initBoltPasswordsService(f influxdbtesting.PasswordFields, t *testing.T) (i func initPasswordsService(s kv.Store, f influxdbtesting.PasswordFields, t *testing.T) (influxdb.PasswordsService, func()) { storage := tenant.NewStore(s) - svc := tenant.NewService(storage) + svc := tenant.NewService(storage, tenant.WithPasswordChecking(false)) for _, u := range f.Users { if err := svc.CreateUser(context.Background(), u); err != nil { @@ -73,6 +150,7 @@ func initPasswordsService(s kv.Store, f influxdbtesting.PasswordFields, t *testi } } + svc.SetUserOptions(tenant.WithPasswordChecking(true)) return svc, func() { for _, u := range f.Users { if err := svc.DeleteUser(context.Background(), u.ID); err != nil { diff --git a/tenant/storage_user.go b/tenant/storage_user.go index 0d813519e5d..3ed7fbb5818 100644 --- a/tenant/storage_user.go +++ b/tenant/storage_user.go @@ -21,7 +21,7 @@ var ( func unmarshalUser(v []byte) (*influxdb.User, error) { u := &influxdb.User{} if err := json.Unmarshal(v, u); err != nil { - return nil, ErrCorruptUser(err) + return nil, errors.ErrCorruptUser(err) } return u, nil @@ -30,7 +30,7 @@ func unmarshalUser(v []byte) (*influxdb.User, error) { func marshalUser(u *influxdb.User) ([]byte, error) { v, err := json.Marshal(u) if err != nil { - return nil, ErrUnprocessableUser(err) + return nil, errors.ErrUnprocessableUser(err) } return v, nil @@ -51,11 +51,11 @@ func (s *Store) uniqueUserName(tx kv.Tx, uname string) error { // no error means this is not unique if err == nil { - return UserAlreadyExistsError(uname) + return errors.UserAlreadyExistsError(uname) } // any other error is some sort of internal server error - return ErrUnprocessableUser(err) + return errors.ErrUnprocessableUser(err) } func (s *Store) uniqueUserID(tx kv.Tx, id platform.ID) error { @@ -72,10 +72,10 @@ func (s *Store) uniqueUserID(tx kv.Tx, id platform.ID) error { } if err == nil { - return UserIDAlreadyExistsError(id.String()) + return errors.UserIDAlreadyExistsError(id.String()) } - return ErrUnprocessableUser(err) + return errors.ErrUnprocessableUser(err) } func (s *Store) GetUser(ctx context.Context, tx kv.Tx, id platform.ID) (user *influxdb.User, retErr error) { @@ -84,7 +84,7 @@ func (s *Store) GetUser(ctx context.Context, tx kv.Tx, id platform.ID) (user *in }() encodedID, err := id.Encode() if err != nil { - return nil, InvalidUserIDError(err) + return nil, errors.InvalidUserIDError(err) } b, err := tx.Bucket(userBucket) @@ -94,7 +94,7 @@ func (s *Store) GetUser(ctx context.Context, tx kv.Tx, id platform.ID) (user *in v, err := b.Get(encodedID) if kv.IsNotFound(err) { - return nil, ErrUserNotFound + return nil, errors.ErrUserNotFound } if err != nil { @@ -115,7 +115,7 @@ func (s *Store) GetUserByName(ctx context.Context, tx kv.Tx, n string) (user *in uid, err := b.Get([]byte(n)) if err == kv.ErrKeyNotFound { - return nil, ErrUserNotFound + return nil, errors.ErrUserNotFound } if err != nil { @@ -195,7 +195,7 @@ func (s *Store) CreateUser(ctx context.Context, tx kv.Tx, u *influxdb.User) (ret encodedID, err := u.ID.Encode() if err != nil { - return InvalidUserIDError(err) + return errors.InvalidUserIDError(err) } // Verify that both the provided username and user ID are not already in-use @@ -298,7 +298,7 @@ func (s *Store) DeleteUser(ctx context.Context, tx kv.Tx, id platform.ID) (retEr encodedID, err := id.Encode() if err != nil { - return InvalidUserIDError(err) + return errors.InvalidUserIDError(err) } idx, err := tx.Bucket(userIndex) @@ -322,7 +322,7 @@ func (s *Store) DeleteUser(ctx context.Context, tx kv.Tx, id platform.ID) (retEr // Clean up user's password. ub, err := tx.Bucket(userpasswordBucket) if err != nil { - return UnavailablePasswordServiceError(err) + return errors.UnavailablePasswordServiceError(err) } if err := ub.Delete(encodedID); err != nil { return err @@ -347,12 +347,12 @@ func (s *Store) DeleteUser(ctx context.Context, tx kv.Tx, id platform.ID) (retEr func (s *Store) GetPassword(ctx context.Context, tx kv.Tx, id platform.ID) (string, error) { encodedID, err := id.Encode() if err != nil { - return "", InvalidUserIDError(err) + return "", errors.InvalidUserIDError(err) } b, err := tx.Bucket(userpasswordBucket) if err != nil { - return "", UnavailablePasswordServiceError(err) + return "", errors.UnavailablePasswordServiceError(err) } passwd, err := b.Get(encodedID) @@ -363,12 +363,12 @@ func (s *Store) GetPassword(ctx context.Context, tx kv.Tx, id platform.ID) (stri func (s *Store) SetPassword(ctx context.Context, tx kv.Tx, id platform.ID, password string) error { encodedID, err := id.Encode() if err != nil { - return InvalidUserIDError(err) + return errors.InvalidUserIDError(err) } b, err := tx.Bucket(userpasswordBucket) if err != nil { - return UnavailablePasswordServiceError(err) + return errors.UnavailablePasswordServiceError(err) } return b.Put(encodedID, []byte(password)) @@ -377,12 +377,12 @@ func (s *Store) SetPassword(ctx context.Context, tx kv.Tx, id platform.ID, passw func (s *Store) DeletePassword(ctx context.Context, tx kv.Tx, id platform.ID) error { encodedID, err := id.Encode() if err != nil { - return InvalidUserIDError(err) + return errors.InvalidUserIDError(err) } b, err := tx.Bucket(userpasswordBucket) if err != nil { - return UnavailablePasswordServiceError(err) + return errors.UnavailablePasswordServiceError(err) } return b.Delete(encodedID) diff --git a/tenant/storage_user_test.go b/tenant/storage_user_test.go index 9f3ef3eac0f..5097e57b658 100644 --- a/tenant/storage_user_test.go +++ b/tenant/storage_user_test.go @@ -9,6 +9,7 @@ import ( "github.com/influxdata/influxdb/v2" "github.com/influxdata/influxdb/v2/kit/platform" + influx_errors "github.com/influxdata/influxdb/v2/kit/platform/errors" "github.com/influxdata/influxdb/v2/kv" "github.com/influxdata/influxdb/v2/tenant" itesting "github.com/influxdata/influxdb/v2/testing" @@ -119,11 +120,11 @@ func TestUser(t *testing.T) { t.Fatalf("expected identical user: \n%+v\n%+v", user, expected) } - if _, err := store.GetUser(context.Background(), tx, 500); !errors.Is(err, tenant.ErrUserNotFound) { + if _, err := store.GetUser(context.Background(), tx, 500); !errors.Is(err, influx_errors.ErrUserNotFound) { t.Fatal("failed to get correct error when looking for invalid user by id") } - if _, err := store.GetUserByName(context.Background(), tx, "notauser"); !errors.Is(err, tenant.ErrUserNotFound) { + if _, err := store.GetUserByName(context.Background(), tx, "notauser"); !errors.Is(err, influx_errors.ErrUserNotFound) { t.Fatal("failed to get correct error when looking for invalid user by name") } @@ -199,7 +200,7 @@ func TestUser(t *testing.T) { update: func(t *testing.T, store *tenant.Store, tx kv.Tx) { user5 := "user5" _, err := store.UpdateUser(context.Background(), tx, platform.ID(3), influxdb.UserUpdate{Name: &user5}) - if err.Error() != tenant.UserAlreadyExistsError(user5).Error() { + if err.Error() != influx_errors.UserAlreadyExistsError(user5).Error() { t.Fatal("failed to error on duplicate username") } @@ -250,7 +251,7 @@ func TestUser(t *testing.T) { } err = store.DeleteUser(context.Background(), tx, 1) - if !errors.Is(err, tenant.ErrUserNotFound) { + if !errors.Is(err, influx_errors.ErrUserNotFound) { t.Fatal("invalid error when deleting user that has already been deleted", err) } diff --git a/testing/passwords.go b/testing/passwords.go index e40894182b1..d26ab500c89 100644 --- a/testing/passwords.go +++ b/testing/passwords.go @@ -2,11 +2,13 @@ package testing import ( "context" + eBase "errors" "fmt" "testing" "github.com/influxdata/influxdb/v2" "github.com/influxdata/influxdb/v2/kit/platform" + "github.com/influxdata/influxdb/v2/kit/platform/errors" ) // PasswordFields will include the IDGenerator, and users and their passwords. @@ -76,7 +78,7 @@ func SetPassword( }, args: args{ user: MustIDBase16(oneID), - password: "howdydoody", + password: "howdY&&doody", }, wants: wants{}, }, @@ -92,10 +94,10 @@ func SetPassword( }, args: args{ user: MustIDBase16(oneID), - password: "short", + password: "A2$u", }, wants: wants{ - err: fmt.Errorf("passwords must be at least 8 characters long"), + err: errors.EPasswordLength, }, }, { @@ -110,7 +112,7 @@ func SetPassword( }, args: args{ user: 33, - password: "howdydoody", + password: "Howdy#Doody", }, wants: wants{ err: fmt.Errorf("your userID is incorrect"), @@ -166,14 +168,31 @@ func ComparePassword( ID: MustIDBase16(oneID), }, }, - Passwords: []string{"howdydoody"}, + Passwords: []string{"Howdy%doody"}, }, args: args{ user: MustIDBase16(oneID), - password: "howdydoody", + password: "Howdy%doody", }, wants: wants{}, }, + { + name: "comparing same weak password forces change", + fields: PasswordFields{ + Users: []*influxdb.User{ + { + Name: "user1", + ID: MustIDBase16(oneID), + }, + }, + Passwords: []string{"Howdydoody"}, + }, + args: args{ + user: MustIDBase16(oneID), + password: "Howdydoody", + }, + wants: wants{eBase.Join(errors.EPasswordChangeRequired, errors.EPasswordChars)}, + }, { name: "comparing different password is an error", fields: PasswordFields{ @@ -283,12 +302,12 @@ func CompareAndSetPassword( ID: MustIDBase16(oneID), }, }, - Passwords: []string{"howdydoody"}, + Passwords: []string{"howdY&doody"}, }, args: args{ user: MustIDBase16(oneID), - old: "howdydoody", - new: "howdydoody", + old: "howdY&doody", + new: "howdY&doody", }, wants: wants{}, }, @@ -329,7 +348,7 @@ func CompareAndSetPassword( new: "short", }, wants: wants{ - err: fmt.Errorf("passwords must be at least 8 characters long"), + err: eBase.Join(errors.EPasswordLength, errors.EPasswordChars), }, }, } diff --git a/v1/authorization/authorizer_test.go b/v1/authorization/authorizer_test.go index 61f19eb83ba..b128942bd39 100644 --- a/v1/authorization/authorizer_test.go +++ b/v1/authorization/authorizer_test.go @@ -6,6 +6,7 @@ import ( "github.com/golang/mock/gomock" "github.com/influxdata/influxdb/v2" + "github.com/influxdata/influxdb/v2/kit/platform/errors" itesting "github.com/influxdata/influxdb/v2/testing" "github.com/influxdata/influxdb/v2/v1/authorization/mocks" "github.com/stretchr/testify/assert" @@ -96,7 +97,7 @@ func TestAuthorizer_Authorize(t *testing.T) { pw := mocks.NewMockPasswordComparer(ctrl) pw.EXPECT(). ComparePassword(ctx, authID, token). - Return(EIncorrectPassword) + Return(errors.EIncorrectPassword) authz := Authorizer{ AuthV1: v1, diff --git a/v1/authorization/caching_password_service_test.go b/v1/authorization/caching_password_service_test.go index e9405c2cafe..b78cd3ee69d 100644 --- a/v1/authorization/caching_password_service_test.go +++ b/v1/authorization/caching_password_service_test.go @@ -7,8 +7,8 @@ import ( "github.com/golang/mock/gomock" "github.com/influxdata/influxdb/v2/kit/platform" + "github.com/influxdata/influxdb/v2/kit/platform/errors" "github.com/influxdata/influxdb/v2/mock" - "github.com/influxdata/influxdb/v2/tenant" "github.com/stretchr/testify/assert" ) @@ -84,7 +84,7 @@ func TestCachingPasswordsService(t *testing.T) { inner := mock.NewMockPasswordsService(ctrl) inner.EXPECT(). ComparePassword(gomock.Any(), user1, "foo"). - Return(tenant.EShortPassword) + Return(errors.EPasswordLength) s := NewCachingPasswordsService(inner) s.authCache[user2] = userE2 @@ -143,7 +143,7 @@ func TestCachingPasswordsService(t *testing.T) { inner := mock.NewMockPasswordsService(ctrl) inner.EXPECT(). SetPassword(gomock.Any(), user1, "foo"). - Return(tenant.EShortPassword) + Return(errors.EPasswordLength) s := NewCachingPasswordsService(inner) s.authCache[user1] = userE1 @@ -153,7 +153,7 @@ func TestCachingPasswordsService(t *testing.T) { _, ok := s.authCache[user1] assert.True(t, ok) - assert.EqualError(t, s.SetPassword(ctx, user1, "foo"), tenant.EShortPassword.Error()) + assert.EqualError(t, s.SetPassword(ctx, user1, "foo"), errors.EPasswordLength.Error()) _, ok = s.authCache[user1] assert.True(t, ok) }) @@ -165,7 +165,7 @@ func TestCachingPasswordsService(t *testing.T) { inner := mock.NewMockPasswordsService(ctrl) inner.EXPECT(). CompareAndSetPassword(gomock.Any(), user1, "foo", "foo2"). - Return(tenant.EShortPassword) + Return(errors.EPasswordLength) s := NewCachingPasswordsService(inner) s.authCache[user1] = userE1 diff --git a/v1/authorization/service.go b/v1/authorization/service.go index da8b6ac1364..2582532204a 100644 --- a/v1/authorization/service.go +++ b/v1/authorization/service.go @@ -16,15 +16,27 @@ var ( ) type Service struct { - store *Store - tenantService TenantService + store *Store + tenantService TenantService + strongPasswords bool } -func NewService(st *Store, ts TenantService) *Service { - return &Service{ +// NewService constructs a new Service. +func NewService(st *Store, ts TenantService, OptFns ...func(*Service)) *Service { + svc := &Service{ store: st, tenantService: ts, } + for _, fn := range OptFns { + fn(svc) + } + return svc +} + +func WithPasswordChecking(strong bool) func(*Service) { + return func(s *Service) { + s.strongPasswords = strong + } } func (s *Service) CreateAuthorization(ctx context.Context, a *influxdb.Authorization) error { diff --git a/v1/authorization/service_password.go b/v1/authorization/service_password.go index 1ad8648924f..10f1f267b6f 100644 --- a/v1/authorization/service_password.go +++ b/v1/authorization/service_password.go @@ -2,6 +2,7 @@ package authorization import ( "context" + eBase "errors" "github.com/influxdata/influxdb/v2/kit/platform" "github.com/influxdata/influxdb/v2/kit/platform/errors" @@ -10,8 +11,6 @@ import ( "golang.org/x/crypto/bcrypt" ) -var EIncorrectPassword = tenant.EIncorrectPassword - // SetPasswordHash updates the password hash for id. If passHash is not a valid bcrypt hash, // SetPasswordHash returns an error. // @@ -38,8 +37,8 @@ func (s *Service) SetPasswordHash(ctx context.Context, authID platform.ID, passH // SetPassword overrides the password of a known user. func (s *Service) SetPassword(ctx context.Context, authID platform.ID, password string) error { - if len(password) < tenant.MinPasswordLen { - return tenant.EShortPassword + if err := tenant.IsPasswordStrong(password, s.strongPasswords); err != nil { + return err } passHash, err := encryptPassword(password) if err != nil { @@ -56,8 +55,17 @@ func (s *Service) SetPassword(ctx context.Context, authID platform.ID, password } // ComparePassword checks if the password matches the password recorded. -// Passwords that do not match return errors. +// Passwords that do not match return errors, as do too weak passwords func (s *Service) ComparePassword(ctx context.Context, authID platform.ID, password string) error { + err := s.comparePasswordNoStrengthCheck(ctx, authID, password) + // If a password matches, but is too weak, force user to change + if errStrength := tenant.IsPasswordStrong(password, s.strongPasswords); err == nil && errStrength != nil { + return eBase.Join(errors.EPasswordChangeRequired, errStrength) + } + return err +} + +func (s *Service) comparePasswordNoStrengthCheck(ctx context.Context, authID platform.ID, password string) error { // get password var hash []byte err := s.store.View(ctx, func(tx kv.Tx) error { @@ -68,7 +76,7 @@ func (s *Service) ComparePassword(ctx context.Context, authID platform.ID, passw h, err := s.store.GetPassword(ctx, tx, authID) if err != nil { if err == kv.ErrKeyNotFound { - return EIncorrectPassword + return errors.EIncorrectPassword } return err } @@ -80,7 +88,7 @@ func (s *Service) ComparePassword(ctx context.Context, authID platform.ID, passw } // compare password if err := bcrypt.CompareHashAndPassword(hash, []byte(password)); err != nil { - return EIncorrectPassword + return errors.EIncorrectPassword } return nil @@ -89,7 +97,7 @@ func (s *Service) ComparePassword(ctx context.Context, authID platform.ID, passw // CompareAndSetPassword checks the password and if they match // updates to the new password. func (s *Service) CompareAndSetPassword(ctx context.Context, authID platform.ID, old, new string) error { - err := s.ComparePassword(ctx, authID, old) + err := s.comparePasswordNoStrengthCheck(ctx, authID, old) if err != nil { return err }