diff --git a/lib/auth/auth.go b/lib/auth/auth.go index ea062eefc58c4..541cda6bc8312 100644 --- a/lib/auth/auth.go +++ b/lib/auth/auth.go @@ -5319,6 +5319,54 @@ func groupByDeviceType(devs []*types.MFADevice, groupWebauthn bool) devicesByTyp return res } +// validateMFAAuthResponseForRegister is akin to [validateMFAAuthResponse], but +// it allows users with no devices to supply a nil/empty response. +// +// The hasDevices response value can only be trusted in the absence of errors. +// +// Use only for registration purposes. +func (a *Server) validateMFAAuthResponseForRegister( + ctx context.Context, + resp *proto.MFAAuthenticateResponse, username string, passwordless bool, +) (hasDevices bool, err error) { + // Let users without a useable device go through registration. + if resp == nil || (resp.GetTOTP() == nil && resp.GetWebauthn() == nil) { + devices, err := a.Services.GetMFADevices(ctx, username, false /* withSecrets */) + if err != nil { + return false, trace.Wrap(err) + } + if len(devices) == 0 { + // Allowed, no devices registered. + return false, nil + } + + authPref, err := a.GetAuthPreference(ctx) + if err != nil { + return false, trace.Wrap(err) + } + totpEnabled := authPref.IsSecondFactorTOTPAllowed() + webauthnEnabled := authPref.IsSecondFactorWebauthnAllowed() + + devsByType := groupByDeviceType(devices, webauthnEnabled) + if (totpEnabled && devsByType.TOTP) || (webauthnEnabled && len(devsByType.Webauthn) > 0) { + return false, trace.BadParameter("second factor authentication required") + } + + // Allowed, no useable devices registered. + return false, nil + } + + if err := a.WithUserLock(username, func() error { + _, _, err := a.validateMFAAuthResponse( + ctx, resp, username, false /* passwordless */) + return err + }); err != nil { + return false, trace.Wrap(err) + } + + return true, nil +} + // validateMFAAuthResponse validates an MFA or passwordless challenge. // Returns the device used to solve the challenge (if applicable) and the // username. diff --git a/lib/auth/usertoken.go b/lib/auth/usertoken.go index ee47d5bc8a78e..42bcbb2e14385 100644 --- a/lib/auth/usertoken.go +++ b/lib/auth/usertoken.go @@ -463,27 +463,12 @@ func (s *Server) CreatePrivilegeToken(ctx context.Context, req *proto.CreatePriv tokenKind := UserTokenTypePrivilege - switch { - case req.GetExistingMFAResponse() == nil: - // Allows users with no devices to bypass second factor re-auth. - devices, err := s.Services.GetMFADevices(ctx, username, false /* withSecrets */) - switch { - case err != nil: - return nil, trace.Wrap(err) - case len(devices) > 0: - return nil, trace.BadParameter("second factor authentication required") - } - + switch hasDevices, err := s.validateMFAAuthResponseForRegister( + ctx, req.GetExistingMFAResponse(), username, false /* passwordless */); { + case err != nil: + return nil, trace.Wrap(err) + case !hasDevices: tokenKind = UserTokenTypePrivilegeException - - default: - if err := s.WithUserLock(username, func() error { - _, _, err := s.validateMFAAuthResponse( - ctx, req.GetExistingMFAResponse(), username, false /* passwordless */) - return err - }); err != nil { - return nil, trace.Wrap(err) - } } // Delete any existing user tokens for user before creating. diff --git a/lib/auth/usertoken_test.go b/lib/auth/usertoken_test.go index 419d396ead386..dae47b66eaabb 100644 --- a/lib/auth/usertoken_test.go +++ b/lib/auth/usertoken_test.go @@ -27,6 +27,7 @@ import ( "github.com/gravitational/trace" "github.com/jonboulle/clockwork" "github.com/pquerna/otp/totp" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/gravitational/teleport" @@ -441,6 +442,110 @@ func TestCreatePrivilegeToken_WithLock(t *testing.T) { } } +// TestCreatePrivilegeToken_unusableDevice tests that it is possible to +// register new devices even if the user has an "unusable" device (due to +// cluster setting changes). +func TestCreatePrivilegeToken_unusableDevice(t *testing.T) { + t.Parallel() + + testServer := newTestTLSServer(t) + authServer := testServer.Auth() + clock := authServer.GetClock() + ctx := context.Background() + + initialPref, err := types.NewAuthPreference(types.AuthPreferenceSpecV2{ + Type: constants.Local, + SecondFactor: constants.SecondFactorOptional, // most permissive setting + Webauthn: &types.Webauthn{ + RPID: "localhost", + }, + }) + require.NoError(t, err, "NewAuthPreference") + + setAuthPref := func(t *testing.T, authPref types.AuthPreference) { + require.NoError(t, + authServer.SetAuthPreference(ctx, authPref), + "SetAuthPreference") + } + setAuthPref(t, initialPref) + + tests := []struct { + name string + existingType, newType proto.DeviceType + newAuthSpec types.AuthPreferenceSpecV2 + }{ + { + name: "unusable totp, new webauthn", + existingType: proto.DeviceType_DEVICE_TYPE_TOTP, + newType: proto.DeviceType_DEVICE_TYPE_WEBAUTHN, + newAuthSpec: types.AuthPreferenceSpecV2{ + Type: initialPref.GetType(), + SecondFactor: constants.SecondFactorWebauthn, // makes TOTP unusable + Webauthn: func() *types.Webauthn { + w, _ := initialPref.GetWebauthn() + return w + }(), + }, + }, + { + name: "unusable webauthn, new totp", + existingType: proto.DeviceType_DEVICE_TYPE_WEBAUTHN, + newType: proto.DeviceType_DEVICE_TYPE_TOTP, + newAuthSpec: types.AuthPreferenceSpecV2{ + Type: initialPref.GetType(), + SecondFactor: constants.SecondFactorOTP, // makes Webauthn unusable + }, + }, + } + + devOpts := []TestDeviceOpt{WithTestDeviceClock(clock)} + for i, test := range tests { + t.Run(test.name, func(t *testing.T) { + setAuthPref(t, initialPref) // restore permissive settings. + + // Create user. + username := fmt.Sprintf("llama-%d", i) + user, _, err := CreateUserAndRole(authServer, username, []string{username} /* logins */, nil /* allowRules */) + require.NoError(t, err, "CreateUserAndRole") + userClient, err := testServer.NewClient(TestUser(user.GetName())) + require.NoError(t, err, "NewClient") + + // Register initial MFA device. + _, err = RegisterTestDevice( + ctx, + userClient, + "existing", test.existingType, nil /* authenticator */, devOpts...) + require.NoError(t, err, "RegisterTestDevice") + + // Sanity check: privilege tokens for test.existingType require a solved + // authn challenge. + _, err = userClient.CreatePrivilegeToken(ctx, &proto.CreatePrivilegeTokenRequest{ + ExistingMFAResponse: &proto.MFAAuthenticateResponse{}, + }) + assert.ErrorContains(t, err, "second factor") + + // Restore initial settings after test. + defer func() { + setAuthPref(t, initialPref) + }() + + // Change cluster settings. + // This should make the device registered above unusable. + newAuthPref, err := types.NewAuthPreference(test.newAuthSpec) + require.NoError(t, err, "NewAuthPreference") + setAuthPref(t, newAuthPref) + + // Create a privilege token for the "new" device without an + // ExistingMFAResponse. + // Not allowed if the device above was usable. + _, err = userClient.CreatePrivilegeToken(ctx, &proto.CreatePrivilegeTokenRequest{ + ExistingMFAResponse: &proto.MFAAuthenticateResponse{}, + }) + assert.NoError(t, err, "CreatePrivilegeToken") + }) + } +} + type debugAuth struct { proxies []types.Server proxiesError bool