Subject: [quicwg/base-drafts] One Retry/VN per UDP datagram (#2303)

From: Martin Thomson <[email-masked]>
Date: Sun, 06 Jan 2019 17:25:33 -0800

This keeps things simple.

Closes #2259.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * One Retry/VN per UDP datagram

-- File Changes --

    M draft-ietf-quic-transport.md (84)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/2303.patch
https://github.com/quicwg/base-drafts/pull/2303.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/2303

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 06 Jan 2019 17:31:32 -0800

kazuho requested changes on this pull request.

> @@ -3630,6 +3627,9 @@ The Version Negotiation packet does not include the Packet Number and Length
 fields present in other packets that use the long header form.  Consequently,
 a Version Negotiation packet consumes an entire UDP datagram.
 
+A server MUST NOT send more than one Version Negotiation packet in response to a
+single UDP datagram.

I believe this paragraph is the one you intend to add, and it looks fine to me.

OTOH I see lots of unrelated changes that essentially reverts the recently merged changes. Maybe you need to cherry-pick?

-- 
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/2303#pullrequestreview-189656542

---

From: Marten Seemann <[email-masked]>
Date: Sun, 06 Jan 2019 17:59:32 -0800

marten-seemann commented on this pull request.

> @@ -3872,7 +3872,9 @@ Destination Connection ID of subsequent packets that it sends.
 
 A server MAY send Retry packets in response to Initial and 0-RTT packets.  A
 server can either discard or buffer 0-RTT packets that it receives.  A server
-can send multiple Retry packets as it receives Initial or 0-RTT packets.
+can send multiple Retry packets as it receives Initial or 0-RTT packets.  A
+server MUST NOT send more than one Retry packet in response to a single UDP
+datagram.

And this one.

-- 
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/2303#pullrequestreview-189658914

---

From: Martin Thomson <[email-masked]>
Date: Sun, 06 Jan 2019 18:03:48 -0800

I have no idea how that happened.  Forced it.

-- 
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/2303#issuecomment-451800184

---

From: Marten Seemann <[email-masked]>
Date: Sun, 06 Jan 2019 18:05:10 -0800

Are we ok with a server sending both a VN packet and a Retry in response to a single UDP datagram?

-- 
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/2303#issuecomment-451800358

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 06 Jan 2019 18:15:42 -0800

@marten-seemann Does that happen? IIUC VN is sent only when there's a version mismatch. Retry is sent only when there's _no_ version mismatch.

-- 
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/2303#issuecomment-451801686

---

From: Marten Seemann <[email-masked]>
Date: Sun, 06 Jan 2019 18:18:54 -0800

@kazuho: A client could coalesce two packets of different versions.

As I've said on the issue, I still feel uneasy with the fact that a client could coalesce around 70 packets into a single datagram, and I'd prefer more rigid rules to solve this problem.

-- 
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/2303#issuecomment-451802121

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 06 Jan 2019 18:26:51 -0800

@marten-seemann 
> A client could coalesce two packets of different versions.

I'd argue that a client can't do that, because the draft states "Senders MUST NOT coalesce QUIC packets for different connections into a single UDP datagram", OTOH I agree that there's nothing that requires the server to drop following QUIC packets with different versions within a combined datagram.

> As I've said on the issue, I still feel uneasy with the fact that a client could coalesce around 70 packets into a single datagram, and I'd prefer more rigid rules to solve this problem.

As I stated on 
https://github.com/quicwg/base-drafts/issues/2259#issuecomment-450711758, I (still) think the approach in this PR would require having minimal state. Would you mind elaborating (possibly on the issue) why you think differently?

-- 
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/2303#issuecomment-451803169

---

From: ianswett <[email-masked]>
Date: Sun, 06 Jan 2019 19:33:31 -0800

ianswett commented on this pull request.

In what use case are you expecting to send multiple Retrys in response to a UDP datagram?  And in those cases, can we require coalescing the Retrys into a single datagram, and so we could say no more than one UDP datagram in response?

-- 
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/2303#discussion_r245542904

---

From: Marten Seemann <[email-masked]>
Date: Sun, 06 Jan 2019 19:39:51 -0800

Sure, the issue here comes from multi-threaded processing, and reusing packet buffers.

I have one thread (or rather, one go-routine) that reads UDP datagrams from the network, splits the coalesced packets and parses the unencrypted part of the header, and then passes the QUIC packets to their respective connections (which are running in a separate go-routine each). In order to reduce allocations, I reuse the buffers I read my packets into. When handling coalesced packets, this means I have to wait until *all* coalesced parts have been processed before reusing the buffer.
Therefore, I can't just parse the first coalesced packet, pass it on for handling in another threat, and then continue parsing. This would create a race condition, since the handling of the packet might be executed before the parsing is finished, and the packet buffer would be returned already. So I have to split the coalesced packets first, and then handle them.

This wouldn't be a problem at all if a coalesced packet just had a few parts, as it's supposed to be. But since we're maximizing flexibility for no reason, an attacker might send me datagram consisting of around 70 packets, which I would all have to completely parse.

-- 
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/2303#issuecomment-451813086

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 06 Jan 2019 20:09:22 -0800

@marten-seemann Thank you for elaborating.

I think that we should be dispatching each QUIC datagram (instead of each QUIC packet) to the respective connections. That's how load balancers are expected to work, and what you are implementing is essentially a load balancer. The benefit of dispatching at the datagram level is that you'd have less things to do on the receiving thread.

> This wouldn't be a problem at all if a coalesced packet just had a few parts, as it's supposed to be. But since we're maximizing flexibility for no reason, an attacker might send me datagram consis…

I understand your point on flexibility. However, in my view, the flexibility is not only how you coalesce, but the redundancy in all the long header fields. We have deliberately chosen the design because it is clear and easy to analyze (especially in regard to encryption). My preference goes to retaining the clarity by avoiding introducing restrictions.

-- 
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/2303#issuecomment-451816576

---

From: janaiyengar <[email-masked]>
Date: Tue, 15 Jan 2019 16:13:56 -0800

I think this PR addresses the amplification problem directly, by tying the number of response datagrams to the number of incoming datagrams, so I am in favor of this PR as a spec solution.

@marten-seemann : quic-go can still choose to drop datagrams with a large (> 3?) coalesced packets, which might help you (slightly). I agree with @kazuho also that since all coalesced packets are supposed to go to the same connection, the implementation would be better aligned if it were to pass the datagram along to the connection instead of the individual packets... but that's an impl choice, and it's certainly not the only way to do 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/2303#issuecomment-454601267

---

From: janaiyengar <[email-masked]>
Date: Tue, 15 Jan 2019 16:14:33 -0800

I'm merging this. Please reopen the issue if you think this ought to be discussed further, especially in Tokyo.

-- 
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/2303#issuecomment-454601406

---

From: janaiyengar <[email-masked]>
Date: Tue, 15 Jan 2019 16:14:50 -0800

Merged #2303 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/2303#event-2076119455