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

argon2: std feature pulls in rand number generator #250

Closed
webmaster128 opened this issue Oct 18, 2021 · 12 comments
Closed

argon2: std feature pulls in rand number generator #250

webmaster128 opened this issue Oct 18, 2021 · 12 comments

Comments

@webmaster128
Copy link

I'm not sure to what degree this is expected, but if I enable the std feature in argon2, it pulls in a random number generator via getrandom. This is unfortunately not available in all std environments, especially when compiling to Wasm.

argon2 = { version = "0.3.1", default-features = false, features = ["std"] }

causes a compile fail with target wasm32-unknown-unknown because

$ cargo tree -i getrandom --target wasm32-unknown-unknown
getrandom v0.2.3
└── rand_core v0.6.3
    └── password-hash v0.3.2
        └── argon2 v0.3.1
            └── argon2id-js v0.1.0 (/projects/cosmjs/wasm/argon2id-js)

Seems like password-hash gets pulled in implicitly with default arguments or something like that.

Adding resolver = "2" does not seems to change things.

Using alloc instead lets me compile.

@tarcieri
Copy link
Member

It's definitely expected. For any of our crates that rely on an RNG, we're trying to make it possible to pull everything required in without a complicated recipe that involves things like upgrading rand_core in lockstep. Our experience has been that's confusing to end users.

It's unfortunate there's no unified RNG option for WASM, however. It seems like the best we can offer is explicitly linking to getrandom and toggling on something like the js feature transitively.

I suppose we should split out something like a rand feature from the std feature that toggles on rand_core/std at the very least.

@webmaster128
Copy link
Author

I suppose we should split out something like a rand feature from the std feature that toggles on rand_core/std at the very least.

What is important for me here is to keep the rng out. Adding the rnd implicitly via std surprised me, especially because there are Argon2 features "password-hash" and "rand" already that I did not enable.

@tarcieri
Copy link
Member

tarcieri commented Oct 18, 2021

Yes, apologies we don't have the WASM ergonomics figured out as we are trying to optimize for more traditional std users.

If you have any suggestions regarding what we can do in order to properly activate RNG features for WASM users, that would be helpful. For example, what is your use case: are you inside a JS environment? What RNG do you have access to?

@webmaster128
Copy link
Author

webmaster128 commented Oct 18, 2021

If you have any suggestions regarding what we can do in order to properly activate RNG features for WASM users, that would be helpful.

I'll think about what that could mean for the feature organization.

For example, what is your use case: are you inside a JS environment? What RNG do you have access to?

Yes, I develop Wasm for a JS environment. But as long as the use case allows me to provide random data to Wasm, the crate does not need to access the entropy source. In this particular use case I create the salt in the host and provide it to the guest in something like (quick'n'dirty PoC):

#[wasm_bindgen]
pub fn hash(password: &str, salt: &[u8]) -> Result<Vec<u8>, JsValue> {
  let argon2 = Argon2::new(
    Algorithm::Argon2id,
    Version::V0x13,
    Params::new(2, 3, 4, Some(32)).map_err(|e| e.to_string())?,
  );

  let mut out = Vec::<u8>::with_capacity(32);

  argon2
    .hash_password_into(password.as_bytes(), salt, &mut out)
    .map_err(|e| e.to_string())?;

  Ok(out)
}

The JS host can then access environment specific entropy sources.

@tarcieri
Copy link
Member

tarcieri commented Oct 18, 2021

I think the best way forward here is to properly split out rand and rand_core features, and have rand activate rand_core/std, rather than std itself

@newpavlov
Copy link
Member

newpavlov commented Oct 18, 2021

I would argue that it's the Rust handling of the wasm32-unknown-unknown target is at fault here. It never should've been an std target. Note that rand_core also pulls getrandom when its std feature is enabled, so I would say you either should not enable the std feature for argon2 and use alloc only or rely on getrandom's js feature.

@webmaster128
Copy link
Author

webmaster128 commented Oct 19, 2021

I would argue that it's the Rust handling of the wasm32-unknown-unknown target is at fault here. It never should've been an std target.

Good question: Is std and entropy source orthogonal? So far my thinking is yes, as getrandom uses OS specific calls and nothing from std:: (so I guess there is no entropy source in std::).

On the otherhand as you pointed out rand_core also pulls getrandom when its std feature is enabled, but lookinh at the history of this fact, it does not seem convincing this is the final answer:
Bildschirmfoto 2021-10-19 um 09 01 48
rust-random/rand#800


you either should not enable the std feature for argon2 and use alloc only

I can do that, it works. But I fear missing out on features that are available in std but not alloc. It would be great to not only solve this particular use case but find a pattern we're happy with for all compile crypto lib from Rust to web use cases for me and other users.

@newpavlov
Copy link
Member

newpavlov commented Oct 19, 2021

so I guess there is no entropy source in std::

Well, there is, but it used only for seeding hash maps for DoS protection. This is one of the reasons, why I hope that getrandom will be eventually incorporated into standard distribution like alloc.

But I fear missing out on features that are available in std but not alloc.

IIRC the main feature dependent on std is implementation of the std::error::Error trait on error types. In some places we also have std::io::Write/Read bridges and that's probably it. So I think only enabling alloc should be perfectly fine. Also I would argue that instead of relying on user-written JS code to pass entropy to WASM you should prefer using getrandom either way (unless you bother with wrapping WASM modules manually without relying on wasm-bindgen).

@webmaster128
Copy link
Author

Also I would argue that instead of relying on user-written JS code to pass entropy to WASM you should prefer using getrandom either way (unless you bother with wrapping WASM modules manually without relying on wasm-bindgen).

TL;DR: It's complicated.

That sounds so simple in theory. Unfortunately wasm-bindgen is an best effort approch in a horrible mess of JS module systems with bad support for Wasm asset compilation. Last time I tried it was impossible to write a JS lib that includes Wasm and works with webpack4+5 and CommonJS. If you try to achive async Wasm compiling it gets even worse. There is still no embed Wasm blob as text target, which is the only compatible solution (used by e.g. long.js and libsodium.js). If you need to support multiple JS environments, the best you get out of wasm-bindgen is an interface on the Wasm side and a draft of a JS wrapper that you then need to modify. Now with getrandom+js you suddenly get not one interface but two interfaces to worry about (call into Wasm and call out of Wasm).

I'm not saying every application developer should mess around with entropy sources. But a well maintained JS library that bundles Wasm is certainly able to use JS system RNGs and pass the random data to Wasm from outside.

@tarcieri
Copy link
Member

tarcieri commented Jan 23, 2022

It seems like what we should actually do here is have a getrandom feature which activates rand_core/getrandom.

@webmaster128
Copy link
Author

webmaster128 commented Jan 23, 2022

which activates rand_core/getrandom.

I think the problem is rand_core/std already pulls in getrandom. So we either need to convice the authors this is not a good idea or deactivate rand_core/std when the getrandom feature is not set.

@tarcieri
Copy link
Member

tarcieri commented Mar 5, 2023

As you said, this issue is ultimately that rand_core/std activates getrandom. We can't do anything about it here.

Given that, closing this issue.

@tarcieri tarcieri closed this as not planned Won't fix, can't repro, duplicate, stale Mar 5, 2023
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

No branches or pull requests

3 participants