iqlusioninc / yubikey.rs

Pure Rust YubiKey host-side driver for PIV-based RSA/ECC key storage + signing/encryption support
BSD 2-Clause "Simplified" License
218 stars 26 forks source link

Concurrent access results in PCSC Error #424

Open wrl opened 1 year ago

wrl commented 1 year ago

This is a bug occurring in https://github.com/indygreg/apple-platform-rs, a downstream which (optionally) uses the yubikey-rs crate. It's unclear to me whether this is an apple-codesign (downstream) bug, one in yubikey-rs, or even one in pcsc-rust.

I've copied and pasted the text from from https://github.com/indygreg/apple-platform-rs/issues/49 below:

I am using rcodesign in a on-premises CI server, with the smartcard functionality backed by a Yubikey 5 token.

When two (or more) jobs attempt to sign executables simultaneously, at least one (sometimes all) of them will begin exiting unsuccessfully with the pcsc-rust error Error::ResetCard, which has the description "The smart card has been reset, so any shared state information is invalid".

It's unclear to me where this error is being raised (i.e. when first connecting to the token, or when the signing operation is attempted), but it seems like pcsc-rust resets the Card when dropped (https://github.com/bluetech/pcsc-rust/blob/master/pcsc/src/lib.rs#L1670-L1684) and yubikey-rs doesn't change this or attempt to reconnect to the card on reset, though it does specify that the card should reset on reconnect (https://github.com/iqlusioninc/yubikey.rs/blob/main/src/yubikey.rs#L220-L224) and it seems like the card should perhaps reconnect if there's an error when starting a transaction (https://github.com/iqlusioninc/yubikey.rs/blob/main/src/yubikey.rs#L241-L245).

I am unfamiliar with PCSC and smartcards as a whole, so I'm not sure what the best practices for all of this are.

wrl commented 1 year ago

I'm very sorry about the repeated posts. Github returned a 500 when I tried to post it, and I just continued refreshing.

This is phenomenally embarrassing.

tarcieri commented 1 year ago

No worries… seems like an idempotence bug on GitHub’s side

indygreg commented 1 year ago

Maintainer of apple-codesign here.

I guess my biggest question is what are the requirements/restrictions around concurrent access? It certainly looks like concurrent processes using this crate race to perform an operation against a YubiKey.

Is locking something that this crate aims to abstract away? If so, did I hit a bug or am I using the library incorrectly?

Or is locking something that needs to happen outside this crate? If so, are there any APIs this crate could provide to make lock acquisition simpler? e.g. are there sufficient APIs to test whether a smartcard device is in use or available? (I'm thinking about wanting to present some info to users if contention issues arise.)

tarcieri commented 1 year ago

This seems like a problem with the pcsc crate. The pcsc::Card type has unsafe impls of Send and Sync:

https://docs.rs/pcsc/2.7.0/src/pcsc/lib.rs.html#1639

We could potentially throw a mutex around it in the yubikey crate as a band aid, but that’s about the best we can offer short of an upstream fix.

wrl commented 1 year ago

As a note, my use-case isn't concurrent uses inside the same process, but concurrent uses in separate processes, triggered by separate CI jobs. While it could be feasible to limit concurrent codesign CI jobs, I'm not seeing this PCSC error behaviour with osslsigncode and a separate (non-Yubikey) token which is nonetheless also accessed through a (different) pcscd process.

tarcieri commented 1 year ago

Perhaps it's doing some kind of locking I'm not familiar with?

Can you run some kind of tracing on it to investigate? (e.g. ktrace/dtrace)

wrl commented 1 year ago

I'm wondering whether this is some sort of shared/exclusive mode thing. Just received my spare Yubikey in the mail and I'm going to drill down and find out where this error is coming from, because right now it's unclear.