Subject: [quicwg/base-drafts] Apply the 3x amplification-limit to migration too (#4264)

From: Martin Thomson <[email-masked]>
Date: Tue, 27 Oct 2020 19:34:58 -0700

This touches a lot of places in the document, but I think that the changes are all manageable in scope.

The major change here is a move to a uniform 3x limit on sending
packets toward unvalidated addresses.  Specifically, this applies when
the packets are sent in response to another packet.  In practice, this
means that only servers are affected by this.

The consequence of this change is that PATH_CHALLENGE won&#39;t always be
padded, as previously agreed.  That also means that PATH_RESPONSE might
not be as well (if the client migrates, it can send PATH_CHALLENGE in
that packet; though we require that to be padded, we specifically don&#39;t
require enforcement, so...).  Address validation using this packet
works, but this doesn&#39;t also provide MTU validation.  To fix that, I
have recommended that a second validation be performed when the address
is valid, or there are enough bytes available that the
anti-amplification limit is not a barrier.

The nice part about this is that unifies the response.  We no longer
have weird carve-outs for rate limiting or minimum congestion window
(both of which I removed as part of this).

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

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

-- Commit Summary --

  * Add a blanket 3x anti-amplification limit
  * Some stuff I missed
  * MUST be consistent

-- File Changes --

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

-- Patch Links --

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

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 27 Oct 2020 19:53:52 -0700

@kazuho commented on this pull request.

> @@ -2206,9 +2212,17 @@ The endpoint MUST use unpredictable data in every PATH_CHALLENGE frame so that
 it can associate the peer's response with the corresponding PATH_CHALLENGE.
 
 An endpoint MUST expand datagrams that contain a PATH_CHALLENGE frame to at
-least the smallest allowed maximum datagram size of 1200 bytes.  Sending UDP
-datagrams of this size ensures that the network path from the endpoint to the
-peer can be used for QUIC; see {{datagram-size}}.
+least the smallest allowed maximum datagram size of 1200 bytes, unless the
+anti-amplification limit for the path does not permit sending a datagram of
+this size.  Sending UDP datagrams of this size ensures that the network path
+from the endpoint to the peer can be used for QUIC; see {{datagram-size}}.
+
+If an endpoint is unable to expand the datagram payload to 1200 bytes, no
+padding is necessary.  However, this means that the path MTU will not be
+validated.  The endpoint MUST perform a second path validation with a datagram
+of at least 1200 bytes once a PATH_RESPONSE is successfully received or when
+enough bytes have been received on the path that sending the larger datagram
+will not result in exceeding the anti-amplification limit.

I think we can change this MUST to a "can", pointing out that the most likely reason for sending a small-sized PATH_CHALLENGE is due to receiving a non-probing packet on an unvalidated path due to unintentional migration. And that in such case, it is unlikely to have an alternative path.

I think that's what @ianswett pointed out.

> @@ -2241,6 +2258,12 @@ the data that was sent in a previous PATH_CHALLENGE frame.  A PATH_RESPONSE
 frame received on any network path validates the path on which the
 PATH_CHALLENGE was sent.
 
+If the PATH_CHALLENGE frame was sent in a datagram that was not expanded to at
+least 1200 bytes, an endpoint can regard the address as valid for the purposes
+of sending more than three times the amount of data that has been received.
+However, the path MTU is not validated so the endpoint MUST initiate another
+path validation to verify that the path MTU is sufficient.

If we are to change the MUST above (see my comment above) to "can," then I think we can get rid of this paragraph.

> @@ -2227,6 +2241,9 @@ enable an attack on migration; see {{off-path-forward}}.
 An endpoint MUST expand datagrams that contain a PATH_RESPONSE frame to at
 least the smallest allowed maximum datagram size of 1200 bytes. This verifies
 that the path is able to carry datagrams of this size in both directions.
+However, an endpoint MUST NOT expand the PATH_RESPONSE if it is constrained
+by an anti-amplification limit.  This will only occur if the PATH_CHALLENGE
+is not send in an expanded packet.

I like this, because this requirement is in symmetry with that being proposed for PATH_CHALLENGE.

I had [something different](https://mailarchive.ietf.org/arch/msg/quic/hUck4IjyiT0LNkKPy-gPsPeR_XA/) in my mind, but I prefer what this PR states.

-- 
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/4264#pullrequestreview-518287636

---

From: Eric Kinnear <[email-masked]>
Date: Tue, 27 Oct 2020 20:25:17 -0700

@ekinnear approved this pull request.

A few minor nits, but this does a really good job walking a very fine line of not-much-change but actually making things a bit _more_ consistent. Thanks @martinthomson!

> @@ -2126,8 +2132,8 @@ Tokens sent in NEW_TOKEN frames MUST include information that allows the server
 to verify that the client IP address has not changed from when the token was
 issued. Servers can use tokens from NEW_TOKEN in deciding not to send a Retry
 packet, even if the client address has changed. If the client IP address has
-changed, the server MUST adhere to the anti-amplification limits found in
-{{validate-handshake}}.  Note that in the presence of NAT, this requirement
+changed, the server MUST adhere to the anti-amplification limits; see

Now that we've given it a real name
```suggestion
changed, the server MUST adhere to the anti-amplification limit; see
```

It also seems like it's possible that we wouldn't be able to send a PATH_CHALLENGE at all, what should an endpoint do then?

Just a wording nit: From context it's clear that this is a qualified MUST, but perhaps we could include that in the sentence itself? Even something like "In this situation, the endpoint MUST" such that if someone were to quote this sentence it would be clear that there's more context that applies.

This is very nice :) 
One minor editorial nit though: 
```suggestion
is not sent in an expanded packet.
```
or
```suggestion
was not sent in an expanded packet.
```

> @@ -2400,15 +2423,11 @@ used to amplify the volume of data that an attacker can generate toward a
 victim.
 
 As described in {{migration-response}}, an endpoint is required to validate a
-peer's new address to confirm the peer's possession of the new address.  Until a
-peer's address is deemed valid, an endpoint MUST limit the rate at which it
-sends data to this address.  The endpoint MUST NOT send more than a minimum
-congestion window's worth of data per estimated round-trip time (kMinimumWindow,
-as defined in {{QUIC-RECOVERY}}).  In the absence of this limit, an endpoint
-risks being used for a denial of service attack against an unsuspecting victim.
-Note that since the endpoint will not have any round-trip time measurements to
-this address, the estimate SHOULD be the default initial value; see
-{{QUIC-RECOVERY}}.
+peer's new address to confirm the peer's possession of the new address. Until a
+peer's address is deemed valid, an endpoint limits the amount of data it sends
+to that address; see {{address-validation}}. In the absence of this limit, an
+endpoint risks being used for a denial of service attack against an
+unsuspecting victim.

