Immediately close with INVALID_TOKEN #3107

ianswett · 2019-10-17T02:40:58Z

If the Retry token is known to be invalid by the server, then the server can close the connection with INVALID_TOKEN instead of waiting for a timeout.

Came out of discussion of Issue #3014
Fixes #3168

Related to #3127

If the Retry token is known to be invalid by the server, then the server can close the connection with INVALID_TOKEN instead of waiting for a timeout.

nibanks · 2019-10-17T04:07:00Z

You need to update the "Transport Error Codes" section too.

ianswett · 2019-10-17T04:40:11Z

Thanks @nibanks I did that in a subsequent update. Refresh and tell me if it looks correct.

nibanks · 2019-10-17T15:08:30Z

@ianswett you're missing the section that gives some text to each error code #error-codes.

draft-ietf-quic-transport.md

martinthomson · 2019-10-22T00:51:47Z

draft-ietf-quic-transport.md

@@ -1641,6 +1641,14 @@ 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 a unverifiable 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


I don't think that we should allow the server to continue. Why not make the INVALID_TOKEN mandatory?

If we are to make this a mandatory requirement, under the assumption that the servers would act like that, should we also suggest that a server should issue tokens in a way that a Retry token and a NEW_TOKEN token are distinguishable from each other?

Unless we recommend servers building a token in that way, I do not think making this a MUST has benefits.

If we assume that servers only require Retries when they’re under attack, they might not bother checking the validity of a token when they’re not under attack.

A token SHOULD be constructed in a way that allows the server to distinguish it from tokens that are sent in Retry packets as they are carried in the same field.

-- Section 8.1.2

We could strengthen that to MUST if you like. It probably should be anyway.

The server could decide it's not under heavy load and proceed with the handshake, even if the token doesn't constitute address validation. ie: The token could expire on a shorter timeline than the session ticket/etc, so 0-RTT could succeed, but address validation would fail.

I'm persuaded by Marten's comment here (which I missed at first, I think because we commented at almost the same time).

I still think the requirement to make them distinguishable should be a MUST. Otherwise, a change of token generation method could make an old NEW_TOKEN token appear to be an invalid Retry token.

draft-ietf-quic-transport.md

MikeBishop

Depending how the server distinguishes them, a change of method / confusion about what server you're sending it to could make you appear to be presenting a Retry token instead of a regular token. This error code enables the client to recover and retry quickly, but there's no language about what the client should do when it receives the code.

MikeBishop · 2019-10-24T18:24:59Z

draft-ietf-quic-transport.md

@@ -1641,6 +1641,14 @@ 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 a unverifiable 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


I still think the requirement to make them distinguishable should be a MUST. Otherwise, a change of token generation method could make an old NEW_TOKEN token appear to be an invalid Retry token.

ianswett · 2019-10-24T18:48:46Z

@MikeBishop do you have a suggestion about what the client should do? Clearly it could start a new connection, but it would't want to do that an unlimited number of times, and it may depend upon whether a TCP fallback is available. Given that, it wasn't clear I should recommend something.

And I agree making them distinguishable should be a MUST, as stated in #3128

martinthomson · 2019-10-24T21:35:40Z

I think that the best thing we can say is "the connection attempt failed". The first version of the PR strongly suggested that making another connection attempt was the right thing to do, but that gets into lots of difficult questions about identifying why. Given that this is an error that only happens if a Retry is spoofed, then I think that all we can say is:

janaiyengar · 2019-11-01T07:23:36Z

draft-ietf-quic-transport.md

@@ -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?

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.

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

Let me retract my comment above. 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.

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?

#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.

kazuho

LGTM. I have one editorial suggestion.

kazuho · 2019-11-01T11:50:45Z

draft-ietf-quic-transport.md

@@ -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


Let me retract my comment above. 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.

About being able to unprotect the Initial packet

janaiyengar

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

draft-ietf-quic-transport.md

janaiyengar · 2019-11-01T22:14:59Z

draft-ietf-quic-transport.md

+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."

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.

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 ..."

By saying 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 less precise and therefor the text we have now is fine.

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

Co-Authored-By: Jana Iyengar <jri.ietf@gmail.com>

martinthomson

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.

draft-ietf-quic-transport.md

Co-Authored-By: Martin Thomson <mt@lowentropy.net>

kazuho

Rereading the changes, I have the following editorial suggestion.

kazuho · 2019-11-14T09:34:22Z

draft-ietf-quic-transport.md

+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.

marten-seemann · 2019-11-23T05:27:44Z

draft-ietf-quic-transport.md

@@ -1641,6 +1641,17 @@ 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 that can be unprotected but contains an
+invalid Retry token, it knows the client will not accept another Retry token.


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?

Note that the text explicitly says "Retry token", so I don't see how this is a problem.

Immediately close with INVALID_TOKEN

1a563f2

