Added support for P-521

[OtaK]

Hi again!

This PR brings support for DHKEM(P-521, HKDF-SHA512) with the help of the newly released p521 0.13 implementation

[rozbb]

This looks great, ty! Will review more carefully shortly, but seems like no surprises here

[OtaK]

Neat! The only thing I'm missing is updating examples/agility.rs to support p521; I'll do that right ASAP (should be today)

Update: done

[OtaK]

Something to note: I tried to run the MLS test vectors over p521 HPKE, and I'm getting length off-by-one panics. I don't know if it's the test vectors or something else (my wild guess would be something wrong with the hpke::De|Serialize impl that returns the wrong vector) so I'll look into it.

The application panicked (crashed).
Message:  assertion `left == right` failed
  left: 65
 right: 66
Location: /home/otak/.cargo/registry/src/index.crates.io-6f17d22bba15001f/generic-array-0.14.7/src/lib.rs:572

[rozbb]

Hmm odd. Especially bc there's nothing in P521 that's supposed to be 65 bytes

[tarcieri]

P-521 field elements are 66-bytes. If they're smaller than that you need to pad with a leading zero

[rozbb]

Ohh, that makes sense. The to_bytes() method actually calls down to this trait, which explicitly warns about padding zeros (also here)

[OtaK]

So I manually checked the MLS test vectors and they contain a mixture of 65 and 66-byte secret keys;
I think they tried to emulate the "wild" where not every implementation is well-behaving.

P-521 field elements are 66-bytes. If they're smaller than that you need to pad with a leading zero

Oh thanks, didn't know that :)

Now the discussion is on whether hpke needs to account for it or if downstream library users need to do it. I'd go for the second one but I expect GH issues to pop up :/

If you want me to implement normalization I don't see an issue in that.

[rozbb]

The spec seems pretty clear that Nsk is only appropriate length for a serialized secret to be. So I think the solution is to make sure secrets here are always 66 bytes, and we simply do a zero-check-then-truncate on the MLS test vectors. Does that make sense?

[OtaK]

I did the necessary on my MLS side (I fully expect some implementations to misbehave as one of the implementations did produce the test vectors...with 65-byte secret keys), so nevermind the issue. I indeed had to prepend null bytes to reach 66-bytes.

I honestly don't think rust-hpke should account for misbehaving implementations. It respects the spec as far as it's concerned so doing more is a bit meh (especially the cost of having to copy/clone the secret keys somewhere to be able to prepend null bytes)

Also I found this: mlswg/mls-implementations#176

[tarcieri]

@OtaK the NIST/FIPS test vectors for ECDSA are also like that, as it were, with leading zeros removed.

I transcoded them into Rust syntax using hex_literal::hex! and zero padded them in the hex literal strings, so they always decode to [u8; 66]: https://github.com/RustCrypto/elliptic-curves/blob/master/p521/src/test_vectors/ecdsa.rs

[OtaK]

