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

move verify method to tests #112

Open
skaunov opened this issue Jun 15, 2024 · 0 comments
Open

move verify method to tests #112

skaunov opened this issue Jun 15, 2024 · 0 comments

Comments

@skaunov
Copy link
Collaborator

skaunov commented Jun 15, 2024

leave a note that secret values handling isn't appropriately implemented there

@skaunov skaunov converted this from a draft issue Jun 15, 2024
@skaunov skaunov changed the title improve secret values handling in verify move verify method to tests Jun 15, 2024
skaunov added a commit that referenced this issue Jul 7, 2024
# Subtle
I was really late to understand that Subtle crypto supports the different curve `secp256r`, *and* it doesn't provide a facility to store secret values. So implementation for `web_sys::SecretKey` turned out to be just extra miles leading nowhere.
```toml
web-sys = { version = "0.3", features = ["CryptoKey", "SubtleCrypto", "Crypto", "EcKeyImportParams"] }
wasm-bindgen-futures = "0.4"
```
```rust
#[wasm_bindgen]
extern "C" {
    // Return type of js_sys::global()
    type Global;
    // // Web Crypto API: Crypto interface (https://www.w3.org/TR/WebCryptoAPI/)
    // type WebCrypto;
    // Getters for the WebCrypto API
    #[wasm_bindgen(method, getter)]
    fn crypto(this: &Global) -> web_sys::Crypto;
}

// `fn sign`
if sk.type_() != "secret" {return Err(JsError::new("`sk` must be secret key"))}
if !js_sys::Object::values(&sk.algorithm().map_err(
    |er|
        JsError::new(er.as_string().expect("TODO check this failing").as_str())
)?).includes(&JsValue::from_str("P-256"), 0) {return Err(JsError::new("`sk` must be from `secp256`"))}

// this was my approach, but seems I got what they did at <https://github.com/rust-random/getrandom/blob/master/src/js.rs>
// js_sys::global().entries().find(); // TODO throw if no Crypto in global

let global_the: Global = js_sys::global().unchecked_into();
let crypto_the: web_sys::Crypto = global_the.crypto();
let subtle_the = crypto_the.subtle();
let sk = JsFuture::from(subtle_the.export_key("pkcs8", &sk)?).await?;

// ...
::from_pkcs8_der(js_sys::ArrayBuffer::from(sk).try_into()?)?;
    zeroize::Zeroizing::new(js_sys::Uint8Array::from(JsFuture::from(subtle_the.export_key("pkcs8", &sk).map_err(
        |er|
            Err(JsError::new(er.as_string().expect("TODO check this failing").as_str()))
        )?).await?).to_vec());

// ...

// `fn try_into`

// ...

// zeroization protection ommitted here due to deprecation // <#112>
// mostly boilerplate from signing; also some excessive ops left for the same reason
// TODO align error-handling in this part
if self.c.type_() != "secret" {return Err(JsError::new("`c` must be secret key"))}
if !js_sys::Object::values(&self.c.algorithm()?).includes(js_sys::JsString::from("P-256").into(), 0) {return Err(JsError::new("`c` must be from `secp256`"))}
this was my approach, but seems I got what they did at <https://github.com/rust-random/getrandom/blob/master/src/js.rs>
js_sys::global().entries().find(); // TODO throw if no Crypto in global
let global_the: Global = js_sys::global().unchecked_into();
let crypto_the: web_sys::Crypto = global_the.crypto();
let subtle_the = crypto_the.subtle();
let c_pkcs = //zeroize::Zeroizing::new(
    js_sys::Uint8Array::from(JsFuture::from(subtle_the.export_key("pkcs8", &self.c)?).await?).to_vec();
// );
let c_scalar = &plume_rustcrypto::SecretKey::from_pkcs8_der(&c_pkcs)?.to_nonzero_scalar();
sk_z.zeroize();

// ...
```