This does introduce some interesting questions around NAT rebinding if the "new address" was only sending very small ACK-only packets, but that should only persist for the single RTT it takes to get the PATH_RESPONSE back. Probably fine to leave the wording here as-is and not call that out, but thought I'd double check.

-- 
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/4264#pullrequestreview-518295563

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 27 Oct 2020 20:30:00 -0700

I'm not sure if there'd be such a case.

* Minimum size of a valid short header packet is 19 bytes (i.e. first-byte (1) + 0-byte CID (0) + 1-byte PN (1) + 1-byte payload (1) + AEAD tag (16)).
* Maximum size of a short header packet that contains PATH_CHALLENGE only is 50 bytes (i.e. first byte (1) + 20-byte CID (20) + 4-byte PN (4) + PATH_CHALLENGE (9) + AEAD tag (16)).

The latter (50) is less than the former multiplied by three (19*3=57).

-- 
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/4264#discussion_r513158582

---

From: Martin Thomson <[email-masked]>
Date: Tue, 27 Oct 2020 23:58:02 -0700

@martinthomson pushed 1 commit.

59f5a3a97613f8e5eaf41018a294cdcdebbdc3c0  Small corrections

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4264/files/97ba8a6cc37b9149f02ce35c7eabc1f0af461059..59f5a3a97613f8e5eaf41018a294cdcdebbdc3c0

---

From: Martin Thomson <[email-masked]>
Date: Wed, 28 Oct 2020 00:01:24 -0700

@martinthomson commented on this pull request.

I would prefer not to.  We decided in the other change (the one that lead to this bug) not to make MTU validation optional.  If we care whether the path supports the MTU, then this requirement follows from that.

Yeah, you should always have space for a challenge once you multiple out overheads, even if you have to spend 20 bytes more than your peer on a connection ID.

 victim.

