From 11ae7f124e25d4425ef8c3f70ce90da419ef6af3 Mon Sep 17 00:00:00 2001 From: TicketWindowSeat <255788533+TicketWindowSeat@users.noreply.github.com> Date: Sat, 18 Jul 2026 16:50:52 -0400 Subject: [PATCH] Select the YubiKey by the name of its smart card reader `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 #649 --- kms/yubikey/yubikey.go | 182 ++++++++++++++++---- kms/yubikey/yubikey_test.go | 328 ++++++++++++++++++++++++++++++++++++ 2 files changed, 480 insertions(+), 30 deletions(-) diff --git a/kms/yubikey/yubikey.go b/kms/yubikey/yubikey.go index a39e5f05..969c1eb7 100644 --- a/kms/yubikey/yubikey.go +++ b/kms/yubikey/yubikey.go @@ -150,18 +150,154 @@ var pivOpen = func(card string) (pivKey, error) { return piv.Open(card) } -// openCard wraps pivOpen with a cache. It loads a card connection from the -// cache if present. -func openCard(card string) (pivKey, error) { +// probeCard opens a connection to the given card, loading it from the cache if +// present. The second return value reports whether we opened the connection: it +// is owned by the caller, who must close it if it does not use the card. A +// connection from the cache is in use by another YubiKey, and it is not ours to +// close. +// +// A connection is only added to the cache when the card is selected: another +// YubiKey must never get a connection that we are about to close. +func probeCard(card string) (pivKey, bool, error) { if v, ok := pivMap.Load(card); ok { - return v.(pivKey), nil + return v.(pivKey), false, nil } yk, err := pivOpen(card) if err != nil { - return nil, err + return nil, false, err + } + return yk, true, nil +} + +// isYubiKey reports whether the name of a smart card reader, e.g. "Yubico +// YubiKey OTP+FIDO+CCID 0", identifies a YubiKey. Looking for a YubiKey by the +// name of its reader is the method documented in github.com/go-piv/piv-go, and +// the one used by Yubico's ykman. +func isYubiKey(card string) bool { + return strings.Contains(strings.ToLower(card), "yubikey") +} + +// selectCard returns a connection to a YubiKey, and the name of the smart card +// reader it is connected to. +// +// The readers that identify a YubiKey by name are always used first. Opening a +// reader takes exclusive access to the card in it, and with piv-go v2.6.0 the +// connection to a card without the PIV applet -- a payment card or a corporate +// badge in the reader built in a laptop -- is not always released, so we avoid +// opening those readers whenever we can. +func selectCard(cards []string, serial string) (pivKey, string, error) { + if len(cards) == 0 { + return nil, "", errors.New("error detecting yubikey: try removing and reconnecting the device") + } + + // A serial number cannot be read without opening the card, so we look for it + // in all the readers, the YubiKeys first. + if serial != "" { + return selectCardWithSerial(cards, serial) + } + return findFirstYubiKey(cards) +} + +// findFirstYubiKey returns a connection to the card in the first reader that +// identifies a YubiKey and can be opened, and the name of that reader. When no +// reader identifies a YubiKey, or none of the YubiKeys can be opened, it falls +// back to the first reader reported by the smart card service, the one that +// this method opened before, so that a device that works today keeps working: +// 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. +func findFirstYubiKey(cards []string) (pivKey, string, error) { + var firstErr error + open := func(card string) (pivKey, bool) { + yk, opened, err := probeCard(card) + if err != nil { + if firstErr == nil { + firstErr = fmt.Errorf("error opening yubikey in reader %q: %w", card, err) + } + return nil, false + } + if opened { + pivMap.Store(card, yk) + } + return yk, true + } + + // Open the YubiKeys in the order reported by the smart card service. + for _, card := range cards { + if !isYubiKey(card) { + continue + } + if yk, ok := open(card); ok { + return yk, card, nil + } + } + if card := cards[0]; !isYubiKey(card) { + if yk, ok := open(card); ok { + return yk, card, nil + } + } + return nil, "", firstErr +} + +// selectCardWithSerial returns a connection to the YubiKey with the given serial +// number, and the name of the smart card reader it is connected to. It scans +// every reader, the YubiKeys before the rest: opening a reader takes exclusive +// access to the card in it, so we only open the other readers when the serial +// number is not on a YubiKey. +func selectCardWithSerial(cards []string, serial string) (pivKey, string, error) { + var firstErr error + scanned := make([]string, 0, len(cards)) + scan := func(card string) (pivKey, bool) { + scanned = append(scanned, strconv.Quote(card)) + yk, opened, err := probeCard(card) + if err != nil { + if firstErr == nil { + firstErr = fmt.Errorf("error opening yubikey in reader %q: %w", card, err) + } + return nil, false + } + s, err := yk.Serial() + if err == nil && serial == strconv.FormatUint(uint64(s), 10) { + if opened { + pivMap.Store(card, yk) + } + return yk, true + } + if err != nil && firstErr == nil { + firstErr = fmt.Errorf("error reading the serial number in reader %q: %w", card, err) + } + // Release the cards that we open and don't use, they would stay locked + // for the rest of the process. + if opened { + _ = yk.Close() + } + return nil, false + } + + var others []string + for _, card := range cards { + if !isYubiKey(card) { + others = append(others, card) + continue + } + if yk, ok := scan(card); ok { + return yk, card, nil + } } - pivMap.Store(card, yk) - return yk, nil + for _, card := range others { + if yk, ok := scan(card); ok { + return yk, card, nil + } + } + + // Always name the readers that we scanned: the error of the reader that + // failed to open, or whose serial number we could not read, is context, it + // is not the reason why we didn't find the key. Surfacing only the failure + // of an empty built-in reader is the error reported in #649. + readers := strings.Join(scanned, ", ") + if firstErr != nil { + return nil, "", fmt.Errorf("failed to find key with serial number %s in the readers %s: %w", serial, readers, firstErr) + } + return nil, "", fmt.Errorf("failed to find key with serial number %s in the readers %s", serial, readers) } // validManagementKeyLengths contains the valid lengths @@ -198,6 +334,12 @@ const maximumManagementKeyLength = 32 // // If the pin or the management key are not provided, we will use the default // ones. +// +// If the serial number is not provided, we will use the first YubiKey. YubiKeys +// are identified by the name of the smart card reader they are connected to: a +// YubiKey connected to an NFC reader, or another device with support for the +// PIV applet, is named after its reader. If no reader identifies a YubiKey, or +// none of the YubiKeys can be opened, we will use the first reader. func New(_ context.Context, opts apiv1.Options) (*YubiKey, error) { pin := "123456" var managementKey [maximumManagementKeyLength]byte @@ -252,30 +394,10 @@ func New(_ context.Context, opts apiv1.Options) (*YubiKey, error) { if err != nil { return nil, err } - if len(cards) == 0 { - return nil, errors.New("error detecting yubikey: try removing and reconnecting the device") - } - card := cards[0] - var yk pivKey - if serial != "" { - // Attempt to locate the yubikey with the given serial. - for _, name := range cards { - if k, err := openCard(name); err == nil { - if s, err := k.Serial(); err == nil { - if serial == strconv.FormatUint(uint64(s), 10) { - yk = k - card = name - break - } - } - } - } - if yk == nil { - return nil, errors.Errorf("failed to find key with serial number %s, slot 0x9a might be empty", serial) - } - } else if yk, err = openCard(cards[0]); err != nil { - return nil, errors.Wrap(err, "error opening yubikey") + yk, card, err := selectCard(cards, serial) + if err != nil { + return nil, err } return &YubiKey{ diff --git a/kms/yubikey/yubikey_test.go b/kms/yubikey/yubikey_test.go index a8268c9a..003f557d 100644 --- a/kms/yubikey/yubikey_test.go +++ b/kms/yubikey/yubikey_test.go @@ -21,6 +21,7 @@ import ( "os" "path/filepath" "reflect" + "slices" "sync" "testing" @@ -46,6 +47,7 @@ type stubPivKey struct { serial uint32 serialErr error closeErr error + closed int } type symmetricAlgorithm int @@ -263,6 +265,7 @@ func (s *stubPivKey) Attest(slot piv.Slot) (*x509.Certificate, error) { } func (s *stubPivKey) Close() error { + s.closed++ return s.closeErr } @@ -322,6 +325,16 @@ func TestNew(t *testing.T) { "Yubico YubiKey OTP+FIDO+CCID 01", }, nil } + okBuiltInReaderPivCards := func() ([]string, error) { + return []string{ + "Broadcom Corp Contacted SmartCard 0", + "Broadcom Corp Contactless SmartCard 0", + "Yubico Yubikey 4 OTP+U2F+CCID 0", + }, nil + } + okOtherReaderPivCards := func() ([]string, error) { + return []string{"Nitrokey Nitrokey 3 [CCID/ICCD Interface] 00 00"}, nil + } failPivCards := func() ([]string, error) { return nil, errors.New("error reading cards") } @@ -332,6 +345,14 @@ func TestNew(t *testing.T) { okPivOpen := func(card string) (pivKey, error) { return yk, nil } + // okYubiKeyPivOpen fails on the readers that don't identify a YubiKey, the + // way an empty reader fails in the smart card service. + okYubiKeyPivOpen := func(card string) (pivKey, error) { + if !isYubiKey(card) { + return nil, errors.New("connecting to smart card: the smart card has been removed, so further communication is not possible") + } + return yk, nil + } failPivOpen := func(card string) (pivKey, error) { return nil, errors.New("error opening card") } @@ -403,6 +424,16 @@ func TestNew(t *testing.T) { pivCards = okPivCards pivOpen = okPivOpen }, &YubiKey{yk: yk, pin: "123456", card: "Yubico YubiKey OTP+FIDO+CCID", managementKey: []byte{0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0x00, 0x11, 0x22, 0x33}}, false}, + {"ok with built-in reader", args{ctx, apiv1.Options{}}, func() { + pivMap = sync.Map{} + pivCards = okBuiltInReaderPivCards + pivOpen = okYubiKeyPivOpen + }, &YubiKey{yk: yk, pin: "123456", card: "Yubico Yubikey 4 OTP+U2F+CCID 0", managementKey: piv.DefaultManagementKey}, false}, + {"ok with other reader", args{ctx, apiv1.Options{}}, func() { + pivMap = sync.Map{} + pivCards = okOtherReaderPivCards + pivOpen = okPivOpen + }, &YubiKey{yk: yk, pin: "123456", card: "Nitrokey Nitrokey 3 [CCID/ICCD Interface] 00 00", managementKey: piv.DefaultManagementKey}, false}, {"fail uri", args{ctx, apiv1.Options{URI: "badschema:"}}, func() { pivMap = sync.Map{} pivCards = okPivCards @@ -462,6 +493,303 @@ func TestNew(t *testing.T) { } } +// TestNew_cardSafety verifies that New does not connect to the readers built in +// a laptop, even if a card in them could be opened, as opening a card takes +// exclusive access to it. +func TestNew_cardSafety(t *testing.T) { + pOpen := pivOpen + pCards := pivCards + t.Cleanup(func() { + pivMap = sync.Map{} + pivOpen = pOpen + pivCards = pCards + }) + + yk := newStubPivKey(t, ECDSA) + + var opened []string + pivMap = sync.Map{} + pivCards = func() ([]string, error) { + return []string{ + "Broadcom Corp Contacted SmartCard 0", + "Broadcom Corp Contactless SmartCard 0", + "Yubico Yubikey 4 OTP+U2F+CCID 0", + }, nil + } + pivOpen = func(card string) (pivKey, error) { + opened = append(opened, card) + return yk, nil + } + + k, err := New(t.Context(), apiv1.Options{}) + require.NoError(t, err) + assert.Equal(t, "Yubico Yubikey 4 OTP+U2F+CCID 0", k.card) + assert.Equal(t, []string{"Yubico Yubikey 4 OTP+U2F+CCID 0"}, opened) +} + +func Test_selectCard(t *testing.T) { + pOpen := pivOpen + t.Cleanup(func() { + pivMap = sync.Map{} + pivOpen = pOpen + }) + + const ( + contacted = "Broadcom Corp Contacted SmartCard 0" + contactless = "Broadcom Corp Contactless SmartCard 0" + yubiKey = "Yubico Yubikey 4 OTP+U2F+CCID 0" + otherYubiKey = "Yubico YubiKey OTP+FIDO+CCID 01" + brokenYubiKey = "Yubico YubiKey OTP+FIDO+CCID 02" + nitroKey = "Nitrokey Nitrokey 3 [CCID/ICCD Interface] 00 00" + ) + + keys := map[string]*stubPivKey{} + for card, serial := range map[string]uint32{ + contacted: 100001, + contactless: 100002, + yubiKey: 112233, + otherYubiKey: 332211, + nitroKey: 445566, + } { + k := newStubPivKey(t, ECDSA) + k.serial = serial + keys[card] = k + } + // A YubiKey that cannot report its serial number. + keys[brokenYubiKey] = newStubPivKey(t, ECDSA) + keys[brokenYubiKey].serialErr = errors.New("error getting serial number") + + type args struct { + cards []string + serial string + } + tests := []struct { + name string + args args + open []string + want string + wantOpened []string + wantClosed map[string]int + wantCached []string + wantErr bool + }{ + {"ok", args{[]string{yubiKey}, ""}, + []string{yubiKey}, yubiKey, + []string{yubiKey}, nil, []string{yubiKey}, false}, + {"ok with built-in reader", args{[]string{contacted, contactless, yubiKey}, ""}, + []string{contacted, contactless, yubiKey}, yubiKey, + []string{yubiKey}, nil, []string{yubiKey}, false}, + {"ok with built-in reader and serial", args{[]string{contacted, contactless, yubiKey}, "112233"}, + []string{contacted, contactless, yubiKey}, yubiKey, + []string{yubiKey}, nil, []string{yubiKey}, false}, + {"ok with built-in readers only", args{[]string{contacted, contactless}, ""}, + []string{contacted, contactless}, contacted, + []string{contacted}, nil, []string{contacted}, false}, + {"ok with multiple yubikeys", args{[]string{yubiKey, otherYubiKey}, ""}, + []string{yubiKey, otherYubiKey}, yubiKey, + []string{yubiKey}, nil, []string{yubiKey}, false}, + {"ok with multiple yubikeys and serial", args{[]string{yubiKey, otherYubiKey}, "332211"}, + []string{yubiKey, otherYubiKey}, otherYubiKey, + []string{yubiKey, otherYubiKey}, map[string]int{yubiKey: 1}, []string{otherYubiKey}, false}, + {"ok with other reader", args{[]string{nitroKey}, ""}, + []string{nitroKey}, nitroKey, + []string{nitroKey}, nil, []string{nitroKey}, false}, + {"ok with other reader and serial before a yubikey", args{[]string{nitroKey, yubiKey}, "445566"}, + []string{nitroKey, yubiKey}, nitroKey, + []string{yubiKey, nitroKey}, map[string]int{yubiKey: 1}, []string{nitroKey}, false}, + {"ok with yubikey that cannot be opened", args{[]string{nitroKey, yubiKey}, ""}, + []string{nitroKey}, nitroKey, + []string{yubiKey, nitroKey}, nil, []string{nitroKey}, false}, + {"ok with one yubikey that cannot be opened", args{[]string{contacted, yubiKey, otherYubiKey}, ""}, + []string{contacted, otherYubiKey}, otherYubiKey, + []string{yubiKey, otherYubiKey}, nil, []string{otherYubiKey}, false}, + {"ok with yubikey that cannot report its serial", args{[]string{brokenYubiKey, yubiKey}, "112233"}, + []string{brokenYubiKey, yubiKey}, yubiKey, + []string{brokenYubiKey, yubiKey}, map[string]int{brokenYubiKey: 1}, []string{yubiKey}, false}, + {"ok with serial behind a reader that cannot be opened", args{[]string{contacted, nitroKey}, "445566"}, + []string{nitroKey}, nitroKey, + []string{contacted, nitroKey}, nil, []string{nitroKey}, false}, + {"fail no cards", args{[]string{}, ""}, + nil, "", + nil, nil, nil, true}, + {"fail no yubikey in readers", args{[]string{contacted, contactless}, ""}, + nil, "", + []string{contacted}, nil, nil, true}, + {"fail yubikey that cannot be opened", args{[]string{contacted, yubiKey}, ""}, + nil, "", + []string{yubiKey, contacted}, nil, nil, true}, + {"fail serial not found", args{[]string{contacted, yubiKey}, "999999"}, + []string{contacted, yubiKey}, "", + []string{yubiKey, contacted}, map[string]int{contacted: 1, yubiKey: 1}, nil, true}, + {"fail serial not found with a reader that cannot be opened", args{[]string{contacted, yubiKey}, "999999"}, + []string{yubiKey}, "", + []string{yubiKey, contacted}, map[string]int{yubiKey: 1}, nil, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pivMap = sync.Map{} + for _, k := range keys { + k.closed = 0 + } + + var opened []string + pivOpen = func(card string) (pivKey, error) { + opened = append(opened, card) + if !slices.Contains(tt.open, card) { + return nil, errors.New("connecting to smart card: the smart card has been removed, so further communication is not possible") + } + return keys[card], nil + } + + got, card, err := selectCard(tt.args.cards, tt.args.serial) + if (err != nil) != tt.wantErr { + t.Errorf("selectCard() error = %v, wantErr %v", err, tt.wantErr) + return + } + if tt.want == "" { + assert.Nil(t, got) + } else { + assert.Equal(t, keys[tt.want], got) + } + assert.Equal(t, tt.want, card) + assert.Equal(t, tt.wantOpened, opened) + + for name, k := range keys { + assert.Equal(t, tt.wantClosed[name], k.closed, "unexpected number of closes of %q", name) + } + + var cached []string + pivMap.Range(func(k, _ any) bool { + cached = append(cached, k.(string)) + return true + }) + assert.ElementsMatch(t, tt.wantCached, cached) + }) + } +} + +// Test_selectCard_serialNotFound verifies that the error returned when no +// YubiKey has the given serial number always names the readers that were +// scanned, even when one of them cannot be opened. An empty reader built in a +// laptop is the one that fails to open, and an error blaming only that reader, +// with no mention of the YubiKey that we did read, is the error reported in +// smallstep/crypto#649. +func Test_selectCard_serialNotFound(t *testing.T) { + pOpen := pivOpen + t.Cleanup(func() { + pivMap = sync.Map{} + pivOpen = pOpen + }) + + const ( + contacted = "Broadcom Corp Contacted SmartCard 0" + yubiKey = "Yubico Yubikey 4 OTP+U2F+CCID 0" + ) + + yk := newStubPivKey(t, ECDSA) + yk.serial = 112233 + + tests := []struct { + name string + open []string + wantErr string + }{ + {"all readers open", []string{contacted, yubiKey}, + `failed to find key with serial number 999999 in the readers "Yubico Yubikey 4 OTP+U2F+CCID 0", "Broadcom Corp Contacted SmartCard 0"`}, + {"built-in reader cannot be opened", []string{yubiKey}, + `failed to find key with serial number 999999 in the readers "Yubico Yubikey 4 OTP+U2F+CCID 0", "Broadcom Corp Contacted SmartCard 0": error opening yubikey in reader "Broadcom Corp Contacted SmartCard 0": error opening card`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pivMap = sync.Map{} + pivOpen = func(card string) (pivKey, error) { + if !slices.Contains(tt.open, card) { + return nil, errors.New("error opening card") + } + return yk, nil + } + + got, card, err := selectCard([]string{contacted, yubiKey}, "999999") + assert.EqualError(t, err, tt.wantErr) + assert.Nil(t, got) + assert.Empty(t, card) + }) + } +} + +// Test_selectCard_serialReadError verifies that the error returned when no +// YubiKey has the given serial number reports a serial number that we could +// not read: a reader skipped because its serial read failed must not be listed +// as scanned with no trace of the failure. +func Test_selectCard_serialReadError(t *testing.T) { + pOpen := pivOpen + t.Cleanup(func() { + pivMap = sync.Map{} + pivOpen = pOpen + }) + + const ( + yubiKey = "Yubico Yubikey 4 OTP+U2F+CCID 0" + brokenYubiKey = "Yubico YubiKey OTP+FIDO+CCID 02" + ) + + yk := newStubPivKey(t, ECDSA) + yk.serial = 112233 + broken := newStubPivKey(t, ECDSA) + broken.serialErr = errors.New("error getting serial number") + keys := map[string]*stubPivKey{yubiKey: yk, brokenYubiKey: broken} + + pivMap = sync.Map{} + pivOpen = func(card string) (pivKey, error) { + return keys[card], nil + } + + got, card, err := selectCard([]string{brokenYubiKey, yubiKey}, "999999") + assert.EqualError(t, err, `failed to find key with serial number 999999 in the readers "Yubico YubiKey OTP+FIDO+CCID 02", "Yubico Yubikey 4 OTP+U2F+CCID 0": error reading the serial number in reader "Yubico YubiKey OTP+FIDO+CCID 02": error getting serial number`) + assert.Nil(t, got) + assert.Empty(t, card) + assert.Equal(t, 1, broken.closed) + assert.Equal(t, 1, yk.closed) +} + +// Test_selectCard_cached verifies that the connections loaded from the cache are +// never closed, as they are in use by another YubiKey. +func Test_selectCard_cached(t *testing.T) { + pOpen := pivOpen + t.Cleanup(func() { + pivMap = sync.Map{} + pivOpen = pOpen + }) + + const ( + yubiKey = "Yubico Yubikey 4 OTP+U2F+CCID 0" + otherYubiKey = "Yubico YubiKey OTP+FIDO+CCID 01" + ) + + yk := newStubPivKey(t, ECDSA) + otherYk := newStubPivKey(t, ECDSA) + otherYk.serial = 332211 + + pivMap = sync.Map{} + pivMap.Store(yubiKey, yk) + pivMap.Store(otherYubiKey, otherYk) + pivOpen = func(card string) (pivKey, error) { + t.Errorf("pivOpen(%q) called, want the connection from the cache", card) + return nil, errors.New("error opening card") + } + + got, card, err := selectCard([]string{yubiKey, otherYubiKey}, "332211") + require.NoError(t, err) + assert.Equal(t, otherYk, got) + assert.Equal(t, otherYubiKey, card) + assert.Zero(t, yk.closed) + assert.Zero(t, otherYk.closed) + + v, ok := pivMap.Load(yubiKey) + assert.True(t, ok) + assert.Equal(t, yk, v) +} + func TestYubiKey_LoadCertificate(t *testing.T) { yk := newStubPivKey(t, ECDSA)