bluetech / pcsc-rust

Rust bindings to PC/SC for smart card communication
https://docs.rs/pcsc
MIT License
111 stars 23 forks source link

Question: best way to distinguish deeper transaction failure from removal of card mid transaction #19

Closed Zylatis closed 3 years ago

Zylatis commented 3 years ago

Hi! First of all, thanks for the crate, it's been really ergonomic to use and learn about this stuff.

My use case is fairly standard and i've modelled my code fairly heavily on the examples given. I have a main listening loop which waits until cards are detected, does stuff using a transaction, and then carries on to the blocking update_status() to wait for next event.

However, i am having trouble finding a nice way to distinguish between a transaction failure due to something about the protocol, and a transaction that has failed because the card was removed before everything could complete. So far the way i have found that more or less models the behaviour i want is to wrap my communication (as distinct from setup/teardown) code in a match and if the error is a pcsc error then attempt to reconnect to the card. If this reconnection gives no card found error, then i can reasonably label the original error as one due to the card being removed prematurely, otherwise it might be a deeper problem.

Is there a better way to do this? I tried seeing if i could detect changes without this reconnection step using rs.event_count() but it didn't work - that count was the same before and after the error is thrown.

The code that handles all this is roughly this

match transact_with_device(&mut card, buffer.clone()) {
    Ok(_) => indicate_success(&card_direct)?,
    Err(e) => {
        let card_removed = match card.reconnect(
            ShareMode::Shared,
            Protocols::ANY,
            Disposition::ResetCard,
        ) {
            Ok(_) => false,
            Err(e) => match e {
                pcsc::Error::NoSmartcard => true,
                _ => false,
            },
        };
        match e {
            NFCError::DeviceResponseError => {
                indicate_general_error(&card_direct)?;
                continue;
            }
            _ => {
                if card_removed {
                    indicate_range_error(&card_direct)?;
                    continue;
                } else {
                    indicate_general_error(&card_direct)?;
                    return Err(e);
                }
            }
        }
    }
}

where i have separate connections for transacting with the device and doing the indicate success/error stuff as one requires a direct connection.

transact_with_device() does a whole bunch of stuff including opening a transaction with the card, and some other back and forth. The NFCError is just a custom enum which contains some elements which wrap pcsc errors. This allows me to determine if i need to restart the NFC server or if the external device is (probably) at fault and to just continue with the listen loop.

I guess the tl;dr is, it seems NotTransacted is always thrown even if the underlying cause is NoSmartcard, and i'm looking for a concise way to get around this (or learn that i am wrong about this!)

Thanks in advance!

bluetech commented 3 years ago

Are you using windows, linux, or macos?

Zylatis commented 3 years ago

Linux, with the following installed:

libpcsclite-dev libpcsclite1 libacsccid1 pcscd pcscd-tools
bluetech commented 3 years ago

I guess the tl;dr is, it seems NotTransacted is always thrown even if the underlying cause is NoSmartcard, and i'm looking for a concise way to get around this (or learn that i am wrong about this!)

If pcsc does not throw a useful/detectable error from the transaction when the card is removed clearly indicating that the problem was caused by the card being removed, then some workaround is needed, not much you can do about it. Thinking about it operationally, it would be difficult for a pcsc implementation to indicate this in all possible states. I guess it would throw different errors depending on when exactly the card is removed.

Is there a better way to do this? I tried seeing if i could detect changes without this reconnection step using rs.event_count() but it didn't work - that count was the same before and after the error is thrown.

In my experience the event counter does work in pcsclite (although it doesn't work in Windows). I'm not exactly sure what you tried here - did you do another get_status_changes() call? The ReaderStates are only updated by get_status_changes(). I guess the way you'd check if the card was removed is:

So far the way i have found that more or less models the behaviour i want is to wrap my communication (as distinct from setup/teardown) code in a match and if the error is a pcsc error then attempt to reconnect to the card.

This seems to me like a pretty good way to do it. It would have been nicer to use card.status() and check the status directly (assuming you don't actually want to reconnect), unfortunately status() is deprecated because it is not portable to Windows. You can still use it though if you don't care about Windows...

bluetech commented 3 years ago

(I just pushed a commit to master which makes card.status2().status() work and portable again. Though when the card is removed a CardRemoved error is returned anyway)

Zylatis commented 3 years ago

Hey, thanks for the comprehensive reply and update to the code! One question about the .status() path though, this needs a call to get_status_changes() too, right, and this blocks? So the setup would be: if transaction goes through, indicate success, otherwise update reader states with blocking call and then check the status.

Is there a risk that the thrown error will not cause a status change and this could hang the thread, or would any error in the transacting function (which in this case would close the transaction because its scoped) result in a status change and so there would always be something for get_status_changes() to read. I see that this can be interrupted but presumably that would require another thread to monitor this process.

Thanks again!

bluetech commented 3 years ago

One question about the .status() path though, this needs a call to get_status_changes() too, right, and this blocks?

No, it doesn't need get_status_changes(), it is just called on the Card. It doesn't block (wait) like get_status_changes(), it only returns the current status.

if transaction goes through, indicate success, otherwise update reader states with blocking call and then check the status.

With status(), you can check the status of the card directly after the transaction fails, e.g. if it's present, removed etc. You can also use get_status_changes() for this (if you change the reader state to UNAWARE, I believe get_status_changes() will update the state and return immediately) but it's a bit harder to manage.

Zylatis commented 3 years ago

Thanks for the update, sorry for my slow reply. I had a go using just the card.status() as i don't currently care about windows, but even in the case where i remove the device too quickly, and the reconnect method detects this, card.status still returns

card.status() = Ok(
    (
        PRESENT | POWERED | NEGOTIABLE,
        T1,
    ),
)

Correctly, i think, if i call card.status() after the reconnect it returns

card.status() = Err(
    ResetCard,
)

Is there some subtlety about calling status on the card vs the transaction that failed and closed? Below is the code i used to print out the statuses, which is the same as original code but just with 2 debug statements

match transact_with_device(&mut card, buffer.clone()) {
    Ok(_) => indicate_success(&card_direct)?,
    Err(e) => {
        dbg!(card.status());
        let card_removed = match card.reconnect(
            ShareMode::Shared,
            Protocols::ANY,
            Disposition::ResetCard,
        ) {
            Ok(_) => false,
            Err(e) => match e {
                pcsc::Error::NoSmartcard => true,
                _ => false,
            },
        };
        dbg!(card.status());
        match e {
            NFCError::DeviceResponseError => {
                indicate_general_error(&card_direct)?;
                continue;
            }
            _ => {
                if card_removed {
                    indicate_range_error(&card_direct)?;
                    continue;
                } else {
                    indicate_general_error(&card_direct)?;
                    return Err(e);
                }
            }
        }
    }
}

Sorry if i'm missing something simple here!

bluetech commented 3 years ago

Hmm in this case it does seem like the reconnect triggers something which causes the status change. But this depends on the PCSC internals, which I cannot say much about. I recommend pointing your question to the pcsclite mailing list (https://lists.infradead.org/mailman/listinfo/pcsclite-muscle), the maintainer (Ludovic Rousseau) is friendly and responsive.

You can try asking with the Rust code, but if they ask for C, the documentation of each Rust function mentions which corresponding C function it calls, so you can use that.

Zylatis commented 3 years ago

Brilliant, thanks for the helpful responses! I'll stick with my wonky solution for now, but I'd love to return to this and understand better so will definitely explore more at some point.