Subject: Re: [quicwg/base-drafts] Address validation for connection migration (#732)

From: ianswett <[email-masked]>
Date: Mon, 02 Oct 2017 01:39:33 +0000 (UTC)

ianswett commented on this pull request.

A few comments, but this looks good overall.

-TODO: see issue #161
+An endpoint that see a new source IP address and port (or just a new source

see/sees

-TODO: see issue #161
+An endpoint that see a new source IP address and port (or just a new source
+port) on packets from its peer is likely seeing a connection migration at the
+peer.
+
+However, it is also possible that the peer is spoofing its source address in
+order to cause the endpoint to send excessive amounts of data to an unwilling
+host.  If the endpoint sends significantly more data than the peer, connection
+migration might be used to amplify the volume of data that an attacker can
+generate toward a victim.
+
+Thus, when seeing a new remote transport address, an endpoint MUST verify that
+its peer can receive and respond to packets at that new address.  By providing
+copies of the frames that it receives, the peer proves that it is receiving

nit: Technically, they're not copies of the frames.

> +of data and packets that it sends to its peer.  At a minimum, this needs to
+consider the possibility that packets are sent without congestion feedback.
+
+Once a connection is established, address validation is relatively simple (see
+{{address-validation}} for the process that is used during the handshake).  An
+endpoint validates a remote address by sending a PING frame containing a payload
+that is hard to guess.  This frame MUST be sent in a packet that is sent to the
+new address.  Once a PONG frame containing the same payload is received, the
+address is considered to be valid.  The PONG frame can use any path on its
+return.  A PING frame containing 12 randomly generated {{?RFC4086}} octets is
+sufficient to ensure that it is easier to receive the packet than it is to guess
+the value correctly.
+
+Note:
+
+: Retransmissions of the PING frame MUST also use the same remote address.

Should we be retransmitting the same PING frame or a different PING frame?

> +
+Once a connection is established, address validation is relatively simple (see
+{{address-validation}} for the process that is used during the handshake).  An
+endpoint validates a remote address by sending a PING frame containing a payload
+that is hard to guess.  This frame MUST be sent in a packet that is sent to the
+new address.  Once a PONG frame containing the same payload is received, the
+address is considered to be valid.  The PONG frame can use any path on its
+return.  A PING frame containing 12 randomly generated {{?RFC4086}} octets is
+sufficient to ensure that it is easier to receive the packet than it is to guess
+the value correctly.
+
+Note:
+
+: Retransmissions of the PING frame MUST also use the same remote address.
+
+If validation of the new remote address fails, after allowing enough time for

What if other packets continue to arrive from the old address?  Shouldn't the connection just stay on the old address? 

> +## Spurious Connection Migrations
+
+A connection migration could be triggered by an attacker that is able to capture
+and forward a packet such that it arrives before the legitimate copy of that
+packet.  Such a packet will appear to be a legitimate connection migration and
+the legitimate copy will be dropped as a duplicate.
+
+After a spurious migration, validation of the source address will fail because
+the entity at the source address does not have the necessary cryptographic keys
+to read or respond to the PING frame that is sent to it, even if it wanted to.
+Such a spurious connection migration could result in the connection being
+dropped when the source address validation fails.  This grants an attacker the
+ability to terminate the connection.
+
+Receipt of packets with higher packet numbers from the legitimate address will
+trigger another connection migration.  This will cause the validation of the

Thanks, this makes sense.  Possibly allude to it earlier in the text?

-- 
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/732#pullrequestreview-66353942

---

From: janaiyengar <[email-masked]>
Date: Fri, 13 Oct 2017 12:20:17 -0700

janaiyengar requested changes on this pull request.

This PR conflates a few things, which will make it harder to land the simpler things. I would simply add the frame type and rules around using PING/PONG in one PR, and then discuss address validation in a separate PR, since that will need quite a bit of discussion. I don't mind having that discussion on an issue. I don't think this PR is ready to be submitted.

> @@ -1351,6 +1355,14 @@ connection when they move networks.  This includes state that can be hard to
 recover such as outstanding requests, which might otherwise be lost with no easy
 way to retry them.
 
+An endpoint that receives packets that contain a source IP address and port that

We're definitely learning that having the ability to probe is quite useful even for connection migration. I've been working on redesigning connection migration, and our experience has shown that there needs to be more nuance around connection migration than simply switching away on the first packet. I don't want normative language here yet. I would suggest, for the sake of getting this PR in, to rephrase the MUST below to be non-normative ("should start sending", not capitalized.) We can then do an issue about what to do around connection migration separately, and figure out what should be normative.

Yes, agreed. We don't want to terminate the connection because a new path that we want to switch to doesn't work.

+
+Note:
+
+: Retransmissions of the PING frame MUST also use the same remote address.
+
+If validation of the new remote address fails, after allowing enough time for
+possible loss and recovery of packets carrying PING and PONG frames, the
+endpoint MUST terminate the connection.  When setting this timer,
+implementations are cautioned that the new path could have a longer round trip
+time than the original.  The endpoint MUST NOT send a CONNECTION_CLOSE frame in
+this case; it has to assume that the remote peer does not want to receive any
+more packets.
+
+If the remote address is validated successfully, the endpoint MAY increase the
+rate that it sends on the new path using the state from the previous path.  The

I would remove the entire paragraph here and simply replace with "If the remote address is validated successfully, the endpoint SHOULD reset the congestion controller to its initial state."

+After verifying an address, the endpoint SHOULD update any address validation
+tokens ({{address-validation}}) that it has issued to its peer if those are no
+longer valid based on the changed address.
+
+Address validation using the PING frame MAY be used at any time by either peer.
+For instance, an endpoint might check that a peer is still in possession of its
+address after a period of quiescence.
+
+Upon seeing a connection migration, an endpoint that sees a new address MUST
+abandon any address validation it is performing with other addresses on the
+expectation that the validation is likely to fail.  Abandoning address
+validation primarily means not closing the connection when a PONG frame is not
+received, but it could also mean ceasing retransmissions of the PING frame.  An
+endpoint that doesn't retransmit a PING frame might receive a PONG frame, which
+it MUST ignore.

This paragraph is unnecessary. Using PINGs on alt paths is fine, you don't want to start sending data on all of them. 

-The PING frame can be used to keep a connection alive when an application or
-application protocol wishes to prevent the connection from timing out.  An
+~~~
+ 0                   1                   2                   3
+ 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+| Pong Length(8)|              Pong Data (*)                  ...

Calling this data length and PING data avoids confusion.

> ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+~~~
+
+Pong Length:
+
+: This 8-bit value describes the length of the Pong Data field.
+
+Pong Data:
+
+: This variable-length field contains arbitrary data.
+
+The receiver of a PING frame with a Pong Data field that is empty simply needs
+to acknowledge the packet containing this frame.
+
+If the Pong Data is not empty, the recipient of this frame MUST generate a PONG
+frame ({{frame-pong}}) containing the same Pong Data.

It's easiest if the PING also generates an ack. Separately, the PONG packet will require an ack.

> @@ -1984,6 +2122,18 @@ Stream ID:
 Error Code:
 : The application-specified reason the sender is ignoring the stream.
 
+
+## PONG Frame {#frame-pong}
+
+The PONG frame (type=0x0d) is sent in response to a PING frame that contains
+data.  Its format is identical to the PING frame ({{frame-ping}}).
+
+An endpoint that receives an unsolicited PONG frame - that is, a PONG frame
+containing a payload that is either empty or not identical to the content of a
+PING frame that the endpoint previously sent - MUST generate a connection error
+of type UNSOLICITED_PONG.

A PONG frame needs to be acked.

-- 
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/732#pullrequestreview-69323047

---

From: janaiyengar <[email-masked]>
Date: Fri, 13 Oct 2017 19:20:47 +0000 (UTC)

BTW, thanks for all the text -- it's great to have this to kick off discussion.

-- 
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/732#issuecomment-336544542

---

From: Martin Thomson <[email-masked]>
Date: Mon, 16 Oct 2017 05:19:06 +0000 (UTC)

martinthomson commented on this pull request.

I'm not looking to improve connection migration here.  Just describe how I believe that is has to work within the current design.  If you want to add make-before-break connection migration, that's awesome, but it will definitely take more changes than this.  (It might build on this, even back out some of these changes.)  I'll split the PING/PONG change out though.

That's entirely too woolly for me.  Can we decide to remove the stronger language once we decide that a less-than-hard cutover is necessary?  I appreciate that there might be some value in testing a path before switching to it, for example, but lowercase "should" leaves a lot up to implementations to work out.

Can we give more concrete advice?  Would it be OK to say that the old path can be used until the probe works or times out?  We're basically conceding that there will be periods of multipath there, which is probably a good thing long-term, but I wanted to avoid that.

+
+Note:
+

That wasn't the conclusion of the discussion on #161.  FWIW, I would prefer that, but I am trying to capture the outcome of the discussion faithfully.

The logic here is such that if the old address continues to be used (and this is not just due to old packets being delayed, because it checks that the packet number increases) then the check on the new path is abandoned and the old path is OK.

 Error Code:
 
+

Correct.  That's the default.

I removed all qualifiers on the names.

+

How far up?

ICE sends a new random number, we should too.

It sounds like you have a design in mind.  Removing this paragraph breaks the guarantee from above.

I can't write about a design that I don't know about.  Are you describing a case where each endpoint is able to probe paths, but not cause the peer to use those paths until it decides that the path is OK?  (ICE does something similar in its make-before-break scenario, it's a reasonable model.)

If so, you need signals so that the poor peer isn't forced to guess which of these paths is the one that is preferred.  You also need some sort of system to ensure that both peers don't get different ideas about which path is "best".  Without that you get uncontrolled path oscillations.  We don't have either of those mechanisms right now, and the only signal we have is receipt of packets.  Unless we add the better design, this design at least works, even if it is suboptimal.  It's easy enough to back out or qualify a MUST or two once we have that better design complete.

-- 
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/732#pullrequestreview-69454094

---

From: Martin Thomson <[email-masked]>
Date: Sun, 15 Oct 2017 22:36:40 -0700

@martinthomson pushed 1 commit.

9d00167  Data field, not payload

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/732/files/ef3864a594dfe7b687828c41296d7d30351e77fe..9d00167b6ec1a5177d75b341dcc2e6d8fd8232a2

---

From: janaiyengar <[email-masked]>
Date: Mon, 16 Oct 2017 22:33:00 +0000 (UTC)

This PR makes assumptions about use that is more appropriate for an issue or a mailing list discussion. As it stands, I don't think this PR should be merged. Connection migration is not simple, and I'm happy to share our experiences with implementing it and deploying it, and I would definitely like to get the Apple folks to chime in as well with their MPTCP experience. This PR is not the right place for 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/732#pullrequestreview-69720425

---

From: janaiyengar <[email-masked]>
Date: Mon, 16 Oct 2017 22:45:06 +0000 (UTC)

janaiyengar commented on this pull request.

(What does "too woolly" mean?) We as a wg haven't decided anything about connection migration yet, so I don't think we should add any normative language until we agree on what is necessary. There's a bigger design issue question here, and I think this PR is unnecessarily rushing to resolve the design issue.

-- 
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/732#discussion_r144988759

---

From: Martin Thomson <[email-masked]>
Date: Tue, 17 Oct 2017 03:20:15 +0000 (UTC)

I agree, we don't know and we should discuss.  But this PR addresses problems with use of new paths that can't otherwise be addressed.  I agree with you that this isn't a great design, it's probably even terrible, but a good design will take time.  You aren't committing to this design though.  I would prefer to see this merged, while recognizing that it is bad in certain specific ways (I don't agree that this is so bad that the protocol will break, however).  I will even open the issue that explains the shortcoming of this design if we merge 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/732#discussion_r145019630

---

From: janaiyengar <[email-masked]>
Date: Tue, 17 Oct 2017 04:35:21 +0000 (UTC)

First off, and sorry for not saying this earlier, thanks for working on all this text -- a lot of this is helpful! I'll be fine with the comments I have inline. Mostly, I'll be fine if you remove the normative text about requiring switching to the new path, since this fundamentally prevents probing. The other stuff is good; I'd prefer less verbose text, but whatever. Happy to iterate after you get this in.

+
+Note:
+

This is about behavior after the path is validated ... conservative behavior would be to reset the congestion controller to initial state. I say SHOULD because there may be good reasons for using other state (such as state stored about the new network). I'm just suggesting a shortening that is adequate, not a different statement here. My suggestion here is to not say anything about what to do with NAT rebinding... if current practice is useful, I'm happy to help document what we observe in the wild. Otherwise, let's just recommend the conservative thing.

I don't think this is too bad, it's just one design that I don't think is adequate. Which is why I don't think there should be normative behavior here yet. I would like to build on this PR, and add normative behavior that _we all agree_ should be normative. At the moment I don't think what you're stating as normative is useful. You're requiring that an endpoint switch immediately, which is unnecessary.

FWIW, I've opened #880.

> @@ -1484,9 +1501,110 @@ number. "packet_number_secret" is derived from the TLS key exchange,
 as described in Section 5.6 of {{QUIC-TLS}}.

-### Address Validation for Migrated Connections
-
-TODO: see issue #161
+### Address Validation for Migrated Connections {#migrate-validate}

I think this section is useful, and does capture the resolution of #161.

-- 
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/732#pullrequestreview-69762241

---

From: Martin Thomson <[email-masked]>
Date: Tue, 17 Oct 2017 04:46:08 +0000 (UTC)

> Mostly, I'll be fine if you remove the normative text about requiring switching to the new path, since this fundamentally prevents probing. 

I don't think that it does.  It makes probing awkward, certainly, and if you probe then expect to receive a probe in return, but you can try out other paths if you want to test them out.  The problem I have with a non-immediate switch is that I don't know how to write an endpoint if you don't switch at some point.  Do you have a proposal for what we would tell the recipient of packets to do?  It doesn't have to be great (we all agree that we're going to nuke this design at the first opportunity), but I feel like we should say *something*.

-- 
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/732#issuecomment-337116542

---

From: janaiyengar <[email-masked]>
Date: Tue, 17 Oct 2017 05:00:53 +0000 (UTC)

Hm, yes, you're right. So, I do have articulated rules about when to switch, but it'll take a lot more to explain, and it is the "more detailed model" of switching. So, IIUC, in this simple case, client swithces and sends packet from new IP/port, server sees packet from new address and starts validation, during and after validation completes continues sending data to new IP/port. Is this correct? This is fine with me. (FWIW, this is exactly what GQUIC used to do/does right now, and we're working through its inadequacies.)

-- 
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/732#issuecomment-337118214

---

From: Martin Thomson <[email-masked]>
Date: Mon, 16 Oct 2017 22:06:49 -0700

I've removed the normative and made this more descriptive than prescriptive.  I hope that gives us the wiggle room we need.

BTW, I look forward to having the more detailed rules.  I hope that they are more comprehensible than RFC 5245 though :)

-- 
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/732#issuecomment-337118979

---

From: janaiyengar <[email-masked]>
Date: Tue, 17 Oct 2017 05:22:40 +0000 (UTC)

janaiyengar approved this pull request.

LGTM. Thanks for patiently working through this -- it's a great addition to the draft! I sure hope we can make the new text more legible than 5246 -- that's certainly much more complicated than connection migration, so if we aren't, we aren't doing a good enough job :-)

-- 
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/732#pullrequestreview-69768115

---

From: janaiyengar <[email-masked]>
Date: Tue, 17 Oct 2017 05:23:24 +0000 (UTC)

Ah! Sorry I read 5246, but you said 5245. Hah! Yes, indeed :-)

-- 
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/732#issuecomment-337121104

---

From: Martin Thomson <[email-masked]>
Date: Tue, 17 Oct 2017 05:23:33 +0000 (UTC)

Merged #732.

-- 
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/732#event-1296150641

---

From: Martin Thomson <[email-masked]>
Date: Tue, 17 Oct 2017 05:27:43 +0000 (UTC)

Heh, well, either one.  5246 is much, much better, but I think we might be able to improve on that some too.

BTW, if you were tempted to look at 5245 for clues, I would probably try the -bis document, which is nearing completion.

-- 
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/732#issuecomment-337121692

---

From: janaiyengar <[email-masked]>
Date: Mon, 16 Oct 2017 22:31:41 -0700

*shudder* I'll do 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/732#issuecomment-337122249