key_update API and automatic key refreshing
#2003
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2003 +/- ##
==========================================
+ Coverage 94.18% 94.23% +0.05%
==========================================
Files 97 97
Lines 21640 21729 +89
==========================================
+ Hits 20382 20477 +95
+ Misses 1258 1252 -6 ☔ View full report in Codecov by Sentry. |
Benchmark resultsInstruction countsSignificant differencesClick to expand
Other differencesClick to expand
Wall-timeSignificant differencesClick to expand
Other differencesClick to expand
Additional informationCheckout details:
|
cpu
reviewed
Jun 12, 2024
ctz
force-pushed
the
jbp-key-update-request
branch
6 times, most recently
from
d11169e
to
5173e7f
Compare
|
This is ready, I think. I have hoisted the more incidental changes to the top of the PR. |
cpu
approved these changes
Jun 19, 2024
a2fd30c introduced `queued_key_update_message` which logically contains an encryption of a key_update response. Because it is encrypted, it must be disbursed _before_ any further encryptions. This didn't happen, for example in this sequence of events: - we receive a key_update with UpdateRequested, and fill in `queued_key_update_message`, - we send any other non-ApplicationData message, adding it to `sendable_tls`, - we send some ApplicationData, moving `queued_key_update_message` into `sendable_tls` This leads to `sendable_tls` containing out-of-order messages, that the peer will fail to decrypt.
This removes `encrypt_exhausted`, `wants_close_before_encrypt` and `remaining_write_seq` and unifies those into one API. The single API is shared between the unbuffered and buffered code to avoid these falling out of line.
Limited by SEQ_SOFT_LIMIT for suites that have no limit.
ctz
force-pushed
the
jbp-key-update-request
branch
from
5173e7f
to
2cb52af
Compare
These were copies of the QUIC values, which made them pessimistic (QUIC's largest message is 2 ** 16, TCP-TLS is 2 ** 14). Double them. Add documentation & references of how these are calculated.
Overwhelmingly `sendable_tls` is empty in this code path, but when it is not (eg, an alert or other post-handshake handshake message), it _must_ be included before further encryptions are performed. Once that is achieved, we can eliminate the special handling of `queued_key_update_message` in `write_plaintext`.
Ensures unbuffered API respects the 7d4e809 fix.
This was previously untested and... didn't work very well. First, sending a close_notify when `send_single_fragment` refuses to send anything further is not fruitful. Exempt alerts from that (they have very little secret content, and we send few if any on a given connection, so cannot meaningfully contribute to reaching the key's birthday bound.) Second, `send_close_notify` when `send_single_fragment` will itself call `send_close_notify` does not terminate. Use the existing `send_close_notify` idempotency to prevent this. Finally, add an error-level log to this code path. It is uncommon, and fatal to the connection.
Calling `eager_send_close_notify` here was wrong, as it is impossible to communicate to the caller that a message has been written to `outgoing_tls` (via its length), _and_ return the error. Instead, use `send_close_notify` which pends data to be sent on the next `EncodeTlsData` state.
ctz
force-pushed
the
jbp-key-update-request
branch
from
2cb52af
to
86010c9
Compare
djc
reviewed
Jun 21, 2024
| @@ -1554,6 +1554,14 @@ impl State<ClientConnectionData> for ExpectTraffic { | |||
| Ok(self) | |||
| } | |||
|
|
|||
| fn send_key_update_request(&mut self, common: &mut CommonState) -> Result<(), Error> { | |||
There was a problem hiding this comment.
Can we deduplicate more of this logic across client/server, maybe by moving it into a method on CommonState?
There was a problem hiding this comment.
Have done this, though it's quite minor -- I would very much like to avoid any dependency from common_state to tls13::key_schedule.
| .record_layer | ||
| .pre_encrypt_action(f as u64) | ||
| { | ||
| record_layer::PreEncryptAction::Nothing => {} |
There was a problem hiding this comment.
Nit: maybe import record_layer::PreEncryptAction.
| /// | ||
| /// If that is not possible (for example, the connection is TLS1.2), a `close_notify` | ||
| /// alert should be sent instead. | ||
| RefreshOrClose, |
There was a problem hiding this comment.
I'd have pulled this back into the originating commit.
Sorry about that -- see #2016 for follow-up |
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.
This PR:
key_updaterequests. This is available in both the unbuffered and classic APIs.key_updaterequests when our encryption key approaches "exhaustion" due to the birthday bound1fixes #946
fixes #755
Footnotes
Note: as before, we track this with the sequence number rather than counting blocks, so sending small messages underestimates the limit significantly. This errs on the side of safety: (for AES-GCM suites) we update keys each 16 million messages whether that represents 16MB or 256GB of data transfer. ↩