Subject: [quicwg/base-drafts] Amplification limit in recovery (#1862)

From: ianswett <[email-masked]>
Date: Tue, 16 Oct 2018 18:07:38 -0700

Follow up for Issue #1764
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/1862

-- Commit Summary --

  * Amplification limit in recovery

-- File Changes --

    M draft-ietf-quic-recovery.md (52)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/1862.patch
https://github.com/quicwg/base-drafts/pull/1862.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862

---

From: ianswett <[email-masked]>
Date: Tue, 16 Oct 2018 18:09:25 -0700

@ianswett pushed 1 commit.

0cc808b  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862/files/d2c9ae53b6fe5ef46432a1fca1f1b8c5c8f64f8d..0cc808b873a118f6708d6547d186f3b2fbdfda78

---

From: Martin Thomson <[email-masked]>
Date: Tue, 16 Oct 2018 19:44:44 -0700

martinthomson commented on this pull request.

> @@ -320,10 +320,17 @@ SHOULD be set for twice the newly computed smoothed RTT.
 
 When crypto packets are sent, the sender SHOULD set a timer for the crypto
 timeout period.  Upon timeout, the sender MUST retransmit all unacknowledged
-CRYPTO data by calling RetransmitAllUnackedCryptoData(). 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.
+CRYPTO data if possible.
+
+Until the server has confirmed the path, it must not send more than 3

rather than "confirmed the path", we should be more consistent and say "the server has validated the client's address on the path"

```suggestion
Until the server has validated the client's address on the path, it must not send more than 3
```

> @@ -320,10 +320,17 @@ SHOULD be set for twice the newly computed smoothed RTT.
 
 When crypto packets are sent, the sender SHOULD set a timer for the crypto
 timeout period.  Upon timeout, the sender MUST retransmit all unacknowledged
-CRYPTO data by calling RetransmitAllUnackedCryptoData(). 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.
+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}}.

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}}.

> @@ -320,10 +320,17 @@ SHOULD be set for twice the newly computed smoothed RTT.
 
 When crypto packets are sent, the sender SHOULD set a timer for the crypto
 timeout period.  Upon timeout, the sender MUST retransmit all unacknowledged
-CRYPTO data by calling RetransmitAllUnackedCryptoData(). 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.
+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}}.
+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,
+then no alarm should be armed until bytes have been received by the peer.
+
+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.

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.

> -closed once a reject is sent, so no timer is set to retransmit the reject.
-
-Version negotiation packets are always stateless, and MUST be sent once per
-handshake packet that uses an unsupported QUIC version, and MAY be sent in
-response to 0-RTT packets.
-
-#### Tail Loss Probe and Retransmission Timer
-
-Tail loss probes {{?TLP}} and retransmission timeouts {{?RFC6298}}
-are timer based mechanisms to recover from cases when there are
-outstanding retransmittable packets, but an acknowledgement has
-not been received in a timely manner.
-
-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.

Did you mean to delete all of this?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#pullrequestreview-165437602

---

From: ianswett <[email-masked]>
Date: Wed, 17 Oct 2018 06:16:52 -0700

ianswett commented on this pull request.

-

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#discussion_r225922691

---

From: ianswett <[email-masked]>
Date: Wed, 17 Oct 2018 13:17:26 +0000 (UTC)

1730b2d  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862/files/0cc808b873a118f6708d6547d186f3b2fbdfda78..1730b2db4aafa13f0abf5df1ad08ad9abe9bf15c

---

From: ianswett <[email-masked]>
Date: Wed, 17 Oct 2018 06:22:59 -0700

e7c83d2  Crypto retransmission timer

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862/files/1730b2db4aafa13f0abf5df1ad08ad9abe9bf15c..e7c83d267088cda790341f728205601e6f8a781b

---

From: ianswett <[email-masked]>
Date: Wed, 17 Oct 2018 06:25:01 -0700

360869e  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862/files/e7c83d267088cda790341f728205601e6f8a781b..360869ecd82c71cab00adad53330efbc51b4aed7

---

From: Mike Bishop <[email-masked]>
Date: Wed, 17 Oct 2018 11:26:22 -0700

MikeBishop commented on this pull request.

 When crypto packets are sent, the sender SHOULD set a timer for the crypto
 timeout period.  Upon timeout, the sender MUST retransmit all unacknowledged
-CRYPTO data by calling RetransmitAllUnackedCryptoData(). 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.
+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}}.

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}}.

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#pullrequestreview-165773669

---

