Make the packet number encryption sampling clearer #1389
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
martinthomson
added
editorial
nibanks
reviewed
May 25, 2018
draft-ietf-quic-tls.md
Outdated
| sample_offset = min(2 + len(destination_connection_id) + | ||
| len(source_connection_id) + | ||
| len(payload_length) + 4, | ||
| packet_length - aead_expansion) |
There was a problem hiding this comment.
I assume packet_length refers to the QUIC packet length and not the UDP payload length, right?
There was a problem hiding this comment.
Ahh, I see the problem. You don't know where the payload length starts. It seems like we need to make the payload length cover the packet number length. More changes inbound.
martinthomson
removed
the
editorial
This ensures that the packet number encryption can work because that relies on knowing where the end of the packet is. This means that packet coalescing in -12 was busted.
kazuho
reviewed
May 25, 2018
draft-ietf-quic-tls.md
Outdated
| handled separately. | ||
|
|
||
| ~~~ | ||
| sample_offset = min(2 + len(destination_connection_id) + |
There was a problem hiding this comment.
Shouldn't 2 be 6? There are the type octet, version (4 octets), CILs (1 octet).
tatsuhiro-t
reviewed
May 25, 2018
draft-ietf-quic-tls.md
Outdated
| @@ -942,16 +942,28 @@ Packet number protection is applied after packet protection is applied (see | |||
| encryption algorithm. | |||
|
|
|||
| In sampling the packet ciphertext, the packet number length is assumed to be the | |||
There was a problem hiding this comment.
Shouldn't "packet number length" be "sample_offset"?
There was a problem hiding this comment.
No, but I see how this text is confusing, I'll rephrase and we can try again.
This was missing a few things, which made it confusing.
tatsuhiro-t
approved these changes
May 25, 2018
MikeBishop
reviewed
May 26, 2018
draft-ietf-quic-tls.md
Outdated
| of the protected packet minus the minimum expansion for the AEAD. For example, | ||
| the sampled ciphertext for a packet with a short header can be determined by: | ||
| In sampling the packet ciphertext, the packet number length is assumed to be | ||
| either 4 octets (its maximum possible encoded length), unless there is |
There was a problem hiding this comment.
Either implies two options. I'd just drop it.
martinthomson
added
editorial
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.
Fixes #1387.