RustCrypto / password-hashes

Password hashing functions / KDFs
611 stars 79 forks source link

argon2: std feature pulls in rand number generator #250

Closed webmaster128 closed 1 year ago

webmaster128 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 _randcore 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

https://github.com/rust-random/rand/pull/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

webmaster128 commented 2 years ago

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 commented 1 year ago

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.