Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add WithError trait to check for error after using RustCrypto API #429

Merged
merged 32 commits into from
Apr 22, 2024

Conversation

zhouwfang
Copy link
Member

To resolve #176.

Please let me know any issue or improvement. Thanks!

  1. I was wondering if there is any existing test in wasefire/examples that could be used to check if these changes break anything, especially using FixedOutputReset for the Hmac trait.

  2. What Rust format tool do you use? As you can see, I use a different format tool and I'm happy to change to yours.

@zhouwfang zhouwfang requested a review from ia0 as a code owner April 17, 2024 05:06
Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

  1. You can just run ./scripts/ci.sh to run all CI locally. However, if you don't want to modify the changelogs just yet, you can run ./scripts/ci-tests.sh which runs the tests of all crates, and ./scripts/hwci.sh host --no-default-features which runs all applet tests. This last one will exerce those APIs (in particular the hash_test and ec_test for example).

Actually, thinking about it, I think a better approach that would statically avoid mistakes, is to define a wrapper API on top of board::crypto::Api that actually return errors such that last_error is handled internally. It would look like this:

pub struct HashApi<T: Hash>(T);
pub struct HmacApi<T: Hmac>(T);

impl<T: Hash> HashApi<T> {
    pub fn new() -> Result<Self, Error> { ... last_error ... }
    pub fn update(&mut self, data: &[u8]) -> Result<(), Error> { ... last_error ... }
    ...
}
// same for Hmac
  1. I use rustfmt (through rust-analyzer, but this shouldn't change anything) which should use the top-level rustfmt.toml configuration file. cargo fmt should also work the same.

@ia0
Copy link
Member

ia0 commented Apr 17, 2024

By the way, I've merged #431 to ensure the board API is documented. You'll have to rebase (the conflicts should be trivial, that PR is just adding documentation).

Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work! This is going in the right direction from what I can see. It seems you still need to do the same as new but for update and finalize.

@zhouwfang
Copy link
Member Author

Actually, thinking about it, I think a better approach that would statically avoid mistakes, is to define a wrapper API on top of board::crypto::Api that actually return errors such that last_error is handled internally. It would look like this:

pub struct HashApi<T: Hash>(T);
pub struct HmacApi<T: Hmac>(T);

impl<T: Hash> HashApi<T> {
    pub fn new() -> Result<Self, Error> { ... last_error ... }
    pub fn update(&mut self, data: &[u8]) -> Result<(), Error> { ... last_error ... }
    ...
}
// same for Hmac

Good idea! (I also had a similar feeling -- need something similar to RAII in C++.)

Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good, it's getting closer. I suspect there might be some difficulties with imports eventually, but we'll deal with them once the rest is fixed.

@zhouwfang zhouwfang requested a review from ia0 April 21, 2024 05:15
@zhouwfang zhouwfang marked this pull request as draft April 21, 2024 05:17
@zhouwfang zhouwfang removed the request for review from ia0 April 21, 2024 05:21
@zhouwfang zhouwfang marked this pull request as ready for review April 21, 2024 05:59
@zhouwfang zhouwfang requested a review from ia0 April 21, 2024 05:59
Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I suspect after this iteration, it should be pretty close to code-complete. Then it will be about following the CI errors and fixing them.

@zhouwfang zhouwfang requested a review from ia0 April 21, 2024 22:53
#[cfg(feature = "software-crypto-sha256")]
impl LastError for sha2::Sha256 {
fn last_error(&self) -> Result<(), Error> {
// TODO: Implement the error.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct. The RustCrypto implementation does not fail. I'll remove the comments in my review commit. Actually I introduced a NoError helper trait.

where D: Support<bool> + Default + BlockSizeUser + Update + FixedOutputReset + HashMarker + LastError
{
fn last_error(&self) -> Result<(), Error> {
// TODO: Implement the error.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this one needs to propagate the hash last error. I'll update it in my commit because I actually realized the LastError signature is too strong. We don't always have access to the object. So I'm changing it to WithError which makes it harder to implement, but it works in our internal use-case.

@ia0 ia0 merged commit bd5a2f2 into google:main Apr 22, 2024
18 checks passed
@zhouwfang zhouwfang changed the title Add LastError trait to check for error after using RustCrypto API Add WithError trait to check for error after using RustCrypto API Apr 23, 2024
@ia0 ia0 added crate:board Modifies the board API for:maintainability Improves maintainers life labels Apr 29, 2024
chris-dietz pushed a commit to chris-dietz/wasefire that referenced this pull request Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate:board Modifies the board API for:maintainability Improves maintainers life
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add error support when RustCrypto doesn't
2 participants