From ffafedb06c0ab2802a05a529878040375054b841 Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Wed, 24 Jan 2024 17:28:20 -0300 Subject: [PATCH 1/5] Use FIDO2 touch functions to wait for touch --- lib/auth/webauthncli/fido2.go | 62 +++++++++++++++++++++++------- lib/auth/webauthncli/fido2_test.go | 51 ++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 13 deletions(-) diff --git a/lib/auth/webauthncli/fido2.go b/lib/auth/webauthncli/fido2.go index 0cd425f735309..50603977bc76f 100644 --- a/lib/auth/webauthncli/fido2.go +++ b/lib/auth/webauthncli/fido2.go @@ -54,8 +54,12 @@ const ( fido2DeviceTimeout = 30 * time.Second // Operation retry interval. - // Keep it less frequent than 2Hz / 0.5s. + // Keep it less frequent than 5Hz / 0.2s. fido2RetryInterval = 500 * time.Millisecond + + // Timeout for touch.Status operations. + // Keep it less frequent than 5Hz / 0.2s. + fido2TouchMaxWait = 200 * time.Second ) // User-friendly device filter errors. @@ -69,6 +73,12 @@ var ( errPasswordlessU2F = errors.New("U2F devices cannot do passwordless") ) +// TouchRequest abstracts *libfido2.TouchRequest for testing. +type TouchRequest interface { + Status(timeout time.Duration) (touched bool, err error) + Stop() error +} + // FIDODevice abstracts *libfido2.Device for testing. type FIDODevice interface { // Info mirrors libfido2.Device.Info. @@ -102,13 +112,28 @@ type FIDODevice interface { credentialIDs [][]byte, pin string, opts *libfido2.AssertionOpts) ([]*libfido2.Assertion, error) + + // TouchBegin mirrors libfido2.Device.TouchBegin. + TouchBegin() (TouchRequest, error) +} + +type fido2DeviceAdapter struct { + *libfido2.Device +} + +func (a *fido2DeviceAdapter) TouchBegin() (TouchRequest, error) { + return a.Device.TouchBegin() } // fidoDeviceLocations and fidoNewDevice are used to allow testing. var ( fidoDeviceLocations = libfido2.DeviceLocations fidoNewDevice = func(path string) (FIDODevice, error) { - return libfido2.NewDevice(path) + dev, err := libfido2.NewDevice(path) + if err != nil { + return nil, err + } + return &fido2DeviceAdapter{dev}, err } ) @@ -787,7 +812,7 @@ func handleDevice( 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) { + if touched, _ := waitForTouch(dev); touched { cancelAll(dev) // Escalate error to ErrUsingNonRegisteredDevice, if appropriate, so we @@ -927,9 +952,10 @@ func withPINHandler(cb deviceCallbackFunc) pinAwareCallbackFunc { // mechanism. Let's run a different operation to ask for a touch. requiresPIN = true - err = waitForTouch(dev) - if errors.Is(err, libfido2.ErrNoCredentials) { + if touched, _ := waitForTouch(dev); touched { err = nil // OK, selected successfully + } else { + err = &nonInteractiveError{err: err} } return } @@ -956,14 +982,24 @@ func (e *nonInteractiveError) Is(err error) bool { return ok } -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 - const cdh = "00000000000000000000000000000000" // "random", size 32 - _, err := dev.Assertion(rpID, []byte(cdh), nil /* credentials */, "", &libfido2.AssertionOpts{ - UP: libfido2.True, - }) - return err +func waitForTouch(dev FIDODevice) (touched bool, err error) { + touch, err := dev.TouchBegin() + if err != nil { + return false, trace.Wrap(err) + } + defer touch.Stop() + + // Block until we get a touch or a cancel. + for { + touched, err := touch.Status(fido2TouchMaxWait) + if err != nil { + log.Debugf("FIDO2: Device touch status error: %v", err) + return false, err + } + if touched { + return true, err + } + } } // deviceInfo contains an aggregate of a device's information and capabilities. diff --git a/lib/auth/webauthncli/fido2_test.go b/lib/auth/webauthncli/fido2_test.go index be4f7b79536f5..750b3e88b7aea 100644 --- a/lib/auth/webauthncli/fido2_test.go +++ b/lib/auth/webauthncli/fido2_test.go @@ -2284,6 +2284,57 @@ func (f *fakeFIDO2Device) Assertion( } } +type fakeTouchRequest struct { + dev *fakeFIDO2Device + done bool // guarded by the device's lock +} + +func (f *fakeFIDO2Device) TouchBegin() (wancli.TouchRequest, error) { + return &fakeTouchRequest{dev: f}, nil +} + +func (r *fakeTouchRequest) Status(timeout time.Duration) (touched bool, err error) { + r.dev.cond.L.Lock() + + // Read/reset up. + up := r.dev.up + if up { + r.dev.up = false + r.done = true + } + + // Read/reset cancel. + cancel := r.dev.cancel + if cancel { + r.dev.cancel = false + r.done = true + } + + r.dev.cond.L.Unlock() + + if cancel { + return false, libfido2.ErrKeepaliveCancel + } + if up { + return true, nil + } + + time.Sleep(1 * time.Millisecond) // Take a quick sleep to avoid tight loops. + return false, nil +} + +func (r *fakeTouchRequest) Stop() error { + r.dev.cond.L.Lock() + if r.done { + r.dev.cond.L.Unlock() + return nil + } + r.done = true + r.dev.cond.L.Unlock() + + return r.dev.Cancel() +} + func (f *fakeFIDO2Device) validatePIN(pin string) error { switch { case f.isBio() && pin == "": // OK, biometric check supersedes PIN. From c5bf9b36db5f91ca9f2a78e4c109149101558426 Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Wed, 24 Jan 2024 17:28:46 -0300 Subject: [PATCH 2/5] nit: Initialize nonInteractiveError uniformly --- lib/auth/webauthncli/fido2.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/auth/webauthncli/fido2.go b/lib/auth/webauthncli/fido2.go index 50603977bc76f..4363b9cc88004 100644 --- a/lib/auth/webauthncli/fido2.go +++ b/lib/auth/webauthncli/fido2.go @@ -786,7 +786,7 @@ func handleDevice( }() if err := dev.SetTimeout(fido2DeviceTimeout); err != nil { - return trace.Wrap(&nonInteractiveError{err}) + return trace.Wrap(&nonInteractiveError{err: err}) } // Gather device information. From e41fa0beac4a2e63bd2c409c3292e46aa6eff97d Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Mon, 29 Jan 2024 11:03:37 -0300 Subject: [PATCH 3/5] Change fido2TouchMaxWait to 200ms --- lib/auth/webauthncli/fido2.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/auth/webauthncli/fido2.go b/lib/auth/webauthncli/fido2.go index 4363b9cc88004..db8cb3550a44a 100644 --- a/lib/auth/webauthncli/fido2.go +++ b/lib/auth/webauthncli/fido2.go @@ -59,7 +59,7 @@ const ( // Timeout for touch.Status operations. // Keep it less frequent than 5Hz / 0.2s. - fido2TouchMaxWait = 200 * time.Second + fido2TouchMaxWait = 200 * time.Millisecond ) // User-friendly device filter errors. From 0f4b9cdae32541a2697073c28a3d6043ad0df42d Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Mon, 29 Jan 2024 11:05:18 -0300 Subject: [PATCH 4/5] Log dev.TouchBegin() errors --- lib/auth/webauthncli/fido2.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/auth/webauthncli/fido2.go b/lib/auth/webauthncli/fido2.go index db8cb3550a44a..3ee18c3f693b9 100644 --- a/lib/auth/webauthncli/fido2.go +++ b/lib/auth/webauthncli/fido2.go @@ -985,6 +985,8 @@ func (e *nonInteractiveError) Is(err error) bool { func waitForTouch(dev FIDODevice) (touched bool, err error) { touch, err := dev.TouchBegin() if err != nil { + // Error logged here as it's mostly ignored by callers. + log.Debugf("FIDO2: Device touch begin error: %v", err) return false, trace.Wrap(err) } defer touch.Stop() @@ -993,6 +995,7 @@ func waitForTouch(dev FIDODevice) (touched bool, err error) { for { touched, err := touch.Status(fido2TouchMaxWait) if err != nil { + // Error logged here as it's mostly ignored by callers. log.Debugf("FIDO2: Device touch status error: %v", err) return false, err } From 959ff0bdb0b5d378ec5d8e4beb702a539a57c453 Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Mon, 29 Jan 2024 13:59:59 -0300 Subject: [PATCH 5/5] Apply various error-handling fixes --- lib/auth/webauthncli/fido2.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/auth/webauthncli/fido2.go b/lib/auth/webauthncli/fido2.go index 3ee18c3f693b9..b9cad23c23731 100644 --- a/lib/auth/webauthncli/fido2.go +++ b/lib/auth/webauthncli/fido2.go @@ -131,9 +131,9 @@ var ( fidoNewDevice = func(path string) (FIDODevice, error) { dev, err := libfido2.NewDevice(path) if err != nil { - return nil, err + return nil, trace.Wrap(err) } - return &fido2DeviceAdapter{dev}, err + return &fido2DeviceAdapter{dev}, nil } ) @@ -997,10 +997,10 @@ func waitForTouch(dev FIDODevice) (touched bool, err error) { if err != nil { // Error logged here as it's mostly ignored by callers. log.Debugf("FIDO2: Device touch status error: %v", err) - return false, err + return false, trace.Wrap(err) } if touched { - return true, err + return true, nil } } }