Support for all status bytes

[sosthene-nitrokey]

The Status enum contains a number of status bytes, but not all defined by the specifications. It also does not contain a fallback (Unknown(u16)) variant that could be used to encode unknown bytes, like does openpgp-card

I think we should add such a variant. However this can become an issue when matching against bytes. For example if one wants to test that a given status bytes are Success, they would need to match against both Success and Unknown::(0x9000) which can be tedious and lead to inconsistencies.

I suggest we instead change Status to a newtype Status(u16) with a bunch of associated constants for known values. You can still do pattern matching with the constants.

For example, reqwest uses this pattern instead of an enum for HTTP status codes: https://docs.rs/reqwest/latest/reqwest/struct.StatusCode.html#associatedconstant.ACCEPTED

[nickray]

I'm not a fan of this approach - the point of using enums to me is to make the universe of possibilities smaller, not allow random bytes throughout the following code (https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/).

If there is an Unknown(u16) field, then the library should at least enforce the invariant that the contained u16 is none of the named variants.

We already have the TryInto (https://github.com/ycrypto/iso7816/blob/main/src/response/status.rs#L83) though, which encodes this philosophy.

What is wrong with adding variants as needed? Or doing a full pass to add most of them? Using the C approach (like reqwest) does not seem to me to make anything easier or more ergonomic.

[nickray]

The constants (public or maybe private) could still be helpful to remove duplication of explicit codes in the two conversions between parsed and raw. Maybe a quick macro-rules macro to define the constants and the two conversions?

[sosthene-nitrokey]

If there is an Unknown(u16) field, then the library should at least enforce the invariant that the contained u16 is none of the named variants.

That's not really possible because you can't make enum constructors private.

We already have the TryInto (https://github.com/ycrypto/iso7816/blob/main/src/response/status.rs#L83) though, which encodes this philosophy.

It's not really used anywhere though.

Also, adding an Unknown(u16) variant would make it a breaking change to then add any other variant, since code might match against it in the Unknown variant, and it would therefore break the matching.

I'm not a fan of this approach - the point of using enums to me is to make the universe of possibilities smaller, not allow random bytes throughout the following code (https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/).

The only invalid SW bytes according to iso7816-4 are those outside of 6XXX, 9XXX and 60XX. I think at least all valid values should be accepted, to avoid data loss. In my case I'm not just creating Status, I'm receiving them from the SE050, so I need to handle the case where the meaning of the status bytes are unknown (I've already encountered 6CXX, which the current enum does not support, and no option looks good to me:

// Choice one: Use a default status if the Status bytes failed to parse (data loss, especially a pain currently when I'm doing debugging).
fn do_something() -> Result<Data, Status>;

// Choice 2: Return the SW bytes if they're incorrect: Overall a pretty bad API.
fn do_something() -> Result<Data, Result<Status, u16>>;

Enums also prevent renaming (without a breaking change), and a lot of the current names don't properly reflect their ISO7816-4 meaning. For example UnspecifiedNonpersistentExecutionError and UnspecifiedPersistentExecutionError actually refer to whether data on flash was changed, which was not clear to me at all, KeyReferenceNotFound (6A88) does not necessarily refer to keys, VerificationFailed is 0x6300 which is just a warning and the standard does not mean it has anything to do with verification.

[sosthene-nitrokey]

I think I have found a better option: simply make the Unknown variant #[doc(hidden)]

[sosthene-nitrokey]

Done by #17