feat(crypto): Initial support for SubtleCrypto #698

nabetti1720 · 2024-11-23T12:25:48Z

Issue # (if available)

Closed #184

Description of changes

With this PR, we're making a small step forward with SubtleCrypto.
We're not yet fully web standards compliant, but we wanted to share our progress so far.

Checklist

Created unit tests in tests/unit and/or in Rust for my feature if needed
Ran make fix to format JS and apply Clippy auto fixes
Made sure my code didn't add any additional warnings: make check
Added relevant type info in types/ directory
Updated documentation if needed (API.md/README.md/Other)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Sytten · 2024-11-23T14:12:39Z

Nice!!!
Two comments:

We will need CryptoKey / CryptoKeyPair classes
Most methods need to be async as let spec and we will want to use tokio spawn_blocking I think.

richarddavison · 2024-11-23T22:31:18Z

Super nice! Yeah, async needs to use ctx.spawn_exit(). For example see random_fill vs random_fill_sync in crypto module.

Sytten · 2024-11-23T23:15:01Z

@richarddavison Not in this case I think, spawn_exit is only for running background tasks. Here we want to asyncify a blocking task, so you have to use the tokio thread pool for that. Same as when we do file operations.

nabetti1720 · 2024-11-24T05:18:04Z

@richarddavison Not in this case I think, spawn_exit is only for running background tasks. Here we want to asyncify a blocking task, so you have to use the tokio thread pool for that. Same as when we do file operations.

There is still debate as to which parts of the implementation should be asynchronous, but for the time being, I think it will be meaningful to simply add "async" to a function in order to conform to the specifications.

For example, the following BLOB functions appear to be marked as "async" to comply with the specification, even though no asynchronous operations are actually being invoked:

llrt/modules/llrt_http/src/blob.rs

Lines 91 to 102 in aa742eb

    
           pub async fn text(&mut self) -> String { 
        
               String::from_utf8_lossy(&self.data).to_string() 
        
           } 
        
           #[qjs(rename = "arrayBuffer")] 
        
           pub async fn array_buffer<'js>(&self, ctx: Ctx<'js>) -> Result<ArrayBuffer<'js>> { 
        
               ArrayBuffer::new(ctx, self.data.to_vec()) 
        
           } 
        
           pub async fn bytes<'js>(&self, ctx: Ctx<'js>) -> Result<Value<'js>> { 
        
               TypedArray::new(ctx, self.data.to_vec()).map(|m| m.into_value()) 
        
           }

nabetti1720 · 2024-11-24T05:49:33Z

We will need CryptoKey / CryptoKeyPair classes

I know it's necessary, but I'm not sure how to implement it yet.

For example, when you run the following code on bun, generateKey is returned as a CryptoKey type, but when you look at it in console.log, it is an ordinary object.

// subtleGenerateKey.js
async function exportCryptoKey(key) {
  const exported = await crypto.subtle.exportKey("raw", key);
  return new Uint8Array(exported);
}

const key = await crypto.subtle
  .generateKey(
    {
      name: "AES-GCM",
      length: 256,
    },
    true,
    ["encrypt", "decrypt"],
  );

console.log(key);
console.log(await exportCryptoKey(key));
console.log(await exportCryptoKey(key));

% bun subtleGenerateKey.js
CryptoKey {
  type: "secret",
  extractable: true,
  algorithm: {
    name: "AES-GCM",
    length: 256,
  },
  usages: [ "decrypt", "encrypt" ],
}
Uint8Array(32) [ 228, 104, 149, 176, 21, 129, 162, 92, 75, 1, 147, 65, 121, 50, 242, 28, 144, 65, 103, 203, 240, 167, 105, 60, 37, 247, 158, 156, 204, 127, 94, 16 ]
Uint8Array(32) [ 228, 104, 149, 176, 21, 129, 162, 92, 75, 1, 147, 65, 121, 50, 242, 28, 144, 65, 103, 203, 240, 167, 105, 60, 37, 247, 158, 156, 204, 127, 94, 16 ]

Nevertheless, since the same value is returned even if you run exportKey multiple times, this object must have hidden information (seed).

https://w3c.github.io/webcrypto/#cryptokey-interface-internal-slots

richarddavison

Some comments

richarddavison · 2024-11-24T12:12:04Z

modules/llrt_crypto/Cargo.toml

Do we really need all these deps? I know SHA1, HMAC is available in ring and used already in crypto module

modules/llrt_crypto/src/subtle/mod.rs

modules/llrt_crypto/src/subtle/decrypt.rs

modules/llrt_crypto/src/subtle/derive_bits.rs

richarddavison · 2024-11-24T18:51:18Z

modules/llrt_crypto/src/subtle/digest.rs

+use sha1::Sha1;
+use sha2::{Digest, Sha256, Sha384, Sha512};


Use from ring? ring::digest

richarddavison · 2024-11-24T19:02:05Z

modules/llrt_crypto/Cargo.toml

