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)