From 07c1201277323035ea6979aab865d49e6fc66496 Mon Sep 17 00:00:00 2001 From: joerger Date: Thu, 10 Oct 2024 11:23:28 -0700 Subject: [PATCH] Address comments. --- api/constants/constants.go | 3 +- api/types/authentication.go | 16 ++++++-- api/types/authentication_test.go | 64 +++++++++++++++++++++++--------- api/types/second_factor.go | 2 +- api/types/second_factor_test.go | 49 +++++++++++++++++++++--- 5 files changed, 103 insertions(+), 31 deletions(-) diff --git a/api/constants/constants.go b/api/constants/constants.go index aea6d8a3b8fb2..9f66302da597d 100644 --- a/api/constants/constants.go +++ b/api/constants/constants.go @@ -208,8 +208,7 @@ const ( // is required for all users. SecondFactorWebauthn = SecondFactorType("webauthn") // SecondFactorOn means that all 2FA protocols are supported and 2FA is - // required for all users, or that SSO MFA is enabled which cannot be - // expressed through a different SecondFactorType. + // required for all users. SecondFactorOn = SecondFactorType("on") // SecondFactorOptional means that all 2FA protocols are supported and 2FA // is required only for users that have MFA devices registered. diff --git a/api/types/authentication.go b/api/types/authentication.go index e91f9064dba14..485af7c086dca 100644 --- a/api/types/authentication.go +++ b/api/types/authentication.go @@ -87,6 +87,8 @@ type AuthPreference interface { IsSecondFactorEnabled() bool // IsSecondFactorEnforced checks if second factor is enforced. IsSecondFactorEnforced() bool + // IsSecondFactorLocalAllowed checks if a local second factor method is enabled (webauthn, totp). + IsSecondFactorLocalAllowed() bool // IsSecondFactorTOTPAllowed checks if users can use TOTP as an MFA method. IsSecondFactorTOTPAllowed() bool // IsSecondFactorWebauthnAllowed checks if users can use WebAuthn as an MFA method. @@ -384,6 +386,11 @@ func (c *AuthPreferenceV2) IsSecondFactorEnforced() bool { return len(c.GetSecondFactors()) > 0 && c.Spec.SecondFactor != constants.SecondFactorOptional } +// IsSecondFactorLocalAllowed checks if a local second factor method is enabled. +func (c *AuthPreferenceV2) IsSecondFactorLocalAllowed() bool { + return c.IsSecondFactorTOTPAllowed() || c.IsSecondFactorWebauthnAllowed() +} + // IsSecondFactorTOTPAllowed checks if users can use TOTP as an MFA method. func (c *AuthPreferenceV2) IsSecondFactorTOTPAllowed() bool { return slices.Contains(c.GetSecondFactors(), SecondFactorType_SECOND_FACTOR_TYPE_OTP) @@ -776,10 +783,11 @@ func (c *AuthPreferenceV2) CheckAndSetDefaults() error { } // Prevent local lockout by disabling local second factor methods. - hasSSO := c.IsSecondFactorSSOAllowed() - hasTOTP := c.IsSecondFactorTOTPAllowed() - if c.GetAllowLocalAuth() && hasSSO && !hasTOTP && !hasWebauthn { - return trace.BadParameter("missing a local second factor method for local users (otp, webauthn), either add a local second factor method or disable local auth") + if c.GetAllowLocalAuth() && c.IsSecondFactorEnforced() && !c.IsSecondFactorLocalAllowed() { + if c.IsSecondFactorSSOAllowed() { + trace.BadParameter("missing a local second factor method for local users (otp, webauthn), either add a local second factor method or disable local auth") + } + return trace.BadParameter("missing a local second factor method for local users (otp, webauthn)") } // Validate connector name for type=local. diff --git a/api/types/authentication_test.go b/api/types/authentication_test.go index 3d97b57527098..c33510d5de33e 100644 --- a/api/types/authentication_test.go +++ b/api/types/authentication_test.go @@ -19,6 +19,7 @@ package types import ( "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/gravitational/trace" @@ -69,7 +70,7 @@ func TestEncodeDecodeRequireMFAType(t *testing.T) { } } -func TestAuthPreference_CheckAndSetDefaults(t *testing.T) { +func TestNewAuthPreference_secondFactors(t *testing.T) { for _, tt := range []struct { name string spec AuthPreferenceSpecV2 @@ -80,23 +81,25 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) { name: "OK default to OTP", spec: AuthPreferenceSpecV2{}, assertAuthPref: func(t *testing.T, authPref AuthPreference) { - require.Equal(t, []SecondFactorType{SecondFactorType_SECOND_FACTOR_TYPE_OTP}, authPref.GetSecondFactors()) + assert.Equal(t, []SecondFactorType{SecondFactorType_SECOND_FACTOR_TYPE_OTP}, authPref.GetSecondFactors()) }, }, { - name: "OK OTP", + name: "OK OTP default settings", spec: AuthPreferenceSpecV2{ SecondFactors: []SecondFactorType{ SecondFactorType_SECOND_FACTOR_TYPE_OTP, }, }, assertAuthPref: func(t *testing.T, authPref AuthPreference) { - require.False(t, authPref.GetAllowPasswordless()) - require.False(t, authPref.GetAllowHeadless()) + assert.False(t, authPref.GetAllowPasswordless()) + assert.False(t, authPref.GetAllowHeadless()) + assert.True(t, authPref.GetAllowLocalAuth()) + assert.False(t, authPref.IsAdminActionMFAEnforced()) }, }, { - name: "OK WebAuthn", + name: "OK WebAuthn default settings", spec: AuthPreferenceSpecV2{ SecondFactors: []SecondFactorType{ SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN, @@ -106,12 +109,14 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) { }, }, assertAuthPref: func(t *testing.T, authPref AuthPreference) { - require.True(t, authPref.GetAllowPasswordless()) - require.True(t, authPref.GetAllowHeadless()) + assert.True(t, authPref.GetAllowPasswordless()) + assert.True(t, authPref.GetAllowHeadless()) + assert.True(t, authPref.GetAllowLocalAuth()) + assert.True(t, authPref.IsAdminActionMFAEnforced()) }, }, { - name: "OK SSO", + name: "OK SSO default settings", spec: AuthPreferenceSpecV2{ SecondFactors: []SecondFactorType{ SecondFactorType_SECOND_FACTOR_TYPE_SSO, @@ -119,9 +124,27 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) { AllowLocalAuth: NewBoolOption(false), }, assertAuthPref: func(t *testing.T, authPref AuthPreference) { - require.False(t, authPref.GetAllowPasswordless()) - require.False(t, authPref.GetAllowHeadless()) - require.False(t, authPref.GetAllowLocalAuth()) + assert.False(t, authPref.GetAllowPasswordless()) + assert.False(t, authPref.GetAllowHeadless()) + assert.False(t, authPref.GetAllowLocalAuth()) + assert.True(t, authPref.IsAdminActionMFAEnforced()) + }, + }, + { + name: "OK all second factors", + spec: AuthPreferenceSpecV2{ + SecondFactors: []SecondFactorType{ + SecondFactorType_SECOND_FACTOR_TYPE_OTP, + SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN, + SecondFactorType_SECOND_FACTOR_TYPE_SSO, + }, + }, + assertAuthPref: func(t *testing.T, authPref AuthPreference) { + assert.True(t, authPref.GetAllowPasswordless()) + assert.True(t, authPref.GetAllowHeadless()) + assert.True(t, authPref.GetAllowLocalAuth()) + // enabling OTP disables admin mfa. + assert.False(t, authPref.IsAdminActionMFAEnforced()) }, }, { @@ -134,6 +157,11 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) { AppID: "https://localhost", }, }, + assertAuthPref: func(t *testing.T, authPref AuthPreference) { + w, err := authPref.GetWebauthn() + assert.NoError(t, err) + assert.Equal(t, &Webauthn{RPID: "localhost"}, w) + }, }, { name: "NOK SecondFactor and SecondFactors both set", @@ -144,7 +172,7 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) { }, }, assertErr: func(t require.TestingT, err error, vals ...interface{}) { - require.ErrorAs(t, err, &trace.BadParameterError{}) + assert.ErrorAs(t, err, new(*trace.BadParameterError)) }, }, { @@ -155,7 +183,7 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) { }, }, assertErr: func(t require.TestingT, err error, vals ...interface{}) { - require.ErrorAs(t, err, &trace.BadParameterError{}) + assert.ErrorAs(t, err, new(*trace.BadParameterError)) }, }, { @@ -167,7 +195,7 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) { AllowPasswordless: NewBoolOption(true), }, assertErr: func(t require.TestingT, err error, vals ...interface{}) { - require.ErrorAs(t, err, &trace.BadParameterError{}) + assert.ErrorAs(t, err, new(*trace.BadParameterError)) }, }, { @@ -179,7 +207,7 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) { AllowHeadless: NewBoolOption(true), }, assertErr: func(t require.TestingT, err error, vals ...interface{}) { - require.ErrorAs(t, err, &trace.BadParameterError{}) + assert.ErrorAs(t, err, new(*trace.BadParameterError)) }, }, { @@ -191,7 +219,7 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) { AllowLocalAuth: NewBoolOption(true), }, assertErr: func(t require.TestingT, err error, vals ...interface{}) { - require.ErrorAs(t, err, &trace.BadParameterError{}) + assert.ErrorAs(t, err, new(*trace.BadParameterError)) }, }, } { @@ -200,7 +228,7 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) { if tt.assertErr != nil { tt.assertErr(t, err) } else { - require.NoError(t, err) + assert.NoError(t, err) } if tt.assertAuthPref != nil { diff --git a/api/types/second_factor.go b/api/types/second_factor.go index 9bef1a73549ef..d876a35493c83 100644 --- a/api/types/second_factor.go +++ b/api/types/second_factor.go @@ -31,7 +31,7 @@ func legacySecondFactorFromSecondFactors(secondFactors []SecondFactorType) const hasWebAuthn := slices.Contains(secondFactors, SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN) switch { - case (hasOTP && hasWebAuthn): + case hasOTP && hasWebAuthn: return constants.SecondFactorOn case hasWebAuthn: return constants.SecondFactorWebauthn diff --git a/api/types/second_factor_test.go b/api/types/second_factor_test.go index ebff27f67e524..17ed27dc8b35d 100644 --- a/api/types/second_factor_test.go +++ b/api/types/second_factor_test.go @@ -19,10 +19,11 @@ package types import ( "testing" - "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" + + "github.com/gravitational/teleport/api/constants" ) -// TestEncodeDecodeSecondFactorType tests encoding/decoding of the SecondFactorType. func TestEncodeDecodeSecondFactorType(t *testing.T) { for _, tt := range []struct { secondFactorType SecondFactorType @@ -42,16 +43,52 @@ func TestEncodeDecodeSecondFactorType(t *testing.T) { t.Run(tt.secondFactorType.String(), func(t *testing.T) { t.Run("encode", func(t *testing.T) { encoded, err := tt.secondFactorType.encode() - require.NoError(t, err) - require.Equal(t, tt.encoded, encoded) + assert.NoError(t, err) + assert.Equal(t, tt.encoded, encoded) }) t.Run("decode", func(t *testing.T) { var decoded SecondFactorType err := decoded.decode(tt.encoded) - require.NoError(t, err) - require.Equal(t, tt.secondFactorType, decoded) + assert.NoError(t, err) + assert.Equal(t, tt.secondFactorType, decoded) }) }) } } + +func TestLegacySecondFactorsFromLegacySecondFactor(t *testing.T) { + for _, tt := range []struct { + sf constants.SecondFactorType + sfs []SecondFactorType + }{ + { + sf: "", + sfs: nil, + }, + { + sf: constants.SecondFactorOff, + sfs: nil, + }, + { + sf: constants.SecondFactorOptional, + sfs: []SecondFactorType{SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN, SecondFactorType_SECOND_FACTOR_TYPE_OTP}, + }, + { + sf: constants.SecondFactorOn, + sfs: []SecondFactorType{SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN, SecondFactorType_SECOND_FACTOR_TYPE_OTP}, + }, + { + sf: constants.SecondFactorOTP, + sfs: []SecondFactorType{SecondFactorType_SECOND_FACTOR_TYPE_OTP}, + }, + { + sf: constants.SecondFactorWebauthn, + sfs: []SecondFactorType{SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN}, + }, + } { + t.Run(string(tt.sf), func(t *testing.T) { + assert.Equal(t, tt.sfs, secondFactorsFromLegacySecondFactor(tt.sf)) + }) + } +}