If the Retry token is known to be invalid by the server, then the server can close the connection with INVALID_TOKEN instead of waiting for a timeout.

ianswett added the -transport label Oct 17, 2019

ianswett added 2 commits October 16, 2019 22:43

Update draft-ietf-quic-transport.md

24d4b63

Update draft-ietf-quic-transport.md

8c46914

kazuho reviewed Oct 17, 2019

View reviewed changes

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved

martinthomson requested changes Oct 22, 2019

View reviewed changes

ianswett added 3 commits October 21, 2019 23:22

Kazuho and MTs comments

6cf5de8

Define the error

b43b5bf

whitespace

f827ca3

nibanks approved these changes Oct 22, 2019

View reviewed changes

MikeBishop reviewed Oct 24, 2019

View reviewed changes

martinthomson added the design An issue that affects the design of the protocol; resolution requires consensus. label Oct 29, 2019

janaiyengar reviewed Nov 1, 2019

View reviewed changes

kazuho approved these changes Nov 1, 2019

View reviewed changes

ianswett added 2 commits November 1, 2019 13:20

Kazuho's suggestion

9617e1d

About being able to unprotect the Initial packet

Update draft-ietf-quic-transport.md

97ed821

janaiyengar reviewed Nov 1, 2019

View reviewed changes

ianswett and others added 6 commits November 2, 2019 16:03

Update draft-ietf-quic-transport.md

85bac67

Co-Authored-By: Jana Iyengar <jri.ietf@gmail.com>

Update draft-ietf-quic-transport.md

70aef67

Co-Authored-By: Jana Iyengar <jri.ietf@gmail.com>

MAY close the connection without creating state

887c909

Jana's suggestion

c36760d

Update draft-ietf-quic-transport.md

b16c4cf

Update draft-ietf-quic-transport.md

3ab9f77

martinthomson approved these changes Nov 4, 2019

View reviewed changes

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved

Martin's suggestion

474291a

martinthomson approved these changes Nov 12, 2019

View reviewed changes

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved

ianswett and others added 2 commits November 11, 2019 21:43

Update draft-ietf-quic-transport.md

4cf3ff9

Co-Authored-By: Martin Thomson <mt@lowentropy.net>

Update draft-ietf-quic-transport.md

1396f07

kazuho approved these changes Nov 14, 2019

View reviewed changes

marten-seemann mentioned this pull request Nov 23, 2019

implement the INVALID_TOKEN error quic-go/quic-go#2237

Closed

marten-seemann suggested changes Nov 23, 2019

View reviewed changes

martinthomson merged commit 7d17a20 into master Dec 3, 2019

martinthomson deleted the ianswett-invalid-token branch December 3, 2019 03:10

dtikhonov mentioned this pull request Jan 27, 2020

Server cannot proceed after invalid Retry token #3396

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Immediately close with INVALID_TOKEN #3107

Immediately close with INVALID_TOKEN #3107

ianswett commented Oct 17, 2019 •

edited

Loading

nibanks commented Oct 17, 2019

ianswett commented Oct 17, 2019

nibanks commented Oct 17, 2019

martinthomson Oct 22, 2019

kazuho Oct 22, 2019

marten-seemann Oct 22, 2019

martinthomson Oct 22, 2019

ianswett Oct 22, 2019

martinthomson Oct 22, 2019

MikeBishop Oct 24, 2019

MikeBishop left a comment

MikeBishop Oct 24, 2019

ianswett commented Oct 24, 2019 •

edited

Loading

martinthomson commented Oct 24, 2019

janaiyengar Nov 1, 2019

kazuho Nov 1, 2019

mikkelfj Nov 1, 2019

kazuho Nov 1, 2019

mikkelfj Nov 1, 2019

ianswett Nov 1, 2019

kazuho left a comment

kazuho Nov 1, 2019

janaiyengar left a comment

janaiyengar Nov 1, 2019

kazuho Nov 1, 2019

janaiyengar Nov 2, 2019

kazuho Nov 2, 2019 •

edited

Loading

ianswett Nov 2, 2019

martinthomson left a comment

kazuho left a comment

kazuho Nov 14, 2019

marten-seemann Nov 23, 2019

martinthomson Nov 29, 2019

Immediately close with INVALID_TOKEN #3107

Immediately close with INVALID_TOKEN #3107

Conversation

ianswett commented Oct 17, 2019 • edited Loading

nibanks commented Oct 17, 2019

ianswett commented Oct 17, 2019

nibanks commented Oct 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeBishop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianswett commented Oct 24, 2019 • edited Loading

martinthomson commented Oct 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazuho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazuho Nov 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson left a comment

Choose a reason for hiding this comment

kazuho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianswett commented Oct 17, 2019 •

edited

Loading

ianswett commented Oct 24, 2019 •

edited

Loading

kazuho Nov 2, 2019 •

edited

Loading