Subject: Re: [quicwg/base-drafts] Include handshake anti-deadlock logic in pseudocode (#2281)

From: ianswett <[email-masked]>
Date: Tue, 02 Apr 2019 21:54:50 +0000 (UTC)

ianswett commented on this pull request.

Thanks for fixing this.  Because of the deadlock prevention, this is definitely a trick to get right.

It'd be nice to use the "handshake complete" concept in recovery, but defining it may be more work than it's worth.

> @@ -1078,6 +1078,19 @@ GetEarliestLossTime():
   return time, space
 
 SetLossDetectionTimer():
+  if (crypto packets are in flight

Two suggestions: 
1) If loss_time is non-zero, it will be earlier than the crypto retransmission timer, so it's important to check that first.

2) The check for ack-eliciting packets can be moved right in front of the PTO timer.

>      // Crypto retransmission timeout.
     RetransmitUnackedCryptoData()
     crypto_count++
+  else if (endpoint is client without 1-RTT keys):
+    if (will send Initial packet):
+       SendOnePaddedPacket()
+     else:
+       SendOnePacket()
+    crypto_count++
+  else if (loss_time != 0):

Time threshold loss detection should still be before the crypto case.

>      // Crypto retransmission timeout.
     RetransmitUnackedCryptoData()
     crypto_count++
+  else if (endpoint is client without 1-RTT keys):
+    if (will send Initial packet):

I'd say
"if (has Handshake keys):
  SendOneHandshakePacket()
 else
  SendOnePaddedInitialPacket()" 

-- 
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/2281#pullrequestreview-221761780

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 02 Apr 2019 17:12:41 -0700

Ralith commented on this pull request.

   return time, space
 
 SetLossDetectionTimer():

Thanks, I was confused about the intended behavior here.

-- 
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/2281#discussion_r271540756

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 02 Apr 2019 17:13:54 -0700

@martinthomson, is this consistent with your earlier suggestions?

-- 
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/2281#discussion_r271540975

---

From: Martin Thomson <[email-masked]>
Date: Wed, 03 Apr 2019 15:05:37 +0000 (UTC)

martinthomson commented on this pull request.

No. The important factor is not whether you have handshake keys, but whether your peer believes your address to be validated. 

-- 
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/2281#discussion_r271789456

---

From: ianswett <[email-masked]>
Date: Wed, 03 Apr 2019 08:28:20 -0700

To be clear, my suggestion started at line 1133.  The intent was to change "Will send Initial packet" to "has Handshake keys", which I believe is clearer.

-- 
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/2281#discussion_r271800450

---

From: Martin Thomson <[email-masked]>
Date: Thu, 04 Apr 2019 09:26:42 +0000 (UTC)

"Will send Initial packet" is correct.

-- 
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/2281#discussion_r272092316

---

From: Martin Thomson <[email-masked]>
Date: Thu, 04 Apr 2019 02:33:47 -0700

martinthomson 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/2281#pullrequestreview-222652861

---

From: ianswett <[email-masked]>
Date: Thu, 04 Apr 2019 13:39:04 +0000 (UTC)

@martinthomson Why?  In particular, do you think it is more correct?

This is already in a conditional that indicate the 1RTT keys aren't present.

@Ralith please add a comment above this section as well, to explain why it's here, since it may not be obvious.

-- 
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/2281#discussion_r272182919

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 04 Apr 2019 17:15:03 -0700

janaiyengar commented on this pull request.

I think I understand @martinthomson's point -- you want to keep sending initial packets until you know that the server has received and processed a handshake packet, which is when the server considers the client validated. This would happen when the client receives an ACK in a Handshake packet (for a Handshake packet) or has the 1-RTT keys.

That said, I agree with @ianswett that "will send Initial packet" is an odd predicate. I think we can say (has 1-RTT keys or has received an ACK in a Handshake packet). I could be wrong, but I think this captures the condition we're looking for.

-- 
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/2281#discussion_r272411065

---

From: Benjamin Saunders <[email-masked]>
Date: Thu, 04 Apr 2019 17:34:03 -0700

That sounds like the approach that I originally had here, which was [argued against](https://github.com/quicwg/base-drafts/pull/2281/files/8088a86a41852b071d3fee371357da4cf56e3a12#discussion_r248056667).

-- 
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/2281#discussion_r272413819

---

From: ianswett <[email-masked]>
Date: Thu, 04 Apr 2019 19:37:23 -0700

Martin didn't want to allow for ACK of handshake stopping the anti-amplification retransmissions, which I think is fine, though a bit overly strict.  So all of this is within the "if (endpoint is client without 1-RTT keys)" conditional, which is our proxy for: Should we keep trying to prevent a deadlock?

To be clear on what I'm suggesting:

else if (endpoint is client without 1-RTT keys):
  // The client must continue sending packets until it's
  // sure the server has validated the path.
  if (has Handshake keys):
    SendOneHandshakePacket()
  else
    SendOnePaddedInitialPacket()

-- 
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/2281#discussion_r272428828

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 04 Apr 2019 22:22:37 -0700

I see. Your proposal seems good to me. @martinthomson: can you clarify what you think is missing?

-- 
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/2281#discussion_r272447280

---

From: Martin Thomson <[email-masked]>
Date: Sun, 07 Apr 2019 17:57:41 -0700

Striking out the bit that we've agreed to remove, the text from the transport draft is:

> Clients MUST ensure that UDP datagrams containing Initial packets are sized to at least 1200 bytes, adding padding to packets in the datagram as necessary. ~~Once a client has received an acknowledgment for a Handshake packet it MAY send smaller datagrams.~~ Sending padded datagrams ensures that the server is not overly constrained by the amplification restriction.

This contradicts that text.  It's true that a server that received a Handshake packet is no longer subject to the anti-DoS limit on bytes transmitted, but it's a bit weird that we have an algorithm that directly contradicts a MUST-level requirement in the other document.

-- 
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/2281#discussion_r272861857

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 08 Apr 2019 21:49:41 +0000 (UTC)

@martinthomson: AIUI, you'd remember this better since it was your PR, but wasn't the bit we agreed to remove was about special-casing padding of Initial packets? My reading of @ianswett 's proposed text doesn't contradict that.

-- 
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/2281#discussion_r273252767

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 09 Apr 2019 15:32:18 -0700

@janaiyengar pushed 1 commit.

5989bf138d79c168b83045cae8b3ccd6ffcd5b31  Update to reflect editors' discussion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2281/files/7e0a5c0e9560f63fb18a144e160c76bb7aee8e29..5989bf138d79c168b83045cae8b3ccd6ffcd5b31

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 09 Apr 2019 15:34:04 -0700

Merged #2281 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/2281#event-2264412206