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

Support HMAC algorithms #179

Merged
merged 4 commits into from
Aug 22, 2024
Merged

Support HMAC algorithms #179

merged 4 commits into from
Aug 22, 2024

Conversation

bboilot-ledger
Copy link
Contributor

@bboilot-ledger bboilot-ledger commented Aug 16, 2024

Hello 👋

This PR adds the support of HMAC for Ripmd160, sha256 and sha512 to the SDK with a similar implementation as what has been done for hashes algorithm.

This will be needed for https://github.com/LedgerHQ/vanadium/issues/2

Few things to note:

  • sha224 and sha384 are not supported because the C SDK does not implement the related *_init_no_throw methods. I have no idea on how to handle the functions that throw from rust plus the macro would not be generic anymore.
  • I did not bump the SDK minor version since I don't know the current versioning flow.

Let me know if any change is needed.

@bboilot-ledger
Copy link
Contributor Author

Added sha224 and sha384 (these functions actually do no throw) but I had to write 2 macro rules since I don't know how to do a condition based on the function name.


fn new(key: &[u8]) -> Self {
let mut ctx: $typename = Default::default();
let _err = unsafe { $initfname(&mut ctx.ctx, key.as_ptr(), key.len() as u32) };
Copy link
Contributor

Choose a reason for hiding this comment

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

@bboilot-ledger I am not sure to understand why two different versions of impl_hmac are required. In the C SDK, no_throw suffixed functions take a size_t as key length parameter whereas throw functions take an unsigned int key length parameter. On a 32-bit platform, both are 32 bits so passing key.len() (usize) should be ok ? Or am I missing anything 🤔 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely agree with you. But the compiler is not really happy about that:
image
(same result if I try the opposite way).

I did try to check dynamically the function name and cast key.len() to the right type accordingly (which avoids code duplication) but still, it won't compile because the check is done at run time. I did not find a way to do "conditional" macros.
I may be missing something there (I am quite new on Rust), so if you see a way of avoiding code duplication, I will be super happy to implement it!

Copy link
Contributor

@yogh333 yogh333 Aug 22, 2024

Choose a reason for hiding this comment

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

Looks like using try_into() enables to avoid code duplication:

fn new(key: &[u8]) -> Self {
    let mut ctx: $typename = Default::default();
    let _err = unsafe {
        $initfname(&mut ctx.ctx, key.as_ptr(), key.len().try_into().unwrap())
    };
    ctx
}

See my commit below...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice, I did not think about that. Thanks for the help @yogh333 !

@yogh333 yogh333 self-requested a review August 22, 2024 08:10
@bboilot-ledger bboilot-ledger merged commit 37c5852 into master Aug 22, 2024
43 checks passed
@bboilot-ledger bboilot-ledger deleted the bboilot/hmac branch August 22, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants