Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v13] fix: Forbid SSO users from logging in using passwordless #41072

Merged
merged 1 commit into from
Apr 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -87,5 +87,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 @@ -5578,6 +5578,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 @@ -430,7 +430,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 @@ -2406,13 +2406,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 @@ -30,6 +30,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 @@ -314,6 +315,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
Loading