So after looking around, apparently, P521 private keys can be between 0 to 66 bytes because of some implementations using a minimum-byte representation (i.e. go's crypto/ecdsa) in the following distribution:

• 66 bytes 49% of the time
• 65 bytes 50% of the time
• 64 bytes 0.2% of the time
• 63 bytes or below with increasingly decreasing percentages

But only HPKE (and not FIPS 186-4) mandates that the private keys must be 66 bytes long.

I honestly don't know if p521 should transparently pre-pad keys in the from_slice implementation; that would seem to be the safest way to ensure users don't get surprises when they have keys that come from somewhere else - which they shouldn't, but it can happen, i.e. migration from a go implementation to a Rust one - because p521 actually requires keys to be 66-byte. I honestly have no clue

[tarcieri]

elliptic_curve::SecretKey::from_slice currently tolerates the key being up to 4-bytes shorter than the expected size.

Though there's an open issue about whether that 4-bytes is a good idea or if it should tolerate more (or not allow other sizes at all): RustCrypto/traits#1330

[OtaK]

elliptic_curve::SecretKey::from_slice currently tolerates the key being up to 4-bytes shorter than the expected size.

Though there's an open issue about whether that 4-bytes is a good idea or if it should tolerate more (or not allow other sizes at all): RustCrypto/traits#1330

Hmm interesting. That would mean that if instead of from_bytes we'd use from_slice in rust-hpke, we could get this behavior for free and also tolerate the variable key length?

What do you think @rozbb? Maybe it's worth a shot?

[tarcieri]

we could get this behavior for free and also tolerate the variable key length?

Correct. I also opened a PR to change the minimum key size to 192-bits, which should be better/more consistent than its current heuristic: RustCrypto/traits#1412

[rozbb]

I agree it'd be somewhat more ergonomic to allow multiple lengths of sk, but I have some hesitation.

1. This requires changing the Deserializable trait, since from_bytes very strictly takes just things that are length Self::OutputSize::USIZE. And the padding cannot apply to all schemes because Xyber, for example, has a more complex secret key.
2. I'm not sure how often people will be loading up secret keys from other systems into HPKE. Ideally, they shouldn't at all.

Basically, I'm willing to wait until this becomes a thing people need in the wild before making it a feature. And if it is a feature, I'd specialize it to P-521 and call it like from_unpadded_bytes or something. Thoughts?

Btw this all looks great and I'm ready to merge

[OtaK]

I agree it'd be somewhat more ergonomic to allow multiple lengths of sk, but I have some hesitation.

1. This requires changing the Deserializable trait, since from_bytes very strictly takes just things that are length Self::OutputSize::USIZE. And the padding cannot apply to all schemes because Xyber, for example, has a more complex secret key.
2. I'm not sure how often people will be loading up secret keys from other systems into HPKE. Ideally, they shouldn't at all.

Basically, I'm willing to wait until this becomes a thing people need in the wild before making it a feature. And if it is a feature, I'd specialize it to P-521 and call it like from_unpadded_bytes or something. Thoughts?

Btw this all looks great and I'm ready to merge

I meant more like indeed localizing this behavior to p521 KEM only and turning

let sk = curve_crate::SecretKey::from_bytes(encoded.into())
                        .map_err(|_| HpkeError::ValidationError)?;

to this

let sk = curve_crate::SecretKey::from_slice(encoded)
                        .map_err(|_| HpkeError::ValidationError)?;

And basically with the issue I encountered, it showed that it's something probably needed in the wild considering the specifics of p521; This led to me having such code pretty much everywhere I encounter raw p521 keys:

fn normalize_secret_key(sk: &[u8]) -> zeroize::Zeroizing<[u8; 66]> {
    let mut sk_buf = zeroize::Zeroizing::new([0u8; 66]);
    sk_buf[66 - sk.len()..].copy_from_slice(sk);
    sk_buf
}

[rozbb]

Ah I see. I had thought that Deserializable said explicitly that it will only accept byte sequences that are exactly Self::OutputSize, but that's not true. Ok, this sounds good. Two questions:

1. Does from_slice change the behavior of p256 or p384 serialization? Will it accept smaller scalars now? Or will this only affect p521 deserialization?
2. (open Q) Where is the appropriate place to document this behavior? The SecretKey types are intentionally hidden right now. One fix that might actually be nice is to expose the per-KEX key types because currently there's a TON of <MyKem as KemTrait>::PublicKey in all HPKE code. Theoretically you could just do type MyPubkey = <MyKem as KemTrait>::PublicKey, but nobody seems to do that. Downside is if you decide to change MyKem, you also have to change your pubkey type. Maybe no big deal though

[rozbb]

Good catch. Dunno how this got here

[rozbb]

I'm gonna merge but we can continue the conversation here. Again, thanks for the PR, this was very easy to read and review!

[OtaK]

1. Does from_slice change the behavior of p256 or p384 serialization? Will it accept smaller scalars now? Or will this only affect p521 deserialization?

No idea. Wouldn't make sense though as keys of p256 or p384 are full 32 or 48 bytes, whereas p521 is 65 bytes and 1 bit. One could argue that you could omit the first byte if it's equal 0x00 like it is 50% of the time and save up on storage space.

2. (open Q) Where is the appropriate place to document this behavior? The SecretKey types are intentionally hidden right now. One fix that might actually be nice is to expose the per-KEX key types because currently there's a TON of <MyKem as KemTrait>::PublicKey in all HPKE code.

They're hidden from docs but they're still exposed because you can't control the visibility of associated types. I make heavy use of the Kem's PrivateKey types. See here for an example: https://github.com/wireapp/core-crypto/blob/60a349d28f6e4b21f875cf0ac6621f7d5e5ea3dd/mls-provider/src/crypto_provider.rs#L534

Theoretically you could just do type MyPubkey = <MyKem as KemTrait>::PublicKey, but nobody seems to do that. Downside is if you decide to change MyKem, you also have to change your pubkey type. Maybe no big deal though

I do though : D I have some generic code that looks like this, in the context of a macro, as an example of the most abusive cases on rust-hpke

fn hash_len(&self) -> usize {
    <<$kdf as hpke::kdf::Kdf>::HashImpl as digest::Digest>::output_size()
}

fn key_len(&self) -> usize {
    <<$kem as hpke::Kem>::PrivateKey as hpke::Serializable>::size()
}

fn aead_tag_len(&self) -> usize {
    use typenum::Unsigned as _;
    <<$aead as hpke::aead::Aead>::AeadImpl as aead::AeadCore>::TagSize::USIZE
}
fn aead_key_len(&self) -> usize {
    use typenum::Unsigned as _;
    <<$aead as hpke::aead::Aead>::AeadImpl as aead::KeySizeUser>::KeySize::USIZE
}
fn aead_nonce_len(&self) -> usize {
    use typenum::Unsigned as _;
    <<$aead as hpke::aead::Aead>::AeadImpl as aead::AeadCore>::NonceSize::USIZE
}

It's the drawback of trait-centric approaches: you currently have no way to restrict library users from "reverse-engineering" (with heavy quotes) the implementation internals of those traits, unless you use a delegate pattern.