Subject: [quicwg/base-drafts] Padding outside QUIC packet (#3333)

From: Tatsuhiro Tsujikawa <[email-masked]>
Date: Sat, 11 Jan 2020 22:16:09 -0800

https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-14 only says that padding is done by PADDING frame.  https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-14

But it looks like it is a common practice to add 0 or garbage outside QUIC packet to fill remaining UDP packet payload to expand packet size.
If it is a good practice (and/or recommended), I think draft should say something about it.
It might slightly simplify the implementation.
Padding in this way has different properties than PADDING frame; for example, it does not contribute to in-flight bytes.

-- 
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/issues/3333

---

From: ekr <[email-masked]>
Date: Sun, 12 Jan 2020 14:02:21 -0800

The only time you can do this is when you have only long header packets (essentially during the handshake). Because short header packets are not length delimited, if you pad on the outside it will be interpreted as part of the packet and the deprotection will fail.

-- 
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/issues/3333#issuecomment-573462194

---

From: Will Hawkins <[email-masked]>
Date: Sun, 12 Jan 2020 21:41:34 -0800

> But it looks like it is a common practice to add 0 or garbage outside QUIC packet to fill remaining UDP packet payload to expand packet size.

Just FYI: Adding 0-value bytes outside QUIC packets to fill remaining UDP datagram is the way that neqo currently performs padding. Of course, as @ekr says, it only does this when it is possible (there are only QUIC packets with long headers in the datagram).

> Padding in this way has different properties than PADDING frame; for example, it does not contribute to in-flight bytes.

  I, too, think it would be nice to have some clarification like this in the standard.  

-- 
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/issues/3333#issuecomment-573515904

---

From: Tatsuhiro Tsujikawa <[email-masked]>
Date: Mon, 13 Jan 2020 00:39:12 -0800

Yes, this kind of padding is for handshake packets only.

-- 
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/issues/3333#issuecomment-573558792

---

From: ianswett <[email-masked]>
Date: Mon, 13 Jan 2020 06:29:38 -0800

As @ekr pointed out, this type of padding doesn't allow coalescing 0-RTT with the client hello or 0.5RTT server data with the server's first flight, so it limits coalescing in common use cases, though I agree may be easier to implement.

To me, the biggest risk here is that the non-QUIC padding is removed by something on the path.  However, that risk seems similar to the risk that something on the path would split a coalesced packet into two UDP datagrams(discussed in #3317), so it seems acceptable.

Because it prevents coalescing in some useful cases, I don't think it should be recommended, but I can't come up with a strong reason for disallowing it, so I think it should continue to be allowed.

-- 
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/issues/3333#issuecomment-573689645

---

From: Tatsuhiro Tsujikawa <[email-masked]>
Date: Mon, 13 Jan 2020 07:10:47 -0800

Another concern about this way of padding is that it does not increase the transmission limit for server.

https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-8.1

> Prior to validating the client address, servers MUST NOT send more than three times as many bytes as the number of bytes they have received. This limits the magnitude of any amplification attack that can be mounted using spoofed source addresses. In determining this limit, servers only count the size of successfully processed packets.

Because bytes outside of QUIC packet is most likely unprocessable, it is not counted to the data size that server has received.   That means that server might get choked during handshake with packet losses.

-- 
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/issues/3333#issuecomment-573709751

---

From: Nick Banks <[email-masked]>
Date: Mon, 13 Jan 2020 07:51:44 -0800

@tatsuhiro-t my understanding is that amplification limit is based on the UDP datagram layer, not the QUIC packet layer. If you receive any bytes from the peer you can see 3x times that in response. They don't have to be valid QUIC packets. I thought there was text that was explicit about that. If not, the text should probably be clarified.

-- 
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/issues/3333#issuecomment-573730981

---

From: ekr <[email-masked]>
Date: Mon, 13 Jan 2020 08:48:19 -0800

This is my understanding as well

On Mon, Jan 13, 2020 at 7:51 AM Nick Banks <[email-masked]> wrote:

> @tatsuhiro-t <https://github.com/tatsuhiro-t> my understanding is that
> amplification limit is based on the UDP datagram layer, not the QUIC packet
> layer. If you receive any bytes from the peer you can see 3x times that in
> response. They don't have to be valid QUIC packets. I thought there was
> text that was explicit about that. If not, the text should probably be
> clarified.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/quicwg/base-drafts/issues/3333?email_source=notifications&email_token=AAIPLIOXMS6JZ2HAISYFFGDQ5SEYNA5CNFSM4KFV7MQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIZHBJI#issuecomment-573730981>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAIPLILWO76VAV5Q32P4QATQ5SEYNANCNFSM4KFV7MQQ>
> .
>

-- 
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/issues/3333#issuecomment-573759288

---

From: Will Hawkins <[email-masked]>
Date: Mon, 13 Jan 2020 10:24:16 -0800

> As @ekr pointed out, this type of padding doesn't allow coalescing 0.5RTT server data with the server's first flight, so it limits coalescing in common use cases, though I agree may be easier to implement.

A server is not *required* pad, is it? https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#name-packet-size

-- 
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/issues/3333#issuecomment-573801530

---

From: ianswett <[email-masked]>
Date: Mon, 13 Jan 2020 10:27:20 -0800

No, padding is not required on the server side, though it's still sometimes useful for path MTU.

-- 
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/issues/3333#issuecomment-573802998

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 13 Jan 2020 17:18:10 -0800

This behavior should continue to be allowed. We allow endpoints to send a QUIC packet (as part of a coalesced packet) knowing that the peer would de unable to decrypt it (see [section 14.3.1](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-14.3.1)). The issue being discussed here is a variant of that.

That said, I am not sure if we want to suggest padding outside of a QUIC packet, because such a design might have wired implications to the CC logic. IIUC, current congestion logic is based on the following principles:
* an ack-eliciting packet consumes bytes-in-flight
* an ack-only packet does not consume bytes-in-flight
* we allow ack-eliciting packets and ack-only packets to be coalesced, because that does not change the load on the network

I am not sure how we can adjust these principles so that padding outside QUIC packets can be taken into consideration (when necessary), or if making such adjustment is worth the additional complexity.

-- 
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/issues/3333#issuecomment-573952468

---

From: Tatsuhiro Tsujikawa <[email-masked]>
Date: Mon, 13 Jan 2020 17:21:32 -0800

I think the current text "In determining this limit, servers only count the size of successfully processed packets." has 2 problems.

- It is unclear that what "packets" indicate: is it UDP packet or QUIC packet?
- It is unclear what "successfully processed packets" means.

-- 
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/issues/3333#issuecomment-573953359

---

From: Marten Seemann <[email-masked]>
Date: Mon, 13 Jan 2020 17:57:16 -0800

> Because bytes outside of QUIC packet are most likely unprocessable, it is not counted to the data size that server has received. That means that server might get choked during handshake with packet losses.

@tatsuhiro-t raises an interesting point here. While it's certainly not impossible to use the size of the UDP datagram for the amplification limit, this would be quite hard to do in my implementation. Undecryptable packets are discarded way down in the stack, and passing up that information to the congestion controller would require quite a bit of extra work.

-- 
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/issues/3333#issuecomment-573961938

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 13 Jan 2020 19:20:25 -0800

It seems that the text regarding the amplification limit ([Section 8.1](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-8.1-2)) has been last changed in response to #1863 which is marked as editorial.

While the intension of the change was to clarify that the amplification limit applies at the QUIC packet level (rather than UDP datagram level), I think @nibanks might be correct in pointing out that our understanding have been the contrary (see https://github.com/quicwg/base-drafts/issues/3333#issuecomment-573730981).

Consider the case where a client is using 0-RTT. When building the first datagram consisting of an Initial and a 0-RTT packet, the only way the client can build the datagram in one-pass is by first building the Initial packet, then the 0-RTT packet due to [the ordering requirement](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-12.2-3). As it is hard to tell the size of the 0-RTT packet before building it, the most likely outcome is that the 0-RTT packet would be padded. However, the server might not be able to process the 0-RTT packet, when it has lost it's resumption secret.

That means that if we are to say that the amplification limit is applied at the QUIC packet layer (for the QUIC packets that are only processed successfully), the client might end up in having very little room. I do not think that is a good outcome.

-- 
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/issues/3333#issuecomment-573980810

---

From: ekr <[email-masked]>
Date: Mon, 13 Jan 2020 20:07:54 -0800

I concur with Kazuho. My understanding had always been that the
amplification liit was on the UDP datagram, more or less for the reasons
Kazuho lays out here.

On Mon, Jan 13, 2020 at 7:20 PM Kazuho Oku <[email-masked]> wrote:

> It seems that the text regarding the amplification limit (Section 8.1
> <https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-8.1-2>)
> has been last changed in response to #1863
> <https://github.com/quicwg/base-drafts/issues/1863> which is marked as
> editorial.
>
> While the intension of the change was to clarify that the amplification
> limit applies at the QUIC packet level (rather than UDP datagram level), I
> think @nibanks <https://github.com/nibanks> might be correct in pointing
> out that our understanding have been the contrary (see #3333 (comment)
> <https://github.com/quicwg/base-drafts/issues/3333#issuecomment-573730981>
> ).
>
> Consider the case where a client is using 0-RTT. When building the first
> datagram consisting of an Initial and a 0-RTT packet, the only way the
> client can build the datagram in one-pass is by first building the Initial
> packet, then the 0-RTT packet due to the ordering requirement
> <https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-12.2-3>.
> As it is hard to tell the size of the 0-RTT packet before building it, the
> most likely outcome is that the 0-RTT packet would be padded. However, the
> server might not be able to process the 0-RTT packet, when it has lost it's
> resumption secret.
>
> That means that if we are to say that the amplification limit is applied
> at the QUIC packet layer (for the QUIC packets that are only processed
> successfully), the client might end up in having very little room. I do not
> think that is a good outcome.
>
> —
> <https://github.com/quicwg/base-drafts/issues/3333?email_source=notifications&email_token=AAIPLIMKHR6FXO4GKKJQILTQ5UVO7A5CNFSM4KFV7MQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI3EBCQ#issuecomment-573980810>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAIPLIKFTPEHZ7NZJLSXD4TQ5UVO7ANCNFSM4KFV7MQQ>
> .
>

-- 
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/issues/3333#issuecomment-573990386

---

From: Martin Thomson <[email-masked]>
Date: Tue, 14 Jan 2020 03:51:01 -0800

The CC implications are interesting for sure, but though amplification should be based on bytes received from an address prior to processing (so that discarded 0-RTT and junk counts): 

> In determining this limit, servers only count the size of successfully processed packets.

So #3340.

-- 
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/issues/3333#issuecomment-574138230

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 14 Jan 2020 06:42:40 -0800

@martinthomson Thank you for opening a new issue. I share the view that the issue deserves its own.

Going back to the original topic, I was a bit confused in my previous comment, but I think that my argument still holds; we should recommend padding inside QUIC packet, because CC happens at QUIC packet layer, and the size of padding affects CC. I am afraid that the specification would become needlessly complex, if we are to suggest that padding outside of QUIC packet is a possibility.

-- 
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/issues/3333#issuecomment-574205810

---

From: Andy Grover <[email-masked]>
Date: Thu, 16 Jan 2020 22:53:12 -0800

> I am afraid that the specification would become needlessly complex, if we are to suggest that padding outside of QUIC packet is a possibility.

Many other implementations didn't initially work against Neqo (which pads outside) and the Wireshark QUIC dissector does not currently handle it either. If the spec doesn't say anything then future implementations are bound to incorrectly handle outside padding. People are bound to assume it is not allowed if the spec says nothing, or just not consider the possibility of 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/issues/3333#issuecomment-575499163

---

From: Kazuho Oku <[email-masked]>
Date: Fri, 17 Jan 2020 03:36:16 -0800

My point is that while I'm not against making an editorial change stating that a receiver should process coalesced packets of a datagram until it sees a broken packet, I would not prefer adding a statement that implies that a client might pad outside of QUIC packets, because doing so is might have negative effects, and trying to resolve those negative effects are likely to introduce complexity.

To be clear, my understanding is that this issue is about a misbehaving client failing to talk to some servers. The specification says that an endpoint MUST expand the packet to 1,200 bytes by "padding to packets in the datagram as necessary," ([section 8.1.3](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-8.1-3)) which explicitly means that padding should happen at the packet level, not at the datagram level.

We might even argue that some servers not handling those broken datagram is a benefit, as it helps us find bugs in the client.

-- 
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/issues/3333#issuecomment-575591688

---

From: ekr <[email-masked]>
Date: Fri, 17 Jan 2020 10:46:33 -0800

Well, this is lawyering, but it doesn't say "by adding", but rather
"adding...as necessary". My argument here is that it's not necessary if you
pad in the datagram.

On Fri, Jan 17, 2020 at 3:36 AM Kazuho Oku <[email-masked]> wrote:

> My point is that while I'm not against making an editorial change stating
> that a receiver should process coalesced packets of a datagram until it
> sees a broken packet, I would not prefer adding a statement that implies
> that a client might pad outside of QUIC packets, because doing so is might
> have negative effects, and trying to resolve those negative effects are
> likely to introduce complexity.
>
> To be clear, my understanding is that this issue is about a misbehaving
> client failing to talk to some servers. The specification says that an
> endpoint MUST expand the packet to 1,200 bytes by "padding to packets in
> the datagram as necessary," (section 8.1.3
> <https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-8.1-3>)
> which explicitly means that padding should happen at the packet level, not
> at the datagram level.
>
> We might even argue that some servers not handling those broken datagram
> is a benefit, as it helps us find bugs in the client.
>
> —
> <https://github.com/quicwg/base-drafts/issues/3333?email_source=notifications&email_token=AAIPLIOSM5LNHTZ4JC3RV73Q6GJ2PA5CNFSM4KFV7MQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJHNKCA#issuecomment-575591688>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAIPLIP42EHA6Q3RX45QSHLQ6GJ2PANCNFSM4KFV7MQQ>
> .
>

-- 
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/issues/3333#issuecomment-575748146

---

From: Martin Thomson <[email-masked]>
Date: Sun, 19 Jan 2020 21:57:27 -0800

I have to admit that I found Kazuho's points about congestion control somewhat compelling.  However, I would say that there is nothing inherently wrong with the approach we take, except for the way that we count those junk bytes toward bytes-in-flight, and particularly how we remove them in the absence of a direct ACK.  But ACKs in Initial packets are worthless anyway, so counting them as though they were Initial probably goes most of the way to fixing the problem.  

Right now we don't account for these bytes at all, which avoids accounting errors, but might not be ideal in terms of congestion control.  INIT_CWND 10 becomes INIT_CWND "10 and a bit".

-- 
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/issues/3333#issuecomment-576118905

---

From: Christian Huitema <[email-masked]>
Date: Sun, 19 Jan 2020 23:17:56 -0800

I think it is much simpler to say that:

1) For the purpose of verifying that an Initial packet is larger than the required minimum, all bytes count.
2) For all other purposes, including congestion control and amplification control, only valid packets count.

Then it becomes a choice of implementations. Padding with padding frames is not hard, many implementations do it. Implementations that want to save a few lines of code or a few CPU cycles could pad their initial packets otherwise, but they will bear the impact on performance.

Also, consider the coalescing requirement about DCID: "Senders MUST NOT coalesce QUIC packets for different connections into a single UDP datagram. Receivers SHOULD ignore any subsequent packets with a different Destination Connection ID than the first packet in the datagram." That's consistent with "ignore the junk for all purposes except maybe verifying packet size".

-- 
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/issues/3333#issuecomment-576138925

---

From: Peter Wu <[email-masked]>
Date: Fri, 31 Jan 2020 08:36:37 -0800

Do note that appending zeroes in the UDP datagram makes it very easy to fingerprint QUIC connections for neqo and others that do the same.

Is the flexibility on using PADDING frames or appending garbage necessary to achieve the minimum datagram size necessary? It does not feel entirely right.

-- 
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/issues/3333#issuecomment-580808970