fix(onboarding): don't ignore failures to set initial password (#20317)

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]
 ----------------------

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

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,

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),
 	}
 )
 

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)

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

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)
+}

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)

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)