Define a Encapsulate::encapsulate_in_place method

[rozbb]

I have an issue in my Saber impl rozbb/saber-rs#6 that relates to this. encapsulate performs a stack allocation for its encapsulated key. This is not an issue for DH-based KEMs, but it's certainly more of an issue with, e.g., Kyber1024 or Firesaber, whose ciphertexts are ~1.5kB.

So I've added an encapsulate_in_place method to Encapsulate that lets the user provide their own ciphertext buffer. One nontrivial choice I made is documenting that "If this errors, the final value of encapsulated_key MUST equal its original value". I think this is probably fine: if the ciphertext is small then you can just do an allocation anyway, and if the ciphertext is large then it's probably a PQC scheme, and all of those are infallible, so you'll always overwrite anyway. In the absolute worst case, you can make a self-zeroizing copy of the buffer before you start working on it.

Happy to get feedback.

[rozbb]

Also if this gets merged I can remove the tests/saber.rs because we'll now have a proper Saber impl that uses this trait

[rozbb]

One issue: it’s not clear to me if there is any EK type outside of [u8; N] that can work with this API.

encapsulate returns an EK, so EK has to be owned type. So the only way to use encapsulate_in_place is to first allocate an EK, which is exactly what we wanted to avoid. Of course when EK = [u8; N] we get to avoid allocating, because we can construct &mut EK from mut slices.

So it seems this API is a little constraining if we want to support no-alloc.

Solution 1: Keep this as it is, with the knowledge that it only makes sense for EK = [u8; N] for some N

Solution 2: Remove EK and make all encapsulated keys just a fixed size byte array (maybe with associated consts or something). Since encapped keys are destined to be serialized, this is not a stretch IMO. It does however mean that the KEM method return values will have to include ser/deser errors.

Solution 3: Make a separate generic parameter EKRefMut that defines the type inputted to encapsulate_in_place. This is the most generic option, and lets us avoid deserializing in decap . But it does complicate the API. Also note that this also implies we need an EKRef to describe the input of decapsulate.

I kinda like solution 2. I always forget what’s possible in current Rust. I’ll see if I hit a barrier here.

[tarcieri]

You could always define an EncapsulateInPlace trait, and if it makes sense, define a blanket impl of EncapsulateInPlace for types which impl Encapsulate, that uses the default impl

[rozbb]

Hmm, I think the blanket impl doesn't quite work for two reasons. Firstly, it can't be overridden, so anything that's Encapsulate must necessarily use the inefficient blanket impl. Second, it might be meaningless, because the EK from Encapsulate might have no constructor (as is actually the case in 2/3 of our examples in tests/), so encapsulate_in_place(&mut EK) becomes a useless function.

But yeah, I'm not opposed to EncapsulateInPlace as a separate trait with no blanket/default impl. I do like the bytes-based API better, though. I wonder if others have stronger opinions. Should I ping @.bifurcation?

[rozbb]

Do you happen to have an opinion @bifurcation?

[tarcieri]

I'll note I'm a big fan of NOT having blanket impls unless they completely make sense 100% of the time

[rozbb]

Update on solution 2: It appears that this requires generic_array, for better or worse. This is because we need something like

pub trait Encapsulate<SS> {
    type Error: Debug;

    const ENCAPSULATED_KEY_LENGTH: usize;

    fn encapsulate(
        &self,
        rng: &mut impl CryptoRngCore,
    ) -> Result<([u8; Self::ENCAPSULATED_KEY_LENGTH], SS), Self::Error>;
}

which is not possible because it uses an associated const in a type signature (classic tracking issue). So we'd either have to use generic_array (not fundamentally opposed to this) or use a different solution (solution 1 is fine by me too).

[tarcieri]

Would recommend hybrid-array, assuming what you're really after is typenum-based associated constants-as-types

[bifurcation]

Couple of quick reactions:

• I don't understand how you arrive at the idea that EK must be a byte slice. It just needs to be something where the calling code can get one, e.g., by implementing Default. Imagine, for example, a compound struct of a few buffers.

• Why do no-alloc for EK but not SS? And if we're going to do SS, then we should do decapsulate_in_place as well.

• I'm not enough of a Rust wizard to confidently opine on trait structuring, though I think I agree with @tarcieri's inclination toward a separate trait. Cf. AeadInPlace

Overall, no strong opinions here, other than a slight impression of premature optimization.

[rozbb]

Thank you for looking at this! In order:

1. The byteslice choice allows us to make everything one trait while not requiring two separate parameters (EK for encapsulate and EKRefMut for encapsulate_in_place). The alternative is having a separate trait without a blanket impl, which, ya may be fine.
2. A shared secret is usually just a length-32 bytestring, no matter what you're using. Not worth thinking about imo.
3. Ok I think that settles it, I'll write a separate EncapsulateInPlace trait 🎉

[rozbb]

Ready for review

[rozbb]

Actually I think maybe this needs some more thinking. I feel like I'm missing something very simple here. Especially wrt the request for more simplicity in #1559. I think I'll close for now. We can discuss more in the Zulip, I think.