Right, I don't think that this is worth calling out :)

-- 
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/4264#discussion_r513215885

---

From: Martin Thomson <[email-masked]>
Date: Wed, 28 Oct 2020 00:02:27 -0700

2df832a7d1953af100084436d86d4ec8ec1c31f7  Reword the second validation requirement slightly

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4264/files/59f5a3a97613f8e5eaf41018a294cdcdebbdc3c0..2df832a7d1953af100084436d86d4ec8ec1c31f7

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 28 Oct 2020 00:18:36 -0700

I might argue that the principle that we have is to reject paths with MTUs below 1200. But in case of NAT rebinding, the path has already switched. It's too late to reject that path.

But to be practical, I think we need to consider how endpoints should behave when MTU of the new path is below 1200. The two options at stake are:
* At the moment, the PR states that path validation MUST fail. But because the server might send smaller packets using the reminder of the 3x window, communication between the endpoints might slowly proceed.
* If we omit MTU discovery, communication between the endpoints is likely to slowly proceed in similar way.

IMO, both options are equally dissatisfatory, and based on that view, my preference goes to the simpler solution (of omitting MTU discovery).

By mandating MTU discovery, do we have a better way of bailing out?

-- 
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/4264#discussion_r513223950

---

From: Martin Thomson <[email-masked]>
Date: Wed, 28 Oct 2020 01:47:42 -0700

When we discussed this as part of the handshake, the principle we applied was a preference for failing fast.  That is, the sooner the problem is identified and the connection abandoned, the less likely it is that the connection persists in a half-broken state for an extended period.

Isn't a mandate for MTU probing a clear application of that principle?

(Nothing stops someone from trying to muddle on, but you need to be smart and effectively do a sub-1200 PMTUD of sorts to find your limits; and you have to rely on the peer doing the same.  That seems fragile.)

-- 
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/4264#discussion_r513268904

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 28 Oct 2020 03:38:00 -0700