# randomness
Somehow I thought Wasm doesn't have access to RNG, so I used a seedable one and required the seed. Here's how `sign` `fn` was different.
```rust
// Wasm environment doesn't have a suitable way to get randomness for the signing process, so this instantiates ChaCha20 RNG with the provided seed.
// @throws a "crypto error" in case of a problem with the secret key, and a verbal error on a problem with `seed`
// @param {Uint8Array} seed - must be exactly 32 bytes.
pub fn sign(seed: &mut [u8], v1: bool, sk: &mut [u8], msg: &[u8]) -> Result<PlumeSignature, JsError> {
    // ...

    let seed_z: zeroize::Zeroizing<[u8; 32]> = zeroize::Zeroizing::new(seed.try_into()?);
    seed.zeroize();

    // TODO switch to `wasi-random` when that will be ready for crypto
    let sig = match v1 {
        true => plume_rustcrypto::PlumeSignature::sign_v1(
            &sk_z, msg, &mut rand_chacha::ChaCha20Rng::from_seed(seed_z)
        ),
        false => plume_rustcrypto::PlumeSignature::sign_v2(
            &sk_z, msg, &mut rand_chacha::ChaCha20Rng::from_seed(seed_z)
        ),
    };

    let sig = signer.sign_with_rng(
        &mut rand_chacha::ChaCha20Rng::from_seed(*seed_z), msg
    );

    // ...
}
```

# `BigInt` conversion
It was appealing to leave `s` as `BigInt` (see the comments), but that seems to be confusing and hinder downstream code reusage. There's an util function left for anybody who would want to have it as `BigInt`, but leaving the contraty function makes less sense and also makes the thing larger. So let me left it here for reference.
```rust
let scalar_from_bigint =
    |n: js_sys::BigInt| -> Result<plume_rustcrypto::NonZeroScalar, anyhow::Error> {
        let result = plume_rustcrypto::NonZeroScalar::from_repr(k256::FieldBytes::from_slice(
            hex::decode({
                let hexstring_freelen = n.to_string(16).map_err(
                    |er|
                        anyhow::Error::msg(er.as_string().expect("`RangeError` can be printed out"))
                )?.as_string().expect("on `JsString` this always produce a `String`");
                let l = hexstring_freelen.len();
                if l > 32*2 {return Err(anyhow::Error::msg("too many digits"))}
                else {["0".repeat(64-l), hexstring_freelen].concat()}
            })?.as_slice()
        ).to_owned());
        if result.is_none().into() {Err(anyhow::Error::msg("isn't valid `secp256` non-zero scalar"))}
        else {Ok(result.expect(EXPECT_NONEALREADYCHECKED))}
    };
```
skaunov added a commit that referenced this issue Jul 17, 2024
Resolves #114

removes respective GA
this one should be tested like a package I guess,
ideas for such tests are welcome as issues

Couple of things left out of the committed code.

# Subtle
I was really late to understand that Subtle crypto supports the different curve `secp256r`, *and* it doesn't provide a facility to store secret values. So implementation for `web_sys::SecretKey` turned out to be just extra miles leading nowhere.
```toml
web-sys = { version = "0.3", features = ["CryptoKey", "SubtleCrypto", "Crypto", "EcKeyImportParams"] }
wasm-bindgen-futures = "0.4"
```
```rust
#[wasm_bindgen]
extern "C" {
    // Return type of js_sys::global()
    type Global;
    // // Web Crypto API: Crypto interface (https://www.w3.org/TR/WebCryptoAPI/)
    // type WebCrypto;
    // Getters for the WebCrypto API
    #[wasm_bindgen(method, getter)]
    fn crypto(this: &Global) -> web_sys::Crypto;
}

// `fn sign`
if sk.type_() != "secret" {return Err(JsError::new("`sk` must be secret key"))}
if !js_sys::Object::values(&sk.algorithm().map_err(
    |er|
        JsError::new(er.as_string().expect("TODO check this failing").as_str())
)?).includes(&JsValue::from_str("P-256"), 0) {return Err(JsError::new("`sk` must be from `secp256`"))}

// this was my approach, but seems I got what they did at <https://github.com/rust-random/getrandom/blob/master/src/js.rs>
// js_sys::global().entries().find(); // TODO throw if no Crypto in global

let global_the: Global = js_sys::global().unchecked_into();
let crypto_the: web_sys::Crypto = global_the.crypto();
let subtle_the = crypto_the.subtle();
let sk = JsFuture::from(subtle_the.export_key("pkcs8", &sk)?).await?;

// ...
::from_pkcs8_der(js_sys::ArrayBuffer::from(sk).try_into()?)?;
    zeroize::Zeroizing::new(js_sys::Uint8Array::from(JsFuture::from(subtle_the.export_key("pkcs8", &sk).map_err(
        |er|
            Err(JsError::new(er.as_string().expect("TODO check this failing").as_str()))
        )?).await?).to_vec());

// ...

// `fn try_into`

// ...

// zeroization protection ommitted here due to deprecation // <#112>
// mostly boilerplate from signing; also some excessive ops left for the same reason
// TODO align error-handling in this part
if self.c.type_() != "secret" {return Err(JsError::new("`c` must be secret key"))}
if !js_sys::Object::values(&self.c.algorithm()?).includes(js_sys::JsString::from("P-256").into(), 0) {return Err(JsError::new("`c` must be from `secp256`"))}
this was my approach, but seems I got what they did at <https://github.com/rust-random/getrandom/blob/master/src/js.rs>
js_sys::global().entries().find(); // TODO throw if no Crypto in global
let global_the: Global = js_sys::global().unchecked_into();
let crypto_the: web_sys::Crypto = global_the.crypto();
let subtle_the = crypto_the.subtle();
let c_pkcs = //zeroize::Zeroizing::new(
    js_sys::Uint8Array::from(JsFuture::from(subtle_the.export_key("pkcs8", &self.c)?).await?).to_vec();
// );
let c_scalar = &plume_rustcrypto::SecretKey::from_pkcs8_der(&c_pkcs)?.to_nonzero_scalar();
sk_z.zeroize();

// ...
```

