rustcrypto / kdfs Goto Github PK

It looks like we got an 0.7.1 release on crates.io (yay!), but there's no tag on the git repo. The version was updated in 87659b1, but was the release made from that, or from a subsequent commit?

We should compare the crates.io tarball against the last four commits, and add a hkdf-0.7.1 tag to the right one.

Would it make sense to have a Concatenation and ASN.1 KDFs in this repository?
These key derivation functions are described in NIST SP 800-56A 5.8.1 and 5.8.2.

If these functions suit this repository, I would like to offer my help (implementation).

The current API leans heavily on Vec. It'd be nice if there were (additionally) a no_std friendly API which avoided allocation which contained the core implementation, and then Vec wrappers gated on e.g. a std trait to expose a "friendlier" API.

http://is.gd/qnpBnq

It would be nice if your API made it easier to perform the expand without the initial extract phase.
This is what I do now:

    let hk = Hkdf::<Sha256>{
        prk: *GenericArray::from_slice(prk),
    };

Possibly there should be a constructor that does this from_slice method call on the u8 slice.

@warner do we still want to do this? Let me know!

P.S. version 0.2.0 is live.

@vladikoff I started to do this myself, but it looks like I'm not an owner on crates.io . Could you cargo release minor, or add me as an author?

I might be missing something about the rational behind the HmacImpl sealed trait (like some implicit invariant), but I'm wondering why Hkdf and HkdfExtract are not parametrized by hmac::Mac instead. This trait already provides OutputSizeUser.

This would be useful when someone has both a Digest and a Mac hardware implementation and would like to compute HKDF using that Mac hardware implementation instead of using the SimpleHmac software implementation on top of the Digest hardware implementation.

Follow HKDF RFC: https://tools.ietf.org/html/rfc5869#section-2.2

HashLen denotes the length of the hash function output in octets

optional salt value (a non-secret random value); if not provided, it is set to a string of HashLen zeros.

The HashLen is OutputLen, not BlockLen.

But for now, the hkdf code set to a string of BlockLen zeros when salt value not provided.

See code:

HMAC Code Line:
https://github.com/RustCrypto/MACs/blob/4ab9f441fb08c754c22d65963fa948693c6e5116/hmac/src/lib.rs#L118-L128

