vectorized encrypt API#385
Conversation
…small, vectorized makes more sense for speed.
huitema
left a comment
huitema
left a comment
There was a problem hiding this comment.
Sorry, I first saw these API changes as part of the fastls PR review.
If we change the API, we need to update the picotls_bcrypt library, which uses the previous API. The current init/update/final set of function can be used to implement do_encrypt_v, except for the parameter ptls_iovec_t aad -- the structure BCRYPT_AUTHENTICATED_CIPHER_MODE_INFO used for AEAD defines the "aad" parameter as
PUCHAR pbAuthData;
ULONG cbAuthData;
If the aad is in fact presented as a vector, it will have to be serialized, which is not great but is also not too complicated.
How do you want to do that? I guess I could prepare a PR against your PR...
|
@huitema Thank you for your quick response.
I do not have a strong opinion here, let's go back to using two arguments.
That would be fantastic. Thank you in advance. Do you have an opinion regarding if we should keep existing init -> update -> final API? |
|
I checked the picoquic sources. The code only uses On the other hand, if an implementation already provides the |
This SGTM. Please let me know if you want me to work on that before you'd be working on the bcrypt side (thank you as always). I'd also be totally fine if you could do everything and open a PR to this PR. |
|
Do you plan to reverse the change to the aad data representation from (value, length) to ptls_iovec_t? That's one of the incompatible changes in the API. Do I have to implement that change in the PR, or should I wait for it to be reversed? |
|
@huitema In ea42ef7, I've reverted all the changes modulo the introduction of the encrypt_v callback. There is now a helper called Line 983 in ea42ef7 Do these work for you on the bcrypt-side? |
|
Please see PR #386, which fixes the bcrypt implementation of encrypt_v. |
|
It would be a good idea to expose an API ( This PR is in good shape and the TLS (over TCP) side already utilizes the new API. Content-type and the record payload is supplied to the AEAD layer using the new encrypt_v callback. |
2 participants
At the moment, the encrypt API provided by picotls only receives one
ptl_iovec_t, which in turn produces an AEAD record. This leads to inefficiency when multiple fragments of data has to be sent concatenated.To give an example, HTTP/2 implementations typically split HTTP response into small chunks, prepending HTTP/2 frame headers to each of those chunks, before they pass them to the TLS stack. At the moment, HTTP/2 implementations have to either:
ptls_sendfor encrypting the HTTP/2 frame header, then make a separate call for encrypting the response chunk.a is inefficient when the implementation already has the response buffered in memory, because an extra copy would be required to concatenate the HTTP/2 frame header and the chunk of the response. b is inefficient due to generating a tiny TLS record that contains only the HTTP/2 frame header, not to mention the privacy aspect of leaking information through how things are chunked.
To address this problem, this pull request attempts to introduce a variant of
ptls_sendfunction that accepts a vector of buffers as input.At the moment, this PR also replaces the init -> update -> final API at the AEAD layer with a new vectorized API as well. The rationale is that vectorized APIs are expected to provide better performance (as they can be integrated tighter to the encrypting code, see #384). But we can probably keep the existing ones deprecated.