# randomness
Somehow I thought Wasm doesn't have access to RNG, so I used a seedable one and required the seed. Here's how `sign` `fn` was different.
```rust
// Wasm environment doesn't have a suitable way to get randomness for the signing process, so this instantiates ChaCha20 RNG with the provided seed.
// @throws a "crypto error" in case of a problem with the secret key, and a verbal error on a problem with `seed`
// @param {Uint8Array} seed - must be exactly 32 bytes.
pub fn sign(seed: &mut [u8], v1: bool, sk: &mut [u8], msg: &[u8]) -> Result<PlumeSignature, JsError> {
    // ...

    let seed_z: zeroize::Zeroizing<[u8; 32]> = zeroize::Zeroizing::new(seed.try_into()?);
    seed.zeroize();

    // TODO switch to `wasi-random` when that will be ready for crypto
    let sig = match v1 {
        true => plume_rustcrypto::PlumeSignature::sign_v1(
            &sk_z, msg, &mut rand_chacha::ChaCha20Rng::from_seed(seed_z)
        ),
        false => plume_rustcrypto::PlumeSignature::sign_v2(
            &sk_z, msg, &mut rand_chacha::ChaCha20Rng::from_seed(seed_z)
        ),
    };

    let sig = signer.sign_with_rng(
        &mut rand_chacha::ChaCha20Rng::from_seed(*seed_z), msg
    );

    // ...
}
```

# `BigInt` conversion
It was appealing to leave `s` as `BigInt` (see the comments), but that seems to be confusing and hinder downstream code reusage. There's an util function left for anybody who would want to have it as `BigInt`, but leaving the contraty function makes less sense and also makes the thing larger. So let me left it here for reference.
```rust
let scalar_from_bigint =
    |n: js_sys::BigInt| -> Result<plume_rustcrypto::NonZeroScalar, anyhow::Error> {
        let result = plume_rustcrypto::NonZeroScalar::from_repr(k256::FieldBytes::from_slice(
            hex::decode({
                let hexstring_freelen = n.to_string(16).map_err(
                    |er|
                        anyhow::Error::msg(er.as_string().expect("`RangeError` can be printed out"))
                )?.as_string().expect("on `JsString` this always produce a `String`");
                let l = hexstring_freelen.len();
                if l > 32*2 {return Err(anyhow::Error::msg("too many digits"))}
                else {["0".repeat(64-l), hexstring_freelen].concat()}
            })?.as_slice()
        ).to_owned());
        if result.is_none().into() {Err(anyhow::Error::msg("isn't valid `secp256` non-zero scalar"))}
        else {Ok(result.expect(EXPECT_NONEALREADYCHECKED))}
    };
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

1 participant