diff --git a/lib/auth/init.go b/lib/auth/init.go index 627b77b70cd51..d0c8fce58f704 100644 --- a/lib/auth/init.go +++ b/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) diff --git a/lib/auth/init_test.go b/lib/auth/init_test.go index 33b5c722812ba..169b48eb56b8f 100644 --- a/lib/auth/init_test.go +++ b/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) { diff --git a/lib/config/configuration_test.go b/lib/config/configuration_test.go index a1a7f6f0807be..6e7457ffaadc1 100644 --- a/lib/config/configuration_test.go +++ b/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{ diff --git a/lib/config/fileconf_test.go b/lib/config/fileconf_test.go index 160e4bc93205e..b6cc274be0aa4 100644 --- a/lib/config/fileconf_test.go +++ b/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 diff --git a/lib/config/testdata_test.go b/lib/config/testdata_test.go index b2adc09d533c6..ba2d2d1bfdd62 100644 --- a/lib/config/testdata_test.go +++ b/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 diff --git a/lib/modules/modules_test.go b/lib/modules/modules_test.go index 4a53eea008c1c..6cb3580a02ed6 100644 --- a/lib/modules/modules_test.go +++ b/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) { diff --git a/lib/services/configuration.go b/lib/services/configuration.go index 6d8ea6eb9add7..5db6afc8fbab2 100644 --- a/lib/services/configuration.go +++ b/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) diff --git a/lib/services/configuration_test.go b/lib/services/configuration_test.go index b5f1baf90330a..b4a45e284fe59 100644 --- a/lib/services/configuration_test.go +++ b/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,6 +110,7 @@ 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)) @@ -116,4 +118,78 @@ func TestAuthPreferenceValidate(t *testing.T) { }) } }) + + 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)) + }) + } + }) } diff --git a/lib/services/local/configuration.go b/lib/services/local/configuration.go index 7c44a6704ad2b..598bc4b7be825 100644 --- a/lib/services/local/configuration.go +++ b/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 {