diff --git a/go.mod b/go.mod index 1589161790b24..374a4a161310d 100644 --- a/go.mod +++ b/go.mod @@ -406,7 +406,7 @@ replace ( github.com/gogo/protobuf => github.com/gravitational/protobuf v1.3.2-teleport.1 github.com/gravitational/teleport/api => ./api github.com/julienschmidt/httprouter => github.com/gravitational/httprouter v1.3.1-0.20220408074523-c876c5e705a5 - github.com/keys-pub/go-libfido2 => github.com/gravitational/go-libfido2 v1.5.3-0.20230202181331-c71192ef1c8a + github.com/keys-pub/go-libfido2 => github.com/gravitational/go-libfido2 v1.5.3-teleport.1 github.com/microsoft/go-mssqldb => github.com/gravitational/go-mssqldb v0.11.1-0.20230331180905-0f76f1751cd3 // replace module github.com/moby/spdystream until https://github.com/moby/spdystream/pull/91 merges and deps are updated // otherwise tests fail with a data race detection. diff --git a/go.sum b/go.sum index 3fe8f9c5eeb94..d9ef7327d1d8f 100644 --- a/go.sum +++ b/go.sum @@ -680,8 +680,8 @@ github.com/gravitational/form v0.0.0-20151109031454-c4048f792f70 h1:To76nCJtM3DI github.com/gravitational/form v0.0.0-20151109031454-c4048f792f70/go.mod h1:88hFR45MpUd23d2vNWE/dYtesU50jKsbz0I9kH7UaBY= github.com/gravitational/go-cassandra-native-protocol v0.0.0-20221005103706-b9e66c056e90 h1:fPNJE2kaWC0Oy2YKxk1tbnqhKl3aTeXVAfjXzphJmI8= github.com/gravitational/go-cassandra-native-protocol v0.0.0-20221005103706-b9e66c056e90/go.mod h1:6FzirJfdffakAVqmHjwVfFkpru/gNbIazUOK5rIhndc= -github.com/gravitational/go-libfido2 v1.5.3-0.20230202181331-c71192ef1c8a h1:Wudq53GAiVk1Z+4A1tFzyvidhA6X7rGDb5JKGU9NV0c= -github.com/gravitational/go-libfido2 v1.5.3-0.20230202181331-c71192ef1c8a/go.mod h1:92J9LtSBl0UyUWljElJpTbMMNhC6VeY8dshsu40qjjo= +github.com/gravitational/go-libfido2 v1.5.3-teleport.1 h1:nPfxiTH2Sr3J6zan280fbHOkWE7gRF/lMqvhcXKh2ek= +github.com/gravitational/go-libfido2 v1.5.3-teleport.1/go.mod h1:92J9LtSBl0UyUWljElJpTbMMNhC6VeY8dshsu40qjjo= github.com/gravitational/go-mssqldb v0.11.1-0.20230331180905-0f76f1751cd3 h1:3JGTacvAeV5tIC4/9XsdLC2K5K7FWaXxIwpW4t+dGH0= github.com/gravitational/go-mssqldb v0.11.1-0.20230331180905-0f76f1751cd3/go.mod h1:LbRWqr72fXehxAGLXO8nDNQAi4gthRz4j7f8L2LS0XM= github.com/gravitational/go-mysql v1.5.0-teleport.1 h1:EyFryeiTYyP6KslLVp0Q5QTKwtUG5RioVrTIoP4pOuI= diff --git a/lib/auth/webauthncli/fido2.go b/lib/auth/webauthncli/fido2.go index a37dff6ab47b0..9d2235bbbc75d 100644 --- a/lib/auth/webauthncli/fido2.go +++ b/lib/auth/webauthncli/fido2.go @@ -40,6 +40,20 @@ import ( wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes" ) +const ( + // Max wait time for closing devices, before "abandoning" the device + // goroutine. + fido2DeviceMaxWait = 100 * time.Millisecond + + // Timeout for blocking operations. + // Functions fail with FIDO_ERR_RX on timeout. + fido2DeviceTimeout = 30 * time.Second + + // Operation retry interval. + // Keep it less frequent than 2Hz / 0.5s. + fido2RetryInterval = 500 * time.Millisecond +) + // User-friendly device filter errors. var ( errHasExcludedCredential = errors.New("device already holds a registered credential") @@ -56,9 +70,18 @@ type FIDODevice interface { // Info mirrors libfido2.Device.Info. Info() (*libfido2.DeviceInfo, error) + // IsFIDO2 mirrors libfido2.Device.IsFIDO2. + IsFIDO2() (bool, error) + // Cancel mirrors libfido2.Device.Cancel. Cancel() error + // Close mirrors libfido2.Device.Close. + Close() error + + // SetTimeout mirrors libfido2.Device.SetTimeout. + SetTimeout(d time.Duration) error + // MakeCredential mirrors libfido2.Device.MakeCredential. MakeCredential( clientDataHash []byte, @@ -143,7 +166,7 @@ func fido2Login( pathToRPID := &sync.Map{} // map[string]string filter := func(dev FIDODevice, info *deviceInfo) error { switch { - case info.u2f && (uv || passwordless): + case !info.fido2 && (uv || passwordless): return errPasswordlessU2F case passwordless && (!info.uvCapable() || !info.rk): return errNoPasswordless @@ -156,8 +179,6 @@ func fido2Login( return nil } - // TODO(codingllama): Kill discoverRPID? It makes behavioral assumptions - // that caused problems before. // Does the device have a suitable credential? const pin = "" actualRPID, err := discoverRPID(dev, info, pin, rpID, appID, allowedCreds) @@ -255,6 +276,10 @@ func discoverRPID(dev FIDODevice, info *deviceInfo, pin, rpID, appID string, all // The actual hash is not necessary here. const cdh = "00000000000000000000000000000000" + // TODO(codingllama): We could cut an assertion here by checking just for + // appID, if it's not empty, and assuming it's rpID otherwise. + // This moves certain "no credentials" handling from the "filter" step to the + // "callback" step, which has a few knock-on effects in the code. opts := &libfido2.AssertionOpts{ UP: libfido2.False, } @@ -395,7 +420,7 @@ func fido2Register( filter := func(dev FIDODevice, info *deviceInfo) error { switch { - case info.u2f && (rrk || uv): + case !info.fido2 && (rrk || uv): return errPasswordlessU2F case plat && !info.plat: return errNoPlatform @@ -537,11 +562,6 @@ func makeAttStatement(attestation *libfido2.Attestation) (string, map[string]int return format, m, nil } -type deviceWithInfo struct { - FIDODevice - info *deviceInfo -} - type ( deviceFilterFunc func(dev FIDODevice, info *deviceInfo) error deviceCallbackFunc func(dev FIDODevice, info *deviceInfo, pin string) error @@ -558,54 +578,283 @@ func runOnFIDO2Devices( filter deviceFilterFunc, deviceCallback deviceCallbackFunc, ) error { - // About to select, prompt user. + locs, err := fidoDeviceLocations() + if err != nil { + return trace.Wrap(err, "device locations") + } + if len(locs) == 0 { + return trace.Wrap(errors.New("no security keys found")) + } + + devices, devicesC, err := startDevices(locs, filter, deviceCallback, prompt) + if err != nil { + return trace.Wrap(err) + } + + var receiveCount int + defer func() { + // Cancel all in-flight requests, if any. + devices.cancelAll(nil /* except */) + + // Give the devices some time to tidy up, but don't wait forever. + maxWait := time.NewTimer(fido2DeviceMaxWait) + defer maxWait.Stop() + + for receiveCount < devices.len() { + select { + case <-devicesC: + receiveCount++ + case <-maxWait.C: + log.Debugf("FIDO2: Abandoning device goroutines after %s", fido2DeviceMaxWait) + return + } + } + log.Debug("FIDO2: Device goroutines exited cleanly") + }() + + // First "interactive" response wins. + for receiveCount < devices.len() { + select { + case err := <-devicesC: + receiveCount++ + + // Keep going on cancels or non-interactive errors. + if errors.Is(err, libfido2.ErrKeepaliveCancel) || errors.Is(err, &nonInteractiveError{}) { + log.Debugf("FIDO2: Got cancel or non-interactive device error: %v", err) + continue + } + + return trace.Wrap(err) + + case <-ctx.Done(): + return trace.Wrap(ctx.Err()) + } + } + return trace.Wrap(errors.New("all MFA devices failed")) +} + +func startDevices( + locs []*libfido2.DeviceLocation, + filter deviceFilterFunc, + deviceCallback deviceCallbackFunc, + prompt runPrompt, +) (devices *openedDevices, devicesC <-chan error, err error) { + fidoDevs := make([]FIDODevice, 0, len(locs)) + openDevs := make([]*openedDevice, 0, len(locs)) + + // closeAll should only be used until the devices are handed over. + // Do not defer-call it. + closeAll := func() { + for i, dev := range fidoDevs { + path := openDevs[i].path + err := dev.Close() + log.Debugf("FIDO2: Close device %v, err=%v", path, err) + } + } + + // Open all devices in one go. + // This ensures cancels propagate to the complete list. + for _, loc := range locs { + path := loc.Path + + dev, err := fidoNewDevice(path) + if err != nil { + closeAll() + return nil, nil, trace.Wrap(err, "device open") + } + + fidoDevs = append(fidoDevs, dev) + openDevs = append(openDevs, &openedDevice{ + path: path, + dev: dev, + }) + } + + // Prompt touch, it's about to begin. ackTouch, err := prompt.PromptTouch() if err != nil { + closeAll() + return nil, nil, trace.Wrap(err) + } + //nolint:ineffassign // closeAll not meant to be used from here onwards. + closeAll = nil + + errC := make(chan error, len(fidoDevs)) + devices = &openedDevices{ + devices: openDevs, + } + + // Fire device handling goroutines. + // From this point onwards devices are owned by their respective goroutines, + // only cancels are supposed to happen outside of them. + for i, dev := range fidoDevs { + path := openDevs[i].path + dev := dev + go func() { + errC <- handleDevice(path, dev, filter, deviceCallback, devices.cancelAll, ackTouch, prompt) + }() + } + + return devices, errC, nil +} + +type openedDevice struct { + path string + + // dev is the opened device. + // Only cancels may be issued outside of the handleDevice goroutine. + dev interface{ Cancel() error } + + // Keep tabs on canceled devices to avoid multiple cancels. + canceled bool +} + +type openedDevices struct { + // mu guards device changes and cancelAll(). + // Note that the size of the devices slice doesn't change after it's assigned, + // only the `canceled` device field changes. + mu sync.Mutex + devices []*openedDevice +} + +func (l *openedDevices) len() int { + // Safe to read without locking, the size of the slice doesn't change after + // assigned. + return len(l.devices) +} + +// cancelAll cancels all devices but `except`. +func (l *openedDevices) cancelAll(except FIDODevice) { + l.mu.Lock() + defer l.mu.Unlock() + + for _, d := range l.devices { + if d.dev == except || d.canceled { + continue + } + + d.canceled = true + + // Note that U2F devices fail Cancel with "invalid argument". + err := d.dev.Cancel() + log.Debugf("FIDO2: Cancel device %v, err=%v", d.path, err) + } +} + +// handleDevice handles all device interactions, apart from external cancels. +func handleDevice( + path string, + dev FIDODevice, + filter deviceFilterFunc, deviceCallback deviceCallbackFunc, + cancelAll func(except FIDODevice), + firstTouchAck func() error, + pinPrompt runPrompt, +) error { + // handleDevice owns the device, thus it has the privilege to shut it down. + defer func() { + err := dev.Close() + log.Debugf("FIDO2: Close device %v, err=%v", path, err) + }() + + if err := dev.SetTimeout(fido2DeviceTimeout); err != nil { + return trace.Wrap(&nonInteractiveError{err}) + } + + // Gather device information. + var info *libfido2.DeviceInfo + isFIDO2, err := dev.IsFIDO2() + if err != nil { + return trace.Wrap(&nonInteractiveError{err: err}) + } + if isFIDO2 { + info, err = devInfo(path, dev) + if err != nil { + return trace.Wrap(&nonInteractiveError{err: err}) + } + log.Debugf("FIDO2: Device %v: info %#v", path, info) + } else { + log.Debugf("FIDO2: Device %v: not a FIDO2 device", path) + } + di := makeDevInfo(path, info, isFIDO2) + + // Apply initial filters, waiting for confirmation if the filter fails before + // relaying the error. + if err := filter(dev, di); err != nil { + log.Debugf("FIDO2: Device %v filtered, err=%v", path, err) + + // If the device is chosen then treat the error as interactive. + if waitErr := waitForTouch(dev); errors.Is(waitErr, libfido2.ErrNoCredentials) { + cancelAll(dev) + + // Escalate error to ErrUsingNonRegisteredDevice, if appropriate, so we + // send a better message to the user. + if errors.Is(err, errNoRegisteredCredentials) { + err = ErrUsingNonRegisteredDevice + } + + } else { + err = &nonInteractiveError{err: err} + } return trace.Wrap(err) } - // List/select devices. + // Run the callback. cb := withPINHandler(withRetries(deviceCallback)) - dev, requiresPIN, err := findAndSelectDevice(ctx, filter, cb) + requiresPIN, err := cb(dev, di, "" /* pin */) + log.Debugf("FIDO2: Device %v: callback returned, requiresPIN=%v, err=%v", path, requiresPIN, err) if err != nil { return trace.Wrap(err) } - if err := ackTouch(); err != nil { + if err := firstTouchAck(); err != nil { return trace.Wrap(err) } + // Cancel other devices only on success. This avoids multiple cancel attempts + // as non-chosen devices return FIDO_ERR_KEEPALIVE_CANCEL. + cancelAll(dev) + if !requiresPIN { return nil } - // Selected device requires PIN, let's use the prompt and run the callback - // again. - pin, err := prompt.PromptPIN() + // Ask for PIN, prompt for next touch. + pin, err := pinPrompt.PromptPIN() switch { case err != nil: return trace.Wrap(err) case pin == "": return libfido2.ErrPinRequired } - - // Prompt a second touch after reading the PIN. - ackTouch, err = prompt.PromptTouch() + ackTouch, err := pinPrompt.PromptTouch() if err != nil { return trace.Wrap(err) } - // Run the callback again with the informed PIN. - // selectDevice is used since it correctly deals with cancellation. cb = withoutPINHandler(withRetries(deviceCallback)) - _, err = selectDevice(ctx, pin, dev, cb) - if err != nil { + if _, err := cb(dev, di, pin); err != nil { return trace.Wrap(err) } - return trace.Wrap(ackTouch()) } +func devInfo(path string, dev FIDODevice) (*libfido2.DeviceInfo, error) { + const infoAttempts = 3 + var lastErr error + for i := 0; i < infoAttempts; i++ { + info, err := dev.Info() + if err == nil { + return info, nil + } + + lastErr = err + log.Debugf("FIDO2: Device %v: Info failed, retrying after %s: %v", path, fido2RetryInterval, err) + time.Sleep(fido2RetryInterval) + } + + return nil, trace.Wrap(lastErr) +} + // withRetries wraps callback with retries and error handling for commonly seen // errors. func withRetries(callback deviceCallbackFunc) deviceCallbackFunc { @@ -703,42 +952,6 @@ func (e *nonInteractiveError) Is(err error) bool { return ok } -func withInteractiveError(filter deviceFilterFunc, cb pinAwareCallbackFunc) pinAwareCallbackFunc { - return func(dev FIDODevice, info *deviceInfo, pin string) (bool, error) { - filterErr := filter(dev, info) - if filterErr == nil { - return cb(dev, info, pin) - } - - // U2F devices tend to cause problems with the waitForTouch strategy below, - // so we filter them silently, as we used to do with all devices in previous - // versions. - if info.u2f { - log.Warnf("FIDO2: Device %v: U2F device filtered due to lack of capabilities", info.path) - return false, &nonInteractiveError{filterErr} - } - - // Device got filtered out, let's see if the user chooses it and provide a - // nice error message. - switch waitErr := waitForTouch(dev); { - case errors.Is(waitErr, libfido2.ErrKeepaliveCancel): - // Device not chosen. - return false, &nonInteractiveError{filterErr} - case errors.Is(waitErr, libfido2.ErrNoCredentials): - // Device chosen. - // Escalate error to ErrUsingNonRegisteredDevice, if appropriate, so we - // send a better message to the user. - if errors.Is(filterErr, errNoRegisteredCredentials) { - filterErr = ErrUsingNonRegisteredDevice - } - default: - log.Warnf("FIDO2: Device %v: unexpected wait error: %q", info.path, waitErr) - } - - return false, trace.Wrap(filterErr) - } -} - func waitForTouch(dev FIDODevice) error { // TODO(codingllama): What we really want here is fido_dev_get_touch_begin. const rpID = "7f364cc0-958c-4177-b3ea-b2d8d7f15d4a" // arbitrary, unlikely to collide with a real RP @@ -749,209 +962,12 @@ func waitForTouch(dev FIDODevice) error { return err } -func findAndSelectDevice(ctx context.Context, filter deviceFilterFunc, deviceCallback pinAwareCallbackFunc) (dev *deviceWithInfo, requiresPIN bool, err error) { - innerCtx, innerCancel := context.WithCancel(ctx) - // innerCancel handled below. - - type devicesResp struct { - devs []*deviceWithInfo - err error - } - // devicesC transport newly-found devices. - // Closed on exit by the device poll goroutine below. - devicesC := make(chan devicesResp) - - // Poll for new devices until the user selects one (via touch). - // Runs until innerCtx is closed. - go func() { - defer close(devicesC) - - // knownPaths is retained between findDevices calls so only "new" devices - // are returned. - knownPaths := make(map[string]struct{}) - - ticker := time.NewTicker(FIDO2PollInterval) - defer ticker.Stop() - - for { - devs, err := findDevices(knownPaths) - devicesC <- devicesResp{ - devs: devs, - err: err, - } - - select { - case <-innerCtx.Done(): - return - case <-ticker.C: - } - } - }() - - type selectResp struct { - dev *deviceWithInfo - requiresPIN bool - err error - } - selectC := make(chan selectResp) - selectGoroutines := 0 - - defer func() { - innerCancel() // Cancel all goroutines - - // Collect select goroutines. - for selectGoroutines > 0 { - <-selectC - selectGoroutines-- - } - - // Empty devicesC, if blocked. - for { - if _, open := <-devicesC; !open { - break - } - } - }() - - cb := withInteractiveError(filter, deviceCallback) - for { - select { - // New devices found. - case resp := <-devicesC: - if resp.err != nil { - return nil, false, trace.Wrap(resp.err) - } - for _, dev := range resp.devs { - dev := dev - selectGoroutines++ - go func() { - requiresPIN, err := selectDevice(innerCtx, "" /* pin */, dev, cb) - selectC <- selectResp{ - dev: dev, - requiresPIN: requiresPIN, - err: err, - } - }() - } - - // User selected device. - case resp := <-selectC: - selectGoroutines-- - if errors.Is(resp.err, &nonInteractiveError{}) { - continue - } - return resp.dev, resp.requiresPIN, trace.Wrap(resp.err) - - // Timed out. - case <-ctx.Done(): - return nil, false, trace.Wrap(ctx.Err()) - } - } -} - -func findDevices(knownPaths map[string]struct{}) ([]*deviceWithInfo, error) { - locs, err := fidoDeviceLocations() - if err != nil { - return nil, trace.Wrap(err, "device locations") - } - - var devs []*deviceWithInfo - for _, loc := range locs { - path := loc.Path - if _, ok := knownPaths[path]; ok { - continue - } - knownPaths[path] = struct{}{} - - dev, err := fidoNewDevice(path) - if err != nil { - return nil, trace.Wrap(err, "device %v: open", path) - } - - var info *libfido2.DeviceInfo - var u2f bool - const infoAttempts = 3 - for i := 0; i < infoAttempts; i++ { - info, err = dev.Info() - switch { - case errors.Is(err, libfido2.ErrNotFIDO2): - u2f = true - case errors.Is(err, libfido2.ErrTX): - // Happens occasionally, let's retry. - fallthrough - case err != nil: - // Unexpected error, retry anyway. - // Note that U2F devices fail in a variety of different ways. - time.Sleep(100 * time.Millisecond) - continue - } - break // err == nil - } - if !u2f && info == nil { - log.Warnf("FIDO2: Device %v: max info attempts reached, treating as U2F", path) - u2f = true - } - log.Debugf("FIDO2: Info for device %v: %#v", path, info) - - devs = append(devs, &deviceWithInfo{ - FIDODevice: dev, - info: makeDevInfo(path, info, u2f), - }) - } - - if l := len(devs); l > 0 { - log.Debugf("FIDO2: Found %v new devices", l) - } - - return devs, nil -} - -func selectDevice( - ctx context.Context, - pin string, dev *deviceWithInfo, cb pinAwareCallbackFunc, -) (requiresPIN bool, err error) { - // Spin a goroutine to run the callback so we can deal with context - // cancellation. - done := make(chan struct{}) - go func() { - requiresPIN, err = cb(dev, dev.info, pin) - close(done) - }() - - select { - case <-done: - log.Debugf("FIDO2: device %v: selected with err=%v", dev.info.path, err) - case <-ctx.Done(): - log.Debugf("FIDO2: device %v: requesting cancel", dev.info.path) - if err := dev.Cancel(); err != nil { - log.Debugf("FIDO2: device %v: cancel errored: %v", dev.info.path, err) - } - - // Give the device a grace period to cancel/cleanup, but do not wait - // forever. - timer := time.NewTimer(500 * time.Millisecond) - defer timer.Stop() - select { - case <-done: - case <-timer.C: - log.Warnf("FIDO2: " + - "Timed out waiting for device cancels. " + - "It's possible some devices are left blinking.") - } - - return false, trace.Wrap(ctx.Err()) - } - - // Returns variables captured by goroutine. - return -} - // deviceInfo contains an aggregate of a device's information and capabilities. // Various fields match options under // https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#authenticatorGetInfo. type deviceInfo struct { path string - u2f bool + fido2 bool plat bool rk bool clientPinCapable, clientPinSet bool @@ -964,14 +980,14 @@ func (di *deviceInfo) uvCapable() bool { return di.uv || di.clientPinSet } -func makeDevInfo(path string, info *libfido2.DeviceInfo, u2f bool) *deviceInfo { +func makeDevInfo(path string, info *libfido2.DeviceInfo, fido2 bool) *deviceInfo { di := &deviceInfo{ - path: path, - u2f: u2f, + path: path, + fido2: fido2, } // U2F devices don't respond to dev.Info(). - if u2f { + if !fido2 { return di } diff --git a/lib/auth/webauthncli/fido2_test.go b/lib/auth/webauthncli/fido2_test.go index 558b9b409093a..e0c3dfa3ec1c7 100644 --- a/lib/auth/webauthncli/fido2_test.go +++ b/lib/auth/webauthncli/fido2_test.go @@ -140,7 +140,7 @@ func (p *pinCancelPrompt) PromptPIN() (string, error) { return p.pin, nil } -func (p pinCancelPrompt) PromptTouch() (wancli.TouchAcknowledger, error) { +func (p *pinCancelPrompt) PromptTouch() (wancli.TouchAcknowledger, error) { // 2nd touch never happens return func() error { return nil }, nil } @@ -155,7 +155,7 @@ func TestIsFIDO2Available(t *testing.T) { { name: "env var unset", setenv: func() { - os.Unsetenv(fido2Key) + _ = os.Unsetenv(fido2Key) }, want: true, }, @@ -407,10 +407,9 @@ func TestFIDO2Login(t *testing.T) { }, }, { - name: "NOK no devices plugged times out", - timeout: 10 * time.Millisecond, - fido2: newFakeFIDO2(), - setUP: func() {}, + name: "NOK no devices plugged errors", + fido2: newFakeFIDO2(), + setUP: func() {}, createAssertion: func() *wantypes.CredentialAssertion { cp := *baseAssertion cp.Response.AllowedCredentials = []wantypes.CredentialDescriptor{ @@ -418,7 +417,7 @@ func TestFIDO2Login(t *testing.T) { } return &cp }, - wantErr: context.DeadlineExceeded.Error(), + wantErr: "no security keys found", }, { name: "NOK no devices touched times out", @@ -585,8 +584,8 @@ func TestFIDO2Login(t *testing.T) { }, } for _, test := range tests { - runTest := func(t *testing.T, f2 *fakeFIDO2) { - f2.setCallbacks() + t.Run(test.name, func(t *testing.T) { + test.fido2.setCallbacks() test.setUP() timeout := test.timeout @@ -656,17 +655,6 @@ func TestFIDO2Login(t *testing.T) { } assert.Equal(t, test.wantUser, actualUser, "actual user mismatch") - } - - // Run tests against both "metered" and "non-metered" fake variants, so we - // can ensure both behave correctly. - // There shouldn't be much of a difference, but tests are fast enough that - // it doesn't hurt either. - t.Run(test.name+"/metered", func(t *testing.T) { - runTest(t, test.fido2) - }) - t.Run(test.name+"/nonMetered", func(t *testing.T) { - runTest(t, test.fido2.withNonMeteredLocations()) }) } } @@ -686,7 +674,7 @@ func TestFIDO2Login_retryUVFailures(t *testing.T) { }) pin1.failUV = true // fail UV regardless of PIN - f2 := newFakeFIDO2(pin1).withNonMeteredLocations() + f2 := newFakeFIDO2(pin1) f2.setCallbacks() const rpID = "example.com" @@ -740,7 +728,7 @@ func TestFIDO2Login_singleResidentCredential(t *testing.T) { }, }) - f2 := newFakeFIDO2(oneCredential, manyCredentials).withNonMeteredLocations() + f2 := newFakeFIDO2(oneCredential, manyCredentials) f2.setCallbacks() const rpID = "example.com" @@ -908,7 +896,7 @@ func TestFIDO2Login_PromptTouch(t *testing.T) { }, { name: "Passwordless PIN plugged requires two touches", - fido2: newFakeFIDO2(pin1).withNonMeteredLocations(), + fido2: newFakeFIDO2(pin1), assertion: pwdlessAssertion, prompt: pin1, wantTouches: 2, @@ -961,7 +949,7 @@ func TestFIDO2Login_u2fDevice(t *testing.T) { dev := mustNewFIDO2Device("/u2f", "" /* pin */, nil /* info */) dev.u2fOnly = true - f2 := newFakeFIDO2(dev).withNonMeteredLocations() + f2 := newFakeFIDO2(dev) f2.setCallbacks() const rpID = "example.com" @@ -1037,7 +1025,7 @@ func TestFIDO2Login_bioErrorHandling(t *testing.T) { }, }) - f2 := newFakeFIDO2(bio).withNonMeteredLocations() + f2 := newFakeFIDO2(bio) f2.setCallbacks() // Prepare a passwordless assertion. @@ -1153,13 +1141,6 @@ func TestFIDO2Login_errors(t *testing.T) { prompt wancli.LoginPrompt wantErr string }{ - { - name: "ok - timeout", // check that good params are good - origin: origin, - assertion: okAssertion, - prompt: prompt, - wantErr: context.DeadlineExceeded.Error(), - }, { name: "nil origin", assertion: okAssertion, @@ -1218,7 +1199,7 @@ func TestFIDO2_LoginRegister_interactionErrors(t *testing.T) { u2f := mustNewFIDO2Device("/u2f", "" /* pin */, nil /* info */) u2f.u2fOnly = true - f2 := newFakeFIDO2(notRegistered, noPIN, noRK, u2f).withNonMeteredLocations() + f2 := newFakeFIDO2(notRegistered, noPIN, noRK, u2f) f2.setCallbacks() const rpID = "goteleport.com" @@ -1325,7 +1306,7 @@ func TestFIDO2_LoginRegister_interactionErrors(t *testing.T) { name: "passwordless U2F", createAssertion: func() *wantypes.CredentialAssertion { return &pwdlessAssertion }, prompt: u2f, - wantErr: context.DeadlineExceeded.Error(), // silently filtered, times out + wantErr: "cannot do passwordless", }, } { t.Run("login/"+test.name, func(t *testing.T) { @@ -1367,7 +1348,7 @@ func TestFIDO2_LoginRegister_interactionErrors(t *testing.T) { name: "excluded credential (U2F)", createCC: func() *wantypes.CredentialCreation { return &excludeCC }, prompt: u2f, - wantErr: context.DeadlineExceeded.Error(), // silently filtered, times out + wantErr: "registered credential", }, { name: "passwordless lacks UV", @@ -1386,7 +1367,7 @@ func TestFIDO2_LoginRegister_interactionErrors(t *testing.T) { name: "passwordless U2F", createCC: func() *wantypes.CredentialCreation { return &pwdlessCC }, prompt: u2f, - wantErr: context.DeadlineExceeded.Error(), // silently filtered, times out + wantErr: "cannot do passwordless", }, } { t.Run("register/"+test.name, func(t *testing.T) { @@ -1619,17 +1600,6 @@ func TestFIDO2Register(t *testing.T) { assert.Equal(t, bio1.credentialID(), ccr.RawId, "RawId mismatch (want bio1)") }, }, - { - name: "NOK timeout without devices", - timeout: 10 * time.Millisecond, - fido2: newFakeFIDO2(), - setUP: func() {}, - createCredential: func() *wantypes.CredentialCreation { - cp := *baseCC - return &cp - }, - wantErr: context.DeadlineExceeded, - }, { name: "passwordless pin device", fido2: newFakeFIDO2(pin2), @@ -1792,13 +1762,6 @@ func TestFIDO2Register_errors(t *testing.T) { prompt wancli.RegisterPrompt wantErr string }{ - { - name: "ok - timeout", // check that good params are good - origin: origin, - createCC: func() *wantypes.CredentialCreation { return okCC }, - prompt: prompt, - wantErr: context.DeadlineExceeded.Error(), - }, { name: "nil origin", createCC: func() *wantypes.CredentialCreation { return okCC }, @@ -1859,7 +1822,7 @@ func TestFIDO2Register_u2fExcludedCredentials(t *testing.T) { Options: authOpts, }) - f2 := newFakeFIDO2(u2fDev, otherDev).withNonMeteredLocations() + f2 := newFakeFIDO2(u2fDev, otherDev) f2.setCallbacks() const origin = "https://example.com" @@ -1919,8 +1882,6 @@ func resetFIDO2AfterTests(t *testing.T) { } type fakeFIDO2 struct { - useNonMeteredLocs bool - locs []*libfido2.DeviceLocation devices map[string]*fakeFIDO2Device } @@ -1941,41 +1902,18 @@ func newFakeFIDO2(devs ...*fakeFIDO2Device) *fakeFIDO2 { return f } -// withNonMeteredLocations makes fakeFIDO2 return all known devices immediately. -// Useful to test flows that optimize for plugged devices. -func (f *fakeFIDO2) withNonMeteredLocations() *fakeFIDO2 { - f.useNonMeteredLocs = true - return f -} - func (f *fakeFIDO2) setCallbacks() { - if f.useNonMeteredLocs { - *wancli.FIDODeviceLocations = f.DeviceLocations - } else { - *wancli.FIDODeviceLocations = f.newMeteredDeviceLocations() - } + *wancli.FIDODeviceLocations = f.DeviceLocations *wancli.FIDONewDevice = f.NewDevice } -func (f *fakeFIDO2) newMeteredDeviceLocations() func() ([]*libfido2.DeviceLocation, error) { - i := 0 - return func() ([]*libfido2.DeviceLocation, error) { - // Delay showing devices for a while to exercise polling. - i++ - const minLoops = 2 - if i < minLoops { - return nil, nil - } - return f.locs, nil - } -} - func (f *fakeFIDO2) DeviceLocations() ([]*libfido2.DeviceLocation, error) { return f.locs, nil } func (f *fakeFIDO2) NewDevice(path string) (wancli.FIDODevice, error) { if dev, ok := f.devices[path]; ok { + dev.open() return dev, nil } // go-libfido2 doesn't actually error here, but we do for simplicity. @@ -2013,8 +1951,8 @@ type fakeFIDO2Device struct { pubKey []byte // cond guards up and cancel. - cond *sync.Cond - up, cancel bool + cond *sync.Cond + up, cancel, opened bool } func mustNewFIDO2Device(path, pin string, info *libfido2.DeviceInfo, creds ...*libfido2.Credential) *fakeFIDO2Device { @@ -2073,15 +2011,26 @@ func (f *fakeFIDO2Device) cert() []byte { return f.key.Cert } -func (f *fakeFIDO2Device) Info() (*libfido2.DeviceInfo, error) { - if f.u2fOnly { - return nil, libfido2.ErrNotFIDO2 +func (f *fakeFIDO2Device) open() { + f.cond.L.Lock() + // Keep the `f.up` value from before open(), it makes tests simpler. + f.cancel = false + f.opened = true + f.cond.L.Unlock() +} + +func (f *fakeFIDO2Device) verifyOpen() error { + f.cond.L.Lock() + defer f.cond.L.Unlock() + if !f.opened { + return errors.New("device closed") } - return f.info, nil + return nil } func (f *fakeFIDO2Device) setUP() { f.cond.L.Lock() + // Set up regardless of opened, makes testing simpler. f.up = true f.cond.L.Unlock() f.cond.Broadcast() @@ -2089,12 +2038,47 @@ func (f *fakeFIDO2Device) setUP() { func (f *fakeFIDO2Device) Cancel() error { f.cond.L.Lock() - f.cancel = true + // Ignore cancels while closed, as this mirrors go-libfido2. + if f.opened { + f.cancel = true + } f.cond.L.Unlock() f.cond.Broadcast() return nil } +func (f *fakeFIDO2Device) Close() error { + f.cond.L.Lock() + f.opened = false + f.cond.L.Unlock() + f.cond.Broadcast() // Unblock any ongoing goroutines. + return nil +} + +func (f *fakeFIDO2Device) Info() (*libfido2.DeviceInfo, error) { + if err := f.verifyOpen(); err != nil { + return nil, err + } + if f.u2fOnly { + return nil, libfido2.ErrNotFIDO2 + } + return f.info, nil +} + +func (f *fakeFIDO2Device) IsFIDO2() (bool, error) { + if err := f.verifyOpen(); err != nil { + return false, err + } + return !f.u2fOnly, nil +} + +func (f *fakeFIDO2Device) SetTimeout(d time.Duration) error { + if err := f.verifyOpen(); err != nil { + return err + } + return nil +} + func (f *fakeFIDO2Device) MakeCredential( clientDataHash []byte, rp libfido2.RelyingParty, @@ -2103,6 +2087,10 @@ func (f *fakeFIDO2Device) MakeCredential( pin string, opts *libfido2.MakeCredentialOpts, ) (*libfido2.Attestation, error) { + if err := f.verifyOpen(); err != nil { + return nil, err + } + switch { case len(clientDataHash) == 0: return nil, errors.New("clientDataHash required") @@ -2169,6 +2157,10 @@ func (f *fakeFIDO2Device) Assertion( pin string, opts *libfido2.AssertionOpts, ) ([]*libfido2.Assertion, error) { + if err := f.verifyOpen(); err != nil { + return nil, err + } + // Give preference to simulated errors. if len(f.assertionErrors) > 0 { err := f.assertionErrors[0]