From e4e5f4652c0479c4b42218b96faf25c69218fe8d Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Tue, 30 Apr 2024 17:24:47 -0300 Subject: [PATCH] fix: Forbid SSO users from logging in using passwordless (#41062) (#41071) * Save the WebAuthn UserHandle in the mock Key * Simplify passwordless tests * fix: Forbid SSO users from logging in using passwordless * Rename tests to TestPasswordlessProhibitedForSSO --- api/types/authentication.go | 6 ++ api/utils/keys/policy.go | 3 + lib/auth/auth.go | 12 ++++ lib/auth/auth_login_test.go | 89 ++++++++++++++++++++++++-- lib/auth/helpers_mfa.go | 3 +- lib/auth/methods.go | 6 +- lib/auth/mocku2f/mocku2f.go | 5 ++ lib/auth/mocku2f/webauthn.go | 20 +++++- lib/auth/webauthn/login_test.go | 5 -- lib/web/apiserver.go | 19 ++++-- lib/web/apiserver_login_test.go | 110 ++++++++++++++++++++++++++++++++ 11 files changed, 257 insertions(+), 21 deletions(-) diff --git a/api/types/authentication.go b/api/types/authentication.go index 445575421cfe4..f001517fa1e52 100644 --- a/api/types/authentication.go +++ b/api/types/authentication.go @@ -47,6 +47,12 @@ var ( ErrPasswordlessDisabledBySettings = &trace.BadParameterError{ Message: "passwordless disabled by cluster settings", } + + // ErrPassswordlessLoginBySSOUser is issued if an SSO user tries to login + // using passwordless. + ErrPassswordlessLoginBySSOUser = &trace.AccessDeniedError{ + Message: "SSO user cannot login using passwordless", + } ) // AuthPreference defines the authentication preferences for a specific diff --git a/api/utils/keys/policy.go b/api/utils/keys/policy.go index b109e4f85a166..b8a4c5a92f5c5 100644 --- a/api/utils/keys/policy.go +++ b/api/utils/keys/policy.go @@ -190,5 +190,8 @@ func ParsePrivateKeyPolicyError(err error) (PrivateKeyPolicy, error) { // IsPrivateKeyPolicyError returns true if the given error is a private key policy error. func IsPrivateKeyPolicyError(err error) bool { + if err == nil { + return false + } return privateKeyPolicyErrRegex.MatchString(err.Error()) } diff --git a/lib/auth/auth.go b/lib/auth/auth.go index 921155f448f65..10c1eed0c1390 100644 --- a/lib/auth/auth.go +++ b/lib/auth/auth.go @@ -5954,6 +5954,18 @@ func (a *Server) validateMFAAuthResponse( Identity: a.Services, } dev, user, err = webLogin.Finish(ctx, assertionResp) + + // Disallow non-local users from logging in with passwordless. + if err == nil { + u, getErr := a.GetUser(user, false /* withSecrets */) + if getErr != nil { + err = trace.Wrap(getErr) + } else if u.GetUserType() != types.UserTypeLocal { + // Return the error unmodified, without the "MFA response validation + // failed" prefix. + return nil, "", trace.Wrap(types.ErrPassswordlessLoginBySSOUser) + } + } } else { webLogin := &wanlib.LoginFlow{ U2F: u2f, diff --git a/lib/auth/auth_login_test.go b/lib/auth/auth_login_test.go index a78a5057f079e..f4d981f9c1f93 100644 --- a/lib/auth/auth_login_test.go +++ b/lib/auth/auth_login_test.go @@ -788,9 +788,6 @@ func TestServer_Authenticate_passwordless(t *testing.T) { }) require.NoError(t, err, "Failed to register passwordless device") - // userWebID is what identifies the user for usernameless/passwordless. - userWebID := registerChallenge.GetWebauthn().PublicKey.User.Id - // Use a proxy client for now on; the user's identity isn't established yet. proxyClient, err := svr.NewClient(TestBuiltin(types.RoleProxy)) require.NoError(t, err) @@ -900,7 +897,6 @@ func TestServer_Authenticate_passwordless(t *testing.T) { // Sign challenge (mocks user interaction). assertionResp, err := pwdKey.SignAssertion(origin, wantypes.CredentialAssertionFromProto(mfaChallenge.GetWebauthnChallenge())) require.NoError(t, err) - assertionResp.AssertionResponse.UserHandle = userWebID // identify user, a real device would set this // Complete login procedure (SSH or Web). test.authenticate(t, assertionResp) @@ -916,6 +912,91 @@ func TestServer_Authenticate_passwordless(t *testing.T) { } } +// TestPasswordlessProhibitedForSSO tests a scenario where an SSO user bypasses +// SSO by using a passwordless login. +func TestPasswordlessProhibitedForSSO(t *testing.T) { + t.Parallel() + + testServer := newTestTLSServer(t) + authServer := testServer.Auth() + clock := testServer.Clock() + + mfa := configureForMFA(t, testServer) + ctx := context.Background() + + // Register a passwordless device. + userClient, err := testServer.NewClient(TestUser(mfa.User)) + require.NoError(t, err, "NewClient failed") + pwdlessDev, err := RegisterTestDevice(ctx, userClient, "pwdless", proto.DeviceType_DEVICE_TYPE_WEBAUTHN, mfa.WebDev, WithPasswordless()) + require.NoError(t, err, "RegisterTestDevice failed") + + // Edit the configured user so it looks like an SSO user attempting local + // logins. This isn't exactly like an SSO user, but it's close enough. + _, err = authServer.UpdateAndSwapUser(ctx, mfa.User, true /* withSecrets */, func(user types.User) (changed bool, err error) { + user.SetCreatedBy(types.CreatedBy{ + Connector: &types.ConnectorRef{ + Type: constants.Github, + ID: "github", + Identity: mfa.User, + }, + Time: clock.Now(), + User: types.UserRef{ + Name: teleport.UserSystem, + }, + }) + return true, nil + }) + require.NoError(t, err, "UpdateAndSwapUser failed") + + // Authentication happens through the Proxy identity. + proxyClient, err := testServer.NewClient(TestBuiltin(types.RoleProxy)) + require.NoError(t, err) + + tests := []struct { + name string + authenticate func(*wantypes.CredentialAssertionResponse) error + }{ + { + name: "web not allowed", + authenticate: func(car *wantypes.CredentialAssertionResponse) error { + _, err := proxyClient.AuthenticateWebUser(ctx, AuthenticateUserRequest{ + Webauthn: car, + }) + return err + }, + }, + { + name: "ssh not allowed", + authenticate: func(car *wantypes.CredentialAssertionResponse) error { + _, err := proxyClient.AuthenticateSSHUser(ctx, AuthenticateSSHRequest{ + AuthenticateUserRequest: AuthenticateUserRequest{ + PublicKey: []byte(sshPubKey), + Webauthn: car, + }, + TTL: 12 * time.Hour, + }) + return err + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + chal, err := proxyClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{ + Request: &proto.CreateAuthenticateChallengeRequest_Passwordless{ + Passwordless: &proto.Passwordless{}, + }, + }) + require.NoError(t, err, "CreateAuthenticateChallenge failed") + + chalResp, err := pwdlessDev.SolveAuthn(chal) + require.NoError(t, err, "SolveAuthn failed") + + err = test.authenticate(wantypes.CredentialAssertionResponseFromProto(chalResp.GetWebauthn())) + assert.ErrorIs(t, err, types.ErrPassswordlessLoginBySSOUser, "authentication error mismatch") + }) + } +} + func TestServer_Authenticate_nonPasswordlessRequiresUsername(t *testing.T) { t.Parallel() svr := newTestTLSServer(t) diff --git a/lib/auth/helpers_mfa.go b/lib/auth/helpers_mfa.go index ae922c39277b7..47eb227b8b1b8 100644 --- a/lib/auth/helpers_mfa.go +++ b/lib/auth/helpers_mfa.go @@ -240,8 +240,7 @@ func (d *TestDevice) solveRegisterWebauthn(c *proto.MFARegisterChallenge) (*prot d.Key.PreferRPID = true if d.passwordless { - d.Key.AllowResidentKey = true - d.Key.SetUV = true + d.Key.SetPasswordless() } resp, err := d.Key.SignCredentialCreation(d.Origin(), wantypes.CredentialCreationFromProto(c.GetWebauthn())) diff --git a/lib/auth/methods.go b/lib/auth/methods.go index f26decb755a00..45e328bfc4c90 100644 --- a/lib/auth/methods.go +++ b/lib/auth/methods.go @@ -471,7 +471,11 @@ func (a *Server) authenticatePasswordless(ctx context.Context, req AuthenticateU }, } dev, user, err := a.validateMFAAuthResponse(ctx, mfaResponse, "", true /* passwordless */) - if err != nil { + switch { + // Don't obfuscate the SSO error. + case errors.Is(err, types.ErrPassswordlessLoginBySSOUser): + return nil, "", trace.Wrap(err) + case err != nil: log.Debugf("Passwordless authentication failed: %v", err) return nil, "", trace.Wrap(authenticateWebauthnError) } diff --git a/lib/auth/mocku2f/mocku2f.go b/lib/auth/mocku2f/mocku2f.go index 53604bfb75e03..0e043dbcdd41b 100644 --- a/lib/auth/mocku2f/mocku2f.go +++ b/lib/auth/mocku2f/mocku2f.go @@ -48,6 +48,11 @@ type Key struct { // Cert is the Key attestation certificate. Cert []byte + // UserHandle is the WebAuthn User ID. + // Saved from passwordless registrations and set on passwordless assertions. + // Requires a passwordless-configured Key (see [Key.SetPasswordless]). + UserHandle []byte + // PreferRPID instructs the Key to use favor using the RPID for Webauthn // ceremonies, even if the U2F App ID extension is present. PreferRPID bool diff --git a/lib/auth/mocku2f/webauthn.go b/lib/auth/mocku2f/webauthn.go index 03faeb31c0d51..3ff23db5df3d0 100644 --- a/lib/auth/mocku2f/webauthn.go +++ b/lib/auth/mocku2f/webauthn.go @@ -77,6 +77,12 @@ func (muk *Key) SignAssertion(origin string, assertion *wantypes.CredentialAsser return nil, trace.Wrap(err) } + // If passwordless, then relay our WebAuthn UserHandle. + var userHandle []byte + if len(assertion.Response.AllowedCredentials) == 0 && len(muk.UserHandle) > 0 { + userHandle = muk.UserHandle + } + return &wantypes.CredentialAssertionResponse{ PublicKeyCredential: wantypes.PublicKeyCredential{ Credential: wantypes.Credential{ @@ -93,7 +99,8 @@ func (muk *Key) SignAssertion(origin string, assertion *wantypes.CredentialAsser }, AuthenticatorData: res.AuthData, // Signature starts after user presence (1byte) and counter (4 bytes). - Signature: res.SignData[5:], + Signature: res.SignData[5:], + UserHandle: userHandle, }, }, nil } @@ -126,7 +133,11 @@ func (muk *Key) SignCredentialCreation(origin string, cc *wantypes.CredentialCre if aa := cc.Response.AuthenticatorSelection.AuthenticatorAttachment; aa == protocol.Platform { return nil, trace.BadParameter("platform attachment required by authenticator selection") } - if rrk := cc.Response.AuthenticatorSelection.RequireResidentKey; rrk != nil && *rrk && !muk.AllowResidentKey { + rrk, err := cc.RequireResidentKey() + if err != nil { + return nil, trace.Wrap(err) + } + if rrk && !muk.AllowResidentKey { return nil, trace.BadParameter("resident key required by authenticator selection") } if uv := cc.Response.AuthenticatorSelection.UserVerification; uv == protocol.VerificationRequired && !muk.SetUV { @@ -182,6 +193,11 @@ func (muk *Key) SignCredentialCreation(origin string, cc *wantypes.CredentialCre return nil, trace.Wrap(err) } + // Save the WebAuthn UserHandle if this is a resident key creation. + if rrk && len(cc.Response.User.ID) > 0 && len(muk.UserHandle) == 0 { + muk.UserHandle = cc.Response.User.ID + } + return &wantypes.CredentialCreationResponse{ PublicKeyCredential: wantypes.PublicKeyCredential{ Credential: wantypes.Credential{ diff --git a/lib/auth/webauthn/login_test.go b/lib/auth/webauthn/login_test.go index 721e9e5c72bde..58b968c0e53db 100644 --- a/lib/auth/webauthn/login_test.go +++ b/lib/auth/webauthn/login_test.go @@ -430,11 +430,6 @@ func TestPasswordlessFlow_BeginAndFinish(t *testing.T) { // User interaction would happen here. assertionResp, err := test.key.SignAssertion(test.origin, assertion) require.NoError(t, err) - // Fetch the stored user handle; in a real-world the scenario the - // authenticator knows it, as passwordless requires a resident credential. - wla, err := identity.GetWebauthnLocalAuth(ctx, test.user) - require.NoError(t, err) - assertionResp.AssertionResponse.UserHandle = wla.UserID // 2nd and last step of the login ceremony. mfaDevice, user, err := webLogin.Finish(ctx, assertionResp) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 770df7ffe68f1..6c72cc28e560a 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -2517,13 +2517,18 @@ func (h *Handler) mfaLoginFinishSession(w http.ResponseWriter, r *http.Request, clientMeta := clientMetaFromReq(r) session, err := h.auth.AuthenticateWebUser(r.Context(), req, clientMeta) - if err != nil { - // Since checking for private key policy meant that they passed authn, - // return policy error as is to help direct user. - if keys.IsPrivateKeyPolicyError(err) { - return nil, trace.Wrap(err) - } - // Obscure all other errors. + switch { + // Since checking for private key policy meant that they passed authn, + // return policy error as is to help direct user. + case keys.IsPrivateKeyPolicyError(err): + return nil, trace.Wrap(err) + + // Return a friendlier error if an SSO user tried to do passwordless. + case errors.Is(err, types.ErrPassswordlessLoginBySSOUser): + return nil, trace.Wrap(err) + + // Obscure all other errors. + case err != nil: return nil, trace.AccessDenied("invalid credentials") } diff --git a/lib/web/apiserver_login_test.go b/lib/web/apiserver_login_test.go index 09b7797fd916c..84378cc546cef 100644 --- a/lib/web/apiserver_login_test.go +++ b/lib/web/apiserver_login_test.go @@ -29,6 +29,7 @@ import ( "github.com/stretchr/testify/require" "golang.org/x/crypto/ssh" + "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/constants" "github.com/gravitational/teleport/api/types" @@ -313,6 +314,115 @@ func TestAuthenticate_passwordless(t *testing.T) { }) } +// TestPasswordlessProhibitedForSSO is rather similar to +// lib/auth.TestPasswordlessProhibitedForSSO, but here our main concern is that +// error messages aren't obfuscated along the way. +func TestPasswordlessProhibitedForSSO(t *testing.T) { + env := newWebPack(t, 1) + + testServer := env.server + authServer := testServer.Auth() + proxyServer := env.proxies[0] + clock := env.clock + + // Prepare user and default devices. + mfa := configureClusterForMFA(t, env, &types.AuthPreferenceSpecV2{ + Type: constants.Local, + SecondFactor: constants.SecondFactorOn, + Webauthn: &types.Webauthn{ + RPID: testServer.ClusterName(), + }, + }) + user := mfa.User + ctx := context.Background() + + // Register a passwordless device. + userClient, err := testServer.NewClient(auth.TestUser(user)) + require.NoError(t, err, "NewClient failed") + pwdlessDev, err := auth.RegisterTestDevice( + ctx, userClient, "pwdless", proto.DeviceType_DEVICE_TYPE_WEBAUTHN, mfa.WebDev, auth.WithPasswordless()) + require.NoError(t, err, "RegisterTestDevice failed") + + // Update the user so it looks like an SSO user. + _, err = authServer.UpdateAndSwapUser(ctx, user, false /* withSecrets */, func(u types.User) (changed bool, err error) { + u.SetCreatedBy(types.CreatedBy{ + Connector: &types.ConnectorRef{ + Type: constants.Github, + ID: "github", + Identity: user, + }, + Time: clock.Now(), + User: types.UserRef{ + Name: teleport.UserSystem, + }, + }) + return true, nil + }) + require.NoError(t, err, "UpdateAndSwapUser failed") + + // Prepare SSH key to be signed. + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err, "GenerateKey failed") + pub, err := ssh.NewPublicKey(&priv.PublicKey) + require.NoError(t, err, "NewPublicKey failed") + pubBytes := ssh.MarshalAuthorizedKey(pub) + + webClient, err := client.NewWebClient( + proxyServer.webURL.String(), + roundtrip.HTTPClient(client.NewInsecureWebClient()), + ) + require.NoError(t, err, "NewWebClient failed") + + tests := []struct { + name string + login func(chalResp *wantypes.CredentialAssertionResponse) error + }{ + { + name: "web", + login: func(chalResp *wantypes.CredentialAssertionResponse) error { + ep := webClient.Endpoint("webapi", "mfa", "login", "finishsession") + _, err := webClient.PostJSON(ctx, ep, &client.AuthenticateWebUserRequest{ + WebauthnAssertionResponse: chalResp, + }) + return err + }, + }, + { + name: "ssh", + login: func(chalResp *wantypes.CredentialAssertionResponse) error { + ep := webClient.Endpoint("webapi", "mfa", "login", "finish") + _, err := webClient.PostJSON(ctx, ep, &client.AuthenticateSSHUserRequest{ + WebauthnChallengeResponse: chalResp, + PubKey: pubBytes, + TTL: 12 * time.Hour, + }) + return err + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // Issue passwordless challenge. + ep := webClient.Endpoint("webapi", "mfa", "login", "begin") + beginResp, err := webClient.PostJSON(ctx, ep, &client.MFAChallengeRequest{ + Passwordless: true, + }) + require.NoError(t, err, "POST /webapi/mfa/login/begin") + mfaChallenge := &client.MFAAuthenticateChallenge{} + require.NoError(t, json.Unmarshal(beginResp.Bytes(), mfaChallenge), "Unmarshal MFA challenge failed") + + // Sign it. + origin := "https://" + testServer.ClusterName() + chalResp, err := pwdlessDev.Key.SignAssertion(origin, mfaChallenge.WebauthnChallenge) + require.NoError(t, err, "SignAssertion failed") + + // Login and verify that the passwordless/SSO error was not obfuscated. + err = test.login(chalResp) + assert.ErrorIs(t, err, types.ErrPassswordlessLoginBySSOUser, "Login error mismatch") + }) + } +} + func TestAuthenticate_rateLimiting(t *testing.T) { ctx := context.Background()