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 Crypto HmacSha256 support #696

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

ewalk153
Copy link
Contributor

@ewalk153 ewalk153 commented Jul 4, 2024

Description of the change

Conforming to the WinterCG crypto Hmac convention, introduce HmacSha256 support. This can be extended to add other crypto functions in the future.

Uses promises, per the WinterCG convention.

Example code so far:

await crypto.subtle.sign({name: "HMAC", hash: "sha-256"}, "my secret and secure key", "input message");

And to be completely WinterCG complaint it will soon be:

(async (secret, body) => {
  let enc = new TextEncoder("utf-8");
  let algorithm = { name: "HMAC", hash: "SHA-256" };

  let key = await crypto.subtle.importKey("raw", enc.encode(secret), algorithm, false, ["sign", "verify"]);
  let signature = await crypto.subtle.sign(algorithm.name, key, enc.encode(body));
  
  return Array.from(new Uint8Array(signature)).map(byte => byte.toString(16).padStart(2, '0')).join('');
})("my secret and secure key", "input message");

TODO:

  • add crypto.subtle.importKey and make this the required key parameter to sign - see sign example code
  • change return value of sign to be a byte array that is converted to a string with user code such as: let hexString = Array.from(new Uint8Array(signature)).map(byte => byte.toString(16).padStart(2, '0')).join('');

Why am I making this change?

There is a strong need for for function input authentication. HmacSha256 strikes a balance between instruction count and cryptographic safety as a sane starting point.

Checklist

  • I've updated the relevant CHANGELOG files if necessary. Changes to javy-cli and javy-core do not require updating CHANGELOG files.
  • I've updated the relevant crate versions if necessary. Versioning policy for library crates
  • I've updated documentation including crate documentation if necessary.

@saulecabrera
Copy link
Member

saulecabrera commented Jul 4, 2024

Apologies for the bikeshed here, the spec is a bit hard to navigate, but after investigating a bit more, I believe that what we want is exposing crypto.suble.digest rather than createHmac which is not standard yet (that's why it was not in the doc).

Thankfully I think that this doesn't represent a huge change to the current state of your PR, in fact, I think it might make the implementation a bit easier. The only wrinkle to think about is that crypto.subtle.digest returns a promise, which I believe we can workaround by wrapping the result via: https://docs.rs/rquickjs/latest/rquickjs/struct.Promise.html#method.from_value, without having to actually defer the computation.

@saulecabrera
Copy link
Member

The other nice thing is that we can now refer to https://github.com/web-platform-tests/wpt/blob/master/WebCryptoAPI/digest/digest.https.any.js to compliance tests.

@ewalk153
Copy link
Contributor Author

ewalk153 commented Jul 4, 2024

Next step: change API signature to:

crypto.subtle.sign({name: "HMAC", hash: "sha-256"}, "my key", "my plaintext")

This aligns with the wintercg spec.

Per convo with @saulecabrera and @jeffcharles

@saulecabrera
Copy link
Member

@ewalk153 I've pushed fa3c661, see the commit description for details. Additionally, while working on these improvements, I found a bug with our event loop handling code (fixed in #697), so I'd suggest rebasing once that one lands. I didn't fix all the unit tests in my last commit here, so there's a chance that we'd need to revisit those.

ewalk153 and others added 22 commits July 15, 2024 09:19
HmacSha256::new_from_slice(&js_string_secret.as_bytes()) =>
 HmacSha256::new_from_slice(js_string_secret.as_bytes())

 No need to pass in address here.

 Same for js_string_message.
Also drop javy_ prefix in anticipation of switching to winter cg.
Ran `cargo fmt -- --check`, and applied suggestions, per ci feedback.
No clear reason on why it fails
Replace read/write message field with update field to match nodejs
crypto api.

Add tests for failure conditions.

Clean up string error handling in digest method.
- addess lint warnings
- renamed variables for clarity
- improved method names for clarity
- clarified fn definition comments
Partial implementation, including:
- crypto.subtle.sign method
- setup wpt testing infra

Missing:
- validation of sign protocol (hmac and sha256)
- add verify method
- return value promise to match API format
This commit improves the `crypto` feature handling by making the
relationship between crypto and the experimental_event_loop explicit. If
`crypto` is defined, the `experimental_event_loop` feature will also be
enbabled.

Additionally, this commit makes it easier to define the async crypto
APIs by introducing a `crypto.js` file that defines JS bits to make it
easier to return a promise.
This allows the crypto tests to run
@MurmeltierS
Copy link

sorry for chirping in: is this still being considered?

@ewalk153
Copy link
Contributor Author

Still looking to introduce a version of these crypto function into Javy, in some form.

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.

3 participants