Skip to content

Commit

Permalink
fix: Forbid SSO users from logging in using passwordless (#41062) (#4…
Browse files Browse the repository at this point in the history
…1071)

* Save the WebAuthn UserHandle in the mock Key

* Simplify passwordless tests

* fix: Forbid SSO users from logging in using passwordless

* Rename tests to TestPasswordlessProhibitedForSSO
  • Loading branch information
codingllama authored Apr 30, 2024
1 parent fa59ff2 commit e4e5f46
Show file tree
Hide file tree
Showing 11 changed files with 257 additions and 21 deletions.
6 changes: 6 additions & 0 deletions api/types/authentication.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions api/utils/keys/policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
12 changes: 12 additions & 0 deletions lib/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
89 changes: 85 additions & 4 deletions lib/auth/auth_login_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
3 changes: 1 addition & 2 deletions lib/auth/helpers_mfa.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
Expand Down
6 changes: 5 additions & 1 deletion lib/auth/methods.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
5 changes: 5 additions & 0 deletions lib/auth/mocku2f/mocku2f.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 18 additions & 2 deletions lib/auth/mocku2f/webauthn.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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{
Expand Down
5 changes: 0 additions & 5 deletions lib/auth/webauthn/login_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
19 changes: 12 additions & 7 deletions lib/web/apiserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down
110 changes: 110 additions & 0 deletions lib/web/apiserver_login_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()

Expand Down

0 comments on commit e4e5f46

Please sign in to comment.