rust-cryptoki
rust-cryptoki copied to clipboard
parallaxsecond
Suggestion: Have verification functions return `Result<bool>` instead of `Result<()>`
Hello Cryptoki team!
I'm opening this here instead of under a Discussion because it didn't seem like they were actively used, but if this would be best moved there, let me know!
I was wondering if it would make more sense for signature verification functions to return a Result<bool>, with Ok(true) indicating that signature verification passed, Ok(false) indicating that the signature did not verify for the given data, and Err(...) indicating any other error.
My reasoning for this approach is two-fold:
- l think this better aligns with the expected return value of signature verification functions
- It prevents people from having to write more boilerplate to handle wrong signatures. Otherwise, one would has to pattern match on the return error value to differentiate between true errors (as in something unexpected went wrong) and just the verification failing. IMO, this also opens the end-user up to implementation details they might not need to bother with, since they must inspect the returned error instead of treating it like an opaque value
Now of course since this is a breaking change, I wouldn't expect this to be taken lightly. Would it make sense to have another set of verification functions that return a bool? Like Let me know what you think!
I would say that when you verify signature, you only care about it being right or not, which is very well described by the Result<()> return type already.
Do you have example of the use case where you would treat "invalid signature" as less error than any other.?
I'd be kind of worried about people not checking that return type. If we went that route I think I'd use an enum instead of bool and mark it as #[must_use] so at least lints warn about the wrong usage.
Maybe a different approach would be to add another variant to the cryptoki::error::Error type to include info about the signature being wrong? 🤔
@Jakuje in my mind, an error result implies that something went wrong with the execution of a function, whereas a wrong signature implies that the verification function still terminated successfully. Likewise, if a signature doesn't pass verification, this is probably something the user would want to handle immediately at the source, as it feels much more local, whereas all other errors that might occur should probably just be bubbled up to the top. To me, while these are both errors, there is a clear distinction between the two types and what someone should do to handle them.
@wiktor-k sorry, I didn't mean to say that the user should just ignore any errors that happen. What I mean to say is: wouldn't the end-user likely just let errors propagate up to the top, without a need to inspect them themselves?
Also, what you suggest could be good solution, since it means the end-user does not need to be familiar with PKCS#11 error codes to understand that the verification failed. i.e. if we view rust-cryptoki as a simpler, more user-friendly API around PKCS#11 stuff, then we keep the user's interaction within the simpler API—they don't have to dip down into the details they would have to read the spec to understand. Though, I guess that would depend on if that's actually one of the goals of the project (and whether someone should be using this library if they haven't read the spec 😆).
Ultimately, what I was proposing was just a suggestion, so I really don't feel so strongly about it. I don't have a use case for this personally, it was just a thought. If the team doesn't think it's appropriate, or it's not worth the breaking change it would cause, I have no problem with closing this!
Nah, let's keep it open. The aim for a simpler API is definitely here, it's just there's some legacy code that needs to be updated. I'm pondering API changes from time to time too and I think it's good to shoot ideas and discuss pros and cons in the open. Thanks!
I see where you are coming with the difference between an error executing the function and a wrong signature with a perfectly functioning function. I think we are an abstraction over PKCS#11 but still trying to provide a 1-1 mapping with the interface in a Rust idiomatic way. If a PKCS11 return error has InvalidSignature then we will also have to have it in our wrapper, unless we completely change the interface.
But going in your direction it could be possible to provide other levels of abstraction on top like the RustCrypto work
Thanks for the suggestion! Seems that the consensus was to stay with the Result<()> for now. I think we can close it and re-open if this comes again as a topic!