+hmac = "0.12"
+sha1 = "0.10"
+sha2 = "0.10"
+aes = "0.8"
+aes-gcm = "0.10"
+cbc = { version = "0.1", features = ["std"] }
+num-traits = "0.2"
+rsa = { version = "0.9", features = ["std", "sha2"], default-features = false }
+p256 = { version = "0.13", features = ["ecdh"] }
+p384 = "0.13"
+ctr = "0.9"
+


A lot of these are already in the ring crate. Use them instead of pulling in separate deps for each signature, hash etc. The only thing that ring doesn't have (yet) is AES encryption AND decryption so we need to keep ctr and cbc and cgm crates until its supported:
briansmith/ring#588 (comment)

Hmm... I'm thinking of replacing some of the ring parts, but even though the names are the same, the methods are different, so it's not going to work.

It looks like I'll need to spend some time on this...

I think this should basically be equivalent:

hmac = ring::hmac
sha1 = ring::digest
sha2 = ring::digest
aes = keep as ring only has encryption and not decryption
aes-gcm = keep as ring only has encryption and not decryption
cbc = keep as ring only has encryption and not decryption
rsa = ring::rsa
p256 = ring:.agreement
p384 = ring:.agreement
~~ctr = keep as ring only has encryption and not decryption~~

Take RSA for example.

// Cargo.toml ... #rsa = { version = "0.9", features = ["std", "sha2"], default-features = false }

Then, if you replace use rsa::... with use ring::rsa::..., you will get an error saying that the import cannot be resolved.

If the command doesn't exist, do we need to look for an alias? If it still doesn't exist, do we need to implement it?

For RSA I don't think ring can decrypt. So we probably need to keep it as well. Rings API is totally different then what you already have so you have to change it when switching to ring (where applicable)

As for the other components, they are completely different from those built into the ring, so it would be difficult to replace them as is.

Even if it were possible, I would like to consider it in a separate PR.

Sure but I'll rather not merge duplicate dependencies. We already use ring for sha hash and hmac. And these can be used as well:

p256 = ring:.agreement p384 = ring:.agreement

If you don't use the p256/p384 crates I don't understand how the implementation below replaces it. Are these features implemented in ring?

modules/llrt_crypto/src/subtle/derive_bits.rs:

p256::SecretKey::from_pkcs8_der()

p256::elliptic_curve::ecdh::diffie_hellman()

p384::SecretKey::from_pkcs8_der()

p384::elliptic_curve::ecdh::diffie_hellman()

modules/llrt_crypto/src/subtle/sign.rs

modules/llrt_crypto/src/subtle/mod.rs

modules/llrt_crypto/src/subtle/verify.rs

modules/llrt_crypto/src/subtle/mod.rs

nabetti1720 · 2024-11-25T23:17:57Z

We will need CryptoKey / CryptoKeyPair classes

I know it's necessary, but I'm not sure how to implement it yet.

Nevertheless, since the same value is returned even if you run exportKey multiple times, this object must have hidden information (seed).

https://w3c.github.io/webcrypto/#cryptokey-interface-internal-slots

@richarddavison and @Sytten , Any good ideas on how to implement this? Is it possible to retain values in LLRT that are not displayed in console.log?

Sytten · 2024-11-25T23:28:09Z

I would create a class to wrap the vec at the very least so the user cant assume it is an array. Then we can start implementing the props the spec requires.

richarddavison · 2024-11-26T08:30:41Z

@richarddavison and @Sytten , Any good ideas on how to implement this? Is it possible to retain values in LLRT that are not displayed in console.log?

Yes, create a Class for CryptoKey and CryptoKeyPair and keep hidden fields in rust only. Then expose only getters to what you need to provide as read only:

#[rquickjs::class]
#[derive(rquickjs::JsLifetime)]
struct CryptoKey<'js>{
   algorithm: Object<'js>,
   usages: Array<'js>,
   ...
}

impl<'js> CryptoKey<'js>{

    #[qjs(get)]
    pub fn algorithm(&self) -> Object<'js> {
        self.algorithm.clone()
    }
}

modules/llrt_crypto/src/subtle/mod.rs

richarddavison · 2024-11-26T08:23:42Z

modules/llrt_crypto/Cargo.toml

+hmac = "0.12"
+sha1 = "0.10"
+sha2 = "0.10"
+aes = "0.8"
+aes-gcm = "0.10"
+cbc = { version = "0.1", features = ["std"] }
+num-traits = "0.2"
+rsa = { version = "0.9", features = ["std", "sha2"], default-features = false }
+p256 = { version = "0.13", features = ["ecdh"] }
+p384 = "0.13"
+ctr = "0.9"
+


Sure but I'll rather not merge duplicate dependencies. We already use ring for sha hash and hmac. And these can be used as well:

p256 = ring:.agreement p384 = ring:.agreement

modules/llrt_crypto/src/subtle/decrypt.rs

modules/llrt_crypto/src/subtle/encrypt.rs

modules/llrt_crypto/src/subtle/mod.rs

richarddavison

Fantastic work so far! 🥇

