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

Rephrase size requirement for Initial packets #2520

Merged
merged 3 commits into from
Mar 15, 2019
Merged

Conversation

janaiyengar
Copy link
Contributor

Closes #2519.

is not overly constrained by the amplification restriction.
Clients MUST ensure that UDP datagrams containing Initial packets are sized to
at least 1200 bytes, padding packets in the datagram if necessary. Once a
client has received an acknowledgment for a Handshake packet it MAY send smaller
Copy link
Member

@kazuho kazuho Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this looks like an outdated text that was written before the TLS handshake design in draft-12.

Now that the server responds with an Initial packet, my understanding is that a client in required to send full-sized datagrams until it receives an Initial packet from the server.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should stick with the simplest text possible and we should remove the second sentence. Once you've received an ACK for a handshake packet, you should be done sending INITIAL packets anyway, so it feels like an unnecessary optimization.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with @ianswett since the time when you receive the ACK for the Handshake packet is exactly when you stop sending Initial frames, so this sentence feels like its pointing out a subtlety that doesn't exist.

If the point of the sentence is to say that, while UDP datagrams containing Initial packets MUST be >= 1200 bytes, all others may or may not be padded (at the implementation's discretion), it would be clearer to say:

Any UDP datagram that does not contain any Initial packets MAY be smaller than 1200 bytes.

@dtikhonov
Copy link
Member

This change does two things:

  1. Clarify that if UDP datagram contains Initial packets, it must be at least 1200 bytes in size; and
  2. Rule out padding the UDP datagram with junk (the "padding packets" part) as @nibanks suggested earlier.

Issue #2519 is only about (1). Not that I am against (2), but this is just so that we are clear.

@martinthomson
Copy link
Member

I think that the reinterpretation of the text here has introduced the second problem @dtikhonov mentions. This also has the unfortunate consequence of disallowing small acknowledgment datagrams (for HelloRetryRequest in particular). This might need to be more carefully phrased.

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A potential friendly amendment, but then we should proceed. I would say after confirming with chairs, because there was a pretty strong design decision involved here, but technically this is only an editorial change.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
martinthomson added a commit that referenced this pull request Mar 15, 2019
martinthomson and others added 2 commits March 15, 2019 13:10
Co-Authored-By: janaiyengar <jri.ietf@gmail.com>
@janaiyengar
Copy link
Contributor Author

As @martinthomson said, this is an editorial change, so I'm going to merge. The substantive one is in @2523.

@janaiyengar
Copy link
Contributor Author

janaiyengar commented Mar 15, 2019

@dtikhonov -- you're right, but I think this was unintentional earlier. I think if we want to make a case for allowing junk, that should be discussed separately. The proposed text captures what I believe our intent was.

@janaiyengar janaiyengar merged commit 3c93c56 into master Mar 15, 2019
@martinthomson martinthomson deleted the jri/padding branch March 25, 2019 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify initial UDP datagram padding
6 participants