impl<D> NewMac for Hmac<D>
where
    D: Update + BlockInput + FixedOutput + Reset + Default + Clone,
    D::BlockSize: ArrayLength<u8>,
    D::OutputSize: ArrayLength<u8>,
{
    type KeySize = D::BlockSize;

    fn new(key: &GenericArray<u8, Self::KeySize>) -> Self {
        Self::new_varkey(key.as_slice()).unwrap()
    }

Fix:

None => Hmac::<D>::new(&Default::default()), should be change to None => Hmac::<D>::new_varkey(&D::OutputSize::default()), .

This is a tracking issue for KDF algorithms we should potentially implement.

Please leave a comment with your requests!

Can't compile with sha2 = "0.8" since it uses digest 0.8.

We've seen one (brief) bug in https://github.com/warner/magic-wormhole.rs in which our protocol requires calling HKDF without a salt, and the initial code used Hkdf::<Sha256>::extract(&[], key) to express this. But the HKDF definition (RFC 5869) says that when the salt is omitted, "it is set to a string of HashLen zeros". Passing an empty salt string is not the same as omitting the salt (i.e. passing 32 zero bytes), causing a protocol mismatch.

To properly omit the salt, you must use something like extract(&[0; Sha256::OutputSize], key) or something that I'm not even sure would compile. Manually setting the salt size feels error-prone.

The python HKDF library lets you pass None as the salt value, and it will do the right thing.

Should we consider changing the extract() API to take an Option<&[u8]> for the salt value?

Can Rust do some kind of polymorphic thing that might let us have two different extract() methods, with different type signatures (one with &[u8], the other with Option) to enable backwards compability?

Is there anything missing for releasing 0.9.0?

As stated by RFC 5869, the info parameter should be optional. For now, we can technically pass an empty array, but I do believe that using an Option<&[u8]> is more rustic.

I am willing to do a PR if needed.

Here’s a minimal test case:

#[test]
fn test_debug() {
    fn check_debug<T: core::fmt::Debug>() {}

    check_debug::<HkdfExtract<Sha256>>();
    check_debug::<Hkdf<Sha256>>();
}

It fails with this error:

error[E0277]: the trait bound `CoreWrapper<CtVariableCoreWrapper<Sha256VarCore, UInt<UInt<UInt<UInt<UInt<UInt<UTerm, B1>, B0>, B0>, B0>, B0>, B0>, OidSha256>>: AlgorithmName` is not satisfied
   --> hkdf/tests/tests.rs:388:19
    |
388 |     check_debug::<HkdfExtract<Sha256>>();
    |                   ^^^^^^^^^^^^^^^^^^^ the trait `AlgorithmName` is not implemented for `CoreWrapper<CtVariableCoreWrapper<Sha256VarCore, UInt<UInt<UInt<UInt<UInt<UInt<UTerm, B1>, B0>, B0>, B0>, B0>, B0>, OidSha256>>`
    |
    = help: the following other types implement trait `AlgorithmName`:
              HmacCore<D>
              Sha1Core
              Sha256VarCore
              Sha512VarCore
              CtVariableCoreWrapper<T, OutSize, O>
    = note: required for `HkdfExtract<CoreWrapper<CtVariableCoreWrapper<Sha256VarCore, UInt<UInt<UInt<UInt<UInt<UInt<UTerm, B1>, B0>, B0>, B0>, B0>, B0>, OidSha256>>>` to implement `Debug`
note: required by a bound in `check_debug`
   --> hkdf/tests/tests.rs:386:23
    |
386 |     fn check_debug<T: core::fmt::Debug>() {}
    |                       ^^^^^^^^^^^^^^^^ required by this bound in `check_debug`

error[E0277]: the trait bound `CoreWrapper<CtVariableCoreWrapper<Sha256VarCore, UInt<UInt<UInt<UInt<UInt<UInt<UTerm, B1>, B0>, B0>, B0>, B0>, B0>, OidSha256>>: AlgorithmName` is not satisfied
   --> hkdf/tests/tests.rs:389:19
    |
389 |     check_debug::<Hkdf<Sha256>>();
    |                   ^^^^^^^^^^^^ the trait `AlgorithmName` is not implemented for `CoreWrapper<CtVariableCoreWrapper<Sha256VarCore, UInt<UInt<UInt<UInt<UInt<UInt<UTerm, B1>, B0>, B0>, B0>, B0>, B0>, OidSha256>>`
    |
    = help: the following other types implement trait `AlgorithmName`:
              HmacCore<D>
              Sha1Core
              Sha256VarCore
              Sha512VarCore
              CtVariableCoreWrapper<T, OutSize, O>
    = note: required for `Hkdf<CoreWrapper<CtVariableCoreWrapper<Sha256VarCore, UInt<UInt<UInt<UInt<UInt<UInt<UTerm, B1>, B0>, B0>, B0>, B0>, B0>, OidSha256>>>` to implement `Debug`
note: required by a bound in `check_debug`
   --> hkdf/tests/tests.rs:386:23
    |
386 |     fn check_debug<T: core::fmt::Debug>() {}
    |                       ^^^^^^^^^^^^^^^^ required by this bound in `check_debug`

For more information about this error, try `rustc --explain E0277`.

Any ideas?

(Edited with a more concise test case and error message.)

This setup is used in most of other repositories in this organization.

Initially reported in Zulip.

Right now if function uses HKDF and wants to be generic over hash functions, trait bounds look quite scary:

fn hkdf<H: CoreProxy>()
where
    <H as CoreProxy>::Core: HashMarker
        + UpdateCore
        + FixedOutputCore
        + BufferKindUser<BufferKind = Eager>
        + Default
        + Clone,
    <<H as CoreProxy>::Core as BlockSizeUser>::BlockSize: IsLess<U256>,
    Le<<<H as CoreProxy>::Core as BlockSizeUser>::BlockSize, U256>: NonZero,
{
    Hkdf::<H>::new(None, &[]);
}

It could be worth to simplify the bounds somehow. Also it may be worth to allow hkdf to use the SimpleHmac in addition to Hmac. It probably will involve introducing a helper trait or two in hmac.

The 0.1.0 release (the latest on crates.io) won't compile with the current version of Rust, because of a syntax change that happened back in the rustc-1.5.0 era. The fix for that is on current master, so the only thing that needs to be done is to make a 0.1.1 release (modify Cargo.toml, tag, publish).

I'm about to file a PR that changes the API (and the dependencies) to use RustCrypto, which is more modern and apparently more popular than rust-crypto. That new API will need a new major release (0.2.x), so I figure maybe we should publish at least one functional crate using the old API first.

I'm happy to drive that release if you want.

thanks!

For coherence with other crates and other reasons.

Contributor checkoff

• @vladikoff (via IRC)
• @warner
• @Ralith