RustCrypto / traits

Collection of cryptography-related traits
580 stars 189 forks source link

password-hash: Add pepper to API #694

Closed TheAlgorythm closed 2 years ago

TheAlgorythm commented 3 years ago

As discussed in RustCrypto/password-hashes#194 it could be considered to add pepper with a MAC to the traits as it's currently particularly uncomfortable to have a password-hash with a pepper. The changes could look like this:

trait PasswordHasher {
...
    fn hash_password_with_pepper<'a>(
        &self,
        password: &[u8],
        algorithm: Option<Ident<'a>>,
        params: Self::Params,
        salt: impl Into<Salt<'a>>,
        pepper_method: impl Pepper,
    ) -> Result<PasswordHash<'a>>;
}
TheAlgorythm commented 3 years ago

I would like to implement it but I would like to get some feedback. The algorithm/id should be extended with a -<mac> but I don't know where to get the name of the MAC. PasswordVerifier & PasswordHash will need additional methods like PasswordHasher to validate the peppered hashes. So the main pain point is the missing identification of the MAC algorithm.

newpavlov commented 3 years ago

In crypto-common we have the AlgorithmName trait, which will be used in the next minor version releases. But it was introduced mostly for Debug implementations, so I am not sure if it will be suitable here.

tarcieri commented 3 years ago

I'm not aware of any standard for including information about the pepper in the algorithm identifier for a password hash (PHC, MCF, or otherwise), nor do I think it makes sense to invent one.

I would suggest using traits from crypto-mac i.e. Mac, and passing in the pepper algorithm that way. That would make it possible to store the key in something like an HSM, for example.

TheAlgorythm commented 3 years ago

I would suggest using traits from crypto-mac i.e. Mac, and passing in the pepper algorithm that way. That would make it possible to store the key in something like an HSM, for example.

Yeah, that is what I proposed with the trait definition. But I think it should be possible to use different MACs for crypto agility.

In my understanding Argon2::hash_password_with_pepper<Hmac<Sha256>> needs a different Ident than Argon2::hash_password_with_pepper<CMAC> or Argon2::hash_password_with_pepper<Hmac<Blake2b>> to verify the password. Therefore I see it as a part of the Ident and would separate the hashing and MAC algorithm by e.g. a -.

So it could look like: $argon2id-hmac-blake2b$v=...

An alternative would be to encode the MAC as a parameter like this: $argon2id$mac=hmac-blake2b$v=...

But in both cases the MAC algorithm needs to be uniquely identifiable.

tarcieri commented 3 years ago

I'd be wary of trying to add some sort of bespoke scheme to identify which algorithm is used for the pepper as part of the identifier, particularly in the context of PHC hashes.

That sort of thing is exactly how MCF hashes became such a mess.

If we're going to call algorithms out in the identifier, I'd prefer to get some input on what that looks like, and ideally have some sort of spec as published through the PHC panel, or potentially even added to the existing PHC spec.

TheAlgorythm commented 3 years ago

Yes my Ident-solution is not very modular, doesn't go with the spec very well and has the potential issue of parser differentials. But the parameter solution should be pretty spec-compliant, or not?

tarcieri commented 3 years ago

I think Pepper should be a trait, not a type, or just use an existing trait like Mac (or possibly both, with a blanket impl of Pepper for Mac).

This is because one of the most important ways pepper can be applied is through some sort of external service or hardware device, so it's important not to require the key as some sort of bytestring.

TheAlgorythm commented 3 years ago

I think I know where you are going. I changed the code above to your proposal.

The Pepper trait could look like this:

trait Pepper: AlgorithmName {
    fn pepper(&self, ...) -> ...;
    fn verify(&self, ...) -> Result<...>;
}

I am not sure whether the trait should be a supertrait of AlgorithmName or if there should be a fn ident(or however you want to call it) with a blanket implementation for Mac + AlgorithmName. I tend towards the second solution.

Owez commented 2 years ago

As an alternative, couldn't it just be included inside of Salt/SaltString? Have methods like:

pub fn generate_pepper(rng: impl CryptoRng + RngCore, pepper: &[u8]) -> Self;
pub fn new_pepper(s: &str, pepper: &[u8]) -> Result<Self>;

Not sure how password-hash works on the inside, so the &[u8] for pepper might be wrong

tarcieri commented 2 years ago

"Pepper" is completely distinct from the salt. Though somewhat ill-defined, the concept of "pepper" is more one of keying the password hash construction somewhat akin to a PRF.

Unlike a salt, the "pepper" is typically shared across all passwords in a database (possibly with a key rotation mechanism).

The Argon2::new_with_secret method provides an example of the sorts of use cases that "pepper" needs to cover.

Ideally an abstract API for this forces "pepper" into the constructor of some sort of stateful password hasher, so the pepper could potentially be kept in an external hardware device or network service.

tarcieri commented 2 years ago

I think the best way to actually address this issue is KDF traits (#5). The crates which support a "pepper" are all password-based KDFs.

KDFs already take a number of inputs, including input key material, generally some sort of salt value, and an additional "info" field which personalizes the output based on some sort of additional qualifier.

I think these can be mapped to a secret key, salt, and password respectively (although it'd be good to check if that's a proper/common mapping).

Given that, I'm going to close this issue, and suggest that keyed password-based KDF discussion continues on #5.