richarddavison · 2024-11-26T18:38:24Z

modules/llrt_crypto/src/subtle/digest.rs

+        algorithm
+            .get_optional::<_, String>("name")?
+            .ok_or_else(|| {
+                Exception::throw_message(&ctx, "Missing algorithm name should cause TypeError")


This is sounds like a comment. I think we can return simply: algorithm 'name' property required. Also if its a type error, it should be Exception::throw_type for TypeErrors

richarddavison · 2024-11-27T19:10:08Z

modules/llrt_crypto/src/subtle/generate_key.rs

+        let obj = Object::new(ctx)?;
+        obj.set("privateKey", private)?;
+        obj.set("publicKey", public)?;
+        Ok(obj.into())


Shouldn't this be a class? CryptoKeyPair? Or is it fine with an object?

feat(crypto): Initial support for SubtleCrypto

7de75c9

nabetti1720 marked this pull request as draft November 23, 2024 12:40

Refactor extract logic

a4d8321

Asynchronous support and tiny fix

0f4050e

nabetti1720 added 2 commits November 24, 2024 20:30

Refactor arguments and return values

5161cce

Refactor argument names

a498919

richarddavison requested changes Nov 24, 2024

View reviewed changes

nabetti1720 added 5 commits November 25, 2024 20:03

Use ObjectBytes

1588058

Refactor Error Handling

e34046a

Fix

be813aa

Use common SYSTEM_RANDOM

5c1a038

Refactor

4187f5a

richarddavison requested changes Nov 25, 2024

View reviewed changes

modules/llrt_crypto/src/subtle/verify.rs Outdated Show resolved Hide resolved

modules/llrt_crypto/src/subtle/verify.rs Outdated Show resolved Hide resolved

nabetti1720 added 2 commits November 25, 2024 22:36

Remove unnecessary to_vec()

095d5ac

Remove more unnecessary to_vec()

2a9a2ab

richarddavison requested changes Nov 25, 2024

View reviewed changes

Optimize ArrayBuffer arguments

51af5db

richarddavison requested changes Nov 26, 2024

View reviewed changes

nabetti1720 added 2 commits November 26, 2024 20:05

chore

87e2c0e

Implement try_into() for enums

ae81bbc

richarddavison requested changes Nov 26, 2024

View reviewed changes

modules/llrt_crypto/src/subtle/mod.rs Outdated Show resolved Hide resolved

modules/llrt_crypto/src/subtle/mod.rs Outdated Show resolved Hide resolved

Refactor the structure

e996284

nabetti1720 added 6 commits November 26, 2024 22:04

Fix error messages

9a12840

Refactor

d3ea7f3

Create CryptoKey Object

7d9cb6f

Fix

4c19eb5

Refactor generateKey

ab0b0a7

Expose CryptoKey and CryptoKeyPair

988590a

richarddavison requested changes Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(crypto): Initial support for SubtleCrypto #698

feat(crypto): Initial support for SubtleCrypto #698

nabetti1720 commented Nov 23, 2024 •

edited

Loading

Sytten commented Nov 23, 2024

richarddavison commented Nov 23, 2024

Sytten commented Nov 23, 2024

nabetti1720 commented Nov 24, 2024 •

edited

Loading

nabetti1720 commented Nov 24, 2024 •

edited

Loading

richarddavison left a comment

richarddavison Nov 24, 2024

richarddavison Nov 24, 2024

richarddavison Nov 24, 2024

nabetti1720 Nov 25, 2024

richarddavison Nov 25, 2024

nabetti1720 Nov 25, 2024

richarddavison Nov 25, 2024

nabetti1720 Nov 25, 2024

richarddavison Nov 26, 2024

nabetti1720 Nov 26, 2024 •

edited

Loading

nabetti1720 commented Nov 25, 2024

Sytten commented Nov 25, 2024

richarddavison commented Nov 26, 2024

richarddavison Nov 26, 2024

richarddavison left a comment

richarddavison Nov 26, 2024

richarddavison Nov 27, 2024

feat(crypto): Initial support for SubtleCrypto #698

Are you sure you want to change the base?

feat(crypto): Initial support for SubtleCrypto #698

Conversation

nabetti1720 commented Nov 23, 2024 • edited Loading

Issue # (if available)

Description of changes

Checklist

Sytten commented Nov 23, 2024

richarddavison commented Nov 23, 2024

Sytten commented Nov 23, 2024

nabetti1720 commented Nov 24, 2024 • edited Loading

nabetti1720 commented Nov 24, 2024 • edited Loading

richarddavison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabetti1720 Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

nabetti1720 commented Nov 25, 2024

Sytten commented Nov 25, 2024

richarddavison commented Nov 26, 2024

Choose a reason for hiding this comment

richarddavison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabetti1720 commented Nov 23, 2024 •

edited

Loading

nabetti1720 commented Nov 24, 2024 •

edited

Loading

nabetti1720 commented Nov 24, 2024 •

edited

Loading

nabetti1720 Nov 26, 2024 •

edited

Loading