From 39e06f56fc7eec26cb0ae3d1c4ee28282738c4ec Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Fri, 8 Jul 2022 16:50:27 -0300 Subject: [PATCH 1/7] Refactor touchid.CredentialInfo --- lib/auth/touchid/api.go | 13 +++++++++---- lib/auth/touchid/api_darwin.go | 6 ++++-- lib/auth/touchid/api_test.go | 10 ++++++---- tool/tsh/touchid.go | 4 ++-- 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/lib/auth/touchid/api.go b/lib/auth/touchid/api.go index 8c7ffc5626a38..127e5efc8db99 100644 --- a/lib/auth/touchid/api.go +++ b/lib/auth/touchid/api.go @@ -113,10 +113,9 @@ type DiagResult struct { // CredentialInfo holds information about a Secure Enclave credential. type CredentialInfo struct { - UserHandle []byte CredentialID string RPID string - User string + User UserInfo PublicKey *ecdsa.PublicKey CreateTime time.Time @@ -125,6 +124,12 @@ type CredentialInfo struct { publicKeyRaw []byte } +// UserInfo holds information about a credential owner. +type UserInfo struct { + UserHandle []byte + Name string +} + var ( cachedDiag *DiagResult cachedDiagMU sync.Mutex @@ -506,9 +511,9 @@ func Login(origin, user string, assertion *wanlib.CredentialAssertion) (*wanlib. }, AuthenticatorData: attData.rawAuthData, Signature: sig, - UserHandle: cred.UserHandle, + UserHandle: cred.User.UserHandle, }, - }, cred.User, nil + }, cred.User.Name, nil } func findAllowedCredential(infos []CredentialInfo, allowedCredentials []protocol.CredentialDescriptor) (CredentialInfo, bool) { diff --git a/lib/auth/touchid/api_darwin.go b/lib/auth/touchid/api_darwin.go index 3a36ad3b8d768..10e897d79970b 100644 --- a/lib/auth/touchid/api_darwin.go +++ b/lib/auth/touchid/api_darwin.go @@ -334,10 +334,12 @@ func readCredentialInfos(find func(**C.CredentialInfo) C.int) ([]CredentialInfo, } infos = append(infos, CredentialInfo{ - UserHandle: userHandle, CredentialID: credentialID, RPID: parsedLabel.rpID, - User: parsedLabel.user, + User: UserInfo{ + UserHandle: userHandle, + Name: parsedLabel.user, + }, CreateTime: createTime, publicKeyRaw: pubKeyRaw, }) diff --git a/lib/auth/touchid/api_test.go b/lib/auth/touchid/api_test.go index fb4eebae3555c..e7d22de29c5a0 100644 --- a/lib/auth/touchid/api_test.go +++ b/lib/auth/touchid/api_test.go @@ -604,12 +604,14 @@ func (f *fakeNative) FindCredentials(rpID, user string) ([]touchid.CredentialInf for _, cred := range f.creds { if cred.rpID == rpID && (user == "" || cred.user == user) { resp = append(resp, touchid.CredentialInfo{ - UserHandle: cred.userHandle, CredentialID: cred.id, RPID: cred.rpID, - User: cred.user, - PublicKey: &cred.key.PublicKey, - CreateTime: cred.createTime, + User: touchid.UserInfo{ + UserHandle: cred.userHandle, + Name: cred.user, + }, + PublicKey: &cred.key.PublicKey, + CreateTime: cred.createTime, }) } } diff --git a/tool/tsh/touchid.go b/tool/tsh/touchid.go index a4646bfcf8fab..90c0676c673d7 100644 --- a/tool/tsh/touchid.go +++ b/tool/tsh/touchid.go @@ -95,7 +95,7 @@ func (c *touchIDLsCommand) run(cf *CLIConf) error { if cmp := strings.Compare(i1.RPID, i2.RPID); cmp != 0 { return cmp < 0 } - if cmp := strings.Compare(i1.User, i2.User); cmp != 0 { + if cmp := strings.Compare(i1.User.Name, i2.User.Name); cmp != 0 { return cmp < 0 } return i1.CreateTime.Before(i2.CreateTime) @@ -105,7 +105,7 @@ func (c *touchIDLsCommand) run(cf *CLIConf) error { for _, info := range infos { t.AddRow([]string{ info.RPID, - info.User, + info.User.Name, info.CreateTime.Format(time.RFC3339), info.CredentialID, }) From 8afe087d0b6f447027268785cdbc6e4206c9ed5e Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Fri, 8 Jul 2022 16:53:56 -0300 Subject: [PATCH 2/7] Refactor wancli.CredentialInfo --- lib/auth/webauthncli/api.go | 39 +++++++++++++++++++++++ lib/auth/webauthncli/fido2.go | 12 +++---- lib/auth/webauthncli/fido2_common.go | 37 --------------------- lib/auth/webauthncli/fido2_prompt.go | 2 +- lib/auth/webauthncli/fido2_prompt_test.go | 28 ++++++++-------- lib/auth/webauthncli/fido2_test.go | 2 +- 6 files changed, 61 insertions(+), 59 deletions(-) diff --git a/lib/auth/webauthncli/api.go b/lib/auth/webauthncli/api.go index 3f5ed78db9f7e..54cb4afce1819 100644 --- a/lib/auth/webauthncli/api.go +++ b/lib/auth/webauthncli/api.go @@ -36,6 +36,35 @@ const ( AttachmentPlatform ) +// CredentialInfo holds information about a WebAuthn credential, typically a +// resident public key credential. +type CredentialInfo struct { + ID []byte + User UserInfo +} + +// UserInfo holds information about a credential owner. +type UserInfo struct { + // UserHandle is the WebAuthn user handle (also referred as user ID). + UserHandle []byte + Name string +} + +// LoginPrompt is the user interface for FIDO2Login. +type LoginPrompt interface { + // PromptPIN prompts the user for their PIN. + PromptPIN() (string, error) + // PromptTouch prompts the user for a security key touch. + // In certain situations multiple touches may be required (PIN-protected + // devices, passwordless flows, etc). + PromptTouch() + // PromptCredential prompts the user to choose a credential, in case multiple + // credentials are available. + // Callers are free to modify the slice, such as by sorting the credentials, + // but must return one of the pointers contained within. + PromptCredential(creds []*CredentialInfo) (*CredentialInfo, error) +} + // LoginOpts groups non-mandatory options for Login. type LoginOpts struct { // User is the desired credential username for login. @@ -124,6 +153,16 @@ func platformLogin(origin, user string, assertion *wanlib.CredentialAssertion) ( }, credentialUser, nil } +// RegisterPrompt is the user interface for FIDO2Register. +type RegisterPrompt interface { + // PromptPIN prompts the user for their PIN. + PromptPIN() (string, error) + // PromptTouch prompts the user for a security key touch. + // In certain situations multiple touches may be required (eg, PIN-protected + // devices) + PromptTouch() +} + // Register performs client-side, U2F-compatible, Webauthn registration. // This method blocks until either device authentication is successful or the // context is canceled. Calling Register without a deadline or cancel condition diff --git a/lib/auth/webauthncli/fido2.go b/lib/auth/webauthncli/fido2.go index 4bca9778523f6..d0a59e3d6c73e 100644 --- a/lib/auth/webauthncli/fido2.go +++ b/lib/auth/webauthncli/fido2.go @@ -280,14 +280,14 @@ func pickAssertion( } // Prepare credentials and show picker. - creds := make([]*Credential, len(assertions)) - credToAssertion := make(map[*Credential]*libfido2.Assertion) + creds := make([]*CredentialInfo, len(assertions)) + credToAssertion := make(map[*CredentialInfo]*libfido2.Assertion) for i, assertion := range assertions { - cred := &Credential{ + cred := &CredentialInfo{ ID: assertion.CredentialID, - User: User{ - ID: assertion.User.ID, - Name: assertion.User.Name, + User: UserInfo{ + UserHandle: assertion.User.ID, + Name: assertion.User.Name, }, } credToAssertion[cred] = assertion diff --git a/lib/auth/webauthncli/fido2_common.go b/lib/auth/webauthncli/fido2_common.go index 3fc64a8febfa7..6a122ef6284c4 100644 --- a/lib/auth/webauthncli/fido2_common.go +++ b/lib/auth/webauthncli/fido2_common.go @@ -30,33 +30,6 @@ import ( // FIDO2PollInterval is the poll interval used to check for new FIDO2 devices. var FIDO2PollInterval = 200 * time.Millisecond -// Credential represents a WebAuthn credential. -type Credential struct { - ID []byte - User User -} - -// User represents a credential user. -type User struct { - ID []byte - Name string -} - -// LoginPrompt is the user interface for FIDO2Login. -type LoginPrompt interface { - // PromptPIN prompts the user for their PIN. - PromptPIN() (string, error) - // PromptTouch prompts the user for a security key touch. - // In certain situations multiple touches may be required (PIN-protected - // devices, passwordless flows, etc). - PromptTouch() - // PromptCredential prompts the user to choose a credential, in case multiple - // credentials are available. - // Callers are free to modify the slice, such as by sorting the credentials, - // but must return one of the pointers contained within. - PromptCredential(creds []*Credential) (*Credential, error) -} - // FIDO2Login implements Login for CTAP1 and CTAP2 devices. // It must be called with a context with timeout, otherwise it can run // indefinitely. @@ -73,16 +46,6 @@ func FIDO2Login( return fido2Login(ctx, origin, assertion, prompt, opts) } -// RegisterPrompt is the user interface for FIDO2Register. -type RegisterPrompt interface { - // PromptPIN prompts the user for their PIN. - PromptPIN() (string, error) - // PromptTouch prompts the user for a security key touch. - // In certain situations multiple touches may be required (eg, PIN-protected - // devices) - PromptTouch() -} - // FIDO2Register implements Register for CTAP1 and CTAP2 devices. // It must be called with a context with timeout, otherwise it can run // indefinitely. diff --git a/lib/auth/webauthncli/fido2_prompt.go b/lib/auth/webauthncli/fido2_prompt.go index bf5130fd9760f..d16daaaa5eaa7 100644 --- a/lib/auth/webauthncli/fido2_prompt.go +++ b/lib/auth/webauthncli/fido2_prompt.go @@ -74,7 +74,7 @@ func (p *DefaultPrompt) PromptTouch() { // PromptCredential prompts the user to choose a credential, in case multiple // credentials are available. -func (p *DefaultPrompt) PromptCredential(creds []*Credential) (*Credential, error) { +func (p *DefaultPrompt) PromptCredential(creds []*CredentialInfo) (*CredentialInfo, error) { // Shouldn't happen, but let's check just in case. if len(creds) == 0 { return nil, errors.New("attempted to prompt credential with empty credentials") diff --git a/lib/auth/webauthncli/fido2_prompt_test.go b/lib/auth/webauthncli/fido2_prompt_test.go index 7b9a9078fecf0..7583b0da9f312 100644 --- a/lib/auth/webauthncli/fido2_prompt_test.go +++ b/lib/auth/webauthncli/fido2_prompt_test.go @@ -29,18 +29,18 @@ func TestDefaultPrompt_PromptCredential(t *testing.T) { oldStdin := prompt.Stdin() t.Cleanup(func() { prompt.SetStdin(oldStdin) }) - llamaCred := &wancli.Credential{ - User: wancli.User{ + llamaCred := &wancli.CredentialInfo{ + User: wancli.UserInfo{ Name: "llama", }, } - alpacaCred := &wancli.Credential{ - User: wancli.User{ + alpacaCred := &wancli.CredentialInfo{ + User: wancli.UserInfo{ Name: "alpaca", }, } - camelCred := &wancli.Credential{ - User: wancli.User{ + camelCred := &wancli.CredentialInfo{ + User: wancli.UserInfo{ Name: "camel", }, } @@ -51,8 +51,8 @@ func TestDefaultPrompt_PromptCredential(t *testing.T) { name string fakeReader *prompt.FakeReader ctx context.Context - creds []*wancli.Credential - wantCred *wancli.Credential + creds []*wancli.CredentialInfo + wantCred *wancli.CredentialInfo wantErr string // Optional, verifies output text. wantOut string @@ -60,19 +60,19 @@ func TestDefaultPrompt_PromptCredential(t *testing.T) { { name: "credential by number (1)", fakeReader: prompt.NewFakeReader().AddString("1"), // sorted by name - creds: []*wancli.Credential{llamaCred, alpacaCred, camelCred}, + creds: []*wancli.CredentialInfo{llamaCred, alpacaCred, camelCred}, wantCred: alpacaCred, }, { name: "credential by number (2)", fakeReader: prompt.NewFakeReader().AddString("3"), // sorted by name - creds: []*wancli.Credential{llamaCred, alpacaCred, camelCred}, + creds: []*wancli.CredentialInfo{llamaCred, alpacaCred, camelCred}, wantCred: llamaCred, }, { name: "credential by name", fakeReader: prompt.NewFakeReader().AddString("alpaca"), - creds: []*wancli.Credential{llamaCred, alpacaCred, camelCred}, + creds: []*wancli.CredentialInfo{llamaCred, alpacaCred, camelCred}, wantCred: alpacaCred, }, { @@ -81,19 +81,19 @@ func TestDefaultPrompt_PromptCredential(t *testing.T) { AddString("bad"). AddString("5"). AddString("llama"), - creds: []*wancli.Credential{llamaCred, alpacaCred, camelCred}, + creds: []*wancli.CredentialInfo{llamaCred, alpacaCred, camelCred}, wantCred: llamaCred, }, { name: "NOK empty credentials errors", fakeReader: prompt.NewFakeReader(), - creds: []*wancli.Credential{}, + creds: []*wancli.CredentialInfo{}, wantErr: "empty credentials", }, { name: "output text", fakeReader: prompt.NewFakeReader().AddString("llama"), - creds: []*wancli.Credential{llamaCred, alpacaCred, camelCred}, + creds: []*wancli.CredentialInfo{llamaCred, alpacaCred, camelCred}, wantCred: llamaCred, wantOut: `[1] alpaca [2] camel diff --git a/lib/auth/webauthncli/fido2_test.go b/lib/auth/webauthncli/fido2_test.go index 92e13baaf2f96..8713b77c2b0fb 100644 --- a/lib/auth/webauthncli/fido2_test.go +++ b/lib/auth/webauthncli/fido2_test.go @@ -108,7 +108,7 @@ var ( // simplePicker is a credential picker that always picks the first credential. type simplePicker struct{} -func (p simplePicker) PromptCredential(creds []*wancli.Credential) (*wancli.Credential, error) { +func (p simplePicker) PromptCredential(creds []*wancli.CredentialInfo) (*wancli.CredentialInfo, error) { return creds[0], nil } From 194f52ec31ce6194a3c9024475229feb1a2de6fa Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Fri, 8 Jul 2022 16:56:43 -0300 Subject: [PATCH 3/7] Move fido2_prompt*.go to prompt*.go --- lib/auth/webauthncli/{fido2_prompt.go => prompt.go} | 0 lib/auth/webauthncli/{fido2_prompt_test.go => prompt_test.go} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename lib/auth/webauthncli/{fido2_prompt.go => prompt.go} (100%) rename lib/auth/webauthncli/{fido2_prompt_test.go => prompt_test.go} (100%) diff --git a/lib/auth/webauthncli/fido2_prompt.go b/lib/auth/webauthncli/prompt.go similarity index 100% rename from lib/auth/webauthncli/fido2_prompt.go rename to lib/auth/webauthncli/prompt.go diff --git a/lib/auth/webauthncli/fido2_prompt_test.go b/lib/auth/webauthncli/prompt_test.go similarity index 100% rename from lib/auth/webauthncli/fido2_prompt_test.go rename to lib/auth/webauthncli/prompt_test.go From 94b48c5284fbde5bd05ad2f3ac900b7ce1e06b05 Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Fri, 8 Jul 2022 15:00:00 -0300 Subject: [PATCH 4/7] Define the touchid credential picker API --- lib/auth/touchid/api.go | 12 ++- lib/auth/touchid/api_test.go | 16 +++- lib/auth/touchid/attempt.go | 4 +- lib/auth/webauthncli/api.go | 8 +- lib/auth/webauthncli/prompt.go | 42 +++++++++ lib/auth/webauthncli/prompt_test.go | 137 ++++++++++++++++++++++++++++ 6 files changed, 207 insertions(+), 12 deletions(-) diff --git a/lib/auth/touchid/api.go b/lib/auth/touchid/api.go index 127e5efc8db99..3bcfa85478d4e 100644 --- a/lib/auth/touchid/api.go +++ b/lib/auth/touchid/api.go @@ -428,10 +428,18 @@ func makeAttestationData(ceremony protocol.CeremonyType, origin, rpID string, ch }, nil } +// CredentialPicker allows users to choose a credential for login. +type CredentialPicker interface { + // PromptCredential prompts the user to pick a credential from the list. + // Prompts only happen if there is more than one credential to choose from. + // Must return one of the pointers from the slice or an error. + PromptCredential(creds []*CredentialInfo) (*CredentialInfo, error) +} + // Login authenticates using a Secure Enclave-backed biometric credential. // It returns the assertion response and the user that owns the credential to // sign it. -func Login(origin, user string, assertion *wanlib.CredentialAssertion) (*wanlib.CredentialAssertionResponse, string, error) { +func Login(origin, user string, assertion *wanlib.CredentialAssertion, picker CredentialPicker) (*wanlib.CredentialAssertionResponse, string, error) { if !IsAvailable() { return nil, "", ErrNotAvailable } @@ -450,6 +458,8 @@ func Login(origin, user string, assertion *wanlib.CredentialAssertion) (*wanlib. return nil, "", errors.New("challenge required") case assertion.Response.RelyingPartyID == "": return nil, "", errors.New("relying party ID required") + case picker == nil: + return nil, "", errors.New("picker required") } rpID := assertion.Response.RelyingPartyID diff --git a/lib/auth/touchid/api_test.go b/lib/auth/touchid/api_test.go index e7d22de29c5a0..a314d2fd32b96 100644 --- a/lib/auth/touchid/api_test.go +++ b/lib/auth/touchid/api_test.go @@ -42,6 +42,12 @@ func init() { touchid.PromptWriter = io.Discard } +type simplePicker struct{} + +func (p simplePicker) PromptCredential(creds []*touchid.CredentialInfo) (*touchid.CredentialInfo, error) { + return creds[0], nil +} + func TestRegisterAndLogin(t *testing.T) { n := *touchid.Native t.Cleanup(func() { @@ -112,7 +118,7 @@ func TestRegisterAndLogin(t *testing.T) { assertion := (*wanlib.CredentialAssertion)(a) test.modifyAssertion(assertion) - assertionResp, actualUser, err := touchid.Login(origin, user, assertion) + assertionResp, actualUser, err := touchid.Login(origin, user, assertion, simplePicker{}) require.NoError(t, err, "Login failed") assert.Equal(t, test.wantUser, actualUser, "actualUser mismatch") assert.Equal(t, 2, fake.userPrompts, "unexpected number of Login prompts") @@ -169,7 +175,7 @@ func TestRegister_rollback(t *testing.T) { RelyingPartyID: web.Config.RPID, UserVerification: "required", }, - }) + }, simplePicker{}) require.Equal(t, touchid.ErrCredentialNotFound, err, "unexpected Login error") } @@ -331,7 +337,7 @@ func TestLogin_findsCorrectCredential(t *testing.T) { RelyingPartyID: web.Config.RPID, AllowedCredentials: allowedCreds, }, - }) + }, simplePicker{}) require.NoError(t, err, "Login failed") wantUser := test.wantUser @@ -398,7 +404,7 @@ func TestLogin_noCredentials_failsWithoutUserInteraction(t *testing.T) { } { t.Run(test.name, func(t *testing.T) { fake.userPrompts = 0 // reset before test - _, _, err := touchid.Login(origin, test.user, test.assertion) + _, _, err := touchid.Login(origin, test.user, test.assertion, simplePicker{}) assert.ErrorIs(t, err, touchid.ErrCredentialNotFound, "Login error mismatch") assert.Zero(t, fake.userPrompts, "Login caused user interaction with no credentials") }) @@ -489,7 +495,7 @@ func TestLogin_noCredentials_failsWithoutUserInteraction(t *testing.T) { } { t.Run(test.name, func(t *testing.T) { fake.userPrompts = 0 // reset before test - _, _, err := touchid.Login(origin, test.user, test.assertion) + _, _, err := touchid.Login(origin, test.user, test.assertion, simplePicker{}) assert.ErrorIs(t, err, touchid.ErrCredentialNotFound, "Login error mismatch") assert.Zero(t, fake.userPrompts, "Login caused user interaction with no credentials") }) diff --git a/lib/auth/touchid/attempt.go b/lib/auth/touchid/attempt.go index f4e277e20854f..ec77de4e80d29 100644 --- a/lib/auth/touchid/attempt.go +++ b/lib/auth/touchid/attempt.go @@ -54,8 +54,8 @@ func (e *ErrAttemptFailed) As(target interface{}) bool { // AttemptLogin attempts a touch ID login. // It returns ErrAttemptFailed if the attempt failed before user interaction. // See Login. -func AttemptLogin(origin, user string, assertion *wanlib.CredentialAssertion) (*wanlib.CredentialAssertionResponse, string, error) { - resp, actualUser, err := Login(origin, user, assertion) +func AttemptLogin(origin, user string, assertion *wanlib.CredentialAssertion, picker CredentialPicker) (*wanlib.CredentialAssertionResponse, string, error) { + resp, actualUser, err := Login(origin, user, assertion, picker) switch { case errors.Is(err, ErrNotAvailable), errors.Is(err, ErrCredentialNotFound): return nil, "", &ErrAttemptFailed{Err: err} diff --git a/lib/auth/webauthncli/api.go b/lib/auth/webauthncli/api.go index 54cb4afce1819..3f6f25ea26c88 100644 --- a/lib/auth/webauthncli/api.go +++ b/lib/auth/webauthncli/api.go @@ -114,10 +114,10 @@ func Login( return crossPlatformLogin(ctx, origin, assertion, prompt, opts) case AttachmentPlatform: log.Debug("Platform login") - return platformLogin(origin, user, assertion) + return platformLogin(origin, user, assertion, prompt) default: log.Debug("Attempting platform login") - resp, credentialUser, err := platformLogin(origin, user, assertion) + resp, credentialUser, err := platformLogin(origin, user, assertion, prompt) if !errors.Is(err, &touchid.ErrAttemptFailed{}) { return resp, credentialUser, trace.Wrap(err) } @@ -141,8 +141,8 @@ func crossPlatformLogin( return resp, "" /* credentialUser */, err } -func platformLogin(origin, user string, assertion *wanlib.CredentialAssertion) (*proto.MFAAuthenticateResponse, string, error) { - resp, credentialUser, err := touchid.AttemptLogin(origin, user, assertion) +func platformLogin(origin, user string, assertion *wanlib.CredentialAssertion, prompt LoginPrompt) (*proto.MFAAuthenticateResponse, string, error) { + resp, credentialUser, err := touchid.AttemptLogin(origin, user, assertion, ToTouchIDCredentialPicker(prompt)) if err != nil { return nil, "", err } diff --git a/lib/auth/webauthncli/prompt.go b/lib/auth/webauthncli/prompt.go index d16daaaa5eaa7..d5f40c30df330 100644 --- a/lib/auth/webauthncli/prompt.go +++ b/lib/auth/webauthncli/prompt.go @@ -22,6 +22,7 @@ import ( "sort" "strconv" + "github.com/gravitational/teleport/lib/auth/touchid" "github.com/gravitational/teleport/lib/utils/prompt" "github.com/gravitational/trace" ) @@ -109,3 +110,44 @@ func (p *DefaultPrompt) PromptCredential(creds []*CredentialInfo) (*CredentialIn fmt.Fprintf(p.out, "Invalid user choice: %q\n", numOrName) } } + +type credentialPicker interface { + PromptCredential([]*CredentialInfo) (*CredentialInfo, error) +} + +// ToTouchIDCredentialPicker adapts a wancli credential picker, such as +// LoginPrompt or DefaultPrompt, to a touchid.CredentialPicker +func ToTouchIDCredentialPicker(p credentialPicker) touchid.CredentialPicker { + return tidPickerAdapter{impl: p} +} + +type tidPickerAdapter struct { + impl credentialPicker +} + +func (p tidPickerAdapter) PromptCredential(creds []*touchid.CredentialInfo) (*touchid.CredentialInfo, error) { + credMap := make(map[*CredentialInfo]*touchid.CredentialInfo) + wcreds := make([]*CredentialInfo, len(creds)) + for i, c := range creds { + cred := &CredentialInfo{ + ID: []byte(c.CredentialID), + User: UserInfo{ + UserHandle: c.User.UserHandle, + Name: c.User.Name, + }, + } + credMap[cred] = c + wcreds[i] = cred + } + + wchoice, err := p.impl.PromptCredential(wcreds) + if err != nil { + return nil, trace.Wrap(err) + } + + choice, ok := credMap[wchoice] + if !ok { + return nil, fmt.Errorf("prompt returned invalid credential: %#v", wchoice) + } + return choice, nil +} diff --git a/lib/auth/webauthncli/prompt_test.go b/lib/auth/webauthncli/prompt_test.go index 7583b0da9f312..44b587c94d4b5 100644 --- a/lib/auth/webauthncli/prompt_test.go +++ b/lib/auth/webauthncli/prompt_test.go @@ -16,12 +16,15 @@ package webauthncli_test import ( "context" + "errors" "strings" "testing" "github.com/gravitational/teleport/lib/utils/prompt" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/gravitational/teleport/lib/auth/touchid" wancli "github.com/gravitational/teleport/lib/auth/webauthncli" ) @@ -124,3 +127,137 @@ func TestDefaultPrompt_PromptCredential(t *testing.T) { }) } } + +type funcToPicker func([]*wancli.CredentialInfo) (*wancli.CredentialInfo, error) + +func (f funcToPicker) PromptCredential(creds []*wancli.CredentialInfo) (*wancli.CredentialInfo, error) { + return f(creds) +} + +func TestToTouchIDCredentialPicker(t *testing.T) { + indexPicker := func(i int) func([]*wancli.CredentialInfo) (*wancli.CredentialInfo, error) { + return func(creds []*wancli.CredentialInfo) (*wancli.CredentialInfo, error) { + return creds[i], nil + } + } + errorPicker := func(err error) func([]*wancli.CredentialInfo) (*wancli.CredentialInfo, error) { + return func(_ []*wancli.CredentialInfo) (*wancli.CredentialInfo, error) { + return nil, err + } + } + bogusPicker := func(resp *wancli.CredentialInfo) func([]*wancli.CredentialInfo) (*wancli.CredentialInfo, error) { + return func(_ []*wancli.CredentialInfo) (*wancli.CredentialInfo, error) { + return resp, nil + } + } + + creds := []*touchid.CredentialInfo{ + { + CredentialID: "id1", + User: touchid.UserInfo{ + UserHandle: []byte("llama"), + Name: "llama", + }, + }, + { + CredentialID: "id2", + User: touchid.UserInfo{ + UserHandle: []byte("alpaca"), + Name: "alpaca", + }, + }, + { + CredentialID: "id3", + User: touchid.UserInfo{ + UserHandle: []byte("camel"), + Name: "camel", + }, + }, + { + CredentialID: "id4", + User: touchid.UserInfo{ + UserHandle: []byte("llama"), + Name: "llama", + }, + }, + } + + tests := []struct { + name string + picker func([]*wancli.CredentialInfo) (*wancli.CredentialInfo, error) + creds []*touchid.CredentialInfo + wantCred *touchid.CredentialInfo + wantErr string + }{ + { + name: "picks first credential", + picker: indexPicker(0), + creds: creds, + wantCred: creds[0], + }, + { + name: "picks middle credential", + picker: indexPicker(1), + creds: creds, + wantCred: creds[1], + }, + { + name: "picks last credential", + picker: indexPicker(3), + creds: creds, + wantCred: creds[3], + }, + { + name: "picker errors", + picker: errorPicker(errors.New("something real bad happened")), + creds: creds, + wantErr: "something real bad happened", + }, + { + name: "picker returns bogus credential", + picker: bogusPicker(&wancli.CredentialInfo{ + // It doesn't matter that the fields match, the pointer is not present + // in the original array. + ID: []byte(creds[0].CredentialID), + User: wancli.UserInfo{ + UserHandle: creds[0].User.UserHandle, + Name: creds[0].User.Name, + }, + }), + creds: creds, + wantErr: "returned invalid credential", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + picker := wancli.ToTouchIDCredentialPicker(funcToPicker(test.picker)) + got, err := picker.PromptCredential(test.creds) + if test.wantErr == "" { + assert.NoError(t, err, "PromptCredential error mismatch") + } else { + assert.ErrorContains(t, err, test.wantErr, "PromptCredential error mismatch") + } + assert.Equal(t, test.wantCred, got, "PromptCredential cred mismatch") + }) + } + + t.Run("credentials converted correctly", func(t *testing.T) { + picker := wancli.ToTouchIDCredentialPicker(funcToPicker( + func(ci []*wancli.CredentialInfo) (*wancli.CredentialInfo, error) { + require.Len(t, ci, len(creds), "creds length mismatch") + for i, c := range ci { + other := creds[i] + + // We are bordering a change detection test here, so let's just check + // the ID and make sure the fields we care about aren't empty. + assert.Equal(t, []byte(other.CredentialID), c.ID, "creds[%v].CredentialID mismatch", i) + assert.NotEmpty(t, c.User.UserHandle, "creds[%v].User.UserHandle empty", i) + assert.NotEmpty(t, c.User.Name, "creds[%v].User.Name empty", i) + } + return ci[0], nil + })) + + _, err := picker.PromptCredential(creds) + require.NoError(t, err, "PromptCredential errored unexpectedly") + }) +} From e2cd49770f003cc971ddad3fa036930137d7174a Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Fri, 8 Jul 2022 19:15:41 -0300 Subject: [PATCH 5/7] Add Touch ID credential picker tests --- lib/auth/touchid/api_test.go | 267 +++++++++++++++++++++++++++++++++++ 1 file changed, 267 insertions(+) diff --git a/lib/auth/touchid/api_test.go b/lib/auth/touchid/api_test.go index a314d2fd32b96..e09c146918ad5 100644 --- a/lib/auth/touchid/api_test.go +++ b/lib/auth/touchid/api_test.go @@ -22,6 +22,7 @@ import ( "crypto/rand" "encoding/json" "errors" + "fmt" "io" "testing" "time" @@ -502,6 +503,272 @@ func TestLogin_noCredentials_failsWithoutUserInteraction(t *testing.T) { } } +type funcToPicker func([]*touchid.CredentialInfo) (*touchid.CredentialInfo, error) + +func (f funcToPicker) PromptCredential(creds []*touchid.CredentialInfo) (*touchid.CredentialInfo, error) { + return f(creds) +} + +func pickByName(name string) func([]*touchid.CredentialInfo) (*touchid.CredentialInfo, error) { + return func(creds []*touchid.CredentialInfo) (*touchid.CredentialInfo, error) { + for _, c := range creds { + if c.User.Name == name { + return c, nil + } + } + return nil, fmt.Errorf("user %v not found", name) + } +} + +func TestLogin_credentialPicker(t *testing.T) { + n := *touchid.Native + t.Cleanup(func() { + *touchid.Native = n + }) + + // Use monotonically-increasing time. + // Newer credentials are preferred. + var timeCounter int64 + fake := &fakeNative{ + timeNow: func() time.Time { + timeCounter++ + return time.Unix(timeCounter, 0) + }, + } + *touchid.Native = fake + + const rpID = "goteleport.com" + const origin = "https://goteleport.com" + baseAssertion := &wanlib.CredentialAssertion{ + Response: protocol.PublicKeyCredentialRequestOptions{ + Challenge: []byte{1, 2, 3, 4, 5}, // arbitrary + RelyingPartyID: rpID, + UserVerification: protocol.VerificationRequired, + }, + } + newAssertion := func(allowedCreds [][]byte) *wanlib.CredentialAssertion { + cp := *baseAssertion + for _, id := range allowedCreds { + cp.Response.AllowedCredentials = append(cp.Response.AllowedCredentials, protocol.CredentialDescriptor{ + Type: protocol.PublicKeyCredentialType, + CredentialID: id, + }) + } + return &cp + } + + // Test results vary depending on registered credentials, so instead of a + // single table we'll build scenarios little-by-litte. + type pickerTest struct { + name string + allowedCreds [][]byte + user string + picker func([]*touchid.CredentialInfo) (*touchid.CredentialInfo, error) + wantID string + wantUser string + } + runTests := func(t *testing.T, tests []pickerTest) { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + fake.userPrompts = 0 // reset before test + + assertion := newAssertion(test.allowedCreds) + picker := funcToPicker(test.picker) + + resp, actualUser, err := touchid.Login(origin, test.user, assertion, picker) + require.NoError(t, err, "Login failed") + assert.Equal(t, test.wantID, resp.ID, "credential ID mismatch") + assert.Equal(t, test.wantUser, actualUser, "user mismatch") + assert.Equal(t, 1, fake.userPrompts, "Login prompted an unexpected number of times") + }) + } + } + + const llamaUser = "llama" + llamaHandle := []byte{1, 1, 1, 1, 1} + const alpacaUser = "alpaca" + alpacaHandle := []byte{1, 1, 1, 1, 2} + const camelUser = "camel" + camelHandle := []byte{1, 1, 1, 1, 3} + + // Single user, single credential. + llama1, err := fake.Register(rpID, llamaUser, llamaHandle) + require.NoError(t, err) + runTests(t, []pickerTest{ + { + name: "single user, single credential, empty user", + wantID: llama1.CredentialID, + wantUser: llamaUser, + }, + { + name: "single user, single credential, explicit user", + user: llamaUser, + wantID: llama1.CredentialID, + wantUser: llamaUser, + }, + { + name: "MFA single credential", + allowedCreds: [][]byte{ + []byte(llama1.CredentialID), + }, + user: llamaUser, + wantID: llama1.CredentialID, + wantUser: llamaUser, + }, + }) + + // Single user, multi credentials. + llama2, err := fake.Register(rpID, llamaUser, llamaHandle) + _ = llama2 // unused apart from registration + require.NoError(t, err) + llama3, err := fake.Register(rpID, llamaUser, llamaHandle) + require.NoError(t, err) + runTests(t, []pickerTest{ + { + name: "single user, multi credential, empty user", + wantID: llama3.CredentialID, // latest registered credential + wantUser: llamaUser, + }, + { + name: "single user, multi credential, explicit user", + user: llamaUser, + wantID: llama3.CredentialID, + wantUser: llamaUser, + }, + }) + + // Multi user, multi credentials. + alpaca1, err := fake.Register(rpID, alpacaUser, alpacaHandle) + require.NoError(t, err) + camel1, err := fake.Register(rpID, camelUser, camelHandle) + require.NoError(t, err) + camel2, err := fake.Register(rpID, camelUser, camelHandle) + require.NoError(t, err) + runTests(t, []pickerTest{ + { + name: "multi user, multi credential, explicit user (1)", + user: llamaUser, + wantID: llama3.CredentialID, // latest credential for llama + wantUser: llamaUser, + }, + { + name: "multi user, multi credential, explicit user (2)", + user: camelUser, + wantID: camel2.CredentialID, // latest credential for camel + wantUser: camelUser, + }, + { + name: "credential picker (1)", + picker: pickByName(llamaUser), + wantID: llama3.CredentialID, + wantUser: llamaUser, + }, + { + name: "credential picker (2)", + picker: pickByName(alpacaUser), + wantID: alpaca1.CredentialID, + wantUser: alpacaUser, + }, + { + name: "MFA multiple credentials (1)", + allowedCreds: [][]byte{ + []byte(llama1.CredentialID), + []byte(camel1.CredentialID), + }, + user: llamaUser, + wantID: llama1.CredentialID, + wantUser: llamaUser, + }, + { + name: "MFA multiple credentials (2)", + allowedCreds: [][]byte{ + []byte(llama1.CredentialID), + []byte(camel1.CredentialID), + }, + user: camelUser, + wantID: camel1.CredentialID, + wantUser: camelUser, + }, + }) + + // Verify that deduping is working as expected. + // Tests above already cover all users. + t.Run("number of credentials is correct", func(t *testing.T) { + picker := funcToPicker(func(creds []*touchid.CredentialInfo) (*touchid.CredentialInfo, error) { + // 3 = llama + alpaca + camel + assert.Len(t, creds, 3, "unexpected number of picker credentials") + return pickByName(llamaUser)(creds) + }) + + _, actualUser, err := touchid.Login(origin, "" /* user */, baseAssertion, picker) + assert.NoError(t, err, "Login failed") + assert.Equal(t, llamaUser, actualUser, "Login user mismatch") + }) + + errUnexpected := errors.New("the llamas escaped") + + // Finally, let's take advantage of the complete setup and run a few error + // tests. + for _, test := range []struct { + name string + allowedCreds [][]byte + user string + picker func([]*touchid.CredentialInfo) (*touchid.CredentialInfo, error) + // At least one of wantErr or wantMsg should be supplied. + wantErr error + wantMsg string + }{ + { + name: "credential picker error", + picker: func(creds []*touchid.CredentialInfo) (*touchid.CredentialInfo, error) { + return nil, errUnexpected + }, + wantErr: errUnexpected, + }, + { + name: "credential picker returns bad pointer", + picker: func(creds []*touchid.CredentialInfo) (*touchid.CredentialInfo, error) { + // Returned pointer not part of creds. + return &touchid.CredentialInfo{ + CredentialID: creds[0].CredentialID, + User: creds[0].User, + }, nil + }, + wantMsg: "returned invalid credential", + }, + { + name: "unknown user requested", + user: "whoami", + wantErr: touchid.ErrCredentialNotFound, + }, + { + name: "MFA no credentials allowed", + allowedCreds: [][]byte{ + []byte("notme"), + []byte("alsonotme"), + }, + user: llamaUser, + wantErr: touchid.ErrCredentialNotFound, + }, + } { + t.Run(test.name, func(t *testing.T) { + require.True(t, test.wantErr != nil || test.wantMsg != "", "Sanity check failed") + + assertion := newAssertion(test.allowedCreds) + picker := funcToPicker(test.picker) + + _, _, err := touchid.Login(origin, test.user, assertion, picker) + require.Error(t, err, "Login succeeded unexpectedly") + if test.wantErr != nil { + assert.ErrorIs(t, err, test.wantErr, "Login error mismatch") + } + if test.wantMsg != "" { + assert.ErrorContains(t, err, test.wantMsg, "Login error mismatch") + } + }) + } +} + type credentialHandle struct { rpID, user string id string From 2aa0297ebad569ed8f492ed411fbfee1725d4e5e Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Fri, 8 Jul 2022 18:14:49 -0300 Subject: [PATCH 6/7] Implement touchid credential picker --- lib/auth/touchid/api.go | 100 ++++++++++++++++++++++++++++++---------- 1 file changed, 76 insertions(+), 24 deletions(-) diff --git a/lib/auth/touchid/api.go b/lib/auth/touchid/api.go index 3bcfa85478d4e..117df5430fd59 100644 --- a/lib/auth/touchid/api.go +++ b/lib/auth/touchid/api.go @@ -479,29 +479,34 @@ func Login(origin, user string, assertion *wanlib.CredentialAssertion, picker Cr return i1.CreateTime.After(i2.CreateTime) }) - // Verify infos against allowed credentials, if any. - cred, ok := findAllowedCredential(infos, assertion.Response.AllowedCredentials) - if !ok { - return nil, "", ErrCredentialNotFound - } - - // Guard first read of chosen credential with an explicit check. - // A more meaningful check can be made once the credential picker is - // implemented. + // Prepare authentication context and prompt for the credential picker. actx := native.NewAuthContext() defer actx.Close() - promptPlatform() - if err := actx.Guard(func() { - log.Debugf("Touch ID: using credential %q", cred.CredentialID) - }); err != nil { + + var prompted bool + promptOnce := func() { + if prompted { + return + } + promptPlatform() + prompted = true + } + + cred, err := pickCredential( + actx, + infos, assertion.Response.AllowedCredentials, + picker, promptOnce, user != "" /* userRequested */) + if err != nil { return nil, "", trace.Wrap(err) } + log.Debugf("Touch ID: using credential %q", cred.CredentialID) attData, err := makeAttestationData(protocol.AssertCeremony, origin, rpID, assertion.Response.Challenge, nil /* cred */) if err != nil { return nil, "", trace.Wrap(err) } + promptOnce() // In case the picker prompt didn't happen. sig, err := native.Authenticate(actx, cred.CredentialID, attData.digest) if err != nil { return nil, "", trace.Wrap(err) @@ -526,21 +531,68 @@ func Login(origin, user string, assertion *wanlib.CredentialAssertion, picker Cr }, cred.User.Name, nil } -func findAllowedCredential(infos []CredentialInfo, allowedCredentials []protocol.CredentialDescriptor) (CredentialInfo, bool) { - if len(infos) > 0 && len(allowedCredentials) == 0 { - // Default to "first" credential for passwordless - return infos[0], true - } - - for _, info := range infos { - for _, cred := range allowedCredentials { - if info.CredentialID == string(cred.CredentialID) { - return info, true +func pickCredential( + actx AuthContext, + infos []CredentialInfo, allowedCredentials []protocol.CredentialDescriptor, + picker CredentialPicker, promptOnce func(), userRequested bool) (*CredentialInfo, error) { + // Handle early exits. + switch l := len(infos); { + // MFA. + case len(allowedCredentials) > 0: + for _, info := range infos { + for _, cred := range allowedCredentials { + if info.CredentialID == string(cred.CredentialID) { + return &info, nil + } } } + return nil, ErrCredentialNotFound + + // Single credential or specific user requested. + // A requested user means that all credentials are for that user, so there + // would be nothing to pick. + case l == 1 && userRequested: + return &infos[0], nil + } + + // Dedup users to avoid confusion. + // This assumes credentials are sorted from most to less preferred. + knownUsers := make(map[string]struct{}) + deduped := make([]*CredentialInfo, 0, len(infos)) + for _, c := range infos { + if _, ok := knownUsers[c.User.Name]; ok { + continue + } + knownUsers[c.User.Name] = struct{}{} + + c := c // Avoid capture-by-reference errors + deduped = append(deduped, &c) + } + if len(deduped) == 1 { + return deduped[0], nil + } + + promptOnce() + var choice *CredentialInfo + var choiceErr error + if err := actx.Guard(func() { + choice, choiceErr = picker.PromptCredential(deduped) + }); err != nil { + return nil, trace.Wrap(err) + } + if choiceErr != nil { + return nil, trace.Wrap(choiceErr) } - return CredentialInfo{}, false + // Is choice a pointer within the slice? + // We could work around this requirement, but it seems better to constrain the + // picker API from the start. + for _, c := range deduped { + if c == choice { + return choice, nil + } + } + return nil, fmt.Errorf("picker returned invalid credential: %#v", choice) } // ListCredentials lists all registered Secure Enclave credentials. From 2112a122471171861a0056efb63b651ad37088ea Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Mon, 18 Jul 2022 14:14:04 -0300 Subject: [PATCH 7/7] Review: Correctly short-circuit explicit user requests --- lib/auth/touchid/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/auth/touchid/api.go b/lib/auth/touchid/api.go index 117df5430fd59..a1a5ab71205d9 100644 --- a/lib/auth/touchid/api.go +++ b/lib/auth/touchid/api.go @@ -551,7 +551,7 @@ func pickCredential( // Single credential or specific user requested. // A requested user means that all credentials are for that user, so there // would be nothing to pick. - case l == 1 && userRequested: + case l == 1 || userRequested: return &infos[0], nil }