From af05bd6209f60c9ffda8043c48be23ebfaad07a7 Mon Sep 17 00:00:00 2001 From: Daniel Moran Date: Wed, 16 Dec 2020 06:43:43 -0800 Subject: [PATCH] fix(onboarding): don't ignore failures to set initial password (#20317) --- CHANGELOG.md | 1 + cmd/influxd/launcher/launcher.go | 11 ++++++----- cmd/influxd/upgrade/security_test.go | 2 +- tenant/error_user.go | 4 +++- tenant/middleware_onboarding_logging.go | 6 +++--- tenant/service_onboarding.go | 22 +++++++++++++++++++++- tenant/service_onboarding_test.go | 24 ++++++++++++++++++++++++ tenant/service_user.go | 2 +- v1/authorization/service_password.go | 2 +- 9 files changed, 61 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce2bd65e74a..dac99cc46c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ 1. [20350](https://github.com/influxdata/influxdb/pull/20350): Don't show the upgrade notice on fresh `influxdb2` installs. 1. [20350](https://github.com/influxdata/influxdb/pull/20350): Ensure `config.toml` is initialized on fresh `influxdb2` installs. 1. [20347](https://github.com/influxdata/influxdb/pull/20347): Include upgrade helper script in goreleaser manifest. +1. [20317](https://github.com/influxdata/influxdb/pull/20317): Don't ignore failures to set password during initial user onboarding. ## v2.0.3 [2020-12-14] ---------------------- diff --git a/cmd/influxd/launcher/launcher.go b/cmd/influxd/launcher/launcher.go index f4cc8645213..1e4f3aea00f 100644 --- a/cmd/influxd/launcher/launcher.go +++ b/cmd/influxd/launcher/launcher.go @@ -1128,15 +1128,16 @@ func (m *Launcher) run(ctx context.Context) (err error) { ts.BucketService = storage.NewBucketService(m.log, ts.BucketService, m.engine) ts.BucketService = dbrp.NewBucketService(m.log, ts.BucketService, dbrpSvc) - var onboardOpts []tenant.OnboardServiceOptionFn + onboardingLogger := m.log.With(zap.String("handler", "onboard")) + onboardOpts := []tenant.OnboardServiceOptionFn{tenant.WithOnboardingLogger(onboardingLogger)} if m.testingAlwaysAllowSetup { onboardOpts = append(onboardOpts, tenant.WithAlwaysAllowInitialUser()) } - onboardSvc := tenant.NewOnboardService(ts, authSvc, onboardOpts...) // basic service - onboardSvc = tenant.NewAuthedOnboardSvc(onboardSvc) // with auth - onboardSvc = tenant.NewOnboardingMetrics(m.reg, onboardSvc, metric.WithSuffix("new")) // with metrics - onboardSvc = tenant.NewOnboardingLogger(m.log.With(zap.String("handler", "onboard")), onboardSvc) // with logging + onboardSvc := tenant.NewOnboardService(ts, authSvc, onboardOpts...) // basic service + onboardSvc = tenant.NewAuthedOnboardSvc(onboardSvc) // with auth + onboardSvc = tenant.NewOnboardingMetrics(m.reg, onboardSvc, metric.WithSuffix("new")) // with metrics + onboardSvc = tenant.NewOnboardingLogger(onboardingLogger, onboardSvc) // with logging var ( authorizerV1 platform.AuthorizerV1 diff --git a/cmd/influxd/upgrade/security_test.go b/cmd/influxd/upgrade/security_test.go index 3e7368728b7..cc3bf3c30d0 100644 --- a/cmd/influxd/upgrade/security_test.go +++ b/cmd/influxd/upgrade/security_test.go @@ -184,7 +184,7 @@ func TestUpgradeSecurity(t *testing.T) { // onboard admin oReq := &influxdb.OnboardingRequest{ User: "admin", - Password: "12345", + Password: "12345678", Org: "testers", Bucket: "def", RetentionPeriod: influxdb.InfiniteRetention, diff --git a/tenant/error_user.go b/tenant/error_user.go index 79192f35d13..05aa1892dd7 100644 --- a/tenant/error_user.go +++ b/tenant/error_user.go @@ -6,6 +6,8 @@ import ( "github.com/influxdata/influxdb/v2" ) +const MinPasswordLen int = 8 + var ( // ErrUserNotFound is used when the user is not found. ErrUserNotFound = &influxdb.Error{ @@ -31,7 +33,7 @@ var ( // acceptable password length. EShortPassword = &influxdb.Error{ Code: influxdb.EInvalid, - Msg: "passwords must be at least 8 characters long", + Msg: fmt.Sprintf("passwords must be at least %d characters long", MinPasswordLen), } ) diff --git a/tenant/middleware_onboarding_logging.go b/tenant/middleware_onboarding_logging.go index 4d0f4245bbb..e2cb438ac46 100644 --- a/tenant/middleware_onboarding_logging.go +++ b/tenant/middleware_onboarding_logging.go @@ -28,7 +28,7 @@ func (l *OnboardingLogger) IsOnboarding(ctx context.Context) (available bool, er defer func(start time.Time) { dur := zap.Duration("took", time.Since(start)) if err != nil { - l.logger.Debug("failed to check onboarding", zap.Error(err), dur) + l.logger.Error("failed to check onboarding", zap.Error(err), dur) return } l.logger.Debug("is onboarding", dur) @@ -41,7 +41,7 @@ func (l *OnboardingLogger) OnboardInitialUser(ctx context.Context, req *influxdb dur := zap.Duration("took", time.Since(start)) if err != nil { msg := fmt.Sprintf("failed to onboard user %s", req.User) - l.logger.Debug(msg, zap.Error(err), dur) + l.logger.Error(msg, zap.Error(err), dur) return } l.logger.Debug("onboard initial user", dur) @@ -54,7 +54,7 @@ func (l *OnboardingLogger) OnboardUser(ctx context.Context, req *influxdb.Onboar dur := zap.Duration("took", time.Since(start)) if err != nil { msg := fmt.Sprintf("failed to onboard user %s", req.User) - l.logger.Debug(msg, zap.Error(err), dur) + l.logger.Error(msg, zap.Error(err), dur) return } l.logger.Debug("onboard user", dur) diff --git a/tenant/service_onboarding.go b/tenant/service_onboarding.go index a0845a853db..eacab0c5409 100644 --- a/tenant/service_onboarding.go +++ b/tenant/service_onboarding.go @@ -6,12 +6,14 @@ import ( "github.com/influxdata/influxdb/v2" icontext "github.com/influxdata/influxdb/v2/context" "github.com/influxdata/influxdb/v2/kv" + "go.uber.org/zap" ) type OnboardService struct { service *Service authSvc influxdb.AuthorizationService alwaysAllow bool + log *zap.Logger } type OnboardServiceOptionFn func(*OnboardService) @@ -25,10 +27,17 @@ func WithAlwaysAllowInitialUser() OnboardServiceOptionFn { } } +func WithOnboardingLogger(logger *zap.Logger) OnboardServiceOptionFn { + return func(s *OnboardService) { + s.log = logger + } +} + func NewOnboardService(svc *Service, as influxdb.AuthorizationService, opts ...OnboardServiceOptionFn) influxdb.OnboardingService { s := &OnboardService{ service: svc, authSvc: as, + log: zap.NewNop(), } for _, opt := range opts { @@ -98,7 +107,18 @@ func (s *OnboardService) onboardUser(ctx context.Context, req *influxdb.Onboardi // create users password if req.Password != "" { - s.service.SetPassword(ctx, user.ID, req.Password) + if err := s.service.SetPassword(ctx, user.ID, req.Password); err != nil { + // Try to clean up. + if cleanupErr := s.service.DeleteUser(ctx, user.ID); cleanupErr != nil { + s.log.Error( + "couldn't clean up user after failing to set password", + zap.String("user", user.Name), + zap.String("user_id", user.ID.String()), + zap.Error(cleanupErr), + ) + } + return nil, err + } } // set the new user in the context diff --git a/tenant/service_onboarding_test.go b/tenant/service_onboarding_test.go index 6a0dd586034..1029a28bf44 100644 --- a/tenant/service_onboarding_test.go +++ b/tenant/service_onboarding_test.go @@ -200,3 +200,27 @@ func TestOnboardService_RetentionPolicy(t *testing.T) { assert.Equal(t, onboard.Bucket.RetentionPeriod, retention, "Retention policy should pass through") } + +func TestOnboardService_WeakPassword(t *testing.T) { + s, _, _ := NewTestInmemStore(t) + storage := tenant.NewStore(s) + ten := tenant.NewService(storage) + + authStore, err := authorization.NewStore(s) + require.NoError(t, err) + + authSvc := authorization.NewService(authStore, ten) + svc := tenant.NewOnboardService(ten, authSvc) + + ctx := icontext.SetAuthorizer(context.Background(), &influxdb.Authorization{ + UserID: 123, + }) + + _, err = svc.OnboardInitialUser(ctx, &influxdb.OnboardingRequest{ + User: "name", + Password: "short", + Org: "name", + Bucket: "name", + }) + assert.Equal(t, err, tenant.EShortPassword) +} diff --git a/tenant/service_user.go b/tenant/service_user.go index 5857cf99161..1814dfe2167 100644 --- a/tenant/service_user.go +++ b/tenant/service_user.go @@ -177,7 +177,7 @@ func (s *UserSvc) FindPermissionForUser(ctx context.Context, uid influxdb.ID) (i // SetPassword overrides the password of a known user. func (s *UserSvc) SetPassword(ctx context.Context, userID influxdb.ID, password string) error { - if len(password) < 8 { + if len(password) < MinPasswordLen { return EShortPassword } passHash, err := encryptPassword(password) diff --git a/v1/authorization/service_password.go b/v1/authorization/service_password.go index 05341cdd58f..15b88708ba8 100644 --- a/v1/authorization/service_password.go +++ b/v1/authorization/service_password.go @@ -37,7 +37,7 @@ func (s *Service) SetPasswordHash(ctx context.Context, authID influxdb.ID, passH // SetPassword overrides the password of a known user. func (s *Service) SetPassword(ctx context.Context, authID influxdb.ID, password string) error { - if len(password) < 8 { + if len(password) < tenant.MinPasswordLen { return tenant.EShortPassword } passHash, err := encryptPassword(password)