Subject: Re: [quicwg/base-drafts] Immediately close with INVALID_TOKEN (#3107)

From: Jana Iyengar <[email-masked]>
Date: Fri, 01 Nov 2019 00:23:46 -0700

janaiyengar commented on this pull request.

> @@ -1641,6 +1641,13 @@ of connection establishment.  By giving the client a different connection ID to
 use, a server can cause the connection to be routed to a server instance with
 more resources available for new connections.
 
+If a server receives a client Initial with an invalid Retry token,
+it knows the client will not accept another Retry token.  It can either

I don't understand this sentence. How would the server know this for sure?

-- 
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/3107#pullrequestreview-310320520

---

From: Kazuho Oku <[email-masked]>
Date: Fri, 01 Nov 2019 01:03:35 -0700

kazuho commented on this pull request.

That's a keen observation (I think).

There is a chance where a Retry token is considered invalid due to packet becoming corrupt while being transmitted. A server can only say for sure that "the client will not accept another Retry token" unless either of the following conditions are met:
* The token is accompanied by a checksum (or an integrity protection). The checksum is correct, but the contents of the Retry token is invalid.
* The server successfully unprotects the Initial.

-- 
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/3107#discussion_r341481158

---

From: MikkelFJ <[email-masked]>
Date: Fri, 01 Nov 2019 01:33:18 -0700

mikkelfj commented on this pull request.

The client could also connect to an unexpected endpoint or the token could have expired.

-- 
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/3107#discussion_r341488027

---

From: Kazuho Oku <[email-masked]>
Date: Fri, 01 Nov 2019 04:51:31 -0700

kazuho approved this pull request.

LGTM. I have one editorial suggestion.

Let me retract my [comment above](https://github.com/quicwg/base-drafts/pull/3107/files#r341481158). We do not need to care about the packet becoming corrupt on the wire, because the prerequisite for responding with a CONNECTION_CLOSE is that the Initial that has been received can be unprotected.

To avoid confusion, maybe change "If a server receives a client Initial with an invalid Retry token" to "If a server receives a client Initial that can be unprotected but contains an invalid Retry token"?

> The client could also connect to an unexpected endpoint or the token could have expired.

I'd say that that's a failure in the server deployment. We do not discuss how that should be handled.

-- 
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/3107#pullrequestreview-310412618

---

From: MikkelFJ <[email-masked]>
Date: Fri, 01 Nov 2019 04:53:52 -0700

> I'd say that that's a failure in the server deployment. We do not discuss how that should be handled.

Since Retry is processed before handshake, why can't it just be the client sending to the wrong IP address?

-- 
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/3107#discussion_r341543509

---

From: ianswett <[email-masked]>
Date: Fri, 01 Nov 2019 10:20:27 -0700

ianswett commented on this pull request.

#3128 requires being able to distinguish between the two types of tokens.  I updated the text with Kazuho's suggestion about being able to unprotect the Initial.

-- 
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/3107#discussion_r341670635

---

From: ianswett <[email-masked]>
Date: Fri, 01 Nov 2019 10:21:02 -0700

@ianswett pushed 1 commit.

9617e1da29cda2a6230e7e59e9ae62a257087d73  Kazuho's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3107/files/f827ca30b70e4ccffcd5f94ce884b0c26584d37f..9617e1da29cda2a6230e7e59e9ae62a257087d73

---

From: ianswett <[email-masked]>
Date: Fri, 01 Nov 2019 12:25:02 -0700

97ed82152c5d9c0a40a87b7a324b2d5dc5c6909b  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3107/files/9617e1da29cda2a6230e7e59e9ae62a257087d73..97ed82152c5d9c0a40a87b7a324b2d5dc5c6909b

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 01 Nov 2019 15:16:05 -0700

Thanks for the changes, @kazuho's text suggestion answers my question. I have a couple more comments.

+If a server receives a client Initial that can be unprotected but contains an
+invalid Retry token, it knows the client will not accept another Retry token.
+It can either proceed with the handshake without verifying the token or
+immediately close ({{immediate-close}}) the connection with a connection

```suggestion
immediately close ({{immediate-close}}) the connection with an INVALID_TOKEN
```

+If a server receives a client Initial that can be unprotected but contains an
+invalid Retry token, it knows the client will not accept another Retry token.
+It can either proceed with the handshake without verifying the token or
+immediately close ({{immediate-close}}) the connection with a connection
+error of INVALID_TOKEN to cause the handshake to fail quickly instead of

```suggestion
error to cause the handshake to fail quickly instead of
```

+If a server receives a client Initial that can be unprotected but contains an
+invalid Retry token, it knows the client will not accept another Retry token.
+It can either proceed with the handshake without verifying the token or
+immediately close ({{immediate-close}}) the connection with a connection
+error of INVALID_TOKEN to cause the handshake to fail quickly instead of
+waiting for the client to timeout.

This suggests 2 options at the server, when there are 3: it can simply drop the packet and let the client time out. You may want to call out all three options. 

Suggestion: "If a server receives a client Initial that can be unprotected but contains an invalid Retry token, it knows the client will not accept another Retry token. The server can drop such a packet and allow the client to time out to detect loss of this connection, but that is a significant latency penalty to the client. If possible, the server SHOULD either immediately close the connection with an INVALID_TOKEN error or proceed with the handshake without verifying the token."

-- 
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/3107#pullrequestreview-310717275

---

From: Kazuho Oku <[email-masked]>
Date: Fri, 01 Nov 2019 16:42:35 -0700

I agree that listing all the three options would be an improvement.

At the same time, I have a concern regarding the suggestion of doing either an immediate close or proceeding the handshake. Those two options both require the server to retain state that cannot be delayed by providing a Retry to token to the client.

I think it we might want to downgrade the SHOULD to MAY, or, suggest that a server can send a CONNECTION_CLOSE frame (with INVALID_TOKEN) without retaining state.

-- 
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/3107#discussion_r341784351

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 01 Nov 2019 18:07:43 -0700

It should be possible to detect an invalid token without per-connection state, right? 

That said, I realized that I don't think we should recommend that a server that might be getting DoSsed SHOULD send out another packet in response. How about "A server that is able to maintain enough state MAY either ..."

-- 
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/3107#discussion_r341792585

---

From: Kazuho Oku <[email-masked]>
Date: Fri, 01 Nov 2019 22:27:29 -0700

By state I mean the closing and the draining state that gets built and retained once you invoke {immediate-close}. We should not recommend such behavior as it would create a DoS vector. But if it is not going to be a SHOULD but instead “can” or MAY, I think we can be precise and therefor the text we have now is fine.

-- 
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/3107#discussion_r341799807

---

From: ianswett <[email-masked]>
Date: Sat, 02 Nov 2019 13:03:17 -0700

85bac671b3282b7464a7b5b73a52ec5c34f2c62a  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3107/files/97ed82152c5d9c0a40a87b7a324b2d5dc5c6909b..85bac671b3282b7464a7b5b73a52ec5c34f2c62a

---

From: ianswett <[email-masked]>
Date: Sat, 02 Nov 2019 13:03:24 -0700

70aef67ee532cb6466e6bc1d485718782052de32  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3107/files/85bac671b3282b7464a7b5b73a52ec5c34f2c62a..70aef67ee532cb6466e6bc1d485718782052de32

---

From: ianswett <[email-masked]>
Date: Sat, 02 Nov 2019 13:13:58 -0700

887c909e025d6d1c3a1f634cf13b62937d51ea37  MAY close the connection without creating state

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3107/files/70aef67ee532cb6466e6bc1d485718782052de32..887c909e025d6d1c3a1f634cf13b62937d51ea37

---

From: ianswett <[email-masked]>
Date: Sat, 02 Nov 2019 13:14:49 -0700

I added a sentence about not creating any state in this case.  PTAL.

-- 
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/3107#discussion_r341824577

---

From: ianswett <[email-masked]>
Date: Sat, 02 Nov 2019 13:19:41 -0700

c36760da1391e5cf66b10b4d9d699c5fda3c8e88  Jana's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3107/files/887c909e025d6d1c3a1f634cf13b62937d51ea37..c36760da1391e5cf66b10b4d9d699c5fda3c8e88

---

From: ianswett <[email-masked]>
Date: Sat, 02 Nov 2019 13:20:27 -0700

b16c4cfb6295dab73b45b234b7e20a1caff13de3  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3107/files/c36760da1391e5cf66b10b4d9d699c5fda3c8e88..b16c4cfb6295dab73b45b234b7e20a1caff13de3

---

From: ianswett <[email-masked]>
Date: Sat, 02 Nov 2019 13:22:26 -0700

3ab9f77f170dd89a902f52b4669035c7fab87add  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3107/files/b16c4cfb6295dab73b45b234b7e20a1caff13de3..3ab9f77f170dd89a902f52b4669035c7fab87add

---

From: Martin Thomson <[email-masked]>
Date: Sun, 03 Nov 2019 16:06:49 -0800

martinthomson approved this pull request.

I found some of the wording to be awkward.  I'm not sure that my formulation is better, but you can integrate as you like.

> @@ -1641,12 +1641,14 @@ of connection establishment.  By giving the client a different connection ID to
 
-If a server receives a client Initial with an invalid Retry token,
-it knows the client will not accept another Retry token.  It can either
-proceed with the handshake without verifying the token or immediately close
-({{immediate-close}}) the connection with an connection error of
-INVALID_TOKEN to cause the handshake to fail quickly instead of waiting
-for the client to timeout.
+If a server receives a client Initial that can be unprotected but contains an
+invalid Retry token, it knows the client will not accept another Retry token.
+The server can drop such a packet and allow the client to time out to detect
+handshake failure, but that is a significant latency penalty to the client.

Maybe instead:

> The server could discard such a packet, but this could result in the client continuing to send the invalid Initial packet until it is eventually forced to abandon the connection attempt.  A server that requires address validation SHOULD immediately close ({{imm...}}) the connection with an INVALID_TOKEN error to allow the client to detect the error sooner.  Note that a server has not established any state for the connection at this point and so does not enter the closing period.  Alternatively, a server MAY proceed with the connection without verifying the token, though the server MUST NOT consider the client address to be valid.

-- 
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/3107#pullrequestreview-310850647

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 04 Nov 2019 17:29:17 -0800

I definitely like this text better. That said, I'm not sure that we should recommend sending a connection close. Esp. when under DoS, we do not want servers doing more work than necessary. 

-- 
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/3107#discussion_r342348480

---

From: Martin Thomson <[email-masked]>
Date: Mon, 04 Nov 2019 18:04:18 -0800

martinthomson commented on this pull request.

What does Roberto say again?  I think that if the server is under duress, then dropping the packet is fine.  This is a recommendation that allows the server to help the client, that's all.  Do you think we need to acknowledge the possibility that the server is overloaded and add the third 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/3107#discussion_r342354961

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 04 Nov 2019 18:20:10 -0800

The text seems to assume that an implementer will handle the third case, where I was leaning towards being explicit. Perhaps something like: "When under duress, the server could discard such a packet. Dropping such a packet could however result in ..." That makes it a bit more explicit.

-- 
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/3107#discussion_r342357675

---

From: Martin Thomson <[email-masked]>
Date: Mon, 11 Nov 2019 15:59:45 -0800

Seems reasonable to me.  @ianswett do you want to 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/3107#discussion_r342897546

---

From: ianswett <[email-masked]>
Date: Mon, 11 Nov 2019 17:40:40 -0800

474291a1da6b83e72105de556fa4ec4bd9fce39a  Martin's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3107/files/3ab9f77f170dd89a902f52b4669035c7fab87add..474291a1da6b83e72105de556fa4ec4bd9fce39a

---

From: ianswett <[email-masked]>
Date: Mon, 11 Nov 2019 17:43:57 -0800

I reworked Martin's suggestions, though I preferred to keep the sentence I had to his first sentence.  

In regards to "When under duress, the server could discard such a packet." I don't think that's adding anything on top of being clear about the consequences of discarding it, so I'd rather not add 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/3107#discussion_r344984278

---

From: Martin Thomson <[email-masked]>
Date: Mon, 11 Nov 2019 17:49:50 -0800

> @@ -1641,6 +1641,17 @@ of connection establishment.  By giving the client a different connection ID to
 
+If a server receives a client Initial that can be unprotected but contains an
+invalid Retry token, it knows the client will not accept another Retry token.
+The server can discard such a packet and allow the client to time out to
+detect handshake failure, but that is a significant latency penalty to the

```suggestion
detect handshake failure, but that could impose a significant latency penalty on 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/3107#pullrequestreview-315253813

---

From: ianswett <[email-masked]>
Date: Mon, 11 Nov 2019 18:43:33 -0800

4cf3ff91650a1731e37acd8dc50c4855acae0718  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3107/files/474291a1da6b83e72105de556fa4ec4bd9fce39a..4cf3ff91650a1731e37acd8dc50c4855acae0718

---

From: ianswett <[email-masked]>
Date: Mon, 11 Nov 2019 18:44:54 -0800

1396f0742e572a6740a05c26cc92cc9deda8799a  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3107/files/4cf3ff91650a1731e37acd8dc50c4855acae0718..1396f0742e572a6740a05c26cc92cc9deda8799a

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 14 Nov 2019 01:35:11 -0800

Rereading the changes, I have the following editorial suggestion.

+If a server receives a client Initial that can be unprotected but contains an
+invalid Retry token, it knows the client will not accept another Retry token.
+The server can discard such a packet and allow the client to time out to
+detect handshake failure, but that could impose a significant latency penalty on
+the client.  A server MAY proceed with the connection without verifying the
+token, though the server MUST NOT consider the client address validated.  If a
+server chooses not to proceed with the handshake, it SHOULD immediately close
+({{immediate-close}}) the connection with an INVALID_TOKEN error.  Note that a
+server has not established any state for the connection at this point and so
+does not enter the closing period.

Would it make sense to change the last two sentences to something like: _If a server chooses not to proceed with the handshake, it SHOULD immediately send a CONNECTION_CLOSE frame with an INVALID_TOKEN error without creating any state. As the server does not establish any state for the connection, it does not enter the closing period ({{immediate-close}})._

Current text is confusing as the normative text suggests following the rule in {{immediate-close}}, then argues *as if it is a fact* that a server would not be retaining any state, even though what we want to do here is to *recommend* the server to send CONNECTION_CLOSE in a stateless manner.

-- 
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/3107#pullrequestreview-316816546

---

From: Marten Seemann <[email-masked]>
Date: Fri, 22 Nov 2019 21:28:01 -0800

marten-seemann requested changes on this pull request.

This only applies if the token is a Retry token (and not a NEW_TOKEN token).
When the client uses a NEW_TOKEN token, it would exchange this token for the Retry token when it receives a Retry packet, wouldn't 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/3107#pullrequestreview-321895279

---

From: Martin Thomson <[email-masked]>
Date: Sun, 24 Nov 2019 22:30:18 -0800

Discarding is mentioned up front, so we don't need to concern ourselves with repeating 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/3107#discussion_r350007666

---

From: Martin Thomson <[email-masked]>
Date: Thu, 28 Nov 2019 17:02:46 -0800

Note that the text explicitly says "Retry token", so I don't see how this is a 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/3107#discussion_r351951286