Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intentionally disable fragmenting KeyUpdates #3708

Merged
merged 4 commits into from
Jan 4, 2023

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Dec 14, 2022

Resolved issues:

Partially addresses #3432

Description of changes:

While working on a more general solution for fragmenting post-handshake messages, I ran into some issues with KeyUpdates.

Our usual fragmentation story is:

  1. Write full handshake message to a stuffer, ie, handshake.io. This is handled by message-type-specific logic.
  2. Send a record. This is handled by common message-type-agnostic logic.
    1. Write the record header to conn->out
    2. Copy a fragment of the handshake message to conn->out
    3. Encrypt the record
    4. Send the record (this step can block)
    5. Repeat until full message consumed

With a KeyUpdate, we need to change the encryption key after sending the message. So if the KeyUpdate message is fragmented, then we need to trigger some KeyUpdate-specific logic AFTER moving on to the common fragmentation / record-writing logic.

We could of course do that. I thought of several methods to solve that problem, but all of them-- and I'd argue, any possible solution-- will make KeyUpdates more complex. And we don't gain much from that extra complexity, since the KeyUpdate message is only 5 bytes and the absolute minimum fragment size we support is 2 bytes. We'd be making the code more complex to save 3 bytes, and any solution will cost at least 1 byte of stored state.

So rather than implement fragmentation for KeyUpdate messages, I think it's a cleaner solution to just bump the minimum send buffer size up by 3 bytes so that we always ensure that there's sufficient space for a KeyUpdate message.

The only use case I can think of for setting your send buffer that low is that you know that you'll always only be sending a few bytes of application data. This seems unlikely to break anyone. There's also precedent, since we already bumped the minimum up by 1 byte in 4f3c16d to properly handle alerts.

An alternative that wouldn't change any behavior is to keep the current minimum, but error when sending a KeyUpdate message if the send buffer is too small. Since we currently only send a partial KeyUpdate if the send buffer is too small, the connection already fails if it requires a KeyUpdate and the send buffer is too small. We could note this behavior in the method documentation, but it seems like an unnecessary sharp edge and I'd rather prevent it.

Callouts

  • The include changes to seemingly random files are side effects of resolving problems caused by adding the new includes to s2n_config.h for the constants the new macro uses.
  • Watch out for me mixing up fragment size and record size / send buffer size in my documentation. I kept doing it -_- I think I fixed it everywhere, but keep an eye out.

Testing:

New unit tests

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Dec 14, 2022
@lrstewart lrstewart force-pushed the postio_send_keyupdate branch from 83f8dfa to 2f5091e Compare December 14, 2022 00:52
@lrstewart lrstewart marked this pull request as ready for review December 14, 2022 08:54
@lrstewart lrstewart force-pushed the postio_send_keyupdate branch from 0455969 to d1c04df Compare December 21, 2022 08:59
@lrstewart lrstewart enabled auto-merge (squash) January 3, 2023 23:05
@lrstewart lrstewart merged commit 9f22dc1 into aws:main Jan 4, 2023
@lrstewart lrstewart deleted the postio_send_keyupdate branch January 4, 2023 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants