Don't construct `EncodedMessage` in `MessageEncrypter`, `MessageDecrypter` impls

[tgeoghegan]

Checklist

• I've searched the issue tracker for similar requests

Is your feature request related to a problem? Please describe.
Each implementation of trait MessageEncrypter in a crypto provider implements the function:

    /// Encrypt the given TLS message `msg`, using the sequence number
    /// `seq` which can be used to derive a unique [`Nonce`].
    fn encrypt(
        &mut self,
        msg: EncodedMessage<OutboundPlain<'_>>,
        seq: u64,
    ) -> Result<EncodedMessage<OutboundOpaque>, Error>;

As a result, each crypto provider must make a decision about what content type and protocol version to put in the returned EncodedMessage<OutboundOpaque>. Some of them copy the content type and protocol version from the input msg, but some of them force those values to ContentType::ApplicationData and ProtocolVersion::TLSv1_2, respectively.

The correct content type or protocol version to emit has nothing to do with which crypto implementation is in use, so this interface seems to unnecessarily introduce an opportunity for providers to make mistakes.

Symmetrically, we have trait MessageDecrypter:

    fn decrypt<'a>(
        &mut self,
        msg: EncodedMessage<InboundOpaque<'a>>,
        seq: u64,
    ) -> Result<EncodedMessage<&'a [u8]>, Error>;

TLS 1.2 implementations ignore everything in msg except the payload, while TLS 1.3 implementations have to remember to use EncodedMessage::into_tls13_unpadded_message.

Describe the solution you'd like
If the interface were changed to take bytes of plaintext instead of EncodedMessage<OutboundPlain> and return bytes of ciphertext instead of EncodedMessage<OutboundOpaque>, then the calling context (crypto::cipher::record_layer::EncryptionState::encrypt_outgoing) could take responsibility for correctly constructing records no matter what provider is in use. So perhaps change the trait method to:

fn encrypt(&mut self, plaintext: &[u8], seq: u64) -> Result<Vec<u8>, Error>;

Or if we consider the proposition in #3068 and the broad push toward client owned buffers, then we could do decryption in-place and avoid allocating:

/// `payload_len` is <= `payload.len()` and is where the plaintext ends. Caller is responsible for ensuring that `payload` is big enough for the ciphertext (e.g. enough room for AEAD tag and padding).
fn encrypt(&mut self, payload: &mut [u8], payload_len: usize seq: u64) -> Result<(), Error>;

Symmetrically, MessageDecrypter could do:

fn decrypt(&mut self, ciphertext: &[u8], seq: u64) -> Result<Vec<u8>, Error>;
//or in-place:
fn decrypt(&mut self, ciphertext: &mut [u8], ciphertext_len: usize, seq: u64) -> Result<(), Error>;

Additional context
Another issue discussing changes to this interface: #3068

[djc]

I think it'd make sense to get #3068 done first. @cpu were you planning on picking that up?

[cpu]

Yeah, I've started to look at it but haven't made much progress yet beyond catching up on the background and experimenting a little bit. I'm hoping to push towards a draft PR soon but it will probably be towards the end of the week after I've finished some in-fight work on other projects. If tgeoghegan wants to run with it before then I don't have any objections.

[tgeoghegan]

Cool, I'm glad to hear that the maintainers think this is a good idea! I'm happy to contribute here. I agree we'd want to do this along with or after #3068, but past that, I think #2940 has implications here, since the buffer we'll be doing in-place encrypt/decrypt in will now be user owned. Then again, if the interface is MessageEncrypter::encrypt(&mut [u8]), then the implementation doesn't need to know if that slice is ultimately owned by the rustls client or by Side::State in ConnectionCommon.

[djc]

Note that I didn't say I thought it was a good idea -- I'm open to it though I'd like to see what API we land on for #3068 first.

[tgeoghegan]

I gave this a try today and found that the change I proposed is mostly doable, except that since the TLS 1.2 AAD needs to know the message type and protocol version, you can't just pass in a &mut [u8]. But in any case, the meatier change here is in-place encrypt/decrypt (#3068), which has implications far beyond this interface, because you'd need the buffer(s) wrapped by EncodedMessage<OutboundPlain> to be (1) mutable and (2) big enough for ciphertext. Anyway, I think I will wait to see what materializes in #3068 before I try this refactor again.