Select the YubiKey by the name of its smart card reader#1071
Open
TicketWindowSeat wants to merge 1 commit into
Open
Select the YubiKey by the name of its smart card reader#1071TicketWindowSeat wants to merge 1 commit into
TicketWindowSeat wants to merge 1 commit into
Conversation
`New` opened `cards[0]`, the first reader reported by the smart card service, and assumed that it was a YubiKey. On a laptop with a built-in reader (the ones that Dell, Lenovo and HP ship) that reader enumerates before any USB device and is usually empty, so the default `yubikey:` uri fails with "error opening yubikey: connecting to smart card: the operation requires a Smart Card, but no Smart Card is currently in the device", an error that names no reader and blames a YubiKey that we never connected to. On Windows the same path fails with "the smart card has been removed, so further communication is not possible". The readers whose name identifies a YubiKey are now used first, keeping the order reported by the smart card service. A YubiKey connected to an NFC reader, or another device with support for the PIV applet, is named after its reader and not after Yubico, so when no reader identifies a YubiKey we open the first one, as before. If every reader that identifies a YubiKey fails to open we also fall back to that first reader, so a device that works today keeps working. Looking for a YubiKey by the name of the reader it is connected to is the method documented in github.com/go-piv/piv-go, and the one used by Yubico's ykman. Preferring the YubiKey also keeps us from taking exclusive access to a card that we don't need. With github.com/go-piv/piv-go/v2 v2.6.0 the connection to a card without the PIV applet, a payment card or a corporate badge in that built-in reader, is not always released, and the card stays locked for the life of the process. A serial number in the uri still looks in every reader, as the serial number cannot be read without opening the card, but the YubiKeys are now looked at first, and the cards that we open and don't select are closed instead of being left open in the cache. The errors now name the reader that failed to open or whose serial number we could not read, and the error for a serial number that we don't find lists the readers that we scanned. Fixes smallstep#649
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Name of feature:
Bug fix:
kms/yubikeynow selects the YubiKey by the name of its smart card reader, instead of opening the first reader and assuming it is a YubiKey.Pain or issue this feature alleviates:
Newassumed that the first reader reported by the smart card service held the YubiKey. On a machine with a built-in smart card reader that assumption is wrong: the built-in reader enumerates before any USB device and is usually empty, so the defaultyubikey:URI fails even though a YubiKey is connected — with an error that names no reader and blames the YubiKey. On Linux:error opening yubikey: connecting to smart card: the operation requires a Smart Card, but no Smart Card is currently in the device(the error reported in #649, from a Dell with a Broadcom 58200); on Windows:... the smart card has been removed, so further communication is not possible. And with a non-YubiKey PIV-capable card in that first reader,Newwould not even fail — it would silently use the wrong device. #649 was closed by documenting theyubikey:serial=workaround, but the default URI stayed broken on this common hardware.Readers whose name identifies a YubiKey are now used first, in the order the smart card service reports them. If no reader identifies a YubiKey — a YubiKey in an NFC reader, or another PIV device, is named after its reader — or none of them can be opened, we fall back to the first reader as before, so setups that work today keep working. Matching the reader name is the method piv-go documents and the one Yubico's
ykmanuses.A
serial=URI (the YubiKey's serial number, as in the documented workaround) still scans every reader — readers only advertise their names, so finding the device with that serial means opening the card in each reader and asking it — YubiKeys first, and the connections we open and don't select are now closed instead of being cached forever. Every error now names the reader that failed to open or whose serial number we could not read, and the serial-not-found error lists the readers that were scanned.Why is this important to the project (if not answered above):
step-cais a long-lived daemon, and opening a reader takes exclusive access to the card in it. The old code took that access liberally and never gave it back, in two ways. First, by design: every card the serial scan opened was added to the connection cache and held for the life of the process, selected or not. Second, by accident: for a card that fails to open — a corporate badge or a payment card without the PIV applet — the currently pinnedgo-piv/piv-go/v2v2.6.0 does not always release the connection either (go-piv/piv-go#192), and the caller gets nothing it could close.This PR changes the ownership discipline: a connection is opened tentatively, closed when its card is not the one selected, and only added to the cache when it is. Preferring YubiKey-named readers narrows the accidental case too — the cards we never needed are (mostly) never opened at all.
Is there documentation on how to use this feature? If so, where?
No usage change: the default
yubikey:URI now just works on machines with built-in readers. Theserial=workaround documented after #649 (smallstep/docs#365) remains valid and is still the right way to pin a specific key.New's doc comment describes the selection order.In what environments or workflows is this feature supported?
All platforms the yubikey KMS builds on (build tag
cgo && !noyubikey). The selection logic is exercised hardware-free by the new table-driven tests (stubbedpivCards/pivOpen), which run in CI. Manually verified: reader enumeration on a Dell with two built-in Broadcom readers on Windows (the #649 topology — the built-in readers enumerate first), and the full package test suite on Windows (amd64) and 32-bit Linux (Raspberry Pi, pcsc-lite). CI cannot exercise a real PC/SC stack, so reader-enumeration order and real SCARD error codes rest on this manual verification plus the stubbed tests.In what environments or workflows is this feature explicitly NOT supported (if any)?
Deliberate behavior change: a setup that relies on the first reader being opened while a YubiKey-named reader is also present will now get the YubiKey first — that is the fix. When no reader matches by name (NFC readers, non-Yubico PIV devices), the previous first-reader behavior is preserved, including when every YubiKey-named reader fails to open.
Supporting links/other PRs/issues:
serial=workaroundYK_READER_NAMEfilter — precedent for name-based selectionReviewed and refined with AI assistance; all changes reviewed, run, and verified by me.
💔Thank you!