From: ianswett <[email-masked]>
Date: Wed, 17 Oct 2018 12:09:09 -0700

Hmm, probably Initial packets.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#discussion_r226059031

---

From: ianswett <[email-masked]>
Date: Wed, 17 Oct 2018 12:09:58 -0700

97f8bf2  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862/files/360869ecd82c71cab00adad53330efbc51b4aed7..97f8bf265bb724cb06468cafd018bffed07fdf96

---

From: martinduke <[email-masked]>
Date: Wed, 17 Oct 2018 15:00:01 -0700

martinduke requested changes on this pull request.

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

 When crypto packets are sent, the sender SHOULD set a timer for the crypto
 timeout period.  Upon timeout, the sender MUST retransmit all unacknowledged
-CRYPTO data by calling RetransmitAllUnackedCryptoData(). 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.
+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 {{QUIC-TRANSPORT}}.
+If not all unacknowledged CRYPTO data can be sent, then all CRYPTO data sent

"then all unacknowledged CRYPTO data sent"

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#pullrequestreview-165853418

---

From: ianswett <[email-masked]>
Date: Thu, 18 Oct 2018 20:58:14 +0000 (UTC)

b523410  Client always sends

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862/files/97f8bf265bb724cb06468cafd018bffed07fdf96..b523410378cac81df39f579543275b53f85100e2

---

From: Martin Thomson <[email-masked]>
Date: Thu, 18 Oct 2018 18:38:05 -0700

>  timeout period.  Upon timeout, the sender MUST retransmit all unacknowledged
 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 {{QUIC-TRANSPORT}}.
-If not all unacknowledged CRYPTO data can be sent, then all CRYPTO data sent
-in Initial packets should be retransmitted.  If no bytes may be sent,
-then no alarm should be armed until bytes have been received by the peer.
+If not all unacknowledged CRYPTO data can be sent, then all unacknowledged
+CRYPTO data sent in Initial packets should be retransmitted.  If no bytes
+may be sent, then no alarm should be armed until bytes have been received from
+the client.
+
+Because the server may be blocked until more packets are received, the
+client MUST arm the crypto retransmission alarm even if there is no

timer, not alarm
```suggestion
client MUST start the crypto retransmission timer even if there is no
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#pullrequestreview-166354410

---

From: ianswett <[email-masked]>
Date: Thu, 18 Oct 2018 18:38:47 -0700

289a942  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862/files/b523410378cac81df39f579543275b53f85100e2..289a942a541a38bbd419d6120732533498531eff

---

From: Martin Thomson <[email-masked]>
Date: Fri, 19 Oct 2018 06:29:08 +0000 (UTC)

> @@ -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

```suggestion
retransmission interval.  Upon timeout, the sender MUST retransmit all unacknowledged
```

```suggestion
can be sent, then no alarm should be armed until bytes have been received from
```

```suggestion
Because the server could be blocked until more packets are received, the
```

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.

-

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#pullrequestreview-166390526

---

From: MikkelFJ <[email-masked]>
Date: Fri, 19 Oct 2018 01:21:01 -0700

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#issuecomment-431284348

---

From: ianswett <[email-masked]>
Date: Fri, 19 Oct 2018 02:24:42 -0700

d2f45ff  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862/files/289a942a541a38bbd419d6120732533498531eff..d2f45ff289284845607edc032c4f6bf50f47621f

---

From: ianswett <[email-masked]>
Date: Fri, 19 Oct 2018 02:25:25 -0700

dd7fc95  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862/files/d2f45ff289284845607edc032c4f6bf50f47621f..dd7fc95cadd189a1430da38f7054034036d689af

---

From: martinduke <[email-masked]>
Date: Sat, 20 Oct 2018 22:18:17 -0700

martinduke approved this pull request.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#pullrequestreview-166770467

---

From: ianswett <[email-masked]>
Date: Sun, 21 Oct 2018 09:54:18 -0700

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#discussion_r226869939

---

From: ianswett <[email-masked]>
Date: Sun, 21 Oct 2018 10:01:03 -0700

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#discussion_r226870110

---

From: ianswett <[email-masked]>
Date: Sun, 21 Oct 2018 10:01:40 -0700

Merged #1862 into master.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#event-1916741625

---

From: ianswett <[email-masked]>
Date: Sun, 21 Oct 2018 11:10:07 -0700

@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?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#issuecomment-431690675

---

From: MikkelFJ <[email-masked]>
Date: Sun, 21 Oct 2018 11:18:02 -0700

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1862#issuecomment-431691293