aws_lc_rs: implement RFC 5077 recommended ticketer #2066
Merged
Conversation
This file contains hidden or 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
457f0c7 to
bb0dc4d
Compare
cpu
commented
Aug 1, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2066 +/- ##
==========================================
+ Coverage 94.47% 94.52% +0.05%
==========================================
Files 100 102 +2
Lines 23245 23465 +220
==========================================
+ Hits 21960 22180 +220
Misses 1285 1285 ☔ View full report in Codecov by Sentry. |
ctz
reviewed
Aug 2, 2024
djc
reviewed
Aug 3, 2024
bb0dc4d to
02c84ab
Compare
ctz
reviewed
Aug 6, 2024
02c84ab to
19b1c2a
Compare
ctz
approved these changes
Aug 6, 2024
Benchmark resultsInstruction countsSignificant differencesClick to expand
Other differencesClick to expand
Wall-timeSignificant differencesClick to expand
Other differencesClick to expand
Additional informationCheckout details:
|
19b1c2a to
b0413af
Compare
There's an instruction count increase for the |
|
Yes, I think the walltime ones are generally more representative. But I'm not surprised that this is a little slower -- quite a lot of area is dedicated now to making AES-GCM go fast. |
djc
approved these changes
Aug 7, 2024
There was a problem hiding this comment.
LGTM!
There are a number of little things that the new implementation cleans up that we could perhaps backport to the ring ticketer, like treating the Ticketer name in doc comment like code, adding an empty line between methods, and moving the initialization of the Ticketer type into a new() method. Might be nice to include those in the ring Ticketer before it gets copied? On the other hand, might not be very high value.
The `try_split_at` helper in the `*ring*`-like ticketer module will soon be useful for an `aws-lc-rs` specific ticketer impl. This commit lifts the helper up out of the `crypto/ring` mod and into a new `polyfill` where it can be shared by both `crypto/ring` and `crypto/aws-lc-rs` without any fuss. If needed we can add other polyfills here in the future.
Much of the `aws-lc-rs` API surface returns an `aws_lc_rs::error::Unspecified` instance that we want to map to an `Error::Other(OtherError)`, taking care to handle the std vs no-std differences in that type. The `unspecified_err()` helper fn from `crypto/aws_lc_rs/hpke.rs` can do this job for us. Let's lift it from `crypto/aws_lc_rs/hpke` to `crypto/aws_lc_rs` so other modules (notably the ticketer) can benefit from it.
Updates the rustdoc to use backticks for code references, and to fix the comment on the no-std constructor to match the std constructor w.r.t what the encryption mechanism is.
This lets `make_ticket_generator` focus on boxing the constructed `AeadTicketer` to adapt to the interface required by the `TicketSwitcher`'s generator.
Previously the `aws_lc_rs` crypto module shared its `Ticketer` implementation with `ring` using the "ring-like" mechanism we use for other shared implementations. In preparation for the `aws-lc-rs` version to be rewritten to follow the RFC 5077 recommended construction this commit splits the shared module, copying the existing code under the `aws-lc-rs` module. We can remove the `duplicate_mod` clippy allow from the `ring` module's copy since it is no longer being `path` included into two places.
Previously the `aws_lc_rs` crypto module shared its `Ticketer` implementation with `ring` using the "ring-like" mechanism we use for other shared implementations. This commit replaces it with an RFC 5077 based design that we believe addresses some theoretical concerns with the design of the ring specific `Ticketer` using APIs only available with `aws-lc-rs`. For background, the ring ticketer is designed around an AEAD using random nonces. We must use an AEAD with `*ring*` because it doesn't expose an unauthenticated cipher like AES-256 CBC we could use for the RFC 5077 design. We must use random nonces with this design to avoid privacy leaks. The combination of these two constraints results in a design that has a more limited security margin (due to the size of the nonces and the implications of re-use). This commit adds an `aws-lc-rs` Ticketer implementation that is a direct translation of the RFC 5077 "Recommended Ticket Construction"[0]. It uses AES-256 CBC for encryption and HMAC-SHA-256 for message authentication. The unit tests from the existing `*ring*` ticketer are duplicated here to test the new RFC 5077 implementation. It may be possible to share these tests between the two, but a straight-forward duplication felt better in this instance given how simple the tests are. [0]: https://www.rfc-editor.org/rfc/rfc5077#section-4
Previously `TICKETER_AEAD` was a `crypto/ring/mod.rs` level constant so that the `aws-lc-rs` based impl could choose a different (FIPS compatible) AEAD by providing a different `TICKETER_AEAD` constant in its module. Now that the `aws-lc-rs` impl is using a different ticketer implementation we can lift this constant out of the `ring` module and into the `ring/ticketer` module and update the docs to make it clearer that CHACHA20-POLY1305 is used whenever the `*ring*` ticketer is used.
b0413af to
833e85a
Compare
Good call-out. It feels nice to port some of those improvements backwards to the original impl. I went ahead and did that before the copy over into |
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.
Previously the
aws_lc_rscrypto module shared itsTicketerimplementation withringusing the "ring-like" mechanism we use for other shared implementations. This branch replaces it with an RFC 5077 based design that we believe addresses some theoretical concerns (#2023) with the design of the ring specificTicketer, using APIs only available withaws-lc-rs.For background, the ring ticketer is designed around an AEAD using random nonces. We must use an AEAD with
*ring*because it doesn't expose an unauthenticated cipher/modes we could use for the RFC 5077 design. We must use random nonces with the AEAD design to avoid privacy leaks. The combination of these two constraints results in a design that has a more limited security margin (due to the size of the nonces and the implications of re-use).This commit adds an
aws-lc-rsTicketer implementation that is a direct translation of the RFC 5077 "Recommended Ticket Construction". It uses AES-256 in CBC mode w/ PKCS#7 padding for encryption and HMAC-SHA-256 for message authentication.Resolves #2021