From 5dcc45e709fa5b5693d9460556ff424d6a53a19d Mon Sep 17 00:00:00 2001 From: Tim Ross Date: Fri, 9 May 2025 15:53:27 -0400 Subject: [PATCH] Move auth preference module validation to RPC layer The module validation rejects auth preferences that have second factor disabled without the environment variable override. Doing this in the storage layer means that in order to disable second factor the environment variable needs to be set on _all_ teleport processes not just Auth. This can result in caches of downstream agents from becoming healthy until the manual override is applied. The intent is to prevent modifying an the auth preference to disable second factor, which when moved to the RPC layer, has the same affect without the possibility of caches performing extra validation. --- lib/auth/init.go | 19 ++++--- lib/auth/init_test.go | 28 +++------- lib/config/configuration_test.go | 2 +- lib/config/fileconf_test.go | 6 +-- lib/config/testdata_test.go | 4 +- lib/modules/modules_test.go | 30 +++++++++-- lib/services/configuration.go | 4 ++ lib/services/configuration_test.go | 80 ++++++++++++++++++++++++++++- lib/services/local/configuration.go | 15 ------ 9 files changed, 134 insertions(+), 54 deletions(-) diff --git a/lib/auth/init.go b/lib/auth/init.go index b92db080fc92b..6c4ab2294bd60 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" @@ -898,15 +896,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 } @@ -936,6 +933,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 { asrv.logger.InfoContext(ctx, "Creating cluster auth preference", "auth_preference", newAuthPref) _, err := asrv.CreateAuthPreference(ctx, newAuthPref) diff --git a/lib/auth/init_test.go b/lib/auth/init_test.go index 46ef316bc045c..8082d62bda8bd 100644 --- a/lib/auth/init_test.go +++ b/lib/auth/init_test.go @@ -610,29 +610,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 0b4020a4667d3..45f1ee9c74b46 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 75320c8e871de..18ffe1bc78cab 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 7c6e1eb7f9ca8..e020401728a44 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" ) // ClusterNameGetter is a service that gets the cluster name from the backend. @@ -157,6 +158,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 54a076f049a4b..472a44e56e1a5 100644 --- a/lib/services/local/configuration.go +++ b/lib/services/local/configuration.go @@ -191,11 +191,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) @@ -217,12 +212,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) @@ -245,11 +235,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 {