add encrypted_payload_len to MessageEncrypter

[japaric]

The LlConnection API proposed in RFC #1420 returns an error when the user attempts to encrypt app-data that won't fit in the provided buffer. To perform this check is necessary to compute the size of the resulting TLS record(s) without performing any encryption or encoding as that would be expensive in the retry cases where the end-user will call the encrypt API twice. To compute the size, MessageEncrypter implementations need to provide the post-encryption size.

This has been send as a separate PR from #1578 because it's a semver breaking change and it would be good to get it into the 0.22 release.

I'm submitting @pvdrz 's implementation as it but would it make sense to have MessageEncrypter report the encryption overhead (e.g. fn encryption_overhead(&self) -> usize), which appears to be constant in all in-tree implementations, instead?

[codecov]

Codecov Report

Merging #1579 (160011b) into main (1f0e6ad) will decrease coverage by 0.30%.
Report is 37 commits behind head on main.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main    #1579      +/-   ##
==========================================
- Coverage   96.41%   96.12%   -0.30%     
==========================================
  Files          75       76       +1     
  Lines       15523    15755     +232     
==========================================
+ Hits        14967    15144     +177     
- Misses        556      611      +55     

Files	Coverage Δ
rustls/src/crypto/ring/tls12.rs	97.82% <100.00%> (+0.07%)	⬆️
rustls/src/crypto/ring/tls13.rs	100.00% <100.00%> (ø)
rustls/src/crypto/cipher.rs	83.16% <0.00%> (-2.55%)	⬇️

... and 31 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[djc]

Makes sense to have something for this!

I agree respinning this as something more like fn encryption_overhead() -> usize is preferable if it can be made to work.

[djc]

Maybe spend a bit more attention on this docstring? (Proper capitalization, ideally a first line that is more concise, and probably don't use "cryptext"?)

[ctz]

Yes I'd prefer it if we stick to "ciphertext", as elsewhere in the codebase and common in the TLS RFCs. (Styled as shown: one word, not hyphenated.)

[ctz]

Hm. It's not necessarily the case that the ciphertext length is a pure function on the plaintext length. It is definitely not a constant.

• not a pure function: someone could decide to apply a random amount of padding (in TLS1.3)
• not a constant: padding with length related to plaintext length

It would be good if the value sampled each time from this function was passed back into the encryption function itself. I'd expect this happens eventually anyway, because presumably we'll change the APIs to take an mut output slice?

For the same reason, it is generally unknowable what the plaintext size for a given cipher text prior to decrypting it -- it definitely is not ciphertext.len() - encryption_overhead().

[japaric]

not a pure function: someone could decide to apply a random amount of padding (in TLS1.3)

but can that be upper bounded? the LlConnection::encrypt(plaintext, out_buffer) implementation needs to know a buffer size where the encrypted contents will fit; it doesn't have to be the exact size

It would be good if the value sampled each time from this function was passed back into the encryption function itself.

not sure I follow. do you mean that the user of MessageEncrypter should be using the API like this?

let plaintext;
let encrypted_len = encrypter.encrypted_payload_len(plaintext.len());
let cryptext = encrypter.encrypt(plaintext, encrypted_len);

I'd expect this happens eventually anyway, because presumably we'll change the APIs to take an mut output slice?

the way LlConnection::encrypt(plaintext, out_buffer) works is that it'll encrypt plaintext IFF it fits in out_buffer. it returns an error with the required buffer size if the buffer is too small; out_buffer is not modified in this error case.

if we change MessageEncrypter to operate on slices it'd expect to behave the same: either it encrypts the whole thing or does nothing so it should not need to be stateful to support incrementally encrypting a record.

[ctz]

but can that be upper bounded? the LlConnection::encrypt(plaintext, out_buffer) implementation needs to know a buffer size where the encrypted contents will fit; it doesn't have to be the exact size

Upper bound is 2**14 + 1 bytes.

not sure I follow. do you mean that the user of MessageEncrypter should be using the API like this?

I was thinking more like:

let plaintext = ...;
let encrypted_len = encrypter.encrypted_payload_len(plaintext.len());
// (validate that the following slice is non-panicing)
encrypter.encrypt_into(plaintext, &mut output_buffer[..encrypted_len]);

So, yes, the same as what you're describing I think?

[ctz]

one docs nit, but otherwise i'm happy with this

[japaric]

I was thinking more like:

ah, I now see what you meant. yeah, using it like that makes sense.

I have tweaked the doc comment and squashed the commits

[japaric]

@cpu I've addressed your comments

[cpu]

Thanks, LGTM

[cpu]

github-merge-queue bot removed this pull request from the merge queue due to failed status checks

I think crates.io is having a bad time. I'm seeing CI fail across multiple repos 😭