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

Amplification limit in recovery #1862

Merged
merged 10 commits into from Oct 21, 2018
Merged

Amplification limit in recovery #1862

merged 10 commits into from Oct 21, 2018

Conversation

ianswett
Copy link
Contributor

@ianswett ianswett commented Oct 17, 2018

Follow up for Issue #1764

Also fixes some remaining inconsistencies from #1859

draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
CRYPTO data if possible.

Until the server has confirmed the path, it must not send more than 3
times the number of received bytes, as specified in the {{QUIC-TRANSPORT}}.
Copy link
Member

Choose a reason for hiding this comment

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

Is this MUST NOT? You might avoid duplicating the requirement by saying that the server is limited in the amount of data it can send, as specified in {{QUIC-TRANSPORT}}.


On each consecutive expiration of the crypto timer without receiving an
acknowledgement for a new packet, the sender SHOULD double the crypto
handshake timeout and set a timer for this period.
Copy link
Member

Choose a reason for hiding this comment

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

The timeouts are still inconsistently named. We have "crypto handshake timeout" here, but "crypto timeout" on 321/322. I think that "crypto retransmission timeout " is most correct. You should fix the section title as well.


The TLP and RTO timers are armed when there are no unacknowledged crypto
packets. The TLP timer is set until the max number of TLP packets have been
sent, and then the RTO timer is set.
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to delete all of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe it's duplicative of the more detailed text above.

Copy link
Member

Choose a reason for hiding this comment

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

OK. It seemed entirely unrelated, but I see we have text on both elsewhere.

@MikeBishop MikeBishop added design An issue that affects the design of the protocol; resolution requires consensus. -recovery labels Oct 17, 2018
CRYPTO data if possible.

Until the server has validated the client's address on the path, the number of
bytes it can send is limited, as specified in the {{QUIC-TRANSPORT}}.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/the//

Until the server has validated the client's address on the path, the number of
bytes it can send is limited, as specified in the {{QUIC-TRANSPORT}}.
If not all unacknowledged CRYPTO data can be sent, then all CRYPTO data in
Initial encryption should be retransmitted. If no bytes may be sent,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be "in the Initial encryption level" or "in Initial packets"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, probably Initial packets.

Copy link
Contributor

@martinduke martinduke left a comment

Choose a reason for hiding this comment

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

Where is the bit about clients sending some PADDING to avoid deadlock? I thought that was supposed to be here.


Until the server has validated the client's address on the path, the number of
bytes it can send is limited, as specified in {{QUIC-TRANSPORT}}.
If not all unacknowledged CRYPTO data can be sent, then all CRYPTO data sent
Copy link
Contributor

Choose a reason for hiding this comment

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

"then all unacknowledged CRYPTO data sent"

Co-Authored-By: ianswett <ianswett@google.com>
@@ -318,12 +318,27 @@ is available, or if the network changes, the initial RTT SHOULD be set to 100ms.
When an acknowledgement is received, a new RTT is computed and the timer
SHOULD be set for twice the newly computed smoothed RTT.

When crypto packets are sent, the sender SHOULD set a timer for the crypto
When crypto packets are sent, the sender MUST set a timer for the crypto
timeout period. Upon timeout, the sender MUST retransmit all unacknowledged
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
timeout period. Upon timeout, the sender MUST retransmit all unacknowledged
retransmission interval. Upon timeout, the sender MUST retransmit all unacknowledged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doc uses timeout and timeout period fairly consistently, so I'd like to keep it as is.

draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved

Until the server has validated the client's address on the path, the number of
bytes it can send is limited, as specified in {{QUIC-TRANSPORT}}.
If not all unacknowledged CRYPTO data can be sent, then all unacknowledged
Copy link
Member

Choose a reason for hiding this comment

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

I think that you mean to say instead

If all CRYPTO data is acknowledged, and there is nothing for the client to send, then it sends another Initial packet.

What that packet contains clearly doesn't matter (though an ACK would be perfectly OK, as would repeating information that the server already has), so I don't think we need to stipulate that explicitly. Otherwise, this is a little ambiguous - and maybe in conflict with what we did in #1843 and the next paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This paragraph talks about the server's behavior, not the client's?


The TLP and RTO timers are armed when there are no unacknowledged crypto
packets. The TLP timer is set until the max number of TLP packets have been
sent, and then the RTO timer is set.
Copy link
Member

Choose a reason for hiding this comment

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

OK. It seemed entirely unrelated, but I see we have text on both elsewhere.

@mikkelfj
Copy link
Contributor

With these MUST retransmit, has it been considered what a Slowloris attack could do to the handshake? I know that there is a general mention in some section.

martinthomson and others added 2 commits October 19, 2018 05:24
Co-Authored-By: ianswett <ianswett@google.com>
Co-Authored-By: ianswett <ianswett@google.com>
@ianswett ianswett merged commit efe45fc into master Oct 21, 2018
@ianswett
Copy link
Contributor Author

@mikkelfj The slowloris attack is against a server, but the peer that must retransmit is the client in this case, so I don't think this particular text is relevant to slowloris?

@mikkelfj
Copy link
Contributor

I got the same thought after writing it, but I suspected the logic could be similar server side - I'm not sufficiently into that handshake details atm.

@martinthomson martinthomson deleted the ianswett-amplification branch October 26, 2018 00:24
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

5 participants