feat: add AEAD seal_in_place_scatter method #206
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
justsmth
reviewed
Aug 1, 2023
aws-lc-rs/src/aead/aes_gcm.rs
Outdated
| if 1 != EVP_AEAD_CTX_seal_scatter( | ||
| aead_ctx, | ||
| in_out.as_mut_ptr(), | ||
| extra_out_and_tag.as_mut_ptr().cast(), |
There was a problem hiding this comment.
NP: The cast() here is not needed.
justsmth
previously approved these changes
Aug 4, 2023
skmcgrail
reviewed
Aug 14, 2023
skmcgrail
reviewed
Aug 23, 2023
aws-lc-rs/src/aead.rs
Outdated
| /// | ||
| #[inline] | ||
| #[allow(clippy::needless_pass_by_value)] | ||
| pub fn seal_in_place_scatter<A>( |
There was a problem hiding this comment.
Had an offline conversation with Justin, so will summarize it here so that we can get this closed out. Let's remove SealingKey::seal_in_place_scatter, and only have support for this feature on LessSafeKey. With that small modification we are open to accepting this feature.
justsmth
previously approved these changes
Aug 24, 2023
justsmth
approved these changes
Aug 24, 2023
skmcgrail
approved these changes
Aug 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
The current AEAD encrypt API takes a
in_out: &mut [u8], which forces callers to copy all of the cleartext into a single slice. This shouldn't be needed since encrypting a payload is copying the data since it has to XOR it with the cipher stream.Luckily, AWS-LC already provides a EVP API to account for this: EVP_AEAD_CTX_seal_scatter. This allows the caller to pass a
extra_in, which is encrypted into thetag_outbuffer.In transport protocols like TLS and QUIC, where the bulk of the data being encrypted is separate from the framing header, using this API can yield large wins. In the case of s2n-quic, we see a reduction in 8% of CPU by avoiding the copy.
Call-outs:
API ergonomics
This new API isn't the most straightforward interface to use. You have to make sure to split the slices up in the correct order and account for the tag size. If we want to limit its use, we can add a feature flag stating that it's somewhat advanced.
API naming
I'm not sure if this is the best name, but it's what I came up with. Let me know if there's something better.
Testing:
I've updated the AEAD tests to check the new API matches the existing ones. I'm using part of the key bytes to pick an arbitrary split point in the input for the
extra_inparameter. This will make the tests deterministic for a given key but still be somewhat "random" in how thein_outis split from theextra_in.s2n-quic without this change
Throughput is ~20Gbps. You can see the
memcpytaking about 8% of CPU cycles in the flamegraph:s2n-quic with this change
Throughput is ~21Gbps. You can see in the flamegraph that the
memcpyis gone:By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.