Deprecate AddMFADevice and its top-level messages

api/client/client.go

@@ -1586,7 +1586,9 @@ func (c *Client) DeleteRole(ctx context.Context, name string) error {
 	return trace.Wrap(err)
 }
 
+// Deprecated: Use AddMFADeviceSync instead.
 func (c *Client) AddMFADevice(ctx context.Context) (proto.AuthService_AddMFADeviceClient, error) {
+	//nolint:staticcheck // SA1019. Kept for backward compatibility testing.
 	stream, err := c.grpc.AddMFADevice(ctx)
 	if err != nil {
 		return nil, trace.Wrap(err)

api/proto/teleport/legacy/client/proto/authservice.proto

@@ -1123,6 +1123,8 @@ message TOTPRegisterResponse {
 }
 
 // AddMFADeviceRequest is a message sent by the client during AddMFADevice RPC.
+//
+// Deprecated: Use [AuthService.AddMFADeviceSync] instead.
 message AddMFADeviceRequest {
   oneof Request {
     // Init describes the new device.
@@ -1138,6 +1140,8 @@ message AddMFADeviceRequest {
 
 // AddMFADeviceResponse is a message sent by the server during AddMFADevice
 // RPC.
+//
+// Deprecated: Use [AuthService.AddMFADeviceSync] instead.
 message AddMFADeviceResponse {
   oneof Response {
     // ExistingMFAChallenge is an auth challenge using an existing MFA
@@ -1152,6 +1156,8 @@ message AddMFADeviceResponse {
 }
 
 // AddMFADeviceRequestInit describes the new MFA device.
+//
+// Deprecated: Use [AuthService.AddMFADeviceSync] instead.
 message AddMFADeviceRequestInit {
   string DeviceName = 1;
   reserved 2; // LegacyDeviceType LegacyType
@@ -2617,7 +2623,11 @@ service AuthService {
   // <- NewMFARegisterChallenge
   // -> NewMFARegisterResponse
   // <- Ack
-  rpc AddMFADevice(stream AddMFADeviceRequest) returns (stream AddMFADeviceResponse);
+  //
+  // Deprecated: Use [AddMFADeviceSync] instead.
+  rpc AddMFADevice(stream AddMFADeviceRequest) returns (stream AddMFADeviceResponse) {
+    option deprecated = true;
+  }
   // DeleteMFADevice deletes an MFA device for the user calling this RPC.
   //
   // The RPC is streaming both ways and the message sequence is:
@@ -2627,7 +2637,14 @@ service AuthService {
   // -> MFAResponse
   // <- Ack
   rpc DeleteMFADevice(stream DeleteMFADeviceRequest) returns (stream DeleteMFADeviceResponse);
-  // AddMFADeviceSync adds a new MFA device (nonstream).
+  // AddMFADeviceSync adds a new MFA device.
+  //
+  // A typical MFA registration sequence calls the following RPCs:
+  //
+  // 1. CreateAuthenticateChallenge (necessary for registration challenge)
+  // 2. (optional) CreatePrivilegeToken
+  // 3. CreateRegisterChallenge (uses authn challenge and optionally a token)
+  // 4. AddMFADeviceSync
   rpc AddMFADeviceSync(AddMFADeviceSyncRequest) returns (AddMFADeviceSyncResponse);
   // DeleteMFADeviceSync deletes a users MFA device (nonstream).
   rpc DeleteMFADeviceSync(DeleteMFADeviceSyncRequest) returns (google.protobuf.Empty);

api/utils/grpc/interceptors/errors_test.go

@@ -81,9 +81,11 @@ func TestGRPCErrorWrapping(t *testing.T) {
 	})
 
 	t.Run("stream interceptor", func(t *testing.T) {
+		//nolint:staticcheck // SA1019. The specific stream used here doesn't matter.
 		stream, err := client.AddMFADevice(context.Background())
 		require.NoError(t, err)
 
+		//nolint:staticcheck // SA1019. The specific stream used here doesn't matter.
 		sendErr := stream.Send(&proto.AddMFADeviceRequest{})
 
 		// io.EOF means the server closed the stream, which can

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
 	}
 

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:

lib/auth/auth_test.go

@@ -2629,6 +2629,154 @@ func TestAddMFADeviceSync(t *testing.T) {
 	}
 }
 
+// DELETE IN 16. Kept so we don't lose coverage on AddMFADevice (codingllama)
+func TestAddMFADevice(t *testing.T) {
+	testServer := newTestTLSServer(t)
+	authServer := testServer.Auth()
+	clock := testServer.Auth().GetClock()
+
+	ctx := context.Background()
+
+	// Start with disabled second factor, makes it easy to set the user's
+	// password.
+	authPreference, err := types.NewAuthPreference(types.AuthPreferenceSpecV2{
+		Type:         constants.Local,
+		SecondFactor: constants.SecondFactorOff, // for initial user creation
+		Webauthn: &types.Webauthn{
+			RPID: "localhost",
+		},
+	})
+	require.NoError(t, err, "NewAuthPreference")
+	require.NoError(t,
+		authServer.SetAuthPreference(ctx, authPreference),
+		"SetAuthPreference")
+
+	// Create user and set password.
+	const username = "TestAddMFADevice"
+	const password = "supersecretpassword!!1!"
+	_, _, err = CreateUserAndRole(authServer, username, []string{username}, nil /* allowRules */)
+	require.NoError(t, err, "CreateUserAndRole")
+
+	resetToken, err := authServer.CreateResetPasswordToken(ctx, CreateUserTokenRequest{
+		Name: username,
+	})
+	require.NoError(t, err, "CreateResetPasswordToken")
+
+	_, err = authServer.ChangeUserAuthentication(ctx, &proto.ChangeUserAuthenticationRequest{
+		TokenID:     resetToken.GetName(),
+		NewPassword: []byte(password),
+	})
+	require.NoError(t, err, "ChangeUserAuthentication")
+
+	// Enable second factor.
+	authPreference.SetSecondFactor(constants.SecondFactorOptional)
+	require.NoError(t,
+		authServer.SetAuthPreference(ctx, authPreference),
+		"SetAuthPreference")
+
+	// User client used from now on.
+	userClient, err := testServer.NewClient(TestUser(username))
+	require.NoError(t, err, "NewClient")
+
+	tests := []struct {
+		name        string
+		deviceName  string
+		deviceType  proto.DeviceType
+		deviceUsage proto.DeviceUsage
+		devOpts     []TestDeviceOpt
+	}{
+		{
+			name:        "totp",
+			deviceName:  "totp",
+			deviceType:  proto.DeviceType_DEVICE_TYPE_TOTP,
+			deviceUsage: proto.DeviceUsage_DEVICE_USAGE_MFA,
+			devOpts:     []TestDeviceOpt{WithTestDeviceClock(clock)},
+		},
+		{
+			name:        "webauthn",
+			deviceName:  "webauthn",
+			deviceType:  proto.DeviceType_DEVICE_TYPE_WEBAUTHN,
+			deviceUsage: proto.DeviceUsage_DEVICE_USAGE_MFA,
+		},
+		{
+			name:        "passwordless",
+			deviceName:  "passwordless",
+			deviceType:  proto.DeviceType_DEVICE_TYPE_WEBAUTHN,
+			deviceUsage: proto.DeviceUsage_DEVICE_USAGE_PASSWORDLESS,
+			devOpts:     []TestDeviceOpt{WithPasswordless()},
+		},
+	}
+
+	var existingDev *TestDevice
+	for _, test := range tests {
+		//nolint:staticcheck // SA1019. Kept for backward compatibility testing.
+		t.Run(test.name, func(t *testing.T) {
+			stream, err := userClient.AddMFADevice(ctx)
+			require.NoError(t, err, "AddMFADevice")
+
+			// Init.
+			require.NoError(t,
+				stream.Send(&proto.AddMFADeviceRequest{
+					Request: &proto.AddMFADeviceRequest_Init{
+						Init: &proto.AddMFADeviceRequestInit{
+							DeviceName:  test.deviceName,
+							DeviceType:  test.deviceType,
+							DeviceUsage: test.deviceUsage,
+						},
+					},
+				}), "Send")
+			resp, err := stream.Recv()
+			require.NoError(t, err, "Recv: init")
+
+			// Solve existing device challenge.
+			// Reply with an empty solution if it's the first device.
+			authnChal := resp.GetExistingMFAChallenge()
+
+			var authnSolved *proto.MFAAuthenticateResponse
+			if hasChallenge := authnChal.GetTOTP() != nil || authnChal.GetWebauthnChallenge() != nil; hasChallenge {
+				// Sanity check.
+				require.NotNil(t, existingDev, "Got an existing challenge, but existingDev is nil")
+
+				authnSolved, err = existingDev.SolveAuthn(authnChal)
+				require.NoError(t, err, "SolveAuthn")
+			} else {
+				authnSolved = &proto.MFAAuthenticateResponse{} // empty reply
+			}
+
+			require.NoError(t,
+				stream.Send(&proto.AddMFADeviceRequest{
+					Request: &proto.AddMFADeviceRequest_ExistingMFAResponse{
+						ExistingMFAResponse: authnSolved,
+					},
+				}), "Send")
+			resp, err = stream.Recv()
+			require.NoError(t, err, "Recv: existing challenge response")
+
+			// Solve new device challenge.
+			registerChal := resp.GetNewMFARegisterChallenge()
+			require.NotNil(t, registerChal, "Got response of type %T, want MFARegisterChallenge", resp.Response)
+
+			newDev, registerSolved, err := NewTestDeviceFromChallenge(registerChal, test.devOpts...)
+			require.NoError(t, err, "NewTestDeviceFromChallenge")
+
+			require.NoError(t,
+				stream.Send(&proto.AddMFADeviceRequest{
+					Request: &proto.AddMFADeviceRequest_NewMFARegisterResponse{
+						NewMFARegisterResponse: registerSolved,
+					},
+				}), "Send")
+			resp, err = stream.Recv()
+			require.NoError(t, err, "Recv: register challenge response")
+			assert.NotNil(t, resp.GetAck().GetDevice(), "Got nil Ack or Ack.Device, want non-nil")
+
+			if existingDev == nil {
+				// Use registered device to solve future existing challenges.
+				existingDev = newDev
+			}
+		})
+	}
+}
+
 func TestGetMFADevices_WithToken(t *testing.T) {
 	t.Parallel()
 	srv := newTestTLSServer(t)

lib/auth/clt.go

@@ -591,7 +591,7 @@ type IdentityService interface {
 
 	// GetMFADevices fetches all MFA devices registered for the calling user.
 	GetMFADevices(ctx context.Context, in *proto.GetMFADevicesRequest) (*proto.GetMFADevicesResponse, error)
-	// AddMFADevice adds a new MFA device for the calling user.
+	// Deprecated: Use AddMFADeviceSync instead.
 	AddMFADevice(ctx context.Context) (proto.AuthService_AddMFADeviceClient, error)
 	// DeleteMFADevice deletes a MFA device for the calling user.
 	DeleteMFADevice(ctx context.Context) (proto.AuthService_DeleteMFADeviceClient, error)