feat!: Require devices to be explicitly closed#15
Conversation
codingllama
changed the title
Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
| if cErr := C.fido_dev_cancel(d.dev); cErr != C.FIDO_OK { | ||
| return errors.Wrap(errFromCode(cErr), "failed to cancel") | ||
| } | ||
| if d == nil { |
There was a problem hiding this comment.
It's good to stay calm and not panic, but is there a valid use case where you call Cancel() on nil except for a mistake?
There was a problem hiding this comment.
It's probably a programming mistake if this happens, although not doing anything is reasonable for a cancel. Other methods should blow up.
As this is a library, I'd rather be safer than normal.
|
Thanks for the reviews, folks! |
codingllama
force-pushed
the
codingllama/dev-tweaks
branch
from
843fcb8 to
728a6b7
Compare
|
FYI, I've just pushed 2 more commits: b14fe43 and b267d1e. The first (b14fe43) is based on libfido2 examples and has the practical effect of making Yubikeys stop blinking on various assertion failures. The second (b267d1e) adds a way to set operation timeouts, which also allows us to set a hard deadline on assertions (and also makes the fobs stop blinking). |
|
I'll go ahead and merge this, as I'm convinced it's a change for the best. Thanks for the prompt reviews! |
3 participants
Expose a Close method on Device, open the device on NewDevice and require devices to be explicitly closed.
Additionally, expose the SetTimeout device method as well.
This is (another) departure from upstream, where instead of doing open/close on the Device for each operation, we allow opening the device just once for the entire lifetime of interactions. The change seems to improve responsiveness and reduce flakiness when interacting with devices. It also seems to be more in-line with various libfido2 examples.
gravitational/teleport#36640