facebook / opaque-ke

An implementation of the OPAQUE password-authenticated key exchange protocol
Apache License 2.0
296 stars 45 forks source link

SlowHash configurability #180

Closed daxpedda closed 3 years ago

daxpedda commented 3 years ago

At the moment, it isn't possible to pass parameters to SlowHash::hash(), the only way I could figure out is to make a new type implementing SlowHash that just executes a different function.

Would it be in scope to pass down a instantiated CipherSuite::SlowHash to SlowHash::hash() through either an additional parameter in ClientRegistration/ClientLogin::finish() or adding something to ClientRegistrationFinishParameters/ClientLoginFinishParameters?

kevinlewi commented 3 years ago

I see... just to clarify: are these parameters which must be set upon calling ClientRegistration/ClientLogin::finish(), and cannot be set during the setup / configuration phase? Feel free to open a PR with what you have in mind.

daxpedda commented 3 years ago

I see... just to clarify: are these parameters which must be set upon calling ClientRegistration/ClientLogin::finish(), and cannot be set during the setup / configuration phase?

No, they could be setup at any time, I just don't know where best to place it.

Feel free to open a PR with what you have in mind.

Will do.

kevinlewi commented 3 years ago

@daxpedda, some more thoughts on this after seeing the PR: can't we already support custom configurations of SlowHash without having to make any changes to master? perhaps this can be supported by just making a small change to the function signature for the hash function of the SlowHash trait, by adding a &self parameter?

For instance:

struct Scrypt {
  n: u32,
  r: u32,
  p: u32,
}

impl Scrypt {
  fn new(n: u32, r: u32, p: u32) -> Self {
    Self { n, r, p }
}

impl<D: Hash> SlowHash<D> for Scrypt {
  fn hash(
    &self,
    input: GenericArray<u8, <D as Digest>::OutputSize>,
  ) -> Result<Vec<u8>, InternalPakeError> {
    // use n,r,p as set by new()
    let params =
      scrypt::ScryptParams::new(self.n, self.r, self.p)
        .map_err(|_| InternalPakeError::SlowHashError)?;
    let mut output = vec![0u8; <D as Digest>::OutputSize::to_usize()];
    scrypt::scrypt(&input, &[], &params, &mut output)
      .map_err(|_| InternalPakeError::SlowHashError)?;
    Ok(output)
  }
}
kevinlewi commented 3 years ago

Actually nevermind, please ignore the above, I realized why it would not work as I had initially expected. I am still looking for a way to keep all of the parameters in the ciphersuite configuration rather than passing them into the finish() functions, though...

For instance, can we just have callers define multiple SlowHash options, and create multiple CipherSuite objects for each option?

struct ScryptWeak;

impl<D: Hash> SlowHash<D> for ScryptWeak {
  fn hash(
    input: GenericArray<u8, <D as Digest>::OutputSize>,
  ) -> Result<Vec<u8>, InternalPakeError> {
    // use n,r,p with some configuration
  }
}

struct ScryptStrong;

impl<D: Hash> SlowHash<D> for ScryptStrong {
  fn hash(
    input: GenericArray<u8, <D as Digest>::OutputSize>,
  ) -> Result<Vec<u8>, InternalPakeError> {
    // use n,r,p with some configuration
  }
}

struct WeakOption;
impl CipherSuite for WeakOption {
    // ...
    type SlowHash = ScryptWeak;
}

struct StrongOption;
impl CipherSuite for StrongOption {
    // ...
    type SlowHash = ScryptStrong;
}
daxpedda commented 3 years ago

This is basically how it has to be done right now. The issue is actually not really SlowHash, the issue is the whole CipherSuite, currently all settings are compile-time only as they are defined with generics, no passing down of parameters can actually happen.

Imagine I want to make multiple Hash options, like SHA-256, SHA3 and BLAKE3. I also want to provide multiple Argon2 variants and configurations. Additionally I would like to add support for P-256 and P-384. You can see combining all of these will add a huge amount of CipherSuite implementations.

One way to solve this with the current setup, is to turn CipherSuite types into instantiatable configurations that can be passed down or setup once at the start.

struct Default;
impl CipherSuite for Default {
    type Group = Groups;
    type KeyExchange = TripleDH;
    type Hash = Hashs;
    type SlowHash = SlowHashs;
}

enum Groups {
  P256,
  P384,
  RistrettoPoint,
}

enum Hashs {
  Sha256,
  Sha384,
  Sha512,
  Sha3,
  Blake3,
}

enum SlowHashs {
  Argon2id { more_configuration, ... },
  Argon2d { more_configuration, ... },
  Scrypt { more_configuration, ... },
  Pbkdf2 { more_configuration, ... },
}

This approach would require passing these instantiated objects into the relevant functions. I don't know how else to solve this problem.

Specifically SlowHash, may require additional configuration that can't be solved at all right now, like passing a salt, additional data or similar.

Alternatively CipherSuite could be changed into this:

struct CipherSuite<G, KE, H, SH>
where
  G: GroupWithMapToCurve<..>,
  KE: KeyExchange<..>,
  H: Hash,
  SH: SlowHash<..>,
{
    group: G,
    key_exchange: KE,
    hash: H,
    slow_hash: SH,
}

and passed down only once as a configuration object.

huitseeker commented 3 years ago

Yep, the trait-based approach can't be a dynamic (runtime) configuration approach.

But in reaching for dynamic configuration, I want to figure out if this means to solve:

If we're trying to solve the first problem, there are possible ways to do it with:

If we're trying to solve the second problem, there is no way around it. Things that are legitimately the matter of runtime should move out of CipherSuite and be a parametric struct, as you suggest. That they're here in the first place probably means this component has outgrown its original scope as a convenience trait.

daxpedda commented 3 years ago

For our needs, we definitely don't need real dynamic configuration (yet :laughing:), so macros was the approach I was taking already at the moment. But to be clear, I don't see any further requirements for opaque_ke, I am only proposing pure API improvements (subjective).

So currently I'm trying to solve the shortcomings by exposing this API to our library consumers:

struct Config {
  group: RistrettoPoint,
  key_exchange: TripleDH,
  hash: Sha512,
  slow_hash: Argon2,
}

On top of that I don't want to let our consumers be constrained by generics, this is useful in the case of transitioning between different configurations for example without having to add additional storage layers because of different data structures, so I wrap objects in an enum:

enum ClientRegistration {
  RistrettoTripleDhSha512Argon2id(opaque_ke::ClientRegistration<RistrettoTripleDhSha512Argon2id>),
  RistrettoTripleDhSha512Argon2d(opaque_ke::ClientRegistration<RistrettoTripleDhSha512Argon2d>),
  RistrettoTripleDhSha512Scrypt(opaque_ke::ClientRegistration<RistrettoTripleDhSha512Scrypt>),
  RistrettoTripleDhSha512Pbkdf2(opaque_ke::ClientRegistration<RistrettoTripleDhSha512Pbkdf2>),
  RistrettoTripleDhBlake3Argon2id(opaque_ke::ClientRegistration<RistrettoTripleDhBlake3Argon2id>),
  ...
}

I don't see how to get rid of generics unless some run-time configuration is introduced (building configuration structs could still use const fn).

As for things that require true run-time configuration, personally I only looked at passing a salt for Argon2, I believe this would be impossible to do without adding some parameter. On the other hand I am no cryptographer, until now I was actually assuming that adding a salt in this case would have no real benefit.

daxpedda commented 3 years ago

For reference, this is what we did now: https://github.com/khonsulabs/custodian/blob/main/password/src/cipher_suite/mod.rs