diff --git a/lib/auth/auth.go b/lib/auth/auth.go index b6351e50eedd5..ec6f40e3031e3 100644 --- a/lib/auth/auth.go +++ b/lib/auth/auth.go @@ -5420,16 +5420,30 @@ 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 */) - switch { - case err != nil: + if err != nil { return false, trace.Wrap(err) - case len(devices) > 0: + } + 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, but no devices registered. + // Allowed, no useable devices registered. return false, nil } diff --git a/lib/auth/auth_login_test.go b/lib/auth/auth_login_test.go index 65f9278c5dcd1..193dd42a3bf4b 100644 --- a/lib/auth/auth_login_test.go +++ b/lib/auth/auth_login_test.go @@ -16,6 +16,7 @@ package auth import ( "context" + "fmt" "sync/atomic" "testing" "time" @@ -431,6 +432,113 @@ func TestCreateRegisterChallenge(t *testing.T) { }) } +// TestCreateRegisterChallenge_unusableDevice tests that it is possible to +// register new devices even if the user has an "unusable" device (due to +// cluster setting changes). +func TestCreateRegisterChallenge_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: register challenges for test.existingType require a + // solved authn challenge. + _, err = userClient.CreateRegisterChallenge(ctx, &proto.CreateRegisterChallengeRequest{ + ExistingMFAResponse: &proto.MFAAuthenticateResponse{}, + DeviceType: test.existingType, + DeviceUsage: proto.DeviceUsage_DEVICE_USAGE_MFA, // not important for this test + }) + 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 challenge for the "new" device without an ExistingMFAResponse. + // Not allowed if the device above was usable. + _, err = userClient.CreateRegisterChallenge(ctx, &proto.CreateRegisterChallengeRequest{ + ExistingMFAResponse: &proto.MFAAuthenticateResponse{}, + DeviceType: test.newType, + DeviceUsage: proto.DeviceUsage_DEVICE_USAGE_MFA, // not important for this test + }) + assert.NoError(t, err, "CreateRegisterChallenge") + }) + } +} + // sshPubKey is a randomly-generated public key used for login tests. // // The corresponding private key is: