[v17] Move auth preference module validation to RPC layer

lib/auth/init.go

@@ -26,9 +26,7 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
-	"os"
 	"slices"
-	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -883,15 +881,14 @@ func initializeAuthPreference(ctx context.Context, asrv *Server, newAuthPref typ
 		}
 
 		if !shouldReplace {
-			if allowNoSecondFactor, _ := strconv.ParseBool(os.Getenv(teleport.EnvVarAllowNoSecondFactor)); allowNoSecondFactor {
-				err := modules.ValidateResource(storedAuthPref)
+			if err := modules.ValidateResource(storedAuthPref); err != nil {
 				if errors.Is(err, modules.ErrCannotDisableSecondFactor) {
 					return trace.Wrap(err, secondFactorUpgradeInstructions)
 				}
-				if err != nil {
-					return trace.Wrap(err)
-				}
+
+				return trace.Wrap(err)
 			}
+
 			return nil
 		}
 
@@ -921,6 +918,14 @@ func initializeAuthPreference(ctx context.Context, asrv *Server, newAuthPref typ
 			newAuthPref.SetSignatureAlgorithmSuite(storedAuthPref.GetSignatureAlgorithmSuite())
 		}
 
+		if err := modules.ValidateResource(newAuthPref); err != nil {
+			if errors.Is(err, modules.ErrCannotDisableSecondFactor) {
+				return trace.Wrap(err, secondFactorUpgradeInstructions)
+			}
+
+			return trace.Wrap(err)
+		}
+
 		if storedAuthPref == nil {
 			log.Infof("Creating cluster auth preference: %v.", newAuthPref)
 			_, err := asrv.CreateAuthPreference(ctx, newAuthPref)

lib/auth/init_test.go

@@ -589,29 +589,15 @@ func TestAuthPreferenceSecondFactorOnly(t *testing.T) {
 	defer modules.SetInsecureTestMode(true)
 	ctx := context.Background()
 
-	t.Run("starting with second_factor disabled fails", func(t *testing.T) {
-		conf := setupConfig(t)
-		authPref, err := types.NewAuthPreferenceFromConfigFile(types.AuthPreferenceSpecV2{
-			SecondFactor: constants.SecondFactorOff,
-		})
-		require.NoError(t, err)
-
-		conf.AuthPreference = authPref
-		_, err = Init(ctx, conf)
-		require.Error(t, err)
+	conf := setupConfig(t)
+	authPref, err := types.NewAuthPreferenceFromConfigFile(types.AuthPreferenceSpecV2{
+		SecondFactor: constants.SecondFactorOff,
 	})
+	require.NoError(t, err)
 
-	t.Run("starting with defaults and dynamically updating to disable second factor fails", func(t *testing.T) {
-		conf := setupConfig(t)
-		s, err := Init(ctx, conf)
-		require.NoError(t, err)
-		authpref, err := types.NewAuthPreference(types.AuthPreferenceSpecV2{
-			SecondFactor: constants.SecondFactorOff,
-		})
-		require.NoError(t, err)
-		_, err = s.UpsertAuthPreference(ctx, authpref)
-		require.Error(t, err)
-	})
+	conf.AuthPreference = authPref
+	_, err = Init(ctx, conf)
+	require.Error(t, err)
 }
 
 func TestClusterNetworkingConfig(t *testing.T) {

lib/config/configuration_test.go

@@ -796,7 +796,7 @@ func TestApplyConfig(t *testing.T) {
 		},
 		Spec: types.AuthPreferenceSpecV2{
 			Type:         constants.Local,
-			SecondFactor: constants.SecondFactorOptional,
+			SecondFactor: constants.SecondFactorWebauthn,
 			Webauthn: &types.Webauthn{
 				RPID: "goteleport.com",
 				AttestationAllowedCAs: []string{

lib/config/fileconf_test.go

@@ -754,7 +754,7 @@ func TestAuthenticationConfig_Parse_deviceTrustPB(t *testing.T) {
 			configYAML: editConfig(t, func(cfg cfgMap) {
 				cfg["auth_service"].(cfgMap)["authentication"] = cfgMap{
 					"type":          "local",
-					"second_factor": "off", // uncharacteristic, but not necessary for this test
+					"second_factor": "otp",
 					"device_trust": cfgMap{
 						"mode": "optional",
 					},
@@ -769,7 +769,7 @@ func TestAuthenticationConfig_Parse_deviceTrustPB(t *testing.T) {
 			configYAML: editConfig(t, func(cfg cfgMap) {
 				cfg["auth_service"].(cfgMap)["authentication"] = cfgMap{
 					"type":          "local",
-					"second_factor": "off", // uncharacteristic, but not necessary for this test
+					"second_factor": "otp",
 					"device_trust": cfgMap{
 						"mode":        "required",
 						"auto_enroll": "yes",
@@ -823,7 +823,7 @@ func TestAuthenticationConfig_Parse_deviceTrustPB(t *testing.T) {
 			configYAML: editConfig(t, func(cfg cfgMap) {
 				cfg["auth_service"].(cfgMap)["authentication"] = cfgMap{
 					"type":          "local",
-					"second_factor": "off", // uncharacteristic, but not necessary for this test
+					"second_factor": "otp",
 					"device_trust": cfgMap{
 						"mode":        "required",
 						"auto_enroll": "NOT A BOOLEAN", // invalid

lib/config/testdata_test.go

@@ -125,7 +125,7 @@ auth_service:
       slot_number: 1
       pin: "example_pin"
   authentication:
-    second_factor: "optional"
+    second_factor: "webauthn"
     webauthn:
       rp_id: "goteleport.com"
       attestation_allowed_cas:
@@ -346,7 +346,7 @@ auth_service:
   - "auth:yyy"
   authentication:
     type: local
-    second_factor: off
+    second_factors: [otp]
 
 ssh_service:
   enabled: no

lib/modules/modules_test.go

@@ -58,12 +58,36 @@ func TestValidateAuthPreferenceOnCloud(t *testing.T) {
 		},
 	})
 
-	authPref, err := testServer.AuthServer.UpsertAuthPreference(ctx, types.DefaultAuthPreference())
+	s, err := auth.NewTestTLSServer(auth.TestTLSServerConfig{
+		APIConfig: &auth.APIConfig{
+			AuthServer: testServer.AuthServer,
+			Authorizer: testServer.Authorizer,
+			AuditLog:   testServer.AuditLog,
+			Emitter:    testServer.AuditLog,
+		},
+		AuthServer: testServer,
+	})
+	require.NoError(t, err)
+
+	clt, err := s.NewClient(auth.TestAdmin())
+	require.NoError(t, err)
+
+	ap := types.DefaultAuthPreference()
+	ap.SetSignatureAlgorithmSuite(types.SignatureAlgorithmSuite_SIGNATURE_ALGORITHM_SUITE_LEGACY)
+	authPref, err := clt.UpsertAuthPreference(ctx, ap)
 	require.NoError(t, err)
 
 	authPref.SetSecondFactor(constants.SecondFactorOff)
-	_, err = testServer.AuthServer.UpdateAuthPreference(ctx, authPref)
-	require.EqualError(t, err, modules.ErrCannotDisableSecondFactor.Error())
+	_, err = clt.UpdateAuthPreference(ctx, authPref)
+	require.ErrorContains(t, err, "rpc error: code = Unknown desc = cannot disable multi-factor authentication")
+
+	// Validate the storage layer doesn't enforce module validation.
+	authPref, err = testServer.AuthServer.UpdateAuthPreference(ctx, authPref)
+	require.NoError(t, err)
+	require.False(t, authPref.IsSecondFactorEnabled())
+	authPref, err = testServer.AuthServer.UpsertAuthPreference(ctx, authPref)
+	require.NoError(t, err)
+	require.False(t, authPref.IsSecondFactorEnabled())
 }
 
 func TestValidateSessionRecordingConfigOnCloud(t *testing.T) {

lib/services/configuration.go

@@ -26,6 +26,7 @@ import (
 	clusterconfigpb "github.com/gravitational/teleport/api/gen/proto/go/teleport/clusterconfig/v1"
 	"github.com/gravitational/teleport/api/types"
 	"github.com/gravitational/teleport/lib/backend"
+	"github.com/gravitational/teleport/lib/modules"
 )
 
 // ClusterConfiguration stores the cluster configuration in the backend. All
@@ -152,6 +153,9 @@ type ClusterConfigurationInternal interface {
 func ValidateAuthPreference(ap types.AuthPreference) error {
 	// TODO(espadolini): the checks that are duplicated in
 	// {Set,Create,Update,Upsert}AuthPreference should be moved here
+	if err := modules.ValidateResource(ap); err != nil {
+		return trace.Wrap(err)
+	}
 
 	if err := ValidateStableUNIXUserConfig(ap.GetStableUNIXUserConfig()); err != nil {
 		return trace.Wrap(err)

lib/services/configuration_test.go

@@ -21,12 +21,13 @@ import (
 
 	"github.com/stretchr/testify/require"
 
+	"github.com/gravitational/teleport"
+	"github.com/gravitational/teleport/api/constants"
 	"github.com/gravitational/teleport/api/types"
+	"github.com/gravitational/teleport/lib/modules"
 )
 
 func TestAuthPreferenceValidate(t *testing.T) {
-	t.Parallel()
-
 	t.Run("default", func(t *testing.T) {
 		t.Parallel()
 
@@ -109,11 +110,86 @@ func TestAuthPreferenceValidate(t *testing.T) {
 				authPref := &types.AuthPreferenceV2{
 					Spec: types.AuthPreferenceSpecV2{
 						StableUnixUserConfig: tc.config,
+						SecondFactors:        []types.SecondFactorType{types.SecondFactorType_SECOND_FACTOR_TYPE_OTP},
 					},
 				}
 				tc.check(t, ValidateAuthPreference(authPref))
 				tc.check(t, ValidateStableUNIXUserConfig(authPref.Spec.StableUnixUserConfig))
 			})
 		}
 	})
+
+	disableSecondFactorAssertion := func(t require.TestingT, err error, a ...any) {
+		require.ErrorIs(t, err, modules.ErrCannotDisableSecondFactor, a...)
+	}
+	t.Run("second_factors", func(t *testing.T) {
+		type testCase struct {
+			name     string
+			spec     types.AuthPreferenceSpecV2
+			features modules.Features
+			bypass   bool
+			check    require.ErrorAssertionFunc
+		}
+
+		testCases := []testCase{
+			{
+				name:  "disabling prevented",
+				check: disableSecondFactorAssertion,
+			},
+			{
+				name:   "cloud prevents disabling",
+				bypass: true,
+				features: modules.Features{
+					Cloud: true,
+				},
+				check: disableSecondFactorAssertion,
+			},
+			{
+				name: "webauthn allowed",
+				spec: types.AuthPreferenceSpecV2{
+					SecondFactor: constants.SecondFactorWebauthn,
+					Webauthn: &types.Webauthn{
+						RPID: "test.example.com",
+					},
+				},
+				check: require.NoError,
+			},
+			{
+				name: "otp allowed",
+				spec: types.AuthPreferenceSpecV2{
+					SecondFactor: constants.SecondFactorOTP,
+				},
+				check: require.NoError,
+			},
+			{
+				name: "sso and webauthn allowed",
+				spec: types.AuthPreferenceSpecV2{
+					SecondFactors: []types.SecondFactorType{types.SecondFactorType_SECOND_FACTOR_TYPE_SSO, types.SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN},
+				},
+				check: require.NoError,
+			},
+			{
+				name:   "bypass self hosted",
+				check:  require.NoError,
+				bypass: true,
+			},
+		}
+
+		for _, tc := range testCases {
+			t.Run(tc.name, func(t *testing.T) {
+				if tc.bypass {
+					t.Setenv(teleport.EnvVarAllowNoSecondFactor, "true")
+				}
+
+				modules.SetTestModules(t, &modules.TestModules{
+					TestFeatures: tc.features,
+				})
+
+				authPref := &types.AuthPreferenceV2{
+					Spec: tc.spec,
+				}
+				tc.check(t, ValidateAuthPreference(authPref))
+			})
+		}
+	})
 }

lib/services/local/configuration.go

@@ -192,11 +192,6 @@ func (s *ClusterConfigurationService) GetAuthPreference(ctx context.Context) (ty
 
 // CreateAuthPreference creates an auth preference if once does not already exist.
 func (s *ClusterConfigurationService) CreateAuthPreference(ctx context.Context, preference types.AuthPreference) (types.AuthPreference, error) {
-	// Perform the modules-provided checks.
-	if err := modules.ValidateResource(preference); err != nil {
-		return nil, trace.Wrap(err)
-	}
-
 	value, err := services.MarshalAuthPreference(preference)
 	if err != nil {
 		return nil, trace.Wrap(err)
@@ -218,12 +213,7 @@ func (s *ClusterConfigurationService) CreateAuthPreference(ctx context.Context,
 
 // UpdateAuthPreference updates an existing auth preference.
 func (s *ClusterConfigurationService) UpdateAuthPreference(ctx context.Context, preference types.AuthPreference) (types.AuthPreference, error) {
-	// Perform the modules-provided checks.
 	rev := preference.GetRevision()
-	if err := modules.ValidateResource(preference); err != nil {
-		return nil, trace.Wrap(err)
-	}
-
 	value, err := services.MarshalAuthPreference(preference)
 	if err != nil {
 		return nil, trace.Wrap(err)
@@ -246,11 +236,6 @@ func (s *ClusterConfigurationService) UpdateAuthPreference(ctx context.Context,
 
 // UpsertAuthPreference creates a new auth preference or overwrites an existing auth preference.
 func (s *ClusterConfigurationService) UpsertAuthPreference(ctx context.Context, preference types.AuthPreference) (types.AuthPreference, error) {
-	// Perform the modules-provided checks.
-	if err := modules.ValidateResource(preference); err != nil {
-		return nil, trace.Wrap(err)
-	}
-
 	rev := preference.GetRevision()
 	value, err := services.MarshalAuthPreference(preference)
 	if err != nil {