Address comments.

api/constants/constants.go

@@ -208,7 +208,8 @@ const (
 	// is required for all users.
 	SecondFactorWebauthn = SecondFactorType("webauthn")
 	// SecondFactorOn means that all 2FA protocols are supported and 2FA is
-	// required for all users.
+	// required for all users, or that SSO MFA is enabled which cannot be
+	// expressed through a different SecondFactorType.
 	SecondFactorOn = SecondFactorType("on")
 	// SecondFactorOptional means that all 2FA protocols are supported and 2FA
 	// is required only for users that have MFA devices registered.

api/types/authentication.go

@@ -78,7 +78,7 @@ type AuthPreference interface {
 	// GetSecondFactors gets a list of supported second factors.
 	GetSecondFactors() []SecondFactorType
 	// SetSecondFactors sets the list of supported second factors.
-	SetSecondFactors([]SecondFactorType)
+	SetSecondFactors(...SecondFactorType)
 	// GetPreferredLocalMFA returns a server-side hint for clients to pick an MFA
 	// method when various options are available.
 	// It is empty if there is nothing to suggest.
@@ -87,7 +87,7 @@ type AuthPreference interface {
 	IsSecondFactorEnabled() bool
 	// IsSecondFactorEnforced checks if second factor is enforced.
 	IsSecondFactorEnforced() bool
-	// IsSecondFactorTOTPAllowed  checks if users can use TOTP as an MFA method.
+	// IsSecondFactorTOTPAllowed checks if users can use TOTP as an MFA method.
 	IsSecondFactorTOTPAllowed() bool
 	// IsSecondFactorWebauthnAllowed checks if users can use WebAuthn as an MFA method.
 	IsSecondFactorWebauthnAllowed() bool
@@ -322,7 +322,7 @@ func (c *AuthPreferenceV2) SetType(s string) {
 // GetSecondFactor returns the type of second factor.
 func (c *AuthPreferenceV2) GetSecondFactor() constants.SecondFactorType {
 	// SecondFactors takes priority if set.
-	if c.Spec.SecondFactors != nil {
+	if len(c.Spec.SecondFactors) > 0 {
 		return legacySecondFactorFromSecondFactors(c.Spec.SecondFactors)
 	}
 
@@ -339,7 +339,7 @@ func (c *AuthPreferenceV2) SetSecondFactor(s constants.SecondFactorType) {
 
 // GetSecondFactors gets a list of supported second factors.
 func (c *AuthPreferenceV2) GetSecondFactors() []SecondFactorType {
-	if c.Spec.SecondFactors != nil {
+	if len(c.Spec.SecondFactors) > 0 {
 		return c.Spec.SecondFactors
 	}
 
@@ -348,8 +348,8 @@ func (c *AuthPreferenceV2) GetSecondFactors() []SecondFactorType {
 }
 
 // SetSecondFactors sets the list of supported second factors.
-func (c *AuthPreferenceV2) SetSecondFactors(s []SecondFactorType) {
-	c.Spec.SecondFactors = s
+func (c *AuthPreferenceV2) SetSecondFactors(sfs ...SecondFactorType) {
+	c.Spec.SecondFactors = sfs
 
 	// Unset SecondFactor, only one can be set at a time.
 	c.Spec.SecondFactor = ""
@@ -711,7 +711,7 @@ func (c *AuthPreferenceV2) CheckAndSetDefaults() error {
 	}
 
 	// Validate SecondFactor and SecondFactors.
-	if c.Spec.SecondFactor != "" && c.Spec.SecondFactors != nil {
+	if c.Spec.SecondFactor != "" && len(c.Spec.SecondFactors) > 0 {
 		return trace.BadParameter("must set either SecondFactor or SecondFactors, not both")
 	}
 
@@ -726,7 +726,7 @@ func (c *AuthPreferenceV2) CheckAndSetDefaults() error {
 		c.Spec.SecondFactor = constants.SecondFactorWebauthn
 	case "":
 		// default to OTP if SecondFactors is also not set.
-		if c.Spec.SecondFactors == nil {
+		if len(c.Spec.SecondFactors) == 0 {
 			c.Spec.SecondFactor = constants.SecondFactorOTP
 		}
 	default:
@@ -775,14 +775,11 @@ func (c *AuthPreferenceV2) CheckAndSetDefaults() error {
 		return trace.BadParameter("missing required Webauthn configuration for headless=true")
 	}
 
-	// Prevent accidental local lockout by disabling local second factor methods, (likely leaving only sso enabled).
+	// Prevent local lockout by disabling local second factor methods.
+	hasSSO := c.IsSecondFactorSSOAllowed()
 	hasTOTP := c.IsSecondFactorTOTPAllowed()
-	if c.GetAllowLocalAuth() && !hasTOTP && !hasWebauthn {
-		switch c.Spec.SecondFactor {
-		case constants.SecondFactorOptional, constants.SecondFactorOff:
-		default:
-			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() && 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")
 	}
 
 	// Validate connector name for type=local.

api/types/authentication_test.go

@@ -20,6 +20,10 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+
+	"github.com/gravitational/trace"
+
+	"github.com/gravitational/teleport/api/constants"
 )
 
 // TestMarshalUnmarshalRequireMFAType tests encoding/decoding of the RequireMFAType.
@@ -64,3 +68,144 @@ func TestEncodeDecodeRequireMFAType(t *testing.T) {
 		})
 	}
 }
+
+func TestAuthPreference_CheckAndSetDefaults(t *testing.T) {
+	for _, tt := range []struct {
+		name           string
+		spec           AuthPreferenceSpecV2
+		assertErr      require.ErrorAssertionFunc
+		assertAuthPref func(t *testing.T, authPref AuthPreference)
+	}{
+		{
+			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())
+			},
+		},
+		{
+			name: "OK OTP",
+			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())
+			},
+		},
+		{
+			name: "OK WebAuthn",
+			spec: AuthPreferenceSpecV2{
+				SecondFactors: []SecondFactorType{
+					SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN,
+				},
+				Webauthn: &Webauthn{
+					RPID: "localhost",
+				},
+			},
+			assertAuthPref: func(t *testing.T, authPref AuthPreference) {
+				require.True(t, authPref.GetAllowPasswordless())
+				require.True(t, authPref.GetAllowHeadless())
+			},
+		},
+		{
+			name: "OK SSO",
+			spec: AuthPreferenceSpecV2{
+				SecondFactors: []SecondFactorType{
+					SecondFactorType_SECOND_FACTOR_TYPE_SSO,
+				},
+				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())
+			},
+		},
+		{
+			name: "OK U2F config provided",
+			spec: AuthPreferenceSpecV2{
+				SecondFactors: []SecondFactorType{
+					SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN,
+				},
+				U2F: &U2F{
+					AppID: "https://localhost",
+				},
+			},
+		},
+		{
+			name: "NOK SecondFactor and SecondFactors both set",
+			spec: AuthPreferenceSpecV2{
+				SecondFactor: constants.SecondFactorWebauthn,
+				SecondFactors: []SecondFactorType{
+					SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN,
+				},
+			},
+			assertErr: func(t require.TestingT, err error, vals ...interface{}) {
+				require.ErrorAs(t, err, &trace.BadParameterError{})
+			},
+		},
+		{
+			name: "NOK WebAuthn config missing",
+			spec: AuthPreferenceSpecV2{
+				SecondFactors: []SecondFactorType{
+					SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN,
+				},
+			},
+			assertErr: func(t require.TestingT, err error, vals ...interface{}) {
+				require.ErrorAs(t, err, &trace.BadParameterError{})
+			},
+		},
+		{
+			name: "NOK prevent passwordless without WebAuthn",
+			spec: AuthPreferenceSpecV2{
+				SecondFactors: []SecondFactorType{
+					SecondFactorType_SECOND_FACTOR_TYPE_OTP,
+				},
+				AllowPasswordless: NewBoolOption(true),
+			},
+			assertErr: func(t require.TestingT, err error, vals ...interface{}) {
+				require.ErrorAs(t, err, &trace.BadParameterError{})
+			},
+		},
+		{
+			name: "NOK prevent headless without WebAuthn",
+			spec: AuthPreferenceSpecV2{
+				SecondFactors: []SecondFactorType{
+					SecondFactorType_SECOND_FACTOR_TYPE_OTP,
+				},
+				AllowHeadless: NewBoolOption(true),
+			},
+			assertErr: func(t require.TestingT, err error, vals ...interface{}) {
+				require.ErrorAs(t, err, &trace.BadParameterError{})
+			},
+		},
+		{
+			name: "NOK prevent local lockout with second factor SSO",
+			spec: AuthPreferenceSpecV2{
+				SecondFactors: []SecondFactorType{
+					SecondFactorType_SECOND_FACTOR_TYPE_SSO,
+				},
+				AllowLocalAuth: NewBoolOption(true),
+			},
+			assertErr: func(t require.TestingT, err error, vals ...interface{}) {
+				require.ErrorAs(t, err, &trace.BadParameterError{})
+			},
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			authPref, err := NewAuthPreference(tt.spec)
+			if tt.assertErr != nil {
+				tt.assertErr(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+
+			if tt.assertAuthPref != nil {
+				tt.assertAuthPref(t, authPref)
+			}
+		})
+	}
+}

api/types/second_factor.go

@@ -29,10 +29,9 @@ import (
 func legacySecondFactorFromSecondFactors(secondFactors []SecondFactorType) constants.SecondFactorType {
 	hasOTP := slices.Contains(secondFactors, SecondFactorType_SECOND_FACTOR_TYPE_OTP)
 	hasWebAuthn := slices.Contains(secondFactors, SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN)
-	hasSSO := slices.Contains(secondFactors, SecondFactorType_SECOND_FACTOR_TYPE_SSO)
 
 	switch {
-	case (hasOTP && hasWebAuthn) || hasSSO:
+	case (hasOTP && hasWebAuthn):
 		return constants.SecondFactorOn
 	case hasWebAuthn:
 		return constants.SecondFactorWebauthn

api/types/second_factor_test.go

@@ -0,0 +1,57 @@
+/*
+Copyright 2022 Gravitational, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package types
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestEncodeDecodeSecondFactorType tests encoding/decoding of the SecondFactorType.
+func TestEncodeDecodeSecondFactorType(t *testing.T) {
+	for _, tt := range []struct {
+		secondFactorType SecondFactorType
+		encoded          string
+	}{
+		{
+			secondFactorType: SecondFactorType_SECOND_FACTOR_TYPE_OTP,
+			encoded:          secondFactorTypeOTPString,
+		}, {
+			secondFactorType: SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN,
+			encoded:          secondFactorTypeWebauthnString,
+		}, {
+			secondFactorType: SecondFactorType_SECOND_FACTOR_TYPE_SSO,
+			encoded:          secondFactorTypeSSOString,
+		},
+	} {
+		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)
+			})
+
+			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)
+			})
+		})
+	}
+}

lib/config/configuration_test.go

@@ -4978,9 +4978,9 @@ func TestProxyUntrustedCert(t *testing.T) {
 	certDir := t.TempDir()
 	certPath := filepath.Join(certDir, "leaf.crt")
 	keyPath := filepath.Join(certDir, "leaf.key")
-	err = os.WriteFile(certPath, leafCert, 0o600)
+	err = os.WriteFile(certPath, leafCert, 0600)
 	require.NoError(t, err)
-	err = os.WriteFile(keyPath, leafPrivPem, 0o600)
+	err = os.WriteFile(keyPath, leafPrivPem, 0600)
 	require.NoError(t, err)
 
 	fc := &FileConfig{