Thank you for walking me through. So you are saying that an endpoint MUST always validate path MTU when responding to an unintentional migration (e.g., NAT rebinding), using a timer as recommended in [section 8.2.4](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#name-failed-path-validation). When the timer expires, and if there's no other active paths, the endpoint closes the connection.

As complicated as it is, I agree that there's benefit in closing early, and that the design is consistent.

The only remaining question I have is the error code to be used when closing the connection. Should we be using NO_ERROR, or do we need a new error code?

-- 
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/4264#discussion_r513340007

---

From: ianswett <[email-masked]>
Date: Wed, 28 Oct 2020 13:11:18 -0700

@ianswett approved this pull request.

I think this LG.  I left a few comments about using the word 'data' in this context, because we're really talking about bytes of UDP payload, and a reader could believe data is application/STREAM data.

> +that responds to packets received from a new address limits the data it sends
+to that address until the peer address is validated.  Prior to validating the
+peer's address, endpoints MUST NOT send data toward that address that exceeds
+three times the amount of data received from that address.  This three times

```suggestion
that responds to packets received from a new address limits the packets it sends
to that address until the peer address is validated.  Prior to validating the
peer's address, endpoints MUST NOT send packets toward that address that exceeds
three times the amount of data received from that address.  This three times
```

> @@ -2672,9 +2693,9 @@ might also occur because the client's access network used a different NAT
 binding for the server's preferred address.
 
 Servers SHOULD initiate path validation to the client's new address upon
-receiving a probe packet from a different address.  Servers MUST NOT send more
-than a minimum congestion window's worth of non-probing packets to the new
-address before path validation is complete.
+receiving a probe packet from a different address.  Before the client address
+is validated, servers MUST NOT send more data on the path than three times the

```suggestion
is validated, servers MUST NOT send more packets on the path than three times the
```

-- 
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/4264#pullrequestreview-519030887

---

From: Martin Thomson <[email-masked]>
Date: Wed, 28 Oct 2020 21:03:01 -0700

@ianswett, I was intentionally avoiding "packets" here as the amplification inputs is the size of the UDP payload, and the rules here also use that.  Would "bytes" or "bytes of UDP payload" be better?

-- 
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/4264#issuecomment-718345375

---

From: Martin Thomson <[email-masked]>
Date: Wed, 28 Oct 2020 23:47:05 -0700

I guess what distinguishes this from other cases is that there is an opportunity to signal an error.

However, in general we have avoided signaling this, because [losing a path doesn't mean losing the connection](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-8.2.4-6).  

I think that this is an error case where NO_ERROR almost makes sense.  Though I wouldn't be opposed to defining a NO_VIABLE_PATH error for this specific case.

-- 
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/4264#discussion_r514029543

---

From: Mike Bishop <[email-masked]>
Date: Thu, 29 Oct 2020 11:19:34 -0700

@MikeBishop commented on this pull request.

Or datagrams, perhaps?  Also "packets ... that exceeds" doesn't agree.

-- 
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/4264#discussion_r514472023

---

From: Mike Bishop <[email-masked]>
Date: Thu, 29 Oct 2020 11:32:25 -0700

@MikeBishop approved this pull request.

+by an anti-amplification limit.  This will only occur if the PATH_CHALLENGE
+was not sent in an expanded packet.

```suggestion
However, an endpoint MUST NOT expand the datagram containing the
PATH_RESPONSE if it is constrained by an anti-amplification limit.  This
will only occur if the PATH_CHALLENGE was not sent in an expanded
datagram.
```
The first sentence says you expand the datagram, so let's not confuse the issue by referring to expanded frames or packets.

```suggestion
that responds to packets received from a new address limits the data it sends
to that address until the peer address is validated.  Prior to validating the
peer's address, endpoints MUST NOT send datagrams toward that address
whose total payload exceeds three times the amount of data received from
that address.  This
```
Saying "three times" again both sounds strange and doesn't seem to add anything.

-- 
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/4264#pullrequestreview-519958208

---

From: ianswett <[email-masked]>
Date: Thu, 29 Oct 2020 12:54:10 -0700

"bytes of UDP payload" would be the most accurate, but "bytes" also works for me.

-- 
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/4264#issuecomment-718984500

---

From: Martin Thomson <[email-masked]>
Date: Thu, 29 Oct 2020 16:57:50 -0700

549564f9c325fae5567e1b1bcccf3e72883282a6  Make it clearer

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4264/files/2df832a7d1953af100084436d86d4ec8ec1c31f7..549564f9c325fae5567e1b1bcccf3e72883282a6

---

From: Martin Thomson <[email-masked]>
Date: Thu, 29 Oct 2020 16:58:58 -0700

8c0c1f9fc9352eb96ca64f333574d5e14d91f4dd  Wrap stuff

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4264/files/549564f9c325fae5567e1b1bcccf3e72883282a6..8c0c1f9fc9352eb96ca64f333574d5e14d91f4dd

---

From: Nick Banks <[email-masked]>
Date: Thu, 29 Oct 2020 17:20:49 -0700

@nibanks 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/4264#pullrequestreview-520203814

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 29 Oct 2020 17:26:12 -0700

NO_VIABLE_PATH sounds good to me.

This specific case is not about losing one path. It is about losing a connection amid data transfer. That's going to have negative consequences, and I think it would be great if we could have insights regarding if that actually happens.

Depending on how often we would see this, we might start considering changing minimum MTU requirement to a smaller number, or remove the path validation requirement upon NAT rebinding.

-- 
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/4264#discussion_r514640819

---

From: Magnus Westerlund <[email-masked]>
Date: Fri, 30 Oct 2020 01:56:52 -0700

@gloinul 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/4264#pullrequestreview-520475556

---

From: martinduke <[email-masked]>
Date: Sat, 31 Oct 2020 13:49:24 -0700

@martinduke requested changes on this pull request.

Based on the argument in this email: https://mailarchive.ietf.org/arch/msg/quic/4JDtL1vUoRj_R1c6zVhLasbAg8g/

I would propose the following text. I'm not sure where it should go:

1) "After a long idle period, clients SHOULD send at least one, and ideally at least three, full size datagrams in its first transmission in case a NAT rebinding forces the server to validate the path and MTU."

2) "When migrating to a new address on a path that has not already been validated by both endpoints, a client SHOULD send at least one, and ideally at least three, full size datagrams to allow fast MTU validation and use of the entire congestion window."

If clients follow this advice, it eliminates all of the performance downsides of the amplification limit.

-- 
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/4264#pullrequestreview-521168827