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

Ignore packet loss when the keys are not available #2327

Merged
merged 20 commits into from Jan 23, 2019

Conversation

ianswett
Copy link
Contributor

@ianswett ianswett commented Jan 9, 2019

Ignore the loss of Handshake, 0-RTT, and 1-RTT packets if the peer doesn't have the keys to process them.

Also unifies the text on discarding keys and no longer implies that 0-RTT has it's own packet number space.

Fixes #1967 Replaces #2028

Ignore the loss of Handshake, 0-RTT, and 1-RTT packets if the peer doesn't have the keys to process them.

Fixes #1967 

Replaces #2028
@ianswett ianswett changed the title Update draft-ietf-quic-recovery.md Ignore the packet loss when the keys are not available Jan 9, 2019
@ianswett ianswett changed the title Ignore the packet loss when the keys are not available Ignore packet loss when the keys are not available Jan 9, 2019
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.

Some suggestions, but this is good. Thanks.

draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
martinthomson and others added 8 commits January 10, 2019 07:04
Co-Authored-By: ianswett <ianswett@users.noreply.github.com>
Co-Authored-By: ianswett <ianswett@users.noreply.github.com>
Co-Authored-By: ianswett <ianswett@users.noreply.github.com>
Co-Authored-By: ianswett <ianswett@users.noreply.github.com>
Co-Authored-By: ianswett <ianswett@users.noreply.github.com>
@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -recovery design An issue that affects the design of the protocol; resolution requires consensus. and removed editorial An issue that does not affect the design of the protocol; does not require consensus. labels Jan 15, 2019
Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

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

A few comments, editorial.

draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
#### Discarding Keys and Packet State {#discarding-packets}

When packet protection keys are discarded (see Section 4.9 of {{QUIC-TLS}}),
recovery state for all in-flight packets sent with those keys is discarded.
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 a slight restructure might help make this text clearer. I've also rephrased some.

"When packet protection keys are discarded (see Section 4.9 of {{QUIC-TLS}}),
all packets that were sent with those keys can no longer be acknowledged because
their acknowledgements cannot be processed anymore. The sender considers them
no longer in flight. That is, the sender SHOULD discard all recovery state associated
with those packets and MUST remove them from the count of bytes in flight.

As described in Section 17.5.1 of {{QUIC-TRANSPORT}}, endpoints stop sending and
receiving Initial packets once they start exchanging Handshake packets. At this
point, recovery state for all in-flight Initial packets is discarded.

When 0-RTT is rejected, recovery state for all in-flight 0-RTT packets is
discarded.

If a server accepts 0-RTT, but does not buffer 0-RTT packets that arrive
before Initial packets, early 0-RTT packets will be declared lost, but that
is expected to be infrequent.

It is expected that keys are discarded after packets encrypted with them would be
acknowledged or declared lost, with the exception of Initial secrets, which might be
destroyed earlier."

The packets are removed from the count of bytes in flight and no
acknowledgements or loss events will occur for those packets. Note that it
is expected that keys are discarded after those packets would be declared lost,
but Initial secrets are destroyed earlier.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this last sentence. Can you rephrase? Specifically, the Initial secrets are destroyed earlier than what event?

Copy link
Member

Choose a reason for hiding this comment

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

IIUC this refers to the provision in https://quicwg.org/base-drafts/draft-ietf-quic-tls.html#rfc.section.4.10. Maybe it's worth adding a reference.

draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
janaiyengar and others added 5 commits January 16, 2019 12:20
Co-Authored-By: ianswett <ianswett@users.noreply.github.com>
Co-Authored-By: ianswett <ianswett@users.noreply.github.com>
Co-Authored-By: ianswett <ianswett@users.noreply.github.com>
martinthomson and others added 2 commits January 16, 2019 16:51
Co-Authored-By: ianswett <ianswett@users.noreply.github.com>
ins: S. Turner
name: Sean Turner
org: sn3rd
email: sean@sn3rd.com
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
email: sean@sn3rd.com

@janaiyengar
Copy link
Contributor

merging this one as well to get this in -18.

@janaiyengar janaiyengar merged commit c2ade6b into master Jan 23, 2019
martinthomson added a commit that referenced this pull request Jan 23, 2019
@MikeBishop MikeBishop deleted the ianswett-ignore-loss branch February 6, 2019